[PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR

[PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR

[Benoît Thébaudeau]

The eSDHC can only DMA from 32-bit-aligned addresses.

This fixes the following test cases of mmc_test:
  11:	Badly aligned write
  12:	Badly aligned read
  13:	Badly aligned multi-block write
  14:	Badly aligned multi-block read

Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>
---
 drivers/mmc/host/sdhci-esdhc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h
index c4bbd74..e7893f2 100644
--- a/drivers/mmc/host/sdhci-esdhc.h
+++ b/drivers/mmc/host/sdhci-esdhc.h
@@ -19,6 +19,7 @@
  */
 
 #define ESDHC_DEFAULT_QUIRKS	(SDHCI_QUIRK_FORCE_BLK_SZ_2048 | \
+				SDHCI_QUIRK_32BIT_DMA_ADDR | \
 				SDHCI_QUIRK_NO_BUSY_IRQ | \
 				SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK | \
 				SDHCI_QUIRK_PIO_NEEDS_DELAY | \
-- 
2.7.4

[PATCH 2/4] mmc: sdhci-esdhc-imx: Fix DAT line software reset

[Benoît Thébaudeau]

On i.MX25, the eSDHC DAT line software reset (SYSCTL.RSTD) unexpectedly
clears at least the data transfer width (PROCTL.DTW), which then results
in data CRC errors. This behavior is not documented, but it has actually
been observed. Consequently, the DAT line software resets triggered by
sdhci.c in case of errors caused unrecoverable errors.

Fix this by making sure that the DAT line software reset does not alter
the Host Control register. This behavior being undocumented, it may also
be present on other i.MX SoCs, so apply this fix for the whole i.MX
family.

Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 59 ++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 23d8b8a..4ee82e1 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -579,7 +579,7 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
-	u32 new_val;
+	u32 new_val = 0;
 	u32 mask;
 
 	switch (reg) {
@@ -610,29 +610,46 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
 
 		esdhc_clrset_le(host, mask, new_val, reg);
 		return;
+	case SDHCI_SOFTWARE_RESET:
+		if (val & SDHCI_RESET_DATA)
+			new_val = readl(host->ioaddr + SDHCI_HOST_CONTROL);
+		break;
 	}
 	esdhc_clrset_le(host, 0xff, val, reg);
 
-	/*
-	 * The esdhc has a design violation to SDHC spec which tells
-	 * that software reset should not affect card detection circuit.
-	 * But esdhc clears its SYSCTL register bits [0..2] during the
-	 * software reset.  This will stop those clocks that card detection
-	 * circuit relies on.  To work around it, we turn the clocks on back
-	 * to keep card detection circuit functional.
-	 */
-	if ((reg == SDHCI_SOFTWARE_RESET) && (val & 1)) {
-		esdhc_clrset_le(host, 0x7, 0x7, ESDHC_SYSTEM_CONTROL);
-		/*
-		 * The reset on usdhc fails to clear MIX_CTRL register.
-		 * Do it manually here.
-		 */
-		if (esdhc_is_usdhc(imx_data)) {
-			/* the tuning bits should be kept during reset */
-			new_val = readl(host->ioaddr + ESDHC_MIX_CTRL);
-			writel(new_val & ESDHC_MIX_CTRL_TUNING_MASK,
-					host->ioaddr + ESDHC_MIX_CTRL);
-			imx_data->is_ddr = 0;
+	if (reg == SDHCI_SOFTWARE_RESET) {
+		if (val & SDHCI_RESET_ALL) {
+			/*
+			 * The esdhc has a design violation to SDHC spec which
+			 * tells that software reset should not affect card
+			 * detection circuit. But esdhc clears its SYSCTL
+			 * register bits [0..2] during the software reset. This
+			 * will stop those clocks that card detection circuit
+			 * relies on. To work around it, we turn the clocks on
+			 * back to keep card detection circuit functional.
+			 */
+			esdhc_clrset_le(host, 0x7, 0x7, ESDHC_SYSTEM_CONTROL);
+			/*
+			 * The reset on usdhc fails to clear MIX_CTRL register.
+			 * Do it manually here.
+			 */
+			if (esdhc_is_usdhc(imx_data)) {
+				/*
+				 * the tuning bits should be kept during reset
+				 */
+				new_val = readl(host->ioaddr + ESDHC_MIX_CTRL);
+				writel(new_val & ESDHC_MIX_CTRL_TUNING_MASK,
+						host->ioaddr + ESDHC_MIX_CTRL);
+				imx_data->is_ddr = 0;
+			}
+		} else if (val & SDHCI_RESET_DATA) {
+			/*
+			 * The eSDHC DAT line software reset clears at least the
+			 * data transfer width on i.MX25, so make sure that the
+			 * Host Control register is unaffected.
+			 */
+			esdhc_clrset_le(host, 0xff, new_val,
+					SDHCI_HOST_CONTROL);
 		}
 	}
 }
-- 
2.7.4

Re: [PATCH 2/4] mmc: sdhci-esdhc-imx: Fix DAT line software reset

[Adrian Hunter]

On 03/05/17 13:05, Benoît Thébaudeau wrote:
> On i.MX25, the eSDHC DAT line software reset (SYSCTL.RSTD) unexpectedly
> clears at least the data transfer width (PROCTL.DTW), which then results
> in data CRC errors. This behavior is not documented, but it has actually
> been observed. Consequently, the DAT line software resets triggered by
> sdhci.c in case of errors caused unrecoverable errors.
> 
> Fix this by making sure that the DAT line software reset does not alter
> the Host Control register. This behavior being undocumented, it may also
> be present on other i.MX SoCs, so apply this fix for the whole i.MX
> family.
> 
> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>

I would expect to see Acks from other sdhci-esdhc-imx users.  Nevertheless,
for sdhci:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 59 ++++++++++++++++++++++++--------------
>  1 file changed, 38 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 23d8b8a..4ee82e1 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -579,7 +579,7 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
> -	u32 new_val;
> +	u32 new_val = 0;
>  	u32 mask;
>  
>  	switch (reg) {
> @@ -610,29 +610,46 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
>  
>  		esdhc_clrset_le(host, mask, new_val, reg);
>  		return;
> +	case SDHCI_SOFTWARE_RESET:
> +		if (val & SDHCI_RESET_DATA)
> +			new_val = readl(host->ioaddr + SDHCI_HOST_CONTROL);
> +		break;
>  	}
>  	esdhc_clrset_le(host, 0xff, val, reg);
>  
> -	/*
> -	 * The esdhc has a design violation to SDHC spec which tells
> -	 * that software reset should not affect card detection circuit.
> -	 * But esdhc clears its SYSCTL register bits [0..2] during the
> -	 * software reset.  This will stop those clocks that card detection
> -	 * circuit relies on.  To work around it, we turn the clocks on back
> -	 * to keep card detection circuit functional.
> -	 */
> -	if ((reg == SDHCI_SOFTWARE_RESET) && (val & 1)) {
> -		esdhc_clrset_le(host, 0x7, 0x7, ESDHC_SYSTEM_CONTROL);
> -		/*
> -		 * The reset on usdhc fails to clear MIX_CTRL register.
> -		 * Do it manually here.
> -		 */
> -		if (esdhc_is_usdhc(imx_data)) {
> -			/* the tuning bits should be kept during reset */
> -			new_val = readl(host->ioaddr + ESDHC_MIX_CTRL);
> -			writel(new_val & ESDHC_MIX_CTRL_TUNING_MASK,
> -					host->ioaddr + ESDHC_MIX_CTRL);
> -			imx_data->is_ddr = 0;
> +	if (reg == SDHCI_SOFTWARE_RESET) {
> +		if (val & SDHCI_RESET_ALL) {
> +			/*
> +			 * The esdhc has a design violation to SDHC spec which
> +			 * tells that software reset should not affect card
> +			 * detection circuit. But esdhc clears its SYSCTL
> +			 * register bits [0..2] during the software reset. This
> +			 * will stop those clocks that card detection circuit
> +			 * relies on. To work around it, we turn the clocks on
> +			 * back to keep card detection circuit functional.
> +			 */
> +			esdhc_clrset_le(host, 0x7, 0x7, ESDHC_SYSTEM_CONTROL);
> +			/*
> +			 * The reset on usdhc fails to clear MIX_CTRL register.
> +			 * Do it manually here.
> +			 */
> +			if (esdhc_is_usdhc(imx_data)) {
> +				/*
> +				 * the tuning bits should be kept during reset
> +				 */
> +				new_val = readl(host->ioaddr + ESDHC_MIX_CTRL);
> +				writel(new_val & ESDHC_MIX_CTRL_TUNING_MASK,
> +						host->ioaddr + ESDHC_MIX_CTRL);
> +				imx_data->is_ddr = 0;
> +			}
> +		} else if (val & SDHCI_RESET_DATA) {
> +			/*
> +			 * The eSDHC DAT line software reset clears at least the
> +			 * data transfer width on i.MX25, so make sure that the
> +			 * Host Control register is unaffected.
> +			 */
> +			esdhc_clrset_le(host, 0xff, new_val,
> +					SDHCI_HOST_CONTROL);
>  		}
>  	}
>  }
> 

Re: [PATCH 2/4] mmc: sdhci-esdhc-imx: Fix DAT line software reset

[Fabio Estevam]

On Wed, May 3, 2017 at 7:05 AM, Benoît Thébaudeau <benoit@wsystem.com> wrote:
> On i.MX25, the eSDHC DAT line software reset (SYSCTL.RSTD) unexpectedly
> clears at least the data transfer width (PROCTL.DTW), which then results
> in data CRC errors. This behavior is not documented, but it has actually
> been observed. Consequently, the DAT line software resets triggered by
> sdhci.c in case of errors caused unrecoverable errors.
>
> Fix this by making sure that the DAT line software reset does not alter
> the Host Control register. This behavior being undocumented, it may also
> be present on other i.MX SoCs, so apply this fix for the whole i.MX
> family.
>
> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

[PATCH 3/4] mmc: sdhci-esdhc-imx: Allow all supported prescaler values

[Benoît Thébaudeau]

On i.MX, SYSCTL.SDCLKFS may always be set to 0 in order to make the SD
clock frequency prescaler divide by 1 in SDR mode, even with the eSDHC.
The previous minimum prescaler value of 2 in SDR mode with the eSDHC was
a code remnant from PowerPC, which actually has this limitation on
earlier revisions.

In DDR mode, the prescaler can divide by up to 512.

The maximum SD clock frequency in High Speed mode is 50 MHz. On i.MX25,
this change makes it possible to get 48 MHz from the USB PLL
(240 MHz / 5 / 1) instead of only 40 MHz from the USB PLL
(240 MHz / 3 / 2) or 33.25 MHz from the AHB clock (133 MHz / 2 / 2).

Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 4ee82e1..fa60d13 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -674,7 +674,8 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
 	unsigned int host_clock = pltfm_host->clock;
-	int pre_div = 2;
+	int ddr_pre_div = imx_data->is_ddr ? 2 : 1;
+	int pre_div = 1;
 	int div = 1;
 	u32 temp, val;
 
@@ -689,28 +690,23 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
 		return;
 	}
 
-	if (esdhc_is_usdhc(imx_data) && !imx_data->is_ddr)
-		pre_div = 1;
-
 	temp = sdhci_readl(host, ESDHC_SYSTEM_CONTROL);
 	temp &= ~(ESDHC_CLOCK_IPGEN | ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN
 		| ESDHC_CLOCK_MASK);
 	sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
 
-	while (host_clock / pre_div / 16 > clock && pre_div < 256)
+	while (host_clock / (16 * pre_div * ddr_pre_div) > clock &&
+			pre_div < 256)
 		pre_div *= 2;
 
-	while (host_clock / pre_div / div > clock && div < 16)
+	while (host_clock / (div * pre_div * ddr_pre_div) > clock && div < 16)
 		div++;
 
-	host->mmc->actual_clock = host_clock / pre_div / div;
+	host->mmc->actual_clock = host_clock / (div * pre_div * ddr_pre_div);
 	dev_dbg(mmc_dev(host->mmc), "desired SD clock: %d, actual: %d\n",
 		clock, host->mmc->actual_clock);
 
-	if (imx_data->is_ddr)
-		pre_div >>= 2;
-	else
-		pre_div >>= 1;
+	pre_div >>= 1;
 	div--;
 
 	temp = sdhci_readl(host, ESDHC_SYSTEM_CONTROL);
-- 
2.7.4

Re: [PATCH 3/4] mmc: sdhci-esdhc-imx: Allow all supported prescaler values

[Adrian Hunter]

On 03/05/17 13:05, Benoît Thébaudeau wrote:
> On i.MX, SYSCTL.SDCLKFS may always be set to 0 in order to make the SD
> clock frequency prescaler divide by 1 in SDR mode, even with the eSDHC.
> The previous minimum prescaler value of 2 in SDR mode with the eSDHC was
> a code remnant from PowerPC, which actually has this limitation on
> earlier revisions.
> 
> In DDR mode, the prescaler can divide by up to 512.
> 
> The maximum SD clock frequency in High Speed mode is 50 MHz. On i.MX25,
> this change makes it possible to get 48 MHz from the USB PLL
> (240 MHz / 5 / 1) instead of only 40 MHz from the USB PLL
> (240 MHz / 3 / 2) or 33.25 MHz from the AHB clock (133 MHz / 2 / 2).
> 
> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>

I would expect to see Acks from other sdhci-esdhc-imx users.  Nevertheless,
for sdhci:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 4ee82e1..fa60d13 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -674,7 +674,8 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
>  	unsigned int host_clock = pltfm_host->clock;
> -	int pre_div = 2;
> +	int ddr_pre_div = imx_data->is_ddr ? 2 : 1;
> +	int pre_div = 1;
>  	int div = 1;
>  	u32 temp, val;
>  
> @@ -689,28 +690,23 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
>  		return;
>  	}
>  
> -	if (esdhc_is_usdhc(imx_data) && !imx_data->is_ddr)
> -		pre_div = 1;
> -
>  	temp = sdhci_readl(host, ESDHC_SYSTEM_CONTROL);
>  	temp &= ~(ESDHC_CLOCK_IPGEN | ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN
>  		| ESDHC_CLOCK_MASK);
>  	sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
>  
> -	while (host_clock / pre_div / 16 > clock && pre_div < 256)
> +	while (host_clock / (16 * pre_div * ddr_pre_div) > clock &&
> +			pre_div < 256)
>  		pre_div *= 2;
>  
> -	while (host_clock / pre_div / div > clock && div < 16)
> +	while (host_clock / (div * pre_div * ddr_pre_div) > clock && div < 16)
>  		div++;
>  
> -	host->mmc->actual_clock = host_clock / pre_div / div;
> +	host->mmc->actual_clock = host_clock / (div * pre_div * ddr_pre_div);
>  	dev_dbg(mmc_dev(host->mmc), "desired SD clock: %d, actual: %d\n",
>  		clock, host->mmc->actual_clock);
>  
> -	if (imx_data->is_ddr)
> -		pre_div >>= 2;
> -	else
> -		pre_div >>= 1;
> +	pre_div >>= 1;
>  	div--;
>  
>  	temp = sdhci_readl(host, ESDHC_SYSTEM_CONTROL);
> 

Re: [PATCH 3/4] mmc: sdhci-esdhc-imx: Allow all supported prescaler values

[Fabio Estevam]

On Wed, May 3, 2017 at 7:05 AM, Benoît Thébaudeau <benoit@wsystem.com> wrote:
> On i.MX, SYSCTL.SDCLKFS may always be set to 0 in order to make the SD
> clock frequency prescaler divide by 1 in SDR mode, even with the eSDHC.
> The previous minimum prescaler value of 2 in SDR mode with the eSDHC was
> a code remnant from PowerPC, which actually has this limitation on
> earlier revisions.
>
> In DDR mode, the prescaler can divide by up to 512.
>
> The maximum SD clock frequency in High Speed mode is 50 MHz. On i.MX25,
> this change makes it possible to get 48 MHz from the USB PLL
> (240 MHz / 5 / 1) instead of only 40 MHz from the USB PLL
> (240 MHz / 3 / 2) or 33.25 MHz from the AHB clock (133 MHz / 2 / 2).
>
> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

[PATCH 4/4] mmc: sdhci-esdhc-imx: Remove the ENGcm07207 workaround

[Benoît Thébaudeau]

The SDHCI_QUIRK_NO_MULTIBLOCK quirk was used as a workaround for the
ENGcm07207 erratum. However, it caused excruciatingly slow SD transfers
(300 kB/s on average), and this erratum actually does not imply that
multiple-block transfers are not supported, so this was overkill.

The suggested workaround for this erratum is to set SYSCTL.RSTA, but the
simple DAT line software reset (which resets the DMA circuit among
others) triggered by sdhci_finish_data() in case of errors seems to be
sufficient. Indeed, generating errors in a controlled manner on i.MX25
using the FEVT register right in the middle of read data transfers
without this quirk shows that nothing is written to the buffer by the
eSDHC past CMD12, and no extra Auto CMD12 is sent with AC12EN set, so
the data transfers on AHB are properly aborted. For write data
transfers, neither extra data nor extra Auto CMD12 is sent, as expected.
Moreover, after intensive stress tests on i.MX25, removing
SDHCI_QUIRK_NO_MULTIBLOCK seems to be safe.

Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index fa60d13..868a51f 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -115,11 +115,6 @@
  */
 #define ESDHC_FLAG_MULTIBLK_NO_INT	BIT(1)
 /*
- * The flag enables the workaround for ESDHC errata ENGcm07207 which
- * affects i.MX25 and i.MX35.
- */
-#define ESDHC_FLAG_ENGCM07207		BIT(2)
-/*
  * The flag tells that the ESDHC controller is an USDHC block that is
  * integrated on the i.MX6 series.
  */
@@ -149,11 +144,11 @@ struct esdhc_soc_data {
 };
 
 static struct esdhc_soc_data esdhc_imx25_data = {
-	.flags = ESDHC_FLAG_ENGCM07207,
+	.flags = ESDHC_FLAG_ERR004536,
 };
 
 static struct esdhc_soc_data esdhc_imx35_data = {
-	.flags = ESDHC_FLAG_ENGCM07207,
+	.flags = ESDHC_FLAG_ERR004536,
 };
 
 static struct esdhc_soc_data esdhc_imx51_data = {
@@ -286,7 +281,7 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
 		 * ADMA2 capability of esdhc, but this bit is messed up on
 		 * some SOCs (e.g. on MX25, MX35 this bit is set, but they
 		 * don't actually support ADMA2). So set the BROKEN_ADMA
-		 * uirk on MX25/35 platforms.
+		 * quirk on MX25/35 platforms.
 		 */
 
 		if (val & SDHCI_CAN_DO_ADMA1) {
@@ -1278,11 +1273,6 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
 	if (IS_ERR(imx_data->pins_default))
 		dev_warn(mmc_dev(host->mmc), "could not get default state\n");
 
-	if (imx_data->socdata->flags & ESDHC_FLAG_ENGCM07207)
-		/* Fix errata ENGcm07207 present on i.MX25 and i.MX35 */
-		host->quirks |= SDHCI_QUIRK_NO_MULTIBLOCK
-			| SDHCI_QUIRK_BROKEN_ADMA;
-
 	if (esdhc_is_usdhc(imx_data)) {
 		host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
 		host->mmc->caps |= MMC_CAP_1_8V_DDR;
-- 
2.7.4

Re: [PATCH 4/4] mmc: sdhci-esdhc-imx: Remove the ENGcm07207 workaround

[Adrian Hunter]

On 03/05/17 13:05, Benoît Thébaudeau wrote:
> The SDHCI_QUIRK_NO_MULTIBLOCK quirk was used as a workaround for the
> ENGcm07207 erratum. However, it caused excruciatingly slow SD transfers
> (300 kB/s on average), and this erratum actually does not imply that
> multiple-block transfers are not supported, so this was overkill.
> 
> The suggested workaround for this erratum is to set SYSCTL.RSTA, but the
> simple DAT line software reset (which resets the DMA circuit among
> others) triggered by sdhci_finish_data() in case of errors seems to be
> sufficient. Indeed, generating errors in a controlled manner on i.MX25
> using the FEVT register right in the middle of read data transfers
> without this quirk shows that nothing is written to the buffer by the
> eSDHC past CMD12, and no extra Auto CMD12 is sent with AC12EN set, so
> the data transfers on AHB are properly aborted. For write data
> transfers, neither extra data nor extra Auto CMD12 is sent, as expected.
> Moreover, after intensive stress tests on i.MX25, removing
> SDHCI_QUIRK_NO_MULTIBLOCK seems to be safe.
> 
> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>

Aside from one comment below...

I would expect to see Acks from other sdhci-esdhc-imx users.  Nevertheless,
for sdhci:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>


> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index fa60d13..868a51f 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -115,11 +115,6 @@
>   */
>  #define ESDHC_FLAG_MULTIBLK_NO_INT	BIT(1)
>  /*
> - * The flag enables the workaround for ESDHC errata ENGcm07207 which
> - * affects i.MX25 and i.MX35.
> - */
> -#define ESDHC_FLAG_ENGCM07207		BIT(2)
> -/*
>   * The flag tells that the ESDHC controller is an USDHC block that is
>   * integrated on the i.MX6 series.
>   */
> @@ -149,11 +144,11 @@ struct esdhc_soc_data {
>  };
>  
>  static struct esdhc_soc_data esdhc_imx25_data = {
> -	.flags = ESDHC_FLAG_ENGCM07207,
> +	.flags = ESDHC_FLAG_ERR004536,
>  };
>  
>  static struct esdhc_soc_data esdhc_imx35_data = {
> -	.flags = ESDHC_FLAG_ENGCM07207,
> +	.flags = ESDHC_FLAG_ERR004536,
>  };
>  
>  static struct esdhc_soc_data esdhc_imx51_data = {
> @@ -286,7 +281,7 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
>  		 * ADMA2 capability of esdhc, but this bit is messed up on
>  		 * some SOCs (e.g. on MX25, MX35 this bit is set, but they
>  		 * don't actually support ADMA2). So set the BROKEN_ADMA
> -		 * uirk on MX25/35 platforms.
> +		 * quirk on MX25/35 platforms.

Please do spelling fixes as a separate patch.  Note there are several
spelling errors in comments in drivers/mmc/host/sdhci-esdhc-imx.c so you
could easily do a few of them to make a separate patch more worthwhile.

>  		 */
>  
>  		if (val & SDHCI_CAN_DO_ADMA1) {
> @@ -1278,11 +1273,6 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  	if (IS_ERR(imx_data->pins_default))
>  		dev_warn(mmc_dev(host->mmc), "could not get default state\n");
>  
> -	if (imx_data->socdata->flags & ESDHC_FLAG_ENGCM07207)
> -		/* Fix errata ENGcm07207 present on i.MX25 and i.MX35 */
> -		host->quirks |= SDHCI_QUIRK_NO_MULTIBLOCK
> -			| SDHCI_QUIRK_BROKEN_ADMA;
> -
>  	if (esdhc_is_usdhc(imx_data)) {
>  		host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
>  		host->mmc->caps |= MMC_CAP_1_8V_DDR;
> 

Re: [PATCH 4/4] mmc: sdhci-esdhc-imx: Remove the ENGcm07207 workaround

[Ulf Hansson]

On 29 May 2017 at 10:07, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 03/05/17 13:05, Benoît Thébaudeau wrote:
>> The SDHCI_QUIRK_NO_MULTIBLOCK quirk was used as a workaround for the
>> ENGcm07207 erratum. However, it caused excruciatingly slow SD transfers
>> (300 kB/s on average), and this erratum actually does not imply that
>> multiple-block transfers are not supported, so this was overkill.
>>
>> The suggested workaround for this erratum is to set SYSCTL.RSTA, but the
>> simple DAT line software reset (which resets the DMA circuit among
>> others) triggered by sdhci_finish_data() in case of errors seems to be
>> sufficient. Indeed, generating errors in a controlled manner on i.MX25
>> using the FEVT register right in the middle of read data transfers
>> without this quirk shows that nothing is written to the buffer by the
>> eSDHC past CMD12, and no extra Auto CMD12 is sent with AC12EN set, so
>> the data transfers on AHB are properly aborted. For write data
>> transfers, neither extra data nor extra Auto CMD12 is sent, as expected.
>> Moreover, after intensive stress tests on i.MX25, removing
>> SDHCI_QUIRK_NO_MULTIBLOCK seems to be safe.
>>
>> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>
>
> Aside from one comment below...
>
> I would expect to see Acks from other sdhci-esdhc-imx users.  Nevertheless,
> for sdhci:

Yes, I would also appreciate some ack/tested by, from the
corresponding sdhci variant users of this series.

However, to allow it to get some results from linux-next, I have
queued up this series for next (amending $subject patch according to
the comment from Adrian). Thanks!

>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
>
>

[...]

Kind regards
Uffe

Re: [PATCH 4/4] mmc: sdhci-esdhc-imx: Remove the ENGcm07207 workaround

[Benoît Thébaudeau]

On 2017/05/29 at 16:42, Ulf Hansson wrote:
> On 29 May 2017 at 10:07, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 03/05/17 13:05, Benoît Thébaudeau wrote:
>>> The SDHCI_QUIRK_NO_MULTIBLOCK quirk was used as a workaround for the
>>> ENGcm07207 erratum. However, it caused excruciatingly slow SD transfers
>>> (300 kB/s on average), and this erratum actually does not imply that
>>> multiple-block transfers are not supported, so this was overkill.
>>>
>>> The suggested workaround for this erratum is to set SYSCTL.RSTA, but the
>>> simple DAT line software reset (which resets the DMA circuit among
>>> others) triggered by sdhci_finish_data() in case of errors seems to be
>>> sufficient. Indeed, generating errors in a controlled manner on i.MX25
>>> using the FEVT register right in the middle of read data transfers
>>> without this quirk shows that nothing is written to the buffer by the
>>> eSDHC past CMD12, and no extra Auto CMD12 is sent with AC12EN set, so
>>> the data transfers on AHB are properly aborted. For write data
>>> transfers, neither extra data nor extra Auto CMD12 is sent, as expected.
>>> Moreover, after intensive stress tests on i.MX25, removing
>>> SDHCI_QUIRK_NO_MULTIBLOCK seems to be safe.
>>>
>>> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>
>>
>> Aside from one comment below...
>>
>> I would expect to see Acks from other sdhci-esdhc-imx users.  Nevertheless,
>> for sdhci:
> 
> Yes, I would also appreciate some ack/tested by, from the
> corresponding sdhci variant users of this series.
> 
> However, to allow it to get some results from linux-next, I have
> queued up this series for next (amending $subject patch according to
> the comment from Adrian). Thanks!

Please note that I had superseded this series with a v2 following Adrian's
comment.

>>
>> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
>>
>>
> 
> [...]

Best regards,
Benoît

Re: [PATCH 4/4] mmc: sdhci-esdhc-imx: Remove the ENGcm07207 workaround

[Fabio Estevam]

On Wed, May 3, 2017 at 7:05 AM, Benoît Thébaudeau <benoit@wsystem.com> wrote:
> The SDHCI_QUIRK_NO_MULTIBLOCK quirk was used as a workaround for the
> ENGcm07207 erratum. However, it caused excruciatingly slow SD transfers
> (300 kB/s on average), and this erratum actually does not imply that
> multiple-block transfers are not supported, so this was overkill.
>
> The suggested workaround for this erratum is to set SYSCTL.RSTA, but the
> simple DAT line software reset (which resets the DMA circuit among
> others) triggered by sdhci_finish_data() in case of errors seems to be
> sufficient. Indeed, generating errors in a controlled manner on i.MX25
> using the FEVT register right in the middle of read data transfers
> without this quirk shows that nothing is written to the buffer by the
> eSDHC past CMD12, and no extra Auto CMD12 is sent with AC12EN set, so
> the data transfers on AHB are properly aborted. For write data
> transfers, neither extra data nor extra Auto CMD12 is sent, as expected.
> Moreover, after intensive stress tests on i.MX25, removing
> SDHCI_QUIRK_NO_MULTIBLOCK seems to be safe.
>
> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

Re: [PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR

[Arnd Bergmann]

On Wed, May 3, 2017 at 12:05 PM, Benoît Thébaudeau <benoit@wsystem.com> wrote:
> The eSDHC can only DMA from 32-bit-aligned addresses.
>
> This fixes the following test cases of mmc_test:
>   11:   Badly aligned write
>   12:   Badly aligned read
>   13:   Badly aligned multi-block write
>   14:   Badly aligned multi-block read
>
> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>

Is this the right description? I thought that SDHCI_QUIRK_32BIT_DMA_ADDR
was for devices that cannot address high memory above 0xffffffff, rather than
requiring a specific alignment.

If this is indeed an address range problem rather than an alignment problem,
are you sure it is the SD controller that is wrong here, rather than having a
64-bit DMA capable  SDHCI connected to a 32-bit parent bus? In the
latter case, the dma-ranges property in the parent bus should limit
the addressing, not the device.

      Arnd

Re: [PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR

[Benoît Thébaudeau]

On 04/05/2017 10:47, Arnd Bergmann wrote:
> On Wed, May 3, 2017 at 12:05 PM, Benoît Thébaudeau <benoit@wsystem.com> wrote:
>> The eSDHC can only DMA from 32-bit-aligned addresses.
>>
>> This fixes the following test cases of mmc_test:
>>   11:   Badly aligned write
>>   12:   Badly aligned read
>>   13:   Badly aligned multi-block write
>>   14:   Badly aligned multi-block read
>>
>> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>
> 
> Is this the right description? I thought that SDHCI_QUIRK_32BIT_DMA_ADDR
> was for devices that cannot address high memory above 0xffffffff, rather than
> requiring a specific alignment.
> 
> If this is indeed an address range problem rather than an alignment problem,
> are you sure it is the SD controller that is wrong here, rather than having a
> 64-bit DMA capable  SDHCI connected to a 32-bit parent bus? In the
> latter case, the dma-ranges property in the parent bus should limit
> the addressing, not the device.

No, this is the right description. This quirk really is about alignment, and not
about address range. See:

drivers/mmc/host/sdhci.h:
>---
/* Controller can only DMA from 32-bit aligned addresses */
#define SDHCI_QUIRK_32BIT_DMA_ADDR			(1<<7)
<---

drivers/mmc/host/sdhci.c @ sdhci_prepare_data():
>---
		offset_mask = 0;
[...]
			if (host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR)
				offset_mask = 3;
[...]
				if (sg->offset & offset_mask) {
					DBG("Reverting to PIO because of bad alignment\n");
					host->flags &= ~SDHCI_REQ_USE_DMA;
					break;
				}
<---

Benoît

Re: [PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR

[Arnd Bergmann]

On Thu, May 4, 2017 at 11:00 AM, Benoît Thébaudeau <benoit@wsystem.com> wrote:
> On 04/05/2017 10:47, Arnd Bergmann wrote:
>> On Wed, May 3, 2017 at 12:05 PM, Benoît Thébaudeau <benoit@wsystem.com> wrote:
>>> The eSDHC can only DMA from 32-bit-aligned addresses.
>>>
>>> This fixes the following test cases of mmc_test:
>>>   11:   Badly aligned write
>>>   12:   Badly aligned read
>>>   13:   Badly aligned multi-block write
>>>   14:   Badly aligned multi-block read
>>>
>>> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>
>>
>> Is this the right description? I thought that SDHCI_QUIRK_32BIT_DMA_ADDR
>> was for devices that cannot address high memory above 0xffffffff, rather than
>> requiring a specific alignment.
>>
>> If this is indeed an address range problem rather than an alignment problem,
>> are you sure it is the SD controller that is wrong here, rather than having a
>> 64-bit DMA capable  SDHCI connected to a 32-bit parent bus? In the
>> latter case, the dma-ranges property in the parent bus should limit
>> the addressing, not the device.
>
> No, this is the right description. This quirk really is about alignment, and not
> about address range. See:
>
> drivers/mmc/host/sdhci.h:
>>---
> /* Controller can only DMA from 32-bit aligned addresses */
> #define SDHCI_QUIRK_32BIT_DMA_ADDR                      (1<<7)
> <---
>
> drivers/mmc/host/sdhci.c @ sdhci_prepare_data():
>>---
>                 offset_mask = 0;
> [...]
>                         if (host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR)
>                                 offset_mask = 3;

Ok, thanks for the clarification. I guess I should have checked this myself
first.

       Arnd

Re: [PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR

[Adrian Hunter]

On 03/05/17 13:05, Benoît Thébaudeau wrote:
> The eSDHC can only DMA from 32-bit-aligned addresses.
> 
> This fixes the following test cases of mmc_test:
>   11:	Badly aligned write
>   12:	Badly aligned read
>   13:	Badly aligned multi-block write
>   14:	Badly aligned multi-block read
> 
> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>

I would expect to see Acks from other sdhci-esdhc users.  Nevertheless for
sdhci:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-esdhc.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h
> index c4bbd74..e7893f2 100644
> --- a/drivers/mmc/host/sdhci-esdhc.h
> +++ b/drivers/mmc/host/sdhci-esdhc.h
> @@ -19,6 +19,7 @@
>   */
>  
>  #define ESDHC_DEFAULT_QUIRKS	(SDHCI_QUIRK_FORCE_BLK_SZ_2048 | \
> +				SDHCI_QUIRK_32BIT_DMA_ADDR | \
>  				SDHCI_QUIRK_NO_BUSY_IRQ | \
>  				SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK | \
>  				SDHCI_QUIRK_PIO_NEEDS_DELAY | \
> 

Re: [PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR

[Fabio Estevam]

On Wed, May 3, 2017 at 7:05 AM, Benoît Thébaudeau <benoit@wsystem.com> wrote:
> The eSDHC can only DMA from 32-bit-aligned addresses.
>
> This fixes the following test cases of mmc_test:
>   11:   Badly aligned write
>   12:   Badly aligned read
>   13:   Badly aligned multi-block write
>   14:   Badly aligned multi-block read
>
> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>