[PATCH] mmc: dw_mmc: Turn the card clock off at suspend time

[PATCH] mmc: dw_mmc: Turn the card clock off at suspend time

[Doug Anderson]

Since the dw_mmc driver was first added to Linux it's had a TODO in it
that we should turn off the card clock during suspend.  I have no idea
for sure why it wasn't done originally, but if I had to guess I'd
guess it was related to the lack of a common clock framework.  Let's
do it now.

There is no reason for the card clock to be left on during suspend and
most systems will eventually turn it off anyway (when whole clock
trees are disabled).  However, it's good to be explicit that it's
disabled at the time that the MMC subsystem is disabled.

This actually fixes a bug on Rockchip rk3288 where an SDIO card wakes
the system back up during suspend.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/mmc/host/dw_mmc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 5a37c33..c448159 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2825,11 +2825,10 @@ EXPORT_SYMBOL(dw_mci_remove);
 
 
 #ifdef CONFIG_PM_SLEEP
-/*
- * TODO: we should probably disable the clock to the card in the suspend path.
- */
 int dw_mci_suspend(struct dw_mci *host)
 {
+	clk_disable(host->ciu_clk);
+
 	return 0;
 }
 EXPORT_SYMBOL(dw_mci_suspend);
@@ -2838,6 +2837,8 @@ int dw_mci_resume(struct dw_mci *host)
 {
 	int i, ret;
 
+	clk_enable(host->ciu_clk);
+
 	if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) {
 		ret = -ENODEV;
 		return ret;
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH] mmc: dw_mmc: Turn the card clock off at suspend time

[Andrew Bresticker]

Doug,

On Wed, Nov 19, 2014 at 10:51 AM, Doug Anderson <dianders@chromium.org> wrote:
> Since the dw_mmc driver was first added to Linux it's had a TODO in it
> that we should turn off the card clock during suspend.  I have no idea
> for sure why it wasn't done originally, but if I had to guess I'd
> guess it was related to the lack of a common clock framework.  Let's
> do it now.
>
> There is no reason for the card clock to be left on during suspend and
> most systems will eventually turn it off anyway (when whole clock
> trees are disabled).  However, it's good to be explicit that it's
> disabled at the time that the MMC subsystem is disabled.

Should the bus clock (biu) be disabled as well?

> This actually fixes a bug on Rockchip rk3288 where an SDIO card wakes
> the system back up during suspend.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/mmc/host/dw_mmc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 5a37c33..c448159 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2825,11 +2825,10 @@ EXPORT_SYMBOL(dw_mci_remove);
>
>
>  #ifdef CONFIG_PM_SLEEP
> -/*
> - * TODO: we should probably disable the clock to the card in the suspend path.
> - */
>  int dw_mci_suspend(struct dw_mci *host)
>  {
> +       clk_disable(host->ciu_clk);

I think you need to check for IS_ERR(host->ciu_clk) since the clock is
optional.  Also, maybe disable_unprepare instead of just disable?

> @@ -2838,6 +2837,8 @@ int dw_mci_resume(struct dw_mci *host)
>  {
>         int i, ret;
>
> +       clk_enable(host->ciu_clk);

Check return value?

Re: [PATCH] mmc: dw_mmc: Turn the card clock off at suspend time

[Doug Anderson]

Andrew,

On Wed, Nov 19, 2014 at 11:03 AM, Andrew Bresticker
<abrestic@chromium.org> wrote:
> Doug,
>
> On Wed, Nov 19, 2014 at 10:51 AM, Doug Anderson <dianders@chromium.org> wrote:
>> Since the dw_mmc driver was first added to Linux it's had a TODO in it
>> that we should turn off the card clock during suspend.  I have no idea
>> for sure why it wasn't done originally, but if I had to guess I'd
>> guess it was related to the lack of a common clock framework.  Let's
>> do it now.
>>
>> There is no reason for the card clock to be left on during suspend and
>> most systems will eventually turn it off anyway (when whole clock
>> trees are disabled).  However, it's good to be explicit that it's
>> disabled at the time that the MMC subsystem is disabled.
>
> Should the bus clock (biu) be disabled as well?

Good question.  I'm slightly hesitant to turn biu_clk off in this
patch, though.  Specifically interrupts are still enabled at this
point in suspend.  I  guess most interrupts shouldn't be going off
right now (nobody is accessing storage, right?), but I could imagine
that a card detect or an SDIO interrupt at just the wrong time could
cause our ISR to go off _after_ this function is called.  The
interrupt handling function doesn't know to turn the BIU clock back on
so you'd get a hang.

...you'll also (I think) get a hang in exynos which has a "no_irq"
function and tries to access dw_mmc without turning on the biu clock.
...of course that wouldn't be hard to fix, but...


Turning off the biu clock when it's not needed seems like a good idea,
but I think it should be a separate patch.  Also, unlike the ciu clock
leaving the biu clock on doesn't seem to hurt anything on the system
I'm currently testing on.


>> This actually fixes a bug on Rockchip rk3288 where an SDIO card wakes
>> the system back up during suspend.
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> ---
>>  drivers/mmc/host/dw_mmc.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 5a37c33..c448159 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -2825,11 +2825,10 @@ EXPORT_SYMBOL(dw_mci_remove);
>>
>>
>>  #ifdef CONFIG_PM_SLEEP
>> -/*
>> - * TODO: we should probably disable the clock to the card in the suspend path.
>> - */
>>  int dw_mci_suspend(struct dw_mci *host)
>>  {
>> +       clk_disable(host->ciu_clk);
>
> I think you need to check for IS_ERR(host->ciu_clk) since the clock is
> optional.  Also, maybe disable_unprepare instead of just disable?

Wow, duh.  Thanks for catching.


>> @@ -2838,6 +2837,8 @@ int dw_mci_resume(struct dw_mci *host)
>>  {
>>         int i, ret;
>>
>> +       clk_enable(host->ciu_clk);
>
> Check return value?

OK, sounds good.  I'll spin this again.

-Doug

Re: [PATCH] mmc: dw_mmc: Turn the card clock off at suspend time

[Andrew Bresticker]

Doug,

On Wed, Nov 19, 2014 at 11:30 AM, Doug Anderson <dianders@chromium.org> wrote:
> Andrew,
>
> On Wed, Nov 19, 2014 at 11:03 AM, Andrew Bresticker
> <abrestic@chromium.org> wrote:
>> Doug,
>>
>> On Wed, Nov 19, 2014 at 10:51 AM, Doug Anderson <dianders@chromium.org> wrote:
>>> Since the dw_mmc driver was first added to Linux it's had a TODO in it
>>> that we should turn off the card clock during suspend.  I have no idea
>>> for sure why it wasn't done originally, but if I had to guess I'd
>>> guess it was related to the lack of a common clock framework.  Let's
>>> do it now.
>>>
>>> There is no reason for the card clock to be left on during suspend and
>>> most systems will eventually turn it off anyway (when whole clock
>>> trees are disabled).  However, it's good to be explicit that it's
>>> disabled at the time that the MMC subsystem is disabled.
>>
>> Should the bus clock (biu) be disabled as well?
>
> Good question.  I'm slightly hesitant to turn biu_clk off in this
> patch, though.  Specifically interrupts are still enabled at this
> point in suspend.  I  guess most interrupts shouldn't be going off
> right now (nobody is accessing storage, right?), but I could imagine
> that a card detect or an SDIO interrupt at just the wrong time could
> cause our ISR to go off _after_ this function is called.  The
> interrupt handling function doesn't know to turn the BIU clock back on
> so you'd get a hang.

Perhaps interrupts should be disabled as well across suspend/resume,
like the sdhci-based hosts do?  Would would happen if we tried to
service an interrupt with the ciu clock disabled?

Re: [PATCH] mmc: dw_mmc: Turn the card clock off at suspend time

[Doug Anderson]

Andrew,

On Wed, Nov 19, 2014 at 11:49 AM, Andrew Bresticker
<abrestic@chromium.org> wrote:
> Doug,
>
> On Wed, Nov 19, 2014 at 11:30 AM, Doug Anderson <dianders@chromium.org> wrote:
>> Andrew,
>>
>> On Wed, Nov 19, 2014 at 11:03 AM, Andrew Bresticker
>> <abrestic@chromium.org> wrote:
>>> Doug,
>>>
>>> On Wed, Nov 19, 2014 at 10:51 AM, Doug Anderson <dianders@chromium.org> wrote:
>>>> Since the dw_mmc driver was first added to Linux it's had a TODO in it
>>>> that we should turn off the card clock during suspend.  I have no idea
>>>> for sure why it wasn't done originally, but if I had to guess I'd
>>>> guess it was related to the lack of a common clock framework.  Let's
>>>> do it now.
>>>>
>>>> There is no reason for the card clock to be left on during suspend and
>>>> most systems will eventually turn it off anyway (when whole clock
>>>> trees are disabled).  However, it's good to be explicit that it's
>>>> disabled at the time that the MMC subsystem is disabled.
>>>
>>> Should the bus clock (biu) be disabled as well?
>>
>> Good question.  I'm slightly hesitant to turn biu_clk off in this
>> patch, though.  Specifically interrupts are still enabled at this
>> point in suspend.  I  guess most interrupts shouldn't be going off
>> right now (nobody is accessing storage, right?), but I could imagine
>> that a card detect or an SDIO interrupt at just the wrong time could
>> cause our ISR to go off _after_ this function is called.  The
>> interrupt handling function doesn't know to turn the BIU clock back on
>> so you'd get a hang.
>
> Perhaps interrupts should be disabled as well across suspend/resume,
> like the sdhci-based hosts do?

Well, interrupts _are_ disabled across suspend/resume.  ...just not at
the point this function runs.  You're right thought that I could do my
work in the "no_irq" variant of the function.

> Would would happen if we tried to
> service an interrupt with the ciu clock disabled?

Hrm, good question.  I was thinking that all would be OK because the
interrupt would kick off something to the MMC layer which would then
realize that the system was suspended, but I that's actually not the
case.

It turns out that the root cause of my problem was actually SDIO
interrupts coming after suspend and causing problems.  It looks like
this might not be a problem in mainline Linux but it was a problem in
my old 3.14-based tree.  Once I fixed that I no longer need this
patch.  Sorry for the noise.  :(  I've been trying to be in the habit
of sending stuff upstream immediately (so I don't forget about stuff),
but a bad side effect of that is some extra noise...  Sigh.


Anyway, I guess I'd say that we should consider this patch abandoned,
though if someone would like me to submit something that turns off
"ciu" and "biu" clock in the "noirq" suspend I'm happy to do it.  I
don't have any strong need now though it would get rid of the TODO
sitting in the code.  ...or we could just remove the TODO.


-Doug