[flashrom] [PATCH 1/1] Add support for reading the current flash contents from a file
Paul Kocialkowski
contact at paulk.fr
Sat Apr 23 19:30:04 CEST 2016
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 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 --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20160423/044c1d03/attachment.asc>
More information about the flashrom
mailing list