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

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