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

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

[nsnehaprabha]

From: Sneha Narnakaje <nsnehaprabha@ti.com>

This patch adds 4-bit ECC support for large page NAND chips using the new ECC
mode NAND_ECC_HW_OOB_FIRST. The platform data from board-dm355-evm has been
adjusted to use this mode.

The patches have been verified on DM355 device with 2K Micron devices using
mtd-tests and JFFS2. Error correction upto 4-bits has also been verified using
nandwrite/nanddump utilities.

This patch series applies to linux-mtd next (mmotm) GIT tree.
This version (v3) addresses the review comment to keep OOB bytes towards the
end of sparebytes region. 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.

Reviewed-by: David Brownell <dbrownell@users.sourceforge.net>
Signed-off-by: Sneha Narnakaje <nsnehaprabha@ti.com>
Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
---
 drivers/mtd/nand/davinci_nand.c |   45 +++++++++++++++++++++++++++++++++-----
 1 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 0fad648..14c72d2 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -348,6 +348,12 @@ compare:
 	if (!(syndrome[0] | syndrome[1] | syndrome[2] | syndrome[3]))
 		return 0;
 
+	/*
+	 * Clear any previous address calculation by doing a dummy read of an
+	 * error address register.
+	 */
+	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.
@@ -359,8 +365,10 @@ compare:
 
 		switch ((fsr >> 8) & 0x0f) {
 		case 0:		/* no error, should not happen */
+			davinci_nand_readl(info, NAND_ERR_ERRVAL1_OFFSET);
 			return 0;
 		case 1:		/* five or more errors detected */
+			davinci_nand_readl(info, NAND_ERR_ERRVAL1_OFFSET);
 			return -EIO;
 		case 2:		/* error addresses computed */
 		case 3:
@@ -500,6 +508,26 @@ static struct nand_ecclayout hwecc4_small __initconst = {
 	},
 };
 
+/* An ECC layout for using 4-bit ECC with large-page (2048bytes) flash,
+ * storing ten ECC bytes plus the manufacturer's bad block marker byte,
+ * and not overlapping the default BBT markers.
+ */
+static struct nand_ecclayout hwecc4_2048 __initconst = {
+	.eccbytes = 40,
+	.eccpos = {
+		/* at the end of spare sector */
+		24, 25, 26, 27, 28, 29,	30, 31, 32, 33,
+		34, 35, 36, 37, 38, 39,	40, 41, 42, 43,
+		44, 45, 46, 47, 48, 49, 50, 51, 52, 53,
+		54, 55, 56, 57, 58, 59, 60, 61, 62, 63,
+		},
+	.oobfree = {
+		/* 1 byte at offset 0 holds manufacturer badblock marker */
+		{.offset = 1, .length = 23, },
+		/* 5 bytes at offset 8 hold BBT markers */
+		/* 8 bytes at offset 16 hold JFFS2 clean markers */
+	},
+};
 
 static int __init nand_davinci_probe(struct platform_device *pdev)
 {
@@ -690,15 +718,20 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
 				info->mtd.oobsize - 16;
 			goto syndrome_done;
 		}
+		if (chunks == 4) {
+			info->ecclayout = hwecc4_2048;
+			info->chip.ecc.mode = NAND_ECC_HW_OOB_FIRST;
+			goto syndrome_done;
+		}
 
-		/* For large page chips we'll be wanting to use a
-		 * not-yet-implemented mode that reads OOB data
-		 * before reading the body of the page, to avoid
-		 * the "infix OOB" model of NAND_ECC_HW_SYNDROME
-		 * (and preserve manufacturer badblock markings).
+		/* 4K page chips are not yet supported. The eccpos from
+		 * nand_ecclayout cannot hold 80 bytes and change to eccpos[]
+		 * breaks userspace ioctl interface with mtd-utils. Once we
+		 * resolve this issue, NAND_ECC_HW_OOB_FIRST mode can be used
+		 * for the 4K page chips.
 		 */
 		dev_warn(&pdev->dev, "no 4-bit ECC support yet "
-				"for large page NAND\n");
+				"for 4K page NAND\n");
 		ret = -EIO;
 		goto err_scan;
 
-- 
1.6.0.4

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

[Troy Kisky]

nsnehaprabha@ti.com wrote:
> +/* An ECC layout for using 4-bit ECC with large-page (2048bytes) flash,
> + * storing ten ECC bytes plus the manufacturer's bad block marker byte,
> + * and not overlapping the default BBT markers.
> + */
> +static struct nand_ecclayout hwecc4_2048 __initconst = {
> +	.eccbytes = 40,
> +	.eccpos = {
> +		/* at the end of spare sector */
> +		24, 25, 26, 27, 28, 29,	30, 31, 32, 33,
> +		34, 35, 36, 37, 38, 39,	40, 41, 42, 43,
> +		44, 45, 46, 47, 48, 49, 50, 51, 52, 53,
> +		54, 55, 56, 57, 58, 59, 60, 61, 62, 63,
> +		},
> +	.oobfree = {
> +		/* 1 byte at offset 0 holds manufacturer badblock marker */
> +		{.offset = 1, .length = 23, },
> +		/* 5 bytes at offset 8 hold BBT markers */
> +		/* 8 bytes at offset 16 hold JFFS2 clean markers */
> +	},
> +};

I hate to sound like a broken record, but

If the bad block marker is only 1 byte. Don't you need to override

static struct nand_bbt_descr largepage_flashbased = {
	.options = NAND_BBT_SCAN2NDPAGE,
	.offs = 0,
	.len = 2,
	.pattern = scan_ff_pattern
};

I think it is easier just to leave it as 2 bytes. That may
allow substituting a different manufacturers chip too.

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

[Narnakaje, Snehaprabha]

> -----Original Message-----
> From: Troy Kisky [mailto:troy.kisky@boundarydevices.com]
> Sent: Thursday, July 16, 2009 7:57 PM
> To: Narnakaje, Snehaprabha
> Cc: linux-mtd@lists.infradead.org; davinci-linux-open-
> source@linux.davincidsp.com; dwmw2@infradead.org; tglx@linutronix.de;
> akpm@linux-foundation.org; Paulraj, Sandeep
> Subject: Re: [PATCH v3 3/3] mtd-nand: DaVinci: Add 4-bit ECC support for
> large page NAND chips
> 
> nsnehaprabha@ti.com wrote:
> > +/* An ECC layout for using 4-bit ECC with large-page (2048bytes) flash,
> > + * storing ten ECC bytes plus the manufacturer's bad block marker byte,
> > + * and not overlapping the default BBT markers.
> > + */
> > +static struct nand_ecclayout hwecc4_2048 __initconst = {
> > +	.eccbytes = 40,
> > +	.eccpos = {
> > +		/* at the end of spare sector */
> > +		24, 25, 26, 27, 28, 29,	30, 31, 32, 33,
> > +		34, 35, 36, 37, 38, 39,	40, 41, 42, 43,
> > +		44, 45, 46, 47, 48, 49, 50, 51, 52, 53,
> > +		54, 55, 56, 57, 58, 59, 60, 61, 62, 63,
> > +		},
> > +	.oobfree = {
> > +		/* 1 byte at offset 0 holds manufacturer badblock marker */
> > +		{.offset = 1, .length = 23, },
> > +		/* 5 bytes at offset 8 hold BBT markers */
> > +		/* 8 bytes at offset 16 hold JFFS2 clean markers */
> > +	},
> > +};
> 
> I hate to sound like a broken record, but
> 
> If the bad block marker is only 1 byte. Don't you need to override
> 
> static struct nand_bbt_descr largepage_flashbased = {
> 	.options = NAND_BBT_SCAN2NDPAGE,
> 	.offs = 0,
> 	.len = 2,
> 	.pattern = scan_ff_pattern
> };
> 
> I think it is easier just to leave it as 2 bytes. That may
> allow substituting a different manufacturers chip too.

Yes, it is better to leave it as 2 bytes.

Thanks
Sneha
 
> 
> 

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

[David Brownell]

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(-)

--- 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));

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

[Narnakaje, Snehaprabha]

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));

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

[David Brownell]

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