On Mon, 18 May 2020 at 22:22, Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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
