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
