[U-Boot] [PATCH 0/5] Random NAND fixes and improvements

[U-Boot] [PATCH 0/5] Random NAND fixes and improvements

[Marek Vasut]

This is a resend of the NAND fixes and improvements. These are squashed
togethere here and sent as a batch.

Marek Vasut (5):
  NAND: Really ignore bad blocks when scrubbing
  NAND: Add nand read.raw and write.raw commands
  NAND: Allow per-buffer allocation
  NAND: Make page, erase, oob size available via cmd_nand
  NAND: Add scrub.quiet command option

 common/cmd_nand.c            |   70 +++++++++++++++++++++++++++++++++++++++--
 drivers/mtd/nand/nand_base.c |   32 ++++++++++++++-----
 drivers/mtd/nand/nand_util.c |   22 +++----------
 include/linux/mtd/mtd.h      |    1 +
 include/linux/mtd/nand.h     |    7 ++--
 5 files changed, 100 insertions(+), 32 deletions(-)

-- 
1.7.5.4

[U-Boot] [PATCH 1/5] NAND: Really ignore bad blocks when scrubbing

[Marek Vasut]

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
---
 drivers/mtd/nand/nand_base.c |    2 +-
 drivers/mtd/nand/nand_util.c |   22 +++++-----------------
 include/linux/mtd/mtd.h      |    1 +
 3 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 1a95a91..d8d30e3 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2224,7 +2224,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 		/*
 		 * heck if we have a bad block, we do not erase bad blocks !
 		 */
-		if (nand_block_checkbad(mtd, ((loff_t) page) <<
+		if (!instr->scrub && nand_block_checkbad(mtd, ((loff_t) page) <<
 					chip->page_shift, 0, allowbbt)) {
 			printk(KERN_WARNING "nand_erase: attempt to erase a "
 			       "bad block at page 0x%08x\n", page);
diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c
index 81bf366..0c3b7f7 100644
--- a/drivers/mtd/nand/nand_util.c
+++ b/drivers/mtd/nand/nand_util.c
@@ -57,12 +57,6 @@ typedef struct mtd_info	  mtd_info_t;
 #define cpu_to_je16(x) (x)
 #define cpu_to_je32(x) (x)
 
-/*****************************************************************************/
-static int nand_block_bad_scrub(struct mtd_info *mtd, loff_t ofs, int getchip)
-{
-	return 0;
-}
-
 /**
  * nand_erase_opts: - erase NAND flash with support for various options
  *		      (jffs2 formating)
@@ -82,10 +76,10 @@ int nand_erase_opts(nand_info_t *meminfo, const nand_erase_options_t *opts)
 	int bbtest = 1;
 	int result;
 	int percent_complete = -1;
-	int (*nand_block_bad_old)(struct mtd_info *, loff_t, int) = NULL;
 	const char *mtd_device = meminfo->name;
 	struct mtd_oob_ops oob_opts;
 	struct nand_chip *chip = meminfo->priv;
+	struct nand_chip *priv_nand = meminfo->priv;
 
 	if ((opts->offset & (meminfo->writesize - 1)) != 0) {
 		printf("Attempt to erase non page aligned data\n");
@@ -110,11 +104,9 @@ int nand_erase_opts(nand_info_t *meminfo, const nand_erase_options_t *opts)
 	 * and disable bad block table while erasing.
 	 */
 	if (opts->scrub) {
-		struct nand_chip *priv_nand = meminfo->priv;
-
-		nand_block_bad_old = priv_nand->block_bad;
-		priv_nand->block_bad = nand_block_bad_scrub;
-		/* we don't need the bad block table anymore...
+		erase.scrub = opts->scrub;
+		/*
+		 * We don't need the bad block table anymore...
 		 * after scrub, there are no bad blocks left!
 		 */
 		if (priv_nand->bbt) {
@@ -204,12 +196,8 @@ int nand_erase_opts(nand_info_t *meminfo, const nand_erase_options_t *opts)
 	if (!opts->quiet)
 		printf("\n");
 
-	if (nand_block_bad_old) {
-		struct nand_chip *priv_nand = meminfo->priv;
-
-		priv_nand->block_bad = nand_block_bad_old;
+	if (opts->scrub)
 		priv_nand->scan_bbt(meminfo);
-	}
 
 	return 0;
 }
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index d36d584..141c960 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -55,6 +55,7 @@ struct erase_info {
 	u_long priv;
 	u_char state;
 	struct erase_info *next;
+	int scrub;
 };
 
 struct mtd_erase_region_info {
-- 
1.7.5.4

[U-Boot] [PATCH 2/5] NAND: Add nand read.raw and write.raw commands

[Marek Vasut]

These commands should work around various "hardware" ECC and BCH methods.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
---
 common/cmd_nand.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 66e06a5..a1c8dfd 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -606,6 +606,20 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 				ret = nand->read_oob(nand, off, &ops);
 			else
 				ret = nand->write_oob(nand, off, &ops);
+		} else if (!strcmp(s, ".raw")) {
+			/* Raw access */
+			mtd_oob_ops_t ops = {
+				.datbuf = (u8 *)addr,
+				.oobbuf = ((u8 *)addr) + nand->writesize,
+				.len = nand->writesize,
+				.ooblen = nand->oobsize,
+				.mode = MTD_OOB_RAW
+			};
+
+			if (read)
+				ret = nand->read_oob(nand, off, &ops);
+			else
+				ret = nand->write_oob(nand, off, &ops);
 		} else {
 			printf("Unknown nand command suffix '%s'.\n", s);
 			return 1;
-- 
1.7.5.4

[U-Boot] [PATCH 3/5] NAND: Allow per-buffer allocation

[Marek Vasut]

Don't allocate NAND buffers as one block, but allocate them separately. This
allows systems where DMA to buffers happen to allocate these buffers properly
aligned.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
---
 drivers/mtd/nand/nand_base.c |   30 +++++++++++++++++++++++-------
 include/linux/mtd/nand.h     |    7 ++++---
 2 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index d8d30e3..3093067 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2749,13 +2749,27 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
  */
 int nand_scan_tail(struct mtd_info *mtd)
 {
-	int i;
+	int i, bufsize;
+	uint8_t *buf;
 	struct nand_chip *chip = mtd->priv;
 
-	if (!(chip->options & NAND_OWN_BUFFERS))
-		chip->buffers = kmalloc(sizeof(*chip->buffers), GFP_KERNEL);
-	if (!chip->buffers)
-		return -ENOMEM;
+	if (!(chip->options & NAND_OWN_BUFFERS)) {
+		chip->buffers = malloc(sizeof(struct nand_buffers));
+		if (!chip->buffers)
+			return -ENOMEM;
+
+		bufsize = NAND_MAX_PAGESIZE + (3 * NAND_MAX_OOBSIZE);
+		buf = malloc(bufsize);
+		if (!buf) {
+			free(chip->buffers);
+			return -ENOMEM;
+		}
+
+		chip->buffers->buffer = buf;
+		chip->buffers->ecccalc = buf;
+		chip->buffers->ecccode = buf + NAND_MAX_OOBSIZE;
+		chip->buffers->databuf = buf + (2 * NAND_MAX_OOBSIZE);
+	}
 
 	/* Set the internal oob buffer location, just after the page data */
 	chip->oob_poi = chip->buffers->databuf + mtd->writesize;
@@ -2996,6 +3010,8 @@ void nand_release(struct mtd_info *mtd)
 
 	/* Free bad block table memory */
 	kfree(chip->bbt);
-	if (!(chip->options & NAND_OWN_BUFFERS))
-		kfree(chip->buffers);
+	if (!(chip->options & NAND_OWN_BUFFERS)) {
+		free(chip->buffers->buffer);
+		free(chip->buffers);
+	}
 }
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 987a2ec..c3449a9 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -370,9 +370,10 @@ struct nand_ecc_ctrl {
  * consecutive order.
  */
 struct nand_buffers {
-	uint8_t	ecccalc[NAND_MAX_OOBSIZE];
-	uint8_t	ecccode[NAND_MAX_OOBSIZE];
-	uint8_t databuf[NAND_MAX_PAGESIZE + NAND_MAX_OOBSIZE];
+	uint8_t *buffer;
+	uint8_t	*ecccalc;
+	uint8_t	*ecccode;
+	uint8_t *databuf;
 };
 
 /**
-- 
1.7.5.4

[U-Boot] [PATCH 4/5] NAND: Make page, erase, oob size available via cmd_nand

[Marek Vasut]

The "nand info" and "nand device" now set shell/environment variables:
	nand_writesize ... nand page size
	nand_oobsize ..... nand oob area size
	nand_erasesize ... nand erase block size

The shell variables are only set if HUSH is enabled.

Also, the "nand info" command now displays this info.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
---
 common/cmd_nand.c |   42 +++++++++++++++++++++++++++++++++++++++---
 1 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index a1c8dfd..5b7e83d 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -27,6 +27,9 @@
 #include <asm/byteorder.h>
 #include <jffs2/jffs2.h>
 #include <nand.h>
+#ifdef	CONFIG_SYS_HUSH_PARSER
+#include <hush.h>
+#endif
 
 #if defined(CONFIG_CMD_MTDPARTS)
 
@@ -362,15 +365,48 @@ usage:
 
 #endif
 
-static void nand_print_info(int idx)
+static void nand_print_and_set_info(int idx)
 {
 	nand_info_t *nand = &nand_info[idx];
 	struct nand_chip *chip = nand->priv;
+	const int bufsz = 32;
+	char buf[bufsz];
+
 	printf("Device %d: ", idx);
 	if (chip->numchips > 1)
 		printf("%dx ", chip->numchips);
 	printf("%s, sector size %u KiB\n",
 	       nand->name, nand->erasesize >> 10);
+	printf("  Page size  %8d b\n", nand->writesize);
+	printf("  OOB size   %8d b\n", nand->oobsize);
+	printf("  Erase size %8d b\n", nand->erasesize);
+
+	/* Set geometry info */
+#ifdef	CONFIG_SYS_HUSH_PARSER
+	memset(buf, 0, bufsz);
+	sprintf(buf, "nand_writesize=%x", nand->writesize);
+	set_local_var(buf, 0);
+
+	memset(buf, 0, bufsz);
+	sprintf(buf, "nand_oobsize=%x", nand->oobsize);
+	set_local_var(buf, 0);
+
+	memset(buf, 0, bufsz);
+	sprintf(buf, "nand_erasesize=%x", nand->erasesize);
+	set_local_var(buf, 0);
+#else
+	memset(buf, 0, bufsz);
+	sprintf(buf, "%x", nand->writesize);
+	setenv("nand_writesize", buf);
+
+	memset(buf, 0, bufsz);
+	sprintf(buf, "%x", nand->oobsize);
+	setenv("nand_oobsize", buf);
+
+	memset(buf, 0, bufsz);
+	sprintf(buf, "%x", nand->erasesize);
+	setenv("nand_erasesize", buf);
+#endif
 }
 
 int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
@@ -407,7 +443,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 		putc('\n');
 		for (i = 0; i < CONFIG_SYS_MAX_NAND_DEVICE; i++) {
 			if (nand_info[i].name)
-				nand_print_info(i);
+				nand_print_and_set_info(i);
 		}
 		return 0;
 	}
@@ -418,7 +454,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 			if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE)
 				puts("no devices available\n");
 			else
-				nand_print_info(dev);
+				nand_print_and_set_info(dev);
 			return 0;
 		}
 
-- 
1.7.5.4

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Marek Vasut]

This allows the scrub command to scrub without asking the user if he really
wants to scrub the area. Useful in scripts.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
---
 common/cmd_nand.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 5b7e83d..45179e9 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -502,11 +502,19 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 		int clean = argc > 2 && !strcmp("clean", argv[2]);
 		int o = clean ? 3 : 2;
 		int scrub = !strncmp(cmd, "scrub", 5);
+		int scrub_quiet = !strncmp(cmd, "scrub.quiet", 11);
 		int part = 0;
 		int chip = 0;
 		int spread = 0;
 		int args = 2;
 
+		/*
+		 * Quiet scrub is a special option only for the scrub command,
+		 * ignore it in the following construction.
+		 */
+		if (scrub_quiet)
+			cmd[5] = 0;
+
 		if (cmd[5] != 0) {
 			if (!strcmp(&cmd[5], ".spread")) {
 				spread = 1;
@@ -543,7 +551,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 		opts.quiet  = quiet;
 		opts.spread = spread;
 
-		if (scrub) {
+		if (scrub && !scrub_quiet) {
 			puts("Warning: "
 			     "scrub option will erase all factory set "
 			     "bad blocks!\n"
@@ -569,6 +577,10 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 				return -1;
 			}
 		}
+
+		if (scrub_quiet)
+			opts.scrub = 1;
+
 		ret = nand_erase_opts(nand, &opts);
 		printf("%s\n", ret ? "ERROR" : "OK");
 
-- 
1.7.5.4

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Detlev Zundel]

Hi Marek,

> This allows the scrub command to scrub without asking the user if he really
> wants to scrub the area. Useful in scripts.
>
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Detlev Zundel <dzu@denx.de>
> ---
>  common/cmd_nand.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
>
> diff --git a/common/cmd_nand.c b/common/cmd_nand.c
> index 5b7e83d..45179e9 100644
> --- a/common/cmd_nand.c
> +++ b/common/cmd_nand.c
> @@ -502,11 +502,19 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>  		int clean = argc > 2 && !strcmp("clean", argv[2]);
>  		int o = clean ? 3 : 2;
>  		int scrub = !strncmp(cmd, "scrub", 5);
> +		int scrub_quiet = !strncmp(cmd, "scrub.quiet", 11);
>  		int part = 0;
>  		int chip = 0;
>  		int spread = 0;
>  		int args = 2;
>  
> +		/*
> +		 * Quiet scrub is a special option only for the scrub command,
> +		 * ignore it in the following construction.
> +		 */
> +		if (scrub_quiet)
> +			cmd[5] = 0;
> +

This is _really_ hackish and makes an effort not to fit into the coding
style at hand.  Please use the available code and extend the construct
below to match the ".quiet" suffix.  It is not that different.

>  		if (cmd[5] != 0) {
>  			if (!strcmp(&cmd[5], ".spread")) {
>  				spread = 1;
> @@ -543,7 +551,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>  		opts.quiet  = quiet;
>  		opts.spread = spread;
>  
> -		if (scrub) {
> +		if (scrub && !scrub_quiet) {
>  			puts("Warning: "
>  			     "scrub option will erase all factory set "
>  			     "bad blocks!\n"
> @@ -569,6 +577,10 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>  				return -1;
>  			}
>  		}
> +
> +		if (scrub_quiet)
> +			opts.scrub = 1;
> +

Urgh.  Please turn this into

if (scrub) {
   if (!scrub_quiet) {
   } else {
   }  
}

Cheers
  Detlev

-- 
In the topologic hell the beer is packed in Klein's bottles.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Marek Vasut]

On Friday, September 09, 2011 05:39:07 PM Detlev Zundel wrote:
> Hi Marek,
> 
> > This allows the scrub command to scrub without asking the user if he
> > really wants to scrub the area. Useful in scripts.
> > 
> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > Cc: Scott Wood <scottwood@freescale.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > Cc: Wolfgang Denk <wd@denx.de>
> > Cc: Detlev Zundel <dzu@denx.de>
> > ---
> > 
> >  common/cmd_nand.c |   14 +++++++++++++-
> >  1 files changed, 13 insertions(+), 1 deletions(-)
> > 
> > diff --git a/common/cmd_nand.c b/common/cmd_nand.c
> > index 5b7e83d..45179e9 100644
> > --- a/common/cmd_nand.c
> > +++ b/common/cmd_nand.c
> > @@ -502,11 +502,19 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc,
> > char * const argv[])
> > 
> >  		int clean = argc > 2 && !strcmp("clean", argv[2]);
> >  		int o = clean ? 3 : 2;
> >  		int scrub = !strncmp(cmd, "scrub", 5);
> > 
> > +		int scrub_quiet = !strncmp(cmd, "scrub.quiet", 11);
> > 
> >  		int part = 0;
> >  		int chip = 0;
> >  		int spread = 0;
> >  		int args = 2;
> > 
> > +		/*
> > +		 * Quiet scrub is a special option only for the scrub command,
> > +		 * ignore it in the following construction.
> > +		 */
> > +		if (scrub_quiet)
> > +			cmd[5] = 0;
> > +
> 
> This is _really_ hackish and makes an effort not to fit into the coding
> style at hand.  Please use the available code and extend the construct
> below to match the ".quiet" suffix.  It is not that different.

Right, I got a bit wild in here. Though still, it'll be a special case, like

} else if (!strncmp(cmd, "scrub.quiet", 11)) {

in the block below, because quiet should only work for scrub (it's no use for 
other commands).

> 
> >  		if (cmd[5] != 0) {
> >  		
> >  			if (!strcmp(&cmd[5], ".spread")) {
> >  			
> >  				spread = 1;
> > 
> > @@ -543,7 +551,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc,
> > char * const argv[])
> > 
> >  		opts.quiet  = quiet;
> >  		opts.spread = spread;
> > 
> > -		if (scrub) {
> > +		if (scrub && !scrub_quiet) {
> > 
> >  			puts("Warning: "
> >  			
> >  			     "scrub option will erase all factory set "
> >  			     "bad blocks!\n"
> > 
> > @@ -569,6 +577,10 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc,
> > char * const argv[])
> > 
> >  				return -1;
> >  			
> >  			}
> >  		
> >  		}
> > 
> > +
> > +		if (scrub_quiet)
> > +			opts.scrub = 1;
> > +
> 
> Urgh.  Please turn this into

What about:

if (scrub && scrub.quiet) {
	opts.scrub = 1;
} else if (scrub) {
...
}

To avoid fragmenting the code too much and avoid too deep indent.

Cheers!
> 
> if (scrub) {
>    if (!scrub_quiet) {
>    } else {
>    }
> }
> 
> Cheers
>   Detlev

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Marek Vasut]

This allows the scrub command to scrub without asking the user if he really
wants to scrub the area. Useful in scripts.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
---
 common/cmd_nand.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 5b7e83d..6d66e5d 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -502,6 +502,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 		int clean = argc > 2 && !strcmp("clean", argv[2]);
 		int o = clean ? 3 : 2;
 		int scrub = !strncmp(cmd, "scrub", 5);
+		int scrub_quiet;
 		int part = 0;
 		int chip = 0;
 		int spread = 0;
@@ -516,6 +517,8 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 			} else if (!strcmp(&cmd[5], ".chip")) {
 				chip = 1;
 				args = 0;
+			} else if (!strncmp(cmd, "scrub.quiet", 11)) {
+				scrub_quiet = 1;
 			} else {
 				goto usage;
 			}
@@ -543,7 +546,9 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 		opts.quiet  = quiet;
 		opts.spread = spread;
 
-		if (scrub) {
+		if (scrub && scrub_quiet) {
+			opts.scrub = 1;
+		} else if (scrub) {
 			puts("Warning: "
 			     "scrub option will erase all factory set "
 			     "bad blocks!\n"
@@ -569,6 +574,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 				return -1;
 			}
 		}
+
 		ret = nand_erase_opts(nand, &opts);
 		printf("%s\n", ret ? "ERROR" : "OK");
 
-- 
1.7.5.4

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Detlev Zundel]

Hi Marek,

> On Friday, September 09, 2011 05:39:07 PM Detlev Zundel wrote:
>> Hi Marek,
>> 
>> > This allows the scrub command to scrub without asking the user if he
>> > really wants to scrub the area. Useful in scripts.
>> > 
>> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>> > Cc: Scott Wood <scottwood@freescale.com>
>> > Cc: Stefano Babic <sbabic@denx.de>
>> > Cc: Wolfgang Denk <wd@denx.de>
>> > Cc: Detlev Zundel <dzu@denx.de>
>> > ---
>> > 
>> >  common/cmd_nand.c |   14 +++++++++++++-
>> >  1 files changed, 13 insertions(+), 1 deletions(-)
>> > 
>> > diff --git a/common/cmd_nand.c b/common/cmd_nand.c
>> > index 5b7e83d..45179e9 100644
>> > --- a/common/cmd_nand.c
>> > +++ b/common/cmd_nand.c
>> > @@ -502,11 +502,19 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc,
>> > char * const argv[])
>> > 
>> >  		int clean = argc > 2 && !strcmp("clean", argv[2]);
>> >  		int o = clean ? 3 : 2;
>> >  		int scrub = !strncmp(cmd, "scrub", 5);
>> > 
>> > +		int scrub_quiet = !strncmp(cmd, "scrub.quiet", 11);
>> > 
>> >  		int part = 0;
>> >  		int chip = 0;
>> >  		int spread = 0;
>> >  		int args = 2;
>> > 
>> > +		/*
>> > +		 * Quiet scrub is a special option only for the scrub command,
>> > +		 * ignore it in the following construction.
>> > +		 */
>> > +		if (scrub_quiet)
>> > +			cmd[5] = 0;
>> > +
>> 
>> This is _really_ hackish and makes an effort not to fit into the coding
>> style at hand.  Please use the available code and extend the construct
>> below to match the ".quiet" suffix.  It is not that different.
>
> Right, I got a bit wild in here. Though still, it'll be a special case, like
>
> } else if (!strncmp(cmd, "scrub.quiet", 11)) {
>
> in the block below, because quiet should only work for scrub (it's no use for 
> other commands).

Ok.

>> >  		if (cmd[5] != 0) {
>> >  		
>> >  			if (!strcmp(&cmd[5], ".spread")) {
>> >  			
>> >  				spread = 1;
>> > 
>> > @@ -543,7 +551,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc,
>> > char * const argv[])
>> > 
>> >  		opts.quiet  = quiet;
>> >  		opts.spread = spread;
>> > 
>> > -		if (scrub) {
>> > +		if (scrub && !scrub_quiet) {
>> > 
>> >  			puts("Warning: "
>> >  			
>> >  			     "scrub option will erase all factory set "
>> >  			     "bad blocks!\n"
>> > 
>> > @@ -569,6 +577,10 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc,
>> > char * const argv[])
>> > 
>> >  				return -1;
>> >  			
>> >  			}
>> >  		
>> >  		}
>> > 
>> > +
>> > +		if (scrub_quiet)
>> > +			opts.scrub = 1;
>> > +
>> 
>> Urgh.  Please turn this into
>
> What about:
>
> if (scrub && scrub.quiet) {
> 	opts.scrub = 1;
> } else if (scrub) {
> ...
> }
>
> To avoid fragmenting the code too much and avoid too deep indent.
>> 
>> if (scrub) {
>>    if (!scrub_quiet) {
>>    } else {
>>    }
>> }

I think it expresses the intention less clear, but I don't care enough
to nak such a thing - it's still better than the current version.

Cherrs
  Detlev  

-- 
Irrationality is the square root of all evil.
                                     -- Douglas Hofstadter
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Mike Frysinger]

On Monday, September 12, 2011 00:04:10 Marek Vasut wrote:
> This allows the scrub command to scrub without asking the user if he really
> wants to scrub the area. Useful in scripts.

"quiet" and "skip user input" are two different things.  can you use a more 
clean option like accepting "-y" to the "scrub" subcommand ?

> @@ -569,6 +574,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char
> * const argv[]) return -1;
>  			}
>  		}
> +
>  		ret = nand_erase_opts(nand, &opts);
>  		printf("%s\n", ret ? "ERROR" : "OK");

unrelated whitespace changes are frowned upon
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110912/d6d9c825/attachment.pgp 

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Marek Vasut]

On Monday, September 12, 2011 06:45:43 PM Mike Frysinger wrote:
> On Monday, September 12, 2011 00:04:10 Marek Vasut wrote:
> > This allows the scrub command to scrub without asking the user if he
> > really wants to scrub the area. Useful in scripts.
> 
> "quiet" and "skip user input" are two different things.  can you use a more
> clean option like accepting "-y" to the "scrub" subcommand ?

I'd prefer to have this hidden from common users as much as possible.

> 
> > @@ -569,6 +574,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc,
> > char * const argv[]) return -1;
> > 
> >  			}
> >  		
> >  		}
> > 
> > +
> > 
> >  		ret = nand_erase_opts(nand, &opts);
> >  		printf("%s\n", ret ? "ERROR" : "OK");
> 
> unrelated whitespace changes are frowned upon
> -mike

Thanks for finding this.

Cheers

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Scott Wood]

On 09/12/2011 12:45 PM, Marek Vasut wrote:
> On Monday, September 12, 2011 06:45:43 PM Mike Frysinger wrote:
>> On Monday, September 12, 2011 00:04:10 Marek Vasut wrote:
>>> This allows the scrub command to scrub without asking the user if he
>>> really wants to scrub the area. Useful in scripts.
>>
>> "quiet" and "skip user input" are two different things.  can you use a more
>> clean option like accepting "-y" to the "scrub" subcommand ?
> 
> I'd prefer to have this hidden from common users as much as possible.

What's the use case for needing to script this, BTW?

-Scott

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Marek Vasut]

On Monday, September 12, 2011 08:06:27 PM Scott Wood wrote:
> On 09/12/2011 12:45 PM, Marek Vasut wrote:
> > On Monday, September 12, 2011 06:45:43 PM Mike Frysinger wrote:
> >> On Monday, September 12, 2011 00:04:10 Marek Vasut wrote:
> >>> This allows the scrub command to scrub without asking the user if he
> >>> really wants to scrub the area. Useful in scripts.
> >> 
> >> "quiet" and "skip user input" are two different things.  can you use a
> >> more clean option like accepting "-y" to the "scrub" subcommand ?
> > 
> > I'd prefer to have this hidden from common users as much as possible.
> 
> What's the use case for needing to script this, BTW?

Update a block in NAND that's not handled by BCH accelerator in the MX28 chip.

The problem is, block 0 has it's own ECC done by bootrom software. That kind of 
ECC is incompatible with BCH-produced ECC. That's also a reason for needing that 
write.raw command.

Now, if you try erasing that block, the BCH reads and writes some of it's 
metadata there. Obviously, since there is different kind of ECC, the metadata 
aren't there and it chokes, claiming the block is bad and refuses to erase it.

And before you ask why -- that's because the BCH accelerator puts the metadata 
at random places in the block (every 512 bytes, it puts a few bytes of it's ECC) 
instead of putting them only to the ECC area. On the other hand, the bootrom ECC 
puts the whole ECC at offset (1024 + 12) bytes from the start of the block 0.

And finally, we obviously don't want to bother the user by asking "do you really 
want to scrub your nand" in the update process.

Cheers
> 
> -Scott

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Scott Wood]

On 09/12/2011 01:24 PM, Marek Vasut wrote:
> On Monday, September 12, 2011 08:06:27 PM Scott Wood wrote:
>> On 09/12/2011 12:45 PM, Marek Vasut wrote:
>>> On Monday, September 12, 2011 06:45:43 PM Mike Frysinger wrote:
>>>> On Monday, September 12, 2011 00:04:10 Marek Vasut wrote:
>>>>> This allows the scrub command to scrub without asking the user if he
>>>>> really wants to scrub the area. Useful in scripts.
>>>>
>>>> "quiet" and "skip user input" are two different things.  can you use a
>>>> more clean option like accepting "-y" to the "scrub" subcommand ?
>>>
>>> I'd prefer to have this hidden from common users as much as possible.
>>
>> What's the use case for needing to script this, BTW?
> 
> Update a block in NAND that's not handled by BCH accelerator in the MX28 chip.
> 
> The problem is, block 0 has it's own ECC done by bootrom software. That kind of 
> ECC is incompatible with BCH-produced ECC. That's also a reason for needing that 
> write.raw command.
> 
> Now, if you try erasing that block, the BCH reads and writes some of it's 
> metadata there. Obviously, since there is different kind of ECC, the metadata 
> aren't there and it chokes, claiming the block is bad and refuses to erase it.
> 
> And before you ask why -- that's because the BCH accelerator puts the metadata 
> at random places in the block (every 512 bytes, it puts a few bytes of it's ECC) 
> instead of putting them only to the ECC area. On the other hand, the bootrom ECC 
> puts the whole ECC at offset (1024 + 12) bytes from the start of the block 0.

Would it make sense to have the driver code treat block 0 specially
(possibly conditioned on an hwconfig or compile-time config), rather
than have it be user-driven?

I'm curious why anything is written on an erase, though, regardless of
data format.

-Scott

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Marek Vasut]

On Monday, September 12, 2011 08:31:12 PM Scott Wood wrote:
> On 09/12/2011 01:24 PM, Marek Vasut wrote:
> > On Monday, September 12, 2011 08:06:27 PM Scott Wood wrote:
> >> On 09/12/2011 12:45 PM, Marek Vasut wrote:
> >>> On Monday, September 12, 2011 06:45:43 PM Mike Frysinger wrote:
> >>>> On Monday, September 12, 2011 00:04:10 Marek Vasut wrote:
> >>>>> This allows the scrub command to scrub without asking the user if he
> >>>>> really wants to scrub the area. Useful in scripts.
> >>>> 
> >>>> "quiet" and "skip user input" are two different things.  can you use a
> >>>> more clean option like accepting "-y" to the "scrub" subcommand ?
> >>> 
> >>> I'd prefer to have this hidden from common users as much as possible.
> >> 
> >> What's the use case for needing to script this, BTW?
> > 
> > Update a block in NAND that's not handled by BCH accelerator in the MX28
> > chip.
> > 
> > The problem is, block 0 has it's own ECC done by bootrom software. That
> > kind of ECC is incompatible with BCH-produced ECC. That's also a reason
> > for needing that write.raw command.
> > 
> > Now, if you try erasing that block, the BCH reads and writes some of it's
> > metadata there. Obviously, since there is different kind of ECC, the
> > metadata aren't there and it chokes, claiming the block is bad and
> > refuses to erase it.
> > 
> > And before you ask why -- that's because the BCH accelerator puts the
> > metadata at random places in the block (every 512 bytes, it puts a few
> > bytes of it's ECC) instead of putting them only to the ECC area. On the
> > other hand, the bootrom ECC puts the whole ECC at offset (1024 + 12)
> > bytes from the start of the block 0.
> 
> Would it make sense to have the driver code treat block 0 specially
> (possibly conditioned on an hwconfig or compile-time config), rather
> than have it be user-driven?

No! What if (very possible situation actually) the user wants to use the whole 
NAND because the user is booting from SD/SPI/... ?

> 
> I'm curious why anything is written on an erase, though, regardless of
> data format.

Badblock markers (some FSL invention) are written always.

> 
> -Scott

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Wolfgang Denk]

Dear Marek Vasut,

In message <201109121945.17407.marek.vasut@gmail.com> you wrote:
> On Monday, September 12, 2011 06:45:43 PM Mike Frysinger wrote:
> > On Monday, September 12, 2011 00:04:10 Marek Vasut wrote:
> > > This allows the scrub command to scrub without asking the user if he
> > > really wants to scrub the area. Useful in scripts.
> > 
> > "quiet" and "skip user input" are two different things.  can you use a more
> > clean option like accepting "-y" to the "scrub" subcommand ?
> 
> I'd prefer to have this hidden from common users as much as possible.

This is probably well-intentioned, but keep in mind old (and good!)
Unix rules like:

"UNIX was not designed to stop you from doing stupid things,  because
that would also stop you from doing clever things."       - Doug Gwyn

Don't try hiding stuff - others might find it useful.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I can't say I've ever been lost, but I was bewildered once for  three
days.                                     - Daniel Boone (Attributed)

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Marek Vasut]

On Monday, September 12, 2011 08:37:47 PM Wolfgang Denk wrote:
> Dear Marek Vasut,
> 
> In message <201109121945.17407.marek.vasut@gmail.com> you wrote:
> > On Monday, September 12, 2011 06:45:43 PM Mike Frysinger wrote:
> > > On Monday, September 12, 2011 00:04:10 Marek Vasut wrote:
> > > > This allows the scrub command to scrub without asking the user if he
> > > > really wants to scrub the area. Useful in scripts.
> > > 
> > > "quiet" and "skip user input" are two different things.  can you use a
> > > more clean option like accepting "-y" to the "scrub" subcommand ?
> > 
> > I'd prefer to have this hidden from common users as much as possible.
> 
> This is probably well-intentioned, but keep in mind old (and good!)
> Unix rules like:
> 
> "UNIX was not designed to stop you from doing stupid things,  because
> that would also stop you from doing clever things."       - Doug Gwyn
> 
> Don't try hiding stuff - others might find it useful.

You can use the usual scrub command, noone is preventing you from anything.

Using this kind of a scrub.quiet command is really an arguable practice.

> 
> Best regards,
> 
> Wolfgang Denk

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Scott Wood]

On 09/12/2011 01:36 PM, Marek Vasut wrote:
> On Monday, September 12, 2011 08:31:12 PM Scott Wood wrote:
>> Would it make sense to have the driver code treat block 0 specially
>> (possibly conditioned on an hwconfig or compile-time config), rather
>> than have it be user-driven?
> 
> No! What if (very possible situation actually) the user wants to use the whole 
> NAND because the user is booting from SD/SPI/... ?

Well, that's why I suggested it be configurable.

>> I'm curious why anything is written on an erase, though, regardless of
>> data format.
> 
> Badblock markers (some FSL invention) are written always.

What's it doing with them?  Migrating them is something that should only
happen on the first use, as there will later be data in the factory bad
block area, right?  So it shouldn't be "always".  Migration should be an
explicitly requested option.  Like scrub. :-)

Where is the code that does this?  Which driver?

-Scott

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Marek Vasut]

On Monday, September 12, 2011 09:19:06 PM Scott Wood wrote:
> On 09/12/2011 01:36 PM, Marek Vasut wrote:
> > On Monday, September 12, 2011 08:31:12 PM Scott Wood wrote:
> >> Would it make sense to have the driver code treat block 0 specially
> >> (possibly conditioned on an hwconfig or compile-time config), rather
> >> than have it be user-driven?
> > 
> > No! What if (very possible situation actually) the user wants to use the
> > whole NAND because the user is booting from SD/SPI/... ?
> 
> Well, that's why I suggested it be configurable.
> 
> >> I'm curious why anything is written on an erase, though, regardless of
> >> data format.
> > 
> > Badblock markers (some FSL invention) are written always.
> 
> What's it doing with them?  Migrating them is something that should only
> happen on the first use, as there will later be data in the factory bad
> block area, right?  So it shouldn't be "always".  Migration should be an
> explicitly requested option.  Like scrub. :-)
> 
> Where is the code that does this?  Which driver?

The BCH accelerator does this.

> 
> -Scott

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Scott Wood]

On 09/12/2011 02:28 PM, Marek Vasut wrote:
> On Monday, September 12, 2011 09:19:06 PM Scott Wood wrote:
>> What's it doing with them?  Migrating them is something that should only
>> happen on the first use, as there will later be data in the factory bad
>> block area, right?  So it shouldn't be "always".  Migration should be an
>> explicitly requested option.  Like scrub. :-)
>>
>> Where is the code that does this?  Which driver?
> 
> The BCH accelerator does this.

In hardware?  What chip is this, and where is the code that drives this
chip?

-Scott

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Marek Vasut]

On Monday, September 12, 2011 09:36:49 PM Scott Wood wrote:
> On 09/12/2011 02:28 PM, Marek Vasut wrote:
> > On Monday, September 12, 2011 09:19:06 PM Scott Wood wrote:
> >> What's it doing with them?  Migrating them is something that should only
> >> happen on the first use, as there will later be data in the factory bad
> >> block area, right?  So it shouldn't be "always".  Migration should be an
> >> explicitly requested option.  Like scrub. :-)
> >> 
> >> Where is the code that does this?  Which driver?
> > 
> > The BCH accelerator does this.
> 
> In hardware?  What chip is this, and where is the code that drives this
> chip?

I think it does something to it, yes. i.MX287, see the patchset [PATCH 00/15 V2] 
Support for the DENX M28 SoM, [PATCH 09/15] iMX28: Add GPMI NAND driver .

> 
> -Scott

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Mike Frysinger]

On Monday, September 12, 2011 13:45:17 Marek Vasut wrote:
> On Monday, September 12, 2011 06:45:43 PM Mike Frysinger wrote:
> > On Monday, September 12, 2011 00:04:10 Marek Vasut wrote:
> > > This allows the scrub command to scrub without asking the user if he
> > > really wants to scrub the area. Useful in scripts.
> > 
> > "quiet" and "skip user input" are two different things.  can you use a
> > more clean option like accepting "-y" to the "scrub" subcommand ?
> 
> I'd prefer to have this hidden from common users as much as possible.

ok, but i dont follow why that makes a difference to my feedback.  "quiet" 
does not mean "do not prompt the user".  however, "scrub -y" does mean "assume 
yes to the sanity prompts".
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110912/2633d85f/attachment.pgp 

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Marek Vasut]

On Monday, September 12, 2011 10:33:16 PM Mike Frysinger wrote:
> On Monday, September 12, 2011 13:45:17 Marek Vasut wrote:
> > On Monday, September 12, 2011 06:45:43 PM Mike Frysinger wrote:
> > > On Monday, September 12, 2011 00:04:10 Marek Vasut wrote:
> > > > This allows the scrub command to scrub without asking the user if he
> > > > really wants to scrub the area. Useful in scripts.
> > > 
> > > "quiet" and "skip user input" are two different things.  can you use a
> > > more clean option like accepting "-y" to the "scrub" subcommand ?
> > 
> > I'd prefer to have this hidden from common users as much as possible.
> 
> ok, but i dont follow why that makes a difference to my feedback.  "quiet"
> does not mean "do not prompt the user".  however, "scrub -y" does mean
> "assume yes to the sanity prompts".
> -mike

Ok, you convinced me ... I added a -y option.

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Scott Wood]

On 09/12/2011 02:42 PM, Marek Vasut wrote:
> On Monday, September 12, 2011 09:36:49 PM Scott Wood wrote:
>> On 09/12/2011 02:28 PM, Marek Vasut wrote:
>>> On Monday, September 12, 2011 09:19:06 PM Scott Wood wrote:
>>>> What's it doing with them?  Migrating them is something that should only
>>>> happen on the first use, as there will later be data in the factory bad
>>>> block area, right?  So it shouldn't be "always".  Migration should be an
>>>> explicitly requested option.  Like scrub. :-)
>>>>
>>>> Where is the code that does this?  Which driver?
>>>
>>> The BCH accelerator does this.
>>
>> In hardware?  What chip is this, and where is the code that drives this
>> chip?
> 
> I think it does something to it, yes. i.MX287, see the patchset [PATCH 00/15 V2] 
> Support for the DENX M28 SoM, [PATCH 09/15] iMX28: Add GPMI NAND driver .

I looked at the code and the datasheet, and without getting into it too
deeply, I don't see how BCH is involved in an erase operation.  What
specifically are you seeing happen?

The "Raw NAND Boot Mode" section (12.12 in my copy of the i.MX28 manual)
says that it uses BCH for ECC -- is this not the case?  Is it some
special configuration of BCH?

I tried reading the "Bad Block Handling in the ROM" section and got a
headache.

I work for the PowerPC side of Freescale, in case you're wondering why
I'm unfamiliar with this. :-)

-Scott

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Marek Vasut]

On Tuesday, September 13, 2011 01:24:22 AM Scott Wood wrote:
> On 09/12/2011 02:42 PM, Marek Vasut wrote:
> > On Monday, September 12, 2011 09:36:49 PM Scott Wood wrote:
> >> On 09/12/2011 02:28 PM, Marek Vasut wrote:
> >>> On Monday, September 12, 2011 09:19:06 PM Scott Wood wrote:
> >>>> What's it doing with them?  Migrating them is something that should
> >>>> only happen on the first use, as there will later be data in the
> >>>> factory bad block area, right?  So it shouldn't be "always". 
> >>>> Migration should be an explicitly requested option.  Like scrub. :-)
> >>>> 
> >>>> Where is the code that does this?  Which driver?
> >>> 
> >>> The BCH accelerator does this.
> >> 
> >> In hardware?  What chip is this, and where is the code that drives this
> >> chip?
> > 
> > I think it does something to it, yes. i.MX287, see the patchset [PATCH
> > 00/15 V2] Support for the DENX M28 SoM, [PATCH 09/15] iMX28: Add GPMI
> > NAND driver .
> 
> I looked at the code and the datasheet, and without getting into it too
> deeply, I don't see how BCH is involved in an erase operation.  What
> specifically are you seeing happen?
> 
> The "Raw NAND Boot Mode" section (12.12 in my copy of the i.MX28 manual)
> says that it uses BCH for ECC -- is this not the case?  Is it some
> special configuration of BCH?
> 
> I tried reading the "Bad Block Handling in the ROM" section and got a
> headache.

12.12.1.8 is exactly it. See fig. 12-11. Also section 12.12.1.9 is interesting 
(at least for the block 0 problem).

For the other problem, you should read chapter 15 and 16. Though they are not 
completely beefed with information on that matter either.

I got most of these information from the source code FSL provided in their MX28 
BSP. The usual flow is the driver uses DMA to send the commands/data to the GPMI 
NAND driver. The GPMI NAND controller diverts the data to the BCH, which does 
the bit obscurisation. Then the data are taken from the BCH and written to NAND.

And the other way in the read process ... GPMI NAND controller reads data back 
from NAND, pushes them through the BCH and then presents them to the user.

The problem with erase here is that the block's ECC is updated on erase. But we 
want to write a block in our own format, so we need to scrub (wipe the block 
completely).

> 
> I work for the PowerPC side of Freescale, in case you're wondering why
> I'm unfamiliar with this. :-)

Interesting ... does everyone work for the PowerPC side of Freescale or is there 
some other reason why I never met anyone working for the ARM side of Freescale ? 
;-)
> 
> -Scott

Cheers

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Wolfgang Denk]

Dear Marek Vasut,

In message <201109130302.16991.marek.vasut@gmail.com> you wrote:
>
> Interesting ... does everyone work for the PowerPC side of Freescale or is there 
> some other reason why I never met anyone working for the ARM side of Freescale ? 
> ;-)

Keep in mind that the i.MX28 is actually not a Freescale design...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The IQ of the group is the lowest IQ of a member of the group divided
by the number of people in the group.

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Marek Vasut]

On Tuesday, September 13, 2011 06:25:42 AM Wolfgang Denk wrote:
> Dear Marek Vasut,
> 
> In message <201109130302.16991.marek.vasut@gmail.com> you wrote:
> > Interesting ... does everyone work for the PowerPC side of Freescale or
> > is there some other reason why I never met anyone working for the ARM
> > side of Freescale ? ;-)
> 
> Keep in mind that the i.MX28 is actually not a Freescale design...

I was just curious here.

Cheers

> 
> Best regards,
> 
> Wolfgang Denk

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Scott Wood]

On 09/12/2011 08:02 PM, Marek Vasut wrote:
> On Tuesday, September 13, 2011 01:24:22 AM Scott Wood wrote:
>> On 09/12/2011 02:42 PM, Marek Vasut wrote:
>>> On Monday, September 12, 2011 09:36:49 PM Scott Wood wrote:
>>>> On 09/12/2011 02:28 PM, Marek Vasut wrote:
>>>>> On Monday, September 12, 2011 09:19:06 PM Scott Wood wrote:
>>>>>> What's it doing with them?  Migrating them is something that should
>>>>>> only happen on the first use, as there will later be data in the
>>>>>> factory bad block area, right?  So it shouldn't be "always". 
>>>>>> Migration should be an explicitly requested option.  Like scrub. :-)
>>>>>>
>>>>>> Where is the code that does this?  Which driver?
>>>>>
>>>>> The BCH accelerator does this.
>>>>
>>>> In hardware?  What chip is this, and where is the code that drives this
>>>> chip?
>>>
>>> I think it does something to it, yes. i.MX287, see the patchset [PATCH
>>> 00/15 V2] Support for the DENX M28 SoM, [PATCH 09/15] iMX28: Add GPMI
>>> NAND driver .
[snip]
> The problem with erase here is that the block's ECC is updated on erase.

I don't understand this -- wouldn't that stop you from being able to
later write a different ECC (when you write the actual data)?  Is the
controller snooping the command bytes you issue for erase, and then
generating a program sequence?  How would the hardware even know that
you told U-Boot to scrub?

>>> I looked at the code and the datasheet, and without getting into it too
>>> deeply, I don't see how BCH is involved in an erase operation.  What
>>> specifically are you seeing happen?
>>>
>>> The "Raw NAND Boot Mode" section (12.12 in my copy of the i.MX28 manual)
>>> says that it uses BCH for ECC -- is this not the case?  Is it some
>>> special configuration of BCH?
>>>
>>> I tried reading the "Bad Block Handling in the ROM" section and got a
>>> headache.
>> 
>> 12.12.1.8 is exactly it. See fig. 12-11.

I think we have different versions, that's "Layout of Boot Page
containing FCB" in my copy, which shows a fixed spare/data/ECC layout
that may not match what is programmed for normal BCH operations, but it
doesn't say anything about bad block markers.

The bad block marker stuff is in 12.12.1.12 (figure 12-13) in my copy.
It says:

> In order to preserve the BI (bad block information), flash updater or gang programmer
> applications need to swap Bad Block Information (BI) data to byte 0 of metadata area for
> every page before programming NAND Flash.

Before first programming the flash, you'll need to make sure that all
blocks that are marked bad in the normal way get marked bad in the new
layout, but you don't need to *swap* anything.  The block's bad, the
rest of the block doesn't matter.  But anyway...

> ROM when loading firmware, copies back
> the value at metadata[0] to BI offset in page data. The following figure shows how the
> factory bad block marker is preserved.

...this is insane.  It seems that they want you to swap this byte even
in good blocks, so that you put the byte of real data that should go
somewhere in the middle of the last 512-byte ECC chunk at offset zero in
the page.  This means that it will show up as a bad block when normal
(but new-layout-using) software looks at it, which is why you need
scrub.  Ew.

How many blocks are being loaded by this mechanism?  Just block zero
(which is normally supposed to be guaranteed good anyway...)?  Or
multiple blocks?

Any chance you could blow the NAND_MEMBLOCK_MARKER_RESERVE fuse? :-)
Otherwise, I guess you do need to scrub.  Have you complained to
Freescale sales/support?

> But we want to write a block in our own format, so we need to scrub (wipe the block 
> completely).

Erase always wipes the block completely, if it erases at all.  Scrub in
this context just means that U-Boot ignores the bad block indications
(marker or table).  Otherwise it would avoid erasing bad blocks, so that
they stay bad, and you won't have scrubbed the entire region requested.

>> I work for the PowerPC side of Freescale, in case you're wondering why
>> I'm unfamiliar with this. :-)
> 
> Interesting ... does everyone work for the PowerPC side of Freescale or is there 
> some other reason why I never met anyone working for the ARM side of Freescale ? 
> ;-)

They exist, but don't seem to engage Open Source development communities
to the same degree.

-Scott

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Marek Vasut]

[...]

> > ROM when loading firmware, copies back
> > the value at metadata[0] to BI offset in page data. The following figure
> > shows how the factory bad block marker is preserved.
> 
> ...this is insane.  It seems that they want you to swap this byte even
> in good blocks, so that you put the byte of real data that should go
> somewhere in the middle of the last 512-byte ECC chunk at offset zero in
> the page.  This means that it will show up as a bad block when normal
> (but new-layout-using) software looks at it, which is why you need
> scrub.  Ew.
> 
> How many blocks are being loaded by this mechanism?  Just block zero
> (which is normally supposed to be guaranteed good anyway...)?  Or
> multiple blocks?

The first block, then the 64th block, 128th block and 192th block (in default 
layout).
> 
> Any chance you could blow the NAND_MEMBLOCK_MARKER_RESERVE fuse? :-)

No, they are one-time programable. Delivering a "damaged" chip isn't a good 
practice.

> Otherwise, I guess you do need to scrub.  Have you complained to
> Freescale sales/support?

In fact no. The BootROM is "broken" and I doubt they will be willing to do 
anything about it.

> 
> > But we want to write a block in our own format, so we need to scrub (wipe
> > the block completely).
> 
> Erase always wipes the block completely, if it erases at all.  Scrub in
> this context just means that U-Boot ignores the bad block indications
> (marker or table).  Otherwise it would avoid erasing bad blocks, so that
> they stay bad, and you won't have scrubbed the entire region requested.
> 
> >> I work for the PowerPC side of Freescale, in case you're wondering why
> >> I'm unfamiliar with this. :-)
> > 
> > Interesting ... does everyone work for the PowerPC side of Freescale or
> > is there some other reason why I never met anyone working for the ARM
> > side of Freescale ? ;-)
> 
> They exist, but don't seem to engage Open Source development communities
> to the same degree.
> 
> -Scott

Cheers

[U-Boot] [PATCH 5/5] NAND: Add scrub.quiet command option

[Scott Wood]

On 09/13/2011 05:41 PM, Marek Vasut wrote:
>> Any chance you could blow the NAND_MEMBLOCK_MARKER_RESERVE fuse? :-)
> 
> No, they are one-time programable. Delivering a "damaged" chip isn't a good 
> practice.

I'd call it "fixed" rather than "damaged" -- they did throw in that fuse
in case of "any defective ROM code in handling bad block marker byte
swapping", and the entire concept is defective.

But I agree that if you're delivering them to other developers, rather
than as part of a final product, they might want to make that choice
themselves.  And who knows, blowing the fuse might expose other defects.

>> Otherwise, I guess you do need to scrub.  Have you complained to
>> Freescale sales/support?
> 
> In fact no. The BootROM is "broken" and I doubt they will be willing to do 
> anything about it.

Probably not, but they should still hear about it, if only for the
chance they'll do it right next time.

-Scott