[PATCH v2] mmc: sdhci-of-esdhc: add workaround for clock glitch issue

[PATCH v2] mmc: sdhci-of-esdhc: add workaround for clock glitch issue

[Yangbo Lu]

A-003980: SDHC: Glitch is generated on the card clock with software reset
or clock divider change
Description: A glitch may occur on the SDHC card clock when the software
sets the RSTA bit (software reset) in the system control register. It can
also be generated by setting the clock divider value. The glitch produced
can cause the external card to switch to an unknown state. The occurrence
is not deterministic.

Workaround:
A simple workaround is to disable the SD card clock before the software
reset, and enable it when the module resumes normal operation. The Host
and the SD card are in a master-slave relationship. The Host provides
clock and control transfer across the interface. Therefore, any existing
operation is discarded when the Host controller is reset.
The recommended flow is as follows:
1. Software disable bit[3], SDCLKEN, of the System Control Register
2. Trigger software reset and/or set clock divider
3. Check bit[3], SDSTB, of the Present State Register for stable clock
4. Enable bit[3], SDCLKEN, of the System Control Register
Using the above method, the eSDHC cannot send command or transfer data
when there is a glitch in the clock line, and the glitch does not cause
any issue.

Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
---
Changes for v2
	- Used sdhci_ops reset instead of platform_reset_enter/out's work
	- Changed commit message
---
 drivers/mmc/host/sdhci-esdhc.h    |  4 ++
 drivers/mmc/host/sdhci-of-esdhc.c | 90 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h
index 3497cfa..ede49f5 100644
--- a/drivers/mmc/host/sdhci-esdhc.h
+++ b/drivers/mmc/host/sdhci-esdhc.h
@@ -27,10 +27,14 @@
 #define ESDHC_CLOCK_MASK	0x0000fff0
 #define ESDHC_PREDIV_SHIFT	8
 #define ESDHC_DIVIDER_SHIFT	4
+#define ESDHC_CLOCK_CRDEN	0x00000008
 #define ESDHC_CLOCK_PEREN	0x00000004
 #define ESDHC_CLOCK_HCKEN	0x00000002
 #define ESDHC_CLOCK_IPGEN	0x00000001
 
+#define ESDHCI_PRESENT_STATE	0x24
+#define ESDHC_CLK_STABLE	0x00000008
+
 /* pltfm-specific */
 #define ESDHC_HOST_CONTROL_LE	0x20
 
diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 22e9111..db78b75 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -21,9 +21,16 @@
 #include <linux/mmc/host.h>
 #include "sdhci-pltfm.h"
 #include "sdhci-esdhc.h"
+#ifdef CONFIG_PPC
+#include <asm/mpc85xx.h>
+#endif
 
 #define VENDOR_V_22	0x12
 #define VENDOR_V_23	0x13
+
+#ifdef CONFIG_PPC
+static u32 svr;
+#endif
 static u32 esdhc_readl(struct sdhci_host *host, int reg)
 {
 	u32 ret;
@@ -202,6 +209,7 @@ static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock)
 	int pre_div = 2;
 	int div = 1;
 	u32 temp;
+	u32 timeout;
 
 	host->mmc->actual_clock = 0;
 
@@ -218,7 +226,7 @@ static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	temp = sdhci_readl(host, ESDHC_SYSTEM_CONTROL);
 	temp &= ~(ESDHC_CLOCK_IPGEN | ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN
-		| ESDHC_CLOCK_MASK);
+		| ESDHC_CLOCK_MASK | ESDHC_CLOCK_CRDEN);
 	sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
 
 	while (host->max_clk / pre_div / 16 > clock && pre_div < 256)
@@ -238,7 +246,21 @@ static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock)
 		| (div << ESDHC_DIVIDER_SHIFT)
 		| (pre_div << ESDHC_PREDIV_SHIFT));
 	sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
-	mdelay(1);
+
+	/* Wait max 20 ms */
+	timeout = 20;
+	while (!(sdhci_readl(host, ESDHCI_PRESENT_STATE) & ESDHC_CLK_STABLE)) {
+		if (timeout == 0) {
+			pr_err("%s: Internal clock never stabilised.\n",
+				mmc_hostname(host->mmc));
+			return;
+		}
+		timeout--;
+		mdelay(1);
+	}
+
+	temp |= ESDHC_CLOCK_CRDEN;
+	sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
 }
 
 static void esdhc_of_platform_init(struct sdhci_host *host)
@@ -276,9 +298,71 @@ static void esdhc_pltfm_set_bus_width(struct sdhci_host *host, int width)
 			ESDHC_CTRL_BUSWIDTH_MASK, ctrl);
 }
 
+/*
+ * A-003980: SDHC: Glitch is generated on the card clock with software reset
+ * or clock divider change
+ * Workaround:
+ * A simple workaround is to disable the SD card clock before the software
+ * reset, and enable it when the module resumes normal operation. The Host
+ * and the SD card are in a master-slave relationship. The Host provides
+ * clock and control transfer across the interface. Therefore, any existing
+ * operation is discarded when the Host controller is reset.
+ */
+static int esdhc_of_reset_workaround(struct sdhci_host *host, u8 mask)
+{
+	u16 clk;
+	bool disable_clk_before_reset = false;
+
+#ifdef CONFIG_PPC
+	svr =  mfspr(SPRN_SVR);
+	/*
+	 * Check for A-003980
+	 * Impact list:
+	 * T4240-4160-R1.0 B4860-4420-R1.0-R2.0 P3041-R1.0-R1.1-R2.0
+	 * P2041-2040-R1.0-R1.1-R2.0 P1010-1014-R1.0 P5020-5010-R1.0-R2.0
+	 * P5040-5021-R1.0-R2.0-R2.1
+	 */
+	if (((SVR_SOC_VER(svr) == SVR_T4240) && (SVR_REV(svr) == 0x10)) ||
+	    ((SVR_SOC_VER(svr) == SVR_T4240) && (SVR_REV(svr) == 0x20)) ||
+	    ((SVR_SOC_VER(svr) == SVR_T4160) && (SVR_REV(svr) == 0x10)) ||
+	    ((SVR_SOC_VER(svr) == SVR_T4160) && (SVR_REV(svr) == 0x20)) ||
+	    ((SVR_SOC_VER(svr) == SVR_B4860) && (SVR_REV(svr) == 0x10)) ||
+	    ((SVR_SOC_VER(svr) == SVR_B4860) && (SVR_REV(svr) == 0x20)) ||
+	    ((SVR_SOC_VER(svr) == SVR_B4420) && (SVR_REV(svr) == 0x10)) ||
+	    ((SVR_SOC_VER(svr) == SVR_B4420) && (SVR_REV(svr) == 0x20)) ||
+	    ((SVR_SOC_VER(svr) == SVR_P5040) && (SVR_REV(svr) <= 0x21)) ||
+	    ((SVR_SOC_VER(svr) == SVR_P5020) && (SVR_REV(svr) <= 0x20)) ||
+	    ((SVR_SOC_VER(svr) == SVR_P5021) && (SVR_REV(svr) <= 0x21)) ||
+	    ((SVR_SOC_VER(svr) == SVR_P5010) && (SVR_REV(svr) <= 0x20)) ||
+	    ((SVR_SOC_VER(svr) == SVR_P3041) && (SVR_REV(svr) <= 0x20)) ||
+	    ((SVR_SOC_VER(svr) == SVR_P2041) && (SVR_REV(svr) <= 0x20)) ||
+	    ((SVR_SOC_VER(svr) == SVR_P2040) && (SVR_REV(svr) <= 0x20)) ||
+	    ((SVR_SOC_VER(svr) == SVR_P1014) && (SVR_REV(svr) == 0x10)) ||
+	    ((SVR_SOC_VER(svr) == SVR_P1010) && (SVR_REV(svr) == 0x10)))
+		disable_clk_before_reset = true;
+#endif
+
+	if (disable_clk_before_reset && (mask & SDHCI_RESET_ALL)) {
+		clk = esdhc_readw(host, SDHCI_CLOCK_CONTROL);
+		clk &= ~ESDHC_CLOCK_CRDEN;
+		esdhc_writew(host, clk, SDHCI_CLOCK_CONTROL);
+		sdhci_reset(host, mask);
+		clk = esdhc_readw(host, SDHCI_CLOCK_CONTROL);
+		clk |= ESDHC_CLOCK_CRDEN;
+		esdhc_writew(host, clk, SDHCI_CLOCK_CONTROL);
+		return 1;
+	}
+	return 0;
+}
+
 static void esdhc_reset(struct sdhci_host *host, u8 mask)
 {
-	sdhci_reset(host, mask);
+	int ret;
+
+	ret = esdhc_of_reset_workaround(host, mask);
+
+	if (!ret)
+		sdhci_reset(host, mask);
 
 	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
 	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
-- 
2.1.0.27.g96db324

Re: [PATCH v2] mmc: sdhci-of-esdhc: add workaround for clock glitch issue

[Ulf Hansson]

On 19 May 2015 at 07:40, Yangbo Lu <yangbo.lu@freescale.com> wrote:
> A-003980: SDHC: Glitch is generated on the card clock with software reset
> or clock divider change
> Description: A glitch may occur on the SDHC card clock when the software
> sets the RSTA bit (software reset) in the system control register. It can
> also be generated by setting the clock divider value. The glitch produced
> can cause the external card to switch to an unknown state. The occurrence
> is not deterministic.
>
> Workaround:
> A simple workaround is to disable the SD card clock before the software
> reset, and enable it when the module resumes normal operation. The Host
> and the SD card are in a master-slave relationship. The Host provides
> clock and control transfer across the interface. Therefore, any existing
> operation is discarded when the Host controller is reset.
> The recommended flow is as follows:
> 1. Software disable bit[3], SDCLKEN, of the System Control Register
> 2. Trigger software reset and/or set clock divider
> 3. Check bit[3], SDSTB, of the Present State Register for stable clock
> 4. Enable bit[3], SDCLKEN, of the System Control Register
> Using the above method, the eSDHC cannot send command or transfer data
> when there is a glitch in the clock line, and the glitch does not cause
> any issue.
>
> Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
> ---
> Changes for v2
>         - Used sdhci_ops reset instead of platform_reset_enter/out's work
>         - Changed commit message
> ---
>  drivers/mmc/host/sdhci-esdhc.h    |  4 ++
>  drivers/mmc/host/sdhci-of-esdhc.c | 90 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 91 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h
> index 3497cfa..ede49f5 100644
> --- a/drivers/mmc/host/sdhci-esdhc.h
> +++ b/drivers/mmc/host/sdhci-esdhc.h
> @@ -27,10 +27,14 @@
>  #define ESDHC_CLOCK_MASK       0x0000fff0
>  #define ESDHC_PREDIV_SHIFT     8
>  #define ESDHC_DIVIDER_SHIFT    4
> +#define ESDHC_CLOCK_CRDEN      0x00000008
>  #define ESDHC_CLOCK_PEREN      0x00000004
>  #define ESDHC_CLOCK_HCKEN      0x00000002
>  #define ESDHC_CLOCK_IPGEN      0x00000001
>
> +#define ESDHCI_PRESENT_STATE   0x24
> +#define ESDHC_CLK_STABLE       0x00000008
> +
>  /* pltfm-specific */
>  #define ESDHC_HOST_CONTROL_LE  0x20
>
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
> index 22e9111..db78b75 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -21,9 +21,16 @@
>  #include <linux/mmc/host.h>
>  #include "sdhci-pltfm.h"
>  #include "sdhci-esdhc.h"
> +#ifdef CONFIG_PPC
> +#include <asm/mpc85xx.h>

As Jaehoon already pointed out, we don't want to include machine
specific headers in drivers. Please find another solution to this.

Kind regards
Uffe

> +#endif
>
>  #define VENDOR_V_22    0x12
>  #define VENDOR_V_23    0x13
> +
> +#ifdef CONFIG_PPC
> +static u32 svr;
> +#endif
>  static u32 esdhc_readl(struct sdhci_host *host, int reg)
>  {
>         u32 ret;
> @@ -202,6 +209,7 @@ static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock)
>         int pre_div = 2;
>         int div = 1;
>         u32 temp;
> +       u32 timeout;
>
>         host->mmc->actual_clock = 0;
>
> @@ -218,7 +226,7 @@ static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock)
>
>         temp = sdhci_readl(host, ESDHC_SYSTEM_CONTROL);
>         temp &= ~(ESDHC_CLOCK_IPGEN | ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN
> -               | ESDHC_CLOCK_MASK);
> +               | ESDHC_CLOCK_MASK | ESDHC_CLOCK_CRDEN);
>         sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
>
>         while (host->max_clk / pre_div / 16 > clock && pre_div < 256)
> @@ -238,7 +246,21 @@ static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock)
>                 | (div << ESDHC_DIVIDER_SHIFT)
>                 | (pre_div << ESDHC_PREDIV_SHIFT));
>         sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
> -       mdelay(1);
> +
> +       /* Wait max 20 ms */
> +       timeout = 20;
> +       while (!(sdhci_readl(host, ESDHCI_PRESENT_STATE) & ESDHC_CLK_STABLE)) {
> +               if (timeout == 0) {
> +                       pr_err("%s: Internal clock never stabilised.\n",
> +                               mmc_hostname(host->mmc));
> +                       return;
> +               }
> +               timeout--;
> +               mdelay(1);
> +       }
> +
> +       temp |= ESDHC_CLOCK_CRDEN;
> +       sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
>  }
>
>  static void esdhc_of_platform_init(struct sdhci_host *host)
> @@ -276,9 +298,71 @@ static void esdhc_pltfm_set_bus_width(struct sdhci_host *host, int width)
>                         ESDHC_CTRL_BUSWIDTH_MASK, ctrl);
>  }
>
> +/*
> + * A-003980: SDHC: Glitch is generated on the card clock with software reset
> + * or clock divider change
> + * Workaround:
> + * A simple workaround is to disable the SD card clock before the software
> + * reset, and enable it when the module resumes normal operation. The Host
> + * and the SD card are in a master-slave relationship. The Host provides
> + * clock and control transfer across the interface. Therefore, any existing
> + * operation is discarded when the Host controller is reset.
> + */
> +static int esdhc_of_reset_workaround(struct sdhci_host *host, u8 mask)
> +{
> +       u16 clk;
> +       bool disable_clk_before_reset = false;
> +
> +#ifdef CONFIG_PPC
> +       svr =  mfspr(SPRN_SVR);
> +       /*
> +        * Check for A-003980
> +        * Impact list:
> +        * T4240-4160-R1.0 B4860-4420-R1.0-R2.0 P3041-R1.0-R1.1-R2.0
> +        * P2041-2040-R1.0-R1.1-R2.0 P1010-1014-R1.0 P5020-5010-R1.0-R2.0
> +        * P5040-5021-R1.0-R2.0-R2.1
> +        */
> +       if (((SVR_SOC_VER(svr) == SVR_T4240) && (SVR_REV(svr) == 0x10)) ||
> +           ((SVR_SOC_VER(svr) == SVR_T4240) && (SVR_REV(svr) == 0x20)) ||
> +           ((SVR_SOC_VER(svr) == SVR_T4160) && (SVR_REV(svr) == 0x10)) ||
> +           ((SVR_SOC_VER(svr) == SVR_T4160) && (SVR_REV(svr) == 0x20)) ||
> +           ((SVR_SOC_VER(svr) == SVR_B4860) && (SVR_REV(svr) == 0x10)) ||
> +           ((SVR_SOC_VER(svr) == SVR_B4860) && (SVR_REV(svr) == 0x20)) ||
> +           ((SVR_SOC_VER(svr) == SVR_B4420) && (SVR_REV(svr) == 0x10)) ||
> +           ((SVR_SOC_VER(svr) == SVR_B4420) && (SVR_REV(svr) == 0x20)) ||
> +           ((SVR_SOC_VER(svr) == SVR_P5040) && (SVR_REV(svr) <= 0x21)) ||
> +           ((SVR_SOC_VER(svr) == SVR_P5020) && (SVR_REV(svr) <= 0x20)) ||
> +           ((SVR_SOC_VER(svr) == SVR_P5021) && (SVR_REV(svr) <= 0x21)) ||
> +           ((SVR_SOC_VER(svr) == SVR_P5010) && (SVR_REV(svr) <= 0x20)) ||
> +           ((SVR_SOC_VER(svr) == SVR_P3041) && (SVR_REV(svr) <= 0x20)) ||
> +           ((SVR_SOC_VER(svr) == SVR_P2041) && (SVR_REV(svr) <= 0x20)) ||
> +           ((SVR_SOC_VER(svr) == SVR_P2040) && (SVR_REV(svr) <= 0x20)) ||
> +           ((SVR_SOC_VER(svr) == SVR_P1014) && (SVR_REV(svr) == 0x10)) ||
> +           ((SVR_SOC_VER(svr) == SVR_P1010) && (SVR_REV(svr) == 0x10)))
> +               disable_clk_before_reset = true;
> +#endif
> +
> +       if (disable_clk_before_reset && (mask & SDHCI_RESET_ALL)) {
> +               clk = esdhc_readw(host, SDHCI_CLOCK_CONTROL);
> +               clk &= ~ESDHC_CLOCK_CRDEN;
> +               esdhc_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +               sdhci_reset(host, mask);
> +               clk = esdhc_readw(host, SDHCI_CLOCK_CONTROL);
> +               clk |= ESDHC_CLOCK_CRDEN;
> +               esdhc_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +               return 1;
> +       }
> +       return 0;
> +}
> +
>  static void esdhc_reset(struct sdhci_host *host, u8 mask)
>  {
> -       sdhci_reset(host, mask);
> +       int ret;
> +
> +       ret = esdhc_of_reset_workaround(host, mask);
> +
> +       if (!ret)
> +               sdhci_reset(host, mask);
>
>         sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
>         sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
> --
> 2.1.0.27.g96db324
>

Re: [PATCH v2] mmc: sdhci-of-esdhc: add workaround for clock glitch issue

[Ulf Hansson]

On 19 May 2015 at 10:59, Lu Y.B. <yangbo.lu@freescale.com> wrote:
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: Tuesday, May 19, 2015 4:37 PM
>> To: Lu Yangbo-B47093
>> Cc: linux-mmc; Chris Ball; Jaehoon Chung
>> Subject: Re: [PATCH v2] mmc: sdhci-of-esdhc: add workaround for clock
>> glitch issue
>>
>> On 19 May 2015 at 07:40, Yangbo Lu <yangbo.lu@freescale.com> wrote:
>> > A-003980: SDHC: Glitch is generated on the card clock with software
>> > reset or clock divider change
>> > Description: A glitch may occur on the SDHC card clock when the
>> > software sets the RSTA bit (software reset) in the system control
>> > register. It can also be generated by setting the clock divider value.
>> > The glitch produced can cause the external card to switch to an
>> > unknown state. The occurrence is not deterministic.
>> >
>> > Workaround:
>> > A simple workaround is to disable the SD card clock before the
>> > software reset, and enable it when the module resumes normal
>> > operation. The Host and the SD card are in a master-slave
>> > relationship. The Host provides clock and control transfer across the
>> > interface. Therefore, any existing operation is discarded when the Host
>> controller is reset.
>> > The recommended flow is as follows:
>> > 1. Software disable bit[3], SDCLKEN, of the System Control Register 2.
>> > Trigger software reset and/or set clock divider 3. Check bit[3],
>> > SDSTB, of the Present State Register for stable clock 4. Enable
>> > bit[3], SDCLKEN, of the System Control Register Using the above
>> > method, the eSDHC cannot send command or transfer data when there is a
>> > glitch in the clock line, and the glitch does not cause any issue.
>> >
>> > Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
>> > ---
>> > Changes for v2
>> >         - Used sdhci_ops reset instead of platform_reset_enter/out's
>> work
>> >         - Changed commit message
>> > ---
>> >  drivers/mmc/host/sdhci-esdhc.h    |  4 ++
>> >  drivers/mmc/host/sdhci-of-esdhc.c | 90
>> > +++++++++++++++++++++++++++++++++++++--
>> >  2 files changed, 91 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/mmc/host/sdhci-esdhc.h
>> > b/drivers/mmc/host/sdhci-esdhc.h index 3497cfa..ede49f5 100644
>> > --- a/drivers/mmc/host/sdhci-esdhc.h
>> > +++ b/drivers/mmc/host/sdhci-esdhc.h
>> > @@ -27,10 +27,14 @@
>> >  #define ESDHC_CLOCK_MASK       0x0000fff0
>> >  #define ESDHC_PREDIV_SHIFT     8
>> >  #define ESDHC_DIVIDER_SHIFT    4
>> > +#define ESDHC_CLOCK_CRDEN      0x00000008
>> >  #define ESDHC_CLOCK_PEREN      0x00000004
>> >  #define ESDHC_CLOCK_HCKEN      0x00000002
>> >  #define ESDHC_CLOCK_IPGEN      0x00000001
>> >
>> > +#define ESDHCI_PRESENT_STATE   0x24
>> > +#define ESDHC_CLK_STABLE       0x00000008
>> > +
>> >  /* pltfm-specific */
>> >  #define ESDHC_HOST_CONTROL_LE  0x20
>> >
>> > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
>> > b/drivers/mmc/host/sdhci-of-esdhc.c
>> > index 22e9111..db78b75 100644
>> > --- a/drivers/mmc/host/sdhci-of-esdhc.c
>> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
>> > @@ -21,9 +21,16 @@
>> >  #include <linux/mmc/host.h>
>> >  #include "sdhci-pltfm.h"
>> >  #include "sdhci-esdhc.h"
>> > +#ifdef CONFIG_PPC
>> > +#include <asm/mpc85xx.h>
>>
>> As Jaehoon already pointed out, we don't want to include machine specific
>> headers in drivers. Please find another solution to this.
>>
> Do you mean I should include this .h file in sdhci-esdhc.h, or all the PPC-specific code in this patch is not good?

I assume this header is included to identify the version of the SoC?

Normally we do such things through DT via compatible strings. Earlier
that was done via platform callbacks and prior that as how you
suggested it in $subject patch.

If you have the possibility to enumerate the device ID (reading device
information directly from the HW), you should do that when adding the
device to its subsystem level bus, not while probing. That's because
you need to be able to match for a compatible driver.

Kind regards
Uffe

Re: [PATCH v2] mmc: sdhci-of-esdhc: add workaround for clock glitch issue

[Ulf Hansson]

On 26 May 2015 at 10:43, Lu Y.B. <yangbo.lu@freescale.com> wrote:
>> >> On 19 May 2015 at 07:40, Yangbo Lu <yangbo.lu@freescale.com> wrote:
>> >> > A-003980: SDHC: Glitch is generated on the card clock with software
>> >> > reset or clock divider change
>> >> > Description: A glitch may occur on the SDHC card clock when the
>> >> > software sets the RSTA bit (software reset) in the system control
>> >> > register. It can also be generated by setting the clock divider
>> value.
>> >> > The glitch produced can cause the external card to switch to an
>> >> > unknown state. The occurrence is not deterministic.
>> >> >
>> >> > Workaround:
>> >> > A simple workaround is to disable the SD card clock before the
>> >> > software reset, and enable it when the module resumes normal
>> >> > operation. The Host and the SD card are in a master-slave
>> >> > relationship. The Host provides clock and control transfer across
>> >> > the interface. Therefore, any existing operation is discarded when
>> >> > the Host
>> >> controller is reset.
>> >> > The recommended flow is as follows:
>> >> > 1. Software disable bit[3], SDCLKEN, of the System Control Register
>> 2.
>> >> > Trigger software reset and/or set clock divider 3. Check bit[3],
>> >> > SDSTB, of the Present State Register for stable clock 4. Enable
>> >> > bit[3], SDCLKEN, of the System Control Register Using the above
>> >> > method, the eSDHC cannot send command or transfer data when there
>> >> > is a glitch in the clock line, and the glitch does not cause any
>> issue.
>> >> >
>> >> > Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
>> >> > ---
>> >> > Changes for v2
>> >> >         - Used sdhci_ops reset instead of
>> >> > platform_reset_enter/out's
>> >> work
>> >> >         - Changed commit message
>> >> > ---
>> >> >  drivers/mmc/host/sdhci-esdhc.h    |  4 ++
>> >> >  drivers/mmc/host/sdhci-of-esdhc.c | 90
>> >> > +++++++++++++++++++++++++++++++++++++--
>> >> >  2 files changed, 91 insertions(+), 3 deletions(-)
>> >> >
>> >> > diff --git a/drivers/mmc/host/sdhci-esdhc.h
>> >> > b/drivers/mmc/host/sdhci-esdhc.h index 3497cfa..ede49f5 100644
>> >> > --- a/drivers/mmc/host/sdhci-esdhc.h
>> >> > +++ b/drivers/mmc/host/sdhci-esdhc.h
>> >> > @@ -27,10 +27,14 @@
>> >> >  #define ESDHC_CLOCK_MASK       0x0000fff0
>> >> >  #define ESDHC_PREDIV_SHIFT     8
>> >> >  #define ESDHC_DIVIDER_SHIFT    4
>> >> > +#define ESDHC_CLOCK_CRDEN      0x00000008
>> >> >  #define ESDHC_CLOCK_PEREN      0x00000004
>> >> >  #define ESDHC_CLOCK_HCKEN      0x00000002
>> >> >  #define ESDHC_CLOCK_IPGEN      0x00000001
>> >> >
>> >> > +#define ESDHCI_PRESENT_STATE   0x24
>> >> > +#define ESDHC_CLK_STABLE       0x00000008
>> >> > +
>> >> >  /* pltfm-specific */
>> >> >  #define ESDHC_HOST_CONTROL_LE  0x20
>> >> >
>> >> > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
>> >> > b/drivers/mmc/host/sdhci-of-esdhc.c
>> >> > index 22e9111..db78b75 100644
>> >> > --- a/drivers/mmc/host/sdhci-of-esdhc.c
>> >> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
>> >> > @@ -21,9 +21,16 @@
>> >> >  #include <linux/mmc/host.h>
>> >> >  #include "sdhci-pltfm.h"
>> >> >  #include "sdhci-esdhc.h"
>> >> > +#ifdef CONFIG_PPC
>> >> > +#include <asm/mpc85xx.h>
>> >>
>> >> As Jaehoon already pointed out, we don't want to include machine
>> >> specific headers in drivers. Please find another solution to this.
>> >>
>> > Do you mean I should include this .h file in sdhci-esdhc.h, or all the
>> PPC-specific code in this patch is not good?
>>
>> I assume this header is included to identify the version of the SoC?
> Yes! Actually the sdhci-of-esdhc.c is only used for PowerPC presently.
>>
>> Normally we do such things through DT via compatible strings. Earlier
>> that was done via platform callbacks and prior that as how you suggested
>> it in $subject patch.
> The DTS is not a proper method since this would add more dts files just for different silicon rev.

We do that already, but I understand your point.

>>
>> If you have the possibility to enumerate the device ID (reading device
>> information directly from the HW), you should do that when adding the
>> device to its subsystem level bus, not while probing. That's because you
>> need to be able to match for a compatible driver.
> Thanks. Could you provide a reference in present code if you know? I'd like to try.

I guess the are several subsystem to look into, but maybe AMBA is one
that you could have a look at.

drivers/amba/bus.c

Kind regards
Uffe

Re: [PATCH v2] mmc: sdhci-of-esdhc: add workaround for clock glitch issue

[Ulf Hansson]

>> >> If you have the possibility to enumerate the device ID (reading
>> >> device information directly from the HW), you should do that when
>> >> adding the device to its subsystem level bus, not while probing.
>> >> That's because you need to be able to match for a compatible driver.
>> > Thanks. Could you provide a reference in present code if you know? I'd
>> like to try.
>>
>> I guess the are several subsystem to look into, but maybe AMBA is one
>> that you could have a look at.
>>
>> drivers/amba/bus.c
>>
> Thank you, Uffe.
> I have seen the code of drivers/amba/ and drivers/of/.
> Do you suggest to get device information from HW when adding device in drivers/of ?
> I am afraid it still faces the same machine specific problem and is complicated.

No matter what you need to move things out of the machine specific
directory, if you want other drivers/buses to be able to make use of
it.

Complicated? Maybe, but I don't find that as good argument for not doing it. :-)

>
> I think it is not so unreasonable to get QorIQ silicon rev in sdhci-of-esdhc.c, because this file is only used for freescale QorIQ platform.

Sorry, I will not accept patches that include machine specific
headers. We can do better than that.

Kind regards
Uffe