[U-Boot] [PATCH] Pad data length for nand write
Signed-off-by: Derek Ou dou@siconix.com --- drivers/mtd/nand/nand_util.c | 25 ++++++++++++++++++------- 1 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c index 6ba52b3..b9d292a 100644 --- a/drivers/mtd/nand/nand_util.c +++ b/drivers/mtd/nand/nand_util.c @@ -475,25 +475,36 @@ int nand_write_skip_bad(nand_info_t *nand, size_t offset, size_t *length, { int rval; size_t left_to_write = *length; + size_t page_offset, pad_len = 0; size_t len_incl_bad; u_char *p_buffer = buffer;
- /* Reject writes, which are not page aligned */ - if ((offset & (nand->writesize - 1)) != 0 || - (*length & (nand->writesize - 1)) != 0) { - printf ("Attempt to write non page aligned data\n"); + /* Reject writes when offset is not page aligned */ + if ((offset & (nand->writesize - 1)) != 0 ) { + printf ("Attempt to write address non page aligned\n"); return -EINVAL; }
- len_incl_bad = get_len_incl_bad (nand, offset, *length); + /* Pad write length if it's not page aligned */ + page_offset = left_to_write & (nand->writesize - 1); + if (page_offset != 0) { + pad_len = nand->writesize - page_offset; + left_to_write += pad_len; + } + + len_incl_bad = get_len_incl_bad (nand, offset, left_to_write);
if ((offset + len_incl_bad) >= nand->size) { printf ("Attempt to write outside the flash area\n"); return -EINVAL; }
- if (len_incl_bad == *length) { - rval = nand_write (nand, offset, length, buffer); + /* now, pad data with 0xff */ + if (page_offset != 0) + memset(buffer + *length, 0xff, pad_len); + + if (len_incl_bad == left_to_write) { + rval = nand_write (nand, offset, &left_to_write, buffer); if (rval != 0) printf ("NAND write to offset %zx failed %d\n", offset, rval);
Signed-off-by: Derek Ou dou@siconix.com --- drivers/mtd/nand/nand_util.c | 25 ++++++++++++++++++------- 1 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c index 6ba52b3..b9d292a 100644 --- a/drivers/mtd/nand/nand_util.c +++ b/drivers/mtd/nand/nand_util.c @@ -475,25 +475,36 @@ int nand_write_skip_bad(nand_info_t *nand, size_t offset, size_t *length, { int rval; size_t left_to_write = *length; + size_t page_offset, pad_len = 0; size_t len_incl_bad; u_char *p_buffer = buffer;
- /* Reject writes, which are not page aligned */ - if ((offset & (nand->writesize - 1)) != 0 || - (*length & (nand->writesize - 1)) != 0) { - printf ("Attempt to write non page aligned data\n"); + /* Reject writes when offset is not page aligned */ + if ((offset & (nand->writesize - 1)) != 0 ) { + printf ("Attempt to write address non page aligned\n"); return -EINVAL; }
- len_incl_bad = get_len_incl_bad (nand, offset, *length); + /* Pad write length if it's not page aligned */ + page_offset = left_to_write & (nand->writesize - 1); + if (page_offset != 0) { + pad_len = nand->writesize - page_offset; + left_to_write += pad_len; + } + + len_incl_bad = get_len_incl_bad (nand, offset, left_to_write);
if ((offset + len_incl_bad) >= nand->size) { printf ("Attempt to write outside the flash area\n"); return -EINVAL; }
- if (len_incl_bad == *length) { - rval = nand_write (nand, offset, length, buffer); + /* now, pad data with 0xff */ + if (page_offset != 0) + memset(buffer + *length, 0xff, pad_len); + + if (len_incl_bad == left_to_write) { + rval = nand_write (nand, offset, &left_to_write, buffer); if (rval != 0) printf ("NAND write to offset %zx failed %d\n", offset, rval);
On Tue, Feb 17, 2009 at 10:15:07AM -0700, Derek Ou wrote:
- /* Reject writes when offset is not page aligned */
- if ((offset & (nand->writesize - 1)) != 0 ) {
No space before ')'. Better yet, just get rid of the != 0 part.
- /* now, pad data with 0xff */
- if (page_offset != 0)
memset(buffer + *length, 0xff, pad_len);
You cannot write to the caller's buffer (or worse, past the end of the caller's buffer) like this. You'll need to allocate a new, padded buffer.
-Scott
This routine is copied from nand_write_opts() at nand_util.c of v1.3.4. It maybe better to pad data length and data buffer in do_nand() at cmd_nand.c. But I don't see real difference. I didn't allocate a new buffer since the data length is defined in the command arguments and I have no knowledge of a safe location and large enough memory space to store data.
Maybe we should implement, like Wolfgang said, a "+length" syntax in the "nand write" command to indicates "round up to the next page boundary". In this case, do_nand() can assume that it's safe to write pass memory address (add + length) and Flash offset (off + length).
Derek Scott Wood wrote:
On Tue, Feb 17, 2009 at 10:15:07AM -0700, Derek Ou wrote:
- /* now, pad data with 0xff */
- if (page_offset != 0)
memset(buffer + *length, 0xff, pad_len);You cannot write to the caller's buffer (or worse, past the end of the caller's buffer) like this. You'll need to allocate a new, padded buffer.
-Scott
Derek Ou wrote:
This routine is copied from nand_write_opts() at nand_util.c of v1.3.4.
v1.3.4 had its own buffer; it did not use the caller's.
It maybe better to pad data length and data buffer in do_nand() at cmd_nand.c.
Or pass a flag to nand_write_skip_bad().
But I
don't see real difference. I didn't allocate a new buffer since the data length is defined in the command arguments and I have no knowledge of a safe location and large enough memory space to store data.
You can define a static array for holding one page, as v1.3.4 did. The space after the caller's buffer is *not* a safe location.
Maybe we should implement, like Wolfgang said, a "+length" syntax in the "nand write" command to indicates "round up to the next page boundary".
Sounds good. If we do that, we should do similarly with "nand erase". It currently (sometimes) warns and rounds up if you give it a non-block-aligned size.
In this case, do_nand() can assume that it's safe to write pass memory address (add + length) and Flash offset (off + length).
No, it will *not* assume that it's safe to write to any memory of the caller's. It will allocate its own buffer.
-Scott
Currently, the nand_util.c does not manipulate NAND flash page by page. It's the nand_base.c provides that level of NAND control.
Implementing this properly requires changes to the existing structure. Also, adding "+length" syntax means command interface change as well. Since it's not to the interest of my company to implement new feature instead of maintaining existing behavior, I will not look into this in my working hours then. So everybody is welcomed to work on this issue and submit your patches. I'd love to see them.
By the way, I think WATCHDOG_RESET() should be added to nand_do_write_opts() and nand_do_read_opts() at nand_base.c. Please include it if you have patch for the above implementation.
Derek Scott Wood wrote:
You can define a static array for holding one page, as v1.3.4 did. The space after the caller's buffer is *not* a safe location.
Maybe we should implement, like Wolfgang said, a "+length" syntax in the "nand write" command to indicates "round up to the next page boundary".
Sounds good. If we do that, we should do similarly with "nand erase". It currently (sometimes) warns and rounds up if you give it a non-block-aligned size.
In this case, do_nand() can assume that it's safe to write pass memory address (add + length) and Flash offset (off + length).
No, it will *not* assume that it's safe to write to any memory of the caller's. It will allocate its own buffer.
-Scott
On Tuesday 17 February 2009 12:15:06 Derek Ou wrote:
Signed-off-by: Derek Ou dou@siconix.com
could you document what this is actually for ? -mike
Sorry that my patch went out twice. I ran into smtp authentication issue with git-send-email. Also, what is the right way to add more comments to a patch? Should I edit the patch generated by git-format-patch or I should just add commit title, blank line and the full commit message when I git commit my change locally.
Anyway, here is the purpose of the patch. Until v1.3.4, "nand write.jffs2" supports non page-aligned data write and pad data automatically to page alignment. As a result, we can use the following scripts to automate downloading a file and writing it to flash. "tftp file.bin" and "nand write.jffs2 add_# off_# $(filesize)" But in the later releases, this feature was no longer supported. So my patch restores this feature to nand write command.
Derek Mike Frysinger wrote:
On Tuesday 17 February 2009 12:15:06 Derek Ou wrote:
Signed-off-by: Derek Ou dou@siconix.com
could you document what this is actually for ? -mike
Derek Ou wrote:
Sorry that my patch went out twice. I ran into smtp authentication issue with git-send-email. Also, what is the right way to add more comments to a patch? Should I edit the patch generated by git-format-patch or I should just add commit title, blank line and the full commit message when I git commit my change locally.
The latter, for anything that belongs in the permanent commit message. For transient information such as what changed since the previous version of the patch, edit the resulting patch below the "---".
-Scott
Dear Derek Ou,
In message 499B03FD.7050207@siconix.com you wrote:
Until v1.3.4, "nand write.jffs2" supports non page-aligned data write and pad data automatically to page alignment. As a result, we can use the following scripts to automate downloading a file and writing it to flash. "tftp file.bin" and "nand write.jffs2 add_# off_# $(filesize)" But in the later releases, this feature was no longer supported. So my patch restores this feature to nand write command.
Such implicit padding is IMHO a bad idea.
I think we should rather add logic similar to what we use on NOR flash and support a "+length" syntax which indicates "please round up to the next block boundary".
You would then write
nand write.jffs2 addr off +${filesize}
to get the wanted behaviour.
Best regards,
Wolfgang Denk
participants (5)
-
Derek Ou -
Derek Ou -
Mike Frysinger -
Scott Wood -
Wolfgang Denk