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