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

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

[Ulrich Hecht]

Hi!

Second revision of the series that eliminates the forced-on eMMC workaround
for Renesas Gen2 SoCs and fixes up the clock imbalance exposed by that,
which is caused by interactions between runtime PM and the tmio hardware
driver.

Thanks to Ulf, Niklas and Geert for reviews and testing, see below for changes.

CU
Uli

Changes since v1:
- Keep clock handling in driver if no power domain is attached.
- Describe clock imbalance issue in commit message.
- Add commit hash for "mmc: tmio: move runtime PM enablement to the
driver implementations".


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

 drivers/mmc/host/tmio_mmc_core.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

-- 
2.7.4

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

[Ulrich Hecht]

This fixes a clock imbalance that occurs on Renesas systems because the SD
clock is handled by both runtime PM and the hardware driver. After a
suspend/resume cycle both enable the same clock, resulting in an enable
count of 2 even if the clock is only used by one device.

See https://www.spinics.net/lists/linux-mmc/msg44431.html for details.

This patch removes the clock handling from the driver's runtime PM
callbacks and turns the clock off after probing if the device has a power
domain attached.

Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
Tested-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 drivers/mmc/host/tmio_mmc_core.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 31ffcc3..733ff96 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 && pdev->dev.pm_domain)
+		_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)
@@ -1325,7 +1330,8 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
 	if (host->clk_cache)
 		host->set_clock(host, 0);
 
-	tmio_mmc_clk_disable(host);
+	if (!dev->pm_domain)
+		tmio_mmc_clk_disable(host);
 
 	return 0;
 }
@@ -1340,7 +1346,9 @@ int tmio_mmc_host_runtime_resume(struct device *dev)
 {
 	struct tmio_mmc_host *host = dev_get_drvdata(dev);
 
-	tmio_mmc_clk_enable(host);
+	if (!dev->pm_domain)
+		tmio_mmc_clk_enable(host);
+
 	tmio_mmc_hw_reset(host->mmc);
 
 	if (host->clk_cache)
-- 
2.7.4

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

[Ulrich Hecht]

Obsoleted by 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 733ff96..72877c6 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 v2 1/2] mmc: tmio: leave clock handling to runtime PM if enabled

[Ulf Hansson]

On Wed, 31 Jul 2019 at 18:17, Ulrich Hecht <uli+renesas@fpond.eu> wrote:
>
> This fixes a clock imbalance that occurs on Renesas systems because the SD
> clock is handled by both runtime PM and the hardware driver. After a
> suspend/resume cycle both enable the same clock, resulting in an enable
> count of 2 even if the clock is only used by one device.
>
> See https://www.spinics.net/lists/linux-mmc/msg44431.html for details.
>
> This patch removes the clock handling from the driver's runtime PM
> callbacks and turns the clock off after probing if the device has a power
> domain attached.
>
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> Tested-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  drivers/mmc/host/tmio_mmc_core.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 31ffcc3..733ff96 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 && pdev->dev.pm_domain)

Hmm.

This seems to work for most cases of yours, but it's fragile, because
how do you know that the pm_domain above is managing the clock? You
don't.

> +               _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)
> @@ -1325,7 +1330,8 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
>         if (host->clk_cache)
>                 host->set_clock(host, 0);
>
> -       tmio_mmc_clk_disable(host);
> +       if (!dev->pm_domain)
> +               tmio_mmc_clk_disable(host);
>
>         return 0;
>  }
> @@ -1340,7 +1346,9 @@ int tmio_mmc_host_runtime_resume(struct device *dev)
>  {
>         struct tmio_mmc_host *host = dev_get_drvdata(dev);
>
> -       tmio_mmc_clk_enable(host);
> +       if (!dev->pm_domain)
> +               tmio_mmc_clk_enable(host);
> +
>         tmio_mmc_hw_reset(host->mmc);
>
>         if (host->clk_cache)
> --
> 2.7.4
>

I am going to think a bit more about this, but at this point, my
gut-feeling is that may need to add some helper functions to let genpd
and/or the pm_clk framework, to share some internal information with
drivers.

Kind regards
Uffe

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

[Wolfram Sang]

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

Hi Ulf,

> > +#ifdef CONFIG_PM
> > +       /* PM handles the clock; disable it so it won't be enabled twice. */
> > +       if (_host->clk_disable && pdev->dev.pm_domain)
> 
> Hmm.
> 
> This seems to work for most cases of yours, but it's fragile, because
> how do you know that the pm_domain above is managing the clock? You
> don't.
> 

...

> I am going to think a bit more about this, but at this point, my
> gut-feeling is that may need to add some helper functions to let genpd
> and/or the pm_clk framework, to share some internal information with
> drivers.

Any outcome of this? Do you want to do it?

Kind regards,

   Wolfram


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

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

[Ulf Hansson]

On Thu, 22 Aug 2019 at 08:35, Wolfram Sang <wsa@the-dreams.de> wrote:
>
> Hi Ulf,
>
> > > +#ifdef CONFIG_PM
> > > +       /* PM handles the clock; disable it so it won't be enabled twice. */
> > > +       if (_host->clk_disable && pdev->dev.pm_domain)
> >
> > Hmm.
> >
> > This seems to work for most cases of yours, but it's fragile, because
> > how do you know that the pm_domain above is managing the clock? You
> > don't.
> >
>
> ...
>
> > I am going to think a bit more about this, but at this point, my
> > gut-feeling is that may need to add some helper functions to let genpd
> > and/or the pm_clk framework, to share some internal information with
> > drivers.
>
> Any outcome of this? Do you want to do it?

Sorry for delay, it's been vacation period. I have some ideas that we
can try out, just not yet being formalize them in code.

I need to catch up a little bit more on mmc reviews, so unless this is
urgent, I can offer my help and post something soonish.

Is this fine by you?

Kind regards
Uffe

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

[Wolfram Sang]

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

Hi Ulf,

> I need to catch up a little bit more on mmc reviews, so unless this is
> urgent, I can offer my help and post something soonish.
> 
> Is this fine by you?

Yep, totally. Hope you had a great vacation! Just wanted ask if you are
happy hacking on this or if we could help here somehow. Not urgent, we
will wait.

Thanks,

   Wolfram


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