| Message ID | 1313879751-1093-1-git-send-email-vapier@gentoo.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Sunday, August 21, 2011 12:35:51 AM Mike Frysinger wrote: > From: Simon Glass <sjg@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@chromium.org> > Signed-off-by: Mike Frysinger <vapier@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 ? > +{ > + 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 *. > + 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. > + > + 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. > + for (skipped = 0; buf < end; buf += todo, offset += todo) { > + todo = min(end - buf, flash->sector_size); > + err_oper = spi_flash_update_block(flash, offset, todo, > + buf, cmp_buf, &skipped); > + } > + } else { > + err_oper = "malloc"; > + } > + free(cmp_buf); > + if (err_oper) { > + printf("SPI flash failed in %s step\n", err_oper); > + return 1; > + } > + printf("%zu bytes written, %zu bytes skipped\n", len - skipped, skipped); > + return 0; > +} > + > static int do_spi_flash_read_write(int argc, char * const argv[]) > { > unsigned long addr; > @@ -137,7 +210,9 @@ static int do_spi_flash_read_write(int argc, char * > const argv[]) return 1; > } > > - if (strcmp(argv[0], "read") == 0) > + if (strcmp(argv[0], "update") == 0) > + ret = spi_flash_update(flash, offset, len, buf); > + else if (strcmp(argv[0], "read") == 0) > ret = spi_flash_read(flash, offset, len, buf); > else > ret = spi_flash_write(flash, offset, len, buf); > @@ -203,7 +278,8 @@ static int do_spi_flash(cmd_tbl_t *cmdtp, int flag, int > argc, char * const argv[ return 1; > } > > - if (strcmp(cmd, "read") == 0 || strcmp(cmd, "write") == 0) > + if (strcmp(cmd, "read") == 0 || strcmp(cmd, "write") == 0 || > + strcmp(cmd, "update") == 0) > ret = do_spi_flash_read_write(argc, argv); > else if (strcmp(cmd, "erase") == 0) > ret = do_spi_flash_erase(argc, argv); > @@ -228,5 +304,7 @@ U_BOOT_CMD( > "sf write addr offset len - write `len' bytes from memory\n" > " at `addr' to flash at `offset'\n" > "sf erase offset [+]len - erase `len' bytes from `offset'\n" > - " `+len' round up `len' to block size" > + " `+len' round up `len' to block size\n" > + "sf update addr offset len - erase and write `len' bytes from memory\n" > + " at `addr' to flash at `offset'" > ); I like this patch! Cheers
On Sat, Aug 20, 2011 at 4:35 PM, Mike Frysinger <vapier@gentoo.org> wrote: > From: Simon Glass <sjg@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@chromium.org> > Signed-off-by: Mike Frysinger <vapier@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 ... ... > +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 */ > + > + cmp_buf = malloc(flash->sector_size); > + if (cmp_buf) { > + for (skipped = 0; buf < end; buf += todo, offset += todo) { Oops I got this wrong: for (skipped = 0; buf < end && !err_oper; buf += todo, offset += todo) { (or if (err_oper) break in the loop) > + todo = min(end - buf, flash->sector_size); > + err_oper = spi_flash_update_block(flash, offset, todo, > + buf, cmp_buf, &skipped); > + } > + } else { > + err_oper = "malloc"; > + } > + free(cmp_buf); > + if (err_oper) { > + printf("SPI flash failed in %s step\n", err_oper); > + return 1; > + } > + printf("%zu bytes written, %zu bytes skipped\n", len - skipped, skipped); > + return 0; > +} > + > static int do_spi_flash_read_write(int argc, char * const argv[]) > { > unsigned long addr; > @@ -137,7 +210,9 @@ static int do_spi_flash_read_write(int argc, char * const argv[]) > return 1; > } > > - if (strcmp(argv[0], "read") == 0) > + if (strcmp(argv[0], "update") == 0) > + ret = spi_flash_update(flash, offset, len, buf); > + else if (strcmp(argv[0], "read") == 0) > ret = spi_flash_read(flash, offset, len, buf); > else > ret = spi_flash_write(flash, offset, len, buf); > @@ -203,7 +278,8 @@ static int do_spi_flash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ > return 1; > } > > - if (strcmp(cmd, "read") == 0 || strcmp(cmd, "write") == 0) > + if (strcmp(cmd, "read") == 0 || strcmp(cmd, "write") == 0 || > + strcmp(cmd, "update") == 0) > ret = do_spi_flash_read_write(argc, argv); > else if (strcmp(cmd, "erase") == 0) > ret = do_spi_flash_erase(argc, argv); > @@ -228,5 +304,7 @@ U_BOOT_CMD( > "sf write addr offset len - write `len' bytes from memory\n" > " at `addr' to flash at `offset'\n" > "sf erase offset [+]len - erase `len' bytes from `offset'\n" > - " `+len' round up `len' to block size" > + " `+len' round up `len' to block size\n" > + "sf update addr offset len - erase and write `len' bytes from memory\n" > + " at `addr' to flash at `offset'" > ); > -- > 1.7.6 > >
Hi Marek, On Sat, Aug 20, 2011 at 5:04 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > On Sunday, August 21, 2011 12:35:51 AM Mike Frysinger wrote: >> From: Simon Glass <sjg@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@chromium.org> >> Signed-off-by: Mike Frysinger <vapier@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@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
On Sunday, August 21, 2011 06:27:16 Simon Glass wrote: > On Sat, Aug 20, 2011 at 4:35 PM, Mike Frysinger wrote: > > +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 */ > > + > > + cmp_buf = malloc(flash->sector_size); > > + if (cmp_buf) { > > + for (skipped = 0; buf < end; buf += todo, offset += todo) > > { > > Oops I got this wrong: > > for (skipped = 0; buf < end && !err_oper; buf += todo, offset += todo) { > > (or if (err_oper) break in the loop) you can send a v5 and i'll just replace it in my tree :) or a diff and i'll just merge it locally ... -mike
On Sunday, August 21, 2011 06:37:30 Simon Glass wrote: > On Sat, Aug 20, 2011 at 5:04 PM, Marek Vasut wrote: > > On Sunday, August 21, 2011 12:35:51 AM Mike Frysinger wrote: > >> +{ > >> + 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...] = { static const char * const names[] = { > "malloc", > "read", > "erase" > ... > }; > > Is that better? only if the code size is smaller ;) -mike
On Sun, Aug 21, 2011 at 9:35 AM, Mike Frysinger <vapier@gentoo.org> wrote: > On Sunday, August 21, 2011 06:37:30 Simon Glass wrote: >> On Sat, Aug 20, 2011 at 5:04 PM, Marek Vasut wrote: >> > On Sunday, August 21, 2011 12:35:51 AM Mike Frysinger wrote: >> >> +{ >> >> + 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...] = { > > static const char * const names[] = { > >> "malloc", >> "read", >> "erase" >> ... >> }; >> >> Is that better? > > only if the code size is smaller ;) Hi Mike, Well it is 4 bytes larger, or 8 if I avoid char * for the array and fix the string length. So I will leave that as is for now, and send a new v5 patch. Regards, Simon > -mike >
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) +{ + 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"; + 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 */ + + cmp_buf = malloc(flash->sector_size); + if (cmp_buf) { + for (skipped = 0; buf < end; buf += todo, offset += todo) { + todo = min(end - buf, flash->sector_size); + err_oper = spi_flash_update_block(flash, offset, todo, + buf, cmp_buf, &skipped); + } + } else { + err_oper = "malloc"; + } + free(cmp_buf); + if (err_oper) { + printf("SPI flash failed in %s step\n", err_oper); + return 1; + } + printf("%zu bytes written, %zu bytes skipped\n", len - skipped, skipped); + return 0; +} + static int do_spi_flash_read_write(int argc, char * const argv[]) { unsigned long addr; @@ -137,7 +210,9 @@ static int do_spi_flash_read_write(int argc, char * const argv[]) return 1; } - if (strcmp(argv[0], "read") == 0) + if (strcmp(argv[0], "update") == 0) + ret = spi_flash_update(flash, offset, len, buf); + else if (strcmp(argv[0], "read") == 0) ret = spi_flash_read(flash, offset, len, buf); else ret = spi_flash_write(flash, offset, len, buf); @@ -203,7 +278,8 @@ static int do_spi_flash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ return 1; } - if (strcmp(cmd, "read") == 0 || strcmp(cmd, "write") == 0) + if (strcmp(cmd, "read") == 0 || strcmp(cmd, "write") == 0 || + strcmp(cmd, "update") == 0) ret = do_spi_flash_read_write(argc, argv); else if (strcmp(cmd, "erase") == 0) ret = do_spi_flash_erase(argc, argv); @@ -228,5 +304,7 @@ U_BOOT_CMD( "sf write addr offset len - write `len' bytes from memory\n" " at `addr' to flash at `offset'\n" "sf erase offset [+]len - erase `len' bytes from `offset'\n" - " `+len' round up `len' to block size" + " `+len' round up `len' to block size\n" + "sf update addr offset len - erase and write `len' bytes from memory\n" + " at `addr' to flash at `offset'" );