On 30.09.21 23:21, Michael Walle wrote:
> Am 2021-09-30 18:19, schrieb Frieder Schrempf:
>> From: Frieder Schrempf <frieder.schre...@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.schre...@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
