[U-Boot] [PATCH] Nand: Implement raw read/write and biterr

[U-Boot] [PATCH] Nand: Implement raw read/write and biterr

[John Rigby]

New commands nand read.raw and write.raw read/write
main and oob area.

Implement previously stubbed nand biterr command.

Document the above and also the previously undocumented
read.oob and write.oob.

Signed-off-by: John Rigby <jrigby@control4.com>
---
 common/cmd_nand.c |  115 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 112 insertions(+), 3 deletions(-)

diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 158a55f..a488038 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -90,6 +90,95 @@ static int nand_dump(nand_info_t *nand, ulong off, int only_oob)
 	return 0;
 }
 
+#define NAND_RW_RAW_READ 0
+#define NAND_RW_RAW_WRITE 1
+
+static int nand_rdwr_raw(int rdwr, nand_info_t *nand, ulong off, u_char *buf,
+				size_t size)
+{
+	struct mtd_oob_ops ops = {
+		.len = nand->writesize,
+		.ooblen = nand->oobsize,
+		.mode = MTD_OOB_RAW,
+	};
+	int i;
+	int nrblocks = size / nand->writesize;
+	loff_t addr = (loff_t)(off & ~(nand->writesize - 1));
+
+	while (nrblocks--) {
+		ops.datbuf = buf;
+		/*
+		 * for read oobbuf must be set, but oob data
+		 * will be appended to ops.datbuf
+		 * for write oobbuf is actually used
+		 */
+		ops.oobbuf = buf + nand->writesize;
+		if (rdwr == NAND_RW_RAW_READ)
+			i = nand->read_oob(nand, addr, &ops);
+		else
+			i = nand->write_oob(nand, addr, &ops);
+		if (i < 0) {
+			printf("Error (%d) %s page %08lx\n", i,
+					rdwr == NAND_RW_RAW_READ ?
+						"reading" : "writing",
+					(unsigned long)addr);
+			return 1;
+		}
+
+		addr += nand->writesize;
+		buf += (nand->writesize + nand->oobsize);
+	}
+	return 0;
+}
+
+static int nand_read_raw(nand_info_t *nand, ulong off, u_char *buf,
+			 size_t size)
+{
+	return nand_rdwr_raw(NAND_RW_RAW_READ, nand, off, buf, size);
+}
+
+static int nand_write_raw(nand_info_t *nand, ulong off, u_char *buf,
+			  size_t size)
+{
+	return nand_rdwr_raw(NAND_RW_RAW_WRITE, nand, off, buf, size);
+}
+
+static int nand_biterr(nand_info_t *nand, ulong off, int bit)
+{
+	int ret = 0;
+	u_char *buf;
+	ulong blockoff = off & ~(nand->erasesize - 1);
+	int byteoff = off & (nand->erasesize - 1);
+	nand_erase_options_t opts = {
+		.offset = blockoff,
+		.length = nand->erasesize,
+	};
+
+	buf = malloc(nand->erasesize +
+			nand->oobsize * (nand->erasesize / nand->writesize));
+	if (!buf) {
+		puts("No memory for page buffer\n");
+		return 1;
+	}
+	nand_read_raw(nand, blockoff, buf, nand->erasesize);
+
+	ret = nand_erase_opts(nand, &opts);
+	if (ret) {
+		puts("Failed to erase block at %x\n");
+		return ret;
+	}
+
+	printf("toggling bit %x in byte %x in block %x %02x ->",
+		bit, byteoff, blockoff, buf[byteoff]);
+
+	buf[byteoff] ^= (1 << bit);
+
+	printf("%02x\n", buf[byteoff]);
+
+	nand_write_raw(nand, blockoff, buf, nand->erasesize);
+	free(buf);
+	return 0;
+}
 /* ------------------------------------------------------------------------- */
 
 static inline int str2long(char *p, ulong *num)
@@ -401,6 +490,13 @@ 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 if (!strcmp(s, ".raw")) {
+			if (read)
+				ret = nand_read_raw(nand, off,
+						    (u_char *)addr, size);
+			else
+				ret = nand_write_raw(nand, off,
+						     (u_char *)addr, size);
 		} else {
 			printf("Unknown nand command suffix '%s'.\n", s);
 			return 1;
@@ -437,9 +533,19 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 		}
 		return ret;
 	}
-
 	if (strcmp(cmd, "biterr") == 0) {
-		/* todo */
+		off = (ulong)simple_strtoul(argv[2], NULL, 16);
+		i = (int)simple_strtoul(argv[3], NULL, 16);
+
+		int ret = nand_biterr(nand, off, i);
+		if (ret == 0) {
+			printf("byte offset 0x%08lx toggled bit %d\n",
+			       (ulong) off, i);
+			return 0;
+		} else {
+			printf("byte offset 0x%08lx could not toggle bit %d\n",
+			       (ulong) addr, i);
+		}
 		return 1;
 	}
 
@@ -494,13 +600,16 @@ U_BOOT_CMD(nand, CONFIG_SYS_MAXARGS, 1, do_nand,
 	"nand write - addr off|partition size\n"
 	"    read/write 'size' bytes starting at offset 'off'\n"
 	"    to/from memory address 'addr', skipping bad blocks.\n"
+	"     .oob - reads/writes oob only.\n"
+	"     .raw - reads/writes both main and oob with no error\n"
+	"            detection or correction\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(s) at offset (UNSAFE)\n"
-	"nand biterr off - make a bit error at offset (UNSAFE)"
+	"nand biterr off bitno - toggle bit bitno in byte off (UNSAFE)\n"
 #ifdef CONFIG_CMD_NAND_LOCK_UNLOCK
 	"\n"
 	"nand lock [tight] [status]\n"
-- 
1.6.3

[U-Boot] [PATCH] Nand: Implement raw read/write and biterr

[Tom]

John Rigby wrote:
> New commands nand read.raw and write.raw read/write
> main and oob area.
> 
> Implement previously stubbed nand biterr command.
> 
> Document the above and also the previously undocumented
> read.oob and write.oob.
> 
> Signed-off-by: John Rigby <jrigby@control4.com>
> ---
>  common/cmd_nand.c |  115 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 112 insertions(+), 3 deletions(-)
> 
> diff --git a/common/cmd_nand.c b/common/cmd_nand.c
> index 158a55f..a488038 100644
> --- a/common/cmd_nand.c
> +++ b/common/cmd_nand.c
> @@ -90,6 +90,95 @@ static int nand_dump(nand_info_t *nand, ulong off, int only_oob)
>  	return 0;
>  }
>  
> +#define NAND_RW_RAW_READ 0
> +#define NAND_RW_RAW_WRITE 1
> +
> +static int nand_rdwr_raw(int rdwr, nand_info_t *nand, ulong off, u_char *buf,
> +				size_t size)
> +{
> +	struct mtd_oob_ops ops = {
> +		.len = nand->writesize,
> +		.ooblen = nand->oobsize,
> +		.mode = MTD_OOB_RAW,
> +	};
> +	int i;
> +	int nrblocks = size / nand->writesize;
> +	loff_t addr = (loff_t)(off & ~(nand->writesize - 1));

Silently dropping bytes.
Would it be better to require the size to be a multiple of the block size ?
Or at least warn of the dropped bytes.

> +
> +	while (nrblocks--) {
> +		ops.datbuf = buf;

<snip>

>  	}
>  
> @@ -494,13 +600,16 @@ U_BOOT_CMD(nand, CONFIG_SYS_MAXARGS, 1, do_nand,
>  	"nand write - addr off|partition size\n"
>  	"    read/write 'size' bytes starting at offset 'off'\n"
>  	"    to/from memory address 'addr', skipping bad blocks.\n"
> +	"     .oob - reads/writes oob only.\n"
> +	"     .raw - reads/writes both main and oob with no error\n"
> +	"            detection or correction\n"

Writing the oob area directly is UNSAFE.
Having done to myself, it was a pain to undo.
You should put at least document that.

Tom

[U-Boot] [PATCH] Nand: Implement raw read/write and biterr

[John Rigby]

Tom,

Thanks for the comments.  I'll wait a couple of days for others then
resubmit with fixes for these problems and any others that come up.

John

On Wed, Sep 30, 2009 at 1:22 PM, Tom <Tom.Rix@windriver.com> wrote:

> John Rigby wrote:
>
>> New commands nand read.raw and write.raw read/write
>> main and oob area.
>>
>> Implement previously stubbed nand biterr command.
>>
>> Document the above and also the previously undocumented
>> read.oob and write.oob.
>>
>> Signed-off-by: John Rigby <jrigby@control4.com>
>> ---
>>  common/cmd_nand.c |  115
>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 112 insertions(+), 3 deletions(-)
>>
>> diff --git a/common/cmd_nand.c b/common/cmd_nand.c
>> index 158a55f..a488038 100644
>> --- a/common/cmd_nand.c
>> +++ b/common/cmd_nand.c
>> @@ -90,6 +90,95 @@ static int nand_dump(nand_info_t *nand, ulong off, int
>> only_oob)
>>        return 0;
>>  }
>>  +#define NAND_RW_RAW_READ 0
>> +#define NAND_RW_RAW_WRITE 1
>> +
>> +static int nand_rdwr_raw(int rdwr, nand_info_t *nand, ulong off, u_char
>> *buf,
>> +                               size_t size)
>> +{
>> +       struct mtd_oob_ops ops = {
>> +               .len = nand->writesize,
>> +               .ooblen = nand->oobsize,
>> +               .mode = MTD_OOB_RAW,
>> +       };
>> +       int i;
>> +       int nrblocks = size / nand->writesize;
>> +       loff_t addr = (loff_t)(off & ~(nand->writesize - 1));
>>
>
> Silently dropping bytes.
> Would it be better to require the size to be a multiple of the block size ?
> Or at least warn of the dropped bytes.
>
>  +
>> +       while (nrblocks--) {
>> +               ops.datbuf = buf;
>>
>
> <snip>
>
>         }
>>  @@ -494,13 +600,16 @@ U_BOOT_CMD(nand, CONFIG_SYS_MAXARGS, 1, do_nand,
>>        "nand write - addr off|partition size\n"
>>        "    read/write 'size' bytes starting at offset 'off'\n"
>>        "    to/from memory address 'addr', skipping bad blocks.\n"
>> +       "     .oob - reads/writes oob only.\n"
>> +       "     .raw - reads/writes both main and oob with no error\n"
>> +       "            detection or correction\n"
>>
>
> Writing the oob area directly is UNSAFE.
> Having done to myself, it was a pain to undo.
> You should put at least document that.
>
> Tom
>

[U-Boot] [PATCH] Nand: Implement raw read/write and biterr

[Scott Wood]

On Wed, Sep 30, 2009 at 02:22:09PM -0500, Tom wrote:
> John Rigby wrote:
>>  @@ -494,13 +600,16 @@ U_BOOT_CMD(nand, CONFIG_SYS_MAXARGS, 1, do_nand,
>>  	"nand write - addr off|partition size\n"
>>  	"    read/write 'size' bytes starting at offset 'off'\n"
>>  	"    to/from memory address 'addr', skipping bad blocks.\n"
>> +	"     .oob - reads/writes oob only.\n"
>> +	"     .raw - reads/writes both main and oob with no error\n"
>> +	"            detection or correction\n"
>
> Writing the oob area directly is UNSAFE.
> Having done to myself, it was a pain to undo.
> You should put at least document that.

If we warn about oob being unsafe, the same warning should be made for
raw.

-Scott

[U-Boot] [PATCH] Nand: Implement raw read/write and biterr

[Scott Wood]

On Wed, Sep 30, 2009 at 12:11:39PM -0600, John Rigby wrote:
> +static int nand_rdwr_raw(int rdwr, nand_info_t *nand, ulong off, u_char *buf,
> +				size_t size)

Offset should be loff_t, here and elsewhere.

> +{
> +	struct mtd_oob_ops ops = {
> +		.len = nand->writesize,
> +		.ooblen = nand->oobsize,
> +		.mode = MTD_OOB_RAW,
> +	};
> +	int i;
> +	int nrblocks = size / nand->writesize;
> +	loff_t addr = (loff_t)(off & ~(nand->writesize - 1));
> +
> +	while (nrblocks--) {
> +		ops.datbuf = buf;
> +		/*
> +		 * for read oobbuf must be set, but oob data
> +		 * will be appended to ops.datbuf

Hmm, that sounds like a bug in nand_do_read_ops().

> +		 * for write oobbuf is actually used
> +		 */
> +		ops.oobbuf = buf + nand->writesize;
> +		if (rdwr == NAND_RW_RAW_READ)
> +			i = nand->read_oob(nand, addr, &ops);
> +		else
> +			i = nand->write_oob(nand, addr, &ops);
> +		if (i < 0) {
> +			printf("Error (%d) %s page %08lx\n", i,
> +					rdwr == NAND_RW_RAW_READ ?
> +						"reading" : "writing",
> +					(unsigned long)addr);

Don't cast to unsigned long; cast to unsigned long long and use %llx
instead.

Change "page" to "offset"; IMHO the former implies "addr / nand->writesize".

> +		buf += (nand->writesize + nand->oobsize);

Unnecessary parens.

> +static int nand_biterr(nand_info_t *nand, ulong off, int bit)
> +{
> +	int ret = 0;
> +	u_char *buf;
> +	ulong blockoff = off & ~(nand->erasesize - 1);
> +	int byteoff = off & (nand->erasesize - 1);
> +	nand_erase_options_t opts = {
> +		.offset = blockoff,
> +		.length = nand->erasesize,
> +	};
> +
> +	buf = malloc(nand->erasesize +
> +			nand->oobsize * (nand->erasesize / nand->writesize));
> +	if (!buf) {
> +		puts("No memory for page buffer\n");

s/page buffer/block buffer/

Also include the name of the function.

> +		return 1;
> +	}
> +	nand_read_raw(nand, blockoff, buf, nand->erasesize);

Check whether it succeeded -- don't erase if you couldn't read the data.

> +	ret = nand_erase_opts(nand, &opts);
> +	if (ret) {
> +		puts("Failed to erase block at %x\n");
> +		return ret;
> +	}
> +
> +	printf("toggling bit %x in byte %x in block %x %02x ->",
> +		bit, byteoff, blockoff, buf[byteoff]);
> +
> +	buf[byteoff] ^= (1 << bit);

Is there any way to flip a bit in the OOB using this command?

Also, unnecessary parens.

> +
> +	printf("%02x\n", buf[byteoff]);

Put a space between -> and the number.

> +		} else if (!strcmp(s, ".raw")) {
> +			if (read)
> +				ret = nand_read_raw(nand, off,
> +						    (u_char *)addr, size);
> +			else
> +				ret = nand_write_raw(nand, off,
> +						     (u_char *)addr, size);

Maybe just pass "!read" into nand_rdwr_raw?

>  		} else {
>  			printf("Unknown nand command suffix '%s'.\n", s);
>  			return 1;
> @@ -437,9 +533,19 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>  		}
>  		return ret;
>  	}
> -
>  	if (strcmp(cmd, "biterr") == 0) {
> -		/* todo */
> +		off = (ulong)simple_strtoul(argv[2], NULL, 16);
> +		i = (int)simple_strtoul(argv[3], NULL, 16);
> +
> +		int ret = nand_biterr(nand, off, i);
> +		if (ret == 0) {
> +			printf("byte offset 0x%08lx toggled bit %d\n",
> +			       (ulong) off, i);
> +			return 0;
> +		} else {
> +			printf("byte offset 0x%08lx could not toggle bit %d\n",
> +			       (ulong) addr, i);
> +		}

Why "addr" on failure but "off" on success?  Looks like addr is used
uninitialized.

As NAND is already on the heavy side, perhaps we should make non-core
functionality like raw accesses and bit errors be configurable?

-Scott