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

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

[Haibo Chen]

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(-)

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

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

[Ulf Hansson]

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!?

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?

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

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

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

[Dong Aisheng]

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

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

[Bough Chen]

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

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

[Dong Aisheng]

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

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

[Ulf Hansson]

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

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

[Dong Aisheng]

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

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

[Dong Aisheng]

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