Re: [PATCH v4 1/4] mmc: dw_mmc: Invalidate cache of current_speed after suspend/resume
Seungwon,
On Wed, Aug 21, 2013 at 5:54 PM, Doug Anderson diand...@chromium.org wrote:
Doug, your analysis is right.
But, let me suggest another approach.
After step #1, core layer actually call mmc_power_off because slot is
empthy(get_cd() is '0').
Then, set_ios is requested with 'ios-clock'.
However, because current implementation doesn't update current_speed in case
ios-clock is '0'.
It causes current_speed has invalid clock rate in resume of dw-mmc.
So, if we can update slot-clock properly, it will be fixed.
-static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
+static void dw_mci_setup_bus(struct dw_mci_slot *slot)
{
struct dw_mci *host = slot-host;
u32 div;
u32 clk_en_a;
- if (slot-clock != host-current_speed || force_clkinit) {
+ if (slot-clock (slot-clock != host-current_speed)) {
@@ -807,13 +807,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;
So this scares me a little bit but you're correct that it's probably
the right thing. Mostly it scares me to remove code that someone
clearly added on purpose without understanding why they originally
added it and why that reason is not valid (or is no longer valid due
to other changes).
OK, I posted up a new patch series. Hopefully this looks good to you.
I did a suspend/resume stress test last night on our chromeos-3.8
branch and things worked well.
I've come up with a new patch that's a little bit more than just your
patch. It actually does something in the case of a zero clock (it
turns the clock off). My patch seems to work OK on my 3.8 branch but
I want to do a little more testing tomorrow. ...but booting on ToT
linux seems broken now on the ARM chromebook (it fails in several
different ways and sometimes works). Sigh.
Somehow this morning ToT linux (same revision) was working much better
on exynos5250-snow. I was able to boot reliably and even
suspend/resume reliably. I think there may be a few niggling issues
that we'll have to track down before too long, though...
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/4] mmc: dw_mmc: Invalidate cache of current_speed after suspend/resume
Seungwon,
On Mon, Aug 12, 2013 at 12:14 AM, Seungwon Jeon tgih@samsung.com wrote:
On Sat, August 10, 2013, Doug Anderson wrote:
Seungwon and Jaehoon,
On Fri, Aug 9, 2013 at 6:32 AM, Seungwon Jeon tgih@samsung.com wrote:
On Wed, August 07, 2013, Doug Anderson wrote:
The dw_mmc driver keeps a cache of the current slot-clock in order to
avoid doing a whole lot of work every time set_ios() is called.
However, after suspend/resume the register values are bogus so we need
to ensure that the cached value is invalidated.
This mismatch comes only in case MMC_PM_KEEP_POWER, right?
Actually, no. I saw problems with the SD Card slot, which doesn't
have MMC_KEEP_POWER. Problems showed up when no card was inserted
across suspend/resume. In other words:
1. At boot time, slot is all setup and configured to 400kHz.
2. Suspend
3. Resume; clock registers are reset (by suspend/resume) and not
restored since dw_mmc still thinks slot is configured for 400kHz due
to host-current_speed cache.
4. Insert card.
5. No code sees any need to change the clock for detecting the card,
since everyone thinks it's at 400kHz. ...but it's not.
Doug, your analysis is right.
But, let me suggest another approach.
After step #1, core layer actually call mmc_power_off because slot is
empthy(get_cd() is '0').
Then, set_ios is requested with 'ios-clock'.
However, because current implementation doesn't update current_speed in case
ios-clock is '0'.
It causes current_speed has invalid clock rate in resume of dw-mmc.
So, if we can update slot-clock properly, it will be fixed.
-static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
+static void dw_mci_setup_bus(struct dw_mci_slot *slot)
{
struct dw_mci *host = slot-host;
u32 div;
u32 clk_en_a;
- if (slot-clock != host-current_speed || force_clkinit) {
+ if (slot-clock (slot-clock != host-current_speed)) {
@@ -807,13 +807,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;
So this scares me a little bit but you're correct that it's probably
the right thing. Mostly it scares me to remove code that someone
clearly added on purpose without understanding why they originally
added it and why that reason is not valid (or is no longer valid due
to other changes).
I've actually got a change similar to this as part of my WIP SDIO 3.0
series that I haven't had time to finish (I've been saying that a lot
recently...) at
https://gerrit.chromium.org/gerrit/#/c/61942/1/drivers/mmc/host/dw_mmc.c.
I see a bunch of patches that seem related to SDIO 3.0 from you that
were just posted. I'll have to look them over if I can find the
time...
I've come up with a new patch that's a little bit more than just your
patch. It actually does something in the case of a zero clock (it
turns the clock off). My patch seems to work OK on my 3.8 branch but
I want to do a little more testing tomorrow. ...but booting on ToT
linux seems broken now on the ARM chromebook (it fails in several
different ways and sometimes works). Sigh.
In any case, I'll post up tomorrow. It won't have as much testing as
my old series (which I'm using every day since it's landed in our
tree), but I'll do some basic testing and we'll deal with any issues
that come up.
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 1/4] mmc: dw_mmc: Invalidate cache of current_speed after suspend/resume
On Sat, August 10, 2013, Doug Anderson wrote:
Seungwon and Jaehoon,
On Fri, Aug 9, 2013 at 6:32 AM, Seungwon Jeon tgih@samsung.com wrote:
On Wed, August 07, 2013, Doug Anderson wrote:
The dw_mmc driver keeps a cache of the current slot-clock in order to
avoid doing a whole lot of work every time set_ios() is called.
However, after suspend/resume the register values are bogus so we need
to ensure that the cached value is invalidated.
This mismatch comes only in case MMC_PM_KEEP_POWER, right?
Actually, no. I saw problems with the SD Card slot, which doesn't
have MMC_KEEP_POWER. Problems showed up when no card was inserted
across suspend/resume. In other words:
1. At boot time, slot is all setup and configured to 400kHz.
2. Suspend
3. Resume; clock registers are reset (by suspend/resume) and not
restored since dw_mmc still thinks slot is configured for 400kHz due
to host-current_speed cache.
4. Insert card.
5. No code sees any need to change the clock for detecting the card,
since everyone thinks it's at 400kHz. ...but it's not.
Doug, your analysis is right.
But, let me suggest another approach.
After step #1, core layer actually call mmc_power_off because slot is
empthy(get_cd() is '0').
Then, set_ios is requested with 'ios-clock'.
However, because current implementation doesn't update current_speed in case
ios-clock is '0'.
It causes current_speed has invalid clock rate in resume of dw-mmc.
So, if we can update slot-clock properly, it will be fixed.
-static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
+static void dw_mci_setup_bus(struct dw_mci_slot *slot)
{
struct dw_mci *host = slot-host;
u32 div;
u32 clk_en_a;
- if (slot-clock != host-current_speed || force_clkinit) {
+ if (slot-clock (slot-clock != host-current_speed)) {
@@ -807,13 +807,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;
Thanks,
Seungwon Jeon
In many cases we got by without this since the core mmc code fiddles
with the clock a lot. If we've got a card present we're probably
running it at something like 50MHz and the core will temporarily
switch us to 400kHz after resume. One case that didn't work (for me)
is the case of having no card in the slot. The slot is initted to
400kHz at boot time. After suspend/resume the slot thinks it's still
at 400kHz (due to the cache) so doesn't adjust timing. When it tries
to send the command at probe time it just times out and gets left in a
bad state.
I understand this change although some part of commit message (boot time,
probe time...) make me
confused.
Sorry to be confusing. I was trying to explain why the old code works
fine in many cases. It's because the core MMC code tends to adjust
the clock a lot around suspend/resume. When it does that, it works
around this problem. ...but I found one case where suspend/resume
would happen and the MMC core didn't adjust the clock.
I guess this change intends to update clock programming forcedly.
It looks like another version of 'dw_mci_setup_bus(slot, true)'.
Eventually, this change does same?
Effectively, yes. As Jaehoon points out, that means we can actually
eliminate the force parameter to dw_mci_setup_bus().
I will send a new version out that eliminates the force parameter
and updates the commit message to (hopefully) be clearer.
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 1/4] mmc: dw_mmc: Invalidate cache of current_speed after suspend/resume
On Wed, August 07, 2013, Doug Anderson wrote:
The dw_mmc driver keeps a cache of the current slot-clock in order to
avoid doing a whole lot of work every time set_ios() is called.
However, after suspend/resume the register values are bogus so we need
to ensure that the cached value is invalidated.
This mismatch comes only in case MMC_PM_KEEP_POWER, right?
In many cases we got by without this since the core mmc code fiddles
with the clock a lot. If we've got a card present we're probably
running it at something like 50MHz and the core will temporarily
switch us to 400kHz after resume. One case that didn't work (for me)
is the case of having no card in the slot. The slot is initted to
400kHz at boot time. After suspend/resume the slot thinks it's still
at 400kHz (due to the cache) so doesn't adjust timing. When it tries
to send the command at probe time it just times out and gets left in a
bad state.
I understand this change although some part of commit message (boot time, probe
time...) make me confused.
I guess this change intends to update clock programming forcedly.
It looks like another version of 'dw_mci_setup_bus(slot, true)'.
Eventually, this change does same?
Thanks,
Seungwon Jeon
Invalidating the current_speed also means that we don't need to call:
dw_mci_setup_bus(slot, true);
...to force an update of the clock in the case when the slot was left
powered.
Signed-off-by: Doug Anderson diand...@chromium.org
---
Changes in v4: None
Changes in v3: None
Changes in v2:
- Fix typo (some - come)
- Use ~0 instead of 0x; add comment about value
drivers/mmc/host/dw_mmc.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index ee5f167..13a363c 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2511,13 +2511,19 @@ int dw_mci_resume(struct dw_mci *host)
DW_MCI_ERROR_FLAGS | SDMMC_INT_CD);
mci_writel(host, CTRL, SDMMC_CTRL_INT_ENABLE);
+ /*
+ * Invalidate the 'current_speed' value since CLKDIV has come up in
+ * default state and our cache is incorrect; set to something we know
+ * slot-clock won't be.
+ */
+ host-current_speed = ~0;
+
for (i = 0; i host-num_slots; i++) {
struct dw_mci_slot *slot = host-slot[i];
if (!slot)
continue;
if (slot-mmc-pm_flags MMC_PM_KEEP_POWER) {
dw_mci_set_ios(slot-mmc, slot-mmc-ios);
- dw_mci_setup_bus(slot, true);
}
ret = mmc_resume_host(host-slot[i]-mmc);
--
1.8.3
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/4] mmc: dw_mmc: Invalidate cache of current_speed after suspend/resume
Seungwon and Jaehoon, On Fri, Aug 9, 2013 at 6:32 AM, Seungwon Jeon tgih@samsung.com wrote: On Wed, August 07, 2013, Doug Anderson wrote: The dw_mmc driver keeps a cache of the current slot-clock in order to avoid doing a whole lot of work every time set_ios() is called. However, after suspend/resume the register values are bogus so we need to ensure that the cached value is invalidated. This mismatch comes only in case MMC_PM_KEEP_POWER, right? Actually, no. I saw problems with the SD Card slot, which doesn't have MMC_KEEP_POWER. Problems showed up when no card was inserted across suspend/resume. In other words: 1. At boot time, slot is all setup and configured to 400kHz. 2. Suspend 3. Resume; clock registers are reset (by suspend/resume) and not restored since dw_mmc still thinks slot is configured for 400kHz due to host-current_speed cache. 4. Insert card. 5. No code sees any need to change the clock for detecting the card, since everyone thinks it's at 400kHz. ...but it's not. In many cases we got by without this since the core mmc code fiddles with the clock a lot. If we've got a card present we're probably running it at something like 50MHz and the core will temporarily switch us to 400kHz after resume. One case that didn't work (for me) is the case of having no card in the slot. The slot is initted to 400kHz at boot time. After suspend/resume the slot thinks it's still at 400kHz (due to the cache) so doesn't adjust timing. When it tries to send the command at probe time it just times out and gets left in a bad state. I understand this change although some part of commit message (boot time, probe time...) make me confused. Sorry to be confusing. I was trying to explain why the old code works fine in many cases. It's because the core MMC code tends to adjust the clock a lot around suspend/resume. When it does that, it works around this problem. ...but I found one case where suspend/resume would happen and the MMC core didn't adjust the clock. I guess this change intends to update clock programming forcedly. It looks like another version of 'dw_mci_setup_bus(slot, true)'. Eventually, this change does same? Effectively, yes. As Jaehoon points out, that means we can actually eliminate the force parameter to dw_mci_setup_bus(). I will send a new version out that eliminates the force parameter and updates the commit message to (hopefully) be clearer. -Doug -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/4] mmc: dw_mmc: Invalidate cache of current_speed after suspend/resume
Hi Doung
On 08/07/2013 06:37 AM, Doug Anderson wrote:
The dw_mmc driver keeps a cache of the current slot-clock in order to
avoid doing a whole lot of work every time set_ios() is called.
However, after suspend/resume the register values are bogus so we need
to ensure that the cached value is invalidated.
In many cases we got by without this since the core mmc code fiddles
with the clock a lot. If we've got a card present we're probably
running it at something like 50MHz and the core will temporarily
switch us to 400kHz after resume. One case that didn't work (for me)
is the case of having no card in the slot. The slot is initted to
400kHz at boot time. After suspend/resume the slot thinks it's still
at 400kHz (due to the cache) so doesn't adjust timing. When it tries
to send the command at probe time it just times out and gets left in a
bad state.
Invalidating the current_speed also means that we don't need to call:
dw_mci_setup_bus(slot, true);
...to force an update of the clock in the case when the slot was left
powered.
Signed-off-by: Doug Anderson diand...@chromium.org
---
Changes in v4: None
Changes in v3: None
Changes in v2:
- Fix typo (some - come)
- Use ~0 instead of 0x; add comment about value
drivers/mmc/host/dw_mmc.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index ee5f167..13a363c 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2511,13 +2511,19 @@ int dw_mci_resume(struct dw_mci *host)
DW_MCI_ERROR_FLAGS | SDMMC_INT_CD);
mci_writel(host, CTRL, SDMMC_CTRL_INT_ENABLE);
+ /*
+ * Invalidate the 'current_speed' value since CLKDIV has come up in
+ * default state and our cache is incorrect; set to something we know
+ * slot-clock won't be.
+ */
+ host-current_speed = ~0;
+
for (i = 0; i host-num_slots; i++) {
struct dw_mci_slot *slot = host-slot[i];
if (!slot)
continue;
if (slot-mmc-pm_flags MMC_PM_KEEP_POWER) {
dw_mci_set_ios(slot-mmc, slot-mmc-ios);
- dw_mci_setup_bus(slot, true);
If we need not to call dw_mci_setup_bus() at here,
the we can also remove the force_clkinit.
Best Regards,
Jaehoon Chung
}
ret = mmc_resume_host(host-slot[i]-mmc);
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/4] mmc: dw_mmc: Invalidate cache of current_speed after suspend/resume
On Tuesday 06 of August 2013 14:37:48 Doug Anderson wrote:
The dw_mmc driver keeps a cache of the current slot-clock in order to
avoid doing a whole lot of work every time set_ios() is called.
However, after suspend/resume the register values are bogus so we need
to ensure that the cached value is invalidated.
In many cases we got by without this since the core mmc code fiddles
with the clock a lot. If we've got a card present we're probably
running it at something like 50MHz and the core will temporarily
switch us to 400kHz after resume. One case that didn't work (for me)
is the case of having no card in the slot. The slot is initted to
400kHz at boot time. After suspend/resume the slot thinks it's still
at 400kHz (due to the cache) so doesn't adjust timing. When it tries
to send the command at probe time it just times out and gets left in a
bad state.
Invalidating the current_speed also means that we don't need to call:
dw_mci_setup_bus(slot, true);
...to force an update of the clock in the case when the slot was left
powered.
Signed-off-by: Doug Anderson diand...@chromium.org
---
Changes in v4: None
Changes in v3: None
Changes in v2:
- Fix typo (some - come)
- Use ~0 instead of 0x; add comment about value
drivers/mmc/host/dw_mmc.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index ee5f167..13a363c 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2511,13 +2511,19 @@ int dw_mci_resume(struct dw_mci *host)
DW_MCI_ERROR_FLAGS | SDMMC_INT_CD);
mci_writel(host, CTRL, SDMMC_CTRL_INT_ENABLE);
+ /*
+ * Invalidate the 'current_speed' value since CLKDIV has come up
in
+ * default state and our cache is incorrect; set to something we
know
+ * slot-clock won't be.
+ */
+ host-current_speed = ~0;
+
for (i = 0; i host-num_slots; i++) {
struct dw_mci_slot *slot = host-slot[i];
if (!slot)
continue;
if (slot-mmc-pm_flags MMC_PM_KEEP_POWER) {
dw_mci_set_ios(slot-mmc, slot-mmc-ios);
- dw_mci_setup_bus(slot, true);
}
ret = mmc_resume_host(host-slot[i]-mmc);
Reviewed-by: Tomasz Figa t.f...@samsung.com
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
