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

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 3079 bytes --]

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.

> @@ -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?

--- 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.

Thanks, Ulf!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]