[U-Boot-Users] [PATCH] NAND read/write.jffs2 fix
Morten Ebbell Hestnes
morten.hestnes at tandberg.com
Fri May 16 11:22:23 CEST 2008
Hi Scott,
Thanks for a good review !
See my comment below.
I will later make av new version of this patch.
Scott Wood 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?
No, it's for a other patch. I will remove it.
>> @@ -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?
I will revert to get a less noisy patch.
>> @@ -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?
I will revert.
>> 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.
Ok.
>> + "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.
>> + " - 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");
>> 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.
I will remove it.
>> + * @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.
Will change it to a const ulong length.
>> +{
>> + 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.
Agree.
>> + 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.
>> + * @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.
I will fix this.
>
>> + 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.
Agree.
>> + }
>> + left_to_write -= write_size;
>> + offset += write_size;
>> + p_buffer = (u_char *)((ulong)p_buffer + write_size);
>
> Why not just p_buffer += write_size?
Agree.
>> +/**
>> + * 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?
Quoting from a conversation with Stefan Roese:
> > 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.
>> +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...
Ok.
>> + 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.
Ok
>> + }
>> + left_to_read -= read_length;
>
> A blank line after the end of a block would be nice, though.
Ok
Best regards,
Morten
More information about the U-Boot
mailing list