| Message ID | 1476776340-23718-1-git-send-email-haibo.chen@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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; } 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@nxp.com> >> --- >> 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 >> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Haibo, On Tue, Oct 18, 2016 at 3:39 PM, 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. > Besides the former comments in my last reply, it would be good to explain a bit more why the former SoC series do not have this issue in the patch commit message. That would make people more clear about it. And more minor comment below: > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > --- > 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 Do you really need this? > + 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 ditto > + pm_runtime_mark_last_busy(host->mmc->parent); > + pm_runtime_put_autosuspend(host->mmc->parent); > +#endif > + > + return ret; > } > #endif > > -- > 1.9.1 > Regards Dong Aisheg -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Dong Aisheng [mailto:dongas86@gmail.com] > Sent: Wednesday, October 19, 2016 5:18 PM > To: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Bough Chen <haibo.chen@nxp.com>; Adrian Hunter > <adrian.hunter@intel.com>; linux-mmc <linux-mmc@vger.kernel.org>; A.S. > Dong <aisheng.dong@nxp.com> > 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@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. > 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@nxp.com> > >> --- > >> 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 > >>
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 20, 2016 at 10:21:46AM +0000, Bough Chen wrote: > > > > -----Original Message----- > > From: Dong Aisheng [mailto:dongas86@gmail.com] > > Sent: Wednesday, October 19, 2016 5:18 PM > > To: Ulf Hansson <ulf.hansson@linaro.org> > > Cc: Bough Chen <haibo.chen@nxp.com>; Adrian Hunter > > <adrian.hunter@intel.com>; linux-mmc <linux-mmc@vger.kernel.org>; A.S. > > Dong <aisheng.dong@nxp.com> > > 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@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. > > > > 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. > Yes, you're right. Device may be already in runtime resumed state before. Regards Dong Aisheng > > 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@nxp.com> > > >> --- > > >> 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 > > >> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. Signed-off-by: Haibo Chen <haibo.chen@nxp.com> --- drivers/mmc/host/sdhci-esdhc-imx.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)