| Message ID | 20201012124547.16649-1-wenbin.mei@mediatek.com |
|---|---|
| Headers | show |
| Series | Add mmc support for MT8192 SoC | expand |
On Mon, Oct 12, 2020 at 8:46 PM Wenbin Mei <wenbin.mei@mediatek.com> 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 <drinkcat@chromium.org> > Signed-off-by: Wenbin Mei <wenbin.mei@mediatek.com> > --- > 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 <wenbin.mei@mediatek.com> > --- > 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 <wenbin.mei@mediatek.com> > > --- > > 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 <wenbin.mei@mediatek.com> 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 <wenbin.mei@mediatek.com> > > > --- > > > 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 <wenbin.mei@mediatek.com> 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 <wenbin.mei@mediatek.com> >>>> --- >>>> 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 <matthias.bgg@gmail.com> wrote: > > > > On 14/10/2020 05:06, Nicolas Boichat wrote: > > On Wed, Oct 14, 2020 at 10:29 AM Wenbin Mei <wenbin.mei@mediatek.com> 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 <wenbin.mei@mediatek.com> [...] > >>>> + 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