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@xxxxxxxxxxxx> wrote:
> Doug,
>
> On Wed, Nov 19, 2014 at 11:30 AM, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>> Andrew,
>>
>> On Wed, Nov 19, 2014 at 11:03 AM, Andrew Bresticker
>> <abrestic@xxxxxxxxxxxx> wrote:
>>> Doug,
>>>
>>> On Wed, Nov 19, 2014 at 10:51 AM, Doug Anderson <dianders@xxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/