On Fri, Jul 5, 2013 at 1:01 PM, Gerlando Falauto <gerlando.fala...@keymile.com> wrote: > Hi Jagan, > > > On 07/04/2013 06:26 PM, Jagan Teki wrote: >> >> On Thu, Jul 4, 2013 at 12:03 AM, Gerlando Falauto >> <gerlando.fala...@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.fala...@keymile.com> >>> Cc: Valentin Longchamp <valentin.longch...@keymile.com> >>> Cc: Holger Brunck <holger.bru...@keymile.com> >>> Acked-by: Simon Glass <s...@chromium.org> >>> --- >>> common/cmd_sf.c | 12 +++++++++++- >>> 1 file changed, 11 insertions(+), 1 deletion(-) >>> >>> diff --git a/common/cmd_sf.c b/common/cmd_sf.c >>> index ab35a94..1141dc1 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 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"; >> >> >> 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. > > > 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.
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. -- Thanks, Jagan. > > Hope this answers your question. > > Thank you, > Gerlando > >> >> -- >> Thanks, >> Jagan. >>> >>> + /* 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 >>> > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot