[U-Boot] SPI flash writing
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
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
- 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.
- 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.
- 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
On Tue, Mar 13, 2012 at 04:11:29PM -0400, Mike Frysinger wrote:
On Tuesday 13 March 2012 14:25:07 Gerlando Falauto wrote:
- 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.
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:
- 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
-----Original Message----- From: Mike Frysinger [mailto:vapier@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:
- 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
On Tuesday 13 March 2012 17:31:02 Falauto, Gerlando wrote:
From: Mike Frysinger [mailto:vapier@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:
- 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
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@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:
- 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
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
On Tue, Mar 13, 2012 at 10:31:02PM +0100, Falauto, Gerlando wrote:
-----Original Message----- From: Mike Frysinger [mailto:vapier@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:
- 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).
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
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:
- 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.
- 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.
- 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
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:
- 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.
- 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.
- 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
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]);
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
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]);
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(-)
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
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";
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
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;
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
participants (7)
-
Falauto, Gerlando -
Gerlando Falauto
-
Jason Cooper -
Mike Frysinger -
Simon Glass -
Tom Rini -
Valentin Longchamp