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

>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