[U-Boot] [PATCH] NAND: DaVinci: Update 4 bit ECC correction

[U-Boot] [PATCH] NAND: DaVinci: Update 4 bit ECC correction

[s-paulraj at ti.com]

From: Sandeep Paulraj <s-paulraj@ti.com>

There was a bug in the 4 bit ECC calculation routine
in the DaVinci NAND driver. This becomes prominent
when we use 4K page size NAND devices.
This is a fix for the issue.

Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
---
 drivers/mtd/nand/davinci_nand.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 41a9568..68a0e15 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -388,6 +388,17 @@ static int nand_davinci_4bit_correct_data(struct mtd_info *mtd, uint8_t *dat,
 	emif_regs->NANDFCR |= 1 << 13;
 
 	/*
+	 * ECC_STATE in NANDFSR register reads 0x3 (Error correction complete)
+	 * immediately after setting the 4BITECC_ADD_CALC_START bit. So if we
+	 * begin trying to poll for the state, we may fall right out of your
+	 * loop without any of the correction calculations having taken place.
+	 * So wait till ECC_STATE reads less than 4.
+	 */
+	do {
+		val = ((emif_regs->NANDFSR >> 8) & 0xf);
+	} while (val < 4);
+
+	/*
 	 * Wait for the corr_state field (bits 8 to 11)in the
 	 * NAND Flash Status register to be equal to 0x0, 0x1, 0x2, or 0x3.
 	 */
-- 
1.6.0.4

[U-Boot] [PATCH] NAND: DaVinci: Update 4 bit ECC correction

[Scott Wood]

On Tue, Dec 01, 2009 at 05:24:59PM -0500, s-paulraj at ti.com wrote:
> From: Sandeep Paulraj <s-paulraj@ti.com>
> 
> There was a bug in the 4 bit ECC calculation routine
> in the DaVinci NAND driver. This becomes prominent
> when we use 4K page size NAND devices.
> This is a fix for the issue.
> 
> Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
> ---
>  drivers/mtd/nand/davinci_nand.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index 41a9568..68a0e15 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -388,6 +388,17 @@ static int nand_davinci_4bit_correct_data(struct mtd_info *mtd, uint8_t *dat,
>  	emif_regs->NANDFCR |= 1 << 13;
>  
>  	/*
> +	 * ECC_STATE in NANDFSR register reads 0x3 (Error correction complete)
> +	 * immediately after setting the 4BITECC_ADD_CALC_START bit. So if we
> +	 * begin trying to poll for the state, we may fall right out of your
> +	 * loop

"your" loop?

>  without any of the correction calculations having taken place.
> +	 * So wait till ECC_STATE reads less than 4.
> +	 */
> +	do {
> +		val = ((emif_regs->NANDFSR >> 8) & 0xf);
> +	} while (val < 4);

Comment says wait until less than 4, code says wait until@least 4.  Could
symbolic constants be used to describe the states instead of magic numbers
like 4 and 0xc00?

Is it possible that there could be a race, whereby the hardware finishes
before you read NANDFSR for the first time, and thus you never see it be
greater than 3?

-Scott

[U-Boot] [PATCH] NAND: DaVinci: Update 4 bit ECC correction

[Paulraj, Sandeep]

> > From: Sandeep Paulraj <s-paulraj@ti.com>
> >
> > There was a bug in the 4 bit ECC calculation routine
> > in the DaVinci NAND driver. This becomes prominent
> > when we use 4K page size NAND devices.
> > This is a fix for the issue.
> >
> > Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
> > ---
> >  drivers/mtd/nand/davinci_nand.c |   11 +++++++++++
> >  1 files changed, 11 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/davinci_nand.c
> b/drivers/mtd/nand/davinci_nand.c
> > index 41a9568..68a0e15 100644
> > --- a/drivers/mtd/nand/davinci_nand.c
> > +++ b/drivers/mtd/nand/davinci_nand.c
> > @@ -388,6 +388,17 @@ static int nand_davinci_4bit_correct_data(struct
> mtd_info *mtd, uint8_t *dat,
> >  	emif_regs->NANDFCR |= 1 << 13;
> >
> >  	/*
> > +	 * ECC_STATE in NANDFSR register reads 0x3 (Error correction
> complete)
> > +	 * immediately after setting the 4BITECC_ADD_CALC_START bit. So if
> we
> > +	 * begin trying to poll for the state, we may fall right out of your
> > +	 * loop
> 
> "your" loop?
> 
> >  without any of the correction calculations having taken place.
> > +	 * So wait till ECC_STATE reads less than 4.
> > +	 */
> > +	do {
> > +		val = ((emif_regs->NANDFSR >> 8) & 0xf);
> > +	} while (val < 4);
> 
> Comment says wait until less than 4, code says wait until at least 4.
> Could
> symbolic constants be used to describe the states instead of magic numbers
> like 4 and 0xc00?
I'll update my explanations and use symbolic constants

> 
> Is it possible that there could be a race, whereby the hardware finishes
> before you read NANDFSR for the first time, and thus you never see it be
> greater than 3?

There are other check conditions after this check condition in the driver.
If we never see the concetned value greater than 3, that actually means that ECC calculation has finished or not required at all because there were no errors

> 
> -Scott

[U-Boot] [PATCH] NAND: DaVinci: Update 4 bit ECC correction

[Scott Wood]

Paulraj, Sandeep wrote:
>> Is it possible that there could be a race, whereby the hardware finishes
>> before you read NANDFSR for the first time, and thus you never see it be
>> greater than 3?
> 
> There are other check conditions after this check condition in the driver.
> If we never see the concetned value greater than 3, that actually
> means that ECC calculation has finished or not required at all
> because there were no errors

And this patch will respond to that by hanging, waiting for a value 
greater than 3.

-Scott