[U-Boot] [PATCH v4] cmd_sf: add "update" subcommand to do smart SPI flash update

[Simon Glass]

Hi Marek,

On Sat, Aug 20, 2011 at 5:04 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
> On Sunday, August 21, 2011 12:35:51 AM Mike Frysinger wrote:
>> From: Simon Glass <sjg at chromium.org>
>>
>> This adds a new SPI flash command which only rewrites blocks if the
>> contents need to change. This can speed up SPI flash programming when much
>> of the data is unchanged from what is already there.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> Signed-off-by: Mike Frysinger <vapier at gentoo.org>
>> ---
>> v4
>>       - tweak summary
>>       - fix printf warnings with %d vs %zu
>>       - fix help string and missing/extra newlines
>>
>> TODO: it'd be nice if we supported +len like we do with erase ...
>>
>>  common/cmd_sf.c |   84
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed,
>> 81 insertions(+), 3 deletions(-)
>>
>> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
>> index 11a491d..9b7d61b 100644
>> --- a/common/cmd_sf.c
>> +++ b/common/cmd_sf.c
>> @@ -6,6 +6,7 @@
>>   */
>>
>>  #include <common.h>
>> +#include <malloc.h>
>>  #include <spi_flash.h>
>>
>>  #include <asm/io.h>
>> @@ -109,6 +110,78 @@ static int do_spi_flash_probe(int argc, char * const
>> argv[]) return 0;
>>  }
>>
>> +/**
>> + * Write a block of data to SPI flash, first checking if it is different
>> from + * what is already there.
>> + *
>> + * If the data being written is the same, then *skipped is incremented by
>> len. + *
>> + * @param flash              flash context pointer
>> + * @param offset     flash offset to write
>> + * @param len                number of bytes to write
>> + * @param buf                buffer to write from
>> + * @param cmp_buf    read buffer to use to compare data
>> + * @param skipped    Count of skipped data (incremented by this function)
>> + * @return NULL if OK, else a string containing the stage which failed
>> + */
>> +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)
>
> Can't you just pass here a structure instead of this wicked pointer alchemy ?

Do you mean create a structure with the things that don't change in it
(flash, cmp_buf and skipped)? Is the problem too many parameters?

>
>> +{
>> +     debug("offset=%#x, sector_size=%#x, len=%#x\n",
>> +             offset, flash->sector_size, len);
>> +     if (spi_flash_read(flash, offset, len, cmp_buf))
>> +             return "read";
>> +     if (memcmp(cmp_buf, buf, len) == 0) {
>> +             debug("Skip region %x size %x: no change\n",
>> +                     offset, len);
>> +             *skipped += len;
>> +             return NULL;
>> +     }
>> +     if (spi_flash_erase(flash, offset, len))
>> +             return "erase";
>> +     if (spi_flash_write(flash, offset, len, buf))
>> +             return "write";
>
> Numeric value won't be ok ? You can have these in the calling function instead
> of returning a char *.

Yes it's a bit odd, but the alternative is quite a bit more verbose:

enum {
   OPER_MALLOC,
   OPER_READ,
   OPER_ERASE,
   ...
};

static const char *names[OPER...] = {
   "malloc",
   "read",
   "erase"
...
};

Is that better?

>
>> +     return NULL;
>> +}
>> +
>> +/**
>> + * Update an area of SPI flash by erasing and writing any blocks which
>> need + * to change. Existing blocks with the correct data are left
>> unchanged. + *
>> + * @param flash              flash context pointer
>> + * @param offset     flash offset to write
>> + * @param len                number of bytes to write
>> + * @param buf                buffer to write from
>> + * @return 0 if ok, 1 on error
>> + */
>> +static int spi_flash_update(struct spi_flash *flash, u32 offset,
>> +             size_t len, const char *buf)
>> +{
>> +     const char *err_oper = NULL;
>> +     char *cmp_buf;
>> +     const char *end = buf + len;
>> +     size_t todo;            /* number of bytes to do in this pass */
>> +     size_t skipped;         /* statistics */
>
> You can allocate a structure holding the internal state of the "update" command,
> which I mentioned above, here, on stack.

Please see question above.

>
>> +
>> +     cmp_buf = malloc(flash->sector_size);
>> +     if (cmp_buf) {
>
> if (!cmp_buf)
>        goto err;
>
> ... rest of code ...
>
> Don't be afraid of goto and failpaths.

OK, will try.

>
> I like this patch!

Thanks!

Regards,
Simon

>
> Cheers
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>