Re: [PATCH] cmd: sf: Support unaligned flash updates with 'sf update'

[Frieder Schrempf <frieder.schrempf@kontron.de>]

On 30.09.21 23:21, Michael Walle wrote:
> Am 2021-09-30 18:19, schrieb Frieder Schrempf:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> Currently 'sf update' supports only offsets that are aligned to the
>> erase block size of the serial flash. Unaligned offsets result in
>> something like:
>>
>> => sf update ${kernel_addr_r} 0x400 ${filesize}
>> device 0 offset 0x400, size 0x11f758
>> SPI flash failed in erase step
>>
>> In order to support unaligned updates, we simply read the first full
>> block and check only the requested part of the block for changes. If
>> necessary, the block is erased, the first (unchanged) part of the block
>> is written back together with the second part of the block (updated
>> data).
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>>  cmd/sf.c | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/cmd/sf.c b/cmd/sf.c
>> index eac27ed2d7..c54b4b10bb 100644
>> --- a/cmd/sf.c
>> +++ b/cmd/sf.c
>> @@ -171,30 +171,32 @@ static int do_spi_flash_probe(int argc, char
>> *const argv[])
>>  static const char *spi_flash_update_block(struct spi_flash *flash,
>> u32 offset,
>>          size_t len, const char *buf, char *cmp_buf, size_t *skipped)
>>  {
>> +    u32 offset_aligned = ALIGN_DOWN(offset, flash->sector_size);
>> +    u32 sector_offset = offset - offset_aligned;
>>      char *ptr = (char *)buf;
>>
>>      debug("offset=%#x, sector_size=%#x, len=%#zx\n",
>>            offset, flash->sector_size, len);
>>      /* Read the entire sector so to allow for rewriting */
>> -    if (spi_flash_read(flash, offset, flash->sector_size, cmp_buf))
>> +    if (spi_flash_read(flash, offset_aligned, flash->sector_size,
>> cmp_buf))
>>          return "read";
>>      /* Compare only what is meaningful (len) */
>> -    if (memcmp(cmp_buf, buf, len) == 0) {
>> +    if (memcmp(cmp_buf + sector_offset, buf, len) == 0) {
>>          debug("Skip region %x size %zx: no change\n",
>>                offset, len);
>>          *skipped += len;
>>          return NULL;
>>      }
>>      /* Erase the entire sector */
>> -    if (spi_flash_erase(flash, offset, flash->sector_size))
>> +    if (spi_flash_erase(flash, offset_aligned, flash->sector_size))
>>          return "erase";
>>      /* If it's a partial sector, copy the data into the temp-buffer */
>>      if (len != flash->sector_size) {
>> -        memcpy(cmp_buf, buf, len);
>> +        memcpy(cmp_buf + sector_offset, buf, len);
>>          ptr = cmp_buf;
>>      }
>>      /* Write one complete sector */
>> -    if (spi_flash_write(flash, offset, flash->sector_size, ptr))
>> +    if (spi_flash_write(flash, offset_aligned, flash->sector_size, ptr))
>>          return "write";
>>
>>      return NULL;
>> @@ -230,7 +232,10 @@ static int spi_flash_update(struct spi_flash
>> *flash, u32 offset,
>>          ulong last_update = get_timer(0);
>>
>>          for (; buf < end && !err_oper; buf += todo, offset += todo) {
>> -            todo = min_t(size_t, end - buf, flash->sector_size);
>> +            if (offset % flash->sector_size)
> 
> do_div() to avoid linking errors on some archs, I guess.

Ok, will fix it.

> 
>> +                todo = flash->sector_size - (offset %
>> flash->sector_size);
> 
> This is missing the end-buf calculation, no? I.e. if you update just one
> sector at an offset and the data you write is smaller than "sector_size
> - offset".

Indeed, thanks for catching this.

> 
>> +            else
>> +                todo = min_t(size_t, end - buf, flash->sector_size);
>>              if (get_timer(last_update) > 100) {
>>                  printf("   \rUpdating, %zu%% %lu B/s",
>>                         100 - (end - buf) / scale,
> 
> -michael