This series of patches addresses some suspend/resume problems with
dw_mmc on exynos platforms. Since suspend/resume is not fully working
on ToT Linux (3.10) on exynos5250-snow, this series was tested against
the current ToT ChromeOS 3.8 tree. I have confirmed basic booting
and eMMC / SD card usage (and compiling, honest!) against ToT Linux.
Doug Anderson (5):
mmc: dw_mmc: Invalidate cache of current_speed after suspend/resume
mmc: dw_mmc: Add suspend/resume callbacks; disable irq during suspend
mmc: dw_mmc: Add exynos resume callback to clear WAKEUP_INT
mmc: dw_mmc: Always setup the bus after suspend/resume
mmc: dw_mmc: Set timeout to max upon resume
drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++
drivers/mmc/host/dw_mmc.c | 26 ++++++++++++++++++++++----
drivers/mmc/host/dw_mmc.h | 4 ++++
3 files changed, 49 insertions(+), 4 deletions(-)
--
1.8.3
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 <[email protected]>
---
drivers/mmc/host/dw_mmc.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index bc3a1bc..f20273e 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2488,13 +2488,18 @@ 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 some up in
+ * default state and our cache is incorrect.
+ */
+ host->current_speed = 0xFFFFFFFF;
+
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
On some platforms (like exynos5420) the dw_mmc controller may be in a
strange state after we wake up from sleep. Add callbacks to allow for
dealing with these quirks. Prevent interrupts from firing when we're
suspended since this strange state may cause interrupts to fire.
In my case I saw the WAKEUP_INT interrupt firing upon resume and
needed to add some code to handle this.
Signed-off-by: Doug Anderson <[email protected]>
---
drivers/mmc/host/dw_mmc.c | 12 ++++++++++++
drivers/mmc/host/dw_mmc.h | 4 ++++
2 files changed, 16 insertions(+)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index f20273e..2aaa93f 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2434,6 +2434,7 @@ EXPORT_SYMBOL(dw_mci_remove);
*/
int dw_mci_suspend(struct dw_mci *host)
{
+ const struct dw_mci_drv_data *drv_data = host->drv_data;
int i, ret = 0;
for (i = 0; i < host->num_slots; i++) {
@@ -2454,14 +2455,25 @@ int dw_mci_suspend(struct dw_mci *host)
if (host->vmmc)
regulator_disable(host->vmmc);
+ disable_irq(host->irq);
+
+ if (drv_data && drv_data->suspend)
+ drv_data->suspend(host);
+
return 0;
}
EXPORT_SYMBOL(dw_mci_suspend);
int dw_mci_resume(struct dw_mci *host)
{
+ const struct dw_mci_drv_data *drv_data = host->drv_data;
int i, ret;
+ if (drv_data && drv_data->resume)
+ drv_data->resume(host);
+
+ enable_irq(host->irq);
+
if (host->vmmc) {
ret = regulator_enable(host->vmmc);
if (ret) {
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 0b74189..52a3266 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -190,6 +190,8 @@ extern int dw_mci_resume(struct dw_mci *host);
* @prepare_command: handle CMD register extensions.
* @set_ios: handle bus specific extensions.
* @parse_dt: parse implementation specific device tree properties.
+ * @suspend: called late in the suspend process
+ * @resume: called early in the resume process
*
* Provide controller implementation specific extensions. The usage of this
* data structure is fully optional and usage of each member in this structure
@@ -202,5 +204,7 @@ struct dw_mci_drv_data {
void (*prepare_command)(struct dw_mci *host, u32 *cmdr);
void (*set_ios)(struct dw_mci *host, struct mmc_ios *ios);
int (*parse_dt)(struct dw_mci *host);
+ void (*suspend)(struct dw_mci *host);
+ void (*resume)(struct dw_mci *host);
};
#endif /* _DW_MMC_H_ */
--
1.8.3
If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up
looping around forever.
Signed-off-by: Doug Anderson <[email protected]>
---
drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index f013e7e..84d3b78 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 SDMMC_CMD_USE_HOLD_REG BIT(29)
@@ -102,6 +103,27 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
return 0;
}
+/**
+ * dw_mci_exynos_resume - Exynos-specific resume code
+ *
+ * We have seen cases (at least on the exynos5420) where turning off the INT
+ * power rail during suspend will 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.
+ */
+
+static int dw_mci_exynos_resume(struct dw_mci *host)
+{
+ u32 clksel;
+
+ clksel = mci_readl(host, CLKSEL);
+ if (clksel & SDMMC_CLKSEL_WAKEUP_INT)
+ mci_writel(host, CLKSEL, clksel);
+
+ return 0;
+}
+
static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr)
{
/*
@@ -165,6 +187,7 @@ static const struct dw_mci_drv_data exynos_drv_data = {
.caps = exynos_dwmmc_caps,
.init = dw_mci_exynos_priv_init,
.setup_clock = dw_mci_exynos_setup_clock,
+ .resume = dw_mci_exynos_resume,
.prepare_command = dw_mci_exynos_prepare_command,
.set_ios = dw_mci_exynos_set_ios,
.parse_dt = dw_mci_exynos_parse_dt,
--
1.8.3
After suspend/resume all of the dw_mmc registers are reset to
defaults. We restore most of them, but specifically don't setup the
clock registers after resume unless we've got a powered card. Things
still work because the core will eventually call set_ios() and we'll
set things up.
There doesn't seem to be any reason that I can see _not_ to set things
up after resume. Restoring this state makes the code easier to reason
about and should help prevent bugs. It also allows us to do a
register dump before and after suspend/resume to confirm that we've
set things up OK.
I examined the state of the dw_mmc instance before and after suspend
after this patch. I had no card inserted in an SD card slot.
Before this patch, differences were:
* CLKDIV (0x08)
* CLKENA (0x10)
* TMOUT (0x14)
* CMD (0x2C) - difference is not important
* CLKSEL (0x9C - exynos specific)
After this patch, only TMOUT was different. I have a separate patch
for that.
Signed-off-by: Doug Anderson <[email protected]>
---
drivers/mmc/host/dw_mmc.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 2aaa93f..a0a07df 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2510,9 +2510,7 @@ int dw_mci_resume(struct dw_mci *host)
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_set_ios(slot->mmc, &slot->mmc->ios);
ret = mmc_resume_host(host->slot[i]->mmc);
if (ret < 0)
--
1.8.3
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 clean.
Signed-off-by: Doug Anderson <[email protected]>
---
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 a0a07df..eedb517 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2494,6 +2494,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
Hi,
On Tue, Jul 9, 2013 at 10:31 AM, Doug Anderson <[email protected]> wrote:
> If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up
> looping around forever.
>
> Signed-off-by: Doug Anderson <[email protected]>
> ---
> drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
Grant just pointed out that the WAKEUP_INT is supposed to only be
enabled if bits 8, 9, or 10 are 1. Our driver never sets those so we
_should_ never get a WAKEUP_INT. Bits 8-10 are marked as RESERVED on
the exynos5420 manual, so the current guess is that they're broken on
that silicon but that sometimes the interrupt fires anyway.
In any case, it is still a reasonable thing to clear this interrupt at
wakeup if it has fired, even if we're on an exynos device without any
problems.
-Doug
Hi Doug,
On 9 July 2013 18:31, Doug Anderson <[email protected]> wrote:
> On some platforms (like exynos5420) the dw_mmc controller may be in a
> strange state after we wake up from sleep. Add callbacks to allow for
> dealing with these quirks. Prevent interrupts from firing when we're
> suspended since this strange state may cause interrupts to fire.
>
> In my case I saw the WAKEUP_INT interrupt firing upon resume and
> needed to add some code to handle this.
>
> Signed-off-by: Doug Anderson <[email protected]>
Would it make sense to take advantage of the {suspend,resume}_noirq
power management callbacks to clear that WAKEUP_INT before interrupts
are re-enabled, rather than explicitly disabling and enabling the
interrupt at the suspend/resume stage?
Cheers
James
James,
On Tue, Jul 9, 2013 at 2:17 PM, James Hogan <[email protected]> wrote:
> Hi Doug,
>
> On 9 July 2013 18:31, Doug Anderson <[email protected]> wrote:
>> On some platforms (like exynos5420) the dw_mmc controller may be in a
>> strange state after we wake up from sleep. Add callbacks to allow for
>> dealing with these quirks. Prevent interrupts from firing when we're
>> suspended since this strange state may cause interrupts to fire.
>>
>> In my case I saw the WAKEUP_INT interrupt firing upon resume and
>> needed to add some code to handle this.
>>
>> Signed-off-by: Doug Anderson <[email protected]>
>
> Would it make sense to take advantage of the {suspend,resume}_noirq
> power management callbacks to clear that WAKEUP_INT before interrupts
> are re-enabled, rather than explicitly disabling and enabling the
> interrupt at the suspend/resume stage?
That's a good suggestion. Let me give it a shot and get back to you
after I validate that it works.
-Doug
This series of patches addresses some suspend/resume problems with
dw_mmc on exynos platforms. Since suspend/resume is not fully working
on ToT Linux (3.10) on exynos5250-snow, this series was tested against
the current ToT ChromeOS 3.8 tree. I have confirmed basic booting
and eMMC / SD card usage (and compiling, honest!) against ToT Linux.
Changes in v2:
- Fix typo (some -> come)
- Use ~0 instead of 0xFFFFFFFF; add comment about value
- Use suspend_noirq as per James Hogan.
Doug Anderson (5):
mmc: dw_mmc: Invalidate cache of current_speed after suspend/resume
mmc: dw_mmc: Add suspend_noirq/resume_noirq callbacks for dw_mmc-pltfm
mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
mmc: dw_mmc: Always setup the bus after suspend/resume
mmc: dw_mmc: Set timeout to max upon resume
drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++
drivers/mmc/host/dw_mmc-pltfm.c | 37 ++++++++++++++++++++++++++++++++++---
drivers/mmc/host/dw_mmc.c | 15 +++++++++++----
drivers/mmc/host/dw_mmc.h | 4 ++++
4 files changed, 72 insertions(+), 7 deletions(-)
--
1.8.3
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 <[email protected]>
---
Changes in v2:
- Fix typo (some -> come)
- Use ~0 instead of 0xFFFFFFFF; 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 bc3a1bc..7a5ce6a 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2488,13 +2488,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
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 clean.
Signed-off-by: Doug Anderson <[email protected]>
---
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 be095b7..d2c5db3 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2482,6 +2482,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
After suspend/resume all of the dw_mmc registers are reset to
defaults. We restore most of them, but specifically don't setup the
clock registers after resume unless we've got a powered card. Things
still work because the core will eventually call set_ios() and we'll
set things up.
There doesn't seem to be any reason that I can see _not_ to set things
up after resume. Restoring this state makes the code easier to reason
about and should help prevent bugs. It also allows us to do a
register dump before and after suspend/resume to confirm that we've
set things up OK.
I examined the state of the dw_mmc instance before and after suspend
after this patch. I had no card inserted in an SD card slot.
Before this patch, differences were:
* CLKDIV (0x08)
* CLKENA (0x10)
* TMOUT (0x14)
* CMD (0x2C) - difference is not important
* CLKSEL (0x9C - exynos specific)
After this patch, only TMOUT was different. I have a separate patch
for that.
Signed-off-by: Doug Anderson <[email protected]>
---
Changes in v2: None
drivers/mmc/host/dw_mmc.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 7a5ce6a..be095b7 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2499,9 +2499,7 @@ int dw_mci_resume(struct dw_mci *host)
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_set_ios(slot->mmc, &slot->mmc->ios);
ret = mmc_resume_host(host->slot[i]->mmc);
if (ret < 0)
--
1.8.3
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.
Signed-off-by: Doug Anderson <[email protected]>
---
Changes in v2:
- Use suspend_noirq as per James Hogan.
drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index f013e7e..36b9620 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 SDMMC_CMD_USE_HOLD_REG BIT(29)
@@ -102,6 +103,27 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
return 0;
}
+/**
+ * dw_mci_exynos_resume_noirq - Exynos-specific resume code
+ *
+ * We have seen cases (at least on the exynos5420) where turning off the INT
+ * power rail during suspend will 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.
+ */
+
+static int dw_mci_exynos_resume_noirq(struct dw_mci *host)
+{
+ u32 clksel;
+
+ clksel = mci_readl(host, CLKSEL);
+ if (clksel & SDMMC_CLKSEL_WAKEUP_INT)
+ mci_writel(host, CLKSEL, clksel);
+
+ return 0;
+}
+
static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr)
{
/*
@@ -165,6 +187,7 @@ static const struct dw_mci_drv_data exynos_drv_data = {
.caps = exynos_dwmmc_caps,
.init = dw_mci_exynos_priv_init,
.setup_clock = dw_mci_exynos_setup_clock,
+ .resume_noirq = dw_mci_exynos_resume_noirq,
.prepare_command = dw_mci_exynos_prepare_command,
.set_ios = dw_mci_exynos_set_ios,
.parse_dt = dw_mci_exynos_parse_dt,
--
1.8.3
On some devices (like exynos5420) the dw_mmc controller may be in a
strange state after we wake up from sleep. Add callbacks to allow for
dealing with these quirks. We use the "_noirq" versions of the
callbacks since in the case of exynos5420 the strange state caused
interrupts to fire so we need to deal with it while interrupts are
still off.
At the moment this support is only added to dw_mmc-pltfm which calls
straight to the callback, since nobody but exynos needs it. We can
add some levels of indirection (a call into the generic dw_mmc code)
when someone finds a need.
Signed-off-by: Doug Anderson <[email protected]>
---
Changes in v2:
- Use suspend_noirq as per James Hogan.
drivers/mmc/host/dw_mmc-pltfm.c | 37 ++++++++++++++++++++++++++++++++++---
drivers/mmc/host/dw_mmc.h | 4 ++++
2 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
index 41c27b7..220568c 100644
--- a/drivers/mmc/host/dw_mmc-pltfm.c
+++ b/drivers/mmc/host/dw_mmc-pltfm.c
@@ -105,12 +105,43 @@ static int dw_mci_pltfm_resume(struct device *dev)
return 0;
}
+
+static int dw_mci_pltfm_suspend_noirq(struct device *dev)
+{
+ struct dw_mci *host = dev_get_drvdata(dev);
+ const struct dw_mci_drv_data *drv_data = host->drv_data;
+
+ if (drv_data && drv_data->suspend_noirq)
+ return drv_data->suspend_noirq(host);
+
+ return 0;
+}
+
+static int dw_mci_pltfm_resume_noirq(struct device *dev)
+{
+ struct dw_mci *host = dev_get_drvdata(dev);
+ const struct dw_mci_drv_data *drv_data = host->drv_data;
+
+ if (drv_data && drv_data->resume_noirq)
+ return drv_data->resume_noirq(host);
+
+ return 0;
+}
+
+
#else
-#define dw_mci_pltfm_suspend NULL
-#define dw_mci_pltfm_resume NULL
+#define dw_mci_pltfm_suspend NULL
+#define dw_mci_pltfm_resume NULL
+#define dw_mci_pltfm_suspend_noirq NULL
+#define dw_mci_pltfm_resume_noirq NULL
#endif /* CONFIG_PM_SLEEP */
-SIMPLE_DEV_PM_OPS(dw_mci_pltfm_pmops, dw_mci_pltfm_suspend, dw_mci_pltfm_resume);
+const struct dev_pm_ops dw_mci_pltfm_pmops = {
+ SET_SYSTEM_SLEEP_PM_OPS(dw_mci_pltfm_suspend, dw_mci_pltfm_resume)
+ .suspend_noirq = dw_mci_pltfm_suspend_noirq,
+ .resume_noirq = dw_mci_pltfm_resume_noirq,
+};
+
EXPORT_SYMBOL_GPL(dw_mci_pltfm_pmops);
static const struct of_device_id dw_mci_pltfm_match[] = {
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 0b74189..5d0398f 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -190,6 +190,8 @@ extern int dw_mci_resume(struct dw_mci *host);
* @prepare_command: handle CMD register extensions.
* @set_ios: handle bus specific extensions.
* @parse_dt: parse implementation specific device tree properties.
+ * @suspend_noirq: called late in the suspend process
+ * @resume_noirq: called early in the resume process
*
* Provide controller implementation specific extensions. The usage of this
* data structure is fully optional and usage of each member in this structure
@@ -202,5 +204,7 @@ struct dw_mci_drv_data {
void (*prepare_command)(struct dw_mci *host, u32 *cmdr);
void (*set_ios)(struct dw_mci *host, struct mmc_ios *ios);
int (*parse_dt)(struct dw_mci *host);
+ int (*suspend_noirq)(struct dw_mci *host);
+ int (*resume_noirq)(struct dw_mci *host);
};
#endif /* _DW_MMC_H_ */
--
1.8.3
Hi Doug,
On 10/07/13 00:19, Doug Anderson wrote:
> On some devices (like exynos5420) the dw_mmc controller may be in a
> strange state after we wake up from sleep. Add callbacks to allow for
> dealing with these quirks. We use the "_noirq" versions of the
> callbacks since in the case of exynos5420 the strange state caused
> interrupts to fire so we need to deal with it while interrupts are
> still off.
>
> At the moment this support is only added to dw_mmc-pltfm which calls
> straight to the callback, since nobody but exynos needs it. We can
> add some levels of indirection (a call into the generic dw_mmc code)
> when someone finds a need.
>
> Signed-off-by: Doug Anderson <[email protected]>
Reviewed-by: James Hogan <[email protected]>
<snip>
>
> -SIMPLE_DEV_PM_OPS(dw_mci_pltfm_pmops, dw_mci_pltfm_suspend, dw_mci_pltfm_resume);
> +const struct dev_pm_ops dw_mci_pltfm_pmops = {
> + SET_SYSTEM_SLEEP_PM_OPS(dw_mci_pltfm_suspend, dw_mci_pltfm_resume)
> + .suspend_noirq = dw_mci_pltfm_suspend_noirq,
> + .resume_noirq = dw_mci_pltfm_resume_noirq,
> +};
Does Exynos support hibernation? I see that SET_SYSTEM_SLEEP_PM_OPS sets
freeze, thaw, poweroff, and restore callbacks too. You may not need the
hibernation specific _noirq callbacks though in which case it's probably
fine as it is.
Cheers
James
On Wed, July 10, 2013, Doug Anderson wrote:
> 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.
>
> Signed-off-by: Doug Anderson <[email protected]>
> ---
> Changes in v2:
> - Use suspend_noirq as per James Hogan.
>
> drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
> index f013e7e..36b9620 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 SDMMC_CMD_USE_HOLD_REG BIT(29)
>
> @@ -102,6 +103,27 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
> return 0;
> }
>
> +/**
> + * dw_mci_exynos_resume_noirq - Exynos-specific resume code
> + *
> + * We have seen cases (at least on the exynos5420) where turning off the INT
> + * power rail during suspend will 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.
> + */
As I know this bit is auto-cleared.
Did you find the cause of this problem?
How about your GPIO setting in sleep?
Currently, we don't know why the problem is happened.
At least, we should make it clear.
Thanks,
Seungwon Jeon
> +
> +static int dw_mci_exynos_resume_noirq(struct dw_mci *host)
> +{
> + u32 clksel;
> +
> + clksel = mci_readl(host, CLKSEL);
> + if (clksel & SDMMC_CLKSEL_WAKEUP_INT)
> + mci_writel(host, CLKSEL, clksel);
> +
> + return 0;
> +}
> +
> static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr)
> {
> /*
> @@ -165,6 +187,7 @@ static const struct dw_mci_drv_data exynos_drv_data = {
> .caps = exynos_dwmmc_caps,
> .init = dw_mci_exynos_priv_init,
> .setup_clock = dw_mci_exynos_setup_clock,
> + .resume_noirq = dw_mci_exynos_resume_noirq,
> .prepare_command = dw_mci_exynos_prepare_command,
> .set_ios = dw_mci_exynos_set_ios,
> .parse_dt = dw_mci_exynos_parse_dt,
> --
> 1.8.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, July 10, 2013, 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 clean.
>
> Signed-off-by: Doug Anderson <[email protected]>
Acked-by: Seungwon Jeon <[email protected]>
Seungwon,
On Wed, Jul 10, 2013 at 7:54 AM, Seungwon Jeon <[email protected]> wrote:
> On Wed, July 10, 2013, Doug Anderson wrote:
>> 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.
>>
>> Signed-off-by: Doug Anderson <[email protected]>
>> ---
>> Changes in v2:
>> - Use suspend_noirq as per James Hogan.
>>
>> drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
>> index f013e7e..36b9620 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 SDMMC_CMD_USE_HOLD_REG BIT(29)
>>
>> @@ -102,6 +103,27 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
>> return 0;
>> }
>>
>> +/**
>> + * dw_mci_exynos_resume_noirq - Exynos-specific resume code
>> + *
>> + * We have seen cases (at least on the exynos5420) where turning off the INT
>> + * power rail during suspend will 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.
>> + */
> As I know this bit is auto-cleared.
> Did you find the cause of this problem?
> How about your GPIO setting in sleep?
> Currently, we don't know why the problem is happened.
> At least, we should make it clear.
Yes, the documentation that I have says that this bit is "auto
cleared" as well but doesn't indicate under what conditions it is auto
cleared. From testing how this bit reacts I have found that writing a
1 to it clears the bit--in other words it behaves like bits in
RINTSTS. That's a terrible design for a bit in a register with shared
config but appears to be how it works.
Note: in a sense it will be "auto cleared" because doing a
read-modify-write of any other bits in this register will clear the
interrupt.
I have asked for official confirmation.
We have found that on exynos5420 bits 8-10 of CLKSEL are marked as
RESERVED. Those bits are documented to enable the WAKEUP_INT on
exynos5250. My best guess is that these bits are not used on
exynos5420 and the WAKEUP_INT line is left floating, which means it
can fire randomly. I have also asked for official confirmation about
this.
I will likely merge this change locally in our kernel tree while
waiting for a response. If you would like to wait before Acking
that's very reasonable. Do you have any other problems with this
change assuming my understanding above is correct?
-Doug
James,
On Wed, Jul 10, 2013 at 1:37 AM, James Hogan <[email protected]> wrote:
>> -SIMPLE_DEV_PM_OPS(dw_mci_pltfm_pmops, dw_mci_pltfm_suspend, dw_mci_pltfm_resume);
>> +const struct dev_pm_ops dw_mci_pltfm_pmops = {
>> + SET_SYSTEM_SLEEP_PM_OPS(dw_mci_pltfm_suspend, dw_mci_pltfm_resume)
>> + .suspend_noirq = dw_mci_pltfm_suspend_noirq,
>> + .resume_noirq = dw_mci_pltfm_resume_noirq,
>> +};
>
> Does Exynos support hibernation? I see that SET_SYSTEM_SLEEP_PM_OPS sets
> freeze, thaw, poweroff, and restore callbacks too. You may not need the
> hibernation specific _noirq callbacks though in which case it's probably
> fine as it is.
Thank you for your review and good suggestions. You're right that I
should add the other "noirq" variants in here. Even if hibernation
isn't supported now that's the right thing to do. I will fix that and
send v3 with your "Reviewed-by".
-Doug
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 <[email protected]>
---
Changes in v3: None
Changes in v2:
- Fix typo (some -> come)
- Use ~0 instead of 0xFFFFFFFF; 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 bc3a1bc..7a5ce6a 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2488,13 +2488,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
After suspend/resume all of the dw_mmc registers are reset to
defaults. We restore most of them, but specifically don't setup the
clock registers after resume unless we've got a powered card. Things
still work because the core will eventually call set_ios() and we'll
set things up.
There doesn't seem to be any reason that I can see _not_ to set things
up after resume. Restoring this state makes the code easier to reason
about and should help prevent bugs. It also allows us to do a
register dump before and after suspend/resume to confirm that we've
set things up OK.
I examined the state of the dw_mmc instance before and after suspend
after this patch. I had no card inserted in an SD card slot.
Before this patch, differences were:
* CLKDIV (0x08)
* CLKENA (0x10)
* TMOUT (0x14)
* CMD (0x2C) - difference is not important
* CLKSEL (0x9C - exynos specific)
After this patch, only TMOUT was different. I have a separate patch
for that.
Signed-off-by: Doug Anderson <[email protected]>
---
Changes in v3: None
Changes in v2: None
drivers/mmc/host/dw_mmc.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 7a5ce6a..be095b7 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2499,9 +2499,7 @@ int dw_mci_resume(struct dw_mci *host)
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_set_ios(slot->mmc, &slot->mmc->ios);
ret = mmc_resume_host(host->slot[i]->mmc);
if (ret < 0)
--
1.8.3
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.
Signed-off-by: Doug Anderson <[email protected]>
---
Changes in v3: None
Changes in v2:
- Use suspend_noirq as per James Hogan.
drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index f013e7e..36b9620 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 SDMMC_CMD_USE_HOLD_REG BIT(29)
@@ -102,6 +103,27 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
return 0;
}
+/**
+ * dw_mci_exynos_resume_noirq - Exynos-specific resume code
+ *
+ * We have seen cases (at least on the exynos5420) where turning off the INT
+ * power rail during suspend will 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.
+ */
+
+static int dw_mci_exynos_resume_noirq(struct dw_mci *host)
+{
+ u32 clksel;
+
+ clksel = mci_readl(host, CLKSEL);
+ if (clksel & SDMMC_CLKSEL_WAKEUP_INT)
+ mci_writel(host, CLKSEL, clksel);
+
+ return 0;
+}
+
static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr)
{
/*
@@ -165,6 +187,7 @@ static const struct dw_mci_drv_data exynos_drv_data = {
.caps = exynos_dwmmc_caps,
.init = dw_mci_exynos_priv_init,
.setup_clock = dw_mci_exynos_setup_clock,
+ .resume_noirq = dw_mci_exynos_resume_noirq,
.prepare_command = dw_mci_exynos_prepare_command,
.set_ios = dw_mci_exynos_set_ios,
.parse_dt = dw_mci_exynos_parse_dt,
--
1.8.3
On some devices (like exynos5420) the dw_mmc controller may be in a
strange state after we wake up from sleep. Add callbacks to allow for
dealing with these quirks. We use the "_noirq" versions of the
callbacks since in the case of exynos5420 the strange state caused
interrupts to fire so we need to deal with it while interrupts are
still off.
At the moment this support is only added to dw_mmc-pltfm which calls
straight to the callback, since nobody but exynos needs it. We can
add some levels of indirection (a call into the generic dw_mmc code)
when someone finds a need.
Signed-off-by: Doug Anderson <[email protected]>
Reviewed-by: James Hogan <[email protected]>
---
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-pltfm.c | 41 ++++++++++++++++++++++++++++++++++++++---
drivers/mmc/host/dw_mmc.h | 4 ++++
2 files changed, 42 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
index 41c27b7..742ef76 100644
--- a/drivers/mmc/host/dw_mmc-pltfm.c
+++ b/drivers/mmc/host/dw_mmc-pltfm.c
@@ -105,12 +105,47 @@ static int dw_mci_pltfm_resume(struct device *dev)
return 0;
}
+
+static int dw_mci_pltfm_suspend_noirq(struct device *dev)
+{
+ struct dw_mci *host = dev_get_drvdata(dev);
+ const struct dw_mci_drv_data *drv_data = host->drv_data;
+
+ if (drv_data && drv_data->suspend_noirq)
+ return drv_data->suspend_noirq(host);
+
+ return 0;
+}
+
+static int dw_mci_pltfm_resume_noirq(struct device *dev)
+{
+ struct dw_mci *host = dev_get_drvdata(dev);
+ const struct dw_mci_drv_data *drv_data = host->drv_data;
+
+ if (drv_data && drv_data->resume_noirq)
+ return drv_data->resume_noirq(host);
+
+ return 0;
+}
+
+
#else
-#define dw_mci_pltfm_suspend NULL
-#define dw_mci_pltfm_resume NULL
+#define dw_mci_pltfm_suspend NULL
+#define dw_mci_pltfm_resume NULL
+#define dw_mci_pltfm_suspend_noirq NULL
+#define dw_mci_pltfm_resume_noirq NULL
#endif /* CONFIG_PM_SLEEP */
-SIMPLE_DEV_PM_OPS(dw_mci_pltfm_pmops, dw_mci_pltfm_suspend, dw_mci_pltfm_resume);
+const struct dev_pm_ops dw_mci_pltfm_pmops = {
+ SET_SYSTEM_SLEEP_PM_OPS(dw_mci_pltfm_suspend, dw_mci_pltfm_resume)
+ .suspend_noirq = dw_mci_pltfm_suspend_noirq,
+ .resume_noirq = dw_mci_pltfm_resume_noirq,
+ .freeze_noirq = dw_mci_pltfm_suspend_noirq,
+ .thaw_noirq = dw_mci_pltfm_resume_noirq,
+ .poweroff_noirq = dw_mci_pltfm_suspend_noirq,
+ .restore_noirq = dw_mci_pltfm_resume_noirq,
+};
+
EXPORT_SYMBOL_GPL(dw_mci_pltfm_pmops);
static const struct of_device_id dw_mci_pltfm_match[] = {
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 0b74189..5d0398f 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -190,6 +190,8 @@ extern int dw_mci_resume(struct dw_mci *host);
* @prepare_command: handle CMD register extensions.
* @set_ios: handle bus specific extensions.
* @parse_dt: parse implementation specific device tree properties.
+ * @suspend_noirq: called late in the suspend process
+ * @resume_noirq: called early in the resume process
*
* Provide controller implementation specific extensions. The usage of this
* data structure is fully optional and usage of each member in this structure
@@ -202,5 +204,7 @@ struct dw_mci_drv_data {
void (*prepare_command)(struct dw_mci *host, u32 *cmdr);
void (*set_ios)(struct dw_mci *host, struct mmc_ios *ios);
int (*parse_dt)(struct dw_mci *host);
+ int (*suspend_noirq)(struct dw_mci *host);
+ int (*resume_noirq)(struct dw_mci *host);
};
#endif /* _DW_MMC_H_ */
--
1.8.3
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 clean.
Signed-off-by: Doug Anderson <[email protected]>
Acked-by: Seungwon Jeon <[email protected]>
---
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 be095b7..d2c5db3 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2482,6 +2482,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
This series of patches addresses some suspend/resume problems with
dw_mmc on exynos platforms. Since suspend/resume is not fully working
on ToT Linux (3.10) on exynos5250-snow, this series was tested against
the current ToT ChromeOS 3.8 tree. I have confirmed basic booting
and eMMC / SD card usage (and compiling, honest!) against ToT Linux.
Changes in v3:
- Add freeze/thaw and poweroff/restore noirq entries.
Changes in v2:
- Fix typo (some -> come)
- Use ~0 instead of 0xFFFFFFFF; add comment about value
- Use suspend_noirq as per James Hogan.
Doug Anderson (5):
mmc: dw_mmc: Invalidate cache of current_speed after suspend/resume
mmc: dw_mmc: Add suspend_noirq/resume_noirq callbacks for dw_mmc-pltfm
mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
mmc: dw_mmc: Always setup the bus after suspend/resume
mmc: dw_mmc: Set timeout to max upon resume
drivers/mmc/host/dw_mmc-exynos.c | 23 ++++++++++++++++++++++
drivers/mmc/host/dw_mmc-pltfm.c | 41 +++++++++++++++++++++++++++++++++++++---
drivers/mmc/host/dw_mmc.c | 15 +++++++++++----
drivers/mmc/host/dw_mmc.h | 4 ++++
4 files changed, 76 insertions(+), 7 deletions(-)
--
1.8.3
On Tue, Jul 9, 2013 at 12:09 PM, Doug Anderson <[email protected]> wrote:
> Hi,
>
> On Tue, Jul 9, 2013 at 10:31 AM, Doug Anderson <[email protected]> wrote:
>> If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up
>> looping around forever.
>>
>> Signed-off-by: Doug Anderson <[email protected]>
>> ---
>> drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>
> Grant just pointed out that the WAKEUP_INT is supposed to only be
> enabled if bits 8, 9, or 10 are 1. Our driver never sets those so we
> _should_ never get a WAKEUP_INT. Bits 8-10 are marked as RESERVED on
> the exynos5420 manual, so the current guess is that they're broken on
> that silicon but that sometimes the interrupt fires anyway.
>
> In any case, it is still a reasonable thing to clear this interrupt at
> wakeup if it has fired, even if we're on an exynos device without any
> problems.
I agree. Can add:
Reviewed-by: Grant Grundler <[email protected]>
thanks,
grant
>
> -Doug
On Thu, July 11, 2013, Doug Anderson wrote:
> Seungwon,
>
> On Wed, Jul 10, 2013 at 7:54 AM, Seungwon Jeon <[email protected]> wrote:
> > On Wed, July 10, 2013, Doug Anderson wrote:
> >> 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.
> >>
> >> Signed-off-by: Doug Anderson <[email protected]>
> >> ---
> >> Changes in v2:
> >> - Use suspend_noirq as per James Hogan.
> >>
> >> drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++
> >> 1 file changed, 23 insertions(+)
> >>
> >> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
> >> index f013e7e..36b9620 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 SDMMC_CMD_USE_HOLD_REG BIT(29)
> >>
> >> @@ -102,6 +103,27 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
> >> return 0;
> >> }
> >>
> >> +/**
> >> + * dw_mci_exynos_resume_noirq - Exynos-specific resume code
> >> + *
> >> + * We have seen cases (at least on the exynos5420) where turning off the INT
> >> + * power rail during suspend will 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.
> >> + */
> > As I know this bit is auto-cleared.
> > Did you find the cause of this problem?
> > How about your GPIO setting in sleep?
> > Currently, we don't know why the problem is happened.
> > At least, we should make it clear.
>
> Yes, the documentation that I have says that this bit is "auto
> cleared" as well but doesn't indicate under what conditions it is auto
> cleared. From testing how this bit reacts I have found that writing a
> 1 to it clears the bit--in other words it behaves like bits in
> RINTSTS. That's a terrible design for a bit in a register with shared
> config but appears to be how it works.
>
> Note: in a sense it will be "auto cleared" because doing a
> read-modify-write of any other bits in this register will clear the
> interrupt.
>
> I have asked for official confirmation.
>
> We have found that on exynos5420 bits 8-10 of CLKSEL are marked as
> RESERVED. Those bits are documented to enable the WAKEUP_INT on
> exynos5250. My best guess is that these bits are not used on
> exynos5420 and the WAKEUP_INT line is left floating, which means it
> can fire randomly. I have also asked for official confirmation about
> this.
Sorry for late response.
Yes, it's not clear.
If you get the confirmation, could you share this problem?
Possibly, auto-clear may not be implemented.
Then, manual should be correct.
Thanks,
Seungwon Jeon
>
>
> I will likely merge this change locally in our kernel tree while
> waiting for a response. If you would like to wait before Acking
> that's very reasonable. Do you have any other problems with this
> change assuming my understanding above is correct?
>
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, July 11, 2013, Doug Anderson wrote:
> On some devices (like exynos5420) the dw_mmc controller may be in a
> strange state after we wake up from sleep. Add callbacks to allow for
> dealing with these quirks. We use the "_noirq" versions of the
> callbacks since in the case of exynos5420 the strange state caused
> interrupts to fire so we need to deal with it while interrupts are
> still off.
>
> At the moment this support is only added to dw_mmc-pltfm which calls
> straight to the callback, since nobody but exynos needs it. We can
> add some levels of indirection (a call into the generic dw_mmc code)
> when someone finds a need.
I think It would be better to add _noirq only to dw_mmc-exynos.
That is we can add dev_pm_ops for dw_mmc-exynos's own.
As you recognize, there is no common routine which is not introduced for dw_mmc xxx_noirq now.
I feel like it is for handling quirk.
If we meet use case for that in some day, it could be added commonly.
How do you think?
Thanks,
Seungwon Jeon
>
> Signed-off-by: Doug Anderson <[email protected]>
> Reviewed-by: James Hogan <[email protected]>
> ---
> 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-pltfm.c | 41 ++++++++++++++++++++++++++++++++++++++---
> drivers/mmc/host/dw_mmc.h | 4 ++++
> 2 files changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
> index 41c27b7..742ef76 100644
> --- a/drivers/mmc/host/dw_mmc-pltfm.c
> +++ b/drivers/mmc/host/dw_mmc-pltfm.c
> @@ -105,12 +105,47 @@ static int dw_mci_pltfm_resume(struct device *dev)
>
> return 0;
> }
> +
> +static int dw_mci_pltfm_suspend_noirq(struct device *dev)
> +{
> + struct dw_mci *host = dev_get_drvdata(dev);
> + const struct dw_mci_drv_data *drv_data = host->drv_data;
> +
> + if (drv_data && drv_data->suspend_noirq)
> + return drv_data->suspend_noirq(host);
> +
> + return 0;
> +}
> +
> +static int dw_mci_pltfm_resume_noirq(struct device *dev)
> +{
> + struct dw_mci *host = dev_get_drvdata(dev);
> + const struct dw_mci_drv_data *drv_data = host->drv_data;
> +
> + if (drv_data && drv_data->resume_noirq)
> + return drv_data->resume_noirq(host);
> +
> + return 0;
> +}
> +
> +
> #else
> -#define dw_mci_pltfm_suspend NULL
> -#define dw_mci_pltfm_resume NULL
> +#define dw_mci_pltfm_suspend NULL
> +#define dw_mci_pltfm_resume NULL
> +#define dw_mci_pltfm_suspend_noirq NULL
> +#define dw_mci_pltfm_resume_noirq NULL
> #endif /* CONFIG_PM_SLEEP */
>
> -SIMPLE_DEV_PM_OPS(dw_mci_pltfm_pmops, dw_mci_pltfm_suspend, dw_mci_pltfm_resume);
> +const struct dev_pm_ops dw_mci_pltfm_pmops = {
> + SET_SYSTEM_SLEEP_PM_OPS(dw_mci_pltfm_suspend, dw_mci_pltfm_resume)
> + .suspend_noirq = dw_mci_pltfm_suspend_noirq,
> + .resume_noirq = dw_mci_pltfm_resume_noirq,
> + .freeze_noirq = dw_mci_pltfm_suspend_noirq,
> + .thaw_noirq = dw_mci_pltfm_resume_noirq,
> + .poweroff_noirq = dw_mci_pltfm_suspend_noirq,
> + .restore_noirq = dw_mci_pltfm_resume_noirq,
> +};
> +
> EXPORT_SYMBOL_GPL(dw_mci_pltfm_pmops);
>
> static const struct of_device_id dw_mci_pltfm_match[] = {
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 0b74189..5d0398f 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -190,6 +190,8 @@ extern int dw_mci_resume(struct dw_mci *host);
> * @prepare_command: handle CMD register extensions.
> * @set_ios: handle bus specific extensions.
> * @parse_dt: parse implementation specific device tree properties.
> + * @suspend_noirq: called late in the suspend process
> + * @resume_noirq: called early in the resume process
> *
> * Provide controller implementation specific extensions. The usage of this
> * data structure is fully optional and usage of each member in this structure
> @@ -202,5 +204,7 @@ struct dw_mci_drv_data {
> void (*prepare_command)(struct dw_mci *host, u32 *cmdr);
> void (*set_ios)(struct dw_mci *host, struct mmc_ios *ios);
> int (*parse_dt)(struct dw_mci *host);
> + int (*suspend_noirq)(struct dw_mci *host);
> + int (*resume_noirq)(struct dw_mci *host);
> };
> #endif /* _DW_MMC_H_ */
> --
> 1.8.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Doug,
I think these patch-set didn't base on latest mmc-next.
If you can rebase on latest mmc-next, it's helpful to me for testing.
Best Regards,
Jaehoon Chung
On 07/11/2013 12:42 AM, Doug Anderson wrote:
> 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.
>
> Signed-off-by: Doug Anderson <[email protected]>
> ---
> Changes in v3: None
> Changes in v2:
> - Use suspend_noirq as per James Hogan.
>
> drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
> index f013e7e..36b9620 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 SDMMC_CMD_USE_HOLD_REG BIT(29)
>
> @@ -102,6 +103,27 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
> return 0;
> }
>
> +/**
> + * dw_mci_exynos_resume_noirq - Exynos-specific resume code
> + *
> + * We have seen cases (at least on the exynos5420) where turning off the INT
> + * power rail during suspend will 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.
> + */
> +
> +static int dw_mci_exynos_resume_noirq(struct dw_mci *host)
> +{
> + u32 clksel;
> +
> + clksel = mci_readl(host, CLKSEL);
> + if (clksel & SDMMC_CLKSEL_WAKEUP_INT)
> + mci_writel(host, CLKSEL, clksel);
> +
> + return 0;
> +}
> +
> static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr)
> {
> /*
> @@ -165,6 +187,7 @@ static const struct dw_mci_drv_data exynos_drv_data = {
> .caps = exynos_dwmmc_caps,
> .init = dw_mci_exynos_priv_init,
> .setup_clock = dw_mci_exynos_setup_clock,
> + .resume_noirq = dw_mci_exynos_resume_noirq,
> .prepare_command = dw_mci_exynos_prepare_command,
> .set_ios = dw_mci_exynos_set_ios,
> .parse_dt = dw_mci_exynos_parse_dt,
>
Seungwon,
On Mon, Jul 15, 2013 at 5:09 AM, Seungwon Jeon <[email protected]> wrote:
> Sorry for late response.
> Yes, it's not clear.
> If you get the confirmation, could you share this problem?
> Possibly, auto-clear may not be implemented.
> Then, manual should be correct.
I just got an update from my contacts. They confirm that bit 11 is
not automatically cleared and that writing to it will clear it.
Hopefully this information will make it into the next version of any
documentation that you receive as well.
It is still unclear exactly why the WAKEUP_INT was being asserted on
our board despite the fact that all of the wakeup control signals
(bits 10:8) were 0. That is still being investigated.
-Doug
Seungwon Jeon,
On Mon, Jul 15, 2013 at 5:09 AM, Seungwon Jeon <[email protected]> wrote:
> On Thu, July 11, 2013, Doug Anderson wrote:
>> On some devices (like exynos5420) the dw_mmc controller may be in a
>> strange state after we wake up from sleep. Add callbacks to allow for
>> dealing with these quirks. We use the "_noirq" versions of the
>> callbacks since in the case of exynos5420 the strange state caused
>> interrupts to fire so we need to deal with it while interrupts are
>> still off.
>>
>> At the moment this support is only added to dw_mmc-pltfm which calls
>> straight to the callback, since nobody but exynos needs it. We can
>> add some levels of indirection (a call into the generic dw_mmc code)
>> when someone finds a need.
> I think It would be better to add _noirq only to dw_mmc-exynos.
> That is we can add dev_pm_ops for dw_mmc-exynos's own.
> As you recognize, there is no common routine which is not introduced for dw_mmc xxx_noirq now.
> I feel like it is for handling quirk.
> If we meet use case for that in some day, it could be added commonly.
> How do you think?
>
> Thanks,
> Seungwon Jeon
Sorry for the long delay in responding.
I originally didn't do what you proposed since dw_mmc-exynos uses the
dw_mci_pltfm_pmops directly. ...but I agree that it is cleaner, so
I've switched the code to do as you say. New patch coming shortly.
Thank you for your review.
-Doug
Seungwon,
On Wed, Jul 31, 2013 at 9:18 AM, Doug Anderson <[email protected]> wrote:
> Seungwon,
>
> On Mon, Jul 15, 2013 at 5:09 AM, Seungwon Jeon <[email protected]> wrote:
>> Sorry for late response.
>> Yes, it's not clear.
>> If you get the confirmation, could you share this problem?
>> Possibly, auto-clear may not be implemented.
>> Then, manual should be correct.
>
> I just got an update from my contacts. They confirm that bit 11 is
> not automatically cleared and that writing to it will clear it.
> Hopefully this information will make it into the next version of any
> documentation that you receive as well.
>
> It is still unclear exactly why the WAKEUP_INT was being asserted on
> our board despite the fact that all of the wakeup control signals
> (bits 10:8) were 0. That is still being investigated.
I have further confirmed from my contacts at Samsung that this is a
real silicon errata and that clearing the WAKEUP_INT as I am doing in
this series is the right workaround.
New patch coming shortly based against ToT linux
(v3.11-rc4-20-g0fff106). I have confirmed that it applies cleanly to
mmc-next, though I haven't tried booting with that tree.
After suspend/resume all of the dw_mmc registers are reset to
defaults. We restore most of them, but specifically don't setup the
clock registers after resume unless we've got a powered card. Things
still work because the core will eventually call set_ios() and we'll
set things up.
There doesn't seem to be any reason that I can see _not_ to set things
up after resume. Restoring this state makes the code easier to reason
about and should help prevent bugs. It also allows us to do a
register dump before and after suspend/resume to confirm that we've
set things up OK.
I examined the state of the dw_mmc instance before and after suspend
after this patch. I had no card inserted in an SD card slot.
Before this patch, differences were:
* CLKDIV (0x08)
* CLKENA (0x10)
* TMOUT (0x14)
* CMD (0x2C) - difference is not important
* CLKSEL (0x9C - exynos specific)
After this patch, only TMOUT was different. I have a separate patch
for that.
Signed-off-by: Doug Anderson <[email protected]>
---
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/mmc/host/dw_mmc.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 13a363c..0fa3135 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2522,9 +2522,7 @@ int dw_mci_resume(struct dw_mci *host)
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_set_ios(slot->mmc, &slot->mmc->ios);
ret = mmc_resume_host(host->slot[i]->mmc);
if (ret < 0)
--
1.8.3
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 <[email protected]>
---
Changes in v4: None
Changes in v3: None
Changes in v2:
- Fix typo (some -> come)
- Use ~0 instead of 0xFFFFFFFF; 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
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 clean.
Signed-off-by: Doug Anderson <[email protected]>
Acked-by: Seungwon Jeon <[email protected]>
---
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 0fa3135..1abcb36 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2505,6 +2505,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
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 <[email protected]>
---
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 | 51 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 49 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index 866edef..0c1f192 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,30 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
return 0;
}
+/**
+ * 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 and the bug
+ * may be more widespread than just exynos5420.
+ */
+
+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;
+}
+
static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr)
{
/*
@@ -187,17 +212,39 @@ static int dw_mci_exynos_probe(struct platform_device *pdev)
return dw_mci_pltfm_register(pdev, drv_data);
}
+static struct dev_pm_ops dw_mci_exynos_pmops;
+
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,
},
};
-module_platform_driver(dw_mci_exynos_pltfm_driver);
+static int __init dw_mci_exynos_init(void)
+{
+ /* Add a "noirq" resume to platform pmops */
+ memcpy(&dw_mci_exynos_pmops, &dw_mci_pltfm_pmops,
+ sizeof(dw_mci_exynos_pmops));
+ WARN_ON(dw_mci_exynos_pmops.resume_noirq ||
+ dw_mci_exynos_pmops.thaw_noirq ||
+ dw_mci_exynos_pmops.restore_noirq);
+ dw_mci_exynos_pmops.resume_noirq = dw_mci_exynos_resume_noirq;
+ dw_mci_exynos_pmops.thaw_noirq = dw_mci_exynos_resume_noirq;
+ dw_mci_exynos_pmops.restore_noirq = dw_mci_exynos_resume_noirq;
+
+ return platform_driver_register(&dw_mci_exynos_pltfm_driver);
+}
+module_init(dw_mci_exynos_init);
+
+static void __exit dw_mci_exynos_exit(void)
+{
+ platform_driver_unregister(&dw_mci_exynos_pltfm_driver);
+}
+module_exit(dw_mci_exynos_exit);
MODULE_DESCRIPTION("Samsung Specific DW-MSHC Driver Extension");
MODULE_AUTHOR("Thomas Abraham <[email protected]");
--
1.8.3
This series of patches addresses some suspend/resume problems with
dw_mmc on exynos platforms, espeically exynos5420. Since
suspend/resume is not fully working on ToT Linux (v3.11-rc4) on
exynos5250-snow, this series was tested against the current ToT
ChromeOS 3.8 tree. I have confirmed basic booting and eMMC / SD card
usage (and compiling, honest!) against ToT Linux.
I have received confirmation from Samsung that the problem solved is a
silicon errata on exynos5420 and that this is a good fix.
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:
- Fix typo (some -> come)
- Use ~0 instead of 0xFFFFFFFF; add comment about value
- Use suspend_noirq as per James Hogan.
Doug Anderson (4):
mmc: dw_mmc: Invalidate cache of current_speed after suspend/resume
mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
mmc: dw_mmc: Always setup the bus after suspend/resume
mmc: dw_mmc: Set timeout to max upon resume
drivers/mmc/host/dw_mmc-exynos.c | 51 ++++++++++++++++++++++++++++++++++++++--
drivers/mmc/host/dw_mmc.c | 15 ++++++++----
2 files changed, 60 insertions(+), 6 deletions(-)
--
1.8.3
Hi Doug,
See my comment inline.
On Tuesday 06 of August 2013 14:37:49 Doug Anderson wrote:
> 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 <[email protected]>
> ---
> 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 | 51
> ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 49
> insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc-exynos.c
> b/drivers/mmc/host/dw_mmc-exynos.c index 866edef..0c1f192 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,30 @@ static int dw_mci_exynos_setup_clock(struct dw_mci
> *host) return 0;
> }
>
> +/**
> + * 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 and
> the bug + * may be more widespread than just exynos5420.
> + */
> +
> +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);
What about clock gating? Will the clock used for clocking this register be
always enabled when this gets called?
Best regards,
Tomasz
> + return 0;
> +}
> +
> static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32
> *cmdr) {
> /*
> @@ -187,17 +212,39 @@ static int dw_mci_exynos_probe(struct
> platform_device *pdev) return dw_mci_pltfm_register(pdev, drv_data);
> }
>
> +static struct dev_pm_ops dw_mci_exynos_pmops;
> +
> 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,
> },
> };
>
> -module_platform_driver(dw_mci_exynos_pltfm_driver);
> +static int __init dw_mci_exynos_init(void)
> +{
> + /* Add a "noirq" resume to platform pmops */
> + memcpy(&dw_mci_exynos_pmops, &dw_mci_pltfm_pmops,
> + sizeof(dw_mci_exynos_pmops));
> + WARN_ON(dw_mci_exynos_pmops.resume_noirq ||
> + dw_mci_exynos_pmops.thaw_noirq ||
> + dw_mci_exynos_pmops.restore_noirq);
> + dw_mci_exynos_pmops.resume_noirq = dw_mci_exynos_resume_noirq;
> + dw_mci_exynos_pmops.thaw_noirq = dw_mci_exynos_resume_noirq;
> + dw_mci_exynos_pmops.restore_noirq = dw_mci_exynos_resume_noirq;
> +
> + return platform_driver_register(&dw_mci_exynos_pltfm_driver);
> +}
> +module_init(dw_mci_exynos_init);
> +
> +static void __exit dw_mci_exynos_exit(void)
> +{
> + platform_driver_unregister(&dw_mci_exynos_pltfm_driver);
> +}
> +module_exit(dw_mci_exynos_exit);
>
> MODULE_DESCRIPTION("Samsung Specific DW-MSHC Driver Extension");
> MODULE_AUTHOR("Thomas Abraham <[email protected]");
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 <[email protected]>
> ---
> Changes in v4: None
> Changes in v3: None
> Changes in v2:
> - Fix typo (some -> come)
> - Use ~0 instead of 0xFFFFFFFF; 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 <[email protected]>
Best regards,
Tomasz
On Tuesday 06 of August 2013 14:37:50 Doug Anderson wrote:
> After suspend/resume all of the dw_mmc registers are reset to
> defaults. We restore most of them, but specifically don't setup the
> clock registers after resume unless we've got a powered card. Things
> still work because the core will eventually call set_ios() and we'll
> set things up.
>
> There doesn't seem to be any reason that I can see _not_ to set things
> up after resume. Restoring this state makes the code easier to reason
> about and should help prevent bugs. It also allows us to do a
> register dump before and after suspend/resume to confirm that we've
> set things up OK.
>
> I examined the state of the dw_mmc instance before and after suspend
> after this patch. I had no card inserted in an SD card slot.
>
> Before this patch, differences were:
> * CLKDIV (0x08)
> * CLKENA (0x10)
> * TMOUT (0x14)
> * CMD (0x2C) - difference is not important
> * CLKSEL (0x9C - exynos specific)
>
> After this patch, only TMOUT was different. I have a separate patch
> for that.
>
> Signed-off-by: Doug Anderson <[email protected]>
> ---
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
> drivers/mmc/host/dw_mmc.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 13a363c..0fa3135 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2522,9 +2522,7 @@ int dw_mci_resume(struct dw_mci *host)
> 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_set_ios(slot->mmc, &slot->mmc->ios);
>
> ret = mmc_resume_host(host->slot[i]->mmc);
> if (ret < 0)
Reviewed-by: Tomasz Figa <[email protected]>
Best regards,
Tomasz
On Tuesday 06 of August 2013 14:37:51 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 clean.
>
> Signed-off-by: Doug Anderson <[email protected]>
> Acked-by: Seungwon Jeon <[email protected]>
> ---
> 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 0fa3135..1abcb36 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2505,6 +2505,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 |
Not sure if we really care about register dumps, but if it gets
initialized in probe as well, then this is fine.
Reviewed-by: Tomasz Figa <[email protected]>
Best regards,
Tomasz
Tomasz,
On Tue, Aug 6, 2013 at 2:58 PM, Tomasz Figa <[email protected]> wrote:
>> +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);
>
> What about clock gating? Will the clock used for clocking this register be
> always enabled when this gets called?
Since this is just accessing and writing a register in the "Mobile
Storage Host" block, I'd imagine that this should be the "biu" (bus
interface unit) clock, right? The dw_mmc code grabs the biu clock at
probe time and never lets it go. That means that we're OK as long as
common clock framework has already restored clocks to normal operation
by this time.
Do you think that common clock framework might not have put the clocks
back into order by the time "noirq" callbacks are executed?
-Doug
On Tuesday 06 of August 2013 15:09:46 Doug Anderson wrote:
> Tomasz,
>
> On Tue, Aug 6, 2013 at 2:58 PM, Tomasz Figa <[email protected]>
wrote:
> >> +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);
> >
> > What about clock gating? Will the clock used for clocking this
> > register be always enabled when this gets called?
>
> Since this is just accessing and writing a register in the "Mobile
> Storage Host" block, I'd imagine that this should be the "biu" (bus
> interface unit) clock, right? The dw_mmc code grabs the biu clock at
> probe time and never lets it go. That means that we're OK as long as
> common clock framework has already restored clocks to normal operation
> by this time.
>
> Do you think that common clock framework might not have put the clocks
> back into order by the time "noirq" callbacks are executed?
Ahh, so the dw_mmc driver doesn't do any clock gating? This is not very
nice of it.
Well, in this case your patch is OK, but possibly some clock gating will
have to be added to this driver at some point of time. Anyway:
Reviewed-by: Tomasz Figa <[email protected]>
Best regards,
Tomasz
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 <[email protected]>
> ---
> Changes in v4: None
> Changes in v3: None
> Changes in v2:
> - Fix typo (some -> come)
> - Use ~0 instead of 0xFFFFFFFF; 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);
>
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 <[email protected]>
> ---
> Changes in v4: None
> Changes in v3: None
> Changes in v2:
> - Fix typo (some -> come)
> - Use ~0 instead of 0xFFFFFFFF; 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, August 07, 2013, Doug Anderson wrote:
> 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 <[email protected]>
> ---
> 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 | 51 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
> index 866edef..0c1f192 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,30 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
> return 0;
> }
>
> +/**
> + * 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 and the bug
> + * may be more widespread than just exynos5420.
I guess just above comment can be removed. (Not be widespread)
Updating the origin value of CLKSEL looks like no harm while SDMMC_CLKSEL_WAKEUP_INT is cleared.
> + */
> +
> +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;
> +}
> +
> static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr)
> {
> /*
> @@ -187,17 +212,39 @@ static int dw_mci_exynos_probe(struct platform_device *pdev)
> return dw_mci_pltfm_register(pdev, drv_data);
> }
>
> +static struct dev_pm_ops dw_mci_exynos_pmops;
> +
> 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,
> },
> };
>
> -module_platform_driver(dw_mci_exynos_pltfm_driver);
> +static int __init dw_mci_exynos_init(void)
> +{
> + /* Add a "noirq" resume to platform pmops */
> + memcpy(&dw_mci_exynos_pmops, &dw_mci_pltfm_pmops,
> + sizeof(dw_mci_exynos_pmops));
> + WARN_ON(dw_mci_exynos_pmops.resume_noirq ||
> + dw_mci_exynos_pmops.thaw_noirq ||
> + dw_mci_exynos_pmops.restore_noirq);
> + dw_mci_exynos_pmops.resume_noirq = dw_mci_exynos_resume_noirq;
> + dw_mci_exynos_pmops.thaw_noirq = dw_mci_exynos_resume_noirq;
> + dw_mci_exynos_pmops.restore_noirq = dw_mci_exynos_resume_noirq;
If CONFIG_PM_SLEEP is not defined, we don't need to add it.
And also, instead of reusing dw_mci_pltfm_pmops, how about defining dw_mci_exynos_pmops's own?
Of course, suspend/resume will not different with dw_mci_pltfm* just now.
But specific code for exynos would be added soon.
Thanks,
Seungwon Jeon
On Wed, August 07, 2013, Doug Anderson wrote:
> After suspend/resume all of the dw_mmc registers are reset to
> defaults. We restore most of them, but specifically don't setup the
> clock registers after resume unless we've got a powered card. Things
> still work because the core will eventually call set_ios() and we'll
> set things up.
Hmm, I didn't get the need of this call during resume.
I think set_ios is only valid where core layer calls.
Besides, important things is ios's parameters.
If suspend has finished successfully, last call of set_ios() is from mmc_power_off().
On seeing fields of 'mmc->ios' stored last, these values aren't proper in resume phase.
Please check mmc_power_off() function.
In case MMC_PM_KEEP_POWER it could be kept.
Thanks,
Seungwon Jeon
>
> There doesn't seem to be any reason that I can see _not_ to set things
> up after resume. Restoring this state makes the code easier to reason
> about and should help prevent bugs. It also allows us to do a
> register dump before and after suspend/resume to confirm that we've
> set things up OK.
>
> I examined the state of the dw_mmc instance before and after suspend
> after this patch. I had no card inserted in an SD card slot.
>
> Before this patch, differences were:
> * CLKDIV (0x08)
> * CLKENA (0x10)
> * TMOUT (0x14)
> * CMD (0x2C) - difference is not important
> * CLKSEL (0x9C - exynos specific)
>
> After this patch, only TMOUT was different. I have a separate patch
> for that.
>
> Signed-off-by: Doug Anderson <[email protected]>
> ---
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
> drivers/mmc/host/dw_mmc.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 13a363c..0fa3135 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2522,9 +2522,7 @@ int dw_mci_resume(struct dw_mci *host)
> 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_set_ios(slot->mmc, &slot->mmc->ios);
>
> ret = mmc_resume_host(host->slot[i]->mmc);
> if (ret < 0)
> --
> 1.8.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Seungwon,
On Fri, Aug 9, 2013 at 6:33 AM, Seungwon Jeon <[email protected]> wrote:
> On Wed, August 07, 2013, Doug Anderson wrote:
>> 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 <[email protected]>
>> ---
>> 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 | 51 ++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 49 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
>> index 866edef..0c1f192 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,30 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
>> return 0;
>> }
>>
>> +/**
>> + * 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 and the bug
>> + * may be more widespread than just exynos5420.
> I guess just above comment can be removed. (Not be widespread)
> Updating the origin value of CLKSEL looks like no harm while SDMMC_CLKSEL_WAKEUP_INT is cleared.
OK, no problem. I'll clean up the comment next time revision.
>> -module_platform_driver(dw_mci_exynos_pltfm_driver);
>> +static int __init dw_mci_exynos_init(void)
>> +{
>> + /* Add a "noirq" resume to platform pmops */
>> + memcpy(&dw_mci_exynos_pmops, &dw_mci_pltfm_pmops,
>> + sizeof(dw_mci_exynos_pmops));
>> + WARN_ON(dw_mci_exynos_pmops.resume_noirq ||
>> + dw_mci_exynos_pmops.thaw_noirq ||
>> + dw_mci_exynos_pmops.restore_noirq);
>> + dw_mci_exynos_pmops.resume_noirq = dw_mci_exynos_resume_noirq;
>> + dw_mci_exynos_pmops.thaw_noirq = dw_mci_exynos_resume_noirq;
>> + dw_mci_exynos_pmops.restore_noirq = dw_mci_exynos_resume_noirq;
>
> If CONFIG_PM_SLEEP is not defined, we don't need to add it.
> And also, instead of reusing dw_mci_pltfm_pmops, how about defining dw_mci_exynos_pmops's own?
> Of course, suspend/resume will not different with dw_mci_pltfm* just now.
> But specific code for exynos would be added soon.
Whoops! ...of course this should be conditional on CONFIG_PM_SLEEP.
Thank you for catching.
I spent a bit of time debating whether I should make my own structure
or do a copy like this. It felt like a bit of a toss up to me, but
I'm happy to do it the other way. I will call dw_mci_suspend(host)
directly and assume hope that nobody adds any important code to
dw_mci_pltfm_suspend(). The other alternative would be make
dw_mci_pltfm_suspend() exported or call it indirectly through
dw_mci_pltfm_pmops, both of which seem slightly worse.
-Doug
Seungwon and Jaehoon,
On Fri, Aug 9, 2013 at 6:32 AM, Seungwon Jeon <[email protected]> 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
Seungwon,
On Fri, Aug 9, 2013 at 6:35 AM, Seungwon Jeon <[email protected]> wrote:
> On Wed, August 07, 2013, Doug Anderson wrote:
>> After suspend/resume all of the dw_mmc registers are reset to
>> defaults. We restore most of them, but specifically don't setup the
>> clock registers after resume unless we've got a powered card. Things
>> still work because the core will eventually call set_ios() and we'll
>> set things up.
>
> Hmm, I didn't get the need of this call during resume.
> I think set_ios is only valid where core layer calls.
> Besides, important things is ios's parameters.
> If suspend has finished successfully, last call of set_ios() is from mmc_power_off().
> On seeing fields of 'mmc->ios' stored last, these values aren't proper in resume phase.
> Please check mmc_power_off() function.
> In case MMC_PM_KEEP_POWER it could be kept.
Most of my reasoning has to do with the fact that the state of the
system after suspend/resume should not be significantly different than
the state of the system before suspend/resume. If the state of the
system is different in the two cases it points out potential problems
or inefficiencies.
To make this more concrete:
1. Boot up a system with no card in the SD Card slot.
2. Note down the value of registers like CLKDIV, CLKENA, etc.
3. Suspend / resume (S2R)
4. Check the values of CLKDIV, CLKENA, etc.
You will notice that they are different. This is a bad sign and can
be a source of bugs (though I don't know of any). ...or it could mean
that power draw is different (could be better, could be worse) after a
suspend/resume cycle.
Said another way, if the value of CLKDIV, CLKENA, etc is not important
when a card is not inserted, why do they get initialized at boot time?
In general, I think that the mmc core code makes the assumption that
it's up to the driver to make sure that its state is preserved across
S2R. For dw_mmc the driver doesn't do the "brute force" that some
drivers do of just saving and restoring all registers using a copy
loop. Instead, the dw_mmc driver runs code that tries to set the
state back to something reasonable. Without my patch the dw_mmc
driver doesn't run any code that restores these registers.
dw_mci_set_ios() will do so.
Another option would be to forcibly save/restore registers in suspend/resume.
-Doug
This series of patches addresses some suspend/resume problems with
dw_mmc on exynos platforms, espeically exynos5420. Since
suspend/resume is not fully working on ToT Linux (v3.11-rc4) on
exynos5250-snow, this series was tested against the current ToT
ChromeOS 3.8 tree. I have confirmed basic booting and eMMC / SD card
usage (and compiling, honest!) against ToT Linux.
I have received confirmation from Samsung that the problem solved is a
silicon errata on exynos5420 and that this is a good fix.
Changes in v5:
- Remove force_clkinit as per Jaehoon.
- Update commit message to (hopefully) be clearer.
- 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:
- Fix typo (some -> come)
- Use ~0 instead of 0xFFFFFFFF; add comment about value
- Use suspend_noirq as per James Hogan.
Doug Anderson (4):
mmc: dw_mmc: Invalidate cache of current_speed after suspend/resume
mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
mmc: dw_mmc: Always setup the bus after suspend/resume
mmc: dw_mmc: Set timeout to max upon resume
drivers/mmc/host/dw_mmc-exynos.c | 56 +++++++++++++++++++++++++++++++++++++++-
drivers/mmc/host/dw_mmc.c | 21 ++++++++++-----
2 files changed, 69 insertions(+), 8 deletions(-)
--
1.8.3
After suspend/resume all of the dw_mmc registers are reset to
defaults. We restore most of them, but specifically don't setup the
clock registers after resume unless we've got a powered card. Things
still work because the core will eventually call set_ios() and we'll
set things up.
There doesn't seem to be any reason that I can see _not_ to set things
up after resume. Restoring this state makes the code easier to reason
about and should help prevent bugs. It also allows us to do a
register dump before and after suspend/resume to confirm that we've
set things up OK.
I examined the state of the dw_mmc instance before and after suspend
after this patch. I had no card inserted in an SD card slot.
Before this patch, differences were:
* CLKDIV (0x08)
* CLKENA (0x10)
* TMOUT (0x14)
* CMD (0x2C) - difference is not important
* CLKSEL (0x9C - exynos specific)
After this patch, only TMOUT was different. I have a separate patch
for that.
Signed-off-by: Doug Anderson <[email protected]>
Reviewed-by: Tomasz Figa <[email protected]>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/mmc/host/dw_mmc.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index e614b03..3d1ee2d 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2522,9 +2522,7 @@ int dw_mci_resume(struct dw_mci *host)
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_set_ios(slot->mmc, &slot->mmc->ios);
ret = mmc_resume_host(host->slot[i]->mmc);
if (ret < 0)
--
1.8.3
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 clean.
Signed-off-by: Doug Anderson <[email protected]>
Acked-by: Seungwon Jeon <[email protected]>
Reviewed-by: Tomasz Figa <[email protected]>
---
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 3d1ee2d..bc9f22c 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2505,6 +2505,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
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 <[email protected]>
---
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 | 56 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 55 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index 866edef..7d88583 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,52 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
return 0;
}
+#ifdef CONFIG_PM_SLEEP
+/*
+ * TODO: we should probably disable the clock to the card in the suspend path.
+ */
+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 +234,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
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.
Specifically 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.
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.
Before this patch, many scenarios worked fine across suspend/resume
since the core mmc code ends up adjusting the clock quite a bit
during/suspend resume (and this ended up invalidating the cache for
us).
Signed-off-by: Doug Anderson <[email protected]>
Reviewed-by: Tomasz Figa <[email protected]>
---
Changes in v5:
- Remove force_clkinit as per Jaehoon
- Update commit message to (hopefully) be clearer
Changes in v4: None
Changes in v3: None
Changes in v2:
- Fix typo (some -> come)
- Use ~0 instead of 0xFFFFFFFF; add comment about value
drivers/mmc/host/dw_mmc.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index ee5f167..e614b03 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -629,13 +629,13 @@ static void mci_send_cmd(struct dw_mci_slot *slot, u32 cmd, u32 arg)
cmd, arg, cmd_status);
}
-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 != host->current_speed) {
div = host->bus_hz / slot->clock;
if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
/*
@@ -819,7 +819,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
drv_data->set_ios(slot->host, ios);
/* Slot specific timing and width adjustment */
- dw_mci_setup_bus(slot, false);
+ dw_mci_setup_bus(slot);
switch (ios->power_mode) {
case MMC_POWER_UP:
@@ -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
On Fri, Aug 9, 2013 at 1:33 PM, Doug Anderson <[email protected]> wrote:
> +#else
> +#define dw_mci_exynos_suspend NULL
> +#define dw_mci_exynos_resume NULL
> +#define dw_mci_exynos_resume_noirq NULL
> +#endif /* CONFIG_PM_SLEEP */
You could avoid this else block if you use 'static SIMPLE_DEV_PM_OPS' below.
> +
> static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr)
> {
> /*
> @@ -187,13 +234,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 SIMPLE_DEV_PM_OPS(dw_mci_exynos_pmops, dw_mci_exynos_suspend,
dw_mci_exynos_resume ) ;
Fabio,
Thanks for your review.
On Fri, Aug 9, 2013 at 9:41 AM, Fabio Estevam <[email protected]> wrote:
> On Fri, Aug 9, 2013 at 1:33 PM, Doug Anderson <[email protected]> wrote:
>
>> +#else
>> +#define dw_mci_exynos_suspend NULL
>> +#define dw_mci_exynos_resume NULL
>> +#define dw_mci_exynos_resume_noirq NULL
>> +#endif /* CONFIG_PM_SLEEP */
>
> You could avoid this else block if you use 'static SIMPLE_DEV_PM_OPS' below.
>
>> +
>> static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr)
>> {
>> /*
>> @@ -187,13 +234,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 SIMPLE_DEV_PM_OPS(dw_mci_exynos_pmops, dw_mci_exynos_suspend,
> dw_mci_exynos_resume ) ;
The big problem is that SIMPLE_DEV_PM_OPS() doesn't take a "_noirq"
parameters. Do you know of one that does?
-Doug
On Sat, August 10, 2013, Doug Anderson wrote:
> Seungwon and Jaehoon,
>
> On Fri, Aug 9, 2013 at 6:32 AM, Seungwon Jeon <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, August 10, 2013,Doug Anderson wrote:
> Seungwon,
>
> On Fri, Aug 9, 2013 at 6:35 AM, Seungwon Jeon <[email protected]> wrote:
> > On Wed, August 07, 2013, Doug Anderson wrote:
> >> After suspend/resume all of the dw_mmc registers are reset to
> >> defaults. We restore most of them, but specifically don't setup the
> >> clock registers after resume unless we've got a powered card. Things
> >> still work because the core will eventually call set_ios() and we'll
> >> set things up.
> >
> > Hmm, I didn't get the need of this call during resume.
> > I think set_ios is only valid where core layer calls.
> > Besides, important things is ios's parameters.
> > If suspend has finished successfully, last call of set_ios() is from mmc_power_off().
> > On seeing fields of 'mmc->ios' stored last, these values aren't proper in resume phase.
> > Please check mmc_power_off() function.
> > In case MMC_PM_KEEP_POWER it could be kept.
>
> Most of my reasoning has to do with the fact that the state of the
> system after suspend/resume should not be significantly different than
> the state of the system before suspend/resume. If the state of the
> system is different in the two cases it points out potential problems
> or inefficiencies.
>
> To make this more concrete:
>
> 1. Boot up a system with no card in the SD Card slot.
> 2. Note down the value of registers like CLKDIV, CLKENA, etc.
> 3. Suspend / resume (S2R)
> 4. Check the values of CLKDIV, CLKENA, etc.
>
> You will notice that they are different. This is a bad sign and can
> be a source of bugs (though I don't know of any). ...or it could mean
> that power draw is different (could be better, could be worse) after a
> suspend/resume cycle.
>
>
> Said another way, if the value of CLKDIV, CLKENA, etc is not important
> when a card is not inserted, why do they get initialized at boot time?
>
>
> In general, I think that the mmc core code makes the assumption that
> it's up to the driver to make sure that its state is preserved across
> S2R. For dw_mmc the driver doesn't do the "brute force" that some
> drivers do of just saving and restoring all registers using a copy
> loop. Instead, the dw_mmc driver runs code that tries to set the
> state back to something reasonable. Without my patch the dw_mmc
> driver doesn't run any code that restores these registers.
> dw_mci_set_ios() will do so.
This seems pretty associated with [1/4 patch]. (Anyway continued, ...)
Basically, both CLKDIV and CLKENA will be set with the reset value of zero. This means clock is disabled.
While resume of dw_mmc is completed, initial configuration registers will be set except for runtime registers.
I think registers related to clock are close to runtime.
Core layer knows the correct clock rate for current device mode and will actually request it by set_ios.
If core layer requests set_ios no more after dw_mmc resume is completed, dw_mmc will keep the clock to be disabled.
Then, dw_mmc doesn't need self call of dw_mci_set_ios.
Thanks,
Seungwon Jeon
>
> Another option would be to forcibly save/restore registers in suspend/resume.
>
> -Doug
Doug,
Looks good to me except for minor comment.
On Sat, August 10, 2013, Doug Anderson wrote:
> 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 <[email protected]>
> ---
> 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 | 56 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
> index 866edef..7d88583 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,52 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
> return 0;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +/*
> + * TODO: we should probably disable the clock to the card in the suspend path.
In suspend, clock is gated, isn't it?
Rather, no comment looks better, if intention is not clear.
Thanks,
Seungwon Jeon
> + */
> +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 +234,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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Doug,
Do you have any update for this series?
Please let me know.
Thanks,
Seungwon Jeon
On Sat, August 10, 2013, Doug Anderson wrote:
> This series of patches addresses some suspend/resume problems with
> dw_mmc on exynos platforms, espeically exynos5420. Since
> suspend/resume is not fully working on ToT Linux (v3.11-rc4) on
> exynos5250-snow, this series was tested against the current ToT
> ChromeOS 3.8 tree. I have confirmed basic booting and eMMC / SD card
> usage (and compiling, honest!) against ToT Linux.
>
> I have received confirmation from Samsung that the problem solved is a
> silicon errata on exynos5420 and that this is a good fix.
>
> Changes in v5:
> - Remove force_clkinit as per Jaehoon.
> - Update commit message to (hopefully) be clearer.
> - 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:
> - Fix typo (some -> come)
> - Use ~0 instead of 0xFFFFFFFF; add comment about value
> - Use suspend_noirq as per James Hogan.
>
> Doug Anderson (4):
> mmc: dw_mmc: Invalidate cache of current_speed after suspend/resume
> mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
> mmc: dw_mmc: Always setup the bus after suspend/resume
> mmc: dw_mmc: Set timeout to max upon resume
>
> drivers/mmc/host/dw_mmc-exynos.c | 56 +++++++++++++++++++++++++++++++++++++++-
> drivers/mmc/host/dw_mmc.c | 21 ++++++++++-----
> 2 files changed, 69 insertions(+), 8 deletions(-)
>
> --
> 1.8.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Seungwon,
On Wed, Aug 21, 2013 at 4:48 AM, Seungwon Jeon <[email protected]> wrote:
> Hi Doug,
> Do you have any update for this series?
> Please let me know.
Thank you for the ping. The changes requested looked big enough that
I knew I was going to have to devote some time to looking this all
over again, which I haven't had time for. I'll make time for it today
or tomorrow and repost. Sorry for the delay.
-Doug
Seungwon,
On Mon, Aug 12, 2013 at 12:14 AM, Seungwon Jeon <[email protected]> wrote:
> On Sat, August 10, 2013, Doug Anderson wrote:
>> Seungwon and Jaehoon,
>>
>> On Fri, Aug 9, 2013 at 6:32 AM, Seungwon Jeon <[email protected]> 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
Seungwon,
On Wed, Aug 21, 2013 at 5:54 PM, Doug Anderson <[email protected]> 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