RE: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

RE: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

[Singh, Vimal]

Hi Frans,

Frans Meulenbroeks wrote:
> This patch for nand_ecc.c fixes three issues
>
> - fix code so it also works on big endian architectures
> - added a printk in case of an uncorrectable ecc error
> - strenghten the test for correctable errors (decreasing the chance
>   that multiple bit faults by accident will be seen as correctable)
>
> Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>
>
> diff --git a/drivers/mtd/nand/nand_ecc.c b/drivers/mtd/nand/nand_ecc.c
> index a8e8413..d99e569 100644

We can make this to support for 512 byte ecc calculation and correction also.
It can be helpful, particularly in cases where hardware ecc option is present and is using 512 byte ecc, to make software ecc and hardware ecc compatible. So that switching could be possible at run time i.e. both can be used by switching at load time.

I have prepared a patch on top of your latest patch.
Please review it and provide your comments on this.

----
Thanks and Regards,
vimal singh

Signed-off-by: Vimal Singh <vimalsingh@ti.com>

Index: linux-omap-2.6/drivers/mtd/nand/nand_ecc.c
===================================================================
--- linux-omap-2.6.orig/drivers/mtd/nand/nand_ecc.c	2008-08-21 10:33:30.000000000 +0530
+++ linux-omap-2.6/drivers/mtd/nand/nand_ecc.c	2008-08-21 10:50:39.000000000 +0530
@@ -42,6 +42,8 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
 #include <linux/mtd/nand_ecc.h>
 #include <asm/byteorder.h>
 #else
@@ -158,10 +160,11 @@
 {
 	int i;
 	const uint32_t *bp = (uint32_t *)buf;
+	/* 256 or 512 bytes/ecc  */
+	uint32_t j =(((struct nand_chip *)mtd->priv)->ecc.size)/256;
 	uint32_t cur;		/* current value in buffer */
-	/* rp0..rp15 are the various accumulated parities (per byte) */
+	/* rp0..rp15..rp17 are the various accumulated parities (per byte) */
 	uint32_t rp0, rp1, rp2, rp3, rp4, rp5, rp6, rp7;
-	uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15;
+	uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15, rp16, rp17;
 	uint32_t par;		/* the cumulative parity for all data */
 	uint32_t tmppar;	/* the cumulative parity for this iteration;
 				   for rp12 and rp14 at the end of the loop */
@@ -173,6 +176,8 @@
 	rp10 = 0;
 	rp12 = 0;
 	rp14 = 0;
+	if (j == 2)
+		rp16 = 0;
 
 	/*
 	 * The loop is unrolled a number of times;
@@ -184,7 +189,7 @@
 	 * needed for calculating rp12, rp14 and par
 	 * also used as a performance improvement for rp6, rp8 and rp10
 	 */
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < 4*j; i++) {
 		cur = *bp++;
 		tmppar = cur;
 		rp4 ^= cur;
@@ -247,6 +252,8 @@
 			rp12 ^= tmppar;
 		if ((i & 0x2) == 0)
 			rp14 ^= tmppar;
+		if (j == 2 && (i & 0x4) == 0)
+			rp16 ^= tmppar;
 	}
 
 	/*
@@ -273,6 +280,11 @@
 	rp14 ^= (rp14 >> 16);
 	rp14 ^= (rp14 >> 8);
 	rp14 &= 0xff;
+	if (j == 2) {
+		rp16 ^= (rp16 >> 16);
+		rp16 ^= (rp16 >> 8);
+		rp16 &= 0xff;
+	}
 
 	/*
 	 * we also need to calculate the row parity for rp0..rp3
@@ -329,6 +341,8 @@
 	rp11 = (par ^ rp10) & 0xff;
 	rp13 = (par ^ rp12) & 0xff;
 	rp15 = (par ^ rp14) & 0xff;
+	if (j == 2)
+		rp17 = (par ^ rp16) & 0xff;
 
 	/*
 	 * Finally calculate the ecc bits.
@@ -375,14 +389,25 @@
 	    (invparity[rp9] << 1)  |
 	    (invparity[rp8]);
 #endif
-	code[2] =
-	    (invparity[par & 0xf0] << 7) |
-	    (invparity[par & 0x0f] << 6) |
-	    (invparity[par & 0xcc] << 5) |
-	    (invparity[par & 0x33] << 4) |
-	    (invparity[par & 0xaa] << 3) |
-	    (invparity[par & 0x55] << 2) |
-	    3;
+	if (j == 1)
+		code[2] =
+		    (invparity[par & 0xf0] << 7) |
+		    (invparity[par & 0x0f] << 6) |
+		    (invparity[par & 0xcc] << 5) |
+		    (invparity[par & 0x33] << 4) |
+		    (invparity[par & 0xaa] << 3) |
+		    (invparity[par & 0x55] << 2) |
+		    3;
+	else if (j == 2)
+		code[2] =
+		    (invparity[par & 0xf0] << 7) |
+		    (invparity[par & 0x0f] << 6) |
+		    (invparity[par & 0xcc] << 5) |
+		    (invparity[par & 0x33] << 4) |
+		    (invparity[par & 0xaa] << 3) |
+		    (invparity[par & 0x55] << 2) |
+		    (invparity[rp17] << 1) |
+		    (invparity[rp16] << 0);
 	return 0;
 }
 EXPORT_SYMBOL(nand_calculate_ecc);
@@ -401,6 +426,7 @@
 {
 	unsigned char b0, b1, b2;
 	unsigned char byte_addr, bit_addr;
+	/* 256 or 512 bytes/ecc  */
+	uint32_t j = (((struct nand_chip *)mtd->priv)->ecc.size)/256;
 
 	/*
 	 * b0 to b2 indicate which bit is faulty (if any)
@@ -426,7 +452,8 @@
 
 	if ((((b0 ^ (b0 >> 1)) & 0x55) == 0x55) &&
 	    (((b1 ^ (b1 >> 1)) & 0x55) == 0x55) &&
-	    (((b2 ^ (b2 >> 1)) & 0x54) == 0x54)) { /* single bit error */
+	    ((j == 1 && ((b2 ^ (b2 >> 1)) & 0x54) == 0x54) ||
+	     (j == 2 && ((b2 ^ (b2 >> 1)) & 0x55) == 0x55))) {
+	/* single bit error */
 		/*
 		 * rp15/13/11/9/7/5/3/1 indicate which byte is the faulty byte
 		 * cp 5/3/1 indicate the faulty bit.
@@ -443,7 +470,10 @@
 		 * We could also do addressbits[b2] >> 1 but for the
 		 * performace it does not make any difference
 		 */
-		byte_addr = (addressbits[b1] << 4) + addressbits[b0];
+		if (j == 1)
+			byte_addr = (addressbits[b1] << 4) + addressbits[b0];
+		else if (j == 2)
+			byte_addr = (addressbits[b2 & 0x3] << 8) + 
+				    (addressbits[b1] << 4) + addressbits[b0];
 		bit_addr = addressbits[b2 >> 2];
 		/* flip the bit */
 		buf[byte_addr] ^= (1 << bit_addr);

RE: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

[Artem Bityutskiy]

Hi,

> +	/* 256 or 512 bytes/ecc  */
> +	uint32_t j =(((struct nand_chip *)mtd->priv)->ecc.size)/256;

It's certainly faster to use >> 8 instead. At least on x86 GCC would not
optimize this for you if the kernel is compiles with -Os, it would use
idiv instead.

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

Re: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

[Frans Meulenbroeks]

Hi Vimal,

Thanks for your patch.
I tried to apply your patch, but only hunk 1 succeeded with fuzz 2.
Guess this is because shortly after submitting my patch another patch
has been applied to it.
Could you verify against the latest version in git and resubmit the patch.

Also there are a few small optimisation possibilities.
E.g. the code:
+       if (j == 2)
+               rp16 = 0;
Here the if statement does not really add. for 256 byte ecc
initialising rp16 does not harm so the if statement creates additional
overhead.

There are a fw more of these (like the /256 reported by Artem.
I'll happily fix these once I have a patch that I can apply.

Best regards, Frans

RE: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

[Singh, Vimal]

Hi Frans,

>I tried to apply your patch, but only hunk 1 succeeded with fuzz 2.
>Guess this is because shortly after submitting my patch another patch
>has been applied to it.
>Could you verify against the latest version in git and resubmit the patch.

I have prepared the patch once again on top of latest version in git.
Hopefully it should apply properly.

Regards,
vimal

Signed-off-by: Vimal Singh <vimalsingh@ti.com>

-----
 nand_ecc.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 49 insertions(+), 13 deletions(-)
--- a/drivers/mtd/nand/nand_ecc.c       2008-08-22 10:41:53.000000000 +0530
+++ b/drivers/mtd/nand/nand_ecc.c       2008-08-22 10:47:40.000000000 +0530
@@ -42,6 +42,8 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
 #include <linux/mtd/nand_ecc.h>
 #else
 #include <stdint.h>
@@ -154,10 +156,12 @@ int nand_calculate_ecc(struct mtd_info *
 {
        int i;
        const uint32_t *bp = (uint32_t *)buf;
+       /* 256 or 512 bytes/ecc  */
+       uint32_t j =(((struct nand_chip *)mtd->priv)->ecc.size) >> 8;
        uint32_t cur;           /* current value in buffer */
-       /* rp0..rp15 are the various accumulated parities (per byte) */
+       /* rp0..rp15..rp17 are the various accumulated parities (per byte) */
        uint32_t rp0, rp1, rp2, rp3, rp4, rp5, rp6, rp7;
-       uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15;
+       uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15, rp16, rp17;
        uint32_t par;           /* the cumulative parity for all data */
        uint32_t tmppar;        /* the cumulative parity for this iteration;
                                   for rp12 and rp14 at the end of the loop */
@@ -169,6 +173,8 @@ int nand_calculate_ecc(struct mtd_info *
        rp10 = 0;
        rp12 = 0;
        rp14 = 0;
+       rp16 = 0;
+       rp17 = 0; /* to make cmpiler happy */

        /*
         * The loop is unrolled a number of times;
@@ -180,7 +186,7 @@ int nand_calculate_ecc(struct mtd_info *
         * needed for calculating rp12, rp14 and par
         * also used as a performance improvement for rp6, rp8 and rp10
         */
-       for (i = 0; i < 4; i++) {
+       for (i = 0; i < 4*j; i++) {
                cur = *bp++;
                tmppar = cur;
                rp4 ^= cur;
@@ -243,6 +249,8 @@ int nand_calculate_ecc(struct mtd_info *
                        rp12 ^= tmppar;
                if ((i & 0x2) == 0)
                        rp14 ^= tmppar;
+               if (j == 2 && (i & 0x4) == 0)
+                       rp16 ^= tmppar;
        }

        /*
@@ -269,6 +277,11 @@ int nand_calculate_ecc(struct mtd_info *
        rp14 ^= (rp14 >> 16);
        rp14 ^= (rp14 >> 8);
        rp14 &= 0xff;
+       if (j == 2) {
+               rp16 ^= (rp16 >> 16);
+               rp16 ^= (rp16 >> 8);
+               rp16 &= 0xff;
+       }

        /*
         * we also need to calculate the row parity for rp0..rp3
@@ -311,6 +324,8 @@ int nand_calculate_ecc(struct mtd_info *
        rp11 = (par ^ rp10) & 0xff;
        rp13 = (par ^ rp12) & 0xff;
        rp15 = (par ^ rp14) & 0xff;
+       if (j == 2)
+               rp17 = (par ^ rp16) & 0xff;

        /*
         * Finally calculate the ecc bits.
@@ -357,14 +372,25 @@ int nand_calculate_ecc(struct mtd_info *
            (invparity[rp9] << 1)  |
            (invparity[rp8]);
 #endif
-       code[2] =
-           (invparity[par & 0xf0] << 7) |
-           (invparity[par & 0x0f] << 6) |
-           (invparity[par & 0xcc] << 5) |
-           (invparity[par & 0x33] << 4) |
-           (invparity[par & 0xaa] << 3) |
-           (invparity[par & 0x55] << 2) |
-           3;
+       if (j == 1)
+               code[2] =
+                   (invparity[par & 0xf0] << 7) |
+                   (invparity[par & 0x0f] << 6) |
+                   (invparity[par & 0xcc] << 5) |
+                   (invparity[par & 0x33] << 4) |
+                   (invparity[par & 0xaa] << 3) |
+                   (invparity[par & 0x55] << 2) |
+                   3;
+       else if (j == 2)
+               code[2] =
+                   (invparity[par & 0xf0] << 7) |
+                   (invparity[par & 0x0f] << 6) |
+                   (invparity[par & 0xcc] << 5) |
+                   (invparity[par & 0x33] << 4) |
+                   (invparity[par & 0xaa] << 3) |
+                   (invparity[par & 0x55] << 2) |
+                   (invparity[rp17] << 1) |
+                   (invparity[rp16] << 0);
        return 0;
 }
 EXPORT_SYMBOL(nand_calculate_ecc);
@@ -384,6 +410,8 @@ int nand_correct_data(struct mtd_info *m
        int nr_bits;
        unsigned char b0, b1, b2;
        unsigned char byte_addr, bit_addr;
+       /* 256 or 512 bytes/ecc  */
+       uint32_t j = (((struct nand_chip *)mtd->priv)->ecc.size) >> 8;

        /*
         * b0 to b2 indicate which bit is faulty (if any)
@@ -408,7 +436,11 @@ int nand_correct_data(struct mtd_info *m
        /* ordered in order of likelihood */
        if (nr_bits == 0)
                return 0;       /* no error */
-       if (nr_bits == 11) {    /* correctable error */
+       if ((((b0 ^ (b0 >> 1)) & 0x55) == 0x55) &&
+           (((b1 ^ (b1 >> 1)) & 0x55) == 0x55) &&
+           ((j == 1 && ((b2 ^ (b2 >> 1)) & 0x54) == 0x54) ||
+            (j == 2 && ((b2 ^ (b2 >> 1)) & 0x55) == 0x55))) {
+       /* single bit error */
                /*
                 * rp15/13/11/9/7/5/3/1 indicate which byte is the faulty byte
                 * cp 5/3/1 indicate the faulty bit.
@@ -425,7 +457,11 @@ int nand_correct_data(struct mtd_info *m
                 * We could also do addressbits[b2] >> 1 but for the
                 * performace it does not make any difference
                 */
-               byte_addr = (addressbits[b1] << 4) + addressbits[b0];
+               if (j == 1)
+                       byte_addr = (addressbits[b1] << 4) + addressbits[b0];
+               else if (j == 2)
+                       byte_addr = (addressbits[b2 & 0x3] << 8) +
+                                   (addressbits[b1] << 4) + addressbits[b0];
                bit_addr = addressbits[b2 >> 2];
                /* flip the bit */
                buf[byte_addr] ^= (1 << bit_addr);

RE: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

[Artem Bityutskiy]

On Fri, 2008-08-22 at 11:09 +0530, Singh, Vimal wrote:
> @@ -154,10 +156,12 @@ int nand_calculate_ecc(struct mtd_info *
>  {
>         int i;
>         const uint32_t *bp = (uint32_t *)buf;
> +       /* 256 or 512 bytes/ecc  */
> +       uint32_t j =(((struct nand_chip *)mtd->priv)->ecc.size) >> 8;

"j" is not descriptive. And I guess "const" modifier may be used for it.
Not sure it'll give any performance benefit, but it may affect gcc
optimizations. I would instead introduce:

const int eccsize_mult = ((struct nand_chip *)mtd->priv)->ecc.size >> 8;

>         uint32_t cur;           /* current value in buffer */
> -       /* rp0..rp15 are the various accumulated parities (per byte) */
> +       /* rp0..rp15..rp17 are the various accumulated parities (per byte) */
>         uint32_t rp0, rp1, rp2, rp3, rp4, rp5, rp6, rp7;
> -       uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15;
> +       uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15, rp16, rp17;
>         uint32_t par;           /* the cumulative parity for all data */
>         uint32_t tmppar;        /* the cumulative parity for this iteration;
>                                    for rp12 and rp14 at the end of the loop */
> @@ -169,6 +173,8 @@ int nand_calculate_ecc(struct mtd_info *
>         rp10 = 0;
>         rp12 = 0;
>         rp14 = 0;
> +       rp16 = 0;
> +       rp17 = 0; /* to make cmpiler happy */
> 
>         /*
>          * The loop is unrolled a number of times;
> @@ -180,7 +186,7 @@ int nand_calculate_ecc(struct mtd_info *
>          * needed for calculating rp12, rp14 and par
>          * also used as a performance improvement for rp6, rp8 and rp10
>          */
> -       for (i = 0; i < 4; i++) {
> +       for (i = 0; i < 4*j; i++) {
Here I would rather use:

for (i = 0; i < eccsize_mult << 2; i++) {

to avoid multiplication.

> +               code[2] =
> +                   (invparity[par & 0xf0] << 7) |
> +                   (invparity[par & 0x0f] << 6) |
> +                   (invparity[par & 0xcc] << 5) |
> +                   (invparity[par & 0x33] << 4) |
> +                   (invparity[par & 0xaa] << 3) |
> +                   (invparity[par & 0x55] << 2) |
> +                   3;
> +       else if (j == 2)
this should be just else, not else if.

> +               code[2] =
> +                   (invparity[par & 0xf0] << 7) |
> +                   (invparity[par & 0x0f] << 6) |
> +                   (invparity[par & 0xcc] << 5) |
> +                   (invparity[par & 0x33] << 4) |
> +                   (invparity[par & 0xaa] << 3) |
> +                   (invparity[par & 0x55] << 2) |
> +                   (invparity[rp17] << 1) |
> +                   (invparity[rp16] << 0);


> -               byte_addr = (addressbits[b1] << 4) + addressbits[b0];
> +               if (j == 1)
> +                       byte_addr = (addressbits[b1] << 4) + addressbits[b0];
> +               else if (j == 2)
> +                       byte_addr = (addressbits[b2 & 0x3] << 8) +
> +                                   (addressbits[b1] << 4) + addressbits[b0];
Similar.

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

RE: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

[Artem Bityutskiy]

On Fri, 2008-08-22 at 11:09 +0530, Singh, Vimal wrote:
> +       rp16 = 0;
> +       rp17 = 0; /* to make cmpiler happy */
Please, use

uint32_t uninitialized_var(rp17);

to avoid unnecessary initialization to 0 and avoid gcc warning as well.

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

Re: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

[Frans Meulenbroeks]

@Vimal: I'll try to look at your patch this weekend (no time this evening)
Let me know if you want to handle the remarks from Artem (and come
with a new patch) or if you want me to handle them (there are maybe
one or two other small changes I want to make.

@David (dwmw2)
Please let me know what the preferred way to integrate this. I can
either provide a patch on top of Vimal' patch if needed, or I can add
my changes to it and submit a patch containing the work of both of us,
or I can send my changes as review comment to Vimal and let him make
the patch. What is the preferred way? (and I highly prefer a solution
where Vimal gets credit for his changes)

Frans.

RE: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

[Singh, Vimal]

>On Fri, 2008-08-22 at 11:09 +0530, Singh, Vimal wrote:
>> +       rp16 = 0;
> >+       rp17 = 0; /* to make cmpiler happy */
>Please, use
>
>uint32_t uninitialized_var(rp17);
>
>to avoid unnecessary initialization to 0 and avoid gcc warning as well.

Here is the patch after taking your inputs with some additional changes
in comments.

----
Thanks and Regards,
vimal singh

Signed-off-by: Vimal Singh <vimalsingh@ti.com>
---
 drivers/mtd/nand/nand_ecc.c |   84 ++++++++++++++++++++++++++++++++------------
 1 files changed, 62 insertions(+), 22 deletions(-)

Index: linux-omap-2.6/drivers/mtd/nand/nand_ecc.c
===================================================================
--- linux-omap-2.6.orig/drivers/mtd/nand/nand_ecc.c	2008-08-22 12:58:33.000000000 +0530
+++ linux-omap-2.6/drivers/mtd/nand/nand_ecc.c	2008-08-22 13:10:10.000000000 +0530
@@ -42,6 +42,8 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
 #include <linux/mtd/nand_ecc.h>
 #else
 #include <stdint.h>
@@ -144,7 +146,8 @@
 };
 
 /**
- * nand_calculate_ecc - [NAND Interface] Calculate 3-byte ECC for 256-byte block
+ * nand_calculate_ecc - [NAND Interface] Calculate 3-byte ECC for 256/512-byte
+ *			 block
  * @mtd:	MTD block structure (unused)
  * @dat:	raw data
  * @ecc_code:	buffer for ECC
@@ -154,13 +157,18 @@
 {
 	int i;
 	const uint32_t *bp = (uint32_t *)buf;
+	/* 256 or 512 bytes/ecc  */
+	const uint32_t eccsize_mult =
+			(((struct nand_chip *)mtd->priv)->ecc.size) >> 8;
 	uint32_t cur;		/* current value in buffer */
-	/* rp0..rp15 are the various accumulated parities (per byte) */
+	/* rp0..rp15..rp17 are the various accumulated parities (per byte) */
 	uint32_t rp0, rp1, rp2, rp3, rp4, rp5, rp6, rp7;
-	uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15;
+	uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15, rp16;
+	uint32_t uninitialized_var(rp17);	/* to keep compiler happy */
 	uint32_t par;		/* the cumulative parity for all data */
 	uint32_t tmppar;	/* the cumulative parity for this iteration;
-				   for rp12 and rp14 at the end of the loop */
+				   for rp12, rp14 and rp16 at the end of the
+				   loop */
 
 	par = 0;
 	rp4 = 0;
@@ -169,6 +177,7 @@
 	rp10 = 0;
 	rp12 = 0;
 	rp14 = 0;
+	rp16 = 0;
 
 	/*
 	 * The loop is unrolled a number of times;
@@ -177,10 +186,10 @@
 	 * Note: passing unaligned data might give a performance penalty.
 	 * It is assumed that the buffers are aligned.
 	 * tmppar is the cumulative sum of this iteration.
-	 * needed for calculating rp12, rp14 and par
+	 * needed for calculating rp12, rp14, rp16 and par
 	 * also used as a performance improvement for rp6, rp8 and rp10
 	 */
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < eccsize_mult << 2; i++) {
 		cur = *bp++;
 		tmppar = cur;
 		rp4 ^= cur;
@@ -243,12 +252,14 @@
 			rp12 ^= tmppar;
 		if ((i & 0x2) == 0)
 			rp14 ^= tmppar;
+		if (eccsize_mult == 2 && (i & 0x4) == 0)
+			rp16 ^= tmppar;
 	}
 
 	/*
 	 * handle the fact that we use longword operations
-	 * we'll bring rp4..rp14 back to single byte entities by shifting and
-	 * xoring first fold the upper and lower 16 bits,
+	 * we'll bring rp4..rp14..rp16 back to single byte entities by
+	 * shifting and xoring first fold the upper and lower 16 bits,
 	 * then the upper and lower 8 bits.
 	 */
 	rp4 ^= (rp4 >> 16);
@@ -269,6 +280,11 @@
 	rp14 ^= (rp14 >> 16);
 	rp14 ^= (rp14 >> 8);
 	rp14 &= 0xff;
+	if (eccsize_mult == 2) {
+		rp16 ^= (rp16 >> 16);
+		rp16 ^= (rp16 >> 8);
+		rp16 &= 0xff;
+	}
 
 	/*
 	 * we also need to calculate the row parity for rp0..rp3
@@ -297,7 +313,7 @@
 	par &= 0xff;
 
 	/*
-	 * and calculate rp5..rp15
+	 * and calculate rp5..rp15..rp17
 	 * note that par = rp4 ^ rp5 and due to the commutative property
 	 * of the ^ operator we can say:
 	 * rp5 = (par ^ rp4);
@@ -311,6 +327,8 @@
 	rp11 = (par ^ rp10) & 0xff;
 	rp13 = (par ^ rp12) & 0xff;
 	rp15 = (par ^ rp14) & 0xff;
+	if (eccsize_mult == 2)
+		rp17 = (par ^ rp16) & 0xff;
 
 	/*
 	 * Finally calculate the ecc bits.
@@ -357,14 +375,25 @@
 	    (invparity[rp9] << 1)  |
 	    (invparity[rp8]);
 #endif
-	code[2] =
-	    (invparity[par & 0xf0] << 7) |
-	    (invparity[par & 0x0f] << 6) |
-	    (invparity[par & 0xcc] << 5) |
-	    (invparity[par & 0x33] << 4) |
-	    (invparity[par & 0xaa] << 3) |
-	    (invparity[par & 0x55] << 2) |
-	    3;
+	if (eccsize_mult == 1)
+		code[2] =
+		    (invparity[par & 0xf0] << 7) |
+		    (invparity[par & 0x0f] << 6) |
+		    (invparity[par & 0xcc] << 5) |
+		    (invparity[par & 0x33] << 4) |
+		    (invparity[par & 0xaa] << 3) |
+		    (invparity[par & 0x55] << 2) |
+		    3;
+	else
+		code[2] =
+		    (invparity[par & 0xf0] << 7) |
+		    (invparity[par & 0x0f] << 6) |
+		    (invparity[par & 0xcc] << 5) |
+		    (invparity[par & 0x33] << 4) |
+		    (invparity[par & 0xaa] << 3) |
+		    (invparity[par & 0x55] << 2) |
+		    (invparity[rp17] << 1) |
+		    (invparity[rp16] << 0);
 	return 0;
 }
 EXPORT_SYMBOL(nand_calculate_ecc);
@@ -376,7 +405,7 @@
  * @read_ecc:	ECC from the chip
  * @calc_ecc:	the ECC calculated from raw data
  *
- * Detect and correct a 1 bit error for 256 byte block
+ * Detect and correct a 1 bit error for 256/512 byte block
  */
 int nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
 		      unsigned char *read_ecc, unsigned char *calc_ecc)
@@ -384,6 +413,9 @@
 	int nr_bits;
 	unsigned char b0, b1, b2;
 	unsigned char byte_addr, bit_addr;
+	/* 256 or 512 bytes/ecc  */
+	const uint32_t eccsize_mult =
+			(((struct nand_chip *)mtd->priv)->ecc.size) >> 8;
 
 	/*
 	 * b0 to b2 indicate which bit is faulty (if any)
@@ -408,10 +440,14 @@
 	/* ordered in order of likelihood */
 	if (nr_bits == 0)
 		return 0;	/* no error */
-	if (nr_bits == 11) {	/* correctable error */
+ 	if ((((b0 ^ (b0 >> 1)) & 0x55) == 0x55) &&
+ 	    (((b1 ^ (b1 >> 1)) & 0x55) == 0x55) &&
+	    ((eccsize_mult == 1 && ((b2 ^ (b2 >> 1)) & 0x54) == 0x54) ||
+	     (eccsize_mult == 2 && ((b2 ^ (b2 >> 1)) & 0x55) == 0x55))) {
+	/* single bit error */
 		/*
-		 * rp15/13/11/9/7/5/3/1 indicate which byte is the faulty byte
-		 * cp 5/3/1 indicate the faulty bit.
+		 * rp17/rp15/13/11/9/7/5/3/1 indicate which byte is the faulty
+		 * byte, cp 5/3/1 indicate the faulty bit.
 		 * A lookup table (called addressbits) is used to filter
 		 * the bits from the byte they are in.
 		 * A marginal optimisation is possible by having three
@@ -425,7 +461,11 @@
 		 * We could also do addressbits[b2] >> 1 but for the
 		 * performace it does not make any difference
 		 */
-		byte_addr = (addressbits[b1] << 4) + addressbits[b0];
+		if (eccsize_mult == 1)
+			byte_addr = (addressbits[b1] << 4) + addressbits[b0];
+		else
+			byte_addr = (addressbits[b2 & 0x3] << 8) +
+				    (addressbits[b1] << 4) + addressbits[b0];
 		bit_addr = addressbits[b2 >> 2];
 		/* flip the bit */
 		buf[byte_addr] ^= (1 << bit_addr);

Re: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

[Frans Meulenbroeks]

Vimal,

This is not a diff against the latest git version.

E.g. this line

> -       if (nr_bits == 11) {    /* correctable error */

was already replaced by the 0x55 tests in this commit:
http://git.infradead.org/mtd-2.6.git?a=commitdiff;h=1077be58ad7baadd86e47e8b4f6209fa5b6364a5


Also in your diff I see that this patch:
http://git.infradead.org/mtd-2.6.git?a=commitdiff;h=17c1d2be28e485c0c8b09661db39d5bf2605069d
is not in the version you compare against.

I suggest to merge your changes with the mtd git version of nand_ecc.c
The current HEAD for this file can be found here:
http://git.infradead.org/mtd-2.6.git?a=blob_plain;f=drivers/mtd/nand/nand_ecc.c;hb=17c1d2be28e485c0c8b09661db39d5bf2605069d

Best regards, Frans.

Re: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

[David Woodhouse]

On Fri, 2008-08-22 at 10:43 +0200, Frans Meulenbroeks wrote:
> @David (dwmw2)
> Please let me know what the preferred way to integrate this. I can
> either provide a patch on top of Vimal' patch if needed, or I can add
> my changes to it and submit a patch containing the work of both of us,
> or I can send my changes as review comment to Vimal and let him make
> the patch. What is the preferred way? (and I highly prefer a solution
> where Vimal gets credit for his changes)

When you forward the patch on to me, put 'From: Vimal Singh <...>'  as
the first line of the _body_ of the email so that it is still attributed
to him. It's reasonable to preserve his signed-off-by, if you're only
making relatively minor changes, and document what _you_ changed in
square brackets. For an example, see
http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg00461.html

Alternatively, if his patch isn't actually introducing a regression, it
doesn't hurt to do it as two separate patches -- whichever you prefer.

When resending an updated patch, make sure the proper commit comment
remains intact -- if you want to add some commentary like 'fixed up
according to review and made some other fixes', which doesn't live in
the commit comment but follows on from the email thread, then add it
after a line with three dashes -- again, you can see that in the example
above.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

RE: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

[Singh, Vimal]

Frans,

Below is the patch prepared on top of the current git head.
 
>I suggest to merge your changes with the mtd git version of nand_ecc.c
>The current HEAD for this file can be found here:
>http://git.infradead.org/mtd-2.6.git?a=blob_plain;f=drivers/mtd/nand/nand_ecc.c;hb=17c1d2be28e485c0c8b09661db39d5bf2605069d

Sorry for sending patches on wrong versions...

---
 drivers/mtd/nand/nand_ecc.c |   82 ++++++++++++++++++++++++++++++++------------
 1 files changed, 60 insertions(+), 22 deletions(-)

Index: linux-omap-2.6/drivers/mtd/nand/nand_ecc.c
===================================================================
--- linux-omap-2.6.orig/drivers/mtd/nand/nand_ecc.c	2008-08-22 14:35:48.000000000 +0530
+++ linux-omap-2.6/drivers/mtd/nand/nand_ecc.c	2008-08-22 14:46:41.000000000 +0530
@@ -42,6 +42,8 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
 #include <linux/mtd/nand_ecc.h>
 #include <asm/byteorder.h>
 #else
@@ -148,7 +150,8 @@
 };
 
 /**
- * nand_calculate_ecc - [NAND Interface] Calculate 3-byte ECC for 256-byte block
+ * nand_calculate_ecc - [NAND Interface] Calculate 3-byte ECC for 256/512-byte
+ *			 block
  * @mtd:	MTD block structure (unused)
  * @buf:	input buffer with raw data
  * @code:	output buffer with ECC
@@ -158,13 +161,18 @@
 {
 	int i;
 	const uint32_t *bp = (uint32_t *)buf;
+	/* 256 or 512 bytes/ecc  */
+	const uint32_t eccsize_mult =
+			(((struct nand_chip *)mtd->priv)->ecc.size) >> 8;
 	uint32_t cur;		/* current value in buffer */
-	/* rp0..rp15 are the various accumulated parities (per byte) */
+	/* rp0..rp15..rp17 are the various accumulated parities (per byte) */
 	uint32_t rp0, rp1, rp2, rp3, rp4, rp5, rp6, rp7;
-	uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15;
+	uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15, rp16;
+	uint32_t uninitialized_var(rp17);	/* to make compiler happy */
 	uint32_t par;		/* the cumulative parity for all data */
 	uint32_t tmppar;	/* the cumulative parity for this iteration;
-				   for rp12 and rp14 at the end of the loop */
+				   for rp12, rp14 and rp16 at the end of the
+				   loop */
 
 	par = 0;
 	rp4 = 0;
@@ -173,6 +181,7 @@
 	rp10 = 0;
 	rp12 = 0;
 	rp14 = 0;
+	rp16 = 0;
 
 	/*
 	 * The loop is unrolled a number of times;
@@ -181,10 +190,10 @@
 	 * Note: passing unaligned data might give a performance penalty.
 	 * It is assumed that the buffers are aligned.
 	 * tmppar is the cumulative sum of this iteration.
-	 * needed for calculating rp12, rp14 and par
+	 * needed for calculating rp12, rp14, rp16 and par
 	 * also used as a performance improvement for rp6, rp8 and rp10
 	 */
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < eccsize_mult << 2; i++) {
 		cur = *bp++;
 		tmppar = cur;
 		rp4 ^= cur;
@@ -247,12 +256,14 @@
 			rp12 ^= tmppar;
 		if ((i & 0x2) == 0)
 			rp14 ^= tmppar;
+		if (eccsize_mult == 2 && (i & 0x4) == 0)
+			rp16 ^= tmppar;
 	}
 
 	/*
 	 * handle the fact that we use longword operations
-	 * we'll bring rp4..rp14 back to single byte entities by shifting and
-	 * xoring first fold the upper and lower 16 bits,
+	 * we'll bring rp4..rp14..rp16 back to single byte entities by
+	 * shifting and xoring first fold the upper and lower 16 bits,
 	 * then the upper and lower 8 bits.
 	 */
 	rp4 ^= (rp4 >> 16);
@@ -273,6 +284,11 @@
 	rp14 ^= (rp14 >> 16);
 	rp14 ^= (rp14 >> 8);
 	rp14 &= 0xff;
+	if (eccsize_mult == 2) {
+		rp16 ^= (rp16 >> 16);
+		rp16 ^= (rp16 >> 8);
+		rp16 &= 0xff;
+	}
 
 	/*
 	 * we also need to calculate the row parity for rp0..rp3
@@ -315,7 +331,7 @@
 	par &= 0xff;
 
 	/*
-	 * and calculate rp5..rp15
+	 * and calculate rp5..rp15..rp17
 	 * note that par = rp4 ^ rp5 and due to the commutative property
 	 * of the ^ operator we can say:
 	 * rp5 = (par ^ rp4);
@@ -329,6 +345,8 @@
 	rp11 = (par ^ rp10) & 0xff;
 	rp13 = (par ^ rp12) & 0xff;
 	rp15 = (par ^ rp14) & 0xff;
+	if (eccsize_mult == 2)
+		rp17 = (par ^ rp16) & 0xff;
 
 	/*
 	 * Finally calculate the ecc bits.
@@ -375,14 +393,25 @@
 	    (invparity[rp9] << 1)  |
 	    (invparity[rp8]);
 #endif
-	code[2] =
-	    (invparity[par & 0xf0] << 7) |
-	    (invparity[par & 0x0f] << 6) |
-	    (invparity[par & 0xcc] << 5) |
-	    (invparity[par & 0x33] << 4) |
-	    (invparity[par & 0xaa] << 3) |
-	    (invparity[par & 0x55] << 2) |
-	    3;
+	if (eccsize_mult == 1)
+		code[2] =
+		    (invparity[par & 0xf0] << 7) |
+		    (invparity[par & 0x0f] << 6) |
+		    (invparity[par & 0xcc] << 5) |
+		    (invparity[par & 0x33] << 4) |
+		    (invparity[par & 0xaa] << 3) |
+		    (invparity[par & 0x55] << 2) |
+		    3;
+	else
+		code[2] =
+		    (invparity[par & 0xf0] << 7) |
+		    (invparity[par & 0x0f] << 6) |
+		    (invparity[par & 0xcc] << 5) |
+		    (invparity[par & 0x33] << 4) |
+		    (invparity[par & 0xaa] << 3) |
+		    (invparity[par & 0x55] << 2) |
+		    (invparity[rp17] << 1) |
+		    (invparity[rp16] << 0);
 	return 0;
 }
 EXPORT_SYMBOL(nand_calculate_ecc);
@@ -394,13 +423,16 @@
  * @read_ecc:	ECC from the chip
  * @calc_ecc:	the ECC calculated from raw data
  *
- * Detect and correct a 1 bit error for 256 byte block
+ * Detect and correct a 1 bit error for 256/512 byte block
  */
 int nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
 		      unsigned char *read_ecc, unsigned char *calc_ecc)
 {
 	unsigned char b0, b1, b2;
 	unsigned char byte_addr, bit_addr;
+	/* 256 or 512 bytes/ecc  */
+	const uint32_t eccsize_mult =
+			(((struct nand_chip *)mtd->priv)->ecc.size) >> 8;
 
 	/*
 	 * b0 to b2 indicate which bit is faulty (if any)
@@ -426,10 +458,12 @@
 
 	if ((((b0 ^ (b0 >> 1)) & 0x55) == 0x55) &&
 	    (((b1 ^ (b1 >> 1)) & 0x55) == 0x55) &&
-	    (((b2 ^ (b2 >> 1)) & 0x54) == 0x54)) { /* single bit error */
+	    ((eccsize_mult == 1 && ((b2 ^ (b2 >> 1)) & 0x54) == 0x54) ||
+	     (eccsize_mult == 2 && ((b2 ^ (b2 >> 1)) & 0x55) == 0x55))) {
+	/* single bit error */
 		/*
-		 * rp15/13/11/9/7/5/3/1 indicate which byte is the faulty byte
-		 * cp 5/3/1 indicate the faulty bit.
+		 * rp17/rp15/13/11/9/7/5/3/1 indicate which byte is the faulty
+		 * byte, cp 5/3/1 indicate the faulty bit.
 		 * A lookup table (called addressbits) is used to filter
 		 * the bits from the byte they are in.
 		 * A marginal optimisation is possible by having three
@@ -443,7 +477,11 @@
 		 * We could also do addressbits[b2] >> 1 but for the
 		 * performace it does not make any difference
 		 */
-		byte_addr = (addressbits[b1] << 4) + addressbits[b0];
+		if (eccsize_mult == 1)
+			byte_addr = (addressbits[b1] << 4) + addressbits[b0];
+		else
+			byte_addr = (addressbits[b2 & 0x3] << 8) +
+				    (addressbits[b1] << 4) + addressbits[b0];
 		bit_addr = addressbits[b2 >> 2];
 		/* flip the bit */
 		buf[byte_addr] ^= (1 << bit_addr);

RE: [PATCH] [MTD] [NAND] nand_ecc.c: adding support for 512 byte ecc

[frans]

From: "Singh, Vimal" <vimalsingh@ti.com>

Support 512 byte ECC calculation

[FM: updated two comments]

Signed-off-by: Vimal Singh <vimalsingh@ti.com>
Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>

---
recreated the patch after making the changes
there are a few other minor optimisations possible, but I am planning to
rework the code and rebench for ARM (there are a few statements
like rp2 = (par ^ rp3) & 0xff; which were marginally faster on pentium
but might be less efficient on ARM and/or MIPS. As this is typically used
for embedded processors it makes sense to optimise for those targets
(especially if the effect for x86 is hardly or not noticable)

Frans.

diff --git a/drivers/mtd/nand/nand_ecc.c b/drivers/mtd/nand/nand_ecc.c
index fd19787..868147a 100644
--- a/drivers/mtd/nand/nand_ecc.c
+++ b/drivers/mtd/nand/nand_ecc.c
@@ -42,6 +42,8 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
 #include <linux/mtd/nand_ecc.h>
 #include <asm/byteorder.h>
 #else
@@ -148,8 +150,9 @@ static const char addressbits[256] = {
 };

 /**
- * nand_calculate_ecc - [NAND Interface] Calculate 3-byte ECC for 256-byte block
- * @mtd:	MTD block structure (unused)
+ * nand_calculate_ecc - [NAND Interface] Calculate 3-byte ECC for 256/512-byte
+ *			 block
+ * @mtd:	MTD block structure
  * @buf:	input buffer with raw data
  * @code:	output buffer with ECC
  */
@@ -158,13 +161,18 @@ int nand_calculate_ecc(struct mtd_info *mtd, const unsigned char *buf,
 {
 	int i;
 	const uint32_t *bp = (uint32_t *)buf;
+	/* 256 or 512 bytes/ecc  */
+	const uint32_t eccsize_mult =
+			(((struct nand_chip *)mtd->priv)->ecc.size) >> 8;
 	uint32_t cur;		/* current value in buffer */
-	/* rp0..rp15 are the various accumulated parities (per byte) */
+	/* rp0..rp15..rp17 are the various accumulated parities (per byte) */
 	uint32_t rp0, rp1, rp2, rp3, rp4, rp5, rp6, rp7;
-	uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15;
+	uint32_t rp8, rp9, rp10, rp11, rp12, rp13, rp14, rp15, rp16;
+	uint32_t uninitialized_var(rp17);	/* to make compiler happy */
 	uint32_t par;		/* the cumulative parity for all data */
 	uint32_t tmppar;	/* the cumulative parity for this iteration;
-				   for rp12 and rp14 at the end of the loop */
+				   for rp12, rp14 and rp16 at the end of the
+				   loop */

 	par = 0;
 	rp4 = 0;
@@ -173,6 +181,7 @@ int nand_calculate_ecc(struct mtd_info *mtd, const unsigned char *buf,
 	rp10 = 0;
 	rp12 = 0;
 	rp14 = 0;
+	rp16 = 0;

 	/*
 	 * The loop is unrolled a number of times;
@@ -181,10 +190,10 @@ int nand_calculate_ecc(struct mtd_info *mtd, const unsigned char *buf,
 	 * Note: passing unaligned data might give a performance penalty.
 	 * It is assumed that the buffers are aligned.
 	 * tmppar is the cumulative sum of this iteration.
-	 * needed for calculating rp12, rp14 and par
+	 * needed for calculating rp12, rp14, rp16 and par
 	 * also used as a performance improvement for rp6, rp8 and rp10
 	 */
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < eccsize_mult << 2; i++) {
 		cur = *bp++;
 		tmppar = cur;
 		rp4 ^= cur;
@@ -247,12 +256,14 @@ int nand_calculate_ecc(struct mtd_info *mtd, const unsigned char *buf,
 			rp12 ^= tmppar;
 		if ((i & 0x2) == 0)
 			rp14 ^= tmppar;
+		if (eccsize_mult == 2 && (i & 0x4) == 0)
+			rp16 ^= tmppar;
 	}

 	/*
 	 * handle the fact that we use longword operations
-	 * we'll bring rp4..rp14 back to single byte entities by shifting and
-	 * xoring first fold the upper and lower 16 bits,
+	 * we'll bring rp4..rp14..rp16 back to single byte entities by
+	 * shifting and xoring first fold the upper and lower 16 bits,
 	 * then the upper and lower 8 bits.
 	 */
 	rp4 ^= (rp4 >> 16);
@@ -273,6 +284,11 @@ int nand_calculate_ecc(struct mtd_info *mtd, const unsigned char *buf,
 	rp14 ^= (rp14 >> 16);
 	rp14 ^= (rp14 >> 8);
 	rp14 &= 0xff;
+	if (eccsize_mult == 2) {
+		rp16 ^= (rp16 >> 16);
+		rp16 ^= (rp16 >> 8);
+		rp16 &= 0xff;
+	}

 	/*
 	 * we also need to calculate the row parity for rp0..rp3
@@ -315,7 +331,7 @@ int nand_calculate_ecc(struct mtd_info *mtd, const unsigned char *buf,
 	par &= 0xff;

 	/*
-	 * and calculate rp5..rp15
+	 * and calculate rp5..rp15..rp17
 	 * note that par = rp4 ^ rp5 and due to the commutative property
 	 * of the ^ operator we can say:
 	 * rp5 = (par ^ rp4);
@@ -329,6 +345,8 @@ int nand_calculate_ecc(struct mtd_info *mtd, const unsigned char *buf,
 	rp11 = (par ^ rp10) & 0xff;
 	rp13 = (par ^ rp12) & 0xff;
 	rp15 = (par ^ rp14) & 0xff;
+	if (eccsize_mult == 2)
+		rp17 = (par ^ rp16) & 0xff;

 	/*
 	 * Finally calculate the ecc bits.
@@ -375,32 +393,46 @@ int nand_calculate_ecc(struct mtd_info *mtd, const unsigned char *buf,
 	    (invparity[rp9] << 1)  |
 	    (invparity[rp8]);
 #endif
-	code[2] =
-	    (invparity[par & 0xf0] << 7) |
-	    (invparity[par & 0x0f] << 6) |
-	    (invparity[par & 0xcc] << 5) |
-	    (invparity[par & 0x33] << 4) |
-	    (invparity[par & 0xaa] << 3) |
-	    (invparity[par & 0x55] << 2) |
-	    3;
+	if (eccsize_mult == 1)
+		code[2] =
+		    (invparity[par & 0xf0] << 7) |
+		    (invparity[par & 0x0f] << 6) |
+		    (invparity[par & 0xcc] << 5) |
+		    (invparity[par & 0x33] << 4) |
+		    (invparity[par & 0xaa] << 3) |
+		    (invparity[par & 0x55] << 2) |
+		    3;
+	else
+		code[2] =
+		    (invparity[par & 0xf0] << 7) |
+		    (invparity[par & 0x0f] << 6) |
+		    (invparity[par & 0xcc] << 5) |
+		    (invparity[par & 0x33] << 4) |
+		    (invparity[par & 0xaa] << 3) |
+		    (invparity[par & 0x55] << 2) |
+		    (invparity[rp17] << 1) |
+		    (invparity[rp16] << 0);
 	return 0;
 }
 EXPORT_SYMBOL(nand_calculate_ecc);

 /**
  * nand_correct_data - [NAND Interface] Detect and correct bit error(s)
- * @mtd:	MTD block structure (unused)
+ * @mtd:	MTD block structure
  * @buf:	raw data read from the chip
  * @read_ecc:	ECC from the chip
  * @calc_ecc:	the ECC calculated from raw data
  *
- * Detect and correct a 1 bit error for 256 byte block
+ * Detect and correct a 1 bit error for 256/512 byte block
  */
 int nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
 		      unsigned char *read_ecc, unsigned char *calc_ecc)
 {
 	unsigned char b0, b1, b2;
 	unsigned char byte_addr, bit_addr;
+	/* 256 or 512 bytes/ecc  */
+	const uint32_t eccsize_mult =
+			(((struct nand_chip *)mtd->priv)->ecc.size) >> 8;

 	/*
 	 * b0 to b2 indicate which bit is faulty (if any)
@@ -426,10 +458,12 @@ int nand_correct_data(struct mtd_info *mtd, unsigned char *buf,

 	if ((((b0 ^ (b0 >> 1)) & 0x55) == 0x55) &&
 	    (((b1 ^ (b1 >> 1)) & 0x55) == 0x55) &&
-	    (((b2 ^ (b2 >> 1)) & 0x54) == 0x54)) { /* single bit error */
+	    ((eccsize_mult == 1 && ((b2 ^ (b2 >> 1)) & 0x54) == 0x54) ||
+	     (eccsize_mult == 2 && ((b2 ^ (b2 >> 1)) & 0x55) == 0x55))) {
+	/* single bit error */
 		/*
-		 * rp15/13/11/9/7/5/3/1 indicate which byte is the faulty byte
-		 * cp 5/3/1 indicate the faulty bit.
+		 * rp17/rp15/13/11/9/7/5/3/1 indicate which byte is the faulty
+		 * byte, cp 5/3/1 indicate the faulty bit.
 		 * A lookup table (called addressbits) is used to filter
 		 * the bits from the byte they are in.
 		 * A marginal optimisation is possible by having three
@@ -443,7 +477,11 @@ int nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
 		 * We could also do addressbits[b2] >> 1 but for the
 		 * performace it does not make any difference
 		 */
-		byte_addr = (addressbits[b1] << 4) + addressbits[b0];
+		if (eccsize_mult == 1)
+			byte_addr = (addressbits[b1] << 4) + addressbits[b0];
+		else
+			byte_addr = (addressbits[b2 & 0x3] << 8) +
+				    (addressbits[b1] << 4) + addressbits[b0];
 		bit_addr = addressbits[b2 >> 2];
 		/* flip the bit */
 		buf[byte_addr] ^= (1 << bit_addr);