* [PATCH v6 0/3] mmc: dw_mmc: fixes for suspend/resume on exynos @ 2013-08-22 16:19 ` Doug Anderson 0 siblings, 0 replies; 25+ messages in thread From: Doug Anderson @ 2013-08-22 16:19 UTC (permalink / raw) To: Chris Ball Cc: Jaehoon Chung, Seungwon Jeon, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, Doug Anderson, linux-samsung-soc, linux-kernel, linux-mmc, Kukjin Kim, linux-arm-kernel This series of patches addresses some suspend/resume problems with dw_mmc on exynos platforms, espeically exynos5420. This patchset was tested on the current ToT Chromeos 3.8 tree (which has lots of backports from 3.10/3.11) and on ToT Linux (v3.11-rc6). I have confirmed basic booting and that SD cards work across suspend/resume (both if they are plugged in and if they are not plugged in). I have received confirmation from Samsung that the problem solved for exynos5420 is a silicon errata and that this is a good fix. Changes in v6: - Took out TODO comment copied from main platform code. - Replaces previous pathes that ensured saving/restoring clocks. Changes in v5: - Cleaned up dw_mci_exynos_resume_noirq() comment as per Seungwon. - Don't memcpy dev_pm_ops structure, define a new one. Changes in v4: - Take Seungwon's suggestion and don't add any dw_mmc-pltfm code. Changes in v3: - Add freeze/thaw and poweroff/restore noirq entries. Changes in v2: - Use suspend_noirq as per James Hogan. Doug Anderson (3): mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) mmc: dw_mmc: Set timeout to max upon resume drivers/mmc/host/dw_mmc-exynos.c | 53 +++++++++++++++++++++++++++++++++++++++- drivers/mmc/host/dw_mmc.c | 24 ++++++++++-------- 2 files changed, 66 insertions(+), 11 deletions(-) -- 1.8.3 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v6 0/3] mmc: dw_mmc: fixes for suspend/resume on exynos @ 2013-08-22 16:19 ` Doug Anderson 0 siblings, 0 replies; 25+ messages in thread From: Doug Anderson @ 2013-08-22 16:19 UTC (permalink / raw) To: linux-arm-kernel This series of patches addresses some suspend/resume problems with dw_mmc on exynos platforms, espeically exynos5420. This patchset was tested on the current ToT Chromeos 3.8 tree (which has lots of backports from 3.10/3.11) and on ToT Linux (v3.11-rc6). I have confirmed basic booting and that SD cards work across suspend/resume (both if they are plugged in and if they are not plugged in). I have received confirmation from Samsung that the problem solved for exynos5420 is a silicon errata and that this is a good fix. Changes in v6: - Took out TODO comment copied from main platform code. - Replaces previous pathes that ensured saving/restoring clocks. Changes in v5: - Cleaned up dw_mci_exynos_resume_noirq() comment as per Seungwon. - Don't memcpy dev_pm_ops structure, define a new one. Changes in v4: - Take Seungwon's suggestion and don't add any dw_mmc-pltfm code. Changes in v3: - Add freeze/thaw and poweroff/restore noirq entries. Changes in v2: - Use suspend_noirq as per James Hogan. Doug Anderson (3): mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) mmc: dw_mmc: Set timeout to max upon resume drivers/mmc/host/dw_mmc-exynos.c | 53 +++++++++++++++++++++++++++++++++++++++- drivers/mmc/host/dw_mmc.c | 24 ++++++++++-------- 2 files changed, 66 insertions(+), 11 deletions(-) -- 1.8.3 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v6 1/3] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT 2013-08-22 16:19 ` Doug Anderson @ 2013-08-22 16:19 ` Doug Anderson -1 siblings, 0 replies; 25+ messages in thread From: Doug Anderson @ 2013-08-22 16:19 UTC (permalink / raw) To: Chris Ball Cc: Jaehoon Chung, Seungwon Jeon, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, Doug Anderson, Kukjin Kim, linux-mmc, linux-arm-kernel, linux-samsung-soc, linux-kernel If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up looping around forever. This has been seen to happen on exynos5420 silicon despite the fact that we haven't enabled any wakeup events due to a silicon errata. It is safe to do on all exynos variants. Signed-off-by: Doug Anderson <dianders@chromium.org> --- Changes in v6: - Took out TODO comment copied from main platform code. Changes in v5: - Cleaned up dw_mci_exynos_resume_noirq() comment as per Seungwon. - Don't memcpy dev_pm_ops structure, define a new one. Changes in v4: - Take Seungwon's suggestion and don't add any dw_mmc-pltfm code. Changes in v3: - Add freeze/thaw and poweroff/restore noirq entries. Changes in v2: - Use suspend_noirq as per James Hogan. drivers/mmc/host/dw_mmc-exynos.c | 53 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c index 866edef..e8c6cc9 100644 --- a/drivers/mmc/host/dw_mmc-exynos.c +++ b/drivers/mmc/host/dw_mmc-exynos.c @@ -30,6 +30,7 @@ #define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \ SDMMC_CLKSEL_CCLK_DRIVE(y) | \ SDMMC_CLKSEL_CCLK_DIVIDER(z)) +#define SDMMC_CLKSEL_WAKEUP_INT BIT(11) #define EXYNOS4210_FIXED_CIU_CLK_DIV 2 #define EXYNOS4412_FIXED_CIU_CLK_DIV 4 @@ -100,6 +101,49 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host) return 0; } +#ifdef CONFIG_PM_SLEEP +static int dw_mci_exynos_suspend(struct device *dev) +{ + struct dw_mci *host = dev_get_drvdata(dev); + + return dw_mci_suspend(host); +} + +static int dw_mci_exynos_resume(struct device *dev) +{ + struct dw_mci *host = dev_get_drvdata(dev); + + return dw_mci_resume(host); +} + +/** + * dw_mci_exynos_resume_noirq - Exynos-specific resume code + * + * On exynos5420 there is a silicon errata that will sometimes leave the + * WAKEUP_INT bit in the CLKSEL register asserted. This bit is 1 to indicate + * that it fired and we can clear it by writing a 1 back. Clear it to prevent + * interrupts from going off constantly. + * + * We run this code on all exynos variants because it doesn't hurt. + */ + +static int dw_mci_exynos_resume_noirq(struct device *dev) +{ + struct dw_mci *host = dev_get_drvdata(dev); + u32 clksel; + + clksel = mci_readl(host, CLKSEL); + if (clksel & SDMMC_CLKSEL_WAKEUP_INT) + mci_writel(host, CLKSEL, clksel); + + return 0; +} +#else +#define dw_mci_exynos_suspend NULL +#define dw_mci_exynos_resume NULL +#define dw_mci_exynos_resume_noirq NULL +#endif /* CONFIG_PM_SLEEP */ + static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr) { /* @@ -187,13 +231,20 @@ static int dw_mci_exynos_probe(struct platform_device *pdev) return dw_mci_pltfm_register(pdev, drv_data); } +const struct dev_pm_ops dw_mci_exynos_pmops = { + SET_SYSTEM_SLEEP_PM_OPS(dw_mci_exynos_suspend, dw_mci_exynos_resume) + .resume_noirq = dw_mci_exynos_resume_noirq, + .thaw_noirq = dw_mci_exynos_resume_noirq, + .restore_noirq = dw_mci_exynos_resume_noirq, +}; + static struct platform_driver dw_mci_exynos_pltfm_driver = { .probe = dw_mci_exynos_probe, .remove = __exit_p(dw_mci_pltfm_remove), .driver = { .name = "dwmmc_exynos", .of_match_table = dw_mci_exynos_match, - .pm = &dw_mci_pltfm_pmops, + .pm = &dw_mci_exynos_pmops, }, }; -- 1.8.3 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v6 1/3] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT @ 2013-08-22 16:19 ` Doug Anderson 0 siblings, 0 replies; 25+ messages in thread From: Doug Anderson @ 2013-08-22 16:19 UTC (permalink / raw) To: linux-arm-kernel If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up looping around forever. This has been seen to happen on exynos5420 silicon despite the fact that we haven't enabled any wakeup events due to a silicon errata. It is safe to do on all exynos variants. Signed-off-by: Doug Anderson <dianders@chromium.org> --- Changes in v6: - Took out TODO comment copied from main platform code. Changes in v5: - Cleaned up dw_mci_exynos_resume_noirq() comment as per Seungwon. - Don't memcpy dev_pm_ops structure, define a new one. Changes in v4: - Take Seungwon's suggestion and don't add any dw_mmc-pltfm code. Changes in v3: - Add freeze/thaw and poweroff/restore noirq entries. Changes in v2: - Use suspend_noirq as per James Hogan. drivers/mmc/host/dw_mmc-exynos.c | 53 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c index 866edef..e8c6cc9 100644 --- a/drivers/mmc/host/dw_mmc-exynos.c +++ b/drivers/mmc/host/dw_mmc-exynos.c @@ -30,6 +30,7 @@ #define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \ SDMMC_CLKSEL_CCLK_DRIVE(y) | \ SDMMC_CLKSEL_CCLK_DIVIDER(z)) +#define SDMMC_CLKSEL_WAKEUP_INT BIT(11) #define EXYNOS4210_FIXED_CIU_CLK_DIV 2 #define EXYNOS4412_FIXED_CIU_CLK_DIV 4 @@ -100,6 +101,49 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host) return 0; } +#ifdef CONFIG_PM_SLEEP +static int dw_mci_exynos_suspend(struct device *dev) +{ + struct dw_mci *host = dev_get_drvdata(dev); + + return dw_mci_suspend(host); +} + +static int dw_mci_exynos_resume(struct device *dev) +{ + struct dw_mci *host = dev_get_drvdata(dev); + + return dw_mci_resume(host); +} + +/** + * dw_mci_exynos_resume_noirq - Exynos-specific resume code + * + * On exynos5420 there is a silicon errata that will sometimes leave the + * WAKEUP_INT bit in the CLKSEL register asserted. This bit is 1 to indicate + * that it fired and we can clear it by writing a 1 back. Clear it to prevent + * interrupts from going off constantly. + * + * We run this code on all exynos variants because it doesn't hurt. + */ + +static int dw_mci_exynos_resume_noirq(struct device *dev) +{ + struct dw_mci *host = dev_get_drvdata(dev); + u32 clksel; + + clksel = mci_readl(host, CLKSEL); + if (clksel & SDMMC_CLKSEL_WAKEUP_INT) + mci_writel(host, CLKSEL, clksel); + + return 0; +} +#else +#define dw_mci_exynos_suspend NULL +#define dw_mci_exynos_resume NULL +#define dw_mci_exynos_resume_noirq NULL +#endif /* CONFIG_PM_SLEEP */ + static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr) { /* @@ -187,13 +231,20 @@ static int dw_mci_exynos_probe(struct platform_device *pdev) return dw_mci_pltfm_register(pdev, drv_data); } +const struct dev_pm_ops dw_mci_exynos_pmops = { + SET_SYSTEM_SLEEP_PM_OPS(dw_mci_exynos_suspend, dw_mci_exynos_resume) + .resume_noirq = dw_mci_exynos_resume_noirq, + .thaw_noirq = dw_mci_exynos_resume_noirq, + .restore_noirq = dw_mci_exynos_resume_noirq, +}; + static struct platform_driver dw_mci_exynos_pltfm_driver = { .probe = dw_mci_exynos_probe, .remove = __exit_p(dw_mci_pltfm_remove), .driver = { .name = "dwmmc_exynos", .of_match_table = dw_mci_exynos_match, - .pm = &dw_mci_pltfm_pmops, + .pm = &dw_mci_exynos_pmops, }, }; -- 1.8.3 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) 2013-08-22 16:19 ` Doug Anderson (?) (?) @ 2013-08-22 16:19 ` Doug Anderson 2013-08-23 13:21 ` Jaehoon Chung 2013-08-26 4:34 ` Seungwon Jeon -1 siblings, 2 replies; 25+ messages in thread From: Doug Anderson @ 2013-08-22 16:19 UTC (permalink / raw) To: Chris Ball Cc: Jaehoon Chung, Seungwon Jeon, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, Doug Anderson, linux-mmc, linux-kernel Previously the dw_mmc driver would ignore any requests to disable the card's clock. This doesn't seem like a good thing in general, but had one extra bad side effect in the following situtation: * mmc core would set clk to 400kHz at boot time while initting * mmc core would set clk to 0 since no card, but it would be ignored. * suspend to ram and resume; clocks in the dw_mmc IP block are now 0 but dw_mmc thinks that they're 400kHz (it ignored the set to 0). * insert card * mmc core would set clk to 400kHz which would be considered a no-op. Note that if there is no card in the slot and we do a suspend/resume cycle, we _do_ still end up with differences in a dw_mmc register dump, but the differences are clock related and we've got the clock disabled both before and after, so this should be OK. Signed-off-by: Doug Anderson <dianders@chromium.org> --- Changes in v6: - Replaces previous pathes that ensured saving/restoring clocks. drivers/mmc/host/dw_mmc.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index ee5f167..f6c7545 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -635,7 +635,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) u32 div; u32 clk_en_a; - if (slot->clock != host->current_speed || force_clkinit) { + if (slot->clock == 0) { + mci_writel(host, CLKENA, 0); + mci_send_cmd(slot, + SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); + } else if (slot->clock != host->current_speed || force_clkinit) { div = host->bus_hz / slot->clock; if (host->bus_hz % slot->clock && host->bus_hz > slot->clock) /* @@ -675,9 +679,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) /* inform CIU */ mci_send_cmd(slot, SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); - - host->current_speed = slot->clock; } + host->current_speed = slot->clock; /* Set the current slot bus width */ mci_writel(host, CTYPE, (slot->ctype << slot->id)); @@ -807,13 +810,11 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) mci_writel(slot->host, UHS_REG, regs); - if (ios->clock) { - /* - * Use mirror of ios->clock to prevent race with mmc - * core ios update when finding the minimum. - */ - slot->clock = ios->clock; - } + /* + * Use mirror of ios->clock to prevent race with mmc + * core ios update when finding the minimum. + */ + slot->clock = ios->clock; if (drv_data && drv_data->set_ios) drv_data->set_ios(slot->host, ios); -- 1.8.3 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) 2013-08-22 16:19 ` [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) Doug Anderson @ 2013-08-23 13:21 ` Jaehoon Chung 2013-08-23 20:40 ` Doug Anderson 2013-08-26 4:34 ` Seungwon Jeon 1 sibling, 1 reply; 25+ messages in thread From: Jaehoon Chung @ 2013-08-23 13:21 UTC (permalink / raw) To: Doug Anderson Cc: Chris Ball, Seungwon Jeon, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, linux-mmc, linux-kernel Hi Doug, If the clock-gating is enabled, then maybe it's continuously printed the kernel message for Bus_speed. Best Regards, Jaehoon Chung On 08/23/2013 01:19 AM, Doug Anderson wrote: > Previously the dw_mmc driver would ignore any requests to disable the > card's clock. This doesn't seem like a good thing in general, but had > one extra bad side effect in the following situtation: > * mmc core would set clk to 400kHz at boot time while initting > * mmc core would set clk to 0 since no card, but it would be ignored. > * suspend to ram and resume; clocks in the dw_mmc IP block are now 0 > but dw_mmc thinks that they're 400kHz (it ignored the set to 0). > * insert card > * mmc core would set clk to 400kHz which would be considered a no-op. > > Note that if there is no card in the slot and we do a suspend/resume > cycle, we _do_ still end up with differences in a dw_mmc register > dump, but the differences are clock related and we've got the clock > disabled both before and after, so this should be OK. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > --- > Changes in v6: > - Replaces previous pathes that ensured saving/restoring clocks. > > drivers/mmc/host/dw_mmc.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index ee5f167..f6c7545 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -635,7 +635,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) > u32 div; > u32 clk_en_a; > > - if (slot->clock != host->current_speed || force_clkinit) { > + if (slot->clock == 0) { > + mci_writel(host, CLKENA, 0); > + mci_send_cmd(slot, > + SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); > + } else if (slot->clock != host->current_speed || force_clkinit) { > div = host->bus_hz / slot->clock; > if (host->bus_hz % slot->clock && host->bus_hz > slot->clock) > /* > @@ -675,9 +679,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) > /* inform CIU */ > mci_send_cmd(slot, > SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); > - > - host->current_speed = slot->clock; > } > + host->current_speed = slot->clock; > > /* Set the current slot bus width */ > mci_writel(host, CTYPE, (slot->ctype << slot->id)); > @@ -807,13 +810,11 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > > mci_writel(slot->host, UHS_REG, regs); > > - if (ios->clock) { > - /* > - * Use mirror of ios->clock to prevent race with mmc > - * core ios update when finding the minimum. > - */ > - slot->clock = ios->clock; > - } > + /* > + * Use mirror of ios->clock to prevent race with mmc > + * core ios update when finding the minimum. > + */ > + slot->clock = ios->clock; > > if (drv_data && drv_data->set_ios) > drv_data->set_ios(slot->host, ios); > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) 2013-08-23 13:21 ` Jaehoon Chung @ 2013-08-23 20:40 ` Doug Anderson 2013-08-26 1:31 ` Jaehoon Chung 0 siblings, 1 reply; 25+ messages in thread From: Doug Anderson @ 2013-08-23 20:40 UTC (permalink / raw) To: Jaehoon Chung Cc: Chris Ball, Seungwon Jeon, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, linux-mmc, linux-kernel Jaehoon, On Fri, Aug 23, 2013 at 6:21 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote: > Hi Doug, > > If the clock-gating is enabled, then maybe it's continuously printed the kernel message for Bus_speed. Can you explain? I don't think dw_mmc has support for clock gating right now. ...or are there some patches that I'm not aware of? I could believe that if you've got some non-upstream clock gating patches that these would need to be modified to handle it... ...but unless those are slated to land upstream it seems like I can't take them into account, can I? Thanks! -Doug ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) 2013-08-23 20:40 ` Doug Anderson @ 2013-08-26 1:31 ` Jaehoon Chung 2013-08-26 3:38 ` Doug Anderson 0 siblings, 1 reply; 25+ messages in thread From: Jaehoon Chung @ 2013-08-26 1:31 UTC (permalink / raw) To: Doug Anderson Cc: Jaehoon Chung, Chris Ball, Seungwon Jeon, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, linux-mmc, linux-kernel Hi Doug, On 08/24/2013 05:40 AM, Doug Anderson wrote: > Jaehoon, > > On Fri, Aug 23, 2013 at 6:21 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote: >> Hi Doug, >> >> If the clock-gating is enabled, then maybe it's continuously printed the kernel message for Bus_speed. > > Can you explain? I don't think dw_mmc has support for clock gating > right now. ...or are there some patches that I'm not aware of? I > could believe that if you've got some non-upstream clock gating > patches that these would need to be modified to handle it... ...but > unless those are slated to land upstream it seems like I can't take > them into account, can I? If i enabled the CONFIG_MMC_CLK_GATE, the i have found the below message whenever some operation is run. I will test more with your patch. [ 6.335000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1) [ 6.345000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1) [ 6.355000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1) [ 6.365000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1) [ 6.375000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1) [ 6.480000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1) [ 6.490000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1) [ 6.500000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1) [ 6.530000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1) [ 6.535000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1) [ 6.545000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1) Best Regards, Jaehoon Chung > > Thanks! > > -Doug > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) 2013-08-26 1:31 ` Jaehoon Chung @ 2013-08-26 3:38 ` Doug Anderson 0 siblings, 0 replies; 25+ messages in thread From: Doug Anderson @ 2013-08-26 3:38 UTC (permalink / raw) To: Jaehoon Chung Cc: Chris Ball, Seungwon Jeon, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, linux-mmc, linux-kernel Jaehoon, On Sun, Aug 25, 2013 at 6:31 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote: > Hi Doug, > > On 08/24/2013 05:40 AM, Doug Anderson wrote: >> Jaehoon, >> >> On Fri, Aug 23, 2013 at 6:21 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote: >>> Hi Doug, >>> >>> If the clock-gating is enabled, then maybe it's continuously printed the kernel message for Bus_speed. >> >> Can you explain? I don't think dw_mmc has support for clock gating >> right now. ...or are there some patches that I'm not aware of? I >> could believe that if you've got some non-upstream clock gating >> patches that these would need to be modified to handle it... ...but >> unless those are slated to land upstream it seems like I can't take >> them into account, can I? > If i enabled the CONFIG_MMC_CLK_GATE, the i have found the below message whenever some operation is run. > I will test more with your patch. Ah, sorry! I wasn't aware of that config option. I was thinking of automatic clock gating based on something like the common clock framework. When there are no more users of a gate clock it will get turned off. To have that work dw_mmc would need to release its biu / ciu clocks at some point. If I had to guess, I'd speculate that perhaps we should just change the printout to a dev_debug(), though I do find that printout incredibly useful. If I had to guess I'd say that the mmc core is switching between a clock of 0 and a full speed clock constantly. If that's true then it means that dw_mmc used to treat that like a no-op. Now it actually gates the clock. If you comment out the printout, do things still work? Does your power consumption go down? Let me know if you find anything. Otherwise I can try to reproduce this week. -Doug ^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) 2013-08-22 16:19 ` [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) Doug Anderson 2013-08-23 13:21 ` Jaehoon Chung @ 2013-08-26 4:34 ` Seungwon Jeon 2013-08-26 9:06 ` Jaehoon Chung 1 sibling, 1 reply; 25+ messages in thread From: Seungwon Jeon @ 2013-08-26 4:34 UTC (permalink / raw) To: 'Doug Anderson', 'Chris Ball' Cc: 'Jaehoon Chung', 'James Hogan', 'Grant Grundler', 'Alim Akhtar', 'Abhilash Kesavan', 'Tomasz Figa', 'Olof Johansson', linux-mmc, linux-kernel On Fri, August 23, 2013, Doug Anderson wrote: > Previously the dw_mmc driver would ignore any requests to disable the > card's clock. This doesn't seem like a good thing in general, but had > one extra bad side effect in the following situtation: > * mmc core would set clk to 400kHz at boot time while initting > * mmc core would set clk to 0 since no card, but it would be ignored. > * suspend to ram and resume; clocks in the dw_mmc IP block are now 0 > but dw_mmc thinks that they're 400kHz (it ignored the set to 0). > * insert card > * mmc core would set clk to 400kHz which would be considered a no-op. > > Note that if there is no card in the slot and we do a suspend/resume > cycle, we _do_ still end up with differences in a dw_mmc register > dump, but the differences are clock related and we've got the clock > disabled both before and after, so this should be OK. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > --- > Changes in v6: > - Replaces previous pathes that ensured saving/restoring clocks. > > drivers/mmc/host/dw_mmc.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index ee5f167..f6c7545 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -635,7 +635,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) > u32 div; > u32 clk_en_a; > > - if (slot->clock != host->current_speed || force_clkinit) { > + if (slot->clock == 0) { > + mci_writel(host, CLKENA, 0); > + mci_send_cmd(slot, > + SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); Basically dw_mmc driver uses host's low power mode(auto clock gating) So, how about keeping origin code rather than programming clock setting to '0'? > + } else if (slot->clock != host->current_speed || force_clkinit) { And then, if condition('slot->clock is not zero') is added in order to allow to set clock, print messages which Jaehoon pointed would be solved. Thanks, Seungwon Jeon > div = host->bus_hz / slot->clock; > if (host->bus_hz % slot->clock && host->bus_hz > slot->clock) > /* > @@ -675,9 +679,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) > /* inform CIU */ > mci_send_cmd(slot, > SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); > - > - host->current_speed = slot->clock; > } > + host->current_speed = slot->clock; > > /* Set the current slot bus width */ > mci_writel(host, CTYPE, (slot->ctype << slot->id)); > @@ -807,13 +810,11 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > > mci_writel(slot->host, UHS_REG, regs); > > - if (ios->clock) { > - /* > - * Use mirror of ios->clock to prevent race with mmc > - * core ios update when finding the minimum. > - */ > - slot->clock = ios->clock; > - } > + /* > + * Use mirror of ios->clock to prevent race with mmc > + * core ios update when finding the minimum. > + */ > + slot->clock = ios->clock; > > if (drv_data && drv_data->set_ios) > drv_data->set_ios(slot->host, ios); > -- > 1.8.3 > > -- > 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) 2013-08-26 4:34 ` Seungwon Jeon @ 2013-08-26 9:06 ` Jaehoon Chung 2013-08-26 16:05 ` Doug Anderson 0 siblings, 1 reply; 25+ messages in thread From: Jaehoon Chung @ 2013-08-26 9:06 UTC (permalink / raw) To: Seungwon Jeon Cc: 'Doug Anderson', 'Chris Ball', 'Jaehoon Chung', 'James Hogan', 'Grant Grundler', 'Alim Akhtar', 'Abhilash Kesavan', 'Tomasz Figa', 'Olof Johansson', linux-mmc, linux-kernel On 08/26/2013 01:34 PM, Seungwon Jeon wrote: > On Fri, August 23, 2013, Doug Anderson wrote: >> Previously the dw_mmc driver would ignore any requests to disable the >> card's clock. This doesn't seem like a good thing in general, but had >> one extra bad side effect in the following situtation: >> * mmc core would set clk to 400kHz at boot time while initting >> * mmc core would set clk to 0 since no card, but it would be ignored. >> * suspend to ram and resume; clocks in the dw_mmc IP block are now 0 >> but dw_mmc thinks that they're 400kHz (it ignored the set to 0). >> * insert card >> * mmc core would set clk to 400kHz which would be considered a no-op. >> >> Note that if there is no card in the slot and we do a suspend/resume >> cycle, we _do_ still end up with differences in a dw_mmc register >> dump, but the differences are clock related and we've got the clock >> disabled both before and after, so this should be OK. >> >> Signed-off-by: Doug Anderson <dianders@chromium.org> >> --- >> Changes in v6: >> - Replaces previous pathes that ensured saving/restoring clocks. >> >> drivers/mmc/host/dw_mmc.c | 21 +++++++++++---------- >> 1 file changed, 11 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index ee5f167..f6c7545 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -635,7 +635,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) >> u32 div; >> u32 clk_en_a; >> >> - if (slot->clock != host->current_speed || force_clkinit) { >> + if (slot->clock == 0) { >> + mci_writel(host, CLKENA, 0); >> + mci_send_cmd(slot, >> + SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); > Basically dw_mmc driver uses host's low power mode(auto clock gating) > So, how about keeping origin code rather than programming clock setting to '0'? Right, Dw-mmc controller can use the Low-power mode. This mode is functionality like clock-gating. Well, i didn't know what benefit we get, if set to 0. Best Regards, Jaehoon Chung > >> + } else if (slot->clock != host->current_speed || force_clkinit) { > And then, if condition('slot->clock is not zero') is added in order to allow to set clock, > print messages which Jaehoon pointed would be solved. > > Thanks, > Seungwon Jeon > >> div = host->bus_hz / slot->clock; >> if (host->bus_hz % slot->clock && host->bus_hz > slot->clock) >> /* >> @@ -675,9 +679,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) >> /* inform CIU */ >> mci_send_cmd(slot, >> SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); >> - >> - host->current_speed = slot->clock; >> } >> + host->current_speed = slot->clock; >> >> /* Set the current slot bus width */ >> mci_writel(host, CTYPE, (slot->ctype << slot->id)); >> @@ -807,13 +810,11 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> >> mci_writel(slot->host, UHS_REG, regs); >> >> - if (ios->clock) { >> - /* >> - * Use mirror of ios->clock to prevent race with mmc >> - * core ios update when finding the minimum. >> - */ >> - slot->clock = ios->clock; >> - } >> + /* >> + * Use mirror of ios->clock to prevent race with mmc >> + * core ios update when finding the minimum. >> + */ >> + slot->clock = ios->clock; >> >> if (drv_data && drv_data->set_ios) >> drv_data->set_ios(slot->host, ios); >> -- >> 1.8.3 >> >> -- >> 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 > > -- > 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 > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) 2013-08-26 9:06 ` Jaehoon Chung @ 2013-08-26 16:05 ` Doug Anderson 2013-08-29 7:04 ` Seungwon Jeon 0 siblings, 1 reply; 25+ messages in thread From: Doug Anderson @ 2013-08-26 16:05 UTC (permalink / raw) To: Jaehoon Chung Cc: Seungwon Jeon, Chris Ball, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, linux-mmc, linux-kernel Jaehoon / Seungwon, On Mon, Aug 26, 2013 at 2:06 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote: > On 08/26/2013 01:34 PM, Seungwon Jeon wrote: >> On Fri, August 23, 2013, Doug Anderson wrote: >>> Previously the dw_mmc driver would ignore any requests to disable the >>> card's clock. This doesn't seem like a good thing in general, but had >>> one extra bad side effect in the following situtation: >>> * mmc core would set clk to 400kHz at boot time while initting >>> * mmc core would set clk to 0 since no card, but it would be ignored. >>> * suspend to ram and resume; clocks in the dw_mmc IP block are now 0 >>> but dw_mmc thinks that they're 400kHz (it ignored the set to 0). >>> * insert card >>> * mmc core would set clk to 400kHz which would be considered a no-op. >>> >>> Note that if there is no card in the slot and we do a suspend/resume >>> cycle, we _do_ still end up with differences in a dw_mmc register >>> dump, but the differences are clock related and we've got the clock >>> disabled both before and after, so this should be OK. >>> >>> Signed-off-by: Doug Anderson <dianders@chromium.org> >>> --- >>> Changes in v6: >>> - Replaces previous pathes that ensured saving/restoring clocks. >>> >>> drivers/mmc/host/dw_mmc.c | 21 +++++++++++---------- >>> 1 file changed, 11 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>> index ee5f167..f6c7545 100644 >>> --- a/drivers/mmc/host/dw_mmc.c >>> +++ b/drivers/mmc/host/dw_mmc.c >>> @@ -635,7 +635,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) >>> u32 div; >>> u32 clk_en_a; >>> >>> - if (slot->clock != host->current_speed || force_clkinit) { >>> + if (slot->clock == 0) { >>> + mci_writel(host, CLKENA, 0); >>> + mci_send_cmd(slot, >>> + SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); >> Basically dw_mmc driver uses host's low power mode(auto clock gating) >> So, how about keeping origin code rather than programming clock setting to '0'? > Right, Dw-mmc controller can use the Low-power mode. > This mode is functionality like clock-gating. Well, i didn't know what benefit we get, if set to 0. Ah, right. ...so it's unlikely that we'd save any power because we're already gating the clock. I'd really still rather honor the MMC subsystem's request. It shouldn't _hurt_ to turn the clock off when the subsystem requests it, right? One reason to honor the mmc core is that it will make things cleaner if/when we support a voltage change operation. The MMC core has the logic for the voltage change, and part of that involves turning off the clock. We'll already need a bunch of special case code in dw_mmc for voltage change, but it would be nice to avoid one extra bit. Another option would be to add a core MMC quirk to disable MMC_CLKGATE on a per-driver basis. In the "single zImage" world that seems like the right thing to do, since the MMC_CLKGATE description seems to indicate that enabling that won't really work on all drivers anyway. -Doug ^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) 2013-08-26 16:05 ` Doug Anderson @ 2013-08-29 7:04 ` Seungwon Jeon 2013-08-29 16:34 ` Doug Anderson 0 siblings, 1 reply; 25+ messages in thread From: Seungwon Jeon @ 2013-08-29 7:04 UTC (permalink / raw) To: 'Doug Anderson', 'Jaehoon Chung' Cc: 'Chris Ball', 'James Hogan', 'Grant Grundler', 'Alim Akhtar', 'Abhilash Kesavan', 'Tomasz Figa', 'Olof Johansson', linux-mmc, linux-kernel On Tuesday, August 27, 2013, Doug Anderson wrote: > Jaehoon / Seungwon, > > On Mon, Aug 26, 2013 at 2:06 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote: > > On 08/26/2013 01:34 PM, Seungwon Jeon wrote: > >> On Fri, August 23, 2013, Doug Anderson wrote: > >>> Previously the dw_mmc driver would ignore any requests to disable the > >>> card's clock. This doesn't seem like a good thing in general, but had > >>> one extra bad side effect in the following situtation: > >>> * mmc core would set clk to 400kHz at boot time while initting > >>> * mmc core would set clk to 0 since no card, but it would be ignored. > >>> * suspend to ram and resume; clocks in the dw_mmc IP block are now 0 > >>> but dw_mmc thinks that they're 400kHz (it ignored the set to 0). > >>> * insert card > >>> * mmc core would set clk to 400kHz which would be considered a no-op. > >>> > >>> Note that if there is no card in the slot and we do a suspend/resume > >>> cycle, we _do_ still end up with differences in a dw_mmc register > >>> dump, but the differences are clock related and we've got the clock > >>> disabled both before and after, so this should be OK. > >>> > >>> Signed-off-by: Doug Anderson <dianders@chromium.org> > >>> --- > >>> Changes in v6: > >>> - Replaces previous pathes that ensured saving/restoring clocks. > >>> > >>> drivers/mmc/host/dw_mmc.c | 21 +++++++++++---------- > >>> 1 file changed, 11 insertions(+), 10 deletions(-) > >>> > >>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > >>> index ee5f167..f6c7545 100644 > >>> --- a/drivers/mmc/host/dw_mmc.c > >>> +++ b/drivers/mmc/host/dw_mmc.c > >>> @@ -635,7 +635,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) > >>> u32 div; > >>> u32 clk_en_a; > >>> > >>> - if (slot->clock != host->current_speed || force_clkinit) { > >>> + if (slot->clock == 0) { > >>> + mci_writel(host, CLKENA, 0); > >>> + mci_send_cmd(slot, > >>> + SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); > >> Basically dw_mmc driver uses host's low power mode(auto clock gating) > >> So, how about keeping origin code rather than programming clock setting to '0'? > > Right, Dw-mmc controller can use the Low-power mode. > > This mode is functionality like clock-gating. Well, i didn't know what benefit we get, if set to 0. > > Ah, right. ...so it's unlikely that we'd save any power because we're > already gating the clock. > > I'd really still rather honor the MMC subsystem's request. It > shouldn't _hurt_ to turn the clock off when the subsystem requests it, Even though turning off by clock programming doesn't hurt, it is costly behavior when considering low power mode of host's own support. Just now, how about focusing on the problem clock isn't updated properly after suspend/resume? > right? One reason to honor the mmc core is that it will make things > cleaner if/when we support a voltage change operation. The MMC core > has the logic for the voltage change, and part of that involves > turning off the clock. We'll already need a bunch of special case > code in dw_mmc for voltage change, but it would be nice to avoid one > extra bit. Turning off clock during voltage switching would be another procedure. I guess it could be discussed later. I want to fix some minor change to prevent frequent message that Jaehoon pointed. Thanks, Seungwon Jeon > > Another option would be to add a core MMC quirk to disable MMC_CLKGATE > on a per-driver basis. In the "single zImage" world that seems like > the right thing to do, since the MMC_CLKGATE description seems to > indicate that enabling that won't really work on all drivers anyway. > > -Doug > -- > 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) 2013-08-29 7:04 ` Seungwon Jeon @ 2013-08-29 16:34 ` Doug Anderson 2013-08-30 3:55 ` Seungwon Jeon 0 siblings, 1 reply; 25+ messages in thread From: Doug Anderson @ 2013-08-29 16:34 UTC (permalink / raw) To: Seungwon Jeon Cc: Jaehoon Chung, Chris Ball, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, linux-mmc, linux-kernel Seungwon, On Thu, Aug 29, 2013 at 12:04 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: >> I'd really still rather honor the MMC subsystem's request. It >> shouldn't _hurt_ to turn the clock off when the subsystem requests it, > Even though turning off by clock programming doesn't hurt, > it is costly behavior when considering low power mode of host's own support. It is costly? We are talking about these two commands, right? mci_writel(host, CLKENA, 0); mci_send_cmd(slot, SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); Do you have a reason to believe that these are more costly than all of the rest of the code that's executed when the user defines CONFIG_MMC_CLKGATE? You're still proposing doing all of the updates of the clock when slot->clock is non-zero, right? ...so at best skipping this code will be 33% faster since the re-enable code disables and then reenables the clock. If it's the "SDMMC_CMD_PRV_DAT_WAIT" that you're worried about then skipping this code will only be 25% faster since there are already three calls with SDMMC_CMD_PRV_DAT_WAIT in the enable code. > Just now, how about focusing on the problem clock isn't updated properly after suspend/resume? I tried to do that in the original patches, but you pointed out (correctly) that we should do the correct fix rather than a hackier fix. IMHO the most correct fix is to honor the MMC core's request to turn the clock off. Partially honoring the MMC core (as you suggest) is certainly less hacky that my original proposal but I still think turning the clock off is better. >> right? One reason to honor the mmc core is that it will make things >> cleaner if/when we support a voltage change operation. The MMC core >> has the logic for the voltage change, and part of that involves >> turning off the clock. We'll already need a bunch of special case >> code in dw_mmc for voltage change, but it would be nice to avoid one >> extra bit. > Turning off clock during voltage switching would be another procedure. > I guess it could be discussed later. Agreed that we're not trying to get voltage switching done here, but forward thinking is nice. If there's no reason _not_ to turn the clock off and it will help us later, let's do it. Also, we've already agreed that MMC_CLKGATE isn't so useful for dw_mmc, so trying to do something awkward to make MMC_CLKGATE slightly faster doesn't seem worth it. > I want to fix some minor change to prevent frequent message that Jaehoon pointed. As far as I can tell, the frequent messages and whether or not to actually turn the clock off are unrelated. I will send up a patch that fixes the frequent messages by caching the last value printed and only printing if it changed. I have verified that this works and that the system still functions OK (can boot to prompt) with CONFIG_MMC_CLKGATE. Note: re-reading over some of the previous messages, it sounds like you're proposing using the patch from your email directly, AKA: http://article.gmane.org/gmane.linux.kernel/1542482 Did you test that patch? Did it work for you? It doesn't actually compile cleanly for me (you removed the "force_clkinit" param in the function but not the callers). That's easy to fix, but implies that this patch was just a proposal and not a tested solution. ...but aside from not compiling cleanly, I don't think it will work for the same reasons that the original code didn't work. Specifically it doesn't address the core problem that we need to update host->current_speed when the clock is 0. Otherwise we won't re-init and we run into the original problem, right? To be certain I took your patch and applied it, then fixed the callers of dw_mci_setup_bus() not to pass a second parameter. I did a suspend/resume with no card in and then plugged a card in. It didn't work. As I said above, new patch coming shortly. As always: feel free to point out any glaring mistakes I made in the above. ;) Note that I will be out of communication for the next week or so and buried beneath email for a few days after that, so my response might be a little delayed. -Doug ^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) 2013-08-29 16:34 ` Doug Anderson @ 2013-08-30 3:55 ` Seungwon Jeon 0 siblings, 0 replies; 25+ messages in thread From: Seungwon Jeon @ 2013-08-30 3:55 UTC (permalink / raw) To: 'Doug Anderson' Cc: 'Jaehoon Chung', 'Chris Ball', 'James Hogan', 'Grant Grundler', 'Alim Akhtar', 'Abhilash Kesavan', 'Tomasz Figa', 'Olof Johansson', linux-mmc, linux-kernel On Fri, August 30, 2013, Doug Anderson wrote: > Seungwon, > > On Thu, Aug 29, 2013 at 12:04 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > >> I'd really still rather honor the MMC subsystem's request. It > >> shouldn't _hurt_ to turn the clock off when the subsystem requests it, > > Even though turning off by clock programming doesn't hurt, > > it is costly behavior when considering low power mode of host's own support. > > It is costly? We are talking about these two commands, right? > > mci_writel(host, CLKENA, 0); > mci_send_cmd(slot, > SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); I mean that because host supports auto clock gating, we don't need to clock programming with setting '0'. If MMC_CLKGATE is enabled, clock programming will be executed with between '0' clock and working clock frequently. Actually the result is same. Of course, if host didn't support this feature, we would have considered that manually to save the power consumption. > > Do you have a reason to believe that these are more costly than all of > the rest of the code that's executed when the user defines > CONFIG_MMC_CLKGATE? You're still proposing doing all of the updates > of the clock when slot->clock is non-zero, right? ...so at best No, origin condition should be remained; Required clock should be different with current clock. > skipping this code will be 33% faster since the re-enable code > disables and then reenables the clock. If it's the > "SDMMC_CMD_PRV_DAT_WAIT" that you're worried about then skipping this > code will only be 25% faster since there are already three calls with > SDMMC_CMD_PRV_DAT_WAIT in the enable code. > > > > Just now, how about focusing on the problem clock isn't updated properly after suspend/resume? > > I tried to do that in the original patches, but you pointed out > (correctly) that we should do the correct fix rather than a hackier > fix. IMHO the most correct fix is to honor the MMC core's request to > turn the clock off. Partially honoring the MMC core (as you suggest) > is certainly less hacky that my original proposal but I still think > turning the clock off is better. > > > >> right? One reason to honor the mmc core is that it will make things > >> cleaner if/when we support a voltage change operation. The MMC core > >> has the logic for the voltage change, and part of that involves > >> turning off the clock. We'll already need a bunch of special case > >> code in dw_mmc for voltage change, but it would be nice to avoid one > >> extra bit. > > Turning off clock during voltage switching would be another procedure. > > I guess it could be discussed later. > > Agreed that we're not trying to get voltage switching done here, but > forward thinking is nice. If there's no reason _not_ to turn the > clock off and it will help us later, let's do it. Also, we've already > agreed that MMC_CLKGATE isn't so useful for dw_mmc, so trying to do > something awkward to make MMC_CLKGATE slightly faster doesn't seem > worth it. > > > > I want to fix some minor change to prevent frequent message that Jaehoon pointed. > > As far as I can tell, the frequent messages and whether or not to > actually turn the clock off are unrelated. I will send up a patch > that fixes the frequent messages by caching the last value printed and > only printing if it changed. I have verified that this works and that > the system still functions OK (can boot to prompt) with > CONFIG_MMC_CLKGATE. > > > Note: re-reading over some of the previous messages, it sounds like > you're proposing using the patch from your email directly, AKA: > > http://article.gmane.org/gmane.linux.kernel/1542482 > > Did you test that patch? Did it work for you? It doesn't actually > compile cleanly for me (you removed the "force_clkinit" param in the > function but not the callers). That's easy to fix, but implies that > this patch was just a proposal and not a tested solution. > > ...but aside from not compiling cleanly, I don't think it will work > for the same reasons that the original code didn't work. Specifically > it doesn't address the core problem that we need to update > host->current_speed when the clock is 0. Otherwise we won't re-init > and we run into the original problem, right? To be certain I took > your patch and applied it, then fixed the callers of > dw_mci_setup_bus() not to pass a second parameter. I did a > suspend/resume with no card in and then plugged a card in. It didn't > work. Some change proposed from me are mixed with both current existing part and your new patch. It's not whole code to replace your patch. But if it makes you confused, sorry about that. Important thing I intended is that if required clock(slot->clock) is '0', not try to update clock. with considering automatic clock gating. Please check first comment from me on v6. Thanks, Seungwon Jeon > > > As I said above, new patch coming shortly. As always: feel free to > point out any glaring mistakes I made in the above. ;) Note that I > will be out of communication for the next week or so and buried > beneath email for a few days after that, so my response might be a > little delayed. > > > > -Doug > -- > 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v6 3/3] mmc: dw_mmc: Set timeout to max upon resume 2013-08-22 16:19 ` Doug Anderson ` (2 preceding siblings ...) (?) @ 2013-08-22 16:19 ` Doug Anderson 2013-08-23 12:56 ` Jaehoon Chung -1 siblings, 1 reply; 25+ messages in thread From: Doug Anderson @ 2013-08-22 16:19 UTC (permalink / raw) To: Chris Ball Cc: Jaehoon Chung, Seungwon Jeon, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, Doug Anderson, linux-mmc, linux-kernel The TMOUT register is initted to 0xffffffff at probe time but isn't initted after suspend/resume. Add an init of this value. No problems were observed without this (it will also get initted in __dw_mci_start_request if there is data to send), but it makes the register dump before and after suspend cleaner. Signed-off-by: Doug Anderson <dianders@chromium.org> Acked-by: Seungwon Jeon <tgih.jun@samsung.com> Reviewed-by: Tomasz Figa <t.figa@samsung.com> --- Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None drivers/mmc/host/dw_mmc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index f6c7545..6a9a846 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -2506,6 +2506,9 @@ int dw_mci_resume(struct dw_mci *host) /* Restore the old value at FIFOTH register */ mci_writel(host, FIFOTH, host->fifoth_val); + /* Put in max timeout */ + mci_writel(host, TMOUT, 0xFFFFFFFF); + mci_writel(host, RINTSTS, 0xFFFFFFFF); mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER | SDMMC_INT_TXDR | SDMMC_INT_RXDR | -- 1.8.3 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v6 3/3] mmc: dw_mmc: Set timeout to max upon resume 2013-08-22 16:19 ` [PATCH v6 3/3] mmc: dw_mmc: Set timeout to max upon resume Doug Anderson @ 2013-08-23 12:56 ` Jaehoon Chung 0 siblings, 0 replies; 25+ messages in thread From: Jaehoon Chung @ 2013-08-23 12:56 UTC (permalink / raw) To: Doug Anderson Cc: Chris Ball, Seungwon Jeon, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, linux-mmc, linux-kernel Acked-by: Jaehoon Chung <jh80.chung@samsung.com> On 08/23/2013 01:19 AM, Doug Anderson wrote: > The TMOUT register is initted to 0xffffffff at probe time but isn't > initted after suspend/resume. Add an init of this value. > > No problems were observed without this (it will also get initted in > __dw_mci_start_request if there is data to send), but it makes the > register dump before and after suspend cleaner. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > Acked-by: Seungwon Jeon <tgih.jun@samsung.com> > Reviewed-by: Tomasz Figa <t.figa@samsung.com> > --- > Changes in v6: None > Changes in v5: None > Changes in v4: None > Changes in v3: None > Changes in v2: None > > drivers/mmc/host/dw_mmc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index f6c7545..6a9a846 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -2506,6 +2506,9 @@ int dw_mci_resume(struct dw_mci *host) > /* Restore the old value at FIFOTH register */ > mci_writel(host, FIFOTH, host->fifoth_val); > > + /* Put in max timeout */ > + mci_writel(host, TMOUT, 0xFFFFFFFF); > + > mci_writel(host, RINTSTS, 0xFFFFFFFF); > mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER | > SDMMC_INT_TXDR | SDMMC_INT_RXDR | > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v7 0/3] mmc: dw_mmc: fixes for suspend/resume on exynos 2013-08-22 16:19 ` Doug Anderson (?) @ 2013-08-29 16:39 ` Doug Anderson -1 siblings, 0 replies; 25+ messages in thread From: Doug Anderson @ 2013-08-29 16:39 UTC (permalink / raw) To: Chris Ball Cc: Jaehoon Chung, Seungwon Jeon, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, Doug Anderson, linux-samsung-soc, linux-kernel, linux-mmc, Kukjin Kim, linux-arm-kernel This series of patches addresses some suspend/resume problems with dw_mmc on exynos platforms, espeically exynos5420. This patchset was tested on the current ToT Chromeos 3.8 tree (which has lots of backports from 3.10/3.11) and on ToT Linux (v3.11-rc6). I have confirmed basic booting and that SD cards work across suspend/resume (both if they are plugged in and if they are not plugged in). I have received confirmation from Samsung that the problem solved for exynos5420 is a silicon errata and that this is a good fix. Changes in v7: - Avoid printing the same clock over and over again w/ MMC_CLKGATE. Changes in v6: - Took out TODO comment copied from main platform code. - Replaces previous pathes that ensured saving/restoring clocks. Changes in v5: - Cleaned up dw_mci_exynos_resume_noirq() comment as per Seungwon. - Don't memcpy dev_pm_ops structure, define a new one. Changes in v4: - Take Seungwon's suggestion and don't add any dw_mmc-pltfm code. Changes in v3: - Add freeze/thaw and poweroff/restore noirq entries. Changes in v2: - Use suspend_noirq as per James Hogan. Doug Anderson (3): mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) mmc: dw_mmc: Set timeout to max upon resume drivers/mmc/host/dw_mmc-exynos.c | 53 +++++++++++++++++++++++++++++++++++++++- drivers/mmc/host/dw_mmc.c | 39 ++++++++++++++++++----------- 2 files changed, 77 insertions(+), 15 deletions(-) -- 1.8.4 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v7 0/3] mmc: dw_mmc: fixes for suspend/resume on exynos @ 2013-08-29 16:39 ` Doug Anderson 0 siblings, 0 replies; 25+ messages in thread From: Doug Anderson @ 2013-08-29 16:39 UTC (permalink / raw) To: linux-arm-kernel This series of patches addresses some suspend/resume problems with dw_mmc on exynos platforms, espeically exynos5420. This patchset was tested on the current ToT Chromeos 3.8 tree (which has lots of backports from 3.10/3.11) and on ToT Linux (v3.11-rc6). I have confirmed basic booting and that SD cards work across suspend/resume (both if they are plugged in and if they are not plugged in). I have received confirmation from Samsung that the problem solved for exynos5420 is a silicon errata and that this is a good fix. Changes in v7: - Avoid printing the same clock over and over again w/ MMC_CLKGATE. Changes in v6: - Took out TODO comment copied from main platform code. - Replaces previous pathes that ensured saving/restoring clocks. Changes in v5: - Cleaned up dw_mci_exynos_resume_noirq() comment as per Seungwon. - Don't memcpy dev_pm_ops structure, define a new one. Changes in v4: - Take Seungwon's suggestion and don't add any dw_mmc-pltfm code. Changes in v3: - Add freeze/thaw and poweroff/restore noirq entries. Changes in v2: - Use suspend_noirq as per James Hogan. Doug Anderson (3): mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) mmc: dw_mmc: Set timeout to max upon resume drivers/mmc/host/dw_mmc-exynos.c | 53 +++++++++++++++++++++++++++++++++++++++- drivers/mmc/host/dw_mmc.c | 39 ++++++++++++++++++----------- 2 files changed, 77 insertions(+), 15 deletions(-) -- 1.8.4 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v7 0/3] mmc: dw_mmc: fixes for suspend/resume on exynos @ 2013-08-29 16:39 ` Doug Anderson 0 siblings, 0 replies; 25+ messages in thread From: Doug Anderson @ 2013-08-29 16:39 UTC (permalink / raw) To: Chris Ball Cc: James Hogan, Seungwon Jeon, linux-kernel, Olof Johansson, linux-mmc, Tomasz Figa, Doug Anderson, Jaehoon Chung, Grant Grundler, linux-samsung-soc, Alim Akhtar, Abhilash Kesavan, Kukjin Kim, linux-arm-kernel This series of patches addresses some suspend/resume problems with dw_mmc on exynos platforms, espeically exynos5420. This patchset was tested on the current ToT Chromeos 3.8 tree (which has lots of backports from 3.10/3.11) and on ToT Linux (v3.11-rc6). I have confirmed basic booting and that SD cards work across suspend/resume (both if they are plugged in and if they are not plugged in). I have received confirmation from Samsung that the problem solved for exynos5420 is a silicon errata and that this is a good fix. Changes in v7: - Avoid printing the same clock over and over again w/ MMC_CLKGATE. Changes in v6: - Took out TODO comment copied from main platform code. - Replaces previous pathes that ensured saving/restoring clocks. Changes in v5: - Cleaned up dw_mci_exynos_resume_noirq() comment as per Seungwon. - Don't memcpy dev_pm_ops structure, define a new one. Changes in v4: - Take Seungwon's suggestion and don't add any dw_mmc-pltfm code. Changes in v3: - Add freeze/thaw and poweroff/restore noirq entries. Changes in v2: - Use suspend_noirq as per James Hogan. Doug Anderson (3): mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) mmc: dw_mmc: Set timeout to max upon resume drivers/mmc/host/dw_mmc-exynos.c | 53 +++++++++++++++++++++++++++++++++++++++- drivers/mmc/host/dw_mmc.c | 39 ++++++++++++++++++----------- 2 files changed, 77 insertions(+), 15 deletions(-) -- 1.8.4 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v7 1/3] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT 2013-08-29 16:39 ` Doug Anderson @ 2013-08-29 16:39 ` Doug Anderson -1 siblings, 0 replies; 25+ messages in thread From: Doug Anderson @ 2013-08-29 16:39 UTC (permalink / raw) To: Chris Ball Cc: Jaehoon Chung, Seungwon Jeon, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, Doug Anderson, Kukjin Kim, linux-mmc, linux-arm-kernel, linux-samsung-soc, linux-kernel If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up looping around forever. This has been seen to happen on exynos5420 silicon despite the fact that we haven't enabled any wakeup events due to a silicon errata. It is safe to do on all exynos variants. Signed-off-by: Doug Anderson <dianders@chromium.org> --- Changes in v7: None Changes in v6: - Took out TODO comment copied from main platform code. Changes in v5: - Cleaned up dw_mci_exynos_resume_noirq() comment as per Seungwon. - Don't memcpy dev_pm_ops structure, define a new one. Changes in v4: - Take Seungwon's suggestion and don't add any dw_mmc-pltfm code. Changes in v3: - Add freeze/thaw and poweroff/restore noirq entries. Changes in v2: - Use suspend_noirq as per James Hogan. drivers/mmc/host/dw_mmc-exynos.c | 53 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c index 866edef..e8c6cc9 100644 --- a/drivers/mmc/host/dw_mmc-exynos.c +++ b/drivers/mmc/host/dw_mmc-exynos.c @@ -30,6 +30,7 @@ #define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \ SDMMC_CLKSEL_CCLK_DRIVE(y) | \ SDMMC_CLKSEL_CCLK_DIVIDER(z)) +#define SDMMC_CLKSEL_WAKEUP_INT BIT(11) #define EXYNOS4210_FIXED_CIU_CLK_DIV 2 #define EXYNOS4412_FIXED_CIU_CLK_DIV 4 @@ -100,6 +101,49 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host) return 0; } +#ifdef CONFIG_PM_SLEEP +static int dw_mci_exynos_suspend(struct device *dev) +{ + struct dw_mci *host = dev_get_drvdata(dev); + + return dw_mci_suspend(host); +} + +static int dw_mci_exynos_resume(struct device *dev) +{ + struct dw_mci *host = dev_get_drvdata(dev); + + return dw_mci_resume(host); +} + +/** + * dw_mci_exynos_resume_noirq - Exynos-specific resume code + * + * On exynos5420 there is a silicon errata that will sometimes leave the + * WAKEUP_INT bit in the CLKSEL register asserted. This bit is 1 to indicate + * that it fired and we can clear it by writing a 1 back. Clear it to prevent + * interrupts from going off constantly. + * + * We run this code on all exynos variants because it doesn't hurt. + */ + +static int dw_mci_exynos_resume_noirq(struct device *dev) +{ + struct dw_mci *host = dev_get_drvdata(dev); + u32 clksel; + + clksel = mci_readl(host, CLKSEL); + if (clksel & SDMMC_CLKSEL_WAKEUP_INT) + mci_writel(host, CLKSEL, clksel); + + return 0; +} +#else +#define dw_mci_exynos_suspend NULL +#define dw_mci_exynos_resume NULL +#define dw_mci_exynos_resume_noirq NULL +#endif /* CONFIG_PM_SLEEP */ + static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr) { /* @@ -187,13 +231,20 @@ static int dw_mci_exynos_probe(struct platform_device *pdev) return dw_mci_pltfm_register(pdev, drv_data); } +const struct dev_pm_ops dw_mci_exynos_pmops = { + SET_SYSTEM_SLEEP_PM_OPS(dw_mci_exynos_suspend, dw_mci_exynos_resume) + .resume_noirq = dw_mci_exynos_resume_noirq, + .thaw_noirq = dw_mci_exynos_resume_noirq, + .restore_noirq = dw_mci_exynos_resume_noirq, +}; + static struct platform_driver dw_mci_exynos_pltfm_driver = { .probe = dw_mci_exynos_probe, .remove = __exit_p(dw_mci_pltfm_remove), .driver = { .name = "dwmmc_exynos", .of_match_table = dw_mci_exynos_match, - .pm = &dw_mci_pltfm_pmops, + .pm = &dw_mci_exynos_pmops, }, }; -- 1.8.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v7 1/3] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT @ 2013-08-29 16:39 ` Doug Anderson 0 siblings, 0 replies; 25+ messages in thread From: Doug Anderson @ 2013-08-29 16:39 UTC (permalink / raw) To: linux-arm-kernel If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up looping around forever. This has been seen to happen on exynos5420 silicon despite the fact that we haven't enabled any wakeup events due to a silicon errata. It is safe to do on all exynos variants. Signed-off-by: Doug Anderson <dianders@chromium.org> --- Changes in v7: None Changes in v6: - Took out TODO comment copied from main platform code. Changes in v5: - Cleaned up dw_mci_exynos_resume_noirq() comment as per Seungwon. - Don't memcpy dev_pm_ops structure, define a new one. Changes in v4: - Take Seungwon's suggestion and don't add any dw_mmc-pltfm code. Changes in v3: - Add freeze/thaw and poweroff/restore noirq entries. Changes in v2: - Use suspend_noirq as per James Hogan. drivers/mmc/host/dw_mmc-exynos.c | 53 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c index 866edef..e8c6cc9 100644 --- a/drivers/mmc/host/dw_mmc-exynos.c +++ b/drivers/mmc/host/dw_mmc-exynos.c @@ -30,6 +30,7 @@ #define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \ SDMMC_CLKSEL_CCLK_DRIVE(y) | \ SDMMC_CLKSEL_CCLK_DIVIDER(z)) +#define SDMMC_CLKSEL_WAKEUP_INT BIT(11) #define EXYNOS4210_FIXED_CIU_CLK_DIV 2 #define EXYNOS4412_FIXED_CIU_CLK_DIV 4 @@ -100,6 +101,49 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host) return 0; } +#ifdef CONFIG_PM_SLEEP +static int dw_mci_exynos_suspend(struct device *dev) +{ + struct dw_mci *host = dev_get_drvdata(dev); + + return dw_mci_suspend(host); +} + +static int dw_mci_exynos_resume(struct device *dev) +{ + struct dw_mci *host = dev_get_drvdata(dev); + + return dw_mci_resume(host); +} + +/** + * dw_mci_exynos_resume_noirq - Exynos-specific resume code + * + * On exynos5420 there is a silicon errata that will sometimes leave the + * WAKEUP_INT bit in the CLKSEL register asserted. This bit is 1 to indicate + * that it fired and we can clear it by writing a 1 back. Clear it to prevent + * interrupts from going off constantly. + * + * We run this code on all exynos variants because it doesn't hurt. + */ + +static int dw_mci_exynos_resume_noirq(struct device *dev) +{ + struct dw_mci *host = dev_get_drvdata(dev); + u32 clksel; + + clksel = mci_readl(host, CLKSEL); + if (clksel & SDMMC_CLKSEL_WAKEUP_INT) + mci_writel(host, CLKSEL, clksel); + + return 0; +} +#else +#define dw_mci_exynos_suspend NULL +#define dw_mci_exynos_resume NULL +#define dw_mci_exynos_resume_noirq NULL +#endif /* CONFIG_PM_SLEEP */ + static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr) { /* @@ -187,13 +231,20 @@ static int dw_mci_exynos_probe(struct platform_device *pdev) return dw_mci_pltfm_register(pdev, drv_data); } +const struct dev_pm_ops dw_mci_exynos_pmops = { + SET_SYSTEM_SLEEP_PM_OPS(dw_mci_exynos_suspend, dw_mci_exynos_resume) + .resume_noirq = dw_mci_exynos_resume_noirq, + .thaw_noirq = dw_mci_exynos_resume_noirq, + .restore_noirq = dw_mci_exynos_resume_noirq, +}; + static struct platform_driver dw_mci_exynos_pltfm_driver = { .probe = dw_mci_exynos_probe, .remove = __exit_p(dw_mci_pltfm_remove), .driver = { .name = "dwmmc_exynos", .of_match_table = dw_mci_exynos_match, - .pm = &dw_mci_pltfm_pmops, + .pm = &dw_mci_exynos_pmops, }, }; -- 1.8.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v7 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) 2013-08-29 16:39 ` Doug Anderson ` (2 preceding siblings ...) (?) @ 2013-08-29 16:39 ` Doug Anderson 2013-08-30 11:31 ` Seungwon Jeon -1 siblings, 1 reply; 25+ messages in thread From: Doug Anderson @ 2013-08-29 16:39 UTC (permalink / raw) To: Chris Ball Cc: Jaehoon Chung, Seungwon Jeon, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, Doug Anderson, linux-mmc, linux-kernel Previously the dw_mmc driver would ignore any requests to disable the card's clock. This doesn't seem like a good thing in general, but had one extra bad side effect in the following situtation: * mmc core would set clk to 400kHz at boot time while initting * mmc core would set clk to 0 since no card, but it would be ignored. * suspend to ram and resume; clocks in the dw_mmc IP block are now 0 but dw_mmc thinks that they're 400kHz (it ignored the set to 0). * insert card * mmc core would set clk to 400kHz which would be considered a no-op. Note that if there is no card in the slot and we do a suspend/resume cycle, we _do_ still end up with differences in a dw_mmc register dump, but the differences are clock related and we've got the clock disabled both before and after, so this should be OK. Signed-off-by: Doug Anderson <dianders@chromium.org> --- Changes in v7: - Avoid printing the same clock over and over again w/ MMC_CLKGATE. Changes in v6: - Replaces previous pathes that ensured saving/restoring clocks. drivers/mmc/host/dw_mmc.c | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index ee5f167..1584705 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -88,6 +88,8 @@ struct idmac_desc { * @queue_node: List node for placing this node in the @queue list of * &struct dw_mci. * @clock: Clock rate configured by set_ios(). Protected by host->lock. + * @last_printed_clock: The last clock we printed. Keeping track of this helps + * us to avoid spamming the console with CONFIG_MMC_CLKGATE. * @flags: Random state bits associated with the slot. * @id: Number of this slot. * @last_detect_state: Most recently observed card detect state. @@ -105,6 +107,7 @@ struct dw_mci_slot { struct list_head queue_node; unsigned int clock; + unsigned int last_printed_clock; unsigned long flags; #define DW_MMC_CARD_PRESENT 0 #define DW_MMC_CARD_NEED_INIT 1 @@ -635,7 +638,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) u32 div; u32 clk_en_a; - if (slot->clock != host->current_speed || force_clkinit) { + if (slot->clock == 0) { + mci_writel(host, CLKENA, 0); + mci_send_cmd(slot, + SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); + } else if (slot->clock != host->current_speed || force_clkinit) { div = host->bus_hz / slot->clock; if (host->bus_hz % slot->clock && host->bus_hz > slot->clock) /* @@ -646,10 +653,14 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0; - dev_info(&slot->mmc->class_dev, - "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ" - " div = %d)\n", slot->id, host->bus_hz, slot->clock, - div ? ((host->bus_hz / div) >> 1) : host->bus_hz, div); + if (slot->clock != slot->last_printed_clock || force_clkinit) { + dev_info(&slot->mmc->class_dev, + "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n", + slot->id, host->bus_hz, slot->clock, + div ? ((host->bus_hz / div) >> 1) : + host->bus_hz, div); + slot->last_printed_clock = slot->clock; + } /* disable clock */ mci_writel(host, CLKENA, 0); @@ -675,9 +686,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) /* inform CIU */ mci_send_cmd(slot, SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); - - host->current_speed = slot->clock; } + host->current_speed = slot->clock; /* Set the current slot bus width */ mci_writel(host, CTYPE, (slot->ctype << slot->id)); @@ -807,13 +817,11 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) mci_writel(slot->host, UHS_REG, regs); - if (ios->clock) { - /* - * Use mirror of ios->clock to prevent race with mmc - * core ios update when finding the minimum. - */ - slot->clock = ios->clock; - } + /* + * Use mirror of ios->clock to prevent race with mmc + * core ios update when finding the minimum. + */ + slot->clock = ios->clock; if (drv_data && drv_data->set_ios) drv_data->set_ios(slot->host, ios); -- 1.8.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* RE: [PATCH v7 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) 2013-08-29 16:39 ` [PATCH v7 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) Doug Anderson @ 2013-08-30 11:31 ` Seungwon Jeon 0 siblings, 0 replies; 25+ messages in thread From: Seungwon Jeon @ 2013-08-30 11:31 UTC (permalink / raw) To: 'Doug Anderson', 'Chris Ball' Cc: 'Jaehoon Chung', 'James Hogan', 'Grant Grundler', 'Alim Akhtar', 'Abhilash Kesavan', 'Tomasz Figa', 'Olof Johansson', linux-mmc, linux-kernel Hi Doug, On Friday, August 30, 2013, Doug Anderson wrote: > Previously the dw_mmc driver would ignore any requests to disable the > card's clock. This doesn't seem like a good thing in general, but had > one extra bad side effect in the following situtation: > * mmc core would set clk to 400kHz at boot time while initting > * mmc core would set clk to 0 since no card, but it would be ignored. > * suspend to ram and resume; clocks in the dw_mmc IP block are now 0 > but dw_mmc thinks that they're 400kHz (it ignored the set to 0). > * insert card > * mmc core would set clk to 400kHz which would be considered a no-op. > > Note that if there is no card in the slot and we do a suspend/resume > cycle, we _do_ still end up with differences in a dw_mmc register > dump, but the differences are clock related and we've got the clock > disabled both before and after, so this should be OK. I looked into this change with various kinds. When we consider the SDIO, clock off may need to be cared. As we know, because low power mode is off in case of SDIO, there is no way to disable clock though '0' is requested for clock rate. Ok, it would be better if some minor things below are applied. I guess I can update those. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > --- > Changes in v7: > - Avoid printing the same clock over and over again w/ MMC_CLKGATE. > > Changes in v6: > - Replaces previous pathes that ensured saving/restoring clocks. > > drivers/mmc/host/dw_mmc.c | 36 ++++++++++++++++++++++-------------- > 1 file changed, 22 insertions(+), 14 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index ee5f167..1584705 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -88,6 +88,8 @@ struct idmac_desc { > * @queue_node: List node for placing this node in the @queue list of > * &struct dw_mci. > * @clock: Clock rate configured by set_ios(). Protected by host->lock. > + * @last_printed_clock: The last clock we printed. Keeping track of this helps 'last_printed_clock' can be changed to a nice name. > + * us to avoid spamming the console with CONFIG_MMC_CLKGATE. > * @flags: Random state bits associated with the slot. > * @id: Number of this slot. > * @last_detect_state: Most recently observed card detect state. > @@ -105,6 +107,7 @@ struct dw_mci_slot { > struct list_head queue_node; > > unsigned int clock; > + unsigned int last_printed_clock; > unsigned long flags; > #define DW_MMC_CARD_PRESENT 0 > #define DW_MMC_CARD_NEED_INIT 1 > @@ -635,7 +638,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) > u32 div; > u32 clk_en_a; > > - if (slot->clock != host->current_speed || force_clkinit) { > + if (slot->clock == 0) { > + mci_writel(host, CLKENA, 0); > + mci_send_cmd(slot, > + SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); In the dw_mci_setup_bus(), above two lines related to clock-disable are necessary both if and else-if statement That is it can be executed unconditionally. > + } else if (slot->clock != host->current_speed || force_clkinit) { > div = host->bus_hz / slot->clock; > if (host->bus_hz % slot->clock && host->bus_hz > slot->clock) > /* > @@ -646,10 +653,14 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) > > div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0; > > - dev_info(&slot->mmc->class_dev, > - "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ" > - " div = %d)\n", slot->id, host->bus_hz, slot->clock, > - div ? ((host->bus_hz / div) >> 1) : host->bus_hz, div); > + if (slot->clock != slot->last_printed_clock || force_clkinit) { If 'bus_hz' or 'div' is different with old one as well, it should be considered. 'div' can be a good factor to compare this condition. Then 'last_printed_clock' should reflect clock divider. Thanks, Seungwon Jeon > + dev_info(&slot->mmc->class_dev, > + "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n", > + slot->id, host->bus_hz, slot->clock, > + div ? ((host->bus_hz / div) >> 1) : > + host->bus_hz, div); > + slot->last_printed_clock = slot->clock; > + } > > /* disable clock */ > mci_writel(host, CLKENA, 0); > @@ -675,9 +686,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) > /* inform CIU */ > mci_send_cmd(slot, > SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0); > - > - host->current_speed = slot->clock; > } > + host->current_speed = slot->clock; > > /* Set the current slot bus width */ > mci_writel(host, CTYPE, (slot->ctype << slot->id)); > @@ -807,13 +817,11 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > > mci_writel(slot->host, UHS_REG, regs); > > - if (ios->clock) { > - /* > - * Use mirror of ios->clock to prevent race with mmc > - * core ios update when finding the minimum. > - */ > - slot->clock = ios->clock; > - } > + /* > + * Use mirror of ios->clock to prevent race with mmc > + * core ios update when finding the minimum. > + */ > + slot->clock = ios->clock; > > if (drv_data && drv_data->set_ios) > drv_data->set_ios(slot->host, ios); > -- > 1.8.4 > > -- > 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v7 3/3] mmc: dw_mmc: Set timeout to max upon resume 2013-08-29 16:39 ` Doug Anderson ` (3 preceding siblings ...) (?) @ 2013-08-29 16:39 ` Doug Anderson -1 siblings, 0 replies; 25+ messages in thread From: Doug Anderson @ 2013-08-29 16:39 UTC (permalink / raw) To: Chris Ball Cc: Jaehoon Chung, Seungwon Jeon, James Hogan, Grant Grundler, Alim Akhtar, Abhilash Kesavan, Tomasz Figa, Olof Johansson, Doug Anderson, linux-mmc, linux-kernel The TMOUT register is initted to 0xffffffff at probe time but isn't initted after suspend/resume. Add an init of this value. No problems were observed without this (it will also get initted in __dw_mci_start_request if there is data to send), but it makes the register dump before and after suspend cleaner. Signed-off-by: Doug Anderson <dianders@chromium.org> Acked-by: Seungwon Jeon <tgih.jun@samsung.com> Reviewed-by: Tomasz Figa <t.figa@samsung.com> --- Changes in v7: None Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None drivers/mmc/host/dw_mmc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 1584705..f8d891a 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -2513,6 +2513,9 @@ int dw_mci_resume(struct dw_mci *host) /* Restore the old value at FIFOTH register */ mci_writel(host, FIFOTH, host->fifoth_val); + /* Put in max timeout */ + mci_writel(host, TMOUT, 0xFFFFFFFF); + mci_writel(host, RINTSTS, 0xFFFFFFFF); mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER | SDMMC_INT_TXDR | SDMMC_INT_RXDR | -- 1.8.4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
end of thread, other threads:[~2013-08-30 11:31 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-08-22 16:19 [PATCH v6 0/3] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson 2013-08-22 16:19 ` Doug Anderson 2013-08-22 16:19 ` [PATCH v6 1/3] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson 2013-08-22 16:19 ` Doug Anderson 2013-08-22 16:19 ` [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) Doug Anderson 2013-08-23 13:21 ` Jaehoon Chung 2013-08-23 20:40 ` Doug Anderson 2013-08-26 1:31 ` Jaehoon Chung 2013-08-26 3:38 ` Doug Anderson 2013-08-26 4:34 ` Seungwon Jeon 2013-08-26 9:06 ` Jaehoon Chung 2013-08-26 16:05 ` Doug Anderson 2013-08-29 7:04 ` Seungwon Jeon 2013-08-29 16:34 ` Doug Anderson 2013-08-30 3:55 ` Seungwon Jeon 2013-08-22 16:19 ` [PATCH v6 3/3] mmc: dw_mmc: Set timeout to max upon resume Doug Anderson 2013-08-23 12:56 ` Jaehoon Chung 2013-08-29 16:39 ` [PATCH v7 0/3] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson 2013-08-29 16:39 ` Doug Anderson 2013-08-29 16:39 ` Doug Anderson 2013-08-29 16:39 ` [PATCH v7 1/3] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson 2013-08-29 16:39 ` Doug Anderson 2013-08-29 16:39 ` [PATCH v7 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock) Doug Anderson 2013-08-30 11:31 ` Seungwon Jeon 2013-08-29 16:39 ` [PATCH v7 3/3] mmc: dw_mmc: Set timeout to max upon resume Doug Anderson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.