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

[Ulf Hansson <ulf.hansson@linaro.org>]

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