> -----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�����٥
