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

[Scott Wood <scottwood@freescale.com>]

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