Patchwork [05/21] mtd: nand: pxa3xx: Add driver-specific ECC BCH support

login
register
mail settings
Submitter Ezequiel Garcia
Date Sept. 19, 2013, 4:01 p.m.
Message ID <1379606505-2529-6-git-send-email-ezequiel.garcia@free-electrons.com>
Download mbox | patch
Permalink /patch/275989/
State New
Headers show

Comments

Ezequiel Garcia - Sept. 19, 2013, 4:01 p.m.
This commit adds the BCH ECC support available in NFCv2 controller.
Depending on the detected required strength the respective ECC layout
is selected.

This commit adds an empty ECC layout, since support to access large
pages is first required. Once that support is added, a proper ECC
layout will be added as well.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 53 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 7 deletions(-)
Brian Norris - Oct. 3, 2013, 12:24 a.m.
On Thu, Sep 19, 2013 at 01:01:29PM -0300, Ezequiel Garcia wrote:
> This commit adds the BCH ECC support available in NFCv2 controller.
> Depending on the detected required strength the respective ECC layout
> is selected.
> 
> This commit adds an empty ECC layout, since support to access large
> pages is first required. Once that support is added, a proper ECC
> layout will be added as well.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 53 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index a0dabe6..86c343e 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c

...

> @@ -1117,10 +1133,6 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
>  	pxa3xx_flash_ids[1].name = NULL;
>  	def = pxa3xx_flash_ids;
>  KEEP_CONFIG:
> -	chip->ecc.mode = NAND_ECC_HW;
> -	chip->ecc.size = host->page_size;
> -	chip->ecc.strength = 1;
> -
>  	if (info->reg_ndcr & NDCR_DWIDTH_M)
>  		chip->options |= NAND_BUSWIDTH_16;
>  
> @@ -1130,8 +1142,35 @@ KEEP_CONFIG:
>  		chip->bbt_options |= NAND_BBT_USE_FLASH;
>  	}
>  
> +	/* Device detection must be done with ECC disabled */
> +	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
> +		nand_writel(info, NDECCCTRL, 0x0);
> +
>  	if (nand_scan_ident(mtd, 1, def))
>  		return -ENODEV;
> +
> +	/* 1-step ECC over the entire detected page size */
> +	chip->ecc.mode = NAND_ECC_HW;
> +	chip->ecc.size = mtd->writesize;

Does the ECC really only work in one step? IOW, do you only correct
1-bit for the entire page? That is under-spec'd for most NAND (they need
1-bit/512-byte, or 4-bit/512-byte). This ecc.size should actually
reflect the correction region (typically 512B or 1024B), and even if
your buffer layout makes a step every 2KB (or 4KB or whatever), the ECC
step size *should* be smaller than that.

Or maybe I'm wrong, and you're actually using a weak 4-bit correction
over 2048 bytes.

> +
> +	/*
> +	 * Armada370 variant supports BCH, which provides 4-bit strength
> +	 * and needs to be supported in a special way.
> +	 */
> +	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 &&

Might there be other variants eventually that support BCH? Perhaps you
want to separate the property of "can use BCH" from "will use BCH"?

> +	    chip->ecc_strength_ds == 4) {

Do you use BCH-4 only for chips that require exactly 4-bit correction?
Is there ever a need to "overclock" the ECC, and choose BCH-4 for chips
that only require 1-bit correction? Can the bootloader ever make a
choice different from yours here?

What about NAND that don't support the ecc_strength_ds field yet?
Remember, this is only filled for ONFI NAND and a few NAND that are in
the full-ID table.

> +	        chip->ecc.layout = &ecc_layout_4KB_bch4bit;

This seems to have an implicit page size assumption. In your case, it
seems like this layout should be chosen based on the page size + OOB
size, at least.

> +		chip->ecc.strength = chip->ecc_strength_ds;
> +		info->ecc_bch = 1;
> +	} else if (mtd->writesize <= 2048) {
> +		/*
> +		 * This is the default ECC Hamming 1-bit strength case,
> +		 * which works for page sizes of 512 and 2048 bytes.
> +		 * The ECC layout will be automatically configured.
> +		 */
> +		chip->ecc.strength = 1;
> +	}

What about the 'else' case? What happens with >2KB page NAND on
non-Armada370 systems? What happens if a NAND needs >4-bit correction?
Perhaps you could include a warning/error? I don't know the flexibility
of NAND types you'll find on an Armada board; maybe they're all
ONFI-compliant?

> +
>  	/* calculate addressing information */
>  	if (mtd->writesize >= 2048)
>  		host->col_addr_cycles = 2;

Brian
Ezequiel Garcia - Oct. 4, 2013, 7:49 p.m.
On Wed, Oct 02, 2013 at 05:24:40PM -0700, Brian Norris wrote:
> On Thu, Sep 19, 2013 at 01:01:29PM -0300, Ezequiel Garcia wrote:
> > This commit adds the BCH ECC support available in NFCv2 controller.
> > Depending on the detected required strength the respective ECC layout
> > is selected.
> > 
> > This commit adds an empty ECC layout, since support to access large
> > pages is first required. Once that support is added, a proper ECC
> > layout will be added as well.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  drivers/mtd/nand/pxa3xx_nand.c | 53 ++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 46 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > index a0dabe6..86c343e 100644
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> 
> ...
> 
> > @@ -1117,10 +1133,6 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
> >  	pxa3xx_flash_ids[1].name = NULL;
> >  	def = pxa3xx_flash_ids;
> >  KEEP_CONFIG:
> > -	chip->ecc.mode = NAND_ECC_HW;
> > -	chip->ecc.size = host->page_size;
> > -	chip->ecc.strength = 1;
> > -
> >  	if (info->reg_ndcr & NDCR_DWIDTH_M)
> >  		chip->options |= NAND_BUSWIDTH_16;
> >  
> > @@ -1130,8 +1142,35 @@ KEEP_CONFIG:
> >  		chip->bbt_options |= NAND_BBT_USE_FLASH;
> >  	}
> >  
> > +	/* Device detection must be done with ECC disabled */
> > +	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
> > +		nand_writel(info, NDECCCTRL, 0x0);
> > +
> >  	if (nand_scan_ident(mtd, 1, def))
> >  		return -ENODEV;
> > +
> > +	/* 1-step ECC over the entire detected page size */
> > +	chip->ecc.mode = NAND_ECC_HW;
> > +	chip->ecc.size = mtd->writesize;
> 
> Does the ECC really only work in one step? IOW, do you only correct
> 1-bit for the entire page? That is under-spec'd for most NAND (they need
> 1-bit/512-byte, or 4-bit/512-byte). This ecc.size should actually
> reflect the correction region (typically 512B or 1024B), and even if
> your buffer layout makes a step every 2KB (or 4KB or whatever), the ECC
> step size *should* be smaller than that.
> 
> Or maybe I'm wrong, and you're actually using a weak 4-bit correction
> over 2048 bytes.
> 

I must admit I'm a bit confused by this ECC step concept. Having
described the ECC engine in the cover-letter, what do you think
the right choice should be?

IMO, given the ECC engine works on the entire FIFO (actually it works
on the transfered chunk) it's a 1-step ECC.

On the other side, a multiple step ECC, i.e. chip->ecc.size != mtd->writesize
makes the NAND base calls the subpage() function and the driver crashes
horribly when using UBIFS.

I'm not sure why subpage should be called in that case, given we don't
support that.

> > +
> > +	/*
> > +	 * Armada370 variant supports BCH, which provides 4-bit strength
> > +	 * and needs to be supported in a special way.
> > +	 */
> > +	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 &&
> 
> Might there be other variants eventually that support BCH? Perhaps you
> want to separate the property of "can use BCH" from "will use BCH"?
> 

Yeah, probably. And maybe setting the BCH from the DT? The problem here
is that because of the ECC choice being intimately related to the OOB
location, this means the ECC choice is intimately related to the bad
block markers.

IOW, the user can't really choose among ECC.

I guess this means the ECC choice is some sort of a hardware description
and rightfully belongs to the DT. Right?

> > +	    chip->ecc_strength_ds == 4) {
> 
> Do you use BCH-4 only for chips that require exactly 4-bit correction?
> Is there ever a need to "overclock" the ECC, and choose BCH-4 for chips
> that only require 1-bit correction? Can the bootloader ever make a
> choice different from yours here?
> 
> What about NAND that don't support the ecc_strength_ds field yet?
> Remember, this is only filled for ONFI NAND and a few NAND that are in
> the full-ID table.
> 
> > +	        chip->ecc.layout = &ecc_layout_4KB_bch4bit;
> 
> This seems to have an implicit page size assumption. In your case, it
> seems like this layout should be chosen based on the page size + OOB
> size, at least.
> 
> > +		chip->ecc.strength = chip->ecc_strength_ds;
> > +		info->ecc_bch = 1;
> > +	} else if (mtd->writesize <= 2048) {
> > +		/*
> > +		 * This is the default ECC Hamming 1-bit strength case,
> > +		 * which works for page sizes of 512 and 2048 bytes.
> > +		 * The ECC layout will be automatically configured.
> > +		 */
> > +		chip->ecc.strength = 1;
> > +	}
> 
> What about the 'else' case? What happens with >2KB page NAND on
> non-Armada370 systems? What happens if a NAND needs >4-bit correction?
> Perhaps you could include a warning/error? I don't know the flexibility
> of NAND types you'll find on an Armada board; maybe they're all
> ONFI-compliant?
> 

Yeah, this 'ifs' could be fixed to better support other cases.

I'll try to rework that.
Brian Norris - Oct. 5, 2013, 12:27 a.m.
On Fri, Oct 04, 2013 at 04:49:24PM -0300, Ezequiel Garcia wrote:
> On Wed, Oct 02, 2013 at 05:24:40PM -0700, Brian Norris wrote:
> > On Thu, Sep 19, 2013 at 01:01:29PM -0300, Ezequiel Garcia wrote:
> > > This commit adds the BCH ECC support available in NFCv2 controller.
> > > Depending on the detected required strength the respective ECC layout
> > > is selected.
> > > 
> > > This commit adds an empty ECC layout, since support to access large
> > > pages is first required. Once that support is added, a proper ECC
> > > layout will be added as well.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > ---
> > >  drivers/mtd/nand/pxa3xx_nand.c | 53 ++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 46 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > > index a0dabe6..86c343e 100644
> > > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > 
> > ...
> > 
> > > @@ -1117,10 +1133,6 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
> > >  	pxa3xx_flash_ids[1].name = NULL;
> > >  	def = pxa3xx_flash_ids;
> > >  KEEP_CONFIG:
> > > -	chip->ecc.mode = NAND_ECC_HW;
> > > -	chip->ecc.size = host->page_size;
> > > -	chip->ecc.strength = 1;
> > > -
> > >  	if (info->reg_ndcr & NDCR_DWIDTH_M)
> > >  		chip->options |= NAND_BUSWIDTH_16;
> > >  
> > > @@ -1130,8 +1142,35 @@ KEEP_CONFIG:
> > >  		chip->bbt_options |= NAND_BBT_USE_FLASH;
> > >  	}
> > >  
> > > +	/* Device detection must be done with ECC disabled */
> > > +	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
> > > +		nand_writel(info, NDECCCTRL, 0x0);
> > > +
> > >  	if (nand_scan_ident(mtd, 1, def))
> > >  		return -ENODEV;
> > > +
> > > +	/* 1-step ECC over the entire detected page size */
> > > +	chip->ecc.mode = NAND_ECC_HW;
> > > +	chip->ecc.size = mtd->writesize;
> > 
> > Does the ECC really only work in one step? IOW, do you only correct
> > 1-bit for the entire page? That is under-spec'd for most NAND (they need
> > 1-bit/512-byte, or 4-bit/512-byte). This ecc.size should actually
> > reflect the correction region (typically 512B or 1024B), and even if
> > your buffer layout makes a step every 2KB (or 4KB or whatever), the ECC
> > step size *should* be smaller than that.
> > 
> > Or maybe I'm wrong, and you're actually using a weak 4-bit correction
> > over 2048 bytes.
> > 
> 
> I must admit I'm a bit confused by this ECC step concept. Having
> described the ECC engine in the cover-letter, what do you think
> the right choice should be?

I'm still not sure. But I'll defer that discussion to the cover-letter.

> IMO, given the ECC engine works on the entire FIFO (actually it works
> on the transfered chunk) it's a 1-step ECC.

That doesn't necessarily mean it's 1-step ECC. The step refers more to
the correction region than to the mechanics of transfer sizes.

> On the other side, a multiple step ECC, i.e. chip->ecc.size != mtd->writesize
> makes the NAND base calls the subpage() function and the driver crashes
> horribly when using UBIFS.

Which function? Read or write? AFAICT, neither relies on chip->ecc.size.

> I'm not sure why subpage should be called in that case, given we don't
> support that.

If you don't support writes, you can flag:
    chip->options |= NAND_NO_SUBPAGE_WRITE

And subpage reads are currently only enabled on soft-ECC with page size
larger than 512.

> > > +
> > > +	/*
> > > +	 * Armada370 variant supports BCH, which provides 4-bit strength
> > > +	 * and needs to be supported in a special way.
> > > +	 */
> > > +	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 &&
> > 
> > Might there be other variants eventually that support BCH? Perhaps you
> > want to separate the property of "can use BCH" from "will use BCH"?
> > 
> 
> Yeah, probably. And maybe setting the BCH from the DT? The problem here
> is that because of the ECC choice being intimately related to the OOB
> location, this means the ECC choice is intimately related to the bad
> block markers.

Speaking of that: how do you deal with bad block markers, if your data
is actually placed in what is traditionally the "spare area"?

> IOW, the user can't really choose among ECC.

This means the choice of ECC must remain consistent across all users of
the NAND, but it doesn't mean there is no initial choice. A user could
choose to enable a higher ECC strength than what is technically required
by the flash.

> I guess this means the ECC choice is some sort of a hardware description
> and rightfully belongs to the DT. Right?

Yes, I think some description of ECC can be placed in DT. We're working
through a DT binding for Pekon's OMAP driver right now.

Brian
Ezequiel Garcia - Oct. 17, 2013, 10:27 p.m.
Brian,

On Fri, Oct 04, 2013 at 05:27:13PM -0700, Brian Norris wrote:
[..]
> 
> Speaking of that: how do you deal with bad block markers, if your data
> is actually placed in what is traditionally the "spare area"?
> 

We currently don't deal with this :(

I'll be sending a v2 soon to address many of the issues you brought
and to re-work the ECC mode 'choice' to be safer.

I'll post the open issues in the v2 cover-letter, and this factory bad block
thing will be among them.

Patch

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index a0dabe6..86c343e 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -56,6 +56,7 @@ 
 #define NDPCR		(0x18) /* Page Count Register */
 #define NDBDR0		(0x1C) /* Bad Block Register 0 */
 #define NDBDR1		(0x20) /* Bad Block Register 1 */
+#define NDECCCTRL	(0x28) /* ECC control */
 #define NDDB		(0x40) /* Data Buffer */
 #define NDCB0		(0x48) /* Command Buffer0 */
 #define NDCB1		(0x4C) /* Command Buffer1 */
@@ -197,6 +198,7 @@  struct pxa3xx_nand_info {
 
 	int			cs;
 	int			use_ecc;	/* use HW ECC ? */
+	int			ecc_bch;	/* using BCH ECC? */
 	int			use_dma;	/* use DMA ? */
 	int			use_spare;	/* use spare ? */
 
@@ -270,6 +272,12 @@  static struct nand_bbt_descr bbt_mirror_descr = {
 	.pattern = bb_mirror_pattern
 };
 
+static struct nand_ecclayout ecc_layout_4KB_bch4bit = {
+	.eccbytes = 0,
+	.eccpos = {},
+	.oobfree = {}
+};
+
 /* Define a default flash type setting serve as flash detecting only */
 #define DEFAULT_FLASH_TYPE (&builtin_flash_types[0])
 
@@ -329,7 +337,10 @@  static void pxa3xx_set_datasize(struct pxa3xx_nand_info *info)
 
 	switch (host->page_size) {
 	case 2048:
-		info->oob_size = (info->use_ecc) ? 40 : 64;
+		if (info->ecc_bch)
+			info->oob_size = (info->use_ecc) ? 32 : 64;
+		else
+			info->oob_size = (info->use_ecc) ? 40 : 64;
 		break;
 	case 512:
 		info->oob_size = (info->use_ecc) ? 8 : 16;
@@ -349,10 +360,15 @@  static void pxa3xx_nand_start(struct pxa3xx_nand_info *info)
 
 	ndcr = info->reg_ndcr;
 
-	if (info->use_ecc)
+	if (info->use_ecc) {
 		ndcr |= NDCR_ECC_EN;
-	else
+		if (info->ecc_bch)
+			nand_writel(info, NDECCCTRL, 0x1);
+	} else {
 		ndcr &= ~NDCR_ECC_EN;
+		if (info->ecc_bch)
+			nand_writel(info, NDECCCTRL, 0x0);
+	}
 
 	if (info->use_dma)
 		ndcr |= NDCR_DMA_EN;
@@ -1117,10 +1133,6 @@  static int pxa3xx_nand_scan(struct mtd_info *mtd)
 	pxa3xx_flash_ids[1].name = NULL;
 	def = pxa3xx_flash_ids;
 KEEP_CONFIG:
-	chip->ecc.mode = NAND_ECC_HW;
-	chip->ecc.size = host->page_size;
-	chip->ecc.strength = 1;
-
 	if (info->reg_ndcr & NDCR_DWIDTH_M)
 		chip->options |= NAND_BUSWIDTH_16;
 
@@ -1130,8 +1142,35 @@  KEEP_CONFIG:
 		chip->bbt_options |= NAND_BBT_USE_FLASH;
 	}
 
+	/* Device detection must be done with ECC disabled */
+	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
+		nand_writel(info, NDECCCTRL, 0x0);
+
 	if (nand_scan_ident(mtd, 1, def))
 		return -ENODEV;
+
+	/* 1-step ECC over the entire detected page size */
+	chip->ecc.mode = NAND_ECC_HW;
+	chip->ecc.size = mtd->writesize;
+
+	/*
+	 * Armada370 variant supports BCH, which provides 4-bit strength
+	 * and needs to be supported in a special way.
+	 */
+	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 &&
+	    chip->ecc_strength_ds == 4) {
+	        chip->ecc.layout = &ecc_layout_4KB_bch4bit;
+		chip->ecc.strength = chip->ecc_strength_ds;
+		info->ecc_bch = 1;
+	} else if (mtd->writesize <= 2048) {
+		/*
+		 * This is the default ECC Hamming 1-bit strength case,
+		 * which works for page sizes of 512 and 2048 bytes.
+		 * The ECC layout will be automatically configured.
+		 */
+		chip->ecc.strength = 1;
+	}
+
 	/* calculate addressing information */
 	if (mtd->writesize >= 2048)
 		host->col_addr_cycles = 2;