Re: [PATCH v6 4/4] mmc: mediatek: Add subsys clock control for MT8192 msdc
On Wed, Oct 14, 2020 at 10:29 AM Wenbin Mei wrote:
>
> On Tue, 2020-10-13 at 17:10 +0200, Matthias Brugger wrote:
> >
> > On 12/10/2020 14:45, Wenbin Mei 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
> > > ---
> > > drivers/mmc/host/mtk-sd.c | 74 +--
> > > 1 file changed, 56 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > > index a704745e5882..c7df7510f120 100644
> > > --- a/drivers/mmc/host/mtk-sd.c
> > > +++ b/drivers/mmc/host/mtk-sd.c
> > [...]
> > > +static int msdc_of_clock_parse(struct platform_device *pdev,
> > > + struct msdc_host *host)
> > > +{
> > > + int ret;
> > > +
> > > + host->src_clk = devm_clk_get(>dev, "source");
> > > + if (IS_ERR(host->src_clk))
> > > + return PTR_ERR(host->src_clk);
> > > +
> > > + host->h_clk = devm_clk_get(>dev, "hclk");
> > > + if (IS_ERR(host->h_clk))
> > > + return PTR_ERR(host->h_clk);
> > > +
> > > + host->bus_clk = devm_clk_get_optional(>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_optional(>dev, "source_cg");
> > > + if (IS_ERR(host->src_clk_cg))
> > > + host->src_clk_cg = NULL;
> > > +
> > > + host->sys_clk_cg = devm_clk_get_optional(>dev, "sys_cg");
> > > + if (IS_ERR(host->sys_clk_cg))
> > > + host->sys_clk_cg = NULL;
> > > +
> > > + /* If present, always enable for this clock gate */
> > > + clk_prepare_enable(host->sys_clk_cg);
> > > +
> > > + host->bulk_clks[0].id = "pclk_cg";
> > > + host->bulk_clks[1].id = "axi_cg";
> > > + host->bulk_clks[2].id = "ahb_cg";
> >
> > That looks at least suspicious. The pointers of id point to some strings
> > defined
> > in the function. Aren't they out of scope once msdc_of_clock_parse() has
> > returned?
> >
> These constants are not in stack range, so they will not be lost.
> And I have confirmed it after msdc_of_clock_parse() has returned, these
> ids still exist.
Yes I guess the constants end up in .rodata (or similar section), but
I'm not sure if this is absolutely guaranteed.
In any case, this is a commonly used pattern, so I'd hope it's fine
(just a sample, there are more):
https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/dwc/pcie-qcom.c#L266
https://elixir.bootlin.com/linux/latest/source/sound/soc/codecs/wm8994.c#L4638
https://elixir.bootlin.com/linux/latest/source/drivers/mfd/madera-core.c#L467
https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpio-dwapb.c#L675
>
> > Regards,
> > Matthias
>
Re: [PATCH v6 4/4] mmc: mediatek: Add subsys clock control for MT8192 msdc
On Wed, Oct 14, 2020 at 3:44 PM Matthias Brugger wrote: > > > > On 14/10/2020 05:06, Nicolas Boichat wrote: > > On Wed, Oct 14, 2020 at 10:29 AM Wenbin Mei wrote: > >> > >> On Tue, 2020-10-13 at 17:10 +0200, Matthias Brugger wrote: > >>> > >>> On 12/10/2020 14:45, Wenbin Mei 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 [...] > + host->bulk_clks[0].id = "pclk_cg"; > + host->bulk_clks[1].id = "axi_cg"; > + host->bulk_clks[2].id = "ahb_cg"; > >>> > >>> That looks at least suspicious. The pointers of id point to some strings > >>> defined > >>> in the function. Aren't they out of scope once msdc_of_clock_parse() has > >>> returned? > >>> > >> These constants are not in stack range, so they will not be lost. > >> And I have confirmed it after msdc_of_clock_parse() has returned, these > >> ids still exist. > > > > Yes I guess the constants end up in .rodata (or similar section), but > > I'm not sure if this is absolutely guaranteed. > > > > In any case, this is a commonly used pattern, so I'd hope it's fine > > (just a sample, there are more): > > https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/dwc/pcie-qcom.c#L266 > > https://elixir.bootlin.com/linux/latest/source/sound/soc/codecs/wm8994.c#L4638 > > https://elixir.bootlin.com/linux/latest/source/drivers/mfd/madera-core.c#L467 > > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpio-dwapb.c#L675 > > > > Alright, then this looks good, sorry for the noise! To close this in more satisfying way, I asked internally, and +Pi-Hsun Shih digged out this answer: """ C11 standard 6.4.5 String literals says: "The multibyte character sequence is then used to initialize an array of >>static storage duration<< and length just sufficient to contain the sequence" """ > Matthias
Re: [PATCH v6 4/4] mmc: mediatek: Add subsys clock control for MT8192 msdc
On 14/10/2020 05:06, Nicolas Boichat wrote:
On Wed, Oct 14, 2020 at 10:29 AM Wenbin Mei wrote:
On Tue, 2020-10-13 at 17:10 +0200, Matthias Brugger wrote:
On 12/10/2020 14:45, Wenbin Mei 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
---
drivers/mmc/host/mtk-sd.c | 74 +--
1 file changed, 56 insertions(+), 18 deletions(-)
diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index a704745e5882..c7df7510f120 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
[...]
+static int msdc_of_clock_parse(struct platform_device *pdev,
+ struct msdc_host *host)
+{
+ int ret;
+
+ host->src_clk = devm_clk_get(>dev, "source");
+ if (IS_ERR(host->src_clk))
+ return PTR_ERR(host->src_clk);
+
+ host->h_clk = devm_clk_get(>dev, "hclk");
+ if (IS_ERR(host->h_clk))
+ return PTR_ERR(host->h_clk);
+
+ host->bus_clk = devm_clk_get_optional(>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_optional(>dev, "source_cg");
+ if (IS_ERR(host->src_clk_cg))
+ host->src_clk_cg = NULL;
+
+ host->sys_clk_cg = devm_clk_get_optional(>dev, "sys_cg");
+ if (IS_ERR(host->sys_clk_cg))
+ host->sys_clk_cg = NULL;
+
+ /* If present, always enable for this clock gate */
+ clk_prepare_enable(host->sys_clk_cg);
+
+ host->bulk_clks[0].id = "pclk_cg";
+ host->bulk_clks[1].id = "axi_cg";
+ host->bulk_clks[2].id = "ahb_cg";
That looks at least suspicious. The pointers of id point to some strings defined
in the function. Aren't they out of scope once msdc_of_clock_parse() has
returned?
These constants are not in stack range, so they will not be lost.
And I have confirmed it after msdc_of_clock_parse() has returned, these
ids still exist.
Yes I guess the constants end up in .rodata (or similar section), but
I'm not sure if this is absolutely guaranteed.
In any case, this is a commonly used pattern, so I'd hope it's fine
(just a sample, there are more):
https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/dwc/pcie-qcom.c#L266
https://elixir.bootlin.com/linux/latest/source/sound/soc/codecs/wm8994.c#L4638
https://elixir.bootlin.com/linux/latest/source/drivers/mfd/madera-core.c#L467
https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpio-dwapb.c#L675
Alright, then this looks good, sorry for the noise!
Matthias
Re: [PATCH v6 4/4] mmc: mediatek: Add subsys clock control for MT8192 msdc
On Tue, 2020-10-13 at 17:10 +0200, Matthias Brugger wrote:
>
> On 12/10/2020 14:45, Wenbin Mei 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
> > ---
> > drivers/mmc/host/mtk-sd.c | 74 +--
> > 1 file changed, 56 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index a704745e5882..c7df7510f120 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> [...]
> > +static int msdc_of_clock_parse(struct platform_device *pdev,
> > + struct msdc_host *host)
> > +{
> > + int ret;
> > +
> > + host->src_clk = devm_clk_get(>dev, "source");
> > + if (IS_ERR(host->src_clk))
> > + return PTR_ERR(host->src_clk);
> > +
> > + host->h_clk = devm_clk_get(>dev, "hclk");
> > + if (IS_ERR(host->h_clk))
> > + return PTR_ERR(host->h_clk);
> > +
> > + host->bus_clk = devm_clk_get_optional(>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_optional(>dev, "source_cg");
> > + if (IS_ERR(host->src_clk_cg))
> > + host->src_clk_cg = NULL;
> > +
> > + host->sys_clk_cg = devm_clk_get_optional(>dev, "sys_cg");
> > + if (IS_ERR(host->sys_clk_cg))
> > + host->sys_clk_cg = NULL;
> > +
> > + /* If present, always enable for this clock gate */
> > + clk_prepare_enable(host->sys_clk_cg);
> > +
> > + host->bulk_clks[0].id = "pclk_cg";
> > + host->bulk_clks[1].id = "axi_cg";
> > + host->bulk_clks[2].id = "ahb_cg";
>
> That looks at least suspicious. The pointers of id point to some strings
> defined
> in the function. Aren't they out of scope once msdc_of_clock_parse() has
> returned?
>
These constants are not in stack range, so they will not be lost.
And I have confirmed it after msdc_of_clock_parse() has returned, these
ids still exist.
> Regards,
> Matthias
Re: [PATCH v6 4/4] mmc: mediatek: Add subsys clock control for MT8192 msdc
On 12/10/2020 14:45, Wenbin Mei 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
---
drivers/mmc/host/mtk-sd.c | 74 +--
1 file changed, 56 insertions(+), 18 deletions(-)
diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index a704745e5882..c7df7510f120 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
[...]
+static int msdc_of_clock_parse(struct platform_device *pdev,
+ struct msdc_host *host)
+{
+ int ret;
+
+ host->src_clk = devm_clk_get(>dev, "source");
+ if (IS_ERR(host->src_clk))
+ return PTR_ERR(host->src_clk);
+
+ host->h_clk = devm_clk_get(>dev, "hclk");
+ if (IS_ERR(host->h_clk))
+ return PTR_ERR(host->h_clk);
+
+ host->bus_clk = devm_clk_get_optional(>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_optional(>dev, "source_cg");
+ if (IS_ERR(host->src_clk_cg))
+ host->src_clk_cg = NULL;
+
+ host->sys_clk_cg = devm_clk_get_optional(>dev, "sys_cg");
+ if (IS_ERR(host->sys_clk_cg))
+ host->sys_clk_cg = NULL;
+
+ /* If present, always enable for this clock gate */
+ clk_prepare_enable(host->sys_clk_cg);
+
+ host->bulk_clks[0].id = "pclk_cg";
+ host->bulk_clks[1].id = "axi_cg";
+ host->bulk_clks[2].id = "ahb_cg";
That looks at least suspicious. The pointers of id point to some strings defined
in the function. Aren't they out of scope once msdc_of_clock_parse() has returned?
Regards,
Matthias
Re: [PATCH v6 4/4] mmc: mediatek: Add subsys clock control for MT8192 msdc
On Mon, Oct 12, 2020 at 8:46 PM Wenbin Mei 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.
>
Looks ok now, but I'd still like to see 1 or 2 follow-up patches that:
1. In msdc_ungate_clock: check all clk_prepare_enable return values
before busy looping (to be consistent with how you now handle
bulk_clks)
2. In msdc_of_clock_parse: All these if(IS_ERR(clk)) clk = NULL;
should be replaced by if (IS_ERR(clk)) return PTR_ERR(clk);
Reviewed-by: Nicolas Boichat
> Signed-off-by: Wenbin Mei
> ---
> drivers/mmc/host/mtk-sd.c | 74 +--
> 1 file changed, 56 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index a704745e5882..c7df7510f120 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -35,6 +35,7 @@
> #include "cqhci.h"
>
> #define MAX_BD_NUM 1024
> +#define MSDC_NR_CLOCKS 3
>
>
> /*--*/
> /* Common Definition
> */
> @@ -425,6 +426,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[MSDC_NR_CLOCKS];
> u32 mclk; /* mmc subsystem clock frequency */
> u32 src_clk_freq; /* source clock frequency */
> unsigned char timing;
> @@ -784,6 +787,7 @@ 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(MSDC_NR_CLOCKS, 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,18 @@ 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(MSDC_NR_CLOCKS, host->bulk_clks);
> + if (ret) {
> + dev_err(host->dev, "Cannot enable pclk/axi/ahb clock
> gates\n");
> + return;
> + }
> +
> while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
> cpu_relax();
> }
> @@ -2366,6 +2378,48 @@ 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)
> +{
> + int ret;
> +
> + host->src_clk = devm_clk_get(>dev, "source");
> + if (IS_ERR(host->src_clk))
> + return PTR_ERR(host->src_clk);
> +
> + host->h_clk = devm_clk_get(>dev, "hclk");
> + if (IS_ERR(host->h_clk))
> + return PTR_ERR(host->h_clk);
> +
> + host->bus_clk = devm_clk_get_optional(>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_optional(>dev, "source_cg");
> + if (IS_ERR(host->src_clk_cg))
> + host->src_clk_cg = NULL;
> +
> + host->sys_clk_cg = devm_clk_get_optional(>dev, "sys_cg");
> + if (IS_ERR(host->sys_clk_cg))
> + host->sys_clk_cg = NULL;
> +
> + /* If present, always enable for this clock gate */
> + clk_prepare_enable(host->sys_clk_cg);
> +
> + host->bulk_clks[0].id = "pclk_cg";
> + host->bulk_clks[1].id = "axi_cg";
> + host->bulk_clks[2].id = "ahb_cg";
> + ret = devm_clk_bulk_get_optional(>dev, MSDC_NR_CLOCKS,
> +host->bulk_clks);
> + if (ret) {
> + dev_err(>dev, "Cannot get pclk/axi/ahb clock gates\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int msdc_drv_probe(struct platform_device *pdev)
> {
> struct mmc_host *mmc;
> @@ -2405,25 +2459,9 @@ static int msdc_drv_probe(struct platform_device *pdev)
> if (ret)
> goto host_free;
>
> - host->src_clk = devm_clk_get(>dev, "source");
> - if (IS_ERR(host->src_clk)) {
> - ret = PTR_ERR(host->src_clk);
> - goto host_free;
> - }
> -
> - host->h_clk = devm_clk_get(>dev, "hclk");
> - if (IS_ERR(host->h_clk)) {
> - ret =
