2020-10-12 13:22:34

by Wenbin Mei (梅文彬)

[permalink] [raw]
Subject: [PATCH v6 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 | 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(&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_optional(&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_optional(&pdev->dev, "source_cg");
+ if (IS_ERR(host->src_clk_cg))
+ host->src_clk_cg = NULL;
+
+ host->sys_clk_cg = devm_clk_get_optional(&pdev->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(&pdev->dev, MSDC_NR_CLOCKS,
+ host->bulk_clks);
+ if (ret) {
+ dev_err(&pdev->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(&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-13 16:25:41

by Nicolas Boichat

[permalink] [raw]
Subject: 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 <[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.
>

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 <[email protected]>

> Signed-off-by: Wenbin Mei <[email protected]>
> ---
> 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(&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_optional(&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_optional(&pdev->dev, "source_cg");
> + if (IS_ERR(host->src_clk_cg))
> + host->src_clk_cg = NULL;
> +
> + host->sys_clk_cg = devm_clk_get_optional(&pdev->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(&pdev->dev, MSDC_NR_CLOCKS,
> + host->bulk_clks);
> + if (ret) {
> + dev_err(&pdev->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(&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-13 23:57:27

by Matthias Brugger

[permalink] [raw]
Subject: 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 <[email protected]>
> ---
> 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(&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_optional(&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_optional(&pdev->dev, "source_cg");
> + if (IS_ERR(host->src_clk_cg))
> + host->src_clk_cg = NULL;
> +
> + host->sys_clk_cg = devm_clk_get_optional(&pdev->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

2020-10-14 08:28:51

by Wenbin Mei (梅文彬)

[permalink] [raw]
Subject: 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 <[email protected]>
> > ---
> > 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(&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_optional(&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_optional(&pdev->dev, "source_cg");
> > + if (IS_ERR(host->src_clk_cg))
> > + host->src_clk_cg = NULL;
> > +
> > + host->sys_clk_cg = devm_clk_get_optional(&pdev->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

2020-10-14 09:26:33

by Nicolas Boichat

[permalink] [raw]
Subject: 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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(&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_optional(&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_optional(&pdev->dev, "source_cg");
> > > + if (IS_ERR(host->src_clk_cg))
> > > + host->src_clk_cg = NULL;
> > > +
> > > + host->sys_clk_cg = devm_clk_get_optional(&pdev->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
>

2020-10-14 14:20:16

by Matthias Brugger

[permalink] [raw]
Subject: 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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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(&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_optional(&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_optional(&pdev->dev, "source_cg");
>>>> + if (IS_ERR(host->src_clk_cg))
>>>> + host->src_clk_cg = NULL;
>>>> +
>>>> + host->sys_clk_cg = devm_clk_get_optional(&pdev->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

2020-10-14 14:20:30

by Nicolas Boichat

[permalink] [raw]
Subject: 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 <[email protected]> wrote:
>
>
>
> On 14/10/2020 05:06, Nicolas Boichat wrote:
> > On Wed, Oct 14, 2020 at 10:29 AM Wenbin Mei <[email protected]> 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 <[email protected]>
[...]
> >>>> + 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