| Message ID | 1377188348-3418-3-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > -- 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
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 -- 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
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/ > -- 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
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 -- 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
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 -- 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
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 > -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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);
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(-)