On Fri, Oct 21, 2016 at 09:42:27AM +0200, Ulf Hansson wrote:
> On 19 October 2016 at 11:18, Dong Aisheng <dongas86@xxxxxxxxx> wrote:
> > Hi Ulf,
> >
> > On Tue, Oct 18, 2016 at 5:18 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> >> On 18 October 2016 at 09:39, Haibo Chen <haibo.chen@xxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
