diff mbox series

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

Message ID 1564589857-17720-2-git-send-email-uli+renesas@fpond.eu (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series mmc: tmio: remove Gen2+ workaround and fix up | expand

Commit Message

Ulrich Hecht July 31, 2019, 4:17 p.m. UTC
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(-)

Comments

Ulf Hansson Aug. 2, 2019, 1:24 p.m. UTC | #1
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
Wolfram Sang Aug. 22, 2019, 6:35 a.m. UTC | #2
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
Ulf Hansson Aug. 22, 2019, 9:43 a.m. UTC | #3
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
Wolfram Sang Aug. 22, 2019, 9:52 a.m. UTC | #4
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
diff mbox series

Patch

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)