[1/2] mmc: tmio: leave clock handling to PM if enabled
diff mbox series

Message ID 1563289264-26432-2-git-send-email-uli+renesas@fpond.eu
State New
Headers show
Series
  • mmc: tmio: remove Gen2+ workaround and fix up
Related show

Commit Message

Ulrich Hecht July 16, 2019, 3:01 p.m. UTC
This fixes a clock imbalance that occurs because the SD clock is handled
by both PM and the hardware driver.
See https://www.spinics.net/lists/linux-mmc/msg44431.html for details.

This patch removes the clock handling from the driver's PM callbacks and
turns the clock off after probing.

Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/mmc/host/tmio_mmc_core.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

Comments

Wolfram Sang July 16, 2019, 7:05 p.m. UTC | #1
On Tue, Jul 16, 2019 at 05:01:03PM +0200, Ulrich Hecht wrote:
> This fixes a clock imbalance that occurs because the SD clock is handled
> by both PM and the hardware driver.
> See https://www.spinics.net/lists/linux-mmc/msg44431.html for details.
> 
> This patch removes the clock handling from the driver's PM callbacks and
> turns the clock off after probing.
> 
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>

Thanks, Uli!

Niklas, I'd really like your feedback on this one because you did the PM
refactorization lately.

I will have a look later, too.
Niklas Söderlund July 17, 2019, 1:13 a.m. UTC | #2
On 2019-07-16 21:05:24 +0200, Wolfram Sang wrote:
> On Tue, Jul 16, 2019 at 05:01:03PM +0200, Ulrich Hecht wrote:
> > This fixes a clock imbalance that occurs because the SD clock is handled
> > by both PM and the hardware driver.
> > See https://www.spinics.net/lists/linux-mmc/msg44431.html for details.
> > 
> > This patch removes the clock handling from the driver's PM callbacks and
> > turns the clock off after probing.
> > 
> > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> 
> Thanks, Uli!
> 
> Niklas, I'd really like your feedback on this one because you did the PM
> refactorization lately.

I would like to test this too. Unfortunately I'm on the road and will be 
back in the office the 23rd so I will have to postpone doing so until 
then. My initial comment is that this looks good, thanks Ulrich.

> 
> I will have a look later, too.
>
Ulf Hansson July 25, 2019, 1:43 p.m. UTC | #3
On Tue, 16 Jul 2019 at 17:01, Ulrich Hecht <uli+renesas@fpond.eu> wrote:
>
> This fixes a clock imbalance that occurs because the SD clock is handled
> by both PM and the hardware driver.
> See https://www.spinics.net/lists/linux-mmc/msg44431.html for details.

This is a generic problem, when a device are being attached to a genpd
and when the genpd has got the ->stop|start() callbacks assigned, as
to manage device clocks.

Can you try to describe this problem a little bit more in detail, as I
think that's important to carry in the change log.

>
> This patch removes the clock handling from the driver's PM callbacks and

runtime PM callbacks and/or system PM callbacks?

> turns the clock off after probing.
>
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> ---
>  drivers/mmc/host/tmio_mmc_core.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 31ffcc3..26dcbba 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -1260,9 +1260,14 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
>         /* See if we also get DMA */
>         tmio_mmc_request_dma(_host, pdata);
>
> -       pm_runtime_set_active(&pdev->dev);
> +#ifdef CONFIG_PM
> +       /* PM handles the clock; disable it so it won't be enabled twice. */
> +       if (_host->clk_disable)
> +               _host->clk_disable(_host);

The clock managed here, is that the same clock as being managed by
genpd's ->stop|start() callbacks?

> +       pm_runtime_get_sync(&pdev->dev);
>         pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
>         pm_runtime_use_autosuspend(&pdev->dev);
> +#endif

So what happens if you have CONFIG_PM set, but the device doesn't have
a genpd attached?

I am guessing the driver should handle the clock in such scenario, right?

>
>         ret = mmc_add_host(mmc);
>         if (ret)
> @@ -1302,20 +1307,6 @@ void tmio_mmc_host_remove(struct tmio_mmc_host *host)
>  EXPORT_SYMBOL_GPL(tmio_mmc_host_remove);
>
>  #ifdef CONFIG_PM
> -static int tmio_mmc_clk_enable(struct tmio_mmc_host *host)
> -{
> -       if (!host->clk_enable)
> -               return -ENOTSUPP;
> -
> -       return host->clk_enable(host);
> -}
> -
> -static void tmio_mmc_clk_disable(struct tmio_mmc_host *host)
> -{
> -       if (host->clk_disable)
> -               host->clk_disable(host);
> -}
> -
>  int tmio_mmc_host_runtime_suspend(struct device *dev)
>  {
>         struct tmio_mmc_host *host = dev_get_drvdata(dev);
> @@ -1325,8 +1316,6 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
>         if (host->clk_cache)
>                 host->set_clock(host, 0);
>
> -       tmio_mmc_clk_disable(host);
> -
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(tmio_mmc_host_runtime_suspend);
> @@ -1340,7 +1329,6 @@ int tmio_mmc_host_runtime_resume(struct device *dev)
>  {
>         struct tmio_mmc_host *host = dev_get_drvdata(dev);
>
> -       tmio_mmc_clk_enable(host);
>         tmio_mmc_hw_reset(host->mmc);
>
>         if (host->clk_cache)
> --
> 2.7.4
>

Kind regards
Uffe
Geert Uytterhoeven July 29, 2019, 8:44 a.m. UTC | #4
Hi Ulf,

On Thu, Jul 25, 2019 at 3:44 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Tue, 16 Jul 2019 at 17:01, Ulrich Hecht <uli+renesas@fpond.eu> wrote:
> > This fixes a clock imbalance that occurs because the SD clock is handled
> > by both PM and the hardware driver.
> > See https://www.spinics.net/lists/linux-mmc/msg44431.html for details.
>
> This is a generic problem, when a device are being attached to a genpd
> and when the genpd has got the ->stop|start() callbacks assigned, as
> to manage device clocks.
>
> Can you try to describe this problem a little bit more in detail, as I
> think that's important to carry in the change log.
>
> >
> > This patch removes the clock handling from the driver's PM callbacks and
>
> runtime PM callbacks and/or system PM callbacks?
>
> > turns the clock off after probing.
> >
> > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> > ---
> >  drivers/mmc/host/tmio_mmc_core.c | 24 ++++++------------------
> >  1 file changed, 6 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> > index 31ffcc3..26dcbba 100644
> > --- a/drivers/mmc/host/tmio_mmc_core.c
> > +++ b/drivers/mmc/host/tmio_mmc_core.c
> > @@ -1260,9 +1260,14 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
> >         /* See if we also get DMA */
> >         tmio_mmc_request_dma(_host, pdata);
> >
> > -       pm_runtime_set_active(&pdev->dev);
> > +#ifdef CONFIG_PM
> > +       /* PM handles the clock; disable it so it won't be enabled twice. */
> > +       if (_host->clk_disable)
> > +               _host->clk_disable(_host);
>
> The clock managed here, is that the same clock as being managed by
> genpd's ->stop|start() callbacks?
>
> > +       pm_runtime_get_sync(&pdev->dev);
> >         pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> >         pm_runtime_use_autosuspend(&pdev->dev);
> > +#endif
>
> So what happens if you have CONFIG_PM set, but the device doesn't have
> a genpd attached?

That's OK for all SoCs served by renesas_sdhi_internal_dmac.c and
renesas_sdhi_sys_dmac.c, as they all have their clock domain described
in DT...

> I am guessing the driver should handle the clock in such scenario, right?

... but it's not for (non-Renesas) systems served by tmio_mmc.c.

Gr{oetje,eeting}s,

                        Geert

Patch
diff mbox series

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 31ffcc3..26dcbba 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -1260,9 +1260,14 @@  int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
 	/* See if we also get DMA */
 	tmio_mmc_request_dma(_host, pdata);
 
-	pm_runtime_set_active(&pdev->dev);
+#ifdef CONFIG_PM
+	/* PM handles the clock; disable it so it won't be enabled twice. */
+	if (_host->clk_disable)
+		_host->clk_disable(_host);
+	pm_runtime_get_sync(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
 	pm_runtime_use_autosuspend(&pdev->dev);
+#endif
 
 	ret = mmc_add_host(mmc);
 	if (ret)
@@ -1302,20 +1307,6 @@  void tmio_mmc_host_remove(struct tmio_mmc_host *host)
 EXPORT_SYMBOL_GPL(tmio_mmc_host_remove);
 
 #ifdef CONFIG_PM
-static int tmio_mmc_clk_enable(struct tmio_mmc_host *host)
-{
-	if (!host->clk_enable)
-		return -ENOTSUPP;
-
-	return host->clk_enable(host);
-}
-
-static void tmio_mmc_clk_disable(struct tmio_mmc_host *host)
-{
-	if (host->clk_disable)
-		host->clk_disable(host);
-}
-
 int tmio_mmc_host_runtime_suspend(struct device *dev)
 {
 	struct tmio_mmc_host *host = dev_get_drvdata(dev);
@@ -1325,8 +1316,6 @@  int tmio_mmc_host_runtime_suspend(struct device *dev)
 	if (host->clk_cache)
 		host->set_clock(host, 0);
 
-	tmio_mmc_clk_disable(host);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tmio_mmc_host_runtime_suspend);
@@ -1340,7 +1329,6 @@  int tmio_mmc_host_runtime_resume(struct device *dev)
 {
 	struct tmio_mmc_host *host = dev_get_drvdata(dev);
 
-	tmio_mmc_clk_enable(host);
 	tmio_mmc_hw_reset(host->mmc);
 
 	if (host->clk_cache)