Re: [flashrom] [PATCH 1/1] Add support for reading the current flash contents from a file
Le mercredi 20 avril 2016 à 16:54 -0700, David Hendricks a écrit :
Looks pretty good to me. FWIW, we implemented something nearly identical in the chromiumos branch a few years ago: https://chromium-review.googlesource.co m/#/c/10180/ (we called it "--diff"). It's been quite useful for development, especially when working with large chips and slow external programmers.
This has been my use case as well, in particular with the kb9012 embedded controller flash. Nice to see that this use case is shared with others and that we came up with a quite similar implementation independently!
On Tue, Apr 19, 2016 at 12:25 PM, Paul Kocialkowski contact@paulk.fr wrote:
Hi,
Could we get the review moving on this patch? I first sent it over 4 months ago and got no feedback. I'd really like to have this feature merged, as I'm using it very often!
Thanks
Le lundi 29 février 2016 à 19:44 +0100, Paul Kocialkowski a écrit :
When developing software that has to be flashed to a flash chip to be executed, it often takes a long time to read the current flash contents (for
flashrom to
know what pages to erase and reprogram) each time, when writing the new
image.
However, when the flash was just reprogrammed, its current state is known
to
be the previous image that was flashed (assuming it was verified).
Thus, it makes sense to provide that image as a file for the flash
contents
instead of wasting valuable time read the whole flash each time.
Signed-off-by: Paul Kocialkowski contact@paulk.fr
cli_classic.c | 46 ++++++++++++++++++++++++++++------------------ flash.h | 2 +- flashrom.c | 14 +++++++++++--- 3 files changed, 40 insertions(+), 22 deletions(-)
diff --git a/cli_classic.c b/cli_classic.c index a2c2014..dc0164d 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -44,24 +44,25 @@ static void cli_classic_usage(const char *name) "[-E|(-r|-w|-v) <file>] [-l <layoutfile> [-i <imagename>]...] [-n] [-f]]\n" "[-V[V[V]]] [-o <logfile>]\n\n", name); - printf(" -h | --help print this help text\n" - " -R | --version print version (release)\n" - " -r | --read <file> read flash and save to <file>\n" - " -w | --write <file> write <file> to flash\n" - " -v | --verify <file> verify flash against <file>\n" - " -E | --erase erase flash memory\n" - " -V | --verbose more verbose output\n" - " -c | --chip <chipname> probe only for specified flash chip\n" - " -f | --force force specific
operations
(see man page)\n" - " -n | --noverify don't auto-verify\n" - " -l | --layout <layoutfile> read ROM layout from <layoutfile>\n" - " -i | --image <name> only flash image <name> from flash layout\n" - " -o | --output <logfile> log output to <logfile>\n" - " -L | --list-supported print supported devices\n" + printf(" -h | --help print this help
text\n"
+ " -R | --version print version (release)\n" + " -r | --read <file> read flash and save to <file>\n" + " -w | --write <file> write <file> to flash\n" + " -v | --verify <file> verify flash against <file>\n" + " -E | --erase erase flash memory\n" + " -V | --verbose more verbose output\n" + " -c | --chip <chipname> probe only for specified flash chip\n" + " -f | --force force specific operations (see man page)\n" + " -n | --noverify don't auto-verify\n" + " -l | --layout <layoutfile> read ROM layout from <layoutfile>\n" + " -i | --image <name> only flash image
<name> > from flash layout\n" > + " -o | --output <logfile> log output to > <logfile>\n" > + " -C | --flash-contents <contentsfile> assume flash contents > to be <contentsfile>\n" > + " -L | --list-supported print supported > devices\n" > #if CONFIG_PRINT_WIKI == 1 > - " -z | --list-supported-wiki print supported devices > in wiki syntax\n" > + " -z | --list-supported-wiki print supported devices > in wiki syntax\n" > #endif > - " -p | --programmer <name>[:<param>] specify the programmer > device. One of\n"); > + " -p | --programmer <name>[:<param>] specify the programmer > device. One of\n"); > list_programmers_linebreak(4, 80, 0); > printf(".\n\nYou can specify one of -h, -R, -L, " > #if CONFIG_PRINT_WIKI == 1 > @@ -106,7 +107,7 @@ int main(int argc, char *argv[]) > enum programmer prog = PROGRAMMER_INVALID; > int ret = 0; > > - static const char optstring[] = "r:Rw:v:nVEfc:l:i:p:Lzho:"; > + static const char optstring[] = "r:Rw:v:nVEfc:l:i:C:p:Lzho:"; > static const struct option long_options[] = { > {"read", 1, NULL, 'r'}, > {"write", 1, NULL, 'w'}, > @@ -118,6 +119,7 @@ int main(int argc, char *argv[]) > {"force", 0, NULL, 'f'}, > {"layout", 1, NULL, 'l'}, > {"image", 1, NULL, 'i'}, > + {"flash-contents", 1, NULL, 'C'}, > {"list-supported", 0, NULL, 'L'}, > {"list-supported-wiki", 0, NULL, 'z'}, > {"programmer", 1, NULL, 'p'}, > @@ -128,6 +130,7 @@ int main(int argc, char *argv[]) > }; > > char *filename = NULL; > + char *contentsfile = NULL; > char *layoutfile = NULL; > #ifndef STANDALONE > char *logfile = NULL; > @@ -221,6 +224,9 @@ int main(int argc, char *argv[]) > cli_classic_abort_usage(); > } > break; > + case 'C': > + contentsfile = strdup(optarg); > + break; > case 'L': > if (++operation_specified > 1) { > fprintf(stderr, "More than one operation " > @@ -332,6 +338,9 @@ int main(int argc, char *argv[]) > if (layoutfile && check_filename(layoutfile, "layout")) { > cli_classic_abort_usage(); > } > + if (contentsfile && check_filename(contentsfile, "contents")) { > + cli_classic_abort_usage(); > + } > > #ifndef STANDALONE > if (logfile && check_filename(logfile, "log")) > @@ -542,7 +551,7 @@ int main(int argc, char *argv[]) > * Give the chip time to settle. > */ > programmer_delay(100000); > - ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, > verify_it); > + ret |= doit(fill_flash, force, filename, contentsfile, read_it, > write_it, erase_it, verify_it); > > unmap_flash(fill_flash); > out_shutdown: > @@ -554,6 +563,7 @@ out: > layout_cleanup(); > free(filename); > free(layoutfile); > + free(contentsfile); > free(pparam); > /* clean up global variables */ > free((char *)chip_to_probe); /* Silence! Freeing is not modifying > contents. */ > diff --git a/flash.h b/flash.h > index dc5c140..2cf7ca6 100644 > --- a/flash.h > +++ b/flash.h > @@ -284,7 +284,7 @@ void print_buildinfo(void); > void print_banner(void); > void list_programmers_linebreak(int startcol, int cols, int paren); > int selfcheck(void); > -int doit(struct flashctx *flash, int force, const char *filename, int > read_it, int write_it, int erase_it, int verify_it); > +int doit(struct flashctx *flash, int force, const char *filename, const char > *contentsfile, int read_it, int write_it, int erase_it, int verify_it); > int read_buf_from_file(unsigned char *buf, unsigned long size, const char > *filename); > int write_buf_to_file(const unsigned char *buf, unsigned long size, const > char *filename); > > diff --git a/flashrom.c b/flashrom.c > index d5c3238..d11f89d 100644 > --- a/flashrom.c > +++ b/flashrom.c > @@ -1978,8 +1978,9 @@ int chip_safety_check(const struct flashctx *flash, int > force, int read_it, int > * but right now it allows us to split off the CLI code. > * Besides that, the function itself is a textbook example of abysmal code > flow. > */ > -int doit(struct flashctx *flash, int force, const char *filename, int > read_it, > - int write_it, int erase_it, int verify_it) > +int doit(struct flashctx *flash, int force, const char *filename, > + const char *contentsfile, int read_it, int write_it, int erase_it, > + int verify_it) > { > uint8_t *oldcontents; > uint8_t *newcontents; > @@ -2068,7 +2069,14 @@ int doit(struct flashctx *flash, int force, const char > *filename, int read_it, > * preserved, but in that case we might perform unneeded erase which > * takes time as well. > */ > - if (read_all_first) { > + if (contentsfile) { > + msg_cinfo("Reading old flash chip contents from file... "); > + if (read_buf_from_file(oldcontents, size, contentsfile)) { > + ret = 1; > + msg_cinfo("FAILED.\n"); > + goto out; > + } > + } else if (read_all_first) { > msg_cinfo("Reading old flash chip contents... "); > if (flash->chip->read(flash, oldcontents, 0, size)) { > ret = 1;
flashrom mailing list flashrom@flashrom.org https://www.flashrom.org/mailman/listinfo/flashrom
-- David Hendricks (dhendrix) Systems Software Engineer, Google Inc.
-
Paul Kocialkowski