Hi Paul,
we're currently trying to transition to git, and thus we're still
working out the best workflow to continue development. Admittedly this
is a larger resource drain than anticipated.
I wish to apologize for the delayed review. Please find a few thoughts
of mine below.
On 19.04.2016 21:25, Paul Kocialkowski 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 <cont...@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 "
You're introducing a new toplevel parameter for this, and I wonder
whether it would make more sense to handle it as a programmer option.
The dummy programmer supports reading in emulated flash chip content on
startup and writing out emulated flash chip content on shutdown, all
specified with -p dummy:emulate=M25P10.RES,image=foobar.bin
For consistency reasons, we should make sure that supplying assumed chip
contents works as designed both for hardware-based programmers as well
as the dummy programmer.
I don't have a strong preference for -C or -p foobar:image=some.bin, and
would like to hear what others think.
Regards,
Carl-Daniel
_______________________________________________
flashrom mailing list
flashrom@flashrom.org
https://www.flashrom.org/mailman/listinfo/flashrom
