linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos
@ 2013-07-09 17:31 Doug Anderson
  2013-07-09 17:31 ` [PATCH 3/5] mmc: dw_mmc: Add exynos resume callback to clear WAKEUP_INT Doug Anderson
  2013-07-09 23:19 ` [PATCH v2 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
  0 siblings, 2 replies; 28+ messages in thread
From: Doug Anderson @ 2013-07-09 17:31 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 3/5] mmc: dw_mmc: Add exynos resume callback to clear WAKEUP_INT
  2013-07-09 17:31 [PATCH 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
@ 2013-07-09 17:31 ` Doug Anderson
  2013-07-09 19:09   ` Doug Anderson
  2013-07-09 23:19 ` [PATCH v2 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
  1 sibling, 1 reply; 28+ messages in thread
From: Doug Anderson @ 2013-07-09 17:31 UTC (permalink / raw)
  To: linux-arm-kernel

If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up
looping around forever.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 3/5] mmc: dw_mmc: Add exynos resume callback to clear WAKEUP_INT
  2013-07-09 17:31 ` [PATCH 3/5] mmc: dw_mmc: Add exynos resume callback to clear WAKEUP_INT Doug Anderson
@ 2013-07-09 19:09   ` Doug Anderson
  2013-07-11  0:43     ` Grant Grundler
  0 siblings, 1 reply; 28+ messages in thread
From: Doug Anderson @ 2013-07-09 19:09 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

On Tue, Jul 9, 2013 at 10:31 AM, Doug Anderson <dianders@chromium.org> wrote:
> If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up
> looping around forever.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos
  2013-07-09 17:31 [PATCH 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
  2013-07-09 17:31 ` [PATCH 3/5] mmc: dw_mmc: Add exynos resume callback to clear WAKEUP_INT Doug Anderson
@ 2013-07-09 23:19 ` Doug Anderson
  2013-07-09 23:19   ` [PATCH v2 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
  2013-07-10 15:42   ` [PATCH v3 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
  1 sibling, 2 replies; 28+ messages in thread
From: Doug Anderson @ 2013-07-09 23:19 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-07-09 23:19 ` [PATCH v2 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
@ 2013-07-09 23:19   ` Doug Anderson
  2013-07-10 14:54     ` Seungwon Jeon
  2013-07-10 15:42   ` [PATCH v3 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
  1 sibling, 1 reply; 28+ messages in thread
From: Doug Anderson @ 2013-07-09 23:19 UTC (permalink / raw)
  To: linux-arm-kernel

If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up
looping around forever.  This has been seen to happen on exynos5420
silicon despite the fact that we haven't enabled any wakeup events.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v2 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-07-09 23:19   ` [PATCH v2 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
@ 2013-07-10 14:54     ` Seungwon Jeon
  2013-07-10 15:05       ` Doug Anderson
  0 siblings, 1 reply; 28+ messages in thread
From: Seungwon Jeon @ 2013-07-10 14:54 UTC (permalink / raw)
  To: linux-arm-kernel

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 <dianders@chromium.org>
> ---
> 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 majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-07-10 14:54     ` Seungwon Jeon
@ 2013-07-10 15:05       ` Doug Anderson
  2013-07-15 12:09         ` Seungwon Jeon
  0 siblings, 1 reply; 28+ messages in thread
From: Doug Anderson @ 2013-07-10 15:05 UTC (permalink / raw)
  To: linux-arm-kernel

Seungwon,

On Wed, Jul 10, 2013 at 7:54 AM, Seungwon Jeon <tgih.jun@samsung.com> 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 <dianders@chromium.org>
>> ---
>> 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos
  2013-07-09 23:19 ` [PATCH v2 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
  2013-07-09 23:19   ` [PATCH v2 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
@ 2013-07-10 15:42   ` Doug Anderson
  2013-07-10 15:42     ` [PATCH v3 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
  2013-08-06 21:37     ` [PATCH v4 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
  1 sibling, 2 replies; 28+ messages in thread
From: Doug Anderson @ 2013-07-10 15:42 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-07-10 15:42   ` [PATCH v3 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
@ 2013-07-10 15:42     ` Doug Anderson
  2013-07-16  1:36       ` Jaehoon Chung
  2013-08-06 21:37     ` [PATCH v4 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
  1 sibling, 1 reply; 28+ messages in thread
From: Doug Anderson @ 2013-07-10 15:42 UTC (permalink / raw)
  To: linux-arm-kernel

If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up
looping around forever.  This has been seen to happen on exynos5420
silicon despite the fact that we haven't enabled any wakeup events.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 3/5] mmc: dw_mmc: Add exynos resume callback to clear WAKEUP_INT
  2013-07-09 19:09   ` Doug Anderson
@ 2013-07-11  0:43     ` Grant Grundler
  0 siblings, 0 replies; 28+ messages in thread
From: Grant Grundler @ 2013-07-11  0:43 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Jul 9, 2013 at 12:09 PM, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Tue, Jul 9, 2013 at 10:31 AM, Doug Anderson <dianders@chromium.org> wrote:
>> If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up
>> looping around forever.
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> ---
>>  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 <grundler@chromium.org>

thanks,
grant

>
> -Doug

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-07-10 15:05       ` Doug Anderson
@ 2013-07-15 12:09         ` Seungwon Jeon
  2013-07-31 16:18           ` Doug Anderson
  0 siblings, 1 reply; 28+ messages in thread
From: Seungwon Jeon @ 2013-07-15 12:09 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, July 11, 2013, Doug Anderson wrote:
> Seungwon,
> 
> On Wed, Jul 10, 2013 at 7:54 AM, Seungwon Jeon <tgih.jun@samsung.com> 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 <dianders@chromium.org>
> >> ---
> >> 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 majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-07-10 15:42     ` [PATCH v3 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
@ 2013-07-16  1:36       ` Jaehoon Chung
  0 siblings, 0 replies; 28+ messages in thread
From: Jaehoon Chung @ 2013-07-16  1:36 UTC (permalink / raw)
  To: linux-arm-kernel

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 <dianders@chromium.org>
> ---
> 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,
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-07-15 12:09         ` Seungwon Jeon
@ 2013-07-31 16:18           ` Doug Anderson
  2013-08-06 21:36             ` Doug Anderson
  0 siblings, 1 reply; 28+ messages in thread
From: Doug Anderson @ 2013-07-31 16:18 UTC (permalink / raw)
  To: linux-arm-kernel

Seungwon,

On Mon, Jul 15, 2013 at 5:09 AM, Seungwon Jeon <tgih.jun@samsung.com> 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-07-31 16:18           ` Doug Anderson
@ 2013-08-06 21:36             ` Doug Anderson
  0 siblings, 0 replies; 28+ messages in thread
From: Doug Anderson @ 2013-08-06 21:36 UTC (permalink / raw)
  To: linux-arm-kernel

Seungwon,

On Wed, Jul 31, 2013 at 9:18 AM, Doug Anderson <dianders@chromium.org> wrote:
> Seungwon,
>
> On Mon, Jul 15, 2013 at 5:09 AM, Seungwon Jeon <tgih.jun@samsung.com> 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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v4 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos
  2013-07-10 15:42   ` [PATCH v3 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
  2013-07-10 15:42     ` [PATCH v3 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
@ 2013-08-06 21:37     ` Doug Anderson
  2013-08-06 21:37       ` [PATCH v4 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
  2013-08-09 16:33       ` [PATCH v5 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
  1 sibling, 2 replies; 28+ messages in thread
From: Doug Anderson @ 2013-08-06 21:37 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v4 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-08-06 21:37     ` [PATCH v4 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
@ 2013-08-06 21:37       ` Doug Anderson
  2013-08-06 21:58         ` Tomasz Figa
  2013-08-09 13:33         ` Seungwon Jeon
  2013-08-09 16:33       ` [PATCH v5 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
  1 sibling, 2 replies; 28+ messages in thread
From: Doug Anderson @ 2013-08-06 21:37 UTC (permalink / raw)
  To: linux-arm-kernel

If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up
looping around forever.  This has been seen to happen on exynos5420
silicon despite the fact that we haven't enabled any wakeup events due
to a silicon errata.  It is safe to do on all exynos variants.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in 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 <thomas.ab@samsung.com");
-- 
1.8.3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-08-06 21:37       ` [PATCH v4 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
@ 2013-08-06 21:58         ` Tomasz Figa
  2013-08-06 22:09           ` Doug Anderson
  2013-08-09 13:33         ` Seungwon Jeon
  1 sibling, 1 reply; 28+ messages in thread
From: Tomasz Figa @ 2013-08-06 21:58 UTC (permalink / raw)
  To: linux-arm-kernel

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 <dianders@chromium.org>
> ---
> 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 <thomas.ab@samsung.com");

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v4 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-08-06 21:58         ` Tomasz Figa
@ 2013-08-06 22:09           ` Doug Anderson
  2013-08-06 22:20             ` Tomasz Figa
  0 siblings, 1 reply; 28+ messages in thread
From: Doug Anderson @ 2013-08-06 22:09 UTC (permalink / raw)
  To: linux-arm-kernel

Tomasz,

On Tue, Aug 6, 2013 at 2:58 PM, Tomasz Figa <tomasz.figa@gmail.com> 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v4 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-08-06 22:09           ` Doug Anderson
@ 2013-08-06 22:20             ` Tomasz Figa
  0 siblings, 0 replies; 28+ messages in thread
From: Tomasz Figa @ 2013-08-06 22:20 UTC (permalink / raw)
  To: linux-arm-kernel

On Tuesday 06 of August 2013 15:09:46 Doug Anderson wrote:
> Tomasz,
> 
> On Tue, Aug 6, 2013 at 2:58 PM, Tomasz Figa <tomasz.figa@gmail.com> 
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 <t.figa@samsung.com>

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v4 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-08-06 21:37       ` [PATCH v4 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
  2013-08-06 21:58         ` Tomasz Figa
@ 2013-08-09 13:33         ` Seungwon Jeon
  2013-08-09 15:05           ` Doug Anderson
  1 sibling, 1 reply; 28+ messages in thread
From: Seungwon Jeon @ 2013-08-09 13:33 UTC (permalink / raw)
  To: linux-arm-kernel

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 <dianders@chromium.org>
> ---
> 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v4 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-08-09 13:33         ` Seungwon Jeon
@ 2013-08-09 15:05           ` Doug Anderson
  0 siblings, 0 replies; 28+ messages in thread
From: Doug Anderson @ 2013-08-09 15:05 UTC (permalink / raw)
  To: linux-arm-kernel

Seungwon,

On Fri, Aug 9, 2013 at 6:33 AM, Seungwon Jeon <tgih.jun@samsung.com> 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 <dianders@chromium.org>
>> ---
>> 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v5 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos
  2013-08-06 21:37     ` [PATCH v4 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
  2013-08-06 21:37       ` [PATCH v4 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
@ 2013-08-09 16:33       ` Doug Anderson
  2013-08-09 16:33         ` [PATCH v5 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
  2013-08-21 11:48         ` [PATCH v5 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Seungwon Jeon
  1 sibling, 2 replies; 28+ messages in thread
From: Doug Anderson @ 2013-08-09 16:33 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v5 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-08-09 16:33       ` [PATCH v5 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
@ 2013-08-09 16:33         ` Doug Anderson
  2013-08-09 16:41           ` Fabio Estevam
  2013-08-12  7:21           ` Seungwon Jeon
  2013-08-21 11:48         ` [PATCH v5 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Seungwon Jeon
  1 sibling, 2 replies; 28+ messages in thread
From: Doug Anderson @ 2013-08-09 16:33 UTC (permalink / raw)
  To: linux-arm-kernel

If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up
looping around forever.  This has been seen to happen on exynos5420
silicon despite the fact that we haven't enabled any wakeup events due
to a silicon errata.  It is safe to do on all exynos variants.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in 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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v5 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-08-09 16:33         ` [PATCH v5 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
@ 2013-08-09 16:41           ` Fabio Estevam
  2013-08-09 16:48             ` Doug Anderson
  2013-08-12  7:21           ` Seungwon Jeon
  1 sibling, 1 reply; 28+ messages in thread
From: Fabio Estevam @ 2013-08-09 16:41 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Aug 9, 2013 at 1:33 PM, Doug Anderson <dianders@chromium.org> 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 ) ;

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v5 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-08-09 16:41           ` Fabio Estevam
@ 2013-08-09 16:48             ` Doug Anderson
  0 siblings, 0 replies; 28+ messages in thread
From: Doug Anderson @ 2013-08-09 16:48 UTC (permalink / raw)
  To: linux-arm-kernel

Fabio,

Thanks for your review.

On Fri, Aug 9, 2013 at 9:41 AM, Fabio Estevam <festevam@gmail.com> wrote:
> On Fri, Aug 9, 2013 at 1:33 PM, Doug Anderson <dianders@chromium.org> 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v5 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
  2013-08-09 16:33         ` [PATCH v5 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
  2013-08-09 16:41           ` Fabio Estevam
@ 2013-08-12  7:21           ` Seungwon Jeon
  1 sibling, 0 replies; 28+ messages in thread
From: Seungwon Jeon @ 2013-08-12  7:21 UTC (permalink / raw)
  To: linux-arm-kernel

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 <dianders@chromium.org>
> ---
> 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 majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v5 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos
  2013-08-09 16:33       ` [PATCH v5 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
  2013-08-09 16:33         ` [PATCH v5 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
@ 2013-08-21 11:48         ` Seungwon Jeon
  2013-08-21 15:13           ` Doug Anderson
  1 sibling, 1 reply; 28+ messages in thread
From: Seungwon Jeon @ 2013-08-21 11:48 UTC (permalink / raw)
  To: linux-arm-kernel

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 majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v5 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos
  2013-08-21 11:48         ` [PATCH v5 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Seungwon Jeon
@ 2013-08-21 15:13           ` Doug Anderson
  0 siblings, 0 replies; 28+ messages in thread
From: Doug Anderson @ 2013-08-21 15:13 UTC (permalink / raw)
  To: linux-arm-kernel

Seungwon,


On Wed, Aug 21, 2013 at 4:48 AM, Seungwon Jeon <tgih.jun@samsung.com> 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2013-08-21 15:13 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-09 17:31 [PATCH 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
2013-07-09 17:31 ` [PATCH 3/5] mmc: dw_mmc: Add exynos resume callback to clear WAKEUP_INT Doug Anderson
2013-07-09 19:09   ` Doug Anderson
2013-07-11  0:43     ` Grant Grundler
2013-07-09 23:19 ` [PATCH v2 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
2013-07-09 23:19   ` [PATCH v2 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
2013-07-10 14:54     ` Seungwon Jeon
2013-07-10 15:05       ` Doug Anderson
2013-07-15 12:09         ` Seungwon Jeon
2013-07-31 16:18           ` Doug Anderson
2013-08-06 21:36             ` Doug Anderson
2013-07-10 15:42   ` [PATCH v3 0/5] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
2013-07-10 15:42     ` [PATCH v3 3/5] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
2013-07-16  1:36       ` Jaehoon Chung
2013-08-06 21:37     ` [PATCH v4 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
2013-08-06 21:37       ` [PATCH v4 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
2013-08-06 21:58         ` Tomasz Figa
2013-08-06 22:09           ` Doug Anderson
2013-08-06 22:20             ` Tomasz Figa
2013-08-09 13:33         ` Seungwon Jeon
2013-08-09 15:05           ` Doug Anderson
2013-08-09 16:33       ` [PATCH v5 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Doug Anderson
2013-08-09 16:33         ` [PATCH v5 2/4] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT Doug Anderson
2013-08-09 16:41           ` Fabio Estevam
2013-08-09 16:48             ` Doug Anderson
2013-08-12  7:21           ` Seungwon Jeon
2013-08-21 11:48         ` [PATCH v5 0/4] mmc: dw_mmc: fixes for suspend/resume on exynos Seungwon Jeon
2013-08-21 15:13           ` Doug Anderson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).