2020-09-30 08:33:23

by Wenbin Mei (梅文彬)

[permalink] [raw]
Subject: [PATCH v3 4/4] mmc: mediatek: Add subsys clock control for MT8192 msdc

MT8192 msdc is an independent sub system, we need control more bus
clocks for it.
Add support for the additional subsys clocks to allow it to be
configured appropriately.

Signed-off-by: Wenbin Mei <[email protected]>
---
drivers/mmc/host/mtk-sd.c | 77 ++++++++++++++++++++++++++++++---------
1 file changed, 59 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index a704745e5882..9a1422955593 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -425,6 +425,8 @@ struct msdc_host {
struct clk *h_clk; /* msdc h_clk */
struct clk *bus_clk; /* bus clock which used to access register */
struct clk *src_clk_cg; /* msdc source clock control gate */
+ struct clk *sys_clk_cg; /* msdc subsys clock control gate */
+ struct clk_bulk_data bulk_clks[3]; /* pclk, axi, ahb clock control gate */
u32 mclk; /* mmc subsystem clock frequency */
u32 src_clk_freq; /* source clock frequency */
unsigned char timing;
@@ -784,6 +786,8 @@ static void msdc_set_busy_timeout(struct msdc_host *host, u64 ns, u64 clks)

static void msdc_gate_clock(struct msdc_host *host)
{
+ clk_bulk_disable_unprepare(ARRAY_SIZE(host->bulk_clks),
+ host->bulk_clks);
clk_disable_unprepare(host->src_clk_cg);
clk_disable_unprepare(host->src_clk);
clk_disable_unprepare(host->bus_clk);
@@ -792,10 +796,17 @@ static void msdc_gate_clock(struct msdc_host *host)

static void msdc_ungate_clock(struct msdc_host *host)
{
+ int ret;
+
clk_prepare_enable(host->h_clk);
clk_prepare_enable(host->bus_clk);
clk_prepare_enable(host->src_clk);
clk_prepare_enable(host->src_clk_cg);
+ ret = clk_bulk_prepare_enable(ARRAY_SIZE(host->bulk_clks),
+ host->bulk_clks);
+ if (ret)
+ dev_dbg(host->dev, "enable clks failed!\n");
+
while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
cpu_relax();
}
@@ -2366,6 +2377,52 @@ static void msdc_of_property_parse(struct platform_device *pdev,
host->cqhci = false;
}

+static int msdc_of_clock_parse(struct platform_device *pdev,
+ struct msdc_host *host)
+{
+ struct clk *clk;
+
+ host->src_clk = devm_clk_get(&pdev->dev, "source");
+ if (IS_ERR(host->src_clk))
+ return PTR_ERR(host->src_clk);
+
+ host->h_clk = devm_clk_get(&pdev->dev, "hclk");
+ if (IS_ERR(host->h_clk))
+ return PTR_ERR(host->h_clk);
+
+ host->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
+ if (IS_ERR(host->bus_clk))
+ host->bus_clk = NULL;
+
+ /*source clock control gate is optional clock*/
+ host->src_clk_cg = devm_clk_get(&pdev->dev, "source_cg");
+ if (IS_ERR(host->src_clk_cg))
+ host->src_clk_cg = NULL;
+
+ host->sys_clk_cg = devm_clk_get(&pdev->dev, "sys_cg");
+ if (IS_ERR(host->sys_clk_cg))
+ host->sys_clk_cg = NULL;
+ else
+ clk_prepare_enable(host->sys_clk_cg);
+
+ clk = devm_clk_get(&pdev->dev, "pclk_cg");
+ if (IS_ERR(clk))
+ clk = NULL;
+ host->bulk_clks[0].clk = clk;
+
+ clk = devm_clk_get(&pdev->dev, "axi_cg");
+ if (IS_ERR(clk))
+ clk = NULL;
+ host->bulk_clks[1].clk = clk;
+
+ clk = devm_clk_get(&pdev->dev, "ahb_cg");
+ if (IS_ERR(clk))
+ clk = NULL;
+ host->bulk_clks[2].clk = clk;
+
+ return 0;
+}
+
static int msdc_drv_probe(struct platform_device *pdev)
{
struct mmc_host *mmc;
@@ -2405,25 +2462,9 @@ static int msdc_drv_probe(struct platform_device *pdev)
if (ret)
goto host_free;

- host->src_clk = devm_clk_get(&pdev->dev, "source");
- if (IS_ERR(host->src_clk)) {
- ret = PTR_ERR(host->src_clk);
- goto host_free;
- }
-
- host->h_clk = devm_clk_get(&pdev->dev, "hclk");
- if (IS_ERR(host->h_clk)) {
- ret = PTR_ERR(host->h_clk);
+ ret = msdc_of_clock_parse(pdev, host);
+ if (ret)
goto host_free;
- }
-
- host->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
- if (IS_ERR(host->bus_clk))
- host->bus_clk = NULL;
- /*source clock control gate is optional clock*/
- host->src_clk_cg = devm_clk_get(&pdev->dev, "source_cg");
- if (IS_ERR(host->src_clk_cg))
- host->src_clk_cg = NULL;

host->reset = devm_reset_control_get_optional_exclusive(&pdev->dev,
"hrst");
--
2.18.0


2020-10-01 06:16:00

by Nicolas Boichat

[permalink] [raw]
Subject: Re: [PATCH v3 4/4] mmc: mediatek: Add subsys clock control for MT8192 msdc

On Wed, Sep 30, 2020 at 4:31 PM Wenbin Mei <[email protected]> wrote:
>
> MT8192 msdc is an independent sub system, we need control more bus
> clocks for it.
> Add support for the additional subsys clocks to allow it to be
> configured appropriately.
>
> Signed-off-by: Wenbin Mei <[email protected]>
> ---
> drivers/mmc/host/mtk-sd.c | 77 ++++++++++++++++++++++++++++++---------
> 1 file changed, 59 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index a704745e5882..9a1422955593 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -425,6 +425,8 @@ struct msdc_host {
> struct clk *h_clk; /* msdc h_clk */
> struct clk *bus_clk; /* bus clock which used to access register */
> struct clk *src_clk_cg; /* msdc source clock control gate */
> + struct clk *sys_clk_cg; /* msdc subsys clock control gate */
> + struct clk_bulk_data bulk_clks[3]; /* pclk, axi, ahb clock control gate */
> u32 mclk; /* mmc subsystem clock frequency */
> u32 src_clk_freq; /* source clock frequency */
> unsigned char timing;
> @@ -784,6 +786,8 @@ static void msdc_set_busy_timeout(struct msdc_host *host, u64 ns, u64 clks)
>
> static void msdc_gate_clock(struct msdc_host *host)
> {
> + clk_bulk_disable_unprepare(ARRAY_SIZE(host->bulk_clks),
> + host->bulk_clks);
> clk_disable_unprepare(host->src_clk_cg);
> clk_disable_unprepare(host->src_clk);
> clk_disable_unprepare(host->bus_clk);
> @@ -792,10 +796,17 @@ static void msdc_gate_clock(struct msdc_host *host)
>
> static void msdc_ungate_clock(struct msdc_host *host)
> {
> + int ret;
> +
> clk_prepare_enable(host->h_clk);
> clk_prepare_enable(host->bus_clk);
> clk_prepare_enable(host->src_clk);
> clk_prepare_enable(host->src_clk_cg);
> + ret = clk_bulk_prepare_enable(ARRAY_SIZE(host->bulk_clks),
> + host->bulk_clks);
> + if (ret)
> + dev_dbg(host->dev, "enable clks failed!\n");

dev_err looks a lot more appropriate. Also, don't you want to exit the
function in that case, rather than going to the while loop below where
you may get stuck?

> +
> while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
> cpu_relax();
> }
> @@ -2366,6 +2377,52 @@ static void msdc_of_property_parse(struct platform_device *pdev,
> host->cqhci = false;
> }
>
> +static int msdc_of_clock_parse(struct platform_device *pdev,
> + struct msdc_host *host)
> +{
> + struct clk *clk;
> +
> + host->src_clk = devm_clk_get(&pdev->dev, "source");
> + if (IS_ERR(host->src_clk))
> + return PTR_ERR(host->src_clk);
> +
> + host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> + if (IS_ERR(host->h_clk))
> + return PTR_ERR(host->h_clk);
> +
> + host->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
> + if (IS_ERR(host->bus_clk))
> + host->bus_clk = NULL;

Use devm_clk_get_optional instead (ditto for the next 2).

> +
> + /*source clock control gate is optional clock*/
> + host->src_clk_cg = devm_clk_get(&pdev->dev, "source_cg");
> + if (IS_ERR(host->src_clk_cg))
> + host->src_clk_cg = NULL;
> +
> + host->sys_clk_cg = devm_clk_get(&pdev->dev, "sys_cg");
> + if (IS_ERR(host->sys_clk_cg))
> + host->sys_clk_cg = NULL;
> + else
> + clk_prepare_enable(host->sys_clk_cg);

This doesn't need to be in an else branch, calling clk_prepare_enable
on a NULL clock is fine.

However, is it expected that this clock is turned on forever after
probe?! At the very least, the clock should be disabled in
msdc_drv_remove, but, really, I think it should be enabled as needed,
like the other clocks, in msdc_gate_clock?

> +
> + clk = devm_clk_get(&pdev->dev, "pclk_cg");
> + if (IS_ERR(clk))
> + clk = NULL;
> + host->bulk_clks[0].clk = clk;
> +
> + clk = devm_clk_get(&pdev->dev, "axi_cg");
> + if (IS_ERR(clk))
> + clk = NULL;
> + host->bulk_clks[1].clk = clk;
> +
> + clk = devm_clk_get(&pdev->dev, "ahb_cg");
> + if (IS_ERR(clk))
> + clk = NULL;
> + host->bulk_clks[2].clk = clk;

Use devm_clk_bulk_get_optional for these 3.

> +
> + return 0;
> +}
> +
> static int msdc_drv_probe(struct platform_device *pdev)
> {
> struct mmc_host *mmc;
> @@ -2405,25 +2462,9 @@ static int msdc_drv_probe(struct platform_device *pdev)
> if (ret)
> goto host_free;
>
> - host->src_clk = devm_clk_get(&pdev->dev, "source");
> - if (IS_ERR(host->src_clk)) {
> - ret = PTR_ERR(host->src_clk);
> - goto host_free;
> - }
> -
> - host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> - if (IS_ERR(host->h_clk)) {
> - ret = PTR_ERR(host->h_clk);
> + ret = msdc_of_clock_parse(pdev, host);
> + if (ret)
> goto host_free;
> - }
> -
> - host->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
> - if (IS_ERR(host->bus_clk))
> - host->bus_clk = NULL;
> - /*source clock control gate is optional clock*/
> - host->src_clk_cg = devm_clk_get(&pdev->dev, "source_cg");
> - if (IS_ERR(host->src_clk_cg))
> - host->src_clk_cg = NULL;
>
> host->reset = devm_reset_control_get_optional_exclusive(&pdev->dev,
> "hrst");
> --
> 2.18.0

2020-10-10 10:16:12

by Wenbin Mei (梅文彬)

[permalink] [raw]
Subject: Re: [PATCH v3 4/4] mmc: mediatek: Add subsys clock control for MT8192 msdc

On Thu, 2020-10-01 at 14:14 +0800, Nicolas Boichat wrote:
> On Wed, Sep 30, 2020 at 4:31 PM Wenbin Mei <[email protected]> wrote:
> >
> > MT8192 msdc is an independent sub system, we need control more bus
> > clocks for it.
> > Add support for the additional subsys clocks to allow it to be
> > configured appropriately.
> >
> > Signed-off-by: Wenbin Mei <[email protected]>
> > ---
> > drivers/mmc/host/mtk-sd.c | 77 ++++++++++++++++++++++++++++++---------
> > 1 file changed, 59 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index a704745e5882..9a1422955593 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -425,6 +425,8 @@ struct msdc_host {
> > struct clk *h_clk; /* msdc h_clk */
> > struct clk *bus_clk; /* bus clock which used to access register */
> > struct clk *src_clk_cg; /* msdc source clock control gate */
> > + struct clk *sys_clk_cg; /* msdc subsys clock control gate */
> > + struct clk_bulk_data bulk_clks[3]; /* pclk, axi, ahb clock control gate */
> > u32 mclk; /* mmc subsystem clock frequency */
> > u32 src_clk_freq; /* source clock frequency */
> > unsigned char timing;
> > @@ -784,6 +786,8 @@ static void msdc_set_busy_timeout(struct msdc_host *host, u64 ns, u64 clks)
> >
> > static void msdc_gate_clock(struct msdc_host *host)
> > {
> > + clk_bulk_disable_unprepare(ARRAY_SIZE(host->bulk_clks),
> > + host->bulk_clks);
> > clk_disable_unprepare(host->src_clk_cg);
> > clk_disable_unprepare(host->src_clk);
> > clk_disable_unprepare(host->bus_clk);
> > @@ -792,10 +796,17 @@ static void msdc_gate_clock(struct msdc_host *host)
> >
> > static void msdc_ungate_clock(struct msdc_host *host)
> > {
> > + int ret;
> > +
> > clk_prepare_enable(host->h_clk);
> > clk_prepare_enable(host->bus_clk);
> > clk_prepare_enable(host->src_clk);
> > clk_prepare_enable(host->src_clk_cg);
> > + ret = clk_bulk_prepare_enable(ARRAY_SIZE(host->bulk_clks),
> > + host->bulk_clks);
> > + if (ret)
> > + dev_dbg(host->dev, "enable clks failed!\n");
>
> dev_err looks a lot more appropriate. Also, don't you want to exit the
> function in that case, rather than going to the while loop below where
> you may get stuck?
>
I will change it in the next version.
> > +
> > while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
> > cpu_relax();
> > }
> > @@ -2366,6 +2377,52 @@ static void msdc_of_property_parse(struct platform_device *pdev,
> > host->cqhci = false;
> > }
> >
> > +static int msdc_of_clock_parse(struct platform_device *pdev,
> > + struct msdc_host *host)
> > +{
> > + struct clk *clk;
> > +
> > + host->src_clk = devm_clk_get(&pdev->dev, "source");
> > + if (IS_ERR(host->src_clk))
> > + return PTR_ERR(host->src_clk);
> > +
> > + host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > + if (IS_ERR(host->h_clk))
> > + return PTR_ERR(host->h_clk);
> > +
> > + host->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
> > + if (IS_ERR(host->bus_clk))
> > + host->bus_clk = NULL;
>
> Use devm_clk_get_optional instead (ditto for the next 2).
>
> > +
> > + /*source clock control gate is optional clock*/
> > + host->src_clk_cg = devm_clk_get(&pdev->dev, "source_cg");
> > + if (IS_ERR(host->src_clk_cg))
> > + host->src_clk_cg = NULL;
> > +
> > + host->sys_clk_cg = devm_clk_get(&pdev->dev, "sys_cg");
> > + if (IS_ERR(host->sys_clk_cg))
> > + host->sys_clk_cg = NULL;
> > + else
> > + clk_prepare_enable(host->sys_clk_cg);
>
> This doesn't need to be in an else branch, calling clk_prepare_enable
> on a NULL clock is fine.
I will change it in the next version.
>
> However, is it expected that this clock is turned on forever after
> probe?! At the very least, the clock should be disabled in
> msdc_drv_remove, but, really, I think it should be enabled as needed,
> like the other clocks, in msdc_gate_clock?
>
This clock gate can not be closed, if it is closed, it can not be
accessed and can not be opened again.
So it is expected that this clock is turned on forever after probe.
> > +
> > + clk = devm_clk_get(&pdev->dev, "pclk_cg");
> > + if (IS_ERR(clk))
> > + clk = NULL;
> > + host->bulk_clks[0].clk = clk;
> > +
> > + clk = devm_clk_get(&pdev->dev, "axi_cg");
> > + if (IS_ERR(clk))
> > + clk = NULL;
> > + host->bulk_clks[1].clk = clk;
> > +
> > + clk = devm_clk_get(&pdev->dev, "ahb_cg");
> > + if (IS_ERR(clk))
> > + clk = NULL;
> > + host->bulk_clks[2].clk = clk;
>
> Use devm_clk_bulk_get_optional for these 3.
>
I will change it in the next version.
> > +
> > + return 0;
> > +}
> > +
> > static int msdc_drv_probe(struct platform_device *pdev)
> > {
> > struct mmc_host *mmc;
> > @@ -2405,25 +2462,9 @@ static int msdc_drv_probe(struct platform_device *pdev)
> > if (ret)
> > goto host_free;
> >
> > - host->src_clk = devm_clk_get(&pdev->dev, "source");
> > - if (IS_ERR(host->src_clk)) {
> > - ret = PTR_ERR(host->src_clk);
> > - goto host_free;
> > - }
> > -
> > - host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > - if (IS_ERR(host->h_clk)) {
> > - ret = PTR_ERR(host->h_clk);
> > + ret = msdc_of_clock_parse(pdev, host);
> > + if (ret)
> > goto host_free;
> > - }
> > -
> > - host->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
> > - if (IS_ERR(host->bus_clk))
> > - host->bus_clk = NULL;
> > - /*source clock control gate is optional clock*/
> > - host->src_clk_cg = devm_clk_get(&pdev->dev, "source_cg");
> > - if (IS_ERR(host->src_clk_cg))
> > - host->src_clk_cg = NULL;
> >
> > host->reset = devm_reset_control_get_optional_exclusive(&pdev->dev,
> > "hrst");
> > --
> > 2.18.0