* [U-Boot] [PATCH v2 0/2] SPI flash update command @ 2013-06-28 7:43 Gerlando Falauto 2013-06-28 7:43 ` [U-Boot] [PATCH v2 1/2] cmd_sf: let "sf update" erase last sector as a whole Gerlando Falauto 2013-06-28 7:43 ` [U-Boot] [PATCH v2 2/2] cmd_sf: "sf update" preserve the final part of the last sector Gerlando Falauto 0 siblings, 2 replies; 5+ messages in thread From: Gerlando Falauto @ 2013-06-28 7:43 UTC (permalink / raw) To: u-boot 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 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 | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) -- 1.7.10.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2 1/2] cmd_sf: let "sf update" erase last sector as a whole 2013-06-28 7:43 [U-Boot] [PATCH v2 0/2] SPI flash update command Gerlando Falauto @ 2013-06-28 7:43 ` Gerlando Falauto 2013-06-28 7:43 ` [U-Boot] [PATCH v2 2/2] cmd_sf: "sf update" preserve the final part of the last sector Gerlando Falauto 1 sibling, 0 replies; 5+ messages in thread From: Gerlando Falauto @ 2013-06-28 7:43 UTC (permalink / raw) To: u-boot 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 file changed, 2 insertions(+), 1 deletion(-) diff --git a/common/cmd_sf.c b/common/cmd_sf.c index 19b0dc9..ab35a94 100644 --- a/common/cmd_sf.c +++ b/common/cmd_sf.c @@ -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"; -- 1.7.10.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2 2/2] cmd_sf: "sf update" preserve the final part of the last sector 2013-06-28 7:43 [U-Boot] [PATCH v2 0/2] SPI flash update command Gerlando Falauto 2013-06-28 7:43 ` [U-Boot] [PATCH v2 1/2] cmd_sf: let "sf update" erase last sector as a whole Gerlando Falauto @ 2013-06-28 7:43 ` Gerlando Falauto 2013-07-03 15:04 ` Jagan Teki 1 sibling, 1 reply; 5+ messages in thread From: Gerlando Falauto @ 2013-06-28 7:43 UTC (permalink / raw) To: u-boot 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 file changed, 14 insertions(+), 1 deletion(-) diff --git a/common/cmd_sf.c b/common/cmd_sf.c index ab35a94..fb87d24 100644 --- a/common/cmd_sf.c +++ b/common/cmd_sf.c @@ -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 meningful (len) */ if (memcmp(cmp_buf, buf, len) == 0) { debug("Skip region %x size %zx: no change\n", offset, len); @@ -163,6 +165,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.10.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2 2/2] cmd_sf: "sf update" preserve the final part of the last sector 2013-06-28 7:43 ` [U-Boot] [PATCH v2 2/2] cmd_sf: "sf update" preserve the final part of the last sector Gerlando Falauto @ 2013-07-03 15:04 ` Jagan Teki 2013-07-03 15:35 ` Gerlando Falauto 0 siblings, 1 reply; 5+ messages in thread From: Jagan Teki @ 2013-07-03 15:04 UTC (permalink / raw) To: u-boot Hi, Thanks for the v2. On Fri, Jun 28, 2013 at 1:13 PM, 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 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/common/cmd_sf.c b/common/cmd_sf.c > index ab35a94..fb87d24 100644 > --- a/common/cmd_sf.c > +++ b/common/cmd_sf.c > @@ -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 meningful (len) */ > if (memcmp(cmp_buf, buf, len) == 0) { > debug("Skip region %x size %zx: no change\n", > offset, len); > @@ -163,6 +165,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); Can comment about memcpy() here, as Simon is also asked in v1 http://patchwork.ozlabs.org/patch/150468/ -- Thanks, Jagan. > + /* 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.10.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2 2/2] cmd_sf: "sf update" preserve the final part of the last sector 2013-07-03 15:04 ` Jagan Teki @ 2013-07-03 15:35 ` Gerlando Falauto 0 siblings, 0 replies; 5+ messages in thread From: Gerlando Falauto @ 2013-07-03 15:35 UTC (permalink / raw) To: u-boot Hi, On 07/03/2013 05:04 PM, Jagan Teki wrote: > Hi, > > Thanks for the v2. > > On Fri, Jun 28, 2013 at 1:13 PM, 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 file changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/common/cmd_sf.c b/common/cmd_sf.c >> index ab35a94..fb87d24 100644 >> --- a/common/cmd_sf.c >> +++ b/common/cmd_sf.c >> @@ -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 meningful (len) */ >> if (memcmp(cmp_buf, buf, len) == 0) { >> debug("Skip region %x size %zx: no change\n", >> offset, len); >> @@ -163,6 +165,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); > > Can comment about memcpy() here, as Simon is also asked in v1 > http://patchwork.ozlabs.org/patch/150468/ Whoops, sorry! I most probably did not see the comment (stopped reading at the Acked-By line) the first time around and *DEFINITELY* did not see it before posting v2. I now understand your point, Simon. This would imply two spi_flash_write() operations as opposed to just one, yet allowing for cleaner code. I guess I was originally trying to both preserve the original behavior (always write whole sectors) and save on the number of write operations. But that does not make much sense as we're speaking of an SPI NOR flash here, the only penalty from an extra write operation only comes from the extra SPI command. Not really much. I'll post a v3 soon. My apologies again. Thanks, Gerlando > > -- > Thanks, > Jagan. > >> + /* 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.10.1 >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-07-03 15:35 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-06-28 7:43 [U-Boot] [PATCH v2 0/2] SPI flash update command Gerlando Falauto 2013-06-28 7:43 ` [U-Boot] [PATCH v2 1/2] cmd_sf: let "sf update" erase last sector as a whole Gerlando Falauto 2013-06-28 7:43 ` [U-Boot] [PATCH v2 2/2] cmd_sf: "sf update" preserve the final part of the last sector Gerlando Falauto 2013-07-03 15:04 ` Jagan Teki 2013-07-03 15:35 ` Gerlando Falauto
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.