[U-Boot] SPI flash writing

[U-Boot] SPI flash writing

[Gerlando Falauto]

Hi everyone,

[I took the liberty to Cc: Mike and Simon as they have provided patches 
in the area]

I struggled for a while trying to update a Kirkwood-based board to the 
latest u-boot (with Keymile's patches).

As it turned out, our update procedure:

sf probe 0;sf erase 0 50000;sf write ${load_addr_r} 0 ${filesize}

mistakenly expects a maximum size of 0x50000 (327680) bytes for
u-boot.kwb. Sadly, the latest u-boot trunk results in a binary size for 
that board which is dangerously close to that limit. Hence, after adding 
some innocent lines of code, the update procedure could brick the board 
(for no evident reason and with no error message whatsoever) if the 
binary size crosses that boundary.

It turns out somebody else also picked up this "magic" number:
http://lacie-nas.org/doku.php?id=uboot#update_u-boot_mainline

And others have bricked their board, most likely for the same reason:
http://www.trimslice.com/forum/viewtopic.php?f=16&t=462

Also, something bad could happen if you make a mistake in the opposite 
direction (use too big a number for the write size):
http://sequanux.org/pipermail/lacie-nas/2012-March/000378.html

 From what I can understand, writing into a sector which has not been 
erased first is an acceptable behaviour of the flash interface, it will 
just set to zero whatever bits are not zero already, without reporting 
any error whatsoever.

Even though any change we introduce now would only apply to upgrades 
FROM future versions, I think it might be worth fixing this somehow.
I believe several things could be easily done here:

1) a "+" syntax to the "sf update" command so it can be used with 
${filesize} as a parameter, and/or some "read,replace,erase,overwrite" 
block mechanism for the last (incomplete) block

2) an out-of-boundary-check againts the flash size so at least a warning 
is issued when you use too big a size value

3) a command line option ("sf write -v" and/or to "sf update -v"), or an 
entirely new command (like "sf writeverify", "sf updateverify") to read 
back after writing so to double-check what really ended up being written 
to the flash before it's too late.

I'm willing to implement them, but I wanted to hear your thoughts first.

Thanks,
Gerland

[U-Boot] SPI flash writing

[Mike Frysinger]

On Tuesday 13 March 2012 14:25:07 Gerlando Falauto wrote:
> As it turned out, our update procedure:
> 
> sf probe 0;sf erase 0 50000;sf write ${load_addr_r} 0 ${filesize}
> 
> mistakenly expects a maximum size of 0x50000 (327680) bytes for
> u-boot.kwb. Sadly, the latest u-boot trunk results in a binary size for
> that board which is dangerously close to that limit. Hence, after adding
> some innocent lines of code, the update procedure could brick the board
> (for no evident reason and with no error message whatsoever) if the
> binary size crosses that boundary.

sounds like you should define CONFIG_BOARD_SIZE_LIMIT.  this will turn it from 
a runtime failure into a build time failure as u-boot will do size checking on 
the image for you.

>  From what I can understand, writing into a sector which has not been
> erased first is an acceptable behaviour of the flash interface, it will
> just set to zero whatever bits are not zero already, without reporting
> any error whatsoever.

correct

> 1) a "+" syntax to the "sf update" command so it can be used with
> ${filesize} as a parameter, and/or some "read,replace,erase,overwrite"
> block mechanism for the last (incomplete) block

"+len" is already in there for erase, so it'd be trivial to add to the update 
command.  feel free to post a patch and i'd be happy to review/merge.

> 2) an out-of-boundary-check againts the flash size so at least a warning
> is issued when you use too big a size value

i'm not sure about this.  if you want to do size checking, then enable the 
hush shell and do it in a script.

> 3) a command line option ("sf write -v" and/or to "sf update -v"), or an
> entirely new command (like "sf writeverify", "sf updateverify") to read
> back after writing so to double-check what really ended up being written
> to the flash before it's too late.

i don't think our other flash interfaces have a verify command, so this would 
be a general question -- the spi flash interface shouldn't diverge if we don't 
want this in general.  then again, this too would be a fairly simple thing to 
write in a hush script.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120313/6afe44c4/attachment.pgp>

[U-Boot] SPI flash writing

[Jason Cooper]

On Tue, Mar 13, 2012 at 04:11:29PM -0400, Mike Frysinger wrote:
> On Tuesday 13 March 2012 14:25:07 Gerlando Falauto wrote:
> > 2) an out-of-boundary-check againts the flash size so at least a warning
> > is issued when you use too big a size value
> 
> i'm not sure about this.  if you want to do size checking, then enable the 
> hush shell and do it in a script.

Is there a programatic way to get the size of the flash at runtime from
the hush script?

thx,

Jason.

[U-Boot] SPI flash writing

[Mike Frysinger]

On Tuesday 13 March 2012 16:17:52 Jason Cooper wrote:
> On Tue, Mar 13, 2012 at 04:11:29PM -0400, Mike Frysinger wrote:
> > On Tuesday 13 March 2012 14:25:07 Gerlando Falauto wrote:
> > > 2) an out-of-boundary-check againts the flash size so at least a
> > > warning is issued when you use too big a size value
> > 
> > i'm not sure about this.  if you want to do size checking, then enable
> > the hush shell and do it in a script.
> 
> Is there a programatic way to get the size of the flash at runtime from
> the hush script?

no.  question is, do you really need that ?  sounds like you know ahead of 
time how big the space is for u-boot, so the size of the flash doesn't matter.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120313/4cdd37ac/attachment.pgp>

[U-Boot] SPI flash writing

[Falauto, Gerlando]

>-----Original Message-----
>From: Mike Frysinger [mailto:vapier at gentoo.org]
>Sent: Tue 13.03.2012 21:35
>To: Jason Cooper
>Cc: Falauto, Gerlando; u-boot; Brunck, Holger
>Subject: Re: [U-Boot] SPI flash writing
 
>On Tuesday 13 March 2012 16:17:52 Jason Cooper wrote:
>> On Tue, Mar 13, 2012 at 04:11:29PM -0400, Mike Frysinger wrote:
>> > On Tuesday 13 March 2012 14:25:07 Gerlando Falauto wrote:
>> > > 2) an out-of-boundary-check againts the flash size so at least a
>> > > warning is issued when you use too big a size value
>> > 
>> > i'm not sure about this.  if you want to do size checking, then enable
>> > the hush shell and do it in a script.
>> 
>> Is there a programatic way to get the size of the flash at runtime from
>> the hush script?

>no.  question is, do you really need that ?  sounds like you know ahead of 
>time how big the space is for u-boot, so the size of the flash doesn't matter.

Can't the same command also be used for burning something *other than* u-boot (e.g. a kernel, config section, or something like that)?
So the size of the flash *does matter*, doesn't it?

Gerlando

[U-Boot] SPI flash writing

[Mike Frysinger]

On Tuesday 13 March 2012 17:31:02 Falauto, Gerlando wrote:
> From: Mike Frysinger [mailto:vapier at gentoo.org]
> >On Tuesday 13 March 2012 16:17:52 Jason Cooper wrote:
> >> On Tue, Mar 13, 2012 at 04:11:29PM -0400, Mike Frysinger wrote:
> >> > On Tuesday 13 March 2012 14:25:07 Gerlando Falauto wrote:
> >> > > 2) an out-of-boundary-check againts the flash size so at least a
> >> > > warning is issued when you use too big a size value
> >> > 
> >> > i'm not sure about this.  if you want to do size checking, then enable
> >> > the hush shell and do it in a script.
> >> 
> >> Is there a programatic way to get the size of the flash at runtime from
> >> the hush script?
> >
> >no.  question is, do you really need that ?  sounds like you know ahead of
> >time how big the space is for u-boot, so the size of the flash doesn't
> >matter.
> 
> Can't the same command also be used for burning something *other than*
> u-boot (e.g. a kernel, config section, or something like that)? So the
> size of the flash *does matter*, doesn't it?

you have to show how it actually does matter.  when you deploy a board and 
programming the flash directly, you don't let the regions grow arbitrarily.  
you know how big the space for u-boot, or the kernel, or dedicated config 
space, or anything else is going to be.  if you want to arbitrarily append 
things, then you're going to use a filesystem.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120313/af5199cd/attachment.pgp>

[U-Boot] SPI flash writing

[Gerlando Falauto]

On 03/14/2012 03:16 AM, Mike Frysinger wrote:
> On Tuesday 13 March 2012 17:31:02 Falauto, Gerlando wrote:
>> From: Mike Frysinger [mailto:vapier at gentoo.org]
>>> On Tuesday 13 March 2012 16:17:52 Jason Cooper wrote:
>>>> On Tue, Mar 13, 2012 at 04:11:29PM -0400, Mike Frysinger wrote:
>>>>> On Tuesday 13 March 2012 14:25:07 Gerlando Falauto wrote:
>>>>>> 2) an out-of-boundary-check againts the flash size so at least a
>>>>>> warning is issued when you use too big a size value
>>>>>
>>>>> i'm not sure about this.  if you want to do size checking, then enable
>>>>> the hush shell and do it in a script.
>>>>
>>>> Is there a programatic way to get the size of the flash at runtime from
>>>> the hush script?
>>>
>>> no.  question is, do you really need that ?  sounds like you know ahead of
>>> time how big the space is for u-boot, so the size of the flash doesn't
>>> matter.
>>
>> Can't the same command also be used for burning something *other than*
>> u-boot (e.g. a kernel, config section, or something like that)? So the
>> size of the flash *does matter*, doesn't it?
>
> you have to show how it actually does matter.  when you deploy a board and
> programming the flash directly, you don't let the regions grow arbitrarily.
> you know how big the space for u-boot, or the kernel, or dedicated config
> space, or anything else is going to be.  if you want to arbitrarily append
> things, then you're going to use a filesystem.
> -mike

You're right, but I failed to stress enough one point.
The thing is, if you issue e write (or erase) and accidentally cross the 
flash size boundary, you get a wraparound (or aliasing, or whatever you 
want to call it) so that you end up overwriting (e.g. zeroing out bits) 
the initial sectors of the flash. Which is where u-boot normally lies. 
[At least, that's the way I understand things are working right now.] I 
hope you'd agree that this is *a bad thing (TM)*.
My concern is not about the fully aware u-boot developer, who normally 
has some other way to restore a dead board (JTAG, rom monitor, etc...).
My conncern is about mistakes made by the absent-minded user who reads 
an upgrade procedure somewhere, puts an extra zero, and ends up bricking 
their board, whereas it could have been easily avoided.

Thanks,
Gerlando

[U-Boot] SPI flash writing

[Mike Frysinger]

On Wednesday 14 March 2012 02:44:45 Gerlando Falauto wrote:
> The thing is, if you issue e write (or erase) and accidentally cross the
> flash size boundary, you get a wraparound (or aliasing, or whatever you
> want to call it) so that you end up overwriting (e.g. zeroing out bits)
> the initial sectors of the flash.

this is a bug that should be fixed.  i thought there was size checking in 
there, but maybe it got lost in the shuffle, or i'm just making sh*t up.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120314/8fd191d7/attachment.pgp>

[U-Boot] SPI flash writing

[Tom Rini]

On Tue, Mar 13, 2012 at 10:31:02PM +0100, Falauto, Gerlando wrote:
> 
> >-----Original Message-----
> >From: Mike Frysinger [mailto:vapier at gentoo.org]
> >Sent: Tue 13.03.2012 21:35
> >To: Jason Cooper
> >Cc: Falauto, Gerlando; u-boot; Brunck, Holger
> >Subject: Re: [U-Boot] SPI flash writing
>  
> >On Tuesday 13 March 2012 16:17:52 Jason Cooper wrote:
> >> On Tue, Mar 13, 2012 at 04:11:29PM -0400, Mike Frysinger wrote:
> >> > On Tuesday 13 March 2012 14:25:07 Gerlando Falauto wrote:
> >> > > 2) an out-of-boundary-check againts the flash size so at least a
> >> > > warning is issued when you use too big a size value
> >> > 
> >> > i'm not sure about this.  if you want to do size checking, then enable
> >> > the hush shell and do it in a script.
> >> 
> >> Is there a programatic way to get the size of the flash at runtime from
> >> the hush script?
> 
> >no.  question is, do you really need that ?  sounds like you know ahead of 
> >time how big the space is for u-boot, so the size of the flash doesn't matter.
> 
> Can't the same command also be used for burning something *other than* u-boot (e.g. a kernel, config section, or something like that)?
> So the size of the flash *does matter*, doesn't it?

How about using mtdparts which I think will tell you when you're going
to write something larger than the defined partition?  If it doesn't,
that would be a handy thing to add (and be a general feature too).

-- 
Tom

[U-Boot] SPI flash writing

[Mike Frysinger]

On Wednesday 14 March 2012 20:02:27 Tom Rini wrote:
> On Tue, Mar 13, 2012 at 10:31:02PM +0100, Falauto, Gerlando wrote:
> > Can't the same command also be used for burning something *other than*
> > u-boot (e.g. a kernel, config section, or something like that)? So the
> > size of the flash *does matter*, doesn't it?
> 
> How about using mtdparts which I think will tell you when you're going
> to write something larger than the defined partition?  If it doesn't,
> that would be a handy thing to add (and be a general feature too).

i was about to suggest this :).  unfortunately, in order to make that work, i 
think the spi flash code needs to be migrated to the mtd layers ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120314/7ad34d44/attachment.pgp>

[U-Boot] SPI flash writing

[Simon Glass]

Hi Gerlando,

On Tue, Mar 13, 2012 at 11:25 AM, Gerlando Falauto
<gerlando.falauto@keymile.com> wrote:
> Hi everyone,
>
> [I took the liberty to Cc: Mike and Simon as they have provided patches in
> the area]
>
> I struggled for a while trying to update a Kirkwood-based board to the
> latest u-boot (with Keymile's patches).
>
> As it turned out, our update procedure:
>
> sf probe 0;sf erase 0 50000;sf write ${load_addr_r} 0 ${filesize}
>
> mistakenly expects a maximum size of 0x50000 (327680) bytes for
> u-boot.kwb. Sadly, the latest u-boot trunk results in a binary size for that
> board which is dangerously close to that limit. Hence, after adding some
> innocent lines of code, the update procedure could brick the board (for no
> evident reason and with no error message whatsoever) if the binary size
> crosses that boundary.
>
> It turns out somebody else also picked up this "magic" number:
> http://lacie-nas.org/doku.php?id=uboot#update_u-boot_mainline
>
> And others have bricked their board, most likely for the same reason:
> http://www.trimslice.com/forum/viewtopic.php?f=16&t=462
>
> Also, something bad could happen if you make a mistake in the opposite
> direction (use too big a number for the write size):
> http://sequanux.org/pipermail/lacie-nas/2012-March/000378.html
>
> From what I can understand, writing into a sector which has not been erased
> first is an acceptable behaviour of the flash interface, it will just set to
> zero whatever bits are not zero already, without reporting any error
> whatsoever.
>
> Even though any change we introduce now would only apply to upgrades FROM
> future versions, I think it might be worth fixing this somehow.
> I believe several things could be easily done here:
>
> 1) a "+" syntax to the "sf update" command so it can be used with
> ${filesize} as a parameter, and/or some "read,replace,erase,overwrite" block
> mechanism for the last (incomplete) block

Sounds reasonable, although I wonder if it is worth worrying about
preserved the rest of the contents of the last block.

>
> 2) an out-of-boundary-check againts the flash size so at least a warning is
> issued when you use too big a size value

Should be easy enough.
>
> 3) a command line option ("sf write -v" and/or to "sf update -v"), or an
> entirely new command (like "sf writeverify", "sf updateverify") to read back
> after writing so to double-check what really ended up being written to the
> flash before it's too late.

I'd like a -V (instead of -v which could perhaps be used for verbose).

But as Mike mentions I wonder if we could/should do this generally for
all flash?

Also, why do you get verify failures? 'sf update' will auto-erase when
it needs to. Do you really have a chip which reports success but then
fails? Or is it just a problem with the size being too large?

>
> I'm willing to implement them, but I wanted to hear your thoughts first.
>

Always good.

Regards,
Simon

> Thanks,
> Gerland

[U-Boot] SPI flash writing

[Gerlando Falauto]

On 03/14/2012 03:18 AM, Simon Glass wrote:
> Hi Gerlando,
>
> On Tue, Mar 13, 2012 at 11:25 AM, Gerlando Falauto
> <gerlando.falauto@keymile.com>  wrote:
>> Hi everyone,
>>
>> [I took the liberty to Cc: Mike and Simon as they have provided patches in
>> the area]
>>
>> I struggled for a while trying to update a Kirkwood-based board to the
>> latest u-boot (with Keymile's patches).
>>
>> As it turned out, our update procedure:
>>
>> sf probe 0;sf erase 0 50000;sf write ${load_addr_r} 0 ${filesize}
>>
>> mistakenly expects a maximum size of 0x50000 (327680) bytes for
>> u-boot.kwb. Sadly, the latest u-boot trunk results in a binary size for that
>> board which is dangerously close to that limit. Hence, after adding some
>> innocent lines of code, the update procedure could brick the board (for no
>> evident reason and with no error message whatsoever) if the binary size
>> crosses that boundary.
>>
>> It turns out somebody else also picked up this "magic" number:
>> http://lacie-nas.org/doku.php?id=uboot#update_u-boot_mainline
>>
>> And others have bricked their board, most likely for the same reason:
>> http://www.trimslice.com/forum/viewtopic.php?f=16&t=462
>>
>> Also, something bad could happen if you make a mistake in the opposite
>> direction (use too big a number for the write size):
>> http://sequanux.org/pipermail/lacie-nas/2012-March/000378.html
>>
>>  From what I can understand, writing into a sector which has not been erased
>> first is an acceptable behaviour of the flash interface, it will just set to
>> zero whatever bits are not zero already, without reporting any error
>> whatsoever.
>>
>> Even though any change we introduce now would only apply to upgrades FROM
>> future versions, I think it might be worth fixing this somehow.
>> I believe several things could be easily done here:
>>
>> 1) a "+" syntax to the "sf update" command so it can be used with
>> ${filesize} as a parameter, and/or some "read,replace,erase,overwrite" block
>> mechanism for the last (incomplete) block
>
> Sounds reasonable, although I wonder if it is worth worrying about
> preserved the rest of the contents of the last block.

Probably unimportant, as everything you'd ever want to write would be 
block aligned. But I think it still makes sense to make the semantics of 
the update command (which could be also thought of as a way of patching 
regions of arbitrary alignment and length) consistent with linux's 
/dev/mtd, which sort of allows you to do any such things (hiding from 
the user any notion of sector/block).
But please correct me if I'm talking nonsense.

>> 2) an out-of-boundary-check againts the flash size so at least a warning is
>> issued when you use too big a size value
>
> Should be easy enough.
>>
>> 3) a command line option ("sf write -v" and/or to "sf update -v"), or an
>> entirely new command (like "sf writeverify", "sf updateverify") to read back
>> after writing so to double-check what really ended up being written to the
>> flash before it's too late.
>
> I'd like a -V (instead of -v which could perhaps be used for verbose).
>
> But as Mike mentions I wonder if we could/should do this generally for
> all flash?

I agree it would totally make sense.

> Also, why do you get verify failures? 'sf update' will auto-erase when
> it needs to.

True. The option would probably make more sense with the "write" command.

> Do you really have a chip which reports success but then
> fails? Or is it just a problem with the size being too large?

Uhm, there's actually two different problems.
One, "forgetting" to erase enough blocks. This is where -V might come 
useful, but with "write" only.
Two, accidentally writing/updating past the flash size. Here -V would 
not help much (unless you wrap around the whole flash and past the 
starting address again!), but size checking should at least warn you.

Best,
Gerlando

[U-Boot] [PATCH] cmd_sf: add size checking to spi flash commands

[Gerlando Falauto]

SPI flash operations inadvertently stretching beyond the flash size will
result in a wraparound. This may be particularly dangerous when burning
u-boot, because the flash contents will be corrupted rendering the board
unusable, without any warning being issued.
So add a consistency checking so not to overflow past the flash size.

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
Cc: Holger Brunck <holger.brunck@keymile.com>
---
 common/cmd_sf.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index 9c76464..3cfedde 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -211,6 +211,13 @@ static int do_spi_flash_read_write(int argc, char * const argv[])
 	if (*argv[3] == 0 || *endp != 0)
 		return -1;
 
+	/* Consistency checking */
+	if (offset + len > flash->size) {
+		printf("ERROR: Attempting SPI flash %s past flash size (0x%x)\n",
+			argv[0], flash->size);
+		return 1;
+	}
+
 	buf = map_physmem(addr, len, MAP_WRBACK);
 	if (!buf) {
 		puts("Failed to map physical memory\n");
@@ -252,6 +259,13 @@ static int do_spi_flash_erase(int argc, char * const argv[])
 	if (ret != 1)
 		return -1;
 
+	/* Consistency checking */
+	if (offset + len > flash->size) {
+		printf("ERROR: Attempting SPI flash %s past flash size (0x%x)\n",
+			argv[0], flash->size);
+		return 1;
+	}
+
 	ret = spi_flash_erase(flash, offset, len);
 	if (ret) {
 		printf("SPI flash %s failed\n", argv[0]);
-- 
1.7.1

[U-Boot] [PATCH] cmd_sf: add size checking to spi flash commands

[Mike Frysinger]

On Tue, Apr 3, 2012 at 07:34, Gerlando Falauto wrote:
> SPI flash operations inadvertently stretching beyond the flash size will
> result in a wraparound. This may be particularly dangerous when burning
> u-boot, because the flash contents will be corrupted rendering the board
> unusable, without any warning being issued.
> So add a consistency checking so not to overflow past the flash size.

looks OK to me.  i'll test it locally and merge it into my SF branch.  cheers!
-mike

[U-Boot] [PATCH v2] cmd_sf: add size checking to spi flash commands

[Mike Frysinger]

From: Gerlando Falauto <gerlando.falauto@keymile.com>

SPI flash operations inadvertently stretching beyond the flash size will
result in a wraparound. This may be particularly dangerous when burning
u-boot, because the flash contents will be corrupted rendering the board
unusable, without any warning being issued.
So add a consistency checking so not to overflow past the flash size.

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v2
	- tweaked the printf strings

 common/cmd_sf.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index 9c76464..5ac1d0c 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -211,6 +211,13 @@ static int do_spi_flash_read_write(int argc, char * const argv[])
 	if (*argv[3] == 0 || *endp != 0)
 		return -1;
 
+	/* Consistency checking */
+	if (offset + len > flash->size) {
+		printf("ERROR: attempting %s past flash size (%#x)\n",
+			argv[0], flash->size);
+		return 1;
+	}
+
 	buf = map_physmem(addr, len, MAP_WRBACK);
 	if (!buf) {
 		puts("Failed to map physical memory\n");
@@ -252,6 +259,13 @@ static int do_spi_flash_erase(int argc, char * const argv[])
 	if (ret != 1)
 		return -1;
 
+	/* Consistency checking */
+	if (offset + len > flash->size) {
+		printf("ERROR: attempting %s past flash size (%#x)\n",
+			argv[0], flash->size);
+		return 1;
+	}
+
 	ret = spi_flash_erase(flash, offset, len);
 	if (ret) {
 		printf("SPI flash %s failed\n", argv[0]);
-- 
1.7.9.7

[U-Boot] [PATCH 0/2] SPI flash update command

[Gerlando Falauto]

Hi,

this patchset allows "sf update" to erase+write a number of bytes which is not a
multiple of the sector size.  Start address must still be sector-aligned though.

The first patch trivially makes it such it will always erase an entire sector
before writing, regardless of the amount of data to write (i.e. the last sector
is erased completely before writing it only partially).

The second patch just makes sure that the original data at the end of the sector
is written back so to apparently remain unchanged.

I anticipate two potential objections already:

- whether it is really worth writing back the portion of the sector which was
  erased but shouldn't have been overwritten (whole purpose of the second patch)

- these changes make the semantics of "sf update" and "sf erase" somewhat
  different, in that "sf erase" needs a "+" to deal with odd lengths, whereas
  "sf update" does not.  I think this is only partially true.  After all, "sf
  update" is already somehow special.  It's not a standard operation for a flash
  (erase, read, write).  It combines two of those operations, and takes care of
  optimizing the process by removing unneeded erase/write operations.  So it
  might as well deserve to be "special", in my opinion.  Plus, it makes the
  command for updating u-boot as easy as

     sf update ${load_addr_r} 0 ${filesize}

Feedback and criticism more than welcome, of course.

Thank you,
Gerlando

Gerlando Falauto (2):
  cmd_sf: let "sf update" erase last sector as a whole
  cmd_sf: "sf update" preserve the final part of the last sector

 common/cmd_sf.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

[U-Boot] [PATCH 0/2] SPI flash update command

[Valentin Longchamp]

Hi,

We had discussed this internally with Gerlando, but I will repeat my position
here on the ML for the sake of the discussion.

On 04/03/2012 05:14 PM, Gerlando Falauto wrote:
> Hi,
> 
> this patchset allows "sf update" to erase+write a number of bytes which is not a
> multiple of the sector size.  Start address must still be sector-aligned though.

First I think this feature is really nice: the u-boot update command then
becomes really simple. But I have some remarks on how we should implement it.

That's my first remark here: some arguments have to be sector aligned and some
not. That's not really consistent.

> 
> The first patch trivially makes it such it will always erase an entire sector
> before writing, regardless of the amount of data to write (i.e. the last sector
> is erased completely before writing it only partially).
> 
> The second patch just makes sure that the original data at the end of the sector
> is written back so to apparently remain unchanged.
> 
> I anticipate two potential objections already:
> 
> - whether it is really worth writing back the portion of the sector which was
>   erased but shouldn't have been overwritten (whole purpose of the second patch)
> 
> - these changes make the semantics of "sf update" and "sf erase" somewhat
>   different, in that "sf erase" needs a "+" to deal with odd lengths, whereas
>   "sf update" does not.  I think this is only partially true.  After all, "sf
>   update" is already somehow special.  It's not a standard operation for a flash
>   (erase, read, write).  It combines two of those operations, and takes care of
>   optimizing the process by removing unneeded erase/write operations.  So it
>   might as well deserve to be "special", in my opinion.  Plus, it makes the
>   command for updating u-boot as easy as

The sector aligned arguments are mandatory for the erase command, and update is
as explained above erase + write commands (some erase/writes may be spared, but
that's transparent to the users). So why have a different nomenclature for
update and erase, when update is only putting erase and write commands together ?

With the "+" approach for the update command, we would achieve exactly the same
as proposed by these 2 patches, plus we are consistent with the "+" nomenclature
of the erase.

> 
>      sf update ${load_addr_r} 0 ${filesize}

It would then only be:

	sf update ${load_addr_r} 0 +${filesize}

Not a lot more complicated.

We have to think about 2 points here:

1) Do we want our sf commands to remain tied to the sectors ? If we want low
level control on the flash, that may be something desirable and then the "+"
approach makes more sense (you know that you may not be sector aligned and imply
with the "+".

2) Do we want our sf commands that are concerned by sector size (erase and
update) to have consistence in the arguments ? Why would an erase require a "+"
that an update does not require ? (This means that if we add the same
functionality than the "+" in update without the "+", why keep it for erase ?).

> 
> Feedback and criticism more than welcome, of course.
> 
> Thank you,
> Gerlando
> 
> Gerlando Falauto (2):
>   cmd_sf: let "sf update" erase last sector as a whole
>   cmd_sf: "sf update" preserve the final part of the last sector
> 
>  common/cmd_sf.c |   18 ++++++++++++++++--
>  1 files changed, 16 insertions(+), 2 deletions(-)
> 

Best Regards

Valentin

[U-Boot] [PATCH 1/2] cmd_sf: let "sf update" erase last sector as a whole

[Gerlando Falauto]

make "sf update" work with unaligned `len' parameter, by deleting the
whole last sector before writing, so to allow for:

 sf update ${load_addr_r} 0 ${filesize}

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
Cc: Holger Brunck <holger.brunck@keymile.com>
---
 common/cmd_sf.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index 3cfedde..d97d4a5 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -142,7 +142,8 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
 		*skipped += len;
 		return NULL;
 	}
-	if (spi_flash_erase(flash, offset, len))
+	/* Erase the entire sector */
+	if (spi_flash_erase(flash, offset, flash->sector_size))
 		return "erase";
 	if (spi_flash_write(flash, offset, len, buf))
 		return "write";
-- 
1.7.1

[U-Boot] [PATCH 1/2] cmd_sf: let "sf update" erase last sector as a whole

[Simon Glass]

On Tue, Apr 3, 2012 at 8:14 AM, Gerlando Falauto
<gerlando.falauto@keymile.com> wrote:
> make "sf update" work with unaligned `len' parameter, by deleting the
> whole last sector before writing, so to allow for:
>
> ?sf update ${load_addr_r} 0 ${filesize}
>
> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
> Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
> Cc: Holger Brunck <holger.brunck@keymile.com>

Acked-by: Simon Glass <sjg@chromium.org>

> ---
> ?common/cmd_sf.c | ? ?3 ++-
> ?1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> index 3cfedde..d97d4a5 100644
> --- a/common/cmd_sf.c
> +++ b/common/cmd_sf.c
> @@ -142,7 +142,8 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
> ? ? ? ? ? ? ? ?*skipped += len;
> ? ? ? ? ? ? ? ?return NULL;
> ? ? ? ?}
> - ? ? ? if (spi_flash_erase(flash, offset, len))
> + ? ? ? /* Erase the entire sector */
> + ? ? ? if (spi_flash_erase(flash, offset, flash->sector_size))
> ? ? ? ? ? ? ? ?return "erase";
> ? ? ? ?if (spi_flash_write(flash, offset, len, buf))
> ? ? ? ? ? ? ? ?return "write";
> --
> 1.7.1
>

[U-Boot] [PATCH 2/2] cmd_sf: "sf update" preserve the final part of the last sector

[Gerlando Falauto]

Since "sf update" erases the last block as a whole, but only rewrites
the meaningful initial part of it, the rest would be left erased,
potentially erasing meaningful information.
So, as a safety measure, have it rewrite the original content.

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
Cc: Holger Brunck <holger.brunck@keymile.com>
---
 common/cmd_sf.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index d97d4a5..0666f52 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -134,8 +134,10 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
 {
 	debug("offset=%#x, sector_size=%#x, len=%#zx\n",
 		offset, flash->sector_size, len);
-	if (spi_flash_read(flash, offset, len, cmp_buf))
+	/* Read the entire sector so to allow for rewriting */
+	if (spi_flash_read(flash, offset, flash->sector_size, cmp_buf))
 		return "read";
+	/* Compare only what is meningful (len) */
 	if (memcmp(cmp_buf, buf, len) == 0) {
 		debug("Skip region %x size %zx: no change\n",
 			offset, len);
@@ -145,6 +147,17 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
 	/* Erase the entire sector */
 	if (spi_flash_erase(flash, offset, flash->sector_size))
 		return "erase";
+	/* If it's a partial sector, preserve the existing part */
+	if (len != flash->sector_size) {
+		/* Overwrite the first part of the sector with input data */
+		memcpy(cmp_buf, buf, len);
+		/* Rewrite the whole sector with original data at the end */
+		if (spi_flash_write(flash, offset, flash->sector_size,
+					cmp_buf))
+			return "write";
+		return NULL;
+	}
+	/* Rewrite the whole block from the source */
 	if (spi_flash_write(flash, offset, len, buf))
 		return "write";
 	return NULL;
-- 
1.7.1

[U-Boot] [PATCH 2/2] cmd_sf: "sf update" preserve the final part of the last sector

[Simon Glass]

Hi Gerlando,

On Tue, Apr 3, 2012 at 8:14 AM, Gerlando Falauto
<gerlando.falauto@keymile.com> wrote:
> Since "sf update" erases the last block as a whole, but only rewrites
> the meaningful initial part of it, the rest would be left erased,
> potentially erasing meaningful information.
> So, as a safety measure, have it rewrite the original content.
>
> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
> Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
> Cc: Holger Brunck <holger.brunck@keymile.com>

Acked-by: Simon Glass <sjg@chromium.org>

> ---
> ?common/cmd_sf.c | ? 15 ++++++++++++++-
> ?1 files changed, 14 insertions(+), 1 deletions(-)
>
> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> index d97d4a5..0666f52 100644
> --- a/common/cmd_sf.c
> +++ b/common/cmd_sf.c
> @@ -134,8 +134,10 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
> ?{
> ? ? ? ?debug("offset=%#x, sector_size=%#x, len=%#zx\n",
> ? ? ? ? ? ? ? ?offset, flash->sector_size, len);
> - ? ? ? if (spi_flash_read(flash, offset, len, cmp_buf))
> + ? ? ? /* Read the entire sector so to allow for rewriting */
> + ? ? ? if (spi_flash_read(flash, offset, flash->sector_size, cmp_buf))
> ? ? ? ? ? ? ? ?return "read";
> + ? ? ? /* Compare only what is meningful (len) */
> ? ? ? ?if (memcmp(cmp_buf, buf, len) == 0) {
> ? ? ? ? ? ? ? ?debug("Skip region %x size %zx: no change\n",
> ? ? ? ? ? ? ? ? ? ? ? ?offset, len);
> @@ -145,6 +147,17 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
> ? ? ? ?/* Erase the entire sector */
> ? ? ? ?if (spi_flash_erase(flash, offset, flash->sector_size))
> ? ? ? ? ? ? ? ?return "erase";
> + ? ? ? /* If it's a partial sector, preserve the existing part */
> + ? ? ? if (len != flash->sector_size) {
> + ? ? ? ? ? ? ? /* Overwrite the first part of the sector with input data */
> + ? ? ? ? ? ? ? memcpy(cmp_buf, buf, len);
> + ? ? ? ? ? ? ? /* Rewrite the whole sector with original data at the end */
> + ? ? ? ? ? ? ? if (spi_flash_write(flash, offset, flash->sector_size,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cmp_buf))
> + ? ? ? ? ? ? ? ? ? ? ? return "write";
> + ? ? ? ? ? ? ? return NULL;

If you wrote just the last part of the sector here then you could
perhaps avoid the memcpy() and the return NULL.

> + ? ? ? }
> + ? ? ? /* Rewrite the whole block from the source */
> ? ? ? ?if (spi_flash_write(flash, offset, len, buf))
> ? ? ? ? ? ? ? ?return "write";
> ? ? ? ?return NULL;
> --
> 1.7.1
>

Regards,
Simon