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

[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