| Message ID | 1416423073-8105-1-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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
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?
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
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;
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(-)