SPI flash writing

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.

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.

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.

Quoted 2 lines · 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.

Quoted 6 lines · Mike Frysinger

Quoted 2 lines · 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?

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

Quoted 5 lines · On Tuesday 13 March 2012 16:17:52 Jason Cooper wrote:

Quoted 4 lines · Mike Frysinger

Quoted 2 lines · 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

Quoted 5 lines

Quoted 2 lines

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.

From: Mike Frysinger [mailto:vapier at gentoo.org]

Quoted 12 lines · On Tuesday 13 March 2012 16:17:52 Jason Cooper wrote:

Quoted 10 lines · Mike Frysinger

Quoted 6 lines · On Tuesday 13 March 2012 14:25:07 Gerlando Falauto wrote:

Quoted 2 lines

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?

Quoted 3 lines

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?

Quoted 13 lines · Gerlando

From: Mike Frysinger [mailto:vapier at gentoo.org]

Quoted 12 lines · On Tuesday 13 March 2012 16:17:52 Jason Cooper wrote:

Quoted 10 lines · Mike Frysinger

Quoted 6 lines · On Tuesday 13 March 2012 14:25:07 Gerlando Falauto wrote:

Quoted 2 lines

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?

Quoted 7 lines

Quoted 3 lines

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

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.

Quoted 17 lines

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

Quoted 8 lines · On Tuesday 13 March 2012 16:17:52 Jason Cooper wrote:

Quoted 6 lines · Mike Frysinger

Quoted 2 lines · 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.

Quoted 5 lines

Quoted 2 lines

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?

Quoted 3 lines · Gerlando

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

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

Hi Gerlando,

Quoted 38 lines · 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:

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

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?

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.

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(-)

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 <[email protected]>

Cc Valentin Longchamp <[email protected]>

Cc Holger Brunck <[email protected]>

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

common/cmd_sf.c +2 -2

index 3cfedde..d97d4a5 100644
@@ -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

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 <[email protected]>

Cc Valentin Longchamp <[email protected]>

Cc Holger Brunck <[email protected]>

---
?common/cmd_sf.c | ? 15 ++++++++++++++-
?1 files changed, 14 insertions(+), 1 deletions(-)

common/cmd_sf.c +3 -1

index d97d4a5..0666f52 100644
@@ -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