* [PATCH 0/2] mmc: tmio: remove Gen2+ workaround and fix up
@ 2019-07-16 15:01 Ulrich Hecht
2019-07-16 15:01 ` [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled Ulrich Hecht
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ulrich Hecht @ 2019-07-16 15:01 UTC (permalink / raw)
To: linux-renesas-soc, linux-mmc
Cc: niklas.soderlund, wsa, yamada.masahiro, geert, ulf.hansson,
magnus.damm, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled
2019-07-16 15:01 [PATCH 0/2] mmc: tmio: remove Gen2+ workaround and fix up Ulrich Hecht
@ 2019-07-16 15:01 ` Ulrich Hecht
2019-07-16 19:05 ` Wolfram Sang
2019-07-25 13:43 ` Ulf Hansson
2019-07-16 15:01 ` [PATCH 2/2] mmc: tmio: remove obsolete PM workaround Ulrich Hecht
2019-07-25 21:15 ` [PATCH 0/2] mmc: tmio: remove Gen2+ workaround and fix up Niklas Söderlund
2 siblings, 2 replies; 9+ messages in thread
From: Ulrich Hecht @ 2019-07-16 15:01 UTC (permalink / raw)
To: linux-renesas-soc, linux-mmc
Cc: niklas.soderlund, wsa, yamada.masahiro, geert, ulf.hansson,
magnus.damm, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] mmc: tmio: remove obsolete PM workaround
2019-07-16 15:01 [PATCH 0/2] mmc: tmio: remove Gen2+ workaround and fix up Ulrich Hecht
2019-07-16 15:01 ` [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled Ulrich Hecht
@ 2019-07-16 15:01 ` Ulrich Hecht
2019-07-29 8:37 ` Geert Uytterhoeven
2019-07-25 21:15 ` [PATCH 0/2] mmc: tmio: remove Gen2+ workaround and fix up Niklas Söderlund
2 siblings, 1 reply; 9+ messages in thread
From: Ulrich Hecht @ 2019-07-16 15:01 UTC (permalink / raw)
To: linux-renesas-soc, linux-mmc
Cc: niklas.soderlund, wsa, yamada.masahiro, geert, ulf.hansson,
magnus.damm, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled
2019-07-16 15:01 ` [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled Ulrich Hecht
@ 2019-07-16 19:05 ` Wolfram Sang
2019-07-17 1:13 ` Niklas Söderlund
2019-07-25 13:43 ` Ulf Hansson
1 sibling, 1 reply; 9+ messages in thread
From: Wolfram Sang @ 2019-07-16 19:05 UTC (permalink / raw)
To: Ulrich Hecht
Cc: linux-renesas-soc, linux-mmc, niklas.soderlund, yamada.masahiro,
geert, ulf.hansson, magnus.damm
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled
2019-07-16 19:05 ` Wolfram Sang
@ 2019-07-17 1:13 ` Niklas Söderlund
0 siblings, 0 replies; 9+ messages in thread
From: Niklas Söderlund @ 2019-07-17 1:13 UTC (permalink / raw)
To: Wolfram Sang
Cc: Ulrich Hecht, linux-renesas-soc, linux-mmc, yamada.masahiro,
geert, ulf.hansson, magnus.damm
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled
2019-07-16 15:01 ` [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled Ulrich Hecht
2019-07-16 19:05 ` Wolfram Sang
@ 2019-07-25 13:43 ` Ulf Hansson
2019-07-29 8:44 ` Geert Uytterhoeven
1 sibling, 1 reply; 9+ messages in thread
From: Ulf Hansson @ 2019-07-25 13:43 UTC (permalink / raw)
To: Ulrich Hecht
Cc: Linux-Renesas, linux-mmc, Niklas Söderlund, Wolfram Sang,
Masahiro Yamada, Geert Uytterhoeven, Magnus Damm
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] mmc: tmio: remove Gen2+ workaround and fix up
2019-07-16 15:01 [PATCH 0/2] mmc: tmio: remove Gen2+ workaround and fix up Ulrich Hecht
2019-07-16 15:01 ` [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled Ulrich Hecht
2019-07-16 15:01 ` [PATCH 2/2] mmc: tmio: remove obsolete PM workaround Ulrich Hecht
@ 2019-07-25 21:15 ` Niklas Söderlund
2 siblings, 0 replies; 9+ messages in thread
From: Niklas Söderlund @ 2019-07-25 21:15 UTC (permalink / raw)
To: Ulrich Hecht
Cc: linux-renesas-soc, linux-mmc, wsa, yamada.masahiro, geert,
ulf.hansson, magnus.damm
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mmc: tmio: remove obsolete PM workaround
2019-07-16 15:01 ` [PATCH 2/2] mmc: tmio: remove obsolete PM workaround Ulrich Hecht
@ 2019-07-29 8:37 ` Geert Uytterhoeven
0 siblings, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2019-07-29 8:37 UTC (permalink / raw)
To: Ulrich Hecht
Cc: Linux-Renesas, Linux MMC List, Niklas Söderlund,
Wolfram Sang, Masahiro Yamada, Ulf Hansson, Magnus Damm
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled
2019-07-25 13:43 ` Ulf Hansson
@ 2019-07-29 8:44 ` Geert Uytterhoeven
0 siblings, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2019-07-29 8:44 UTC (permalink / raw)
To: Ulf Hansson
Cc: Ulrich Hecht, Linux-Renesas, linux-mmc, Niklas Söderlund,
Wolfram Sang, Masahiro Yamada, Magnus Damm
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-29 8:44 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-16 15:01 [PATCH 0/2] mmc: tmio: remove Gen2+ workaround and fix up Ulrich Hecht
2019-07-16 15:01 ` [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled Ulrich Hecht
2019-07-16 19:05 ` Wolfram Sang
2019-07-17 1:13 ` Niklas Söderlund
2019-07-25 13:43 ` Ulf Hansson
2019-07-29 8:44 ` Geert Uytterhoeven
2019-07-16 15:01 ` [PATCH 2/2] mmc: tmio: remove obsolete PM workaround Ulrich Hecht
2019-07-29 8:37 ` Geert Uytterhoeven
2019-07-25 21:15 ` [PATCH 0/2] mmc: tmio: remove Gen2+ workaround and fix up Niklas Söderlund
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).