On Tue, Jul 11, 2017 at 03:54:17PM +0200, Ulf Hansson wrote: > On 11 July 2017 at 15:33, Ludovic Desroches > <ludovic.desroc...@microchip.com> wrote: > > On Tue, Jul 11, 2017 at 02:42:44PM +0200, Ulf Hansson wrote: > >> On 16 June 2017 at 09:29, Quentin Schulz > >> <quentin.sch...@free-electrons.com> 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.sch...@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); > >> > + > >> > >> 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?
Exactly. Thanks for your smart solution! Regards Ludovic