NAND read/write.jffs2 fix

extern nand_info_t nand_info[]; /* info for NAND chips */
+extern struct nand_chip nand_chip[]; /* extra info for NAND chips */

common/cmd_nand.c +0 -8

 U_BOOT_CMD(nand, 5, 1, do_nand,
-	"nand    - NAND sub-system\n",
-	"info                  - show available NAND devices\n"
-	"nand device [dev]     - show or set current device\n"
-	"nand read[.jffs2]     - addr off|partition size\n"
-	"nand write[.jffs2]    - addr off|partition size - read/write `size' bytes starting\n"
-	"    at offset `off' to/from memory address `addr'\n"
-	"nand erase [clean] [off size] - erase `size' bytes from\n"
-	"    offset `off' (entire device if not specified)\n"

drivers/mtd/nand/nand_util.c +0 -0

index 1916aca..dd4ed6b 100644

+ * @return image length including bad blocks
+ */
+static ulong get_len_incl_bad (nand_info_t *nand, ulong offset, ulong *len)

+/**
+ * nand_write_skip_bad:
+ *
+ * Write image to NAND flash.
+ * Blocks that are marked bad are skipped and the is written to the next
+ * block instead as long as the image is short enough to fit even after
+ * skipping the bad blocks.
+ *
+ * @param nand NAND device
+ * @param offset offset in flash
+ * @param len buffer lenght

+ * @param buf buffer to read from
+ * @return 0 in case of success
+ */

drivers/mtd/nand/nand_util.c +15 -0

+int nand_write_skip_bad(nand_info_t *nand, ulong offset, ulong *len, u_char *buf)
+{
+	int     result;
+	u_char *p_buffer = buf;
+	ulong   left_to_write = *len;
+	ulong   len_incl_bad;
+
+	DEBUGP ("offset %08x, len  0x%05x\n", offset, *len);
+
+	/* Reject writes, which are not page aligned */
+	if (((offset & (nand->writesize - 1)) != 0) || 
+	    ((*len & (nand->writesize - 1)) != 0)) {
+		printf ("Attempt to write not page aligned data\n");
+		return -EINVAL;
+	}

drivers/mtd/nand/nand_util.c +13 -0

+	while (left_to_write > 0) {
+		ulong block_offset = offset & (nand->erasesize - 1);
+		ulong write_size;
+
+		DEBUGP ("left %05x, offset %08x, block_off %05x, p_buf %08x\n",
+			left_to_write, offset, block_offset, p_buffer);
+
+		if (nand_block_isbad (nand, offset)) {
+			printf ("Skip bad block 0x%08x\n",
+				offset & ~(nand->erasesize - 1));
+			offset += nand->erasesize - block_offset;
+			continue;
+		}

+/**
+ * nand_read_ff_upon_bad:
+ *
+ * Read image from NAND flash.

+ * Data from blocks that are marked bad is read as 0xff.
+ *
+ * @param nand NAND device
+ * @param offset offset in flash
+ * @param len buffer length
+ * @param buf buffer to write to
+ * @return 0 in case of success
+ */

drivers/mtd/nand/nand_util.c +10 -0

+	while (left_to_read > 0) {
+		ulong block_offset = offset & (nand->erasesize - 1);
+		ulong read_length;
+
+		DEBUGP ("left %05x, offset %08x, block_off %04x, p_buf %08x\n",
+			left_to_read, offset, block_offset, p_buffer);
+
+		if (left_to_read < (nand->erasesize - block_offset))
+			read_length = left_to_read;
+		else

Quoted 5 lines

static int nand_dump(nand_info_t *nand, ulong off, int only_oob) {

common/cmd_nand.c static int nand_dump(nand_info_t *nand, ulong off, int only_oob) · line 79

Quoted 9 lines

common/cmd_nand.c +3 -3

@@ -73,9 +74,9 @@ static int nand_dump(nand_info_t *nand, ulong off, int only_oob)
 	while (i--) {
 		if (!only_oob) {
 			printf( "\t%02x %02x %02x %02x %02x %02x %02x %02x"
-					"  %02x %02x %02x %02x %02x %02x %02x %02x\n",
-					p[0], p[1], p[2], p[3], p[4], p[5], p[6], p[7],
-					p[8], p[9], p[10], p[11], p[12], p[13], p[14], p[15]);
+				"  %02x %02x %02x %02x %02x %02x %02x %02x\n",
+				p[0], p[1], p[2], p[3], p[4], p[5], p[6], p[7],
+				p[8], p[9], p[10], p[11], p[12], p[13], p[14], p[15]);

Still exceeds 80 characters on the last line.

Quoted 14 lines · On Tue, 13 May 2008 15:41:06 -0500 Scott Wood <[email protected]> wrote:

common/cmd_nand.c static int nand_dump(nand_info_t *nand, ulong off, int only_oob) · line 79

Quoted 13 lines

static int nand_dump(nand_info_t *nand, ulong off, int only_oob) {

common/cmd_nand.c +3 -3

@@ -73,9 +74,9 @@ static int nand_dump(nand_info_t *nand, ulong off, int only_oob)
 	while (i--) {
 		if (!only_oob) {
 			printf( "\t%02x %02x %02x %02x %02x %02x %02x %02x"
-					"  %02x %02x %02x %02x %02x %02x %02x %02x\n",
-					p[0], p[1], p[2], p[3], p[4], p[5], p[6], p[7],
-					p[8], p[9], p[10], p[11], p[12], p[13], p[14], p[15]);
+				"  %02x %02x %02x %02x %02x %02x %02x %02x\n",
+				p[0], p[1], p[2], p[3], p[4], p[5], p[6], p[7],
+				p[8], p[9], p[10], p[11], p[12], p[13], p[14], p[15]);

Still exceeds 80 characters on the last line.

or use lib_generic/display_options.c:print_buffer()

Quoted 2 lines

extern nand_info_t nand_info[]; /* info for NAND chips */
+extern struct nand_chip nand_chip[]; /* extra info for NAND chips */

Where is this defined or used?

Quoted 4 lines

common/cmd_nand.c +0 -0

@@ -327,40 +327,19 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int 
 argc, char *argv[])
             goto usage;
 

common/cmd_nand.c int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) · line 333

Quoted 6 lines

common/cmd_nand.c +0 -1

         addr = (ulong)simple_strtoul(argv[2], NULL, 16);
-
         read = strncmp(cmd, "read", 4) == 0; /* 1 = read, 0 = write */
         printf("\nNAND %s: ", read ? "read" : "write");
         if (arg_off_size(argc - 3, argv + 3, nand, &off, &size) != 0)
             return 1;

What's wrong with that newline?

common/cmd_nand.c int do_nand(cmd_tbl_t * cmdtp, int flag, int · line 356

Quoted 7 lines

common/cmd_nand.c +1 -0

@@ -372,9 +351,10 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int 
 argc, char *argv[])
                 ret = nand->read_oob(nand, off, &ops);
             else
                 ret = nand->write_oob(nand, off, &ops);
+
         } else {

Why add a blank line here?

Quoted 17 lines

common/cmd_nand.c +0 -5

 U_BOOT_CMD(nand, 5, 1, do_nand,
-    "nand    - NAND sub-system\n",
-    "info                  - show available NAND devices\n"
-    "nand device [dev]     - show or set current device\n"
-    "nand read[.jffs2]     - addr off|partition size\n"
-    "nand write[.jffs2]    - addr off|partition size - read/write 

`size' bytes starting\n"

common/cmd_nand.c +0 -9

-    "    at offset `off' to/from memory address `addr'\n"
-    "nand erase [clean] [off size] - erase `size' bytes from\n"
-    "    offset `off' (entire device if not specified)\n"
-    "nand bad - show bad blocks\n"
-    "nand dump[.oob] off - dump page\n"
-    "nand scrub - really clean NAND erasing bad blocks (UNSAFE)\n"
-    "nand markbad off - mark bad block at offset (UNSAFE)\n"
-    "nand biterr off - make a bit error at offset (UNSAFE)\n"
-    "nand lock [tight] [status] - bring nand to lock state or display 

locked pages\n"

common/cmd_nand.c usage: · line 476

Quoted 4 lines

common/cmd_nand.c +3 -1

-    "nand unlock [offset] [size] - unlock section\n");
+       "nand    - NAND sub-system\n",
+       "info              - show available NAND devices\n"
+       "nand device [dev]      - show or set current device\n"

Should either keep these lined up or consistently have only one character before the dash.

common/cmd_nand.c usage: · line 478

Quoted 2 lines

common/cmd_nand.c +2 -0

+       "nand read[.jffs2, .i]  addr off|partition size\n"
+       "nand write[.jffs2, .i] addr off|partition size\n"

What about .e? Is it just for backwards compatibility that we have three commands that mean the same thing? Do we want to document all three?

Quoted 2 lines

+ " - read/write size' bytes starting at offset off' to/from
memory address `addr'\n"

Maybe mention the effect of .jffs2/.i/.e here in addition to the offline documentation.

Quoted 8 lines

drivers/mtd/nand/nand_util.c +0 -0

index 1916aca..dd4ed6b 100644
@@ -52,6 +52,14 @@ typedef struct mtd_info      mtd_info_t;
 #define cpu_to_je16(x) (x)
 #define cpu_to_je32(x) (x)
 

drivers/mtd/nand/nand_util.c typedef struct mtd_info mtd_info_t; · line 61

Quoted 9 lines

drivers/mtd/nand/nand_util.c +7 -0

+#if 0
+#define MARK printf("Was here: %s, %s, %d\n",__FILE__, __FUNCTION__, 
 __LINE__);
+#define DEBUGP(fmt,args...) printf("%s, %s, %d: "fmt,__FILE__, 
 __FUNCTION__, __LINE__,##args)
+#else
+#define MARK
+#define DEBUGP(fmt,args...)
+#endif

We should probably use the existing MTD DEBUG() facility.

Quoted 4 lines

drivers/mtd/nand/nand_util.c +3 -0

+ * @return        image length including bad blocks
+ */
+static ulong get_len_incl_bad (nand_info_t *nand, ulong offset, ulong 

*len)

Why is len a pointer? It doesn't seem to be modified.

drivers/mtd/nand/nand_util.c int nand_unlock(nand_info_t *meminfo, ulong start, ulong length) · line 457

Quoted 9 lines

drivers/mtd/nand/nand_util.c +9 -0

+{
+    ulong   len_incl_bad;
+    ulong   len_excl_bad;
+    ulong   block_len;
+
+    DEBUGP ("*len %05x, nand->erasesize %05x\n", *len, nand->erasesize);
+
+    for (len_excl_bad = 0, len_incl_bad = 0;  len_excl_bad < *len;  )
+    {

Brace on preceding line. Might be clearer as a while statement.

drivers/mtd/nand/nand_util.c int nand_unlock(nand_info_t *meminfo, ulong start, ulong length) · line 461

Quoted 4 lines

drivers/mtd/nand/nand_util.c +4 -0

+        block_len = nand->erasesize - (offset & (nand->erasesize - 1));
+
+        if (!nand_block_isbad (nand, offset))
+            len_excl_bad += block_len;

Hmm, is it safe to call nand_block_isbad with an offset that isn't the start of the block? The BBT implementation seems OK with it, but what about block_bad callbacks?

Quoted 18 lines

+ * @param buf buffer to read from
+ * @return 0 in case of success
+ */
+int nand_write_skip_bad(nand_info_t *nand, ulong offset, ulong *len, u_char *buf)

drivers/mtd/nand/nand_util.c +10 -0

+{
+    int     result;
+    u_char *p_buffer = buf;
+    ulong   left_to_write = *len;
+    ulong   len_incl_bad;
+
+    DEBUGP ("offset %08x, len  0x%05x\n", offset, *len);
+
+    /* Reject writes, which are not page aligned */
+    if (((offset & (nand->writesize - 1)) != 0) || +        ((*len & 

(nand->writesize - 1)) != 0)) {

drivers/mtd/nand/nand_util.c +2 -0

+        printf ("Attempt to write not page aligned data\n");
+        return -EINVAL;

drivers/mtd/nand/nand_util.c int nand_unlock(nand_info_t *meminfo, ulong start, ulong length) · line 513

Quoted 10 lines

drivers/mtd/nand/nand_util.c +10 -0

+    }
+
+    len_incl_bad = get_len_incl_bad (nand, offset, len); +
+    if ((offset + len_incl_bad) > nand->size) {
+        printf ("Attempt to write outside the flash area\n");
+        return -EINVAL;
+    }
+   
+    if (len_incl_bad == *len)
+        return (nand_write (nand, offset, len, buf));

Unnecessary parentheses.

Here, len will be modified to show how much was written on an error, but in the loop below, it won't. In the loop below, we'll print an error if the write fails, but here we won't.

Quoted 20 lines

drivers/mtd/nand/nand_util.c +19 -0

+    while (left_to_write > 0) {
+        ulong block_offset = offset & (nand->erasesize - 1);
+        ulong write_size;
+
+        DEBUGP ("left %05x, offset %08x, block_off %05x, p_buf %08x\n",
+            left_to_write, offset, block_offset, p_buffer);
+
+        if (nand_block_isbad (nand, offset)) {
+            printf ("Skip bad block 0x%08x\n",
+                offset & ~(nand->erasesize - 1));
+            offset += nand->erasesize - block_offset;
+            continue;
+        }
+
+        if (left_to_write < (nand->erasesize - block_offset))
+            write_size = left_to_write;
+        else
+            write_size = nand->erasesize - block_offset;
+

drivers/mtd/nand/nand_util.c int nand_unlock(nand_info_t *meminfo, ulong start, ulong length) · line 537

Quoted 4 lines

drivers/mtd/nand/nand_util.c +4 -0

+        result = nand_write (nand, offset, &write_size, p_buffer);
+        if (result != 0) {
+            printf ("Write failed %d\n", result);
+            return result;

We should probably be more detailed about exactly what failed. It's obvious (except for the exact block that failed) if the user typed in the command directly, but not if it's from a script.

drivers/mtd/nand/nand_util.c int nand_unlock(nand_info_t *meminfo, ulong start, ulong length) · line 541

Quoted 4 lines

drivers/mtd/nand/nand_util.c +4 -0

+        }       
+        left_to_write -= write_size;
+        offset        += write_size;
+        p_buffer = (u_char *)((ulong)p_buffer + write_size);

Why not just p_buffer += write_size?

Quoted 11 lines

+/**
+ * nand_read_ff_upon_bad: + *
+ * Read image from NAND flash. + * Data from blocks that are marked bad is read as 0xff.
+ *
+ * @param nand NAND device
+ * @param offset offset in flash
+ * @param len buffer length
+ * @param buf buffer to write to
+ * @return 0 in case of success
+ */

What was the reason for changing the behavior of non-jffs2 reads, BTW?

Quoted 9 lines

In the README.nand:
"
nand read.jffs2 addr ofs|partition size
Like `read', but the data for blocks that are marked bad is read as 0xff. This gives a readable JFFS2 image that can be processed by the JFFS2 commands such as ls and fsload.
"
Is this correct or should read skip the bad blocks and read the next instead ?

No. This seems to be incorrect. IIRC the "normal" nand read read 0xff's upon bad blocks. Please fix this when you submit a new patch.

drivers/mtd/nand/nand_util.c int nand_unlock(nand_info_t *meminfo, ulong start, ulong length) · line 564

Quoted 6 lines

+int nand_read_ff_upon_bad (nand_info_t *nand, ulong offset, ulong *len, u_char *buf)

drivers/mtd/nand/nand_util.c +4 -0

+{
+    int     result;
+    u_char *p_buffer = buf;
+    ulong   left_to_read = *len;

The aligned variable declaration style can be annoying to maintain if a new variable with a longer-named type has to be added in the future...

drivers/mtd/nand/nand_util.c int nand_unlock(nand_info_t *meminfo, ulong start, ulong length) · line 647

Quoted 5 lines

drivers/mtd/nand/nand_util.c +5 -0

+                offset & ~(nand->erasesize - 1));
+            for (i = 0;  i < read_length;  i++)
+                p_buffer[i] = 0xff;
+
+        } else {

No blank line at the end of the block.

drivers/mtd/nand/nand_util.c

Quoted 2 lines

drivers/mtd/nand/nand_util.c +2 -0

+        }
+        left_to_read -= read_length;

A blank line after the end of a block would be nice, though.

Quoted 6 lines

common/cmd_nand.c usage: · line 478

Quoted 2 lines

common/cmd_nand.c +2 -0

+       "nand read[.jffs2, .i]  addr off|partition size\n"
+       "nand write[.jffs2, .i] addr off|partition size\n"

What about .e? Is it just for backwards compatibility that we have three commands that mean the same thing? Do we want to document all three?

The doc/README.nand only mention .jffs2 and .i
The option .jffs2 only has skipping bad blocks (and ECC) in common with the jffs2 file system.
If we want to be backwards compatible we should use [.jffs2|.i|.e] otherwise [.i|.e] or [.skip] would be better.
I leave this choice to you Scott.

Quoted 5 lines

Quoted 2 lines

+ " - read/write size' bytes starting at offset off' to/from
memory address `addr'\n"

Maybe mention the effect of .jffs2/.i/.e here in addition to the offline documentation.

Agree.
Something like this ?
"nand - NAND sub-system\n",
"info - show available NAND devices\n"
"nand device [dev] - show or set current device\n"
"nand read[.jffs2|.i|.e] addr off|partition size - read size' bytes\n" " starting at offset off' to memory address addr'. Using one of\n" " the .' options bad blocks is skipped otherwise read as 0xff\n" "nand write[.jffs2|.i|.e] addr off|partition size - write size'\n" " bytes starting at offset off' from memory address addr'. Using\n" " one of .' options bad blocks is skipped otherwise write fails\n" "nand erase [clean] [off size] - erase size' bytes from offset\n" " off' (entire device if not specified)\n"

"nand bad - show bad blocks\n"
"nand dump[.oob] off - dump page\n"
"nand scrub - really clean NAND erasing bad blocks (UNSAFE)\n" "nand markbad off - mark bad block at offset (UNSAFE)\n" "nand biterr off - make a bit error at offset (UNSAFE)\n" "nand lock [tight] [status] - bring nand to lock state or display\n" " locked pages\n"
"nand unlock [offset] [size] - unlock section\n");

Quoted 8 lines

drivers/mtd/nand/nand_util.c int nand_unlock(nand_info_t *meminfo, ulong start, ulong length) · line 461

Quoted 4 lines

drivers/mtd/nand/nand_util.c +4 -0

+        block_len = nand->erasesize - (offset & (nand->erasesize - 1));
+
+        if (!nand_block_isbad (nand, offset))
+            len_excl_bad += block_len;

Hmm, is it safe to call nand_block_isbad with an offset that isn't the start of the block? The BBT implementation seems OK with it, but what about block_bad callbacks?

Both nand_block_bad and nand_isbad_bbt seems ok.
But just in case it could be changed to "if (!nand_block_isbad (nand, offset & (~nand->erasesize + 1))"

I do not understand what you mean about block_bad callbacks.