Patchwork [v3,3/3] mtd-nand: DaVinci: Add 4-bit ECC support for large page NAND chips

login
register
mail settings
Submitter David Brownell
Date July 17, 2009, 7:32 p.m.
Message ID <200907171232.53725.david-b@pacbell.net>
Download mbox | patch
Permalink /patch/29935/
State New, archived
Headers show

Comments

David Brownell - July 17, 2009, 7:32 p.m.
On Thursday 16 July 2009, nsnehaprabha@ti.com wrote:
> 	It also fixes a bug in the ECC correction handler.
> When we introduce 5 bit-errors in chunk, error correction stops working. When
> errors are detected the 4BITECC_START bit was left high, which should be
> cleared. 

Agreed that needs to be fixed, but there should be a comment
about this being an *undocumented* behavior in the hardware.

The reason that the bug exists at all is because this step
has never been documented.  So, please roll in this update.

- Dave

---
 drivers/mtd/nand/davinci_nand.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
nsnehaprabha@ti.com - July 17, 2009, 8:18 p.m.
Dave,

> -----Original Message-----
> From: David Brownell [mailto:david-b@pacbell.net]
> Sent: Friday, July 17, 2009 3:33 PM
> To: davinci-linux-open-source@linux.davincidsp.com
> Cc: Narnakaje, Snehaprabha; linux-mtd@lists.infradead.org;
> dwmw2@infradead.org; tglx@linutronix.de; akpm@linux-foundation.org
> Subject: Re: [PATCH v3 3/3] mtd-nand: DaVinci: Add 4-bit ECC support for
> large page NAND chips
> 
> On Thursday 16 July 2009, nsnehaprabha@ti.com wrote:
> > 	It also fixes a bug in the ECC correction handler.
> > When we introduce 5 bit-errors in chunk, error correction stops working.
> When
> > errors are detected the 4BITECC_START bit was left high, which should be
> > cleared.
> 
> Agreed that needs to be fixed, but there should be a comment
> about this being an *undocumented* behavior in the hardware.
> 
> The reason that the bug exists at all is because this step
> has never been documented.  So, please roll in this update.

How about, we get the documents updated for this missing step.

Thanks
Sneha

> 
> - Dave
> 
> ---
>  drivers/mtd/nand/davinci_nand.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -350,13 +350,16 @@ compare:
> 
>  	/*
>  	 * Clear any previous address calculation by doing a dummy read of
> an
> -	 * error address register.
> +	 * error address register.  UNDOCUMENTED that the ECC engine won't
> +	 * recover after 5-bit ECC errors without this step.
>  	 */
>  	davinci_nand_readl(info, NAND_ERR_ADD1_OFFSET);
> 
>  	/* Start address calculation, and wait for it to complete.
>  	 * We _could_ start reading more data while this is working,
> -	 * to speed up the overall page read.
> +	 * to speed up the overall page read.  UNDOCUMENTED that
> +	 * reading some ERRVAL register is needed in all cases, not
> +	 * just when an error must be corrected.
>  	 */
>  	davinci_nand_writel(info, NANDFCR_OFFSET,
>  			davinci_nand_readl(info, NANDFCR_OFFSET) | BIT(13));
David Brownell - July 17, 2009, 9:18 p.m.
On Friday 17 July 2009, Narnakaje, Snehaprabha wrote:
> 
> > Agreed that needs to be fixed, but there should be a comment
> > about this being an *undocumented* behavior in the hardware.
> > 
> > The reason that the bug exists at all is because this step
> > has never been documented.  So, please roll in this update.
> 
> How about, we get the documents updated for this missing step.

I had hoped this was in the works, but I was told
that for some reason being able to do that was a
problem.  And in the few months since this issue
was identified, the docs haven't been updated.  :(

- Dve

Patch

--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -350,13 +350,16 @@  compare:
 
 	/*
 	 * Clear any previous address calculation by doing a dummy read of an
-	 * error address register.
+	 * error address register.  UNDOCUMENTED that the ECC engine won't
+	 * recover after 5-bit ECC errors without this step.
 	 */
 	davinci_nand_readl(info, NAND_ERR_ADD1_OFFSET);
 
 	/* Start address calculation, and wait for it to complete.
 	 * We _could_ start reading more data while this is working,
-	 * to speed up the overall page read.
+	 * to speed up the overall page read.  UNDOCUMENTED that
+	 * reading some ERRVAL register is needed in all cases, not
+	 * just when an error must be corrected.
 	 */
 	davinci_nand_writel(info, NANDFCR_OFFSET,
 			davinci_nand_readl(info, NANDFCR_OFFSET) | BIT(13));