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