[PATCH] mtd-nand: davinci: Correct 4-bit error correction

[PATCH] mtd-nand: davinci: Correct 4-bit error correction

[Sudhakar Rajashekhara]

On TI's DA830/OMAP-L137, DA850/OMAP-L138 and DM365, after
setting the 4BITECC_ADD_CALC_START bit in the NAND Flash
control register to 1 and before waiting for the NAND Flash
status register to be equal to 1, 2 or 3, we have to wait
till the ECC HW goes to correction state. Without this wait,
ECC correction calculations will not be proper.

This has been tested on DA830/OMAP-L137, DA850/OMAP-L138,
DM355 and DM365 EVMs.

Signed-off-by: Sudhakar Rajashekhara <sudhakar.raj@ti.com>
Acked-by: Sneha Narnakaje <nsnehaprabha@ti.com>
---
 drivers/mtd/nand/davinci_nand.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index fe3eba8..8a32999 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -310,6 +310,7 @@ static int nand_davinci_correct_4bit(struct mtd_info *mtd,
 	unsigned short ecc10[8];
 	unsigned short *ecc16;
 	u32 syndrome[4];
+	u32 ecc_state;
 	unsigned num_errors, corrected;
 
 	/* All bytes 0xff?  It's an erased page; ignore its ECC. */
@@ -360,6 +361,21 @@ compare:
 	 */
 	davinci_nand_writel(info, NANDFCR_OFFSET,
 			davinci_nand_readl(info, NANDFCR_OFFSET) | BIT(13));
+
+	/*
+	 * ECC_STATE field reads 0x3 (Error correction complete) immediately
+	 * after setting the 4BITECC_ADD_CALC_START bit. So if you immediately
+	 * begin trying to poll for the state, you may fall right out of your
+	 * loop without any of the correction calculations having taken place.
+	 * The recommendation from the hardware team is to wait till ECC_STATE
+	 * reads less than 4, which means ECC HW has entered correction state.
+	 */
+	do {
+		ecc_state = (davinci_nand_readl(info,
+				NANDFSR_OFFSET) >> 8) & 0x0f;
+		cpu_relax();
+	} while (ecc_state < 4);
+
 	for (;;) {
 		u32	fsr = davinci_nand_readl(info, NANDFSR_OFFSET);
 
-- 
1.5.6

Re: [PATCH] mtd-nand: davinci: Correct 4-bit error correction

[Artem Bityutskiy]

On Tue, 2009-11-03 at 16:01 +0530, Sudhakar Rajashekhara wrote:
> On TI's DA830/OMAP-L137, DA850/OMAP-L138 and DM365, after
> setting the 4BITECC_ADD_CALC_START bit in the NAND Flash
> control register to 1 and before waiting for the NAND Flash
> status register to be equal to 1, 2 or 3, we have to wait
> till the ECC HW goes to correction state. Without this wait,
> ECC correction calculations will not be proper.
> 
> This has been tested on DA830/OMAP-L137, DA850/OMAP-L138,
> DM355 and DM365 EVMs.
> 
> Signed-off-by: Sudhakar Rajashekhara <sudhakar.raj@ti.com>
> Acked-by: Sneha Narnakaje <nsnehaprabha@ti.com>
> ---
>  drivers/mtd/nand/davinci_nand.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index fe3eba8..8a32999 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -310,6 +310,7 @@ static int nand_davinci_correct_4bit(struct mtd_info *mtd,
>  	unsigned short ecc10[8];
>  	unsigned short *ecc16;
>  	u32 syndrome[4];
> +	u32 ecc_state;
>  	unsigned num_errors, corrected;
>  
>  	/* All bytes 0xff?  It's an erased page; ignore its ECC. */
> @@ -360,6 +361,21 @@ compare:
>  	 */
>  	davinci_nand_writel(info, NANDFCR_OFFSET,
>  			davinci_nand_readl(info, NANDFCR_OFFSET) | BIT(13));
> +
> +	/*
> +	 * ECC_STATE field reads 0x3 (Error correction complete) immediately
> +	 * after setting the 4BITECC_ADD_CALC_START bit. So if you immediately
> +	 * begin trying to poll for the state, you may fall right out of your
> +	 * loop without any of the correction calculations having taken place.
> +	 * The recommendation from the hardware team is to wait till ECC_STATE
> +	 * reads less than 4, which means ECC HW has entered correction state.
> +	 */
> +	do {
> +		ecc_state = (davinci_nand_readl(info,
> +				NANDFSR_OFFSET) >> 8) & 0x0f;
> +		cpu_relax();
> +	} while (ecc_state < 4);
> +
>  	for (;;) {
>  		u32	fsr = davinci_nand_readl(info, NANDFSR_OFFSET);

I see a lot of constructs like this in many mtd drivers. I wonder, what
happens if ecc_state never becomes < 4? Should there be some protection
against this, e.g., exit the loop with an error message if we are
looping for more than 1 or several seconds?

What is the "official" / "right" way for this kind of loops?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH] mtd-nand: davinci: correct 4-bit error correction

[Sudhakar Rajashekhara]

On TI's DA830/OMAP-L137, DA850/OMAP-L138 and DM365, after setting the
4BITECC_ADD_CALC_START bit in the NAND Flash control register to 1 and
before waiting for the NAND Flash status register to be equal to 1, 2 or
3, we have to wait till the ECC HW goes to correction state.  Without this
wait, ECC correction calculations will not be proper.

This has been tested on DA830/OMAP-L137, DA850/OMAP-L138, DM355 and DM365
EVMs.

Signed-off-by: Sudhakar Rajashekhara <sudhakar.raj@ti.com>
Acked-by: Sneha Narnakaje <nsnehaprabha@ti.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
This patch applies on top of Linus's master.

This patch was present in -mm tree and was dropped because it was merged
into mainline. But now this patch is not present neither in Linus's tree
nor in linux-next. Hence resending it again.

 drivers/mtd/nand/davinci_nand.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 9c9d893..2ac7367 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -311,7 +311,9 @@ static int nand_davinci_correct_4bit(struct mtd_info *mtd,
 	unsigned short ecc10[8];
 	unsigned short *ecc16;
 	u32 syndrome[4];
+	u32 ecc_state;
 	unsigned num_errors, corrected;
+	unsigned long timeo = jiffies + msecs_to_jiffies(100);
 
 	/* All bytes 0xff?  It's an erased page; ignore its ECC. */
 	for (i = 0; i < 10; i++) {
@@ -361,6 +363,21 @@ compare:
 	 */
 	davinci_nand_writel(info, NANDFCR_OFFSET,
 			davinci_nand_readl(info, NANDFCR_OFFSET) | BIT(13));
+
+	/*
+	 * ECC_STATE field reads 0x3 (Error correction complete) immediately
+	 * after setting the 4BITECC_ADD_CALC_START bit. So if you immediately
+	 * begin trying to poll for the state, you may fall right out of your
+	 * loop without any of the correction calculations having taken place.
+	 * The recommendation from the hardware team is to wait till ECC_STATE
+	 * reads less than 4, which means ECC HW has entered correction state.
+	 */
+	do {
+		ecc_state = (davinci_nand_readl(info,
+				NANDFSR_OFFSET) >> 8) & 0x0f;
+		cpu_relax();
+	} while ((ecc_state < 4) && time_before(jiffies, timeo));
+
 	for (;;) {
 		u32	fsr = davinci_nand_readl(info, NANDFSR_OFFSET);
 
-- 
1.5.6

Re: [PATCH] mtd-nand: davinci: correct 4-bit error correction

[Andrew Morton]

On Fri,  9 Jul 2010 10:59:49 +0530
Sudhakar Rajashekhara <sudhakar.raj@ti.com> wrote:

> +
> +	/*
> +	 * ECC_STATE field reads 0x3 (Error correction complete) immediately
> +	 * after setting the 4BITECC_ADD_CALC_START bit. So if you immediately
> +	 * begin trying to poll for the state, you may fall right out of your
> +	 * loop without any of the correction calculations having taken place.
> +	 * The recommendation from the hardware team is to wait till ECC_STATE
> +	 * reads less than 4, which means ECC HW has entered correction state.
> +	 */
> +	do {
> +		ecc_state = (davinci_nand_readl(info,
> +				NANDFSR_OFFSET) >> 8) & 0x0f;
> +		cpu_relax();
> +	} while ((ecc_state < 4) && time_before(jiffies, timeo));

An up-to-100-milliseond busy wait is pretty bad.  For how long do you
expect this to spin in practice?

RE: [PATCH] mtd-nand: davinci: correct 4-bit error correction

[Sudhakar Rajashekhara]

On Sat, Jul 10, 2010 at 04:09:32, Andrew Morton wrote:
> On Fri,  9 Jul 2010 10:59:49 +0530
> Sudhakar Rajashekhara <sudhakar.raj@ti.com> wrote:
> 
> > +
> > +	/*
> > +	 * ECC_STATE field reads 0x3 (Error correction complete) immediately
> > +	 * after setting the 4BITECC_ADD_CALC_START bit. So if you immediately
> > +	 * begin trying to poll for the state, you may fall right out of your
> > +	 * loop without any of the correction calculations having taken place.
> > +	 * The recommendation from the hardware team is to wait till ECC_STATE
> > +	 * reads less than 4, which means ECC HW has entered correction state.
> > +	 */
> > +	do {
> > +		ecc_state = (davinci_nand_readl(info,
> > +				NANDFSR_OFFSET) >> 8) & 0x0f;
> > +		cpu_relax();
> > +	} while ((ecc_state < 4) && time_before(jiffies, timeo));
> 
> An up-to-100-milliseond busy wait is pretty bad.  For how long do you
> expect this to spin in practice?

On the hardware, I have never seen this taking 100 msec to come out of
the loop. I'll check with the hardware folks on the maximum time to wait
for, before the ECC engine is ready.

Thanks,
Sudhakar

RE: [PATCH] mtd-nand: davinci: correct 4-bit error correction

[Sudhakar Rajashekhara]

Hi,

On Mon, Jul 12, 2010 at 11:58:18, Sudhakar Rajashekhara wrote:
> On Sat, Jul 10, 2010 at 04:09:32, Andrew Morton wrote:
> > On Fri,  9 Jul 2010 10:59:49 +0530
> > Sudhakar Rajashekhara <sudhakar.raj@ti.com> wrote:
> > 
> > > +
> > > +	/*
> > > +	 * ECC_STATE field reads 0x3 (Error correction complete) immediately
> > > +	 * after setting the 4BITECC_ADD_CALC_START bit. So if you immediately
> > > +	 * begin trying to poll for the state, you may fall right out of your
> > > +	 * loop without any of the correction calculations having taken place.
> > > +	 * The recommendation from the hardware team is to wait till ECC_STATE
> > > +	 * reads less than 4, which means ECC HW has entered correction state.
> > > +	 */
> > > +	do {
> > > +		ecc_state = (davinci_nand_readl(info,
> > > +				NANDFSR_OFFSET) >> 8) & 0x0f;
> > > +		cpu_relax();
> > > +	} while ((ecc_state < 4) && time_before(jiffies, timeo));
> > 
> > An up-to-100-milliseond busy wait is pretty bad.  For how long do you
> > expect this to spin in practice?
> 
> On the hardware, I have never seen this taking 100 msec to come out of
> the loop. I'll check with the hardware folks on the maximum time to wait
> for, before the ECC engine is ready.

I checked this with the hardware team but no one is sure about the exact
time one should wait before the ECC engine becomes ready. But everyone is
of the opinion that 100 loop cycles should be enough. To be on the safer
side, I'll be changing the timeout to 10 milliseconds in the next version
of this patch.

Thanks,
Sudhakar

Re: [PATCH] mtd-nand: davinci: correct 4-bit error correction

[Andrew Morton]

On Tue, 13 Jul 2010 15:02:59 +0530 "Sudhakar Rajashekhara" <sudhakar.raj@ti.com> wrote:

> Hi,
> 
> On Mon, Jul 12, 2010 at 11:58:18, Sudhakar Rajashekhara wrote:
> > On Sat, Jul 10, 2010 at 04:09:32, Andrew Morton wrote:
> > > On Fri,  9 Jul 2010 10:59:49 +0530
> > > Sudhakar Rajashekhara <sudhakar.raj@ti.com> wrote:
> > > 
> > > > +
> > > > +	/*
> > > > +	 * ECC_STATE field reads 0x3 (Error correction complete) immediately
> > > > +	 * after setting the 4BITECC_ADD_CALC_START bit. So if you immediately
> > > > +	 * begin trying to poll for the state, you may fall right out of your
> > > > +	 * loop without any of the correction calculations having taken place.
> > > > +	 * The recommendation from the hardware team is to wait till ECC_STATE
> > > > +	 * reads less than 4, which means ECC HW has entered correction state.
> > > > +	 */
> > > > +	do {
> > > > +		ecc_state = (davinci_nand_readl(info,
> > > > +				NANDFSR_OFFSET) >> 8) & 0x0f;
> > > > +		cpu_relax();
> > > > +	} while ((ecc_state < 4) && time_before(jiffies, timeo));
> > > 
> > > An up-to-100-milliseond busy wait is pretty bad.  For how long do you
> > > expect this to spin in practice?
> > 
> > On the hardware, I have never seen this taking 100 msec to come out of
> > the loop. I'll check with the hardware folks on the maximum time to wait
> > for, before the ECC engine is ready.
> 
> I checked this with the hardware team but no one is sure about the exact
> time one should wait before the ECC engine becomes ready. But everyone is
> of the opinion that 100 loop cycles should be enough. To be on the safer
> side, I'll be changing the timeout to 10 milliseconds in the next version
> of this patch.

Going from 100ms down to 10ms sounds a bit risky.  It'd be better to
retain the 100ms and to make the kernel spend most of that time
sleeping, rather than busywaiting, IMO.

RE: [PATCH] mtd-nand: davinci: correct 4-bit error correction

[Sudhakar Rajashekhara]

Hi,

On Tue, Jul 13, 2010 at 23:11:26, Andrew Morton wrote:
> On Tue, 13 Jul 2010 15:02:59 +0530 "Sudhakar Rajashekhara" <sudhakar.raj@ti.com> wrote:
> 
> > Hi,
> > 
> > On Mon, Jul 12, 2010 at 11:58:18, Sudhakar Rajashekhara wrote:
> > > On Sat, Jul 10, 2010 at 04:09:32, Andrew Morton wrote:
> > > > On Fri,  9 Jul 2010 10:59:49 +0530
> > > > Sudhakar Rajashekhara <sudhakar.raj@ti.com> wrote:
> > > > 
> > > > > +
> > > > > +	/*
> > > > > +	 * ECC_STATE field reads 0x3 (Error correction complete) immediately
> > > > > +	 * after setting the 4BITECC_ADD_CALC_START bit. So if you immediately
> > > > > +	 * begin trying to poll for the state, you may fall right out of your
> > > > > +	 * loop without any of the correction calculations having taken place.
> > > > > +	 * The recommendation from the hardware team is to wait till ECC_STATE
> > > > > +	 * reads less than 4, which means ECC HW has entered correction state.
> > > > > +	 */
> > > > > +	do {
> > > > > +		ecc_state = (davinci_nand_readl(info,
> > > > > +				NANDFSR_OFFSET) >> 8) & 0x0f;
> > > > > +		cpu_relax();
> > > > > +	} while ((ecc_state < 4) && time_before(jiffies, timeo));
> > > > 
> > > > An up-to-100-milliseond busy wait is pretty bad.  For how long do you
> > > > expect this to spin in practice?
> > > 
> > > On the hardware, I have never seen this taking 100 msec to come out of
> > > the loop. I'll check with the hardware folks on the maximum time to wait
> > > for, before the ECC engine is ready.
> > 
> > I checked this with the hardware team but no one is sure about the exact
> > time one should wait before the ECC engine becomes ready. But everyone is
> > of the opinion that 100 loop cycles should be enough. To be on the safer
> > side, I'll be changing the timeout to 10 milliseconds in the next version
> > of this patch.
> 
> Going from 100ms down to 10ms sounds a bit risky.  It'd be better to
> retain the 100ms and to make the kernel spend most of that time
> sleeping, rather than busywaiting, IMO.
> 

I chose 100ms timeout randomly, but today I did some calculation using
do_gettimeofday() to find out the actual time spent inside the loop. I
found that, control is coming out of loop within 5 microseconds. So I'll
go ahead and use usecs_to_jiffies(100) as timeout. Busywaiting for such
a short duration may not be a problem.

Regards,
Sudhakar