[U-Boot] [PATCH] Add 'sf update' command to do smart SPI flash update

[Simon Glass]

Hi Mike,

On Fri, Aug 19, 2011 at 9:14 AM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Friday, August 19, 2011 09:47:04 Simon Glass wrote:
>> 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.
>
> sounds good
>
>> +static int spi_flash_update(struct spi_flash *flash, u32 offset,
>> +             size_t len, const char *buf)
>> +{
>> +     const char *oper;
>
> i wonder if there'd be any code size difference if we had:
>        static const char * const opers[] = {
>                "malloc",
>                "read",
>                ...
>        };
>        const char *oper;
>        ...
>                oper = 0;
>        ...
>                ++oper;
>        ...
>                ++oper;
>        ...
>        printf("...%s...", ..., opers[oper]);
>
> i hope it'd be slightly smaller as you would be loading up a small int in the
> for loop and not an entire pointer ...

It's exactly the same code/data size on ARM, but perhaps smaller on a
different architecture. I like that approach anyway so will change it.

>
>> +     int err = 0;
>> +     char *cmp_buf = NULL;
>> +     size_t cmp_size = 0;
>> +     size_t todo;    /* number of bytes to do in this pass */
>> +     size_t skipped, written;                /* statistics */
>> +
>> +     for (skipped = written = 0; !err && len > 0;
>> +                     buf += todo, offset += todo, len -= todo) {
>> +             todo = min(len, flash->sector_size);
>> +             debug("offset=%#x, sector_size=%#x, todo=%#x\n",
>> +                   offset, flash->sector_size, todo);
>> +             if (!err) {
>
> why do you need to check err here ?  you already have "!err" in the for loop
> constraint, err starts off at 0, and there is no setting of err between the
> top of the for loop and this point.

It's a hangover from previous code - will remove it.

>
>> +             if (!err) {
>> +                     oper = "read";
>> +                     err = spi_flash_read(flash, offset, todo, cmp_buf);
>> +             }
>
> all this error checking makes my head spin.  i wonder if it'd look any better
> using the (more normal?) style:
> err = ...;
> if (err)
>        break;
>
> err = ...;
> if (err)
>        break;
>
> you could then also drop "!err" from the for loop constraint, and the initial
> "err = 0".  not that big of a deal if the code size is the same ...

Ick. I will send a new patch with the code in a function and see what
you think about that. It is 0x88 bytes smaller on ARM than this patch.

>
>> +             if (!err) {
>> +                     if (0 == memcmp(cmp_buf, buf, todo)) {
>
> please reverse the sides of this compare.  this is unusual style for the
> linux/u-boot projects.

ok

>
>> +     if (cmp_buf)
>> +             free(cmp_buf);
>
> the if() isn't necessary ... free(NULL) works fine and would add extra
> overhead only in the (uncommon) failure case.  same goes for the malloc()
> logic earlier in this func.

OK.

>
>> +     "sf update addr offset len      - erase and write `len' bytes from "
>> +             "memory\n"
>
> please don't split the string constant on non-\n boundaries

ok, will go over 80cols.

> -mike
>