[PATCH] MTD: fix s3c2410 error correction

[PATCH] MTD: fix s3c2410 error correction

[Matt Reimer]

From: Matt Reimer <mreimer@vpop.net>

The single-bit error correction was, well, incorrect. For determing which
bit to correct it was using P1' P2' P4' P8' instead of P1 P2 P4 P8, and
it was using P16' P32' P64' P128' P256' P512' P1024' P2048' instead of
P16 P32 P64 P128 P256 P512 P1024 P2048.

Signed-off-by: Matt Reimer <mreimer@vpop.net>
---
 drivers/mtd/nand/s3c2410.c |   26 ++++++++++++--------------
 1 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/s3c2410.c b/drivers/mtd/nand/s3c2410.c
index 903db18..077fdcc 100644
--- a/drivers/mtd/nand/s3c2410.c
+++ b/drivers/mtd/nand/s3c2410.c
@@ -364,23 +364,21 @@ static int s3c2410_nand_correct_data(struct mtd_info *mtd, u_char *dat,
 	    ((diff2 ^ (diff2 >> 1)) & 0x55) == 0x55) {
 		/* calculate the bit position of the error */
 
-		bit  = (diff2 >> 2) & 1;
-		bit |= (diff2 >> 3) & 2;
-		bit |= (diff2 >> 4) & 4;
+		bit  = ((diff2 >> 3) & 1) |
+		       ((diff2 >> 4) & 2) |
+		       ((diff2 >> 5) & 4);
 
 		/* calculate the byte position of the error */
 
-		byte  = (diff1 << 1) & 0x80;
-		byte |= (diff1 << 2) & 0x40;
-		byte |= (diff1 << 3) & 0x20;
-		byte |= (diff1 << 4) & 0x10;
-
-		byte |= (diff0 >> 3) & 0x08;
-		byte |= (diff0 >> 2) & 0x04;
-		byte |= (diff0 >> 1) & 0x02;
-		byte |= (diff0 >> 0) & 0x01;
-
-		byte |= (diff2 << 8) & 0x100;
+		byte = ((diff2 << 7) & 0x100) |
+		       ((diff1 << 0) & 0x80)  |
+		       ((diff1 << 1) & 0x40)  |
+		       ((diff1 << 2) & 0x20)  |
+		       ((diff1 << 3) & 0x10)  |
+		       ((diff0 >> 4) & 0x08)  |
+		       ((diff0 >> 3) & 0x04)  |
+		       ((diff0 >> 2) & 0x02)  |
+		       ((diff0 >> 1) & 0x01);
 
 		dev_dbg(info->device, "correcting error bit %d, byte %d\n",
 			bit, byte);
-- 
1.5.3.2

[PATCH] MTD: make s3c2410's correct_data indicate an error for multi-bit read errors

[Matt Reimer]

From: Matt Reimer <mreimer@vpop.net>

If there were multiple bit errors in the data s3c2410_nand_correct_data()
was returning 0 (no error) instead of -1, so the upper layers (like JFFS2)
would not know the data is corrupt.

Signed-off-by: Matt Reimer <mreimer@vpop.net>
---
 drivers/mtd/nand/s3c2410.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/s3c2410.c b/drivers/mtd/nand/s3c2410.c
index 077fdcc..2e6b435 100644
--- a/drivers/mtd/nand/s3c2410.c
+++ b/drivers/mtd/nand/s3c2410.c
@@ -397,7 +397,7 @@ static int s3c2410_nand_correct_data(struct mtd_info *mtd, u_char *dat,
 	if ((diff0 & ~(1<<fls(diff0))) == 0)
 		return 1;
 
-	return 0;
+	return -1;
 }
 
 /* ECC functions
-- 
1.5.3.2

Re: [PATCH] MTD: fix s3c2410 error correction

[Matt Reimer]

Anybody willing to ACK this patch? It definitely fixes the problem I'm
seeing on a s3c2440a. For more information, compare the s3c2440a or
s3c2410 manuals with page 8 of the following documentation:

http://www.samsung.com/global/business/semiconductor/products/flash/downloads/applicationnote/ecc_algorithm_for_web_512b.pdf

Matt


On 10/18/07, Matt Reimer <mattjreimer@gmail.com> wrote:
> From: Matt Reimer <mreimer@vpop.net>
>
> The single-bit error correction was, well, incorrect. For determing which
> bit to correct it was using P1' P2' P4' P8' instead of P1 P2 P4 P8, and
> it was using P16' P32' P64' P128' P256' P512' P1024' P2048' instead of
> P16 P32 P64 P128 P256 P512 P1024 P2048.
>
> Signed-off-by: Matt Reimer <mreimer@vpop.net>
> ---
>  drivers/mtd/nand/s3c2410.c |   26 ++++++++++++--------------
>  1 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mtd/nand/s3c2410.c b/drivers/mtd/nand/s3c2410.c
> index 903db18..077fdcc 100644
> --- a/drivers/mtd/nand/s3c2410.c
> +++ b/drivers/mtd/nand/s3c2410.c
> @@ -364,23 +364,21 @@ static int s3c2410_nand_correct_data(struct mtd_info *mtd, u_char *dat,
>             ((diff2 ^ (diff2 >> 1)) & 0x55) == 0x55) {
>                 /* calculate the bit position of the error */
>
> -               bit  = (diff2 >> 2) & 1;
> -               bit |= (diff2 >> 3) & 2;
> -               bit |= (diff2 >> 4) & 4;
> +               bit  = ((diff2 >> 3) & 1) |
> +                      ((diff2 >> 4) & 2) |
> +                      ((diff2 >> 5) & 4);
>
>                 /* calculate the byte position of the error */
>
> -               byte  = (diff1 << 1) & 0x80;
> -               byte |= (diff1 << 2) & 0x40;
> -               byte |= (diff1 << 3) & 0x20;
> -               byte |= (diff1 << 4) & 0x10;
> -
> -               byte |= (diff0 >> 3) & 0x08;
> -               byte |= (diff0 >> 2) & 0x04;
> -               byte |= (diff0 >> 1) & 0x02;
> -               byte |= (diff0 >> 0) & 0x01;
> -
> -               byte |= (diff2 << 8) & 0x100;
> +               byte = ((diff2 << 7) & 0x100) |
> +                      ((diff1 << 0) & 0x80)  |
> +                      ((diff1 << 1) & 0x40)  |
> +                      ((diff1 << 2) & 0x20)  |
> +                      ((diff1 << 3) & 0x10)  |
> +                      ((diff0 >> 4) & 0x08)  |
> +                      ((diff0 >> 3) & 0x04)  |
> +                      ((diff0 >> 2) & 0x02)  |
> +                      ((diff0 >> 1) & 0x01);
>
>                 dev_dbg(info->device, "correcting error bit %d, byte %d\n",
>                         bit, byte);
> --
> 1.5.3.2
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Re: [PATCH] MTD: fix s3c2410 error correction

[Matt Reimer]

ping

On 10/25/07, Matt Reimer <mattjreimer@gmail.com> wrote:
> Anybody willing to ACK this patch? It definitely fixes the problem I'm
> seeing on a s3c2440a. For more information, compare the s3c2440a or
> s3c2410 manuals with page 8 of the following documentation:
>
> http://www.samsung.com/global/business/semiconductor/products/flash/downloads/applicationnote/ecc_algorithm_for_web_512b.pdf
>
> Matt
>
>
> On 10/18/07, Matt Reimer <mattjreimer@gmail.com> wrote:
> > From: Matt Reimer <mreimer@vpop.net>
> >
> > The single-bit error correction was, well, incorrect. For determing which
> > bit to correct it was using P1' P2' P4' P8' instead of P1 P2 P4 P8, and
> > it was using P16' P32' P64' P128' P256' P512' P1024' P2048' instead of
> > P16 P32 P64 P128 P256 P512 P1024 P2048.
> >
> > Signed-off-by: Matt Reimer <mreimer@vpop.net>
> > ---
> >  drivers/mtd/nand/s3c2410.c |   26 ++++++++++++--------------
> >  1 files changed, 12 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/s3c2410.c b/drivers/mtd/nand/s3c2410.c
> > index 903db18..077fdcc 100644
> > --- a/drivers/mtd/nand/s3c2410.c
> > +++ b/drivers/mtd/nand/s3c2410.c
> > @@ -364,23 +364,21 @@ static int s3c2410_nand_correct_data(struct mtd_info *mtd, u_char *dat,
> >             ((diff2 ^ (diff2 >> 1)) & 0x55) == 0x55) {
> >                 /* calculate the bit position of the error */
> >
> > -               bit  = (diff2 >> 2) & 1;
> > -               bit |= (diff2 >> 3) & 2;
> > -               bit |= (diff2 >> 4) & 4;
> > +               bit  = ((diff2 >> 3) & 1) |
> > +                      ((diff2 >> 4) & 2) |
> > +                      ((diff2 >> 5) & 4);
> >
> >                 /* calculate the byte position of the error */
> >
> > -               byte  = (diff1 << 1) & 0x80;
> > -               byte |= (diff1 << 2) & 0x40;
> > -               byte |= (diff1 << 3) & 0x20;
> > -               byte |= (diff1 << 4) & 0x10;
> > -
> > -               byte |= (diff0 >> 3) & 0x08;
> > -               byte |= (diff0 >> 2) & 0x04;
> > -               byte |= (diff0 >> 1) & 0x02;
> > -               byte |= (diff0 >> 0) & 0x01;
> > -
> > -               byte |= (diff2 << 8) & 0x100;
> > +               byte = ((diff2 << 7) & 0x100) |
> > +                      ((diff1 << 0) & 0x80)  |
> > +                      ((diff1 << 1) & 0x40)  |
> > +                      ((diff1 << 2) & 0x20)  |
> > +                      ((diff1 << 3) & 0x10)  |
> > +                      ((diff0 >> 4) & 0x08)  |
> > +                      ((diff0 >> 3) & 0x04)  |
> > +                      ((diff0 >> 2) & 0x02)  |
> > +                      ((diff0 >> 1) & 0x01);
> >
> >                 dev_dbg(info->device, "correcting error bit %d, byte %d\n",
> >                         bit, byte);
> > --
> > 1.5.3.2
> >
> >
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> >
>

Re: [PATCH] MTD: fix s3c2410 error correction

[Jörn Engel]

On Tue, 6 November 2007 09:09:00 -0800, Matt Reimer wrote:
> 
> ping

Ben doesn't seem to comment on this, so I will.  And I must say, I don't
like the function either before or after your patch.

1. In principle this is just the bog-standard 1-bit error correction
   code everyone and his dog is using.
2. The original version of this function is subtly different from
   nand_correct_data().  12 of the constants are off-by-one.  But no
   explanation why.
3. After your patch the bit-position constants are back to original, the
   0x54 remains 0x55, half the byte-position constants are back to
   original and the other half that were identical are off-by-one now.
   Still no explanation why.

So my first question is not whether your patch is correct or not, but
what makes s3c2410 so different from all the other bog-standard ecc
schemes?


Side note:
The last conditional in both functions,
	if ((diff0 & ~(1<<fls(diff0))) == 0)
can be replaced by is_power_of_2().

Jörn

-- 
Happiness isn't having what you want, it's wanting what you have.
-- unknown

Re: [PATCH] MTD: fix s3c2410 error correction

[Matt Reimer]

On 11/6/07, Jörn Engel <joern@logfs.org> wrote:
> On Tue, 6 November 2007 09:09:00 -0800, Matt Reimer wrote:
> >
> > ping
>
> Ben doesn't seem to comment on this, so I will.  And I must say, I don't
> like the function either before or after your patch.
>
> 1. In principle this is just the bog-standard 1-bit error correction
>    code everyone and his dog is using.
> 2. The original version of this function is subtly different from
>    nand_correct_data().  12 of the constants are off-by-one.  But no
>    explanation why.
> 3. After your patch the bit-position constants are back to original, the
>    0x54 remains 0x55, half the byte-position constants are back to
>    original and the other half that were identical are off-by-one now.
>    Still no explanation why.
>
> So my first question is not whether your patch is correct or not, but
> what makes s3c2410 so different from all the other bog-standard ecc
> schemes?

The explanation is that the s3c hardware ECC gives us the bits in that
order, and this order is what Samsung recommends.

"For more information, compare the s3c2440a or s3c2410 manuals with
page 8 of the following documentation:
http://www.samsung.com/global/business/semiconductor/products/flash/downloads/applicationnote/ecc_algorithm_for_web_512b.pdf"


> Side note:
> The last conditional in both functions,
>         if ((diff0 & ~(1<<fls(diff0))) == 0)
> can be replaced by is_power_of_2().

Thanks, that makes it much more readable.

Matt

Re: [PATCH] MTD: fix s3c2410 error correction

[Jörn Engel]

On Tue, 6 November 2007 14:34:54 -0800, Matt Reimer wrote:
> 
> The explanation is that the s3c hardware ECC gives us the bits in that
> order, and this order is what Samsung recommends.
> 
> "For more information, compare the s3c2440a or s3c2410 manuals with
> page 8 of the following documentation:
> http://www.samsung.com/global/business/semiconductor/products/flash/downloads/applicationnote/ecc_algorithm_for_web_512b.pdf

Can you send me a copy of those manuals or a URL?

Jörn

-- 
With a PC, I always felt limited by the software available. On Unix,
I am limited only by my knowledge.
-- Peter J. Schoenster

Re: [PATCH] MTD: fix s3c2410 error correction

[Matthieu CASTET]

Hi,

Matt Reimer wrote:
 > Anybody willing to ACK this patch? It definitely fixes the problem I'm
 > seeing on a s3c2440a. For more information, compare the s3c2440a or
 > s3c2410 manuals with page 8 of the following documentation:
 >
This is also need for s3c2412.


Matthieu

Re: [PATCH] MTD: fix s3c2410 error correction

[Matt Reimer]

On Oct 18, 2007 5:02 PM, Matt Reimer <mattjreimer@gmail.com> wrote:
> From: Matt Reimer <mreimer@vpop.net>
>
> The single-bit error correction was, well, incorrect. For determing which
> bit to correct it was using P1' P2' P4' P8' instead of P1 P2 P4 P8, and
> it was using P16' P32' P64' P128' P256' P512' P1024' P2048' instead of
> P16 P32 P64 P128 P256 P512 P1024 P2048.

Is there any chance of this getting committed? It has been confirmed
to be needed on s3c2440 and s3c2412.

Matt

Re: [PATCH] MTD: fix s3c2410 error correction

[Jörn Engel]

On Thu, 29 November 2007 11:10:41 -0800, Matt Reimer wrote:
> On Oct 18, 2007 5:02 PM, Matt Reimer <mattjreimer@gmail.com> wrote:
> > From: Matt Reimer <mreimer@vpop.net>
> >
> > The single-bit error correction was, well, incorrect. For determing which
> > bit to correct it was using P1' P2' P4' P8' instead of P1 P2 P4 P8, and
> > it was using P16' P32' P64' P128' P256' P512' P1024' P2048' instead of
> > P16 P32 P64 P128 P256 P512 P1024 P2048.
> 
> Is there any chance of this getting committed? It has been confirmed
> to be needed on s3c2440 and s3c2412.

Ok, I'm getting sick of this.  Even though I didn't fully review the
patch, I'll gladly take the chances of breaking something and
complaining to Matt afterwards.  Certainly beats ignoring this patch for
all eternity.

Acked-by: Joern Engel <joern@logfs.org>

Jörn

-- 
You ain't got no problem, Jules. I'm on the motherfucker. Go back in
there, chill them niggers out and wait for the Wolf, who should be
coming directly.
-- Marsellus Wallace

Re: [PATCH] MTD: fix s3c2410 error correction

[Matt Reimer]

On Dec 3, 2007 7:20 AM, Jörn Engel <joern@logfs.org> wrote:
>
> On Thu, 29 November 2007 11:10:41 -0800, Matt Reimer wrote:
> >
> > Is there any chance of this getting committed? It has been confirmed
> > to be needed on s3c2440 and s3c2412.
>
> Ok, I'm getting sick of this.  Even though I didn't fully review the
> patch, I'll gladly take the chances of breaking something and
> complaining to Matt afterwards.  Certainly beats ignoring this patch for
> all eternity.

Thanks Jörn.

Matt

Re: [PATCH] MTD: fix s3c2410 error correction

[Matt Reimer]

On Jan 17, 2008 10:26 AM, Werner Almesberger <werner@openmoko.org> wrote:
> FYI, I've verified the S3C2410 ECC fix
> http://lists.infradead.org/pipermail/linux-mtd/2007-October/019586.html
> on the OpenMoko GTA01 (2410) and GTA02 (2442).
>
> For this, I've written a crude little program to tilt bits in Flash:
> http://svn.openmoko.org/developers/werner/nandtilt/

Great, thanks! That's useful.

Matt

Re: [PATCH] MTD: fix s3c2410 error correction

[Matt Reimer]

On Dec 3, 2007 7:20 AM, Jörn Engel <joern@logfs.org> wrote:
>
> On Thu, 29 November 2007 11:10:41 -0800, Matt Reimer wrote:
> > On Oct 18, 2007 5:02 PM, Matt Reimer <mattjreimer@gmail.com> wrote:
> > > From: Matt Reimer <mreimer@vpop.net>
> > >
> > > The single-bit error correction was, well, incorrect. For determing which
> > > bit to correct it was using P1' P2' P4' P8' instead of P1 P2 P4 P8, and
> > > it was using P16' P32' P64' P128' P256' P512' P1024' P2048' instead of
> > > P16 P32 P64 P128 P256 P512 P1024 P2048.
> >
> > Is there any chance of this getting committed? It has been confirmed
> > to be needed on s3c2440 and s3c2412.
>
> Ok, I'm getting sick of this.  Even though I didn't fully review the
> patch, I'll gladly take the chances of breaking something and
> complaining to Matt afterwards.  Certainly beats ignoring this patch for
> all eternity.

Any takers? Pretty please?

Matt