[PATCH 1/2] mmc: sdhci-of-at91: factor out clks and presets setting

[PATCH 1/2] mmc: sdhci-of-at91: factor out clks and presets setting

[Quentin Schulz]

The setting of clocks and presets is currently done in probe only but
once deep PM support is added, it'll be needed in the resume function.

Let's create a function for this setting.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 drivers/mmc/host/sdhci-of-at91.c | 147 ++++++++++++++++++++++-----------------
 1 file changed, 82 insertions(+), 65 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index 7611fd679f1a..fb8c6011f13d 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -128,6 +128,84 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, sdhci_at91_dt_match);
 
+static int sdhci_at91_set_clks_presets(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	int ret;
+	unsigned int			caps0, caps1;
+	unsigned int			clk_base, clk_mul;
+	unsigned int			gck_rate, real_gck_rate;
+	unsigned int			preset_div;
+
+	/*
+	 * The mult clock is provided by as a generated clock by the PMC
+	 * controller. In order to set the rate of gck, we have to get the
+	 * base clock rate and the clock mult from capabilities.
+	 */
+	clk_prepare_enable(priv->hclock);
+	caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES);
+	caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1);
+	clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
+	clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT;
+	gck_rate = clk_base * 1000000 * (clk_mul + 1);
+	ret = clk_set_rate(priv->gck, gck_rate);
+	if (ret < 0) {
+		dev_err(dev, "failed to set gck");
+		clk_disable_unprepare(priv->hclock);
+		return ret;
+	}
+	/*
+	 * We need to check if we have the requested rate for gck because in
+	 * some cases this rate could be not supported. If it happens, the rate
+	 * is the closest one gck can provide. We have to update the value
+	 * of clk mul.
+	 */
+	real_gck_rate = clk_get_rate(priv->gck);
+	if (real_gck_rate != gck_rate) {
+		clk_mul = real_gck_rate / (clk_base * 1000000) - 1;
+		caps1 &= (~SDHCI_CLOCK_MUL_MASK);
+		caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) &
+			  SDHCI_CLOCK_MUL_MASK);
+		/* Set capabilities in r/w mode. */
+		writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN,
+		       host->ioaddr + SDMMC_CACR);
+		writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1);
+		/* Set capabilities in ro mode. */
+		writel(0, host->ioaddr + SDMMC_CACR);
+		dev_info(dev, "update clk mul to %u as gck rate is %u Hz\n",
+			 clk_mul, real_gck_rate);
+	}
+
+	/*
+	 * We have to set preset values because it depends on the clk_mul
+	 * value. Moreover, SDR104 is supported in a degraded mode since the
+	 * maximum sd clock value is 120 MHz instead of 208 MHz. For that
+	 * reason, we need to use presets to support SDR104.
+	 */
+	preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1;
+	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
+	       host->ioaddr + SDHCI_PRESET_FOR_SDR12);
+	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
+	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
+	       host->ioaddr + SDHCI_PRESET_FOR_SDR25);
+	preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1;
+	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
+	       host->ioaddr + SDHCI_PRESET_FOR_SDR50);
+	preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1;
+	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
+	       host->ioaddr + SDHCI_PRESET_FOR_SDR104);
+	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
+	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
+	       host->ioaddr + SDHCI_PRESET_FOR_DDR50);
+
+	clk_prepare_enable(priv->mainck);
+	clk_prepare_enable(priv->gck);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM
 static int sdhci_at91_runtime_suspend(struct device *dev)
 {
@@ -192,11 +270,7 @@ static int sdhci_at91_probe(struct platform_device *pdev)
 	struct sdhci_host		*host;
 	struct sdhci_pltfm_host		*pltfm_host;
 	struct sdhci_at91_priv		*priv;
-	unsigned int			caps0, caps1;
-	unsigned int			clk_base, clk_mul;
-	unsigned int			gck_rate, real_gck_rate;
 	int				ret;
-	unsigned int			preset_div;
 
 	match = of_match_device(sdhci_at91_dt_match, &pdev->dev);
 	if (!match)
@@ -228,66 +302,9 @@ static int sdhci_at91_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->gck);
 	}
 
-	/*
-	 * The mult clock is provided by as a generated clock by the PMC
-	 * controller. In order to set the rate of gck, we have to get the
-	 * base clock rate and the clock mult from capabilities.
-	 */
-	clk_prepare_enable(priv->hclock);
-	caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES);
-	caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1);
-	clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
-	clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT;
-	gck_rate = clk_base * 1000000 * (clk_mul + 1);
-	ret = clk_set_rate(priv->gck, gck_rate);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to set gck");
-		goto hclock_disable_unprepare;
-	}
-	/*
-	 * We need to check if we have the requested rate for gck because in
-	 * some cases this rate could be not supported. If it happens, the rate
-	 * is the closest one gck can provide. We have to update the value
-	 * of clk mul.
-	 */
-	real_gck_rate = clk_get_rate(priv->gck);
-	if (real_gck_rate != gck_rate) {
-		clk_mul = real_gck_rate / (clk_base * 1000000) - 1;
-		caps1 &= (~SDHCI_CLOCK_MUL_MASK);
-		caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) & SDHCI_CLOCK_MUL_MASK);
-		/* Set capabilities in r/w mode. */
-		writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN, host->ioaddr + SDMMC_CACR);
-		writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1);
-		/* Set capabilities in ro mode. */
-		writel(0, host->ioaddr + SDMMC_CACR);
-		dev_info(&pdev->dev, "update clk mul to %u as gck rate is %u Hz\n",
-			 clk_mul, real_gck_rate);
-	}
-
-	/*
-	 * We have to set preset values because it depends on the clk_mul
-	 * value. Moreover, SDR104 is supported in a degraded mode since the
-	 * maximum sd clock value is 120 MHz instead of 208 MHz. For that
-	 * reason, we need to use presets to support SDR104.
-	 */
-	preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1;
-	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
-	       host->ioaddr + SDHCI_PRESET_FOR_SDR12);
-	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
-	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
-	       host->ioaddr + SDHCI_PRESET_FOR_SDR25);
-	preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1;
-	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
-	       host->ioaddr + SDHCI_PRESET_FOR_SDR50);
-	preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1;
-	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
-	       host->ioaddr + SDHCI_PRESET_FOR_SDR104);
-	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
-	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
-	       host->ioaddr + SDHCI_PRESET_FOR_DDR50);
-
-	clk_prepare_enable(priv->mainck);
-	clk_prepare_enable(priv->gck);
+	ret = sdhci_at91_set_clks_presets(&pdev->dev);
+	if (ret)
+		goto sdhci_pltfm_free;
 
 	ret = mmc_of_parse(host->mmc);
 	if (ret)
@@ -335,8 +352,8 @@ static int sdhci_at91_probe(struct platform_device *pdev)
 clocks_disable_unprepare:
 	clk_disable_unprepare(priv->gck);
 	clk_disable_unprepare(priv->mainck);
-hclock_disable_unprepare:
 	clk_disable_unprepare(priv->hclock);
+sdhci_pltfm_free:
 	sdhci_pltfm_free(pdev);
 	return ret;
 }
-- 
2.11.0

[PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Quentin Schulz]

This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
SoC's SDHCI controller.

When resuming from deepest state, it is required to restore preset
registers as the registers are lost since VDD core has been shut down
when entering deepest state on the SAMA5D2. The clocks need to be
reconfigured as well.

The other registers and init process are taken care of by the SDHCI
core.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index fb8c6011f13d..300513fc1068 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
 }
 
 #ifdef CONFIG_PM
+static int sdhci_at91_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	int ret;
+
+	ret = sdhci_suspend_host(host);
+
+	if (host->runtime_suspended)
+		return ret;
+
+	clk_disable_unprepare(priv->gck);
+	clk_disable_unprepare(priv->hclock);
+	clk_disable_unprepare(priv->mainck);
+
+	return ret;
+}
+
+static int sdhci_at91_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	int ret;
+
+	ret = sdhci_at91_set_clks_presets(dev);
+	if (ret)
+		return ret;
+
+	return sdhci_resume_host(host);
+}
+
 static int sdhci_at91_runtime_suspend(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
@@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
 #endif /* CONFIG_PM */
 
 static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
-				pm_runtime_force_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume)
 	SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend,
 			   sdhci_at91_runtime_resume,
 			   NULL)
-- 
2.11.0

Re: [PATCH 1/2] mmc: sdhci-of-at91: factor out clks and presets setting

[Ludovic Desroches]

On Fri, Jun 16, 2017 at 09:29:28AM +0200, Quentin Schulz wrote:
> The setting of clocks and presets is currently done in probe only but
> once deep PM support is added, it'll be needed in the resume function.
> 
> Let's create a function for this setting.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>

Thanks

> ---
>  drivers/mmc/host/sdhci-of-at91.c | 147 ++++++++++++++++++++++-----------------
>  1 file changed, 82 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index 7611fd679f1a..fb8c6011f13d 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -128,6 +128,84 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, sdhci_at91_dt_match);
>  
> +static int sdhci_at91_set_clks_presets(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	int ret;
> +	unsigned int			caps0, caps1;
> +	unsigned int			clk_base, clk_mul;
> +	unsigned int			gck_rate, real_gck_rate;
> +	unsigned int			preset_div;
> +
> +	/*
> +	 * The mult clock is provided by as a generated clock by the PMC
> +	 * controller. In order to set the rate of gck, we have to get the
> +	 * base clock rate and the clock mult from capabilities.
> +	 */
> +	clk_prepare_enable(priv->hclock);
> +	caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES);
> +	caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1);
> +	clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
> +	clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT;
> +	gck_rate = clk_base * 1000000 * (clk_mul + 1);
> +	ret = clk_set_rate(priv->gck, gck_rate);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to set gck");
> +		clk_disable_unprepare(priv->hclock);
> +		return ret;
> +	}
> +	/*
> +	 * We need to check if we have the requested rate for gck because in
> +	 * some cases this rate could be not supported. If it happens, the rate
> +	 * is the closest one gck can provide. We have to update the value
> +	 * of clk mul.
> +	 */
> +	real_gck_rate = clk_get_rate(priv->gck);
> +	if (real_gck_rate != gck_rate) {
> +		clk_mul = real_gck_rate / (clk_base * 1000000) - 1;
> +		caps1 &= (~SDHCI_CLOCK_MUL_MASK);
> +		caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) &
> +			  SDHCI_CLOCK_MUL_MASK);
> +		/* Set capabilities in r/w mode. */
> +		writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN,
> +		       host->ioaddr + SDMMC_CACR);
> +		writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1);
> +		/* Set capabilities in ro mode. */
> +		writel(0, host->ioaddr + SDMMC_CACR);
> +		dev_info(dev, "update clk mul to %u as gck rate is %u Hz\n",
> +			 clk_mul, real_gck_rate);
> +	}
> +
> +	/*
> +	 * We have to set preset values because it depends on the clk_mul
> +	 * value. Moreover, SDR104 is supported in a degraded mode since the
> +	 * maximum sd clock value is 120 MHz instead of 208 MHz. For that
> +	 * reason, we need to use presets to support SDR104.
> +	 */
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR12);
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR25);
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR50);
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR104);
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_DDR50);
> +
> +	clk_prepare_enable(priv->mainck);
> +	clk_prepare_enable(priv->gck);
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_PM
>  static int sdhci_at91_runtime_suspend(struct device *dev)
>  {
> @@ -192,11 +270,7 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>  	struct sdhci_host		*host;
>  	struct sdhci_pltfm_host		*pltfm_host;
>  	struct sdhci_at91_priv		*priv;
> -	unsigned int			caps0, caps1;
> -	unsigned int			clk_base, clk_mul;
> -	unsigned int			gck_rate, real_gck_rate;
>  	int				ret;
> -	unsigned int			preset_div;
>  
>  	match = of_match_device(sdhci_at91_dt_match, &pdev->dev);
>  	if (!match)
> @@ -228,66 +302,9 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>  		return PTR_ERR(priv->gck);
>  	}
>  
> -	/*
> -	 * The mult clock is provided by as a generated clock by the PMC
> -	 * controller. In order to set the rate of gck, we have to get the
> -	 * base clock rate and the clock mult from capabilities.
> -	 */
> -	clk_prepare_enable(priv->hclock);
> -	caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES);
> -	caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1);
> -	clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
> -	clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT;
> -	gck_rate = clk_base * 1000000 * (clk_mul + 1);
> -	ret = clk_set_rate(priv->gck, gck_rate);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to set gck");
> -		goto hclock_disable_unprepare;
> -	}
> -	/*
> -	 * We need to check if we have the requested rate for gck because in
> -	 * some cases this rate could be not supported. If it happens, the rate
> -	 * is the closest one gck can provide. We have to update the value
> -	 * of clk mul.
> -	 */
> -	real_gck_rate = clk_get_rate(priv->gck);
> -	if (real_gck_rate != gck_rate) {
> -		clk_mul = real_gck_rate / (clk_base * 1000000) - 1;
> -		caps1 &= (~SDHCI_CLOCK_MUL_MASK);
> -		caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) & SDHCI_CLOCK_MUL_MASK);
> -		/* Set capabilities in r/w mode. */
> -		writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN, host->ioaddr + SDMMC_CACR);
> -		writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1);
> -		/* Set capabilities in ro mode. */
> -		writel(0, host->ioaddr + SDMMC_CACR);
> -		dev_info(&pdev->dev, "update clk mul to %u as gck rate is %u Hz\n",
> -			 clk_mul, real_gck_rate);
> -	}
> -
> -	/*
> -	 * We have to set preset values because it depends on the clk_mul
> -	 * value. Moreover, SDR104 is supported in a degraded mode since the
> -	 * maximum sd clock value is 120 MHz instead of 208 MHz. For that
> -	 * reason, we need to use presets to support SDR104.
> -	 */
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR12);
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR25);
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR50);
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR104);
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_DDR50);
> -
> -	clk_prepare_enable(priv->mainck);
> -	clk_prepare_enable(priv->gck);
> +	ret = sdhci_at91_set_clks_presets(&pdev->dev);
> +	if (ret)
> +		goto sdhci_pltfm_free;
>  
>  	ret = mmc_of_parse(host->mmc);
>  	if (ret)
> @@ -335,8 +352,8 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>  clocks_disable_unprepare:
>  	clk_disable_unprepare(priv->gck);
>  	clk_disable_unprepare(priv->mainck);
> -hclock_disable_unprepare:
>  	clk_disable_unprepare(priv->hclock);
> +sdhci_pltfm_free:
>  	sdhci_pltfm_free(pdev);
>  	return ret;
>  }
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mmc: sdhci-of-at91: factor out clks and presets setting

[Ludovic Desroches]

On Fri, Jun 16, 2017 at 09:29:28AM +0200, Quentin Schulz wrote:
> The setting of clocks and presets is currently done in probe only but
> once deep PM support is added, it'll be needed in the resume function.
> 
> Let's create a function for this setting.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>

Thanks

> ---
>  drivers/mmc/host/sdhci-of-at91.c | 147 ++++++++++++++++++++++-----------------
>  1 file changed, 82 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index 7611fd679f1a..fb8c6011f13d 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -128,6 +128,84 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, sdhci_at91_dt_match);
>  
> +static int sdhci_at91_set_clks_presets(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	int ret;
> +	unsigned int			caps0, caps1;
> +	unsigned int			clk_base, clk_mul;
> +	unsigned int			gck_rate, real_gck_rate;
> +	unsigned int			preset_div;
> +
> +	/*
> +	 * The mult clock is provided by as a generated clock by the PMC
> +	 * controller. In order to set the rate of gck, we have to get the
> +	 * base clock rate and the clock mult from capabilities.
> +	 */
> +	clk_prepare_enable(priv->hclock);
> +	caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES);
> +	caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1);
> +	clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
> +	clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT;
> +	gck_rate = clk_base * 1000000 * (clk_mul + 1);
> +	ret = clk_set_rate(priv->gck, gck_rate);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to set gck");
> +		clk_disable_unprepare(priv->hclock);
> +		return ret;
> +	}
> +	/*
> +	 * We need to check if we have the requested rate for gck because in
> +	 * some cases this rate could be not supported. If it happens, the rate
> +	 * is the closest one gck can provide. We have to update the value
> +	 * of clk mul.
> +	 */
> +	real_gck_rate = clk_get_rate(priv->gck);
> +	if (real_gck_rate != gck_rate) {
> +		clk_mul = real_gck_rate / (clk_base * 1000000) - 1;
> +		caps1 &= (~SDHCI_CLOCK_MUL_MASK);
> +		caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) &
> +			  SDHCI_CLOCK_MUL_MASK);
> +		/* Set capabilities in r/w mode. */
> +		writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN,
> +		       host->ioaddr + SDMMC_CACR);
> +		writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1);
> +		/* Set capabilities in ro mode. */
> +		writel(0, host->ioaddr + SDMMC_CACR);
> +		dev_info(dev, "update clk mul to %u as gck rate is %u Hz\n",
> +			 clk_mul, real_gck_rate);
> +	}
> +
> +	/*
> +	 * We have to set preset values because it depends on the clk_mul
> +	 * value. Moreover, SDR104 is supported in a degraded mode since the
> +	 * maximum sd clock value is 120 MHz instead of 208 MHz. For that
> +	 * reason, we need to use presets to support SDR104.
> +	 */
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR12);
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR25);
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR50);
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR104);
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_DDR50);
> +
> +	clk_prepare_enable(priv->mainck);
> +	clk_prepare_enable(priv->gck);
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_PM
>  static int sdhci_at91_runtime_suspend(struct device *dev)
>  {
> @@ -192,11 +270,7 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>  	struct sdhci_host		*host;
>  	struct sdhci_pltfm_host		*pltfm_host;
>  	struct sdhci_at91_priv		*priv;
> -	unsigned int			caps0, caps1;
> -	unsigned int			clk_base, clk_mul;
> -	unsigned int			gck_rate, real_gck_rate;
>  	int				ret;
> -	unsigned int			preset_div;
>  
>  	match = of_match_device(sdhci_at91_dt_match, &pdev->dev);
>  	if (!match)
> @@ -228,66 +302,9 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>  		return PTR_ERR(priv->gck);
>  	}
>  
> -	/*
> -	 * The mult clock is provided by as a generated clock by the PMC
> -	 * controller. In order to set the rate of gck, we have to get the
> -	 * base clock rate and the clock mult from capabilities.
> -	 */
> -	clk_prepare_enable(priv->hclock);
> -	caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES);
> -	caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1);
> -	clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
> -	clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT;
> -	gck_rate = clk_base * 1000000 * (clk_mul + 1);
> -	ret = clk_set_rate(priv->gck, gck_rate);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to set gck");
> -		goto hclock_disable_unprepare;
> -	}
> -	/*
> -	 * We need to check if we have the requested rate for gck because in
> -	 * some cases this rate could be not supported. If it happens, the rate
> -	 * is the closest one gck can provide. We have to update the value
> -	 * of clk mul.
> -	 */
> -	real_gck_rate = clk_get_rate(priv->gck);
> -	if (real_gck_rate != gck_rate) {
> -		clk_mul = real_gck_rate / (clk_base * 1000000) - 1;
> -		caps1 &= (~SDHCI_CLOCK_MUL_MASK);
> -		caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) & SDHCI_CLOCK_MUL_MASK);
> -		/* Set capabilities in r/w mode. */
> -		writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN, host->ioaddr + SDMMC_CACR);
> -		writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1);
> -		/* Set capabilities in ro mode. */
> -		writel(0, host->ioaddr + SDMMC_CACR);
> -		dev_info(&pdev->dev, "update clk mul to %u as gck rate is %u Hz\n",
> -			 clk_mul, real_gck_rate);
> -	}
> -
> -	/*
> -	 * We have to set preset values because it depends on the clk_mul
> -	 * value. Moreover, SDR104 is supported in a degraded mode since the
> -	 * maximum sd clock value is 120 MHz instead of 208 MHz. For that
> -	 * reason, we need to use presets to support SDR104.
> -	 */
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR12);
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR25);
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR50);
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR104);
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_DDR50);
> -
> -	clk_prepare_enable(priv->mainck);
> -	clk_prepare_enable(priv->gck);
> +	ret = sdhci_at91_set_clks_presets(&pdev->dev);
> +	if (ret)
> +		goto sdhci_pltfm_free;
>  
>  	ret = mmc_of_parse(host->mmc);
>  	if (ret)
> @@ -335,8 +352,8 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>  clocks_disable_unprepare:
>  	clk_disable_unprepare(priv->gck);
>  	clk_disable_unprepare(priv->mainck);
> -hclock_disable_unprepare:
>  	clk_disable_unprepare(priv->hclock);
> +sdhci_pltfm_free:
>  	sdhci_pltfm_free(pdev);
>  	return ret;
>  }
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Ludovic Desroches]

On Fri, Jun 16, 2017 at 09:29:29AM +0200, Quentin Schulz wrote:
> This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
> SoC's SDHCI controller.
> 
> When resuming from deepest state, it is required to restore preset
> registers as the registers are lost since VDD core has been shut down
> when entering deepest state on the SAMA5D2. The clocks need to be
> reconfigured as well.
> 
> The other registers and init process are taken care of by the SDHCI
> core.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> ---
>  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index fb8c6011f13d..300513fc1068 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
>  }
>  
>  #ifdef CONFIG_PM
> +static int sdhci_at91_suspend(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	int ret;
> +
> +	ret = sdhci_suspend_host(host);
> +
> +	if (host->runtime_suspended)
> +		return ret;
> +
> +	clk_disable_unprepare(priv->gck);
> +	clk_disable_unprepare(priv->hclock);
> +	clk_disable_unprepare(priv->mainck);
> +
> +	return ret;
> +}
> +
> +static int sdhci_at91_resume(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = sdhci_at91_set_clks_presets(dev);
> +	if (ret)
> +		return ret;
> +
> +	return sdhci_resume_host(host);
> +}
> +
>  static int sdhci_at91_runtime_suspend(struct device *dev)
>  {
>  	struct sdhci_host *host = dev_get_drvdata(dev);
> @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
>  #endif /* CONFIG_PM */
>  
>  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> -				pm_runtime_force_resume)
> +	SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume)
>  	SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend,
>  			   sdhci_at91_runtime_resume,
>  			   NULL)
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Ludovic Desroches]

On Fri, Jun 16, 2017 at 09:29:29AM +0200, Quentin Schulz wrote:
> This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
> SoC's SDHCI controller.
> 
> When resuming from deepest state, it is required to restore preset
> registers as the registers are lost since VDD core has been shut down
> when entering deepest state on the SAMA5D2. The clocks need to be
> reconfigured as well.
> 
> The other registers and init process are taken care of by the SDHCI
> core.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> ---
>  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index fb8c6011f13d..300513fc1068 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
>  }
>  
>  #ifdef CONFIG_PM
> +static int sdhci_at91_suspend(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	int ret;
> +
> +	ret = sdhci_suspend_host(host);
> +
> +	if (host->runtime_suspended)
> +		return ret;
> +
> +	clk_disable_unprepare(priv->gck);
> +	clk_disable_unprepare(priv->hclock);
> +	clk_disable_unprepare(priv->mainck);
> +
> +	return ret;
> +}
> +
> +static int sdhci_at91_resume(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = sdhci_at91_set_clks_presets(dev);
> +	if (ret)
> +		return ret;
> +
> +	return sdhci_resume_host(host);
> +}
> +
>  static int sdhci_at91_runtime_suspend(struct device *dev)
>  {
>  	struct sdhci_host *host = dev_get_drvdata(dev);
> @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
>  #endif /* CONFIG_PM */
>  
>  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> -				pm_runtime_force_resume)
> +	SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume)
>  	SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend,
>  			   sdhci_at91_runtime_resume,
>  			   NULL)
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mmc: sdhci-of-at91: factor out clks and presets setting

[Adrian Hunter]

On 16/06/17 10:29, Quentin Schulz wrote:
> The setting of clocks and presets is currently done in probe only but
> once deep PM support is added, it'll be needed in the resume function.
> 
> Let's create a function for this setting.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>

Apart from cosmetic comment below:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-of-at91.c | 147 ++++++++++++++++++++++-----------------
>  1 file changed, 82 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index 7611fd679f1a..fb8c6011f13d 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -128,6 +128,84 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, sdhci_at91_dt_match);
>  
> +static int sdhci_at91_set_clks_presets(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	int ret;
> +	unsigned int			caps0, caps1;
> +	unsigned int			clk_base, clk_mul;
> +	unsigned int			gck_rate, real_gck_rate;
> +	unsigned int			preset_div;

Too much whitespace.

> +
> +	/*
> +	 * The mult clock is provided by as a generated clock by the PMC
> +	 * controller. In order to set the rate of gck, we have to get the
> +	 * base clock rate and the clock mult from capabilities.
> +	 */
> +	clk_prepare_enable(priv->hclock);
> +	caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES);
> +	caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1);
> +	clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
> +	clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT;
> +	gck_rate = clk_base * 1000000 * (clk_mul + 1);
> +	ret = clk_set_rate(priv->gck, gck_rate);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to set gck");
> +		clk_disable_unprepare(priv->hclock);
> +		return ret;
> +	}
> +	/*
> +	 * We need to check if we have the requested rate for gck because in
> +	 * some cases this rate could be not supported. If it happens, the rate
> +	 * is the closest one gck can provide. We have to update the value
> +	 * of clk mul.
> +	 */
> +	real_gck_rate = clk_get_rate(priv->gck);
> +	if (real_gck_rate != gck_rate) {
> +		clk_mul = real_gck_rate / (clk_base * 1000000) - 1;
> +		caps1 &= (~SDHCI_CLOCK_MUL_MASK);
> +		caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) &
> +			  SDHCI_CLOCK_MUL_MASK);
> +		/* Set capabilities in r/w mode. */
> +		writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN,
> +		       host->ioaddr + SDMMC_CACR);
> +		writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1);
> +		/* Set capabilities in ro mode. */
> +		writel(0, host->ioaddr + SDMMC_CACR);
> +		dev_info(dev, "update clk mul to %u as gck rate is %u Hz\n",
> +			 clk_mul, real_gck_rate);
> +	}
> +
> +	/*
> +	 * We have to set preset values because it depends on the clk_mul
> +	 * value. Moreover, SDR104 is supported in a degraded mode since the
> +	 * maximum sd clock value is 120 MHz instead of 208 MHz. For that
> +	 * reason, we need to use presets to support SDR104.
> +	 */
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR12);
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR25);
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR50);
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR104);
> +	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> +	       host->ioaddr + SDHCI_PRESET_FOR_DDR50);
> +
> +	clk_prepare_enable(priv->mainck);
> +	clk_prepare_enable(priv->gck);
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_PM
>  static int sdhci_at91_runtime_suspend(struct device *dev)
>  {
> @@ -192,11 +270,7 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>  	struct sdhci_host		*host;
>  	struct sdhci_pltfm_host		*pltfm_host;
>  	struct sdhci_at91_priv		*priv;
> -	unsigned int			caps0, caps1;
> -	unsigned int			clk_base, clk_mul;
> -	unsigned int			gck_rate, real_gck_rate;
>  	int				ret;
> -	unsigned int			preset_div;
>  
>  	match = of_match_device(sdhci_at91_dt_match, &pdev->dev);
>  	if (!match)
> @@ -228,66 +302,9 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>  		return PTR_ERR(priv->gck);
>  	}
>  
> -	/*
> -	 * The mult clock is provided by as a generated clock by the PMC
> -	 * controller. In order to set the rate of gck, we have to get the
> -	 * base clock rate and the clock mult from capabilities.
> -	 */
> -	clk_prepare_enable(priv->hclock);
> -	caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES);
> -	caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1);
> -	clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
> -	clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT;
> -	gck_rate = clk_base * 1000000 * (clk_mul + 1);
> -	ret = clk_set_rate(priv->gck, gck_rate);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to set gck");
> -		goto hclock_disable_unprepare;
> -	}
> -	/*
> -	 * We need to check if we have the requested rate for gck because in
> -	 * some cases this rate could be not supported. If it happens, the rate
> -	 * is the closest one gck can provide. We have to update the value
> -	 * of clk mul.
> -	 */
> -	real_gck_rate = clk_get_rate(priv->gck);
> -	if (real_gck_rate != gck_rate) {
> -		clk_mul = real_gck_rate / (clk_base * 1000000) - 1;
> -		caps1 &= (~SDHCI_CLOCK_MUL_MASK);
> -		caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) & SDHCI_CLOCK_MUL_MASK);
> -		/* Set capabilities in r/w mode. */
> -		writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN, host->ioaddr + SDMMC_CACR);
> -		writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1);
> -		/* Set capabilities in ro mode. */
> -		writel(0, host->ioaddr + SDMMC_CACR);
> -		dev_info(&pdev->dev, "update clk mul to %u as gck rate is %u Hz\n",
> -			 clk_mul, real_gck_rate);
> -	}
> -
> -	/*
> -	 * We have to set preset values because it depends on the clk_mul
> -	 * value. Moreover, SDR104 is supported in a degraded mode since the
> -	 * maximum sd clock value is 120 MHz instead of 208 MHz. For that
> -	 * reason, we need to use presets to support SDR104.
> -	 */
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR12);
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR25);
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR50);
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR104);
> -	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
> -	       host->ioaddr + SDHCI_PRESET_FOR_DDR50);
> -
> -	clk_prepare_enable(priv->mainck);
> -	clk_prepare_enable(priv->gck);
> +	ret = sdhci_at91_set_clks_presets(&pdev->dev);
> +	if (ret)
> +		goto sdhci_pltfm_free;
>  
>  	ret = mmc_of_parse(host->mmc);
>  	if (ret)
> @@ -335,8 +352,8 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>  clocks_disable_unprepare:
>  	clk_disable_unprepare(priv->gck);
>  	clk_disable_unprepare(priv->mainck);
> -hclock_disable_unprepare:
>  	clk_disable_unprepare(priv->hclock);
> +sdhci_pltfm_free:
>  	sdhci_pltfm_free(pdev);
>  	return ret;
>  }
> 

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Adrian Hunter]

On 16/06/17 10:29, Quentin Schulz wrote:
> This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
> SoC's SDHCI controller.
> 
> When resuming from deepest state, it is required to restore preset
> registers as the registers are lost since VDD core has been shut down
> when entering deepest state on the SAMA5D2. The clocks need to be
> reconfigured as well.
> 
> The other registers and init process are taken care of by the SDHCI
> core.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
>  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index fb8c6011f13d..300513fc1068 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
>  }
>  
>  #ifdef CONFIG_PM

Should be CONFIG_PM_SLEEP for suspend / resume callbacks.

> +static int sdhci_at91_suspend(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	int ret;
> +
> +	ret = sdhci_suspend_host(host);
> +
> +	if (host->runtime_suspended)
> +		return ret;

Suspending while runtime suspended seems like a bad idea.  Have you
considered just adding sdhci_at91_set_clks_presets() to
sdhci_at91_runtime_resume()?

> +
> +	clk_disable_unprepare(priv->gck);
> +	clk_disable_unprepare(priv->hclock);
> +	clk_disable_unprepare(priv->mainck);
> +
> +	return ret;
> +}
> +
> +static int sdhci_at91_resume(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = sdhci_at91_set_clks_presets(dev);
> +	if (ret)
> +		return ret;
> +
> +	return sdhci_resume_host(host);
> +}
> +
>  static int sdhci_at91_runtime_suspend(struct device *dev)
>  {
>  	struct sdhci_host *host = dev_get_drvdata(dev);
> @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
>  #endif /* CONFIG_PM */
>  
>  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> -				pm_runtime_force_resume)
> +	SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume)
>  	SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend,
>  			   sdhci_at91_runtime_resume,
>  			   NULL)
> 

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Quentin Schulz]

Hi Adrian,

On 20/06/2017 09:39, Adrian Hunter wrote:
> On 16/06/17 10:29, Quentin Schulz wrote:
>> This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
>> SoC's SDHCI controller.
>>
>> When resuming from deepest state, it is required to restore preset
>> registers as the registers are lost since VDD core has been shut down
>> when entering deepest state on the SAMA5D2. The clocks need to be
>> reconfigured as well.
>>
>> The other registers and init process are taken care of by the SDHCI
>> core.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>> ---
>>  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
>>  1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
>> index fb8c6011f13d..300513fc1068 100644
>> --- a/drivers/mmc/host/sdhci-of-at91.c
>> +++ b/drivers/mmc/host/sdhci-of-at91.c
>> @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
>>  }
>>  
>>  #ifdef CONFIG_PM
> 
> Should be CONFIG_PM_SLEEP for suspend / resume callbacks.
> 

So I let this CONFIG_PM around the runtime_suspend/resume but put
another CONFIG_PM_SLEEP around the suspend/resume functions?

>> +static int sdhci_at91_suspend(struct device *dev)
>> +{
>> +	struct sdhci_host *host = dev_get_drvdata(dev);
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +	int ret;
>> +
>> +	ret = sdhci_suspend_host(host);
>> +
>> +	if (host->runtime_suspended)
>> +		return ret;
> 
> Suspending while runtime suspended seems like a bad idea.  Have you
> considered just adding sdhci_at91_set_clks_presets() to
> sdhci_at91_runtime_resume()?
> 

Adding sdhci_at91_set_clks_presets() to runtime_resume() seems a bad
idea as well. You don't need to recompute the clock rate, set it and set
the presets registers each time you do a runtime_resume. As the
runtime_pm of sdhci has a quite aggressive policy of activation, this
seems like a bad idea on the optimization side.

Thanks,
Quentin

>> +
>> +	clk_disable_unprepare(priv->gck);
>> +	clk_disable_unprepare(priv->hclock);
>> +	clk_disable_unprepare(priv->mainck);
>> +
>> +	return ret;
>> +}
>> +
>> +static int sdhci_at91_resume(struct device *dev)
>> +{
>> +	struct sdhci_host *host = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	ret = sdhci_at91_set_clks_presets(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return sdhci_resume_host(host);
>> +}
>> +
>>  static int sdhci_at91_runtime_suspend(struct device *dev)
>>  {
>>  	struct sdhci_host *host = dev_get_drvdata(dev);
>> @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
>>  #endif /* CONFIG_PM */
>>  
>>  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
>> -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> -				pm_runtime_force_resume)
>> +	SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume)
>>  	SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend,
>>  			   sdhci_at91_runtime_resume,
>>  			   NULL)
>>
> 

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/2] mmc: sdhci-of-at91: factor out clks and presets setting

[Quentin Schulz]

Hi Adrian,

On 20/06/2017 08:36, Adrian Hunter wrote:
> On 16/06/17 10:29, Quentin Schulz wrote:
>> The setting of clocks and presets is currently done in probe only but
>> once deep PM support is added, it'll be needed in the resume function.
>>
>> Let's create a function for this setting.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> 
> Apart from cosmetic comment below:
> 
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> 
>> ---
>>  drivers/mmc/host/sdhci-of-at91.c | 147 ++++++++++++++++++++++-----------------
>>  1 file changed, 82 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
>> index 7611fd679f1a..fb8c6011f13d 100644
>> --- a/drivers/mmc/host/sdhci-of-at91.c
>> +++ b/drivers/mmc/host/sdhci-of-at91.c
>> @@ -128,6 +128,84 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, sdhci_at91_dt_match);
>>  
>> +static int sdhci_at91_set_clks_presets(struct device *dev)
>> +{
>> +	struct sdhci_host *host = dev_get_drvdata(dev);
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +	int ret;
>> +	unsigned int			caps0, caps1;
>> +	unsigned int			clk_base, clk_mul;
>> +	unsigned int			gck_rate, real_gck_rate;
>> +	unsigned int			preset_div;
> 
> Too much whitespace.
> 

Simply moved some variables from the original code (see sdhci_at91_probe
below).

Thanks,
Quentin

>> +
>> +	/*
>> +	 * The mult clock is provided by as a generated clock by the PMC
>> +	 * controller. In order to set the rate of gck, we have to get the
>> +	 * base clock rate and the clock mult from capabilities.
>> +	 */
>> +	clk_prepare_enable(priv->hclock);
>> +	caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES);
>> +	caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1);
>> +	clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
>> +	clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT;
>> +	gck_rate = clk_base * 1000000 * (clk_mul + 1);
>> +	ret = clk_set_rate(priv->gck, gck_rate);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to set gck");
>> +		clk_disable_unprepare(priv->hclock);
>> +		return ret;
>> +	}
>> +	/*
>> +	 * We need to check if we have the requested rate for gck because in
>> +	 * some cases this rate could be not supported. If it happens, the rate
>> +	 * is the closest one gck can provide. We have to update the value
>> +	 * of clk mul.
>> +	 */
>> +	real_gck_rate = clk_get_rate(priv->gck);
>> +	if (real_gck_rate != gck_rate) {
>> +		clk_mul = real_gck_rate / (clk_base * 1000000) - 1;
>> +		caps1 &= (~SDHCI_CLOCK_MUL_MASK);
>> +		caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) &
>> +			  SDHCI_CLOCK_MUL_MASK);
>> +		/* Set capabilities in r/w mode. */
>> +		writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN,
>> +		       host->ioaddr + SDMMC_CACR);
>> +		writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1);
>> +		/* Set capabilities in ro mode. */
>> +		writel(0, host->ioaddr + SDMMC_CACR);
>> +		dev_info(dev, "update clk mul to %u as gck rate is %u Hz\n",
>> +			 clk_mul, real_gck_rate);
>> +	}
>> +
>> +	/*
>> +	 * We have to set preset values because it depends on the clk_mul
>> +	 * value. Moreover, SDR104 is supported in a degraded mode since the
>> +	 * maximum sd clock value is 120 MHz instead of 208 MHz. For that
>> +	 * reason, we need to use presets to support SDR104.
>> +	 */
>> +	preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1;
>> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
>> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR12);
>> +	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
>> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
>> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR25);
>> +	preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1;
>> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
>> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR50);
>> +	preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1;
>> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
>> +	       host->ioaddr + SDHCI_PRESET_FOR_SDR104);
>> +	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
>> +	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
>> +	       host->ioaddr + SDHCI_PRESET_FOR_DDR50);
>> +
>> +	clk_prepare_enable(priv->mainck);
>> +	clk_prepare_enable(priv->gck);
>> +
>> +	return 0;
>> +}
>> +
>>  #ifdef CONFIG_PM
>>  static int sdhci_at91_runtime_suspend(struct device *dev)
>>  {
>> @@ -192,11 +270,7 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>>  	struct sdhci_host		*host;
>>  	struct sdhci_pltfm_host		*pltfm_host;
>>  	struct sdhci_at91_priv		*priv;
>> -	unsigned int			caps0, caps1;
>> -	unsigned int			clk_base, clk_mul;
>> -	unsigned int			gck_rate, real_gck_rate;
>>  	int				ret;
>> -	unsigned int			preset_div;
>>  
>>  	match = of_match_device(sdhci_at91_dt_match, &pdev->dev);
>>  	if (!match)
>> @@ -228,66 +302,9 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>>  		return PTR_ERR(priv->gck);
>>  	}
>>  
>> -	/*
>> -	 * The mult clock is provided by as a generated clock by the PMC
>> -	 * controller. In order to set the rate of gck, we have to get the
>> -	 * base clock rate and the clock mult from capabilities.
>> -	 */
>> -	clk_prepare_enable(priv->hclock);
>> -	caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES);
>> -	caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1);
>> -	clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
>> -	clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT;
>> -	gck_rate = clk_base * 1000000 * (clk_mul + 1);
>> -	ret = clk_set_rate(priv->gck, gck_rate);
>> -	if (ret < 0) {
>> -		dev_err(&pdev->dev, "failed to set gck");
>> -		goto hclock_disable_unprepare;
>> -	}
>> -	/*
>> -	 * We need to check if we have the requested rate for gck because in
>> -	 * some cases this rate could be not supported. If it happens, the rate
>> -	 * is the closest one gck can provide. We have to update the value
>> -	 * of clk mul.
>> -	 */
>> -	real_gck_rate = clk_get_rate(priv->gck);
>> -	if (real_gck_rate != gck_rate) {
>> -		clk_mul = real_gck_rate / (clk_base * 1000000) - 1;
>> -		caps1 &= (~SDHCI_CLOCK_MUL_MASK);
>> -		caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) & SDHCI_CLOCK_MUL_MASK);
>> -		/* Set capabilities in r/w mode. */
>> -		writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN, host->ioaddr + SDMMC_CACR);
>> -		writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1);
>> -		/* Set capabilities in ro mode. */
>> -		writel(0, host->ioaddr + SDMMC_CACR);
>> -		dev_info(&pdev->dev, "update clk mul to %u as gck rate is %u Hz\n",
>> -			 clk_mul, real_gck_rate);
>> -	}
>> -
>> -	/*
>> -	 * We have to set preset values because it depends on the clk_mul
>> -	 * value. Moreover, SDR104 is supported in a degraded mode since the
>> -	 * maximum sd clock value is 120 MHz instead of 208 MHz. For that
>> -	 * reason, we need to use presets to support SDR104.
>> -	 */
>> -	preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1;
>> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
>> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR12);
>> -	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
>> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
>> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR25);
>> -	preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1;
>> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
>> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR50);
>> -	preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1;
>> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
>> -	       host->ioaddr + SDHCI_PRESET_FOR_SDR104);
>> -	preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1;
>> -	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
>> -	       host->ioaddr + SDHCI_PRESET_FOR_DDR50);
>> -
>> -	clk_prepare_enable(priv->mainck);
>> -	clk_prepare_enable(priv->gck);
>> +	ret = sdhci_at91_set_clks_presets(&pdev->dev);
>> +	if (ret)
>> +		goto sdhci_pltfm_free;
>>  
>>  	ret = mmc_of_parse(host->mmc);
>>  	if (ret)
>> @@ -335,8 +352,8 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>>  clocks_disable_unprepare:
>>  	clk_disable_unprepare(priv->gck);
>>  	clk_disable_unprepare(priv->mainck);
>> -hclock_disable_unprepare:
>>  	clk_disable_unprepare(priv->hclock);
>> +sdhci_pltfm_free:
>>  	sdhci_pltfm_free(pdev);
>>  	return ret;
>>  }
>>
> 

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Ludovic Desroches]

On Tue, Jun 20, 2017 at 10:07:06AM +0200, Quentin Schulz wrote:
> Hi Adrian,
> 
> On 20/06/2017 09:39, Adrian Hunter wrote:
> > On 16/06/17 10:29, Quentin Schulz wrote:
> >> This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
> >> SoC's SDHCI controller.
> >>
> >> When resuming from deepest state, it is required to restore preset
> >> registers as the registers are lost since VDD core has been shut down
> >> when entering deepest state on the SAMA5D2. The clocks need to be
> >> reconfigured as well.
> >>
> >> The other registers and init process are taken care of by the SDHCI
> >> core.
> >>
> >> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> >> ---
> >>  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
> >>  1 file changed, 32 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> >> index fb8c6011f13d..300513fc1068 100644
> >> --- a/drivers/mmc/host/sdhci-of-at91.c
> >> +++ b/drivers/mmc/host/sdhci-of-at91.c
> >> @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
> >>  }
> >>  
> >>  #ifdef CONFIG_PM
> > 
> > Should be CONFIG_PM_SLEEP for suspend / resume callbacks.
> > 
> 
> So I let this CONFIG_PM around the runtime_suspend/resume but put
> another CONFIG_PM_SLEEP around the suspend/resume functions?
> 
> >> +static int sdhci_at91_suspend(struct device *dev)
> >> +{
> >> +	struct sdhci_host *host = dev_get_drvdata(dev);
> >> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> +	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
> >> +	int ret;
> >> +
> >> +	ret = sdhci_suspend_host(host);
> >> +
> >> +	if (host->runtime_suspended)
> >> +		return ret;
> > 
> > Suspending while runtime suspended seems like a bad idea.  Have you
> > considered just adding sdhci_at91_set_clks_presets() to
> > sdhci_at91_runtime_resume()?
> > 
> 
> Adding sdhci_at91_set_clks_presets() to runtime_resume() seems a bad
> idea as well. You don't need to recompute the clock rate, set it and set
> the presets registers each time you do a runtime_resume. As the
> runtime_pm of sdhci has a quite aggressive policy of activation, this
> seems like a bad idea on the optimization side.

So maybe increment/decrement the device's usage counter. It should be
safer.

Ludovic

> 
> Thanks,
> Quentin
> 
> >> +
> >> +	clk_disable_unprepare(priv->gck);
> >> +	clk_disable_unprepare(priv->hclock);
> >> +	clk_disable_unprepare(priv->mainck);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int sdhci_at91_resume(struct device *dev)
> >> +{
> >> +	struct sdhci_host *host = dev_get_drvdata(dev);
> >> +	int ret;
> >> +
> >> +	ret = sdhci_at91_set_clks_presets(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	return sdhci_resume_host(host);
> >> +}
> >> +
> >>  static int sdhci_at91_runtime_suspend(struct device *dev)
> >>  {
> >>  	struct sdhci_host *host = dev_get_drvdata(dev);
> >> @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
> >>  #endif /* CONFIG_PM */
> >>  
> >>  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
> >> -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> >> -				pm_runtime_force_resume)
> >> +	SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume)
> >>  	SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend,
> >>  			   sdhci_at91_runtime_resume,
> >>  			   NULL)
> >>
> > 
> 
> -- 
> Quentin Schulz, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Ludovic Desroches]

On Tue, Jun 20, 2017 at 10:07:06AM +0200, Quentin Schulz wrote:
> Hi Adrian,
> 
> On 20/06/2017 09:39, Adrian Hunter wrote:
> > On 16/06/17 10:29, Quentin Schulz wrote:
> >> This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
> >> SoC's SDHCI controller.
> >>
> >> When resuming from deepest state, it is required to restore preset
> >> registers as the registers are lost since VDD core has been shut down
> >> when entering deepest state on the SAMA5D2. The clocks need to be
> >> reconfigured as well.
> >>
> >> The other registers and init process are taken care of by the SDHCI
> >> core.
> >>
> >> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> >> ---
> >>  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
> >>  1 file changed, 32 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> >> index fb8c6011f13d..300513fc1068 100644
> >> --- a/drivers/mmc/host/sdhci-of-at91.c
> >> +++ b/drivers/mmc/host/sdhci-of-at91.c
> >> @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
> >>  }
> >>  
> >>  #ifdef CONFIG_PM
> > 
> > Should be CONFIG_PM_SLEEP for suspend / resume callbacks.
> > 
> 
> So I let this CONFIG_PM around the runtime_suspend/resume but put
> another CONFIG_PM_SLEEP around the suspend/resume functions?
> 
> >> +static int sdhci_at91_suspend(struct device *dev)
> >> +{
> >> +	struct sdhci_host *host = dev_get_drvdata(dev);
> >> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> +	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
> >> +	int ret;
> >> +
> >> +	ret = sdhci_suspend_host(host);
> >> +
> >> +	if (host->runtime_suspended)
> >> +		return ret;
> > 
> > Suspending while runtime suspended seems like a bad idea.  Have you
> > considered just adding sdhci_at91_set_clks_presets() to
> > sdhci_at91_runtime_resume()?
> > 
> 
> Adding sdhci_at91_set_clks_presets() to runtime_resume() seems a bad
> idea as well. You don't need to recompute the clock rate, set it and set
> the presets registers each time you do a runtime_resume. As the
> runtime_pm of sdhci has a quite aggressive policy of activation, this
> seems like a bad idea on the optimization side.

So maybe increment/decrement the device's usage counter. It should be
safer.

Ludovic

> 
> Thanks,
> Quentin
> 
> >> +
> >> +	clk_disable_unprepare(priv->gck);
> >> +	clk_disable_unprepare(priv->hclock);
> >> +	clk_disable_unprepare(priv->mainck);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int sdhci_at91_resume(struct device *dev)
> >> +{
> >> +	struct sdhci_host *host = dev_get_drvdata(dev);
> >> +	int ret;
> >> +
> >> +	ret = sdhci_at91_set_clks_presets(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	return sdhci_resume_host(host);
> >> +}
> >> +
> >>  static int sdhci_at91_runtime_suspend(struct device *dev)
> >>  {
> >>  	struct sdhci_host *host = dev_get_drvdata(dev);
> >> @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
> >>  #endif /* CONFIG_PM */
> >>  
> >>  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
> >> -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> >> -				pm_runtime_force_resume)
> >> +	SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume)
> >>  	SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend,
> >>  			   sdhci_at91_runtime_resume,
> >>  			   NULL)
> >>
> > 
> 
> -- 
> Quentin Schulz, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Quentin Schulz]

Hi Adrian and Ludovic,

On 20/06/2017 11:49, Ludovic Desroches wrote:
> On Tue, Jun 20, 2017 at 10:07:06AM +0200, Quentin Schulz wrote:
>> Hi Adrian,
>>
>> On 20/06/2017 09:39, Adrian Hunter wrote:
>>> On 16/06/17 10:29, Quentin Schulz wrote:
>>>> This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
>>>> SoC's SDHCI controller.
>>>>
>>>> When resuming from deepest state, it is required to restore preset
>>>> registers as the registers are lost since VDD core has been shut down
>>>> when entering deepest state on the SAMA5D2. The clocks need to be
>>>> reconfigured as well.
>>>>
>>>> The other registers and init process are taken care of by the SDHCI
>>>> core.
>>>>
>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>>> ---
>>>>  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 32 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
>>>> index fb8c6011f13d..300513fc1068 100644
>>>> --- a/drivers/mmc/host/sdhci-of-at91.c
>>>> +++ b/drivers/mmc/host/sdhci-of-at91.c
>>>> @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
>>>>  }
>>>>  
>>>>  #ifdef CONFIG_PM
>>>
>>> Should be CONFIG_PM_SLEEP for suspend / resume callbacks.
>>>
>>
>> So I let this CONFIG_PM around the runtime_suspend/resume but put
>> another CONFIG_PM_SLEEP around the suspend/resume functions?
>>
>>>> +static int sdhci_at91_suspend(struct device *dev)
>>>> +{
>>>> +	struct sdhci_host *host = dev_get_drvdata(dev);
>>>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>> +	int ret;
>>>> +
>>>> +	ret = sdhci_suspend_host(host);
>>>> +
>>>> +	if (host->runtime_suspended)
>>>> +		return ret;
>>>
>>> Suspending while runtime suspended seems like a bad idea.  Have you
>>> considered just adding sdhci_at91_set_clks_presets() to
>>> sdhci_at91_runtime_resume()?
>>>
>>
>> Adding sdhci_at91_set_clks_presets() to runtime_resume() seems a bad
>> idea as well. You don't need to recompute the clock rate, set it and set
>> the presets registers each time you do a runtime_resume. As the
>> runtime_pm of sdhci has a quite aggressive policy of activation, this
>> seems like a bad idea on the optimization side.
> 
> So maybe increment/decrement the device's usage counter. It should be
> safer.
> 

>From what I've understood from the runtime_pm documentation[1], it seems
that there is no need in my case to test if the system has been runtime
suspended before being suspended. So I think we can safely remove the
test and leave the rest as is.

My understanding is the following:
If the system is not runtime suspended before doing suspend, then it
just does suspend and then resume.
=> enable and disable clocks are called once each so it is balanced.

If the system is already runtime suspended when suspending, the resume
will be called and once the device will be used, the runtime resume will
be called.
=> enable and disable clocks are called twice each (once in runtime and
system suspend/resume) so it is balanced.

A few quick tests on my sama5d2_xplained seem to be validating those
hypothesis.

Do we agree on removing the `if (host->runtime_suspended)`?

Thanks,
Quentin

> Ludovic
> 
>>
>> Thanks,
>> Quentin
>>
>>>> +
>>>> +	clk_disable_unprepare(priv->gck);
>>>> +	clk_disable_unprepare(priv->hclock);
>>>> +	clk_disable_unprepare(priv->mainck);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int sdhci_at91_resume(struct device *dev)
>>>> +{
>>>> +	struct sdhci_host *host = dev_get_drvdata(dev);
>>>> +	int ret;
>>>> +
>>>> +	ret = sdhci_at91_set_clks_presets(dev);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	return sdhci_resume_host(host);
>>>> +}
>>>> +
>>>>  static int sdhci_at91_runtime_suspend(struct device *dev)
>>>>  {
>>>>  	struct sdhci_host *host = dev_get_drvdata(dev);
>>>> @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
>>>>  #endif /* CONFIG_PM */
>>>>  
>>>>  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
>>>> -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>>> -				pm_runtime_force_resume)
>>>> +	SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume)
>>>>  	SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend,
>>>>  			   sdhci_at91_runtime_resume,
>>>>  			   NULL)
>>>>
>>>
>>
>> -- 
>> Quentin Schulz, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Quentin Schulz]

Better with the link.

On 05/07/2017 08:23, Quentin Schulz wrote:
> Hi Adrian and Ludovic,
> 
> On 20/06/2017 11:49, Ludovic Desroches wrote:
>> On Tue, Jun 20, 2017 at 10:07:06AM +0200, Quentin Schulz wrote:
>>> Hi Adrian,
>>>
>>> On 20/06/2017 09:39, Adrian Hunter wrote:
>>>> On 16/06/17 10:29, Quentin Schulz wrote:
>>>>> This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
>>>>> SoC's SDHCI controller.
>>>>>
>>>>> When resuming from deepest state, it is required to restore preset
>>>>> registers as the registers are lost since VDD core has been shut down
>>>>> when entering deepest state on the SAMA5D2. The clocks need to be
>>>>> reconfigured as well.
>>>>>
>>>>> The other registers and init process are taken care of by the SDHCI
>>>>> core.
>>>>>
>>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>>>> ---
>>>>>  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
>>>>>  1 file changed, 32 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
>>>>> index fb8c6011f13d..300513fc1068 100644
>>>>> --- a/drivers/mmc/host/sdhci-of-at91.c
>>>>> +++ b/drivers/mmc/host/sdhci-of-at91.c
>>>>> @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
>>>>>  }
>>>>>  
>>>>>  #ifdef CONFIG_PM
>>>>
>>>> Should be CONFIG_PM_SLEEP for suspend / resume callbacks.
>>>>
>>>
>>> So I let this CONFIG_PM around the runtime_suspend/resume but put
>>> another CONFIG_PM_SLEEP around the suspend/resume functions?
>>>
>>>>> +static int sdhci_at91_suspend(struct device *dev)
>>>>> +{
>>>>> +	struct sdhci_host *host = dev_get_drvdata(dev);
>>>>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>> +	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = sdhci_suspend_host(host);
>>>>> +
>>>>> +	if (host->runtime_suspended)
>>>>> +		return ret;
>>>>
>>>> Suspending while runtime suspended seems like a bad idea.  Have you
>>>> considered just adding sdhci_at91_set_clks_presets() to
>>>> sdhci_at91_runtime_resume()?
>>>>
>>>
>>> Adding sdhci_at91_set_clks_presets() to runtime_resume() seems a bad
>>> idea as well. You don't need to recompute the clock rate, set it and set
>>> the presets registers each time you do a runtime_resume. As the
>>> runtime_pm of sdhci has a quite aggressive policy of activation, this
>>> seems like a bad idea on the optimization side.
>>
>> So maybe increment/decrement the device's usage counter. It should be
>> safer.
>>
> 
> From what I've understood from the runtime_pm documentation[1], it seems
> that there is no need in my case to test if the system has been runtime
> suspended before being suspended. So I think we can safely remove the
> test and leave the rest as is.
> 
> My understanding is the following:
> If the system is not runtime suspended before doing suspend, then it
> just does suspend and then resume.
> => enable and disable clocks are called once each so it is balanced.
> 
> If the system is already runtime suspended when suspending, the resume
> will be called and once the device will be used, the runtime resume will
> be called.
> => enable and disable clocks are called twice each (once in runtime and
> system suspend/resume) so it is balanced.
> 
> A few quick tests on my sama5d2_xplained seem to be validating those
> hypothesis.
> 
> Do we agree on removing the `if (host->runtime_suspended)`?
> 

[1]
http://elixir.free-electrons.com/linux/latest/source/Documentation/power/runtime_pm.txt#L613

> Thanks,
> Quentin
> 
>> Ludovic
>>
>>>
>>> Thanks,
>>> Quentin
>>>
>>>>> +
>>>>> +	clk_disable_unprepare(priv->gck);
>>>>> +	clk_disable_unprepare(priv->hclock);
>>>>> +	clk_disable_unprepare(priv->mainck);
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static int sdhci_at91_resume(struct device *dev)
>>>>> +{
>>>>> +	struct sdhci_host *host = dev_get_drvdata(dev);
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = sdhci_at91_set_clks_presets(dev);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	return sdhci_resume_host(host);
>>>>> +}
>>>>> +
>>>>>  static int sdhci_at91_runtime_suspend(struct device *dev)
>>>>>  {
>>>>>  	struct sdhci_host *host = dev_get_drvdata(dev);
>>>>> @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
>>>>>  #endif /* CONFIG_PM */
>>>>>  
>>>>>  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
>>>>> -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>>>> -				pm_runtime_force_resume)
>>>>> +	SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume)
>>>>>  	SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend,
>>>>>  			   sdhci_at91_runtime_resume,
>>>>>  			   NULL)
>>>>>
>>>>
>>>
>>> -- 
>>> Quentin Schulz, Free Electrons
>>> Embedded Linux and Kernel engineering
>>> http://free-electrons.com
> 

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Adrian Hunter]

On 07/05/2017 09:45 AM, Quentin Schulz wrote:
> Better with the link.
> 
> On 05/07/2017 08:23, Quentin Schulz wrote:
>> Hi Adrian and Ludovic,
>>
>> On 20/06/2017 11:49, Ludovic Desroches wrote:
>>> On Tue, Jun 20, 2017 at 10:07:06AM +0200, Quentin Schulz wrote:
>>>> Hi Adrian,
>>>>
>>>> On 20/06/2017 09:39, Adrian Hunter wrote:
>>>>> On 16/06/17 10:29, Quentin Schulz wrote:
>>>>>> This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
>>>>>> SoC's SDHCI controller.
>>>>>>
>>>>>> When resuming from deepest state, it is required to restore preset
>>>>>> registers as the registers are lost since VDD core has been shut down
>>>>>> when entering deepest state on the SAMA5D2. The clocks need to be
>>>>>> reconfigured as well.
>>>>>>
>>>>>> The other registers and init process are taken care of by the SDHCI
>>>>>> core.
>>>>>>
>>>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>>>>> ---
>>>>>>  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
>>>>>>  1 file changed, 32 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
>>>>>> index fb8c6011f13d..300513fc1068 100644
>>>>>> --- a/drivers/mmc/host/sdhci-of-at91.c
>>>>>> +++ b/drivers/mmc/host/sdhci-of-at91.c
>>>>>> @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
>>>>>>  }
>>>>>>  
>>>>>>  #ifdef CONFIG_PM
>>>>>
>>>>> Should be CONFIG_PM_SLEEP for suspend / resume callbacks.
>>>>>
>>>>
>>>> So I let this CONFIG_PM around the runtime_suspend/resume but put
>>>> another CONFIG_PM_SLEEP around the suspend/resume functions?
>>>>
>>>>>> +static int sdhci_at91_suspend(struct device *dev)
>>>>>> +{
>>>>>> +	struct sdhci_host *host = dev_get_drvdata(dev);
>>>>>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>> +	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = sdhci_suspend_host(host);
>>>>>> +
>>>>>> +	if (host->runtime_suspended)
>>>>>> +		return ret;
>>>>>
>>>>> Suspending while runtime suspended seems like a bad idea.  Have you
>>>>> considered just adding sdhci_at91_set_clks_presets() to
>>>>> sdhci_at91_runtime_resume()?
>>>>>
>>>>
>>>> Adding sdhci_at91_set_clks_presets() to runtime_resume() seems a bad
>>>> idea as well. You don't need to recompute the clock rate, set it and set
>>>> the presets registers each time you do a runtime_resume. As the
>>>> runtime_pm of sdhci has a quite aggressive policy of activation, this
>>>> seems like a bad idea on the optimization side.

What is the runtime resume time with and without sdhci_at91_set_clks_presets()?

>>>
>>> So maybe increment/decrement the device's usage counter. It should be
>>> safer.
>>>
>>
>> From what I've understood from the runtime_pm documentation[1], it seems
>> that there is no need in my case to test if the system has been runtime
>> suspended before being suspended. So I think we can safely remove the
>> test and leave the rest as is.
>>
>> My understanding is the following:
>> If the system is not runtime suspended before doing suspend, then it
>> just does suspend and then resume.
>> => enable and disable clocks are called once each so it is balanced.
>>
>> If the system is already runtime suspended when suspending, the resume
>> will be called and once the device will be used, the runtime resume will
>> be called.
>> => enable and disable clocks are called twice each (once in runtime and
>> system suspend/resume) so it is balanced.
>>
>> A few quick tests on my sama5d2_xplained seem to be validating those
>> hypothesis.
>>
>> Do we agree on removing the `if (host->runtime_suspended)`?
>>
> 
> [1]
> http://elixir.free-electrons.com/linux/latest/source/Documentation/power/runtime_pm.txt#L613

In the future we may want to rationalize sdhci pm functions or make other
changes.  That means they must be used correctly.  In particular, they must
not be interleaved or nested. i.e. it is not acceptable to call
sdhci_suspend_host() in between calls to sdhci_runtime_suspend_host() and
sdhci_runtime_resume_host().

Also use pm_runtime_suspended() not host->runtime_suspended.

> 
>> Thanks,
>> Quentin
>>
>>> Ludovic
>>>
>>>>
>>>> Thanks,
>>>> Quentin
>>>>
>>>>>> +
>>>>>> +	clk_disable_unprepare(priv->gck);
>>>>>> +	clk_disable_unprepare(priv->hclock);
>>>>>> +	clk_disable_unprepare(priv->mainck);
>>>>>> +
>>>>>> +	return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int sdhci_at91_resume(struct device *dev)
>>>>>> +{
>>>>>> +	struct sdhci_host *host = dev_get_drvdata(dev);
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = sdhci_at91_set_clks_presets(dev);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	return sdhci_resume_host(host);
>>>>>> +}
>>>>>> +
>>>>>>  static int sdhci_at91_runtime_suspend(struct device *dev)
>>>>>>  {
>>>>>>  	struct sdhci_host *host = dev_get_drvdata(dev);
>>>>>> @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
>>>>>>  #endif /* CONFIG_PM */
>>>>>>  
>>>>>>  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
>>>>>> -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>>>>> -				pm_runtime_force_resume)
>>>>>> +	SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume)
>>>>>>  	SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend,
>>>>>>  			   sdhci_at91_runtime_resume,
>>>>>>  			   NULL)
>>>>>>
>>>>>
>>>>
>>>> -- 
>>>> Quentin Schulz, Free Electrons
>>>> Embedded Linux and Kernel engineering
>>>> http://free-electrons.com
>>
> 

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Ulf Hansson]

On 16 June 2017 at 09:29, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
> SoC's SDHCI controller.
>
> When resuming from deepest state, it is required to restore preset
> registers as the registers are lost since VDD core has been shut down
> when entering deepest state on the SAMA5D2. The clocks need to be
> reconfigured as well.

Right, so compared to runtime resume there is some additional
operations that is needed during system resume. Fair enough.

However by looking at the changes below, you also change the system
suspend operations, as it now calls sdhci_suspend_host(). Is that
change needed? Then why?

>
> The other registers and init process are taken care of by the SDHCI
> core.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
>  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index fb8c6011f13d..300513fc1068 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
>  }
>
>  #ifdef CONFIG_PM
> +static int sdhci_at91_suspend(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +       int ret;
> +
> +       ret = sdhci_suspend_host(host);
> +

This is wrong, you can't call sdhci_suspend_host() unless the device
is runtime resumed...

> +       if (host->runtime_suspended)
> +               return ret;

... and this is weird...

> +
> +       clk_disable_unprepare(priv->gck);
> +       clk_disable_unprepare(priv->hclock);
> +       clk_disable_unprepare(priv->mainck);
> +
> +       return ret;
> +}
> +
> +static int sdhci_at91_resume(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       int ret;
> +
> +       ret = sdhci_at91_set_clks_presets(dev);
> +       if (ret)
> +               return ret;

Instead of doing it like this, I suggest you set a new flag to true
here, let's call it "restore_needed".

In the ->runtime_resume() callback, you check the restore_needed flag
and performs the extra operations in that case. When that's done, the
->runtime_resume() callback clears the flag, as to avoid the next
runtime resume from unnecessary doing the extra operations.

> +
> +       return sdhci_resume_host(host);

Remove this and call pm_runtime_force_resume().

> +}
> +
>  static int sdhci_at91_runtime_suspend(struct device *dev)
>  {
>         struct sdhci_host *host = dev_get_drvdata(dev);
> @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
>  #endif /* CONFIG_PM */
>
>  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
> -       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,

Leave the pm_runtime_force_suspend() here, unless you have other
reasons not being described in the change log, to change the system
suspend operations.

> -                               pm_runtime_force_resume)
> +       SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume)
>         SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend,
>                            sdhci_at91_runtime_resume,
>                            NULL)
> --
> 2.11.0
>

Adopting my changes should simplify the code, avoid unnecessary
resuming the device but instead deferring that until really needed via
runtime PM.

Kind regards
Uffe

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Ludovic Desroches]

On Tue, Jul 11, 2017 at 02:42:44PM +0200, Ulf Hansson wrote:
> On 16 June 2017 at 09:29, Quentin Schulz
> <quentin.schulz@free-electrons.com> wrote:
> > This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
> > SoC's SDHCI controller.
> >
> > When resuming from deepest state, it is required to restore preset
> > registers as the registers are lost since VDD core has been shut down
> > when entering deepest state on the SAMA5D2. The clocks need to be
> > reconfigured as well.
> 
> Right, so compared to runtime resume there is some additional
> operations that is needed during system resume. Fair enough.
> 
> However by looking at the changes below, you also change the system
> suspend operations, as it now calls sdhci_suspend_host(). Is that
> change needed? Then why?
> 
> >
> > The other registers and init process are taken care of by the SDHCI
> > core.
> >
> > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> > ---
> >  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
> >  1 file changed, 32 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> > index fb8c6011f13d..300513fc1068 100644
> > --- a/drivers/mmc/host/sdhci-of-at91.c
> > +++ b/drivers/mmc/host/sdhci-of-at91.c
> > @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
> >  }
> >
> >  #ifdef CONFIG_PM
> > +static int sdhci_at91_suspend(struct device *dev)
> > +{
> > +       struct sdhci_host *host = dev_get_drvdata(dev);
> > +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +       struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +       int ret;
> > +
> > +       ret = sdhci_suspend_host(host);
> > +
> 
> This is wrong, you can't call sdhci_suspend_host() unless the device
> is runtime resumed...
> 
> > +       if (host->runtime_suspended)
> > +               return ret;
> 
> ... and this is weird...
> 
> > +
> > +       clk_disable_unprepare(priv->gck);
> > +       clk_disable_unprepare(priv->hclock);
> > +       clk_disable_unprepare(priv->mainck);
> > +
> > +       return ret;
> > +}
> > +
> > +static int sdhci_at91_resume(struct device *dev)
> > +{
> > +       struct sdhci_host *host = dev_get_drvdata(dev);
> > +       int ret;
> > +
> > +       ret = sdhci_at91_set_clks_presets(dev);
> > +       if (ret)
> > +               return ret;
> 
> Instead of doing it like this, I suggest you set a new flag to true
> here, let's call it "restore_needed".
> 
> In the ->runtime_resume() callback, you check the restore_needed flag
> and performs the extra operations in that case. When that's done, the
> ->runtime_resume() callback clears the flag, as to avoid the next
> runtime resume from unnecessary doing the extra operations.
> 
> > +
> > +       return sdhci_resume_host(host);
> 
> Remove this and call pm_runtime_force_resume().
> 
> > +}
> > +
> >  static int sdhci_at91_runtime_suspend(struct device *dev)
> >  {
> >         struct sdhci_host *host = dev_get_drvdata(dev);
> > @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
> >  #endif /* CONFIG_PM */
> >
> >  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
> > -       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> 
> Leave the pm_runtime_force_suspend() here, unless you have other
> reasons not being described in the change log, to change the system
> suspend operations.

I think we need to keep it to be able to set the restore_needed flag, isn't it?

Regards

Ludovic

> 
> > -                               pm_runtime_force_resume)
> > +       SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume)
> >         SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend,
> >                            sdhci_at91_runtime_resume,
> >                            NULL)
> > --
> > 2.11.0
> >
> 
> Adopting my changes should simplify the code, avoid unnecessary
> resuming the device but instead deferring that until really needed via
> runtime PM.
> 
> Kind regards
> Uffe

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Ludovic Desroches]

On Tue, Jul 11, 2017 at 02:42:44PM +0200, Ulf Hansson wrote:
> On 16 June 2017 at 09:29, Quentin Schulz
> <quentin.schulz@free-electrons.com> wrote:
> > This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
> > SoC's SDHCI controller.
> >
> > When resuming from deepest state, it is required to restore preset
> > registers as the registers are lost since VDD core has been shut down
> > when entering deepest state on the SAMA5D2. The clocks need to be
> > reconfigured as well.
> 
> Right, so compared to runtime resume there is some additional
> operations that is needed during system resume. Fair enough.
> 
> However by looking at the changes below, you also change the system
> suspend operations, as it now calls sdhci_suspend_host(). Is that
> change needed? Then why?
> 
> >
> > The other registers and init process are taken care of by the SDHCI
> > core.
> >
> > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> > ---
> >  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
> >  1 file changed, 32 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> > index fb8c6011f13d..300513fc1068 100644
> > --- a/drivers/mmc/host/sdhci-of-at91.c
> > +++ b/drivers/mmc/host/sdhci-of-at91.c
> > @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
> >  }
> >
> >  #ifdef CONFIG_PM
> > +static int sdhci_at91_suspend(struct device *dev)
> > +{
> > +       struct sdhci_host *host = dev_get_drvdata(dev);
> > +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +       struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +       int ret;
> > +
> > +       ret = sdhci_suspend_host(host);
> > +
> 
> This is wrong, you can't call sdhci_suspend_host() unless the device
> is runtime resumed...
> 
> > +       if (host->runtime_suspended)
> > +               return ret;
> 
> ... and this is weird...
> 
> > +
> > +       clk_disable_unprepare(priv->gck);
> > +       clk_disable_unprepare(priv->hclock);
> > +       clk_disable_unprepare(priv->mainck);
> > +
> > +       return ret;
> > +}
> > +
> > +static int sdhci_at91_resume(struct device *dev)
> > +{
> > +       struct sdhci_host *host = dev_get_drvdata(dev);
> > +       int ret;
> > +
> > +       ret = sdhci_at91_set_clks_presets(dev);
> > +       if (ret)
> > +               return ret;
> 
> Instead of doing it like this, I suggest you set a new flag to true
> here, let's call it "restore_needed".
> 
> In the ->runtime_resume() callback, you check the restore_needed flag
> and performs the extra operations in that case. When that's done, the
> ->runtime_resume() callback clears the flag, as to avoid the next
> runtime resume from unnecessary doing the extra operations.
> 
> > +
> > +       return sdhci_resume_host(host);
> 
> Remove this and call pm_runtime_force_resume().
> 
> > +}
> > +
> >  static int sdhci_at91_runtime_suspend(struct device *dev)
> >  {
> >         struct sdhci_host *host = dev_get_drvdata(dev);
> > @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
> >  #endif /* CONFIG_PM */
> >
> >  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
> > -       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> 
> Leave the pm_runtime_force_suspend() here, unless you have other
> reasons not being described in the change log, to change the system
> suspend operations.

I think we need to keep it to be able to set the restore_needed flag, isn't it?

Regards

Ludovic

> 
> > -                               pm_runtime_force_resume)
> > +       SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume)
> >         SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend,
> >                            sdhci_at91_runtime_resume,
> >                            NULL)
> > --
> > 2.11.0
> >
> 
> Adopting my changes should simplify the code, avoid unnecessary
> resuming the device but instead deferring that until really needed via
> runtime PM.
> 
> Kind regards
> Uffe

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Ulf Hansson]

On 11 July 2017 at 15:33, Ludovic Desroches
<ludovic.desroches@microchip.com> wrote:
> On Tue, Jul 11, 2017 at 02:42:44PM +0200, Ulf Hansson wrote:
>> On 16 June 2017 at 09:29, Quentin Schulz
>> <quentin.schulz@free-electrons.com> wrote:
>> > This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
>> > SoC's SDHCI controller.
>> >
>> > When resuming from deepest state, it is required to restore preset
>> > registers as the registers are lost since VDD core has been shut down
>> > when entering deepest state on the SAMA5D2. The clocks need to be
>> > reconfigured as well.
>>
>> Right, so compared to runtime resume there is some additional
>> operations that is needed during system resume. Fair enough.
>>
>> However by looking at the changes below, you also change the system
>> suspend operations, as it now calls sdhci_suspend_host(). Is that
>> change needed? Then why?
>>
>> >
>> > The other registers and init process are taken care of by the SDHCI
>> > core.
>> >
>> > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>> > ---
>> >  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
>> >  1 file changed, 32 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
>> > index fb8c6011f13d..300513fc1068 100644
>> > --- a/drivers/mmc/host/sdhci-of-at91.c
>> > +++ b/drivers/mmc/host/sdhci-of-at91.c
>> > @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
>> >  }
>> >
>> >  #ifdef CONFIG_PM
>> > +static int sdhci_at91_suspend(struct device *dev)
>> > +{
>> > +       struct sdhci_host *host = dev_get_drvdata(dev);
>> > +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> > +       struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> > +       int ret;
>> > +
>> > +       ret = sdhci_suspend_host(host);
>> > +
>>
>> This is wrong, you can't call sdhci_suspend_host() unless the device
>> is runtime resumed...
>>
>> > +       if (host->runtime_suspended)
>> > +               return ret;
>>
>> ... and this is weird...
>>
>> > +
>> > +       clk_disable_unprepare(priv->gck);
>> > +       clk_disable_unprepare(priv->hclock);
>> > +       clk_disable_unprepare(priv->mainck);
>> > +
>> > +       return ret;
>> > +}
>> > +
>> > +static int sdhci_at91_resume(struct device *dev)
>> > +{
>> > +       struct sdhci_host *host = dev_get_drvdata(dev);
>> > +       int ret;
>> > +
>> > +       ret = sdhci_at91_set_clks_presets(dev);
>> > +       if (ret)
>> > +               return ret;
>>
>> Instead of doing it like this, I suggest you set a new flag to true
>> here, let's call it "restore_needed".
>>
>> In the ->runtime_resume() callback, you check the restore_needed flag
>> and performs the extra operations in that case. When that's done, the
>> ->runtime_resume() callback clears the flag, as to avoid the next
>> runtime resume from unnecessary doing the extra operations.
>>
>> > +
>> > +       return sdhci_resume_host(host);
>>
>> Remove this and call pm_runtime_force_resume().
>>
>> > +}
>> > +
>> >  static int sdhci_at91_runtime_suspend(struct device *dev)
>> >  {
>> >         struct sdhci_host *host = dev_get_drvdata(dev);
>> > @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
>> >  #endif /* CONFIG_PM */
>> >
>> >  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
>> > -       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>
>> Leave the pm_runtime_force_suspend() here, unless you have other
>> reasons not being described in the change log, to change the system
>> suspend operations.
>
> I think we need to keep it to be able to set the restore_needed flag, isn't it?

Yeah, perhaps it's better to set the flag from sdhci_at91_suspend()
and instead leave the resume callback being assigned to
pm_runtime_force_resume().

I guess that is what you meant?

[...]

Kind regards
Uffe

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Ludovic Desroches]

On Tue, Jul 11, 2017 at 03:54:17PM +0200, Ulf Hansson wrote:
> On 11 July 2017 at 15:33, Ludovic Desroches
> <ludovic.desroches@microchip.com> wrote:
> > On Tue, Jul 11, 2017 at 02:42:44PM +0200, Ulf Hansson wrote:
> >> On 16 June 2017 at 09:29, Quentin Schulz
> >> <quentin.schulz@free-electrons.com> wrote:
> >> > This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
> >> > SoC's SDHCI controller.
> >> >
> >> > When resuming from deepest state, it is required to restore preset
> >> > registers as the registers are lost since VDD core has been shut down
> >> > when entering deepest state on the SAMA5D2. The clocks need to be
> >> > reconfigured as well.
> >>
> >> Right, so compared to runtime resume there is some additional
> >> operations that is needed during system resume. Fair enough.
> >>
> >> However by looking at the changes below, you also change the system
> >> suspend operations, as it now calls sdhci_suspend_host(). Is that
> >> change needed? Then why?
> >>
> >> >
> >> > The other registers and init process are taken care of by the SDHCI
> >> > core.
> >> >
> >> > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> >> > ---
> >> >  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
> >> >  1 file changed, 32 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> >> > index fb8c6011f13d..300513fc1068 100644
> >> > --- a/drivers/mmc/host/sdhci-of-at91.c
> >> > +++ b/drivers/mmc/host/sdhci-of-at91.c
> >> > @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
> >> >  }
> >> >
> >> >  #ifdef CONFIG_PM
> >> > +static int sdhci_at91_suspend(struct device *dev)
> >> > +{
> >> > +       struct sdhci_host *host = dev_get_drvdata(dev);
> >> > +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> > +       struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
> >> > +       int ret;
> >> > +
> >> > +       ret = sdhci_suspend_host(host);
> >> > +
> >>
> >> This is wrong, you can't call sdhci_suspend_host() unless the device
> >> is runtime resumed...
> >>
> >> > +       if (host->runtime_suspended)
> >> > +               return ret;
> >>
> >> ... and this is weird...
> >>
> >> > +
> >> > +       clk_disable_unprepare(priv->gck);
> >> > +       clk_disable_unprepare(priv->hclock);
> >> > +       clk_disable_unprepare(priv->mainck);
> >> > +
> >> > +       return ret;
> >> > +}
> >> > +
> >> > +static int sdhci_at91_resume(struct device *dev)
> >> > +{
> >> > +       struct sdhci_host *host = dev_get_drvdata(dev);
> >> > +       int ret;
> >> > +
> >> > +       ret = sdhci_at91_set_clks_presets(dev);
> >> > +       if (ret)
> >> > +               return ret;
> >>
> >> Instead of doing it like this, I suggest you set a new flag to true
> >> here, let's call it "restore_needed".
> >>
> >> In the ->runtime_resume() callback, you check the restore_needed flag
> >> and performs the extra operations in that case. When that's done, the
> >> ->runtime_resume() callback clears the flag, as to avoid the next
> >> runtime resume from unnecessary doing the extra operations.
> >>
> >> > +
> >> > +       return sdhci_resume_host(host);
> >>
> >> Remove this and call pm_runtime_force_resume().
> >>
> >> > +}
> >> > +
> >> >  static int sdhci_at91_runtime_suspend(struct device *dev)
> >> >  {
> >> >         struct sdhci_host *host = dev_get_drvdata(dev);
> >> > @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
> >> >  #endif /* CONFIG_PM */
> >> >
> >> >  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
> >> > -       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> >>
> >> Leave the pm_runtime_force_suspend() here, unless you have other
> >> reasons not being described in the change log, to change the system
> >> suspend operations.
> >
> > I think we need to keep it to be able to set the restore_needed flag, isn't it?
> 
> Yeah, perhaps it's better to set the flag from sdhci_at91_suspend()
> and instead leave the resume callback being assigned to
> pm_runtime_force_resume().
> 
> I guess that is what you meant?

Exactly. Thanks for your smart solution!

Regards

Ludovic

Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM

[Ludovic Desroches]

On Tue, Jul 11, 2017 at 03:54:17PM +0200, Ulf Hansson wrote:
> On 11 July 2017 at 15:33, Ludovic Desroches
> <ludovic.desroches@microchip.com> wrote:
> > On Tue, Jul 11, 2017 at 02:42:44PM +0200, Ulf Hansson wrote:
> >> On 16 June 2017 at 09:29, Quentin Schulz
> >> <quentin.schulz@free-electrons.com> wrote:
> >> > This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2
> >> > SoC's SDHCI controller.
> >> >
> >> > When resuming from deepest state, it is required to restore preset
> >> > registers as the registers are lost since VDD core has been shut down
> >> > when entering deepest state on the SAMA5D2. The clocks need to be
> >> > reconfigured as well.
> >>
> >> Right, so compared to runtime resume there is some additional
> >> operations that is needed during system resume. Fair enough.
> >>
> >> However by looking at the changes below, you also change the system
> >> suspend operations, as it now calls sdhci_suspend_host(). Is that
> >> change needed? Then why?
> >>
> >> >
> >> > The other registers and init process are taken care of by the SDHCI
> >> > core.
> >> >
> >> > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> >> > ---
> >> >  drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++--
> >> >  1 file changed, 32 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> >> > index fb8c6011f13d..300513fc1068 100644
> >> > --- a/drivers/mmc/host/sdhci-of-at91.c
> >> > +++ b/drivers/mmc/host/sdhci-of-at91.c
> >> > @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
> >> >  }
> >> >
> >> >  #ifdef CONFIG_PM
> >> > +static int sdhci_at91_suspend(struct device *dev)
> >> > +{
> >> > +       struct sdhci_host *host = dev_get_drvdata(dev);
> >> > +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> > +       struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
> >> > +       int ret;
> >> > +
> >> > +       ret = sdhci_suspend_host(host);
> >> > +
> >>
> >> This is wrong, you can't call sdhci_suspend_host() unless the device
> >> is runtime resumed...
> >>
> >> > +       if (host->runtime_suspended)
> >> > +               return ret;
> >>
> >> ... and this is weird...
> >>
> >> > +
> >> > +       clk_disable_unprepare(priv->gck);
> >> > +       clk_disable_unprepare(priv->hclock);
> >> > +       clk_disable_unprepare(priv->mainck);
> >> > +
> >> > +       return ret;
> >> > +}
> >> > +
> >> > +static int sdhci_at91_resume(struct device *dev)
> >> > +{
> >> > +       struct sdhci_host *host = dev_get_drvdata(dev);
> >> > +       int ret;
> >> > +
> >> > +       ret = sdhci_at91_set_clks_presets(dev);
> >> > +       if (ret)
> >> > +               return ret;
> >>
> >> Instead of doing it like this, I suggest you set a new flag to true
> >> here, let's call it "restore_needed".
> >>
> >> In the ->runtime_resume() callback, you check the restore_needed flag
> >> and performs the extra operations in that case. When that's done, the
> >> ->runtime_resume() callback clears the flag, as to avoid the next
> >> runtime resume from unnecessary doing the extra operations.
> >>
> >> > +
> >> > +       return sdhci_resume_host(host);
> >>
> >> Remove this and call pm_runtime_force_resume().
> >>
> >> > +}
> >> > +
> >> >  static int sdhci_at91_runtime_suspend(struct device *dev)
> >> >  {
> >> >         struct sdhci_host *host = dev_get_drvdata(dev);
> >> > @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev)
> >> >  #endif /* CONFIG_PM */
> >> >
> >> >  static const struct dev_pm_ops sdhci_at91_dev_pm_ops = {
> >> > -       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> >>
> >> Leave the pm_runtime_force_suspend() here, unless you have other
> >> reasons not being described in the change log, to change the system
> >> suspend operations.
> >
> > I think we need to keep it to be able to set the restore_needed flag, isn't it?
> 
> Yeah, perhaps it's better to set the flag from sdhci_at91_suspend()
> and instead leave the resume callback being assigned to
> pm_runtime_force_resume().
> 
> I guess that is what you meant?

Exactly. Thanks for your smart solution!

Regards

Ludovic