| Message ID | 1277501791-3146-1-git-send-email-norris@broadcom.com |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2010-06-25 at 14:36 -0700, Brian Norris wrote: > @@ -2920,9 +2920,13 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, > chip->chip_shift = ffs((unsigned)(chip->chipsize >> 32)) + 32 - 1; > > /* Set the bad block position */ > - chip->badblockpos = mtd->writesize > 512 ? > - NAND_LARGE_BADBLOCK_POS : NAND_SMALL_BADBLOCK_POS; > - chip->badblockbits = 8; > + chip->badblockpos = (!(busw & NAND_BUSWIDTH_16) && > + (*maf_id == NAND_MFR_STMICRO || > + (*maf_id == NAND_MFR_SAMSUNG && > + mtd->writesize == 512) || > + *maf_id == NAND_MFR_AMD)) > + ? NAND_SMALL_BADBLOCK_POS : NAND_LARGE_BADBLOCK_POS; I'd prefer if () else, ? : is not appropriate for such huge conditions. Nevertheless, you patch looks indeed like a step in the right direction, but I cannot really test it and do not have time to review. But could you please re-send it without trailing white-spaces - I'll put it to my l2 tree then? You can do a 'git am' to your own patches - it'll check for trailing white-spaces for you.
This is a two-patch correction and update to my previous attempt to address a few issues with bad block scanning. Overall, they should help renovate some incorrect and unwieldy code. I included the top few maintainers from get_maintainers.pl. Sorry if these were incorrect addressees. Brian Norris (2): mtd/nand, BB detect: factory marker in page 1, 2, last and byte 1 or 6 mtd/nand: More BB Detection, dynamic scan options drivers/mtd/nand/nand_base.c | 38 ++++++++++++++++-- drivers/mtd/nand/nand_bbt.c | 89 +++++++++++++++++++++++++++++------------ include/linux/mtd/bbm.h | 4 ++ 3 files changed, 101 insertions(+), 30 deletions(-)
On Tue, 2010-07-13 at 15:12 -0700, Brian Norris wrote: > This is a two-patch correction and update to my previous attempt > to address a few issues with bad block scanning. Overall, they should > help renovate some incorrect and unwieldy code. > > I included the top few maintainers from get_maintainers.pl. Sorry if these > were incorrect addressees. > > Brian Norris (2): > mtd/nand, BB detect: factory marker in page 1, 2, last and byte 1 or 6 > mtd/nand: More BB Detection, dynamic scan options > > drivers/mtd/nand/nand_base.c | 38 ++++++++++++++++-- > drivers/mtd/nand/nand_bbt.c | 89 +++++++++++++++++++++++++++++------------ > include/linux/mtd/bbm.h | 4 ++ > 3 files changed, 101 insertions(+), 30 deletions(-) I did not _really_ review this, it the patches look good. How did you test them? Did you test on both large and small page NANDs? I pushed your patches to my l2-mtd-2.6.git / master
On 07/18/2010 09:38 AM, Artem Bityutskiy wrote: > I did not _really_ review this, it the patches look good. How did you > test them? Did you test on both large and small page NANDs? I referenced 30+ data sheets (covering 100+ parts), and I tested a selection of 10 different chips to varying degrees. Particularly, I tested the creation of bad-block descriptors and basic BB scanning on three parts: ST NAND04GW3B2D, 2K page ST NAND128W3A, 512B page Samsung K9F1G08U0A, 2K page To test these, I wrote some fake bad block markers to the flash (in OOB bytes 1, 6, and elsewhere) to see if the scanning routine would detect them properly. However, this method was somewhat limited because the driver I am using has some bugs in its OOB write functionality.
On Mon, 2010-07-19 at 12:32 -0700, Brian Norris wrote: > On 07/18/2010 09:38 AM, Artem Bityutskiy wrote: > > I did not _really_ review this, it the patches look good. How did you > > test them? Did you test on both large and small page NANDs? > > I referenced 30+ data sheets (covering 100+ parts), and I tested a > selection of 10 different chips to varying degrees. Particularly, I > tested the creation of bad-block descriptors and basic BB scanning on > three parts: > > ST NAND04GW3B2D, 2K page > ST NAND128W3A, 512B page > Samsung K9F1G08U0A, 2K page > > To test these, I wrote some fake bad block markers to the flash (in OOB > bytes 1, 6, and elsewhere) to see if the scanning routine would detect > them properly. However, this method was somewhat limited because the > driver I am using has some bugs in its OOB write functionality. Sounds like you did a lot of work. I'll add this information to the patch description.
On Wed, Jul 21, 2010 at 11:49 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Mon, 2010-07-19 at 12:32 -0700, Brian Norris wrote: >> On 07/18/2010 09:38 AM, Artem Bityutskiy wrote: >> > I did not _really_ review this, it the patches look good. How did you >> > test them? Did you test on both large and small page NANDs? >> >> I referenced 30+ data sheets (covering 100+ parts), and I tested a >> selection of 10 different chips to varying degrees. Particularly, I >> tested the creation of bad-block descriptors and basic BB scanning on >> three parts: >> >> ST NAND04GW3B2D, 2K page >> ST NAND128W3A, 512B page >> Samsung K9F1G08U0A, 2K page >> >> To test these, I wrote some fake bad block markers to the flash (in OOB >> bytes 1, 6, and elsewhere) to see if the scanning routine would detect >> them properly. However, this method was somewhat limited because the >> driver I am using has some bugs in its OOB write functionality. > > Sounds like you did a lot of work. I'll add this information to the > patch description. > I haven't seen many nandsim feedback, though it allows precise test scenarios and easy flash forensics. Still, FWIW, I too laud the effort.
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index e6cf9ae..2d1ee3c 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2920,9 +2920,13 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, chip->chip_shift = ffs((unsigned)(chip->chipsize >> 32)) + 32 - 1; /* Set the bad block position */ - chip->badblockpos = mtd->writesize > 512 ? - NAND_LARGE_BADBLOCK_POS : NAND_SMALL_BADBLOCK_POS; - chip->badblockbits = 8; + chip->badblockpos = (!(busw & NAND_BUSWIDTH_16) && + (*maf_id == NAND_MFR_STMICRO || + (*maf_id == NAND_MFR_SAMSUNG && + mtd->writesize == 512) || + *maf_id == NAND_MFR_AMD)) + ? NAND_SMALL_BADBLOCK_POS : NAND_LARGE_BADBLOCK_POS; + /* Get chip options, preserve non chip based options */ chip->options &= ~NAND_CHIPOPTIONS_MSK; @@ -2941,12 +2945,23 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, /* * Bad block marker is stored in the last page of each block - * on Samsung and Hynix MLC devices + * on Samsung and Hynix MLC devices; stored in first two pages + * of each block on Micron devices with 2KiB pages and on + * SLC Samsung, Hynix, and AMD/Spansion. All others scan only + * the first page. */ if ((chip->cellinfo & NAND_CI_CELLTYPE_MSK) && (*maf_id == NAND_MFR_SAMSUNG || *maf_id == NAND_MFR_HYNIX)) chip->options |= NAND_BBT_SCANLASTPAGE; + else if ((!(chip->cellinfo & NAND_CI_CELLTYPE_MSK) && + (*maf_id == NAND_MFR_SAMSUNG || + *maf_id == NAND_MFR_HYNIX || + *maf_id == NAND_MFR_AMD)) || + (mtd->writesize == 2048 && + *maf_id == NAND_MFR_MICRON)) + chip->options |= NAND_BBT_SCAN2NDPAGE; + /* Check for AND chips with 4 page planes */ if (chip->options & NAND_4PAGE_ARRAY) diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c index 71d83be..1719c56 100644 --- a/drivers/mtd/nand/nand_bbt.c +++ b/drivers/mtd/nand/nand_bbt.c @@ -1093,29 +1093,50 @@ int nand_update_bbt(struct mtd_info *mtd, loff_t offs) static uint8_t scan_ff_pattern[] = { 0xff, 0xff }; static struct nand_bbt_descr smallpage_memorybased = { - .options = NAND_BBT_SCAN2NDPAGE, - .offs = 5, + .options = 0, + .offs = NAND_SMALL_BADBLOCK_POS, .len = 1, .pattern = scan_ff_pattern }; +static struct nand_bbt_descr smallpage_scan2nd_memorybased = { + .options = NAND_BBT_SCAN2NDPAGE, + .offs = NAND_SMALL_BADBLOCK_POS, + .len = 2, + .pattern = scan_ff_pattern +}; + static struct nand_bbt_descr largepage_memorybased = { .options = 0, - .offs = 0, + .offs = NAND_LARGE_BADBLOCK_POS, + .len = 1, + .pattern = scan_ff_pattern +}; + +static struct nand_bbt_descr largepage_scan2nd_memorybased = { + .options = NAND_BBT_SCAN2NDPAGE, + .offs = NAND_LARGE_BADBLOCK_POS, .len = 2, .pattern = scan_ff_pattern }; +static struct nand_bbt_descr lastpage_memorybased = { + .options = NAND_BBT_SCANLASTPAGE, + .offs = 0, + .len = 1, + .pattern = scan_ff_pattern +}; + static struct nand_bbt_descr smallpage_flashbased = { .options = NAND_BBT_SCAN2NDPAGE, - .offs = 5, + .offs = NAND_SMALL_BADBLOCK_POS, .len = 1, .pattern = scan_ff_pattern }; static struct nand_bbt_descr largepage_flashbased = { .options = NAND_BBT_SCAN2NDPAGE, - .offs = 0, + .offs = NAND_LARGE_BADBLOCK_POS, .len = 2, .pattern = scan_ff_pattern }; @@ -1197,8 +1218,14 @@ int nand_default_bbt(struct mtd_info *mtd) this->bbt_td = NULL; this->bbt_md = NULL; if (!this->badblock_pattern) { - this->badblock_pattern = (mtd->writesize > 512) ? - &largepage_memorybased : &smallpage_memorybased; + if (this->options & NAND_BBT_SCANLASTPAGE) + this->badblock_pattern = &lastpage_memorybased; + else if (this->options & NAND_BBT_SCAN2NDPAGE) + this->badblock_pattern = this->badblockpos == NAND_SMALL_BADBLOCK_POS ? + &smallpage_scan2nd_memorybased : &largepage_scan2nd_memorybased; + else + this->badblock_pattern = this->badblockpos == NAND_SMALL_BADBLOCK_POS ? + &smallpage_memorybased : &largepage_memorybased; } } return nand_scan_bbt(mtd, this->badblock_pattern);
Some level of support for various scanning locations was already built in, but this required clean-up. First, BB marker location cannot be determined _only_ by the page size. Instead, I implemented some heuristic detection based on data sheets from various manufacturers (all found in nand_base.c:nand_get_flash_type()). Second, once these options were identified, they were not handled properly by nand_bbt.c:nand_default_bbt(). I believe there are only 5 combinations of page location/byte offset, so I updated the nand_bbt_desc structs accordingly. I have tested these changes with several chips, but particularly a Micron 29F8G08MAA chip. We use a memory-based, not flash-based, BBT; hence the edits only to the "memory_based" side of things. Most edits could be applicable to both, but I can't verify that. For instance, I do not understand why the small discrepancies between the "memory_based" and "flash_based" nand_bbt_descr structs. Finally, please provide any feedback you can. I'm sure there's something I have overlooked, but I feel this is a step in the right direction. Signed-off-by: Brian Norris <norris@broadcom.com> --- drivers/mtd/nand/nand_base.c | 23 +++++++++++++++++++---- drivers/mtd/nand/nand_bbt.c | 41 ++++++++++++++++++++++++++++++++++------- 2 files changed, 53 insertions(+), 11 deletions(-)