Re: [PATCH] mmc: sdhci-esdhc-imx: make sure usdhc clock enabled while doing suspend

[Dong Aisheng <dongas86@gmail.com>]

On Fri, Oct 21, 2016 at 09:42:27AM +0200, Ulf Hansson wrote:
> On 19 October 2016 at 11:18, Dong Aisheng <dongas86@gmail.com> wrote:
> > Hi Ulf,
> >
> > On Tue, Oct 18, 2016 at 5:18 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >> On 18 October 2016 at 09:39, Haibo Chen <haibo.chen@nxp.com> wrote:
> >>> When suspend usdhc, it will access usdhc register. So usdhc clock
> >>> should be enabled, otherwise the access usdhc register will return
> >>> error or cause system hung.
> >>>
> >>> Take this into consideration, if system enable a usdhc and do not
> >>> connect any SD/SDIO/MMC card, after system boot up, this usdhc
> >>> will do runtime suspend, and close all usdhc clock. At this time,
> >>> if suspend the system, due to no card persent, usdhc runtime resume
> >>> will not be called. So usdhc clock still closed, then in suspend,
> >>> once access usdhc register, system hung or bus error return.
> >>>
> >>> This patch make sure usdhc clock always enabled while doing usdhc
> >>> suspend.
> >>
> >> Yes, and since the clocks are kept enabled during system suspend that
> >> means wasting power, doesn't it!?
> >>
> >
> > IMX SoCs will disable all modules clocks in system stop mode
> > automatically by hardware
> > even it's enabled before
> > CCGR value Clock Activity Description:
> > 00 clock is off during all modes. stop enter hardware handshake is disabled.
> > 01 clock is on in run mode, but off in wait and stop modes
> > 10 Not applicable (Reserved).
> > 11 clock is on during all modes, except stop mode.
> >
> > Although HW will gate off it automatically, but i think it's still
> > good to align the state between
> > SW and HW.
> 
> Yes, indeed!
> 
> >
> >> May I propose another solution. Currently you deal only with clock
> >> gating/ungating during runtime suspend/resume. I am wondering whether
> >> you could extend those operations to be similar to what is needed
> >> during system suspend/resume?
> >>
> >
> > IMX driver are calling sdhci_runtime_suspend_host() and
> > sdhci_suspend_host() for runtime
> > suspend and system sleep case respectively.
> > Those two APIs definitions are different.
> > e.g. sdhci_suspend_host will disable card detection and enable wakeup
> > if any while
> > sdhci_runtime_suspend_host() not.
> 
> Yes, and that is weird and most likely wrong!
> 
> >
> > It may not be suitable to extend runtime operations to be similar as
> > sleep pm operations
> 
> I have several times by know had kind of similar discussions, but for
> different sdhci variants. I believe we are papering over a PM issue in
> sdhci, instead of actually trying to solve the real problem and in a
> generic fashion.
> 
> As a first step, why not try to combine sdhci_suspend_host() and
> sdhci_runtime_suspend_host() into one function, and vice verse for the
> resume functions.
> 

Yes, that's a reasonable way.
We may need double check whether we can combine them into one function
since they're defined for different purpose currently.

> Then sdhci variants can decide how they want to use them.
> Particularly, those that uses runtime PM, should benefit from using
> the runtime PM centric approach and thus get system PM suspend/resume
> for "free".
> 
> > if using common sdhci suspend function, unless we implement totoally
> > IMX specific
> > PM/Runtime PM function.
> >
> >
> > Another option may be like what omap_hsmmc does:
> >
> > Something like:
> >
> > int sdhci_esdhc_suspend(struct device *dev)
> > {
> >         pm_runtime_get_sync(host->dev);
> >         ret = sdhci_pltfm_suspend(dev);
> >         pm_runtime_put_sync(host->dev);
> >
> >         return ret;
> > }
> >
> > int sdhci_esdhc_resume(struct device *dev)
> > {
> >         pm_runtime_get_sync(host->dev);
> >
> >         ...
> >         ret = sdhci_pltfm_resume(dev);
> >
> >         pm_runtime_mark_last_busy(host->dev);
> >         pm_runtime_put_autosuspend(host->dev);
> >
> >         return ret;
> > }
> >
> > Does that seem ok?
> 
> No, this doesn't work!
> 
> People that aren't into runtime PM, believes that the
> pm_runtime_put*() in these paths means that the device enters runtime
> suspend state. That's not the case.
> 
> Instead the device will remain runtime resumed while the system enters
> suspend. Is that really what you want?

Yes, it's true.
Thanks for spotting it.

> 
> [...]
> 
> Kind regards
> Uffe

Regards
Dong Aisheng