[flashrom] [PATCH 1/1] Add support for reading the current flash contents from a file
David Hendricks
dhendrix at google.com
Thu Apr 21 01:54:34 CEST 2016
Looks pretty good to me. FWIW, we implemented something nearly identical in
the chromiumos branch a few years ago:
https://chromium-review.googlesource.com/#/c/10180/ (we called it
"--diff"). It's been quite useful for development, especially when working
with large chips and slow external programmers.
On Tue, Apr 19, 2016 at 12:25 PM, Paul Kocialkowski <contact at 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 at 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 at flashrom.org
> https://www.flashrom.org/mailman/listinfo/flashrom
>
--
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20160420/0140adfb/attachment.html>
More information about the flashrom
mailing list