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