[U-Boot] [PATCH] ENGR00299356 ARM:imx6 Fix USDHC driver bug in PIO mode

[U-Boot] [PATCH] ENGR00299356 ARM:imx6 Fix USDHC driver bug in PIO mode

[Ye.Li]

When configure the USDHC driver to PIO mode by defining
"CONFIG_SYS_FSL_ESDHC_USE_PIO", the SD/MMC read and write will fail.

Two bugs in the driver to cause the issue:
1. The read buffer was invalidated after reading from DATAPORT register,
which should be only applied to DMA mode. The valid data in cache was
overwritten by physical memory.
2. The watermarks are not set in PIO mode, will cause according state not
be set.

Signed-off-by: Ye.Li <B37916@freescale.com>
---
 drivers/mmc/fsl_esdhc.c |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index b5f6d62..bd0399a 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -190,7 +190,7 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
 	int timeout;
 	struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv;
 	struct fsl_esdhc *regs = (struct fsl_esdhc *)cfg->esdhc_base;
-#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO
+
 	uint wml_value;
 
 	wml_value = data->blocksize/4;
@@ -200,12 +200,15 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
 			wml_value = WML_RD_WML_MAX_VAL;
 
 		esdhc_clrsetbits32(&regs->wml, WML_RD_WML_MASK, wml_value);
+#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO
 		esdhc_write32(&regs->dsaddr, (u32)data->dest);
+#endif
 	} else {
+#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO
 		flush_dcache_range((ulong)data->src,
 				   (ulong)data->src+data->blocks
 					 *data->blocksize);
-
+#endif
 		if (wml_value > WML_WR_WML_MAX)
 			wml_value = WML_WR_WML_MAX_VAL;
 		if ((esdhc_read32(&regs->prsstat) & PRSSTAT_WPSPL) == 0) {
@@ -215,19 +218,10 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
 
 		esdhc_clrsetbits32(&regs->wml, WML_WR_WML_MASK,
 					wml_value << 16);
+#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO
 		esdhc_write32(&regs->dsaddr, (u32)data->src);
+#endif
 	}
-#else	/* CONFIG_SYS_FSL_ESDHC_USE_PIO */
-	if (!(data->flags & MMC_DATA_READ)) {
-		if ((esdhc_read32(&regs->prsstat) & PRSSTAT_WPSPL) == 0) {
-			printf("\nThe SD card is locked. "
-				"Can not write to a locked card.\n\n");
-			return TIMEOUT;
-		}
-		esdhc_write32(&regs->dsaddr, (u32)data->src);
-	} else
-		esdhc_write32(&regs->dsaddr, (u32)data->dest);
-#endif	/* CONFIG_SYS_FSL_ESDHC_USE_PIO */
 
 	esdhc_write32(&regs->blkattr, data->blocks << 16 | data->blocksize);
 
@@ -410,9 +404,10 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
 			if (irqstat & DATA_ERR)
 				return COMM_ERR;
 		} while ((irqstat & DATA_COMPLETE) != DATA_COMPLETE);
-#endif
+
 		if (data->flags & MMC_DATA_READ)
 			check_and_invalidate_dcache_range(cmd, data);
+#endif
 	}
 
 	esdhc_write32(&regs->irqstat, -1);
-- 
1.7.4.1

[U-Boot] [PATCH] ENGR00299356 ARM:imx6 Fix USDHC driver bug in PIO mode

[Albert ARIBAUD]

Hi Ye.Li,

On Wed, 19 Feb 2014 18:33:31 +0800, Ye.Li <B37916@freescale.com> wrote:

> When configure the USDHC driver to PIO mode by defining
> "CONFIG_SYS_FSL_ESDHC_USE_PIO", the SD/MMC read and write will fail.
> 
> Two bugs in the driver to cause the issue:
> 1. The read buffer was invalidated after reading from DATAPORT register,
> which should be only applied to DMA mode. The valid data in cache was
> overwritten by physical memory.
> 2. The watermarks are not set in PIO mode, will cause according state not
> be set.

What does the ENGR00299356 part of the subject line mean?

Also, in subject line: rewrite "arm:imx6 " as "arm: imx56: "

> Signed-off-by: Ye.Li <B37916@freescale.com>
> ---
>  drivers/mmc/fsl_esdhc.c |   23 +++++++++--------------
>  1 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index b5f6d62..bd0399a 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -190,7 +190,7 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
>  	int timeout;
>  	struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv;
>  	struct fsl_esdhc *regs = (struct fsl_esdhc *)cfg->esdhc_base;
> -#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO
> +
>  	uint wml_value;
>  
>  	wml_value = data->blocksize/4;
> @@ -200,12 +200,15 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
>  			wml_value = WML_RD_WML_MAX_VAL;
>  
>  		esdhc_clrsetbits32(&regs->wml, WML_RD_WML_MASK, wml_value);
> +#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO
>  		esdhc_write32(&regs->dsaddr, (u32)data->dest);
> +#endif
>  	} else {
> +#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO
>  		flush_dcache_range((ulong)data->src,
>  				   (ulong)data->src+data->blocks
>  					 *data->blocksize);
> -
> +#endif
>  		if (wml_value > WML_WR_WML_MAX)
>  			wml_value = WML_WR_WML_MAX_VAL;
>  		if ((esdhc_read32(&regs->prsstat) & PRSSTAT_WPSPL) == 0) {
> @@ -215,19 +218,10 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
>  
>  		esdhc_clrsetbits32(&regs->wml, WML_WR_WML_MASK,
>  					wml_value << 16);
> +#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO
>  		esdhc_write32(&regs->dsaddr, (u32)data->src);
> +#endif
>  	}
> -#else	/* CONFIG_SYS_FSL_ESDHC_USE_PIO */
> -	if (!(data->flags & MMC_DATA_READ)) {
> -		if ((esdhc_read32(&regs->prsstat) & PRSSTAT_WPSPL) == 0) {
> -			printf("\nThe SD card is locked. "
> -				"Can not write to a locked card.\n\n");
> -			return TIMEOUT;
> -		}
> -		esdhc_write32(&regs->dsaddr, (u32)data->src);
> -	} else
> -		esdhc_write32(&regs->dsaddr, (u32)data->dest);
> -#endif	/* CONFIG_SYS_FSL_ESDHC_USE_PIO */
>  
>  	esdhc_write32(&regs->blkattr, data->blocks << 16 | data->blocksize);
>  
> @@ -410,9 +404,10 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
>  			if (irqstat & DATA_ERR)
>  				return COMM_ERR;
>  		} while ((irqstat & DATA_COMPLETE) != DATA_COMPLETE);
> -#endif
> +
>  		if (data->flags & MMC_DATA_READ)
>  			check_and_invalidate_dcache_range(cmd, data);
> +#endif
>  	}
>  
>  	esdhc_write32(&regs->irqstat, -1);


Amicalement,
-- 
Albert.

[U-Boot] [PATCH] ENGR00299356 ARM:imx6 Fix USDHC driver bug in PIO mode

[Stefano Babic]

Hi Ye.Li,

On 19/02/2014 11:33, Ye.Li wrote:
> When configure the USDHC driver to PIO mode by defining
> "CONFIG_SYS_FSL_ESDHC_USE_PIO", the SD/MMC read and write will fail.
> 
> Two bugs in the driver to cause the issue:
> 1. The read buffer was invalidated after reading from DATAPORT register,
> which should be only applied to DMA mode. The valid data in cache was
> overwritten by physical memory.
> 2. The watermarks are not set in PIO mode, will cause according state not
> be set.
> 
> Signed-off-by: Ye.Li <B37916@freescale.com>
> ---

Why is this an  ARM/i.MX6 issue only ? As far as I read, this is not
strictly related to the SOC. Currently, there is no ARM boards setting
CONFIG_SYS_FSL_ESDHC_USE_PIO. The only boards using it are PowerPC
boards (the driver is platform independent): P1_P2_RDB and MPC8308RDB.

Regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH] ENGR00299356 ARM:imx6 Fix USDHC driver bug in PIO mode

[Ye Li]

Thanks, I will change the commit log accordingly.

Best regards,
Ye Li
-----Original Message-----
From: Stefano Babic [mailto:sbabic at denx.de] 
Sent: Wednesday, February 19, 2014 9:41 PM
To: Li Ye-B37916; sbabic at denx.de; Estevam Fabio-R49496
Cc: u-boot at lists.denx.de; Albert ARIBAUD
Subject: Re: [PATCH] ENGR00299356 ARM:imx6 Fix USDHC driver bug in PIO mode

Hi Ye.Li,

On 19/02/2014 11:33, Ye.Li wrote:
> When configure the USDHC driver to PIO mode by defining 
> "CONFIG_SYS_FSL_ESDHC_USE_PIO", the SD/MMC read and write will fail.
> 
> Two bugs in the driver to cause the issue:
> 1. The read buffer was invalidated after reading from DATAPORT 
> register, which should be only applied to DMA mode. The valid data in 
> cache was overwritten by physical memory.
> 2. The watermarks are not set in PIO mode, will cause according state 
> not be set.
> 
> Signed-off-by: Ye.Li <B37916@freescale.com>
> ---

Why is this an  ARM/i.MX6 issue only ? As far as I read, this is not strictly related to the SOC. Currently, there is no ARM boards setting CONFIG_SYS_FSL_ESDHC_USE_PIO. The only boards using it are PowerPC boards (the driver is platform independent): P1_P2_RDB and MPC8308RDB.

Regards,
Stefano Babic

--
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de =====================================================================

[U-Boot] [PATCH] ENGR00299356 ARM:imx6 Fix USDHC driver bug in PIO mode

[Wolfgang Denk]

Dear Ye.Li <B37916@freescale.com>,


repl: bad addresses:
	Ye.Li <B37916@freescale.com> -- no at-sign after local-part (<)
In message <1392806011-18821-1-git-send-email-B37916@freescale.com> you wrote:
> When configure the USDHC driver to PIO mode by defining
> "CONFIG_SYS_FSL_ESDHC_USE_PIO", the SD/MMC read and write will fail.

Can we please get rid of things like this "ENGR00299356" in the commit
message?  This may carry some meaning to you, but without explanation
and reference it is meaningless for the rest of us.  In any case, if
you want to keep it, then please remove it from the Subject and add it
to (the end of) the commit message.

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I knew then (in 1970) that a 4-kbyte minicomputer would cost as much
as a house. So I reasoned  that  after  college,  I'd  have  to  live
cheaply in an apartment and put all my money into owning a computer."
      - Apple co-founder Steve Wozniak, EE Times, June 6, 1988, pg 45