| Message ID | 1379606505-2529-6-git-send-email-ezequiel.garcia@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
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
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.
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;
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(-)