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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Dong Aisheng [mailto:dongas86@xxxxxxxxx]
> Sent: Wednesday, October 19, 2016 5:18 PM
> To: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> Cc: Bough Chen <haibo.chen@xxxxxxx>; Adrian Hunter
> <adrian.hunter@xxxxxxxxx>; linux-mmc <linux-mmc@xxxxxxxxxxxxxxx>; A.S.
> Dong <aisheng.dong@xxxxxxx>
> Subject: Re: [PATCH] mmc: sdhci-esdhc-imx: make sure usdhc clock enabled
> while doing suspend
> 
> 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.
>
 
Agree

> > 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.
> 
> It may not be suitable to extend runtime operations to be similar as sleep pm
> operations 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;
> }
> 

I think this method still has the same issue.
According to the /Documentation/power/runtime_pm.txt

370   int pm_runtime_get_sync(struct device *dev);
371     - increment the device's usage counter, run pm_runtime_resume(dev) and
372       return its result

385   int pm_runtime_put_sync(struct device *dev);
386     - decrement the device's usage counter; if the result is 0 then run
387       pm_runtime_idle(dev) and return its result

pm_runtime_put_sync() will not call pm_runtime_suspend(), so clock still enabled, the same issue.

> 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?
> 
> > If that is possible, you can instead deploy the runtime PM centric
> > approach and get system suspend/resume for free. All you would have to
> > do is to assign the system PM callbacks to
> > pm_runtime_force_suspend|resume(). In that way, the above problem
> > would be solved and you don't need to keep the clocks enabled during
> > system suspend/resume.
> >
> > Kind regards
> > Uffe
> >
> 
> Regards
> Dong Aisheng
> 
> >>
> >> Signed-off-by: Haibo Chen <haibo.chen@xxxxxxx>
> >> ---
> >>  drivers/mmc/host/sdhci-esdhc-imx.c | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> >> b/drivers/mmc/host/sdhci-esdhc-imx.c
> >> index 7123ef9..1df3846 100644
> >> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> >> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> >> @@ -1322,17 +1322,28 @@ static int sdhci_esdhc_suspend(struct device
> >> *dev)  {
> >>         struct sdhci_host *host = dev_get_drvdata(dev);
> >>
> >> +#ifdef CONFIG_PM
> >> +       pm_runtime_get_sync(host->mmc->parent);
> >> +#endif
> >> +
> >>         return sdhci_suspend_host(host);  }
> >>
> >>  static int sdhci_esdhc_resume(struct device *dev)  {
> >>         struct sdhci_host *host = dev_get_drvdata(dev);
> >> +       int ret;
> >>
> >>         /* re-initialize hw state in case it's lost in low power mode */
> >>         sdhci_esdhc_imx_hwinit(host);
> >> +       ret = sdhci_resume_host(host);
> >>
> >> -       return sdhci_resume_host(host);
> >> +#ifdef CONFIG_PM
> >> +       pm_runtime_mark_last_busy(host->mmc->parent);
> >> +       pm_runtime_put_autosuspend(host->mmc->parent);
> >> +#endif
> >> +
> >> +       return ret;
> >>  }
> >>  #endif
> >>
> >> --
> >> 1.9.1
> >>
��.n��������+%������w��{.n�����{��i��)��jg��������ݢj����G�������j:+v���w�m������w�������h�����٥




[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux