[PATCH 0/2] mmc: tmio: remove Gen2+ workaround and fix up

[PATCH 0/2] mmc: tmio: remove Gen2+ workaround and fix up

[Ulrich Hecht]

Hi!

The second patch in this series removes a workaround that forced eMMC devices
always on and that is no longer required.

Removing it does expose a bug, however, that leads to a clock imbalance due
to the clock being enabled by both PM and the hardware driver. (See
https://www.spinics.net/lists/linux-mmc/msg54009.html for discussion.)
This bug is taken care of by the first patch.

Tested on r8a7790 (Lager), r8a7795 and r8a7796 (Salvator-X) with SD and
eMMC, before and after suspend.

CU
Uli


Ulrich Hecht (2):
  mmc: tmio: leave clock handling to PM if enabled
  mmc: tmio: remove obsolete PM workaround

 drivers/mmc/host/tmio_mmc_core.c | 33 ++++++---------------------------
 1 file changed, 6 insertions(+), 27 deletions(-)

-- 
2.7.4

[PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled

[Ulrich Hecht]

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

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

Re: [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled

[Niklas Söderlund]

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



-- 
Regards,
Niklas Söderlund

Re: [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled

[Ulf Hansson]

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

Re: [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 2/2] mmc: tmio: remove obsolete PM workaround

[Ulrich Hecht]

Obsoleted by
"mmc: tmio: move runtime PM enablement to the driver implementations".

Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/mmc/host/tmio_mmc_core.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 26dcbba..29c0d2c 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -1221,15 +1221,6 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
 		_host->reset = tmio_mmc_reset;
 
 	/*
-	 * On Gen2+, eMMC with NONREMOVABLE currently fails because native
-	 * hotplug gets disabled. It seems RuntimePM related yet we need further
-	 * research. Since we are planning a PM overhaul anyway, let's enforce
-	 * for now the device being active by enabling native hotplug always.
-	 */
-	if (pdata->flags & TMIO_MMC_MIN_RCAR2)
-		_host->native_hotplug = true;
-
-	/*
 	 * While using internal tmio hardware logic for card detection, we need
 	 * to ensure it stays powered for it to work.
 	 */
-- 
2.7.4

Re: [PATCH 2/2] mmc: tmio: remove obsolete PM workaround

[Geert Uytterhoeven]

On Tue, Jul 16, 2019 at 5:01 PM Ulrich Hecht <uli+renesas@fpond.eu> wrote:
> Obsoleted by
> "mmc: tmio: move runtime PM enablement to the driver implementations".

commit 7ff213193310ef8d ("mmc: tmio: move runtime PM enablement to the
driver implementations")

>
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> ---
>  drivers/mmc/host/tmio_mmc_core.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 26dcbba..29c0d2c 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -1221,15 +1221,6 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
>                 _host->reset = tmio_mmc_reset;
>
>         /*
> -        * On Gen2+, eMMC with NONREMOVABLE currently fails because native
> -        * hotplug gets disabled. It seems RuntimePM related yet we need further
> -        * research. Since we are planning a PM overhaul anyway, let's enforce
> -        * for now the device being active by enabling native hotplug always.
> -        */
> -       if (pdata->flags & TMIO_MMC_MIN_RCAR2)
> -               _host->native_hotplug = true;
> -
> -       /*
>          * While using internal tmio hardware logic for card detection, we need
>          * to ensure it stays powered for it to work.
>          */

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/2] mmc: tmio: remove Gen2+ workaround and fix up

[Niklas Söderlund]

Hi Ulrich,

Thanks for your work.

On 2019-07-16 17:01:02 +0200, Ulrich Hecht wrote:
> Hi!
> 
> The second patch in this series removes a workaround that forced eMMC devices
> always on and that is no longer required.
> 
> Removing it does expose a bug, however, that leads to a clock imbalance due
> to the clock being enabled by both PM and the hardware driver. (See
> https://www.spinics.net/lists/linux-mmc/msg54009.html for discussion.)
> This bug is taken care of by the first patch.
> 
> Tested on r8a7790 (Lager), r8a7795 and r8a7796 (Salvator-X) with SD and
> eMMC, before and after suspend.
> 
> CU
> Uli
> 
> 
> Ulrich Hecht (2):
>   mmc: tmio: leave clock handling to PM if enabled
>   mmc: tmio: remove obsolete PM workaround

Tested-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> 
>  drivers/mmc/host/tmio_mmc_core.c | 33 ++++++---------------------------
>  1 file changed, 6 insertions(+), 27 deletions(-)
> 
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund