[U-Boot-Users] [PATCH] NAND read/write.jffs2 fix

[Scott Wood]

Morten Ebbell Hestens wrote:
>  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?

>  
>  static int nand_dump(nand_info_t *nand, ulong off, int only_oob)
>  {
> @@ -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.

> @@ -327,40 +327,19 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>  			goto usage;
>  
>  		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?

> @@ -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?

>  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"
> -	"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"
> -	"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.

> +	   "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?

> +	   "  - 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.

> diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c
> index 1916aca..dd4ed6b 100644
> --- a/drivers/mtd/nand/nand_util.c
> +++ b/drivers/mtd/nand/nand_util.c
> @@ -52,6 +52,14 @@ typedef struct mtd_info	  mtd_info_t;
>  #define cpu_to_je16(x) (x)
>  #define cpu_to_je32(x) (x)
>  
> +#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.

> +/**
> + * get_len_incl_bad
> + *
> + * Check if length including bad blocks fits into device.
> + *
> + * @param nand  	NAND device
> + * @param offset 	offset in flash
> + * @param len		image lenght

length

> + * @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.

> +{
> +	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.

> +		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?

> +/**
> + * 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

length

> + * @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)
> +{
> +	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;
> +	}
> +
> +	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.

> +	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;
> +
> +		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.

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

Why not just p_buffer += write_size?

> +/**
> + * 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?

> +int nand_read_ff_upon_bad (nand_info_t *nand, ulong offset, ulong *len, u_char *buf)
> +{
> +	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...

> +	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
> +			read_length = nand->erasesize - block_offset;
> +		
> +		if (nand_block_isbad (nand, offset)) {
> +			int i;
> +			printf ("Bad block 0x%08x. Read this block as 0xff\n",
> +				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.

> +			result = nand_read (nand, offset, &read_length, p_buffer);
> +			if (result != 0) {
> +				printf ("Read failed %d\n", result);
> +				return result;
> +			}		
> +		}
> +		left_to_read -= read_length;

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

-Scott