Change in flashrom[master]: udelay.c: Enable providing delay calibration value through CLI
Hrvoje Čavrak has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/44517 )
Change subject: udelay.c: Enable providing delay calibration value through CLI ......................................................................
udelay.c: Enable providing delay calibration value through CLI
This commit implements --get-loop-delay/--set-loop-delay options to retrieve and provide the calibration loop delay through the command line. The delay calibration procedure can take up to few seconds, which adds up when executed a number of times consecutively. Therefore, this provides a means to execute it only once, and then re-use the calibrated value on subsequent runs. Also, it implements a sanity check, expecting the value to land within 10% of the theoretical delay expectance, otherwise it ignores the provided value and runs the calibration procedure instead.
Signed-off-by: Hrvoje Cavrak hrvoje@hrvoje.org Change-Id: I7c9a491c5cb5fb3af56a691a0c6af9b53dcff550 --- M cli_classic.c M flashrom.8.tmpl M libflashrom.c M programmer.h M udelay.c 5 files changed, 60 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/17/44517/1
diff --git a/cli_classic.c b/cli_classic.c index 967ff50..b023c45 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -63,6 +63,8 @@ " -o | --output <logfile> log output to <logfile>\n" " --flash-contents <ref-file> assume flash contents to be <ref-file>\n" " -L | --list-supported print supported devices\n" + " --set-loop-delay <value> set loop delay calibration manually\n" + " --get-loop-delay retrieve loop delay calibration\n" #if CONFIG_PRINT_WIKI == 1 " -z | --list-supported-wiki print supported devices in wiki syntax\n" #endif @@ -118,6 +120,7 @@ int flash_name = 0, flash_size = 0; int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; int dont_verify_it = 0, dont_verify_all = 0, list_supported = 0, operation_specified = 0; + unsigned long provided_delay = 0; struct flashrom_layout *layout = NULL; enum programmer prog = PROGRAMMER_INVALID; enum { @@ -127,6 +130,8 @@ OPTION_FLASH_CONTENTS, OPTION_FLASH_NAME, OPTION_FLASH_SIZE, + OPTION_GET_LOOP_DELAY, + OPTION_SET_LOOP_DELAY, }; int ret = 0;
@@ -156,6 +161,8 @@ {"help", 0, NULL, 'h'}, {"version", 0, NULL, 'R'}, {"output", 1, NULL, 'o'}, + {"get-loop-delay", 0, NULL, OPTION_GET_LOOP_DELAY}, + {"set-loop-delay", 1, NULL, OPTION_SET_LOOP_DELAY}, {NULL, 0, NULL, 0}, };
@@ -365,6 +372,14 @@ } #endif /* STANDALONE */ break; + case OPTION_GET_LOOP_DELAY: + /* Make it easy for scripts to parse this by omitting anything else */ + msg_pinfo("%lu\n", get_calibration_value()); + exit(0); + break; + case OPTION_SET_LOOP_DELAY: + provided_delay = strtoul(strdup(optarg), NULL, 0); + break; default: cli_classic_abort_usage(NULL); break; @@ -457,7 +472,10 @@ }
/* FIXME: Delay calibration should happen in programmer code. */ - myusec_calibrate_delay(); + if (provided_delay) + set_external_calibration(provided_delay); + else + myusec_calibrate_delay(0);
if (programmer_init(prog, pparam)) { msg_perr("Error: Programmer initialization failed.\n"); diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index db50d59..4aada91 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -50,7 +50,7 @@ [\fB-E\fR|\fB-r\fR <file>|\fB-w\fR <file>|\fB-v\fR <file>] [(\fB-l\fR <file>|\fB--ifd|\fB --fmap\fR|\fB--fmap-file\fR <file>) [\fB-i\fR <image>]] [\fB-n\fR] [\fB-N\fR] [\fB-f\fR])] - [\fB-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>] + [\fB-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>] [\fB-d\fR get | <value>] .SH DESCRIPTION .B flashrom is a utility for detecting, reading, writing, verifying and erasing flash @@ -364,6 +364,17 @@ .TP .B "-R, --version" Show version information and exit. +.TP +.B "--get-loop-delay" +Retrieve the loop calibration delay value or provide one externally. Calibrate +delay procedure can take a few seconds and for repeated execution on a large +number of hosts it can provide a significant overhead. By getting the value, +it can later be provided by --set-loop-delay +.TP +.B "--set-loop-delay <value>" +This argument makes it possible to provide the calibration delay through the +command line option. + .SH PROGRAMMER-SPECIFIC INFORMATION Some programmer drivers accept further parameters to set programmer-specific parameters. These parameters are separated from the programmer name by a diff --git a/libflashrom.c b/libflashrom.c index ae2d33d..92e2527 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -51,7 +51,7 @@ { if (perform_selfcheck && selfcheck()) return 1; - myusec_calibrate_delay(); + myusec_calibrate_delay(0); return 0; }
diff --git a/programmer.h b/programmer.h index c5cab18..e0925e2 100644 --- a/programmer.h +++ b/programmer.h @@ -280,9 +280,11 @@
/* udelay.c */ void myusec_delay(unsigned int usecs); -void myusec_calibrate_delay(void); +void myusec_calibrate_delay(int force); void internal_sleep(unsigned int usecs); void internal_delay(unsigned int usecs); +unsigned long get_calibration_value(void); +void set_external_calibration(unsigned long external_micro);
#if CONFIG_INTERNAL == 1 /* board_enable.c */ diff --git a/udelay.c b/udelay.c index 6c0efc4..698f1ca 100644 --- a/udelay.c +++ b/udelay.c @@ -133,9 +133,9 @@ return timeusec; }
-void myusec_calibrate_delay(void) +void myusec_calibrate_delay(int force) { - if (clock_check_res()) + if (clock_check_res() && !force) return;
unsigned long count = 1000; @@ -221,6 +221,28 @@ msg_pinfo("OK.\n"); }
+void set_external_calibration(unsigned long external_micro) +{ + micro = external_micro; + unsigned long elapsed = measure_delay(100000); + /* Do a sanity check if the provided parameter makes loops fall + * within 10% of the desired value. Otherwise, ignore provided value + * and recalibrate. + */ + if (elapsed > 90000 && elapsed < 110000) + msg_pinfo("Provided delay is acceptable!\n"); + else { + msg_pinfo("Elapsed: %lu, provided delay is NOT acceptable!\n", elapsed); + myusec_calibrate_delay(1); /* Ignore using the system clock for loop timing */ + } +} + +unsigned long get_calibration_value(void) +{ + myusec_calibrate_delay(1); /* Ignore using the system clock for loop timing */ + return micro; +} + /* Not very precise sleep. */ void internal_sleep(unsigned int usecs) { @@ -250,7 +272,7 @@ #else #include <libpayload.h>
-void myusec_calibrate_delay(void) +void myusec_calibrate_delay(int force) { get_cpu_speed(); }
Hrvoje Čavrak has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44517 )
Change subject: udelay.c: Enable providing delay calibration value through CLI ......................................................................
Patch Set 1:
Hey David, as discussed I'm sending a revised patch suggestion for the get/set loop delay :)
Hello David Hendricks,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/44517
to look at the new patch set (#2).
Change subject: udelay.c: Enable providing delay calibration value through CLI ......................................................................
udelay.c: Enable providing delay calibration value through CLI
This commit implements --get-loop-delay/--set-loop-delay options to retrieve and provide the calibration loop delay through the command line. The delay calibration procedure can take up to few seconds, which adds up when executed a number of times consecutively. Therefore, this provides a means to execute it only once, and then re-use the calibrated value on subsequent runs. Also, it implements a sanity check, expecting the value to land within 10% of the theoretical delay expectance, otherwise it ignores the provided value and runs the calibration procedure instead.
Signed-off-by: Hrvoje Cavrak hrvoje@hrvoje.org Change-Id: I7c9a491c5cb5fb3af56a691a0c6af9b53dcff550 --- M cli_classic.c M flashrom.8.tmpl M libflashrom.c M programmer.h M udelay.c 5 files changed, 60 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/17/44517/2
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44517 )
Change subject: udelay.c: Enable providing delay calibration value through CLI ......................................................................
Patch Set 2: Code-Review-1
(4 comments)
Thanks, Hrvoje. I think the code looks good, I just have a few cosmetic nits.
https://review.coreboot.org/c/flashrom/+/44517/2/flashrom.8.tmpl File flashrom.8.tmpl:
https://review.coreboot.org/c/flashrom/+/44517/2/flashrom.8.tmpl@371 PS2, Line 371: provide s/provide a/cause
https://review.coreboot.org/c/flashrom/+/44517/2/flashrom.8.tmpl@370 PS2, Line 370: on a large : number of hosts Although you're correct, I think it's best to leave this out since "a large number of hosts" is an uncommon scenario. The "repeated execution" aspect is convincing on its own.
https://review.coreboot.org/c/flashrom/+/44517/2/udelay.c File udelay.c:
https://review.coreboot.org/c/flashrom/+/44517/2/udelay.c@236 PS2, Line 236: Ignore using the system clock for loop timing This comment seems more confusing than helpful. IIUC we're just ignoring the externally provided value.
https://review.coreboot.org/c/flashrom/+/44517/2/udelay.c@242 PS2, Line 242: /* Ignore using the system clock for loop timing */ Similar as above, the comment seems confusing (if I understand the intention correctly)
Hello build bot (Jenkins), David Hendricks,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/44517
to look at the new patch set (#3).
Change subject: udelay.c: Enable providing delay calibration value through CLI ......................................................................
udelay.c: Enable providing delay calibration value through CLI
This commit implements --get-loop-delay/--set-loop-delay options to retrieve and provide the calibration loop delay through the command line. The delay calibration procedure can take up to few seconds, which adds up when executed a number of times consecutively. Therefore, this provides a means to execute it only once, and then re-use the calibrated value on subsequent runs. Also, it implements a sanity check, expecting the value to land within 10% of the theoretical delay expectance, otherwise it ignores the provided value and runs the calibration procedure instead.
Signed-off-by: Hrvoje Cavrak hrvoje@hrvoje.org Change-Id: I7c9a491c5cb5fb3af56a691a0c6af9b53dcff550 --- M cli_classic.c M flashrom.8.tmpl M libflashrom.c M programmer.h M udelay.c 5 files changed, 60 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/17/44517/3
Hrvoje Čavrak has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44517 )
Change subject: udelay.c: Enable providing delay calibration value through CLI ......................................................................
Patch Set 3:
Patch Set 2: Code-Review-1
(4 comments)
Thanks, Hrvoje. I think the code looks good, I just have a few cosmetic nits.
Absolutely, that's what the process is for! :)
Hrvoje Čavrak has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44517 )
Change subject: udelay.c: Enable providing delay calibration value through CLI ......................................................................
Patch Set 3:
Patch Set 2: Code-Review-1
(4 comments)
Thanks, Hrvoje. I think the code looks good, I just have a few cosmetic nits.
Hey David, did you perhaps find time to look into this? Thanks! 😜
Hrvoje Čavrak has removed Paul Menzel from this change. ( https://review.coreboot.org/c/flashrom/+/44517 )
Change subject: udelay.c: Enable providing delay calibration value through CLI ......................................................................
Removed reviewer Paul Menzel.
-
David Hendricks (Code Review) -
Hrvoje Čavrak (Code Review)