Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1667491 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after resume from deepest PM Quentin Schulz <quentin.schulz@free-electrons.com> - 2017-06-16 09:30 +0200
    Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after  resume from deepest PM Ludovic Desroches <ludovic.desroches@microchip.com> - 2017-06-20 08:40 +0200
    Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after  resume from deepest PM Adrian Hunter <adrian.hunter@intel.com> - 2017-06-20 09:50 +0200
      Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after  resume from deepest PM Quentin Schulz <quentin.schulz@free-electrons.com> - 2017-06-20 10:10 +0200
        Re: [PATCH 2/2] mmc: sdhci-of-at91: set clocks and presets after  resume from deepest PM Ludovic Desroches <ludovic.desroches@microchip.com> - 2017-06-20 12:00 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web