diff mbox series

[2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

Message ID 20200515140459.15273-1-ulf.hansson@linaro.org
State Superseded
Headers show
Series [1/2] mmc: tmio: Further fixup runtime PM management at remove | expand

Commit Message

Ulf Hansson May 15, 2020, 2:04 p.m. UTC
If the tmio device is attached to a genpd (PM domain), that genpd may have
->start|stop() callback assigned to it. To make sure the device is
accessible during ->probe(), genpd's ->start() callback must be invoked,
which is currently managed by tmio_mmc_host_probe(). This is very likely to
be too late for some cases, as registers may be read and written way before
that.

To fix this behaviour, let's drop the call to dev_pm_domain_start() from
tmio_mmc_host_probe() - and let the tmio clients manage this instead.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

---
 drivers/mmc/host/renesas_sdhi_core.c | 3 +++
 drivers/mmc/host/tmio_mmc.c          | 3 +++
 drivers/mmc/host/tmio_mmc_core.c     | 2 --
 drivers/mmc/host/uniphier-sd.c       | 3 +++
 4 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.20.1

Comments

Geert Uytterhoeven May 18, 2020, 9:07 p.m. UTC | #1
Hi Ulf,

On Fri, May 15, 2020 at 4:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> If the tmio device is attached to a genpd (PM domain), that genpd may have
> ->start|stop() callback assigned to it. To make sure the device is
> accessible during ->probe(), genpd's ->start() callback must be invoked,
> which is currently managed by tmio_mmc_host_probe(). This is very likely to
> be too late for some cases, as registers may be read and written way before
> that.
>
> To fix this behaviour, let's drop the call to dev_pm_domain_start() from
> tmio_mmc_host_probe() - and let the tmio clients manage this instead.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

So this moves the call to dev_pm_domain_start(). No new calls are added.
If I get it right, dev_pm_domain_start() just calls into
genpd_dev_pm_start() through a function pointer, and starts the device
through the system-specific PM Domain handler.  On R-Car SoCs, that
is pm_clk_resume(), i.e. enabling the module clock through the clock
domain.

I have two questions there:
  1. What if the device is already started?
     There seems to be no reference counting involved.
  2. Who stops the device again?

I always thought the PM Domain was powered on (if still off), and the
device started, by calling pm_runtime_get_sync().

What am I missing?
Thanks!

Gr{oetje,eeting}s,

                        Geert
Ulf Hansson May 19, 2020, 7:50 a.m. UTC | #2
On Mon, 18 May 2020 at 22:22, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> On Fri, May 15, 2020 at 04:04:59PM +0200, Ulf Hansson wrote:
> > If the tmio device is attached to a genpd (PM domain), that genpd may have
> > ->start|stop() callback assigned to it. To make sure the device is
> > accessible during ->probe(), genpd's ->start() callback must be invoked,
> > which is currently managed by tmio_mmc_host_probe(). This is very likely to
> > be too late for some cases, as registers may be read and written way before
> > that.
> >
> > To fix this behaviour, let's drop the call to dev_pm_domain_start() from
> > tmio_mmc_host_probe() - and let the tmio clients manage this instead.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Okay, this seems to work on Gen3.

Great, thanks!

>
> > @@ -909,6 +910,8 @@ int renesas_sdhi_probe(struct platform_device *pdev,
> >       if (ret)
> >               goto efree;
> >
> > +     dev_pm_domain_start(&pdev->dev);
> > +
>
> Can't we put it before the custom clk_enable()? And then clean up
> further like this to have the main clock only controlled via RPM?

I understand what you want to achieve, but to allow that to work we
need to consider the below things first.

1. If the driver is built with CONFIG_PM unset, then runtime PM
doesn't work and hence the clock won't be managed by a PM domain.
2. If there is a platform configuration where a PM domain (genpd)
isn't going to be attached, then the clock needs to be managed locally
in the driver.

>
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -83,16 +83,11 @@ static int renesas_sdhi_clk_enable(struct tmio_mmc_host *host)
>  {
>         struct mmc_host *mmc = host->mmc;
>         struct renesas_sdhi *priv = host_to_priv(host);
> -       int ret = clk_prepare_enable(priv->clk);
> -
> -       if (ret < 0)
> -               return ret;
> +       int ret;
>
>         ret = clk_prepare_enable(priv->clk_cd);
> -       if (ret < 0) {
> -               clk_disable_unprepare(priv->clk);
> +       if (ret < 0)
>                 return ret;
> -       }
>
>         /*
>          * The clock driver may not know what maximum frequency
> @@ -198,7 +193,6 @@ static void renesas_sdhi_clk_disable(struct tmio_mmc_host *host)
>  {
>         struct renesas_sdhi *priv = host_to_priv(host);
>
> -       clk_disable_unprepare(priv->clk);
>         clk_disable_unprepare(priv->clk_cd);
>  }
>
> @@ -906,12 +900,12 @@ int renesas_sdhi_probe(struct platform_device *pdev,
>         /* All SDHI have SDIO status bits which must be 1 */
>         mmc_data->flags |= TMIO_MMC_SDIO_STATUS_SETBITS;
>
> +       dev_pm_domain_start(&pdev->dev);
> +
>         ret = renesas_sdhi_clk_enable(host);
>         if (ret)
>                 goto efree;
>
> -       dev_pm_domain_start(&pdev->dev);
> -
>         ver = sd_ctrl_read16(host, CTL_VERSION);
>         /* GEN2_SDR104 is first known SDHI to use 32bit block count */
>         if (ver < SDHI_VER_GEN2_SDR104 && mmc_data->max_blk_count > U16_MAX)
>
>
> Again, this is only tested on Gen3. I will check Gen2 tomorrow.
>
> > diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
> > index 93e83ad25976..b8f5687e10be 100644
> > --- a/drivers/mmc/host/tmio_mmc.c
> > +++ b/drivers/mmc/host/tmio_mmc.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/mmc/host.h>
> >  #include <linux/module.h>
> >  #include <linux/pagemap.h>
> > +#include <linux/pm_domain.h>
> >  #include <linux/scatterlist.h>
> >
> >  #include "tmio_mmc.h"
> > @@ -172,6 +173,8 @@ static int tmio_mmc_probe(struct platform_device *pdev)
> >       host->mmc->f_max = pdata->hclk;
> >       host->mmc->f_min = pdata->hclk / 512;
> >
> > +     dev_pm_domain_start(&pdev->dev);
> > +
>
> I am quite sure tmio_mmc won't need this, but better safe than sorry.

Okay!

I observed that tmio_mmc doesn't manage external clocks and don't have
->clk_enable|disable() callbacks, but using runtime PM.

That made me think that perhaps the clocks were managed through the PM domain.

Kind regards
Uffe
Geert Uytterhoeven May 19, 2020, 8:29 a.m. UTC | #3
Hi Ulf,

On Tue, May 19, 2020 at 10:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Mon, 18 May 2020 at 23:07, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, May 15, 2020 at 4:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > If the tmio device is attached to a genpd (PM domain), that genpd may have
> > > ->start|stop() callback assigned to it. To make sure the device is
> > > accessible during ->probe(), genpd's ->start() callback must be invoked,
> > > which is currently managed by tmio_mmc_host_probe(). This is very likely to
> > > be too late for some cases, as registers may be read and written way before
> > > that.
> > >
> > > To fix this behaviour, let's drop the call to dev_pm_domain_start() from
> > > tmio_mmc_host_probe() - and let the tmio clients manage this instead.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> >
> > So this moves the call to dev_pm_domain_start(). No new calls are added.
> > If I get it right, dev_pm_domain_start() just calls into
> > genpd_dev_pm_start() through a function pointer, and starts the device
> > through the system-specific PM Domain handler.  On R-Car SoCs, that
> > is pm_clk_resume(), i.e. enabling the module clock through the clock
> > domain.
>
> Correct.
>
> > I have two questions there:
> >   1. What if the device is already started?
> >      There seems to be no reference counting involved.
>
> The device can't be started as runtime PM hasn't been enabled yet for
> the device - and this is controlled by the tmio/renesas driver.

OK.

> >   2. Who stops the device again?
>
> Beyond this point there are two main cases to consider.
>
> 1. The driver is probed successfully and thus up and running. Then,
> when the device becomes runtime suspended (because of request
> inactivity, for example), the device will be "stopped" through genpd.

OK.

> 2. The driver failed to probe or the ->remove() callback is invoked
> for the device. This will trigger the platform bus to call
> dev_pm_domain_detach(). In this path, genpd invokes the
> genpd->detach_dev() callback for the device, which allows the genpd
> provider to deal with the clean up. In this particular case, I assume
> pm_clk_destroy() is going to be called for the device.

OK.

> > I always thought the PM Domain was powered on (if still off), and the
> > device started, by calling pm_runtime_get_sync().
>
> Correct.
>
> However, deploying that kind of pattern in a driver can be a bit
> messy, while considering that CONFIG_PM may be set or unset and the
> driver should work in both configurations. In principle, it leads to
> boilerplate code in drivers, especially if the driver has runtime PM
> callbacks assigned to it as shown in commit 1b32999e205b ("mmc: tmio:
> Avoid boilerplate code in ->runtime_suspend()").
>
> Does it make sense?

Now it does, thanks!

Note that for devices handled by renesas_sdhi, CONFIG_PM is always
set (cfr. drivers/soc/renesas/Kconfig), and there will always by a genpd.
So the "messy" part matters for TMIO and UniPhier only.

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven May 19, 2020, 8:53 a.m. UTC | #4
Hi Wolfram,

On Tue, May 19, 2020 at 10:46 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > Can't we put it before the custom clk_enable()? And then clean up
> > > further like this to have the main clock only controlled via RPM?
> >
> > I understand what you want to achieve, but to allow that to work we
> > need to consider the below things first.
> >
> > 1. If the driver is built with CONFIG_PM unset, then runtime PM
> > doesn't work and hence the clock won't be managed by a PM domain.
> > 2. If there is a platform configuration where a PM domain (genpd)
> > isn't going to be attached, then the clock needs to be managed locally
> > in the driver.
>
> Similar to what Geert responded, for Renesas SDHI both is always true
> AFAIU. Geert?

For Renesas SDHI, both are always false ;-)
I.e. CONFIG_PM is always set, genpd is always attached.

Gr{oetje,eeting}s,

                        Geert
Ulf Hansson May 19, 2020, 9:15 a.m. UTC | #5
On Tue, 19 May 2020 at 10:53, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Wolfram,
>
> On Tue, May 19, 2020 at 10:46 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > > > Can't we put it before the custom clk_enable()? And then clean up
> > > > further like this to have the main clock only controlled via RPM?
> > >
> > > I understand what you want to achieve, but to allow that to work we
> > > need to consider the below things first.
> > >
> > > 1. If the driver is built with CONFIG_PM unset, then runtime PM
> > > doesn't work and hence the clock won't be managed by a PM domain.
> > > 2. If there is a platform configuration where a PM domain (genpd)
> > > isn't going to be attached, then the clock needs to be managed locally
> > > in the driver.
> >
> > Similar to what Geert responded, for Renesas SDHI both is always true
> > AFAIU. Geert?
>
> For Renesas SDHI, both are always false ;-)
> I.e. CONFIG_PM is always set, genpd is always attached.

OK, thanks for clarifying.

This means dev_pm_domain_start() is needed only for the SDHI renesas
variants. But on the other hand, it doesn't hurt for the others (the
uniphier-sd variant doesn't even use runtime PM as confirmed by
Yamada-san).

I don't have a strong opinion, but it looks like we can either apply
$subject patch as is, or modify it to make dev_pm_domain_start() be
called only for the SDHI renesas variants.

What do you prefer?

Kind regards
Uffe
Wolfram Sang May 19, 2020, 11:32 a.m. UTC | #6
> And no matter what solution, can always drop to manage the "main"
> clock from renesas_sdhi_clk_enable|disable() as it's managed by the PM
> domain.

Good. That was what I was aiming for.

> 
> Although, then we need to call dev_pm_domain_start() prior
> renesas_sdhi_clk_enable() during ->probe()?

Yep, I moved it upwards in my proof-of-concept diff. But it would be
better if you would apply this change in your patch already.
Wolfram Sang May 19, 2020, 11:35 a.m. UTC | #7
> This means dev_pm_domain_start() is needed only for the SDHI renesas
> variants. But on the other hand, it doesn't hurt for the others (the
> uniphier-sd variant doesn't even use runtime PM as confirmed by
> Yamada-san).
> 
> I don't have a strong opinion, but it looks like we can either apply
> $subject patch as is, or modify it to make dev_pm_domain_start() be
> called only for the SDHI renesas variants.
> 
> What do you prefer?

Dealing with PM is confusing often enough. To keep it simple, I'd
suggest to avoid unneeded calls. It may take a while later to rediscover
if this call is essential or not. So, for the uniphier case, we know it
is not needed. If we agree on a best effort basis that it is also not
needed for tmio_mmc, then I think we should keep it only for Renesas
SDHI.
diff mbox series

Patch

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index ff72b381a6b3..5d44240d57ec 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -24,6 +24,7 @@ 
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/slot-gpio.h>
 #include <linux/mfd/tmio.h>
@@ -909,6 +910,8 @@  int renesas_sdhi_probe(struct platform_device *pdev,
 	if (ret)
 		goto efree;
 
+	dev_pm_domain_start(&pdev->dev);
+
 	ver = sd_ctrl_read16(host, CTL_VERSION);
 	/* GEN2_SDR104 is first known SDHI to use 32bit block count */
 	if (ver < SDHI_VER_GEN2_SDR104 && mmc_data->max_blk_count > U16_MAX)
diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
index 93e83ad25976..b8f5687e10be 100644
--- a/drivers/mmc/host/tmio_mmc.c
+++ b/drivers/mmc/host/tmio_mmc.c
@@ -17,6 +17,7 @@ 
 #include <linux/mmc/host.h>
 #include <linux/module.h>
 #include <linux/pagemap.h>
+#include <linux/pm_domain.h>
 #include <linux/scatterlist.h>
 
 #include "tmio_mmc.h"
@@ -172,6 +173,8 @@  static int tmio_mmc_probe(struct platform_device *pdev)
 	host->mmc->f_max = pdata->hclk;
 	host->mmc->f_min = pdata->hclk / 512;
 
+	dev_pm_domain_start(&pdev->dev);
+
 	ret = tmio_mmc_host_probe(host);
 	if (ret)
 		goto host_free;
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index ba301fb7656b..d7fde57c78c1 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -39,7 +39,6 @@ 
 #include <linux/module.h>
 #include <linux/pagemap.h>
 #include <linux/platform_device.h>
-#include <linux/pm_domain.h>
 #include <linux/pm_qos.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
@@ -1192,7 +1191,6 @@  int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
 	/* See if we also get DMA */
 	tmio_mmc_request_dma(_host, pdata);
 
-	dev_pm_domain_start(&pdev->dev);
 	pm_runtime_get_noresume(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
diff --git a/drivers/mmc/host/uniphier-sd.c b/drivers/mmc/host/uniphier-sd.c
index f82baf99fd69..0c41dc1cc96c 100644
--- a/drivers/mmc/host/uniphier-sd.c
+++ b/drivers/mmc/host/uniphier-sd.c
@@ -15,6 +15,7 @@ 
 #include <linux/of_device.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 #include <linux/reset.h>
 
 #include "tmio_mmc.h"
@@ -624,6 +625,8 @@  static int uniphier_sd_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_host;
 
+	dev_pm_domain_start(dev);
+
 	uniphier_sd_host_init(host);
 
 	tmio_data->ocr_mask = MMC_VDD_32_33 | MMC_VDD_33_34;