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
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
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
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
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
>
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
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