linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] mtd: nand: renumber conflicting BBT flags
@ 2011-03-19  4:53 Brian Norris
  2011-03-19  4:53 ` [PATCH 2/2] mtd: nand: dynamic allocation of flash-based BBT structs Brian Norris
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Brian Norris @ 2011-03-19  4:53 UTC (permalink / raw)
  To: linux-mtd
  Cc: Brian Norris, Sebastian Andrzej Siewior, Kevin Cernekee,
	David Woodhouse, Artem Bityutskiy

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.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 include/linux/mtd/nand.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index ae67ef5..80b471b 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] mtd: nand: dynamic allocation of flash-based BBT structs
  2011-03-19  4:53 [PATCH 1/2] mtd: nand: renumber conflicting BBT flags Brian Norris
@ 2011-03-19  4:53 ` Brian Norris
  2011-03-31 12:58 ` [PATCH 1/2] mtd: nand: renumber conflicting BBT flags Artem Bityutskiy
  2011-04-04  7:58 ` [PATCH 1/2] mtd: nand: renumber conflicting BBT flags Artem Bityutskiy
  2 siblings, 0 replies; 11+ messages in thread
From: Brian Norris @ 2011-03-19  4:53 UTC (permalink / raw)
  To: linux-mtd
  Cc: Brian Norris, Sebastian Andrzej Siewior, Kevin Cernekee,
	David Woodhouse, Artem Bityutskiy

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

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_bbt.c |   27 ++++-----------------------
 1 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index a1e8b30..2753bb1 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] mtd: nand: renumber conflicting BBT flags
  2011-03-19  4:53 [PATCH 1/2] mtd: nand: renumber conflicting BBT flags Brian Norris
  2011-03-19  4:53 ` [PATCH 2/2] mtd: nand: dynamic allocation of flash-based BBT structs Brian Norris
@ 2011-03-31 12:58 ` Artem Bityutskiy
  2011-04-02  8:04   ` Brian Norris
  2011-04-04  7:58 ` [PATCH 1/2] mtd: nand: renumber conflicting BBT flags Artem Bityutskiy
  2 siblings, 1 reply; 11+ messages in thread
From: Artem Bityutskiy @ 2011-03-31 12:58 UTC (permalink / raw)
  To: Brian Norris
  Cc: David Woodhouse, Sebastian Andrzej Siewior, Kevin Cernekee,
	linux-mtd

On Fri, 2011-03-18 at 21:53 -0700, Brian Norris wrote:
> 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.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  include/linux/mtd/nand.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index ae67ef5..80b471b 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -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

Hmm, it seems that the issue is that flags which belong to the same
"space" should be in a single file. AFAICS, we have 2 spaces:

1. Chip flags
2. BBT flags

They are 2 different things. But some of the flags are shared. And this
is quite subtle thing.

What I think we should do instead is to avoid sharing the same symbolic
constant between 2 different spaces. Is this possible?

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] mtd: nand: renumber conflicting BBT flags
  2011-03-31 12:58 ` [PATCH 1/2] mtd: nand: renumber conflicting BBT flags Artem Bityutskiy
@ 2011-04-02  8:04   ` Brian Norris
  2011-04-04  7:52     ` Artem Bityutskiy
  0 siblings, 1 reply; 11+ messages in thread
From: Brian Norris @ 2011-04-02  8:04 UTC (permalink / raw)
  To: dedekind1
  Cc: David Woodhouse, Sebastian Andrzej Siewior, Kevin Cernekee,
	linux-mtd

Hello,

On 3/31/2011 5:58 AM, Artem Bityutskiy wrote:
> Hmm, it seems that the issue is that flags which belong to the same
> "space" should be in a single file. AFAICS, we have 2 spaces:
> 
> 1. Chip flags
> 2. BBT flags
> 
> They are 2 different things. But some of the flags are shared. And this
> is quite subtle thing.
> 
> What I think we should do instead is to avoid sharing the same symbolic
> constant between 2 different spaces. Is this possible?

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] mtd: nand: renumber conflicting BBT flags
  2011-04-02  8:04   ` Brian Norris
@ 2011-04-04  7:52     ` Artem Bityutskiy
  2011-04-20  7:13       ` [RFC] mtd: nand: separate chip options / bbt_options Brian Norris
  0 siblings, 1 reply; 11+ messages in thread
From: Artem Bityutskiy @ 2011-04-04  7:52 UTC (permalink / raw)
  To: Brian Norris
  Cc: David Woodhouse, Sebastian Andrzej Siewior, Kevin Cernekee,
	linux-mtd

On Sat, 2011-04-02 at 01:04 -0700, Brian Norris wrote:
> Hello,
> 
> On 3/31/2011 5:58 AM, Artem Bityutskiy wrote:
> > Hmm, it seems that the issue is that flags which belong to the same
> > "space" should be in a single file. AFAICS, we have 2 spaces:
> > 
> > 1. Chip flags
> > 2. BBT flags
> > 
> > They are 2 different things. But some of the flags are shared. And this
> > is quite subtle thing.
> > 
> > What I think we should do instead is to avoid sharing the same symbolic
> > constant between 2 different spaces. Is this possible?
> 
> 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.

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?

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

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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] mtd: nand: renumber conflicting BBT flags
  2011-03-19  4:53 [PATCH 1/2] mtd: nand: renumber conflicting BBT flags Brian Norris
  2011-03-19  4:53 ` [PATCH 2/2] mtd: nand: dynamic allocation of flash-based BBT structs Brian Norris
  2011-03-31 12:58 ` [PATCH 1/2] mtd: nand: renumber conflicting BBT flags Artem Bityutskiy
@ 2011-04-04  7:58 ` Artem Bityutskiy
  2 siblings, 0 replies; 11+ messages in thread
From: Artem Bityutskiy @ 2011-04-04  7:58 UTC (permalink / raw)
  To: Brian Norris
  Cc: David Woodhouse, Sebastian Andrzej Siewior, Kevin Cernekee,
	linux-mtd

On Fri, 2011-03-18 at 21:53 -0700, Brian Norris wrote:
> 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.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

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 (Артём Битюцкий)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RFC] mtd: nand: separate chip options / bbt_options
  2011-04-04  7:52     ` Artem Bityutskiy
@ 2011-04-20  7:13       ` Brian Norris
  2011-04-22  8:02         ` Artem Bityutskiy
  0 siblings, 1 reply; 11+ messages in thread
From: Brian Norris @ 2011-04-20  7:13 UTC (permalink / raw)
  To: Artem Bityutskiy
  Cc: Brian Norris, linux-mtd, Sebastian Andrzej Siewior,
	Kevin Cernekee, David Woodhouse

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

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index c54a4cb..9fc7f60 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -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;
 	}
 
diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index ccbeaa1..136ae8d 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -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;
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index c2b9ac4..42f70e2 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [RFC] mtd: nand: separate chip options / bbt_options
  2011-04-20  7:13       ` [RFC] mtd: nand: separate chip options / bbt_options Brian Norris
@ 2011-04-22  8:02         ` Artem Bityutskiy
  2011-05-25 18:15           ` Brian Norris
  0 siblings, 1 reply; 11+ messages in thread
From: Artem Bityutskiy @ 2011-04-22  8:02 UTC (permalink / raw)
  To: Brian Norris
  Cc: linux-mtd, Sebastian Andrzej Siewior, Kevin Cernekee,
	David Woodhouse

Hi Brian,

On Wed, 2011-04-20 at 00:13 -0700, Brian Norris wrote:
> 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.

Sounds good.

> 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.

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

> 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...

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

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

Yes, I think so.

> 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.

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 (Артём Битюцкий)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] mtd: nand: separate chip options / bbt_options
  2011-04-22  8:02         ` Artem Bityutskiy
@ 2011-05-25 18:15           ` Brian Norris
  2011-05-26  8:04             ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 11+ messages in thread
From: Brian Norris @ 2011-05-25 18:15 UTC (permalink / raw)
  To: dedekind1
  Cc: linux-mtd, Sebastian Andrzej Siewior, Kevin Cernekee,
	David Woodhouse

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.

On Fri, Apr 22, 2011 at 1:02 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-04-20 at 00:13 -0700, Brian Norris wrote:
>> 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...
>
> What exactly this option mean and how could it be used?

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] mtd: nand: separate chip options / bbt_options
  2011-05-25 18:15           ` Brian Norris
@ 2011-05-26  8:04             ` Sebastian Andrzej Siewior
  2011-05-31 17:25               ` Brian Norris
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2011-05-26  8:04 UTC (permalink / raw)
  To: Brian Norris; +Cc: linux-mtd, Kevin Cernekee, David Woodhouse, dedekind1

Brian Norris wrote:
> Hi,
Hi Brian,

> On Fri, Apr 22, 2011 at 1:02 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> On Wed, 2011-04-20 at 00:13 -0700, Brian Norris wrote:
>>> 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...
>> What exactly this option mean and how could it be used?
> 
> 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.

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.

> 
> Brian

Sebastian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] mtd: nand: separate chip options / bbt_options
  2011-05-26  8:04             ` Sebastian Andrzej Siewior
@ 2011-05-31 17:25               ` Brian Norris
  0 siblings, 0 replies; 11+ messages in thread
From: Brian Norris @ 2011-05-31 17:25 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-mtd, Kevin Cernekee, David Woodhouse, dedekind1

Hi Sebastian,

On Thu, May 26, 2011 at 1:04 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> 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.

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.

> On the second
> scan we use the on-flash BBT information (which is created by mtd) so it
> is not empty again.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2011-05-31 17:27 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-19  4:53 [PATCH 1/2] mtd: nand: renumber conflicting BBT flags Brian Norris
2011-03-19  4:53 ` [PATCH 2/2] mtd: nand: dynamic allocation of flash-based BBT structs Brian Norris
2011-03-31 12:58 ` [PATCH 1/2] mtd: nand: renumber conflicting BBT flags Artem Bityutskiy
2011-04-02  8:04   ` Brian Norris
2011-04-04  7:52     ` Artem Bityutskiy
2011-04-20  7:13       ` [RFC] mtd: nand: separate chip options / bbt_options Brian Norris
2011-04-22  8:02         ` Artem Bityutskiy
2011-05-25 18:15           ` Brian Norris
2011-05-26  8:04             ` Sebastian Andrzej Siewior
2011-05-31 17:25               ` Brian Norris
2011-04-04  7:58 ` [PATCH 1/2] mtd: nand: renumber conflicting BBT flags Artem Bityutskiy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).