mtd: nand: renumber conflicting BBT flags

The NAND_USE_FLASH_BBT_NO_OOB and NAND_CREATE_EMPTY_BBT flags conflict with the NAND_BBT_SCANBYTE1AND6 and NAND_BBT_DYNAMICSTRUCT flags, respectively. This change will allow us to utilize these options independently.

---
 include/linux/mtd/nand.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index ae67ef5..80b471b 100644
@@ -237,9 +237,9 @@ typedef enum {
  * If passed additionally to NAND_USE_FLASH_BBT then BBT code will not touch
  * the OOB area.
  */
-#define NAND_USE_FLASH_BBT_NO_OOB	0x00100000
+#define NAND_USE_FLASH_BBT_NO_OOB	0x00800000
 /* Create an empty BBT with no vendor information if the BBT is available */
-#define NAND_CREATE_EMPTY_BBT		0x00200000
+#define NAND_CREATE_EMPTY_BBT		0x01000000
 
 /* Options set by nand scan */
 /* Nand scan has allocated controller struct */

1.7.0.4

It is nicer to dynamically create our badblock patterns than to statically define them. The nand_create_default_bbt_descr() function does a sufficient job of handling various bad block scanning options for either flash-based or non-flash-based BBTs, so we might as well use the function for both cases.

This patch simplifies and shortens our code (and removes a TODO that I left a few months ago).

---
 drivers/mtd/nand/nand_bbt.c |   27 ++++-----------------------
 1 files changed, 4 insertions(+), 23 deletions(-)

index a1e8b30..2753bb1 100644
@@ -1276,20 +1276,6 @@ int nand_update_bbt(struct mtd_info *mtd, loff_t offs)
  * while scanning a device for factory marked good / bad blocks. */
 static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
 
-static struct nand_bbt_descr smallpage_flashbased = {
-	.options = NAND_BBT_SCAN2NDPAGE,
-	.offs = NAND_SMALL_BADBLOCK_POS,
-	.len = 1,
-	.pattern = scan_ff_pattern
-};
-
-static struct nand_bbt_descr largepage_flashbased = {
-	.options = NAND_BBT_SCAN2NDPAGE,
-	.offs = NAND_LARGE_BADBLOCK_POS,
-	.len = 2,
-	.pattern = scan_ff_pattern
-};
-
 static uint8_t scan_agand_pattern[] = { 0x1C, 0x71, 0xC7, 0x1C, 0x71, 0xC7 };
 
 static struct nand_bbt_descr agand_flashbased = {

@@ -1355,10 +1341,6 @@ static struct nand_bbt_descr bbt_mirror_no_bbt_descr = {
  * this->badblock_pattern. Thus, this->badblock_pattern should be NULL when
  * passed to this function.
  *
- * TODO: Handle other flags, replace other static structs
- *        (e.g. handle NAND_BBT_FLASH for flash-based BBT,
- *             replace smallpage_flashbased)
- *
  */
 static int nand_create_default_bbt_descr(struct nand_chip *this)
 {

@@ -1422,15 +1404,14 @@ int nand_default_bbt(struct mtd_info *mtd)
 				this->bbt_md = &bbt_mirror_descr;
 			}
 		}
-		if (!this->badblock_pattern) {
-			this->badblock_pattern = (mtd->writesize > 512) ? &largepage_flashbased : &smallpage_flashbased;
-		}
 	} else {
 		this->bbt_td = NULL;
 		this->bbt_md = NULL;
-		if (!this->badblock_pattern)
-			nand_create_default_bbt_descr(this);
 	}
+
+	if (!this->badblock_pattern)
+		nand_create_default_bbt_descr(this);
+
 	return nand_scan_bbt(mtd, this->badblock_pattern);
 }
 

1.7.0.4

Hello,

I'm not quite sure. Many of the "shared" options go into the nand_chip.options field only so that they can later be copied to a nand_bbt_descr.options field. I think this is only out of convenience so that we can detect chip-based BBT options like 'scan 2nd page' before we have actually allocated and assigned our bbt descriptors. For these flags, we can use a new field in nand_chip, like a "nand_chip.bbm_options". Then, many shared flags would really become "early BBT flags" that could safely be copied over without conflict.

Does this make any sense or are there holes in my idea here? I can try an RFC patch soon if that would help.

Thanks,
Brian

I think it might even be better to kill the options field and instead introduce bit-fields like:

unsigned int bbt_scan_byte_1and6:1
unsigned int bbt_scan_2nd_page:1
... and so on for all possible flags ...

And of course put sensible comments.

The only caveat is that this is a bit error-prone because users of these bit-fields may mistakenly decide they are atomic and have nasty race condition when changing them simultaneously from different threads. But it does not look like this is of our concern in MTD case.

What do you think?

Yes, that makes sense for me for sure. Please, send the patch!

Best Regards,
Artem Bityutskiy (Артём Битюцкий)

This RFC begins to handle the conflicts we've been having with using conflicting flags from nand.h and bbm.h in the same nand_chip.options field. We should try to separate these two spaces a little more clearly, and so I have added a bbt_options field to nand_chip.

Important notes about nand_chip fields:
* bbt_options field should contain ONLY flags from bbm.h. They should be able to pass safely to a nand_bbt_descr data structure.
* options field should contian ONLY flags from nand.h. Ideally, they should not be involved in any BBT related options.

Other things to consider (not yet implemented):
* Is it safe to move NAND_CREATE_EMPTY_BBT to bbm.h and require it to be put in bbt_options? It seems not to be used by any in-kernel drivers so it's only likely to mess with independent drivers...

* Consider the following three flags:
(1) NAND_USE_FLASH_BBT (nand.h)
(2) NAND_USE_FLASH_BBT_NO_OOB (nand.h)
(3) NAND_BBT_NO_OOB (bbm.h)

These flags are all related, yet they are in different headers. Also, flag (2) is simply the combination of (1) and (3) and seemingly can be eliminated. Is it safe to move (1) and (3) to bbm.h and remove (2) altogether? (with appropriate code adjustments of course)

* The main problem I see with moving more flags to bbm.h is the implicit requirement for drivers to put these options in the bbt_options field, not the options field, in order to have nand_base handle them properly.

Regarding Artem's suggestion of bit-fields:
If we turn all the flags into bit-fields in nand_chip, we still need to add these fields to the bbt_descr, right? That seems like too much duplication of information and would just be messier.

Please send comments! I plan to implement some more of the changes listed above if there are no real objections or modifications.

Sincerely,
Brian

---
 drivers/mtd/nand/nand_base.c |   15 ++++++++-------
 drivers/mtd/nand/nand_bbt.c  |    6 ++----
 include/linux/mtd/nand.h     |    4 ++++
 3 files changed, 14 insertions(+), 11 deletions(-)

index c54a4cb..9fc7f60 100644
@@ -347,7 +347,7 @@ static int nand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip)
 	struct nand_chip *chip = mtd->priv;
 	u16 bad;
 
-	if (chip->options & NAND_BBT_SCANLASTPAGE)
+	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
 		ofs += mtd->erasesize - mtd->writesize;
 
 	page = (int)(ofs >> chip->page_shift) & chip->pagemask;

@@ -399,7 +399,7 @@ static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs)
 	uint8_t buf[2] = { 0, 0 };
 	int block, ret, i = 0;
 
-	if (chip->options & NAND_BBT_SCANLASTPAGE)
+	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
 		ofs += mtd->erasesize - mtd->writesize;
 
 	/* Get block number */

@@ -426,14 +426,15 @@ static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs)
 
 			ret = nand_do_write_oob(mtd, ofs, &chip->ops);
 
-			if (!ret && (chip->options & NAND_BBT_SCANBYTE1AND6)) {
+			if (!ret && (chip->bbt_options &
+						NAND_BBT_SCANBYTE1AND6)) {
 				chip->ops.ooboffs = NAND_SMALL_BADBLOCK_POS
 					& ~0x01;
 				ret = nand_do_write_oob(mtd, ofs, &chip->ops);
 			}
 			i++;
 			ofs += mtd->writesize;
-		} while (!ret && (chip->options & NAND_BBT_SCAN2NDPAGE) &&
+		} while (!ret && (chip->bbt_options & NAND_BBT_SCAN2NDPAGE) &&
 				i < 2);
 
 		nand_release_device(mtd);

@@ -3128,7 +3129,7 @@ ident_done:
 	if ((chip->cellinfo & NAND_CI_CELLTYPE_MSK) &&
 			(*maf_id == NAND_MFR_SAMSUNG ||
 			 *maf_id == NAND_MFR_HYNIX))
-		chip->options |= NAND_BBT_SCANLASTPAGE;
+		chip->bbt_options |= NAND_BBT_SCANLASTPAGE;
 	else if ((!(chip->cellinfo & NAND_CI_CELLTYPE_MSK) &&
 				(*maf_id == NAND_MFR_SAMSUNG ||
 				 *maf_id == NAND_MFR_HYNIX ||

@@ -3136,7 +3137,7 @@ ident_done:
 				 *maf_id == NAND_MFR_AMD)) ||
 			(mtd->writesize == 2048 &&
 			 *maf_id == NAND_MFR_MICRON))
-		chip->options |= NAND_BBT_SCAN2NDPAGE;
+		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
 
 	/*
 	 * Numonyx/ST 2K pages, x8 bus use BOTH byte 1 and 6

@@ -3144,7 +3145,7 @@ ident_done:
 	if (!(busw & NAND_BUSWIDTH_16) &&
 			*maf_id == NAND_MFR_STMICRO &&
 			mtd->writesize == 2048) {
-		chip->options |= NAND_BBT_SCANBYTE1AND6;
+		chip->bbt_options |= NAND_BBT_SCANBYTE1AND6;
 		chip->badblockpos = 0;
 	}
 

index ccbeaa1..136ae8d 100644
@@ -546,7 +546,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 		from = (loff_t)startblock << (this->bbt_erase_shift - 1);
 	}
 
-	if (this->options & NAND_BBT_SCANLASTPAGE)
+	if (this->bbt_options & NAND_BBT_SCANLASTPAGE)
 		from += mtd->erasesize - (mtd->writesize * len);
 
 	for (i = startblock; i < numblocks;) {

@@ -1330,8 +1330,6 @@ static struct nand_bbt_descr bbt_mirror_no_bbt_descr = {
 	.pattern = mirror_pattern
 };
 
-#define BBT_SCAN_OPTIONS (NAND_BBT_SCANLASTPAGE | NAND_BBT_SCAN2NDPAGE | \
-		NAND_BBT_SCANBYTE1AND6)
 /**
  * nand_create_default_bbt_descr - [Internal] Creates a BBT descriptor structure
  * @this:	NAND chip to create descriptor for

@@ -1354,7 +1352,7 @@ static int nand_create_default_bbt_descr(struct nand_chip *this)
 		printk(KERN_ERR "nand_create_default_bbt_descr: Out of memory\n");
 		return -ENOMEM;
 	}
-	bd->options = this->options & BBT_SCAN_OPTIONS;
+	bd->options = this->bbt_options;
 	bd->offs = this->badblockpos;
 	bd->len = (this->options & NAND_BUSWIDTH_16) ? 2 : 1;
 	bd->pattern = scan_ff_pattern;

index c2b9ac4..42f70e2 100644
@@ -449,6 +449,9 @@ struct nand_buffers {
  * @options:		[BOARDSPECIFIC] various chip options. They can partly
  *			be set to inform nand_scan about special functionality.
  *			See the defines for further explanation.
+ * @bbt_options:	[INTERN] bad block specific options. All options used
+ *			here must come from bbm.h. By default, these options
+ *			will be copied to the appropriate nand_bbt_descr's.
  * @badblockpos:	[INTERN] position of the bad block marker in the oob
  *			area.
  * @badblockbits:	[INTERN] number of bits to left-shift the bad block

@@ -509,6 +512,7 @@ struct nand_chip {
 
 	int chip_delay;
 	unsigned int options;
+	unsigned int bbt_options;
 
 	int page_shift;
 	int phys_erase_shift;

1.7.0.4

Hi Brian,

Sounds good.

Sounds good. I'd add the following:

* NAND chip option flags start with the: NAND_ prefix
* BBT options start with BBT_ prefix.

You may choose different prefixes, e.g., NAND_BBT_ for BBT options, but the separation is needed, I think. Also, the renaming of the options should be a separate patch or set of patches.

I'd also add:
* Every flag should have a nice comment explaining what the flag is.

This is optional, but would be nice :-)

What exactly this option mean and how could it be used?

Yes, I think so.

Let's for get about this for now, then. We can look at this idea at the end of the clean-up then, again, may be.

Thanks!

Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Hi,

Again, sorry for the wait time on this. I've been busy. Now I'm addressing most of these topics and will send a patch soon. I haven't completely answered the following question, though.

I'm not entirely sure how it would be used (perhaps ask Sebastian, the one who introduced it in commit 453281a9), but I have an idea what it means.

It seems that (when combined with NAND_BBT_CREATE) this flag causes nand_scan_bbt() / check_create() to skip the scanning portion of creating a bad block table, effectively leaving an empty bad block table. According to the commit message:
it will create an empty BBT table without considering vendor's BBT information. Vendor's information may be unavailable if the NAND controller has a different DATA & OOB layout or this information may be allready purged.

I'm prepared to simply move this over to bbm.h (next to NAND_BBT_CREATE) if it's worth saving. Otherwise, it can just as easily be dumped.

Brian

Hi Brian,

It is kinda required here :) The long story:
The NAND-controller needs the complete OOB area for ECC. So we have no room for BBT in OOB. Therefore we use BBT table on flash without the recognition marking in the OOB area.
This option (NAND_CREATE_EMPTY_BBT) is used to have an empty BBT table on the first scan. The reason is that the boards are in-some kind production test before I got them and the format which is used there is unknown and Vendor's (Nand-chip Vendor) BBT information in OOB is lost. On the second scan we use the on-flash BBT information (which is created by mtd) so it is not empty again.

Sebastian

Hi Sebastian,

I assume you're using CREATE_EMPTY_BBT along with the new flag NAND_USE_FLASH_BBT_NO_OOB, then? FYI, I'm reworking that option as well so please review my patches.

I'm a little confused here. Who/what gives the on-flash BBT information? It seems to me like a combination of the "FLASH_BBT_NO_OOB" options and "CREATE_EMPTY_BBT" will leave you with an empty, flash-based BBT that is stored in-band. It would not contain any manufacturer info, as you stated that "information in OOB is lost."

Anyway, it looks like this option may be worth saving, but I'm just trying to get a handle of the rationale and usage so that we can properly document it.

Brian

I've pushed both patches to l2-mtd-2.6.git, thanks.

P.S. Hopefully you won't be discouraged from doing further flags polishing :-).

Best Regards,
Artem Bityutskiy (Артём Битюцкий)