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

[Ulf Hansson <ulf.hansson@linaro.org>]

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.

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?

[...]

Kind regards
Uffe