Change in flashrom[master]: udelay.c: Enable providing delay calibration value through CLI

[Hrvoje Čavrak (Code Review)]

Hrvoje Čavrak has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/39841 )

Change subject: udelay.c: Enable providing delay calibration value through CLI
......................................................................

udelay.c: Enable providing delay calibration value through CLI

This commit implements --delay option 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.

modified:   cli_classic.c
        modified:   flashrom.8.tmpl
        modified:   programmer.h
        modified:   udelay.c

Change-Id: Iea2a7f62300663bc0a32ed4abced57c8c55c90c8
Signed-off-by: Hrvoje Cavrak 
---
M cli_classic.c
M flashrom.8.tmpl
M programmer.h
M udelay.c
4 files changed, 51 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/41/39841/1

diff --git a/cli_classic.c b/cli_classic.c
index 73cc417..f0842d4 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"
+	       " -d | --loop-delay <value>          set loop delay calibration manually\n"
+	       "      --loop-delay get              show 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 {
@@ -130,7 +133,7 @@
    };
    int ret = 0;

-	static const char optstring[] = "r:Rw:v:nNVEfc:l:i:p:Lzho:";
+	static const char optstring[] = "r:Rw:v:nNVEfc:l:i:p:Lzho:d:";
    static const struct option long_options[] = {
    	{"read",		1, NULL, 'r'},
    	{"write",		1, NULL, 'w'},
@@ -156,6 +159,7 @@
    	{"help",		0, NULL, 'h'},
    	{"version",		0, NULL, 'R'},
    	{"output",		1, NULL, 'o'},
+		{"delay",		1, NULL, 'd'},
    	{NULL,			0, NULL, 0},
    };

@@ -362,6 +366,14 @@
    		}
 #endif /* STANDALONE */
    		break;
+			case 'd':
+				provided_delay = strtoul(strdup(optarg), NULL, 0);
+				if (!provided_delay) {
+					/* Make it easy for scripts to parse this by omitting anything else */
+					msg_pinfo("%lu\n", get_calibration_value());
+					exit(0);
+				}
+				break;
    	default:
    		cli_classic_abort_usage(NULL);
    		break;
@@ -454,7 +466,10 @@
    }

/* FIXME: Delay calibration should happen in programmer code. */
-	myusec_calibrate_delay();
+	if (provided_delay)
+		set_external_calibration(provided_delay);
+	else
+		myusec_calibrate_delay();

if (programmer_init(prog, pparam)) {
    	msg_perr("Error: Programmer initialization failed.\n");
diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl
index fde98c0..c14f2de 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,18 @@
 .TP
 .B "-R, --version"
 Show version information and exit.
+.TP
+.B "-d, --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. Therefore, this argument
+makes it possible to provide the calibration delay through the command line
+option.
+
+To retrieve the value, run
+.BR "flashrom --loop-delay get " "and to provide it during programming, add "
+.BR "--loop-delay <value> " "to your command."
+
 .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/programmer.h b/programmer.h
index 08500c6..a659a18 100644
--- a/programmer.h
+++ b/programmer.h
@@ -277,6 +277,8 @@
 void myusec_calibrate_delay(void);
 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..90c4c9e 100644
--- a/udelay.c
+++ b/udelay.c
@@ -221,6 +221,25 @@
    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
+		myusec_calibrate_delay();
+}
+
+unsigned long get_calibration_value(void)
+{
+	myusec_calibrate_delay();
+	return micro;
+}
+
 /* Not very precise sleep. */
 void internal_sleep(unsigned int usecs)
 {

[Hrvoje Čavrak (Code Review)]

Hrvoje Čavrak has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39841 )

Change subject: udelay.c: Enable providing delay calibration value through CLI
......................................................................

Patch Set 1:

Hey, could you please review my patch and consider accepting it to the main branch?

This Gerrit takes some getting used to! :D :D :D

[Miklós Márton (Code Review)]

Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39841 )

Change subject: udelay.c: Enable providing delay calibration value through CLI
......................................................................

Patch Set 1:

(4 comments)

https://review.coreboot.org/c/flashrom/+/39841/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/39841/1//COMMIT_MSG@9 
PS1, Line 9: This commit implements --delay option to retrieve and provide the calibration
Use --loop-delay here as well

https://review.coreboot.org/c/flashrom/+/39841/1/cli_classic.c 
File cli_classic.c:

https://review.coreboot.org/c/flashrom/+/39841/1/cli_classic.c@67 
PS1, Line 67: 	       "      --loop-delay get              show loop delay calibration\n"
I would suggest to rename the --loop-delay get to --get-loop-delay because I see no other arguments having second argument. But let's hear others opinion about it.

https://review.coreboot.org/c/flashrom/+/39841/1/cli_classic.c@162 
PS1, Line 162: 		{"delay",		1, NULL, 'd'},
The help was modified to use the loop-delay argument so this one should be as well.

https://review.coreboot.org/c/flashrom/+/39841/1/udelay.c 
File udelay.c:

https://review.coreboot.org/c/flashrom/+/39841/1/udelay.c@228 
PS1, Line 228:     /* Do a sanity check if the provided parameter makes loops fall within 10% of
Please use tabs for the indentation of this comment.

[Hrvoje Čavrak (Code Review)]

Hello build bot (Jenkins), Miklós Márton, David Hendricks,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/39841

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 --loop-delay option 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.

modified:   cli_classic.c
        modified:   flashrom.8.tmpl
        modified:   programmer.h
        modified:   udelay.c

Change-Id: Iea2a7f62300663bc0a32ed4abced57c8c55c90c8
Signed-off-by: Hrvoje Cavrak 
---
M cli_classic.c
M flashrom.8.tmpl
M programmer.h
M udelay.c
4 files changed, 52 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/41/39841/2

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39841 )

Change subject: udelay.c: Enable providing delay calibration value through CLI
......................................................................

Patch Set 2: Code-Review+1

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39841 )

Change subject: udelay.c: Enable providing delay calibration value through CLI
......................................................................

Patch Set 2: Code-Review-1

(1 comment)

https://review.coreboot.org/c/flashrom/+/39841/1/cli_classic.c 
File cli_classic.c:

https://review.coreboot.org/c/flashrom/+/39841/1/cli_classic.c@67 
PS1, Line 67: 	       "      --loop-delay get              show loop delay calibration\n"
> I would suggest to rename the --loop-delay get to --get-loop-delay because I see no other arguments  […]

I agree with Miklós... `--loop-delay get` is a somewhat different syntax than we've used in the past, so it's probably better to stay consistent.

[Miklós Márton (Code Review)]

Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39841 )

Change subject: udelay.c: Enable providing delay calibration value through CLI
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review-1
>
> (1 comment)

We have had a discussion with Hrvoje that the delay calibration should be only performed for programmers which rely on (mainly bitbanging programmers I think). He is using linux_spi programmer where I doubt that it is necessary. So it might be more useful to fix this fixme:
https://github.com/flashrom/flashrom/blob/master/cli_classic.c#L456
So rather than adding an argument it might makes more sense to run the delay calibration only when it is really necessary.

[Hrvoje Čavrak (Code Review)]

Hrvoje Čavrak has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/39841 )

Change subject: udelay.c: Enable providing delay calibration value through CLI
......................................................................

Abandoned