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

[Michael Walle]

Am 2021-09-30 18:19, schrieb Frieder Schrempf:
> From: Frieder Schrempf <frieder.schrempf at 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 at 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.

> +				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".

> +			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