[U-Boot] [PATCH v3 2/2] cmd_sf: let "sf update" preserve the final part of the last sector

[Jagan Teki]

On Fri, Jul 5, 2013 at 1:01 PM, Gerlando Falauto
<gerlando.falauto at 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.falauto at 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 at keymile.com>
>>> Cc: Valentin Longchamp <valentin.longchamp at keymile.com>
>>> Cc: Holger Brunck <holger.brunck at keymile.com>
>>> Acked-by: Simon Glass <sjg at 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
>>>
>