SPI flash update command
v3 cover SPI flash update command
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.
Changes from v2:
- Perform two write operations but have cleaner code
Changes from v1 [April, 03, 2012 (!)]:
- Rebased on top of u-boot-spi/master
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 | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
Signature · 1 line
1.8.0.1
PATCH v3 1/2 cmd_sf: let "sf update" erase last sector as a whole
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}
--- common/cmd_sf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
common/cmd_sf.c +2 -1
@@ -160,7 +160,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" ;
Signature · 1 line
1.8.0.1
PATCH v3 1/2 cmd_sf: let "sf update" erase last sector as a whole
Quoted 19 lines · 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 <[email protected]>Cc Valentin Longchamp <[email protected]>Cc Holger Brunck <[email protected]>Acked-by Simon Glass <[email protected]>--- common/cmd_sf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)common/cmd_sf.c +0
-0
common/cmd_sf.c static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset, · line 167
Quoted 9 lines
common/cmd_sf.c +2
-1@@ -160,7 +160,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" ;
--
Thanks,
Jagan.
PATCH v3 1/2 cmd_sf: let "sf update" erase last sector as a whole
Quoted 32 lines · Jagan Teki
Quoted 27 lines · 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 <[email protected]>Cc Valentin Longchamp <[email protected]>Cc Holger Brunck <[email protected]>Acked-by Simon Glass <[email protected]>--- common/cmd_sf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)common/cmd_sf.c +2
-1@@ -160,7 +160,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))Quoted 2 lines
return "write";Reviewed-by Jagannadha Sutradharudu Teki <[email protected]>
Quoted 3 lines
--
Thanks,
Jagan.
Applied to u-boot-spi/master
--
Thanks,
Jagan.
PATCH v3 2/2 cmd_sf: let "sf update" preserve the final part of the last sector
In reply to Gerlando FalautoSince "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.
--- common/cmd_sf.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
common/cmd_sf.c +11 -1
@@ -152,8 +152,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 meaningful (len) */ if (memcmp (cmp_buf, buf, len) ==0 ) {debug ("Skip region %x size %zx: no change\n" , offset, len);
@@ -163,8 +165,16 @@ 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" ; +/* Write the initial part of the block from the source */ if (spi_flash_write (flash, offset, len, buf))return "write" ; +/* If it's a partial sector, rewrite the existing part */ +if (len != flash->sector_size) { +/* Rewrite the original data to the end of the sector */ +if (spi_flash_write (flash, offset + len, + flash->sector_size - len, &cmp_buf[len])) +return "write" ; + }return NULL ; }
Signature · 1 line
1.8.0.1
PATCH v3 2/2 cmd_sf: let "sf update" preserve the final part of the last sector
Quoted 30 lines · 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 <[email protected]>Cc Valentin Longchamp <[email protected]>Cc Holger Brunck <[email protected]>Acked-by Simon Glass <[email protected]>--- common/cmd_sf.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)common/cmd_sf.c +3
-1@@ -152,8 +152,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 meaningful (len) */ if (memcmp (cmp_buf, buf, len) ==0 ) {debug ("Skip region %x size %zx: no change\n" , offset, len);
common/cmd_sf.c static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset, · line 170
Quoted 6 lines
common/cmd_sf.c +1
-0@@ -163,8 +165,16 @@ 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" ; +/* Write the initial part of the block from the source */ if (spi_flash_write (flash, offset, len, buf))return "write" ;
I din't understand why the below write is required again- As erase ops requires only sector operation and read + write will do the operations on partial sizes
Can you send the failure case w/o this.
--
Thanks,
Jagan.
Quoted 13 lines
common/cmd_sf.c +7
-1+/* If it's a partial sector, rewrite the existing part */ +if (len != flash->sector_size) { +/* Rewrite the original data to the end of the sector */ +if (spi_flash_write (flash, offset + len, + flash->sector_size - len, &cmp_buf[len])) +return "write" ; + }return NULL ; } --1.8.0.1
PATCH v3 2/2 cmd_sf: let "sf update" preserve the final part of the last sector
Hi Jagan,
Quoted 43 lines · Jagan Teki
Quoted 30 lines · 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 <[email protected]>Cc Valentin Longchamp <[email protected]>Cc Holger Brunck <[email protected]>Acked-by Simon Glass <[email protected]>--- common/cmd_sf.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)common/cmd_sf.c +3
-1@@ -152,8 +152,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 meaningful (len) */ if (memcmp (cmp_buf, buf, len) ==0 ) {debug ("Skip region %x size %zx: no change\n" , offset, len);common/cmd_sf.c static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset, · line 170
Quoted 6 lines
common/cmd_sf.c +1
-0@@ -163,8 +165,16 @@ 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" ; +/* Write the initial part of the block from the source */ if (spi_flash_write (flash, offset, len, buf))return "write" ;I din't understand why the below write is required again- As erase ops requires only sector operation and read + write will do the operations on partial sizes
Quoted 1 line
Can you send the failure case w/o this.
I'm not sure I understand your question.
I thought the commit message above was clear enough.
In any case, the idea is to have "sf update" be as agnostic as possible wrt to sector size, so it virtually only erases the same amount of data as it is going to overwrite. The rest will be preserved.
This way you could, for instance, store some binary proprietary firmware towards the end of the space reserved for u-boot, without having to reserve a whole flash sector for it. The reason for doing such a thing (as opposed to just embedding it within u-boot itself) is licensing issues, so you might want to keep the firmware as close as possible to the u-boot binary yet not link it.
Then when you update u-boot (GPL), your firmware is preserved.
Another extreme use case that comes to my mind would be where you have the u-boot environment within the same sector where u-boot lies, though
a) I'm not sure it's even possible
b) is of course a BAD, BAD idea
c) See b)
d) See c) and then b), plus is a BAD idea and therefore discouraged
e) it would only make sense if the u-boot environment is never meant to be altered except during production
To be honest with you, I don't remember if there was a real use case leading me to write this or if it was just all hypothetical or I just thought it was nicer that way.
As for changes of v3 wrt v2, the two should be functionally equivalent:
- In v2 I used a memcpy() to write the whole sector at once
- In v3 I avoided the memcpy() but this requires writing the two portions separately.
Hope this answers your question.
Thank you,
Gerlando
Quoted 17 lines
--
Thanks,
Jagan.Quoted 13 lines
common/cmd_sf.c +7
-1+/* If it's a partial sector, rewrite the existing part */ +if (len != flash->sector_size) { +/* Rewrite the original data to the end of the sector */ +if (spi_flash_write (flash, offset + len, + flash->sector_size - len, &cmp_buf[len])) +return "write" ; + }return NULL ; } --1.8.0.1
PATCH v3 2/2 cmd_sf: let "sf update" preserve the final part of the last sector
Quoted 79 lines · Gerlando Falauto
Hi Jagan,
Quoted 48 lines · Jagan Teki
Quoted 32 lines · 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 <[email protected]>Cc Valentin Longchamp <[email protected]>Cc Holger Brunck <[email protected]>Acked-by Simon Glass <[email protected]>--- common/cmd_sf.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)common/cmd_sf.c +3
-1@@ -152,8 +152,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 meaningful (len) */ if (memcmp (cmp_buf, buf, len) ==0 ) { debug("Skip region %x size %zx: no change\n",offset, len);common/cmd_sf.c static const char *spi_flash_update_block(struct · line 171
Quoted 7 lines
common/cmd_sf.c +1
-0@@ -163,8 +165,16 @@ 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"; + /* Write the initial part of the block from the source */ if (spi_flash_write(flash, offset, len, buf)) return "write";I din't understand why the below write is required again- As erase ops requires only sector operation and read + write will do the operations on partial sizes
Quoted 1 line
Can you send the failure case w/o this.
I'm not sure I understand your question.
I thought the commit message above was clear enough.In any case, the idea is to have "sf update" be as agnostic as possible wrt to sector size, so it virtually only erases the same amount of data as it is going to overwrite. The rest will be preserved.
This way you could, for instance, store some binary proprietary firmware towards the end of the space reserved for u-boot, without having to reserve a whole flash sector for it. The reason for doing such a thing (as opposed to just embedding it within u-boot itself) is licensing issues, so you might want to keep the firmware as close as possible to the u-boot binary yet not link it.
Then when you update u-boot (GPL), your firmware is preserved.Another extreme use case that comes to my mind would be where you have the u-boot environment within the same sector where u-boot lies, though
a) I'm not sure it's even possible
b) is of course a BAD, BAD idea
c) See b)
d) See c) and then b), plus is a BAD idea and therefore discouraged
e) it would only make sense if the u-boot environment is never meant to be altered except during production
Quoted 8 lines
To be honest with you, I don't remember if there was a real use case leading me to write this or if it was just all hypothetical or I just thought it was nicer that way.
As for changes of v3 wrt v2, the two should be functionally equivalent:
- In v2 I used amemcpy()to write the whole sector at once
- In v3 I avoided thememcpy()but this requires writing the two portions separately.
What if don't use second write, is that a condition where we are not sure the write gonna be happen
properly in case of partial sectors.
Signature · 2 lines
Thanks, Jagan.
Quoted 25 lines
Hope this answers your question.
Thank you,
GerlandoQuoted 18 lines
--
Thanks,
Jagan.Quoted 14 lines
common/cmd_sf.c +7
-1+/* If it's a partial sector, rewrite the existing part */ +if (len != flash->sector_size) { +/* Rewrite the original data to the end of the sector */ +if (spi_flash_write (flash, offset + len, + flash->sector_size - len, &cmp_buf[len])) +return "write" ; + }return NULL ; } --1.8.0.1