[U-Boot] [PATCH] Pad data length for nand write

[U-Boot] [PATCH] Pad data length for nand write

[Derek Ou]

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);
-- 
1.5.4.3

[U-Boot] [PATCH] Pad data length for nand write

[Derek Ou]

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);
-- 
1.5.4.3

[U-Boot] [PATCH] Pad data length for nand write

[Scott Wood]

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

[U-Boot] [PATCH] Pad data length for nand write

[Derek Ou]

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
>   

[U-Boot] [PATCH] Pad data length for nand write

[Scott Wood]

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

[U-Boot] [PATCH] Pad data length for nand write

[Derek Ou]

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

[U-Boot] [PATCH] Pad data length for nand write

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090217/ccc12d84/attachment.pgp 

[U-Boot] [PATCH] Pad data length for nand write

[Derek Ou]

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
>   

[U-Boot] [PATCH] Pad data length for nand write

[Scott Wood]

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

[U-Boot] [PATCH] Pad data length for nand write

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A rolling stone gathers momentum.