Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/23057 )
Change subject: buspirate_spi: Add support for variable serial speeds
......................................................................
buspirate_spi: Add support for variable serial speeds
This patch sets the default baud rate for communication between
the host device and the Bus Pirate for hardware versions 3.0
and greater to 2M baud.
It also introduces the ability to manually set the baud rate via
the added 'serialspeed' programmer parameter.
This is done in two parts. Firstly, the requested serial speed is looked up
in a table to determine the appropriate clock divisor and the divisor is sent
to the bus pirate. Then, the system's baud rate for the selected serial port
is set using serial.c's 'serialport_config'. This function's prototype had to
be added to programmer.h.
In testing, using the 2M baud rate was able to significantly decrease
flash times (down from 20+ minutes to less than 2 minutes for an 8MB flash).
Change-Id: I3706f17a94fdf056063f2ad4a5f0a219665cdcbf
Signed-off-by: Shawn Anastasio <shawnanastasio(a)yahoo.com>
Reviewed-on: https://review.coreboot.org/23057
Reviewed-by: Nico Huber <nico.h(a)gmx.de>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M buspirate_spi.c
M flashrom.8.tmpl
M programmer.h
3 files changed, 134 insertions(+), 11 deletions(-)
Approvals:
Nico Huber: Looks good to me, approved
build bot (Jenkins): Verified
diff --git a/buspirate_spi.c b/buspirate_spi.c
index 6f4c6d0..f825415 100644
--- a/buspirate_spi.c
+++ b/buspirate_spi.c
@@ -30,16 +30,18 @@
/* Change this to #define if you want to test without a serial implementation */
#undef FAKE_COMMUNICATION
-struct buspirate_spispeeds {
+struct buspirate_speeds {
const char *name;
const int speed;
};
+#define BP_DEFAULTBAUD 115200
+
#ifndef FAKE_COMMUNICATION
static int buspirate_serialport_setup(char *dev)
{
/* 115200bps, 8 databits, no parity, 1 stopbit */
- sp_fd = sp_openserport(dev, 115200);
+ sp_fd = sp_openserport(dev, BP_DEFAULTBAUD);
if (sp_fd == SER_INV_FD)
return 1;
return 0;
@@ -146,7 +148,7 @@
.write_aai = default_spi_write_aai,
};
-static const struct buspirate_spispeeds spispeeds[] = {
+static const struct buspirate_speeds spispeeds[] = {
{"30k", 0x0},
{"125k", 0x1},
{"250k", 0x2},
@@ -155,7 +157,16 @@
{"2.6M", 0x5},
{"4M", 0x6},
{"8M", 0x7},
- {NULL, 0x0},
+ {NULL, 0x0}
+};
+
+static const struct buspirate_speeds serialspeeds[] = {
+ {"115200", 115200},
+ {"230400", 230400},
+ {"250000", 250000},
+ {"2000000", 2000000},
+ {"2M", 2000000},
+ {NULL, 0}
};
static int buspirate_spi_shutdown(void *data)
@@ -199,15 +210,26 @@
}
#define BP_FWVERSION(a,b) ((a) << 8 | (b))
+#define BP_HWVERSION(a,b) BP_FWVERSION(a,b)
+
+/**
+ * The Bus Pirate's PIC microcontroller supports custom baud rates by manually specifying a
+ * clock divisor that can be calculated with the formula (16000000 / (4 * baud)) - 1.
+ */
+#define BP_DIVISOR(baud) ((4000000/(baud)) - 1)
int buspirate_spi_init(void)
{
char *tmp;
char *dev;
int i;
+ int cnt;
unsigned int fw_version_major = 0;
unsigned int fw_version_minor = 0;
+ unsigned int hw_version_major = 0;
+ unsigned int hw_version_minor = 0;
int spispeed = 0x7;
+ int serialspeed_index = -1;
int ret = 0;
int pullup = 0;
@@ -234,6 +256,20 @@
}
free(tmp);
+ /* Extract serialspeed paramater */
+ tmp = extract_programmer_param("serialspeed");
+ if (tmp) {
+ for (i = 0; serialspeeds[i].name; i++) {
+ if (!strncasecmp(serialspeeds[i].name, tmp, strlen(serialspeeds[i].name))) {
+ serialspeed_index = i;
+ break;
+ }
+ }
+ if (!serialspeeds[i].name)
+ msg_perr("Invalid serial speed %s, using default.\n", tmp);
+ }
+ free(tmp);
+
tmp = extract_programmer_param("pullups");
if (tmp) {
if (strcasecmp("on", tmp) == 0)
@@ -313,7 +349,20 @@
break;
}
bp_commbuf[i] = '\0';
- msg_pdbg("Detected Bus Pirate hardware %s\n", bp_commbuf);
+ msg_pdbg("Detected Bus Pirate hardware ");
+ if (bp_commbuf[0] != 'v')
+ msg_pdbg("(unknown version number format)");
+ else if (!strchr("0123456789", bp_commbuf[1]))
+ msg_pdbg("(unknown version number format)");
+ else {
+ hw_version_major = strtoul((char *)bp_commbuf + 1, &tmp, 10);
+ while ((*tmp != '\0') && !strchr("0123456789", *tmp))
+ tmp++;
+ hw_version_minor = strtoul(tmp, NULL, 10);
+ msg_pdbg("%u.%u", hw_version_major, hw_version_minor);
+ }
+ msg_pdbg2(" (\"%s\")", bp_commbuf);
+ msg_pdbg("\n");
if ((ret = buspirate_wait_for_string(bp_commbuf, "irmware ")))
return ret;
@@ -342,7 +391,7 @@
if ((ret = buspirate_wait_for_string(bp_commbuf, "HiZ>")))
return ret;
-
+
/* Tell the user about missing SPI binary mode in firmware 2.3 and older. */
if (BP_FWVERSION(fw_version_major, fw_version_minor) < BP_FWVERSION(2, 4)) {
msg_pinfo("Bus Pirate firmware 2.3 and older does not support binary SPI access.\n");
@@ -352,7 +401,7 @@
/* Use fast SPI mode in firmware 5.5 and newer. */
if (BP_FWVERSION(fw_version_major, fw_version_minor) >= BP_FWVERSION(5, 5)) {
- msg_pdbg("Using SPI command set v2.\n");
+ msg_pdbg("Using SPI command set v2.\n");
/* Sensible default buffer size. */
if (buspirate_commbuf_grow(260 + 5))
return ERROR_OOM;
@@ -379,10 +428,71 @@
msg_pinfo("It is recommended to upgrade to firmware 6.2 or newer.\n");
spispeed = 0x4;
}
-
+
/* This works because speeds numbering starts at 0 and is contiguous. */
msg_pdbg("SPI speed is %sHz\n", spispeeds[spispeed].name);
+ /* Set 2M baud serial speed by default on hardware 3.0 and newer if a custom speed was not set */
+ if (serialspeed_index == -1 && BP_HWVERSION(hw_version_major, hw_version_minor) >= BP_HWVERSION(3, 0)) {
+ serialspeed_index = ARRAY_SIZE(serialspeeds) - 2;
+ msg_pdbg("Bus Pirate v3 or newer detected. Set serial speed to 2M baud.\n");
+ }
+
+ /* Set custom serial speed if specified */
+ if (serialspeed_index != -1) {
+ if (BP_FWVERSION(fw_version_major, fw_version_minor) < BP_FWVERSION(5, 5)) {
+ /* This feature requires firmware 5.5 or newer */
+ msg_perr("Bus Pirate firmware 5.4 and older does not support custom serial speeds."
+ "Using default speed of 115200 baud.\n");
+ } else if (serialspeeds[serialspeed_index].speed != BP_DEFAULTBAUD) {
+ /* Set the serial speed to match the user's choice if it doesn't already */
+
+ if (BP_HWVERSION(hw_version_major, hw_version_minor) < BP_HWVERSION(3, 0))
+ msg_pwarn("Increased serial speeds may not work on older (<3.0) Bus Pirates."
+ " Continue at your own risk.\n");
+
+ /* Enter baud rate configuration mode */
+ cnt = snprintf((char *)bp_commbuf, DEFAULT_BUFSIZE, "b\n");
+ if ((ret = buspirate_sendrecv(bp_commbuf, cnt, 0)))
+ return ret;
+ if ((ret = buspirate_wait_for_string(bp_commbuf, ">")))
+ return ret;
+
+ /* Enter manual clock divisor entry mode */
+ cnt = snprintf((char *)bp_commbuf, DEFAULT_BUFSIZE, "10\n");
+ if ((ret = buspirate_sendrecv(bp_commbuf, cnt, 0)))
+ return ret;
+ if ((ret = buspirate_wait_for_string(bp_commbuf, ">")))
+ return ret;
+
+ /* Set the clock divisor to the value calculated from the user's input */
+ cnt = snprintf((char *)bp_commbuf, DEFAULT_BUFSIZE, "%d\n",
+ BP_DIVISOR(serialspeeds[serialspeed_index].speed));
+
+ if ((ret = buspirate_sendrecv(bp_commbuf, cnt, 0)))
+ return ret;
+ sleep(1);
+
+ /* Reconfigure the host's serial baud rate to the new value */
+ if ((ret = serialport_config(sp_fd, serialspeeds[serialspeed_index].speed))) {
+ msg_perr("Unable to configure system baud rate to specified value.");
+ return ret;
+ }
+
+ /* Return to the main prompt */
+ bp_commbuf[0] = ' ';
+ if ((ret = buspirate_sendrecv(bp_commbuf, 1, 0)))
+ return ret;
+ if ((ret = buspirate_wait_for_string(bp_commbuf, "HiZ>")))
+ return ret;
+
+ msg_pdbg("Serial speed is %d baud\n", serialspeeds[serialspeed_index].speed);
+ }
+
+ }
+
+
+
/* Enter raw bitbang mode */
for (i = 0; i < 20; i++) {
bp_commbuf[0] = 0x00;
@@ -434,7 +544,7 @@
msg_perr("Protocol error while setting SPI speed!\n");
return 1;
}
-
+
/* Set SPI config: output type, idle, clock edge, sample */
bp_commbuf[0] = 0x80 | 0xa;
ret = buspirate_sendrecv(bp_commbuf, 1, 1);
@@ -534,7 +644,7 @@
bp_commbuf[i++] = (readcnt >> 8) & 0xff;
bp_commbuf[i++] = readcnt & 0xff;
memcpy(bp_commbuf + i, writearr, writecnt);
-
+
ret = buspirate_sendrecv(bp_commbuf, i + writecnt, 1 + readcnt);
if (ret) {
@@ -551,4 +661,4 @@
memcpy(readarr, bp_commbuf + 1, readcnt);
return ret;
-}
+}
\ No newline at end of file
diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl
index 143d76c..1600113 100644
--- a/flashrom.8.tmpl
+++ b/flashrom.8.tmpl
@@ -817,6 +817,18 @@
.BR 30k ", " 125k ", " 250k ", " 1M ", " 2M ", " 2.6M ", " 4M " or " 8M
(in Hz). The default is the maximum frequency of 8 MHz.
.sp
+The baud rate for communication between the host and the Bus Pirate can be specified with the optional
+.B serialspeed
+parameter. Syntax is
+.sp
+.B " flashrom -p buspirate_spi:serialspeed=baud
+.sp
+where
+.B baud
+can be
+.BR 115200 ", " 230400 ", " 250000 " or " 2000000 " (" 2M ")."
+The default is 2M baud for Bus Pirate hardware version 3.0 and greater, and 115200 otherwise.
+.sp
An optional pullups parameter specifies the use of the Bus Pirate internal pull-up resistors. This may be
needed if you are working with a flash ROM chip that you have physically removed from the board. Syntax is
.sp
diff --git a/programmer.h b/programmer.h
index 8736449..139f4fa 100644
--- a/programmer.h
+++ b/programmer.h
@@ -759,6 +759,7 @@
void sp_flush_incoming(void);
fdtype sp_openserport(char *dev, int baud);
extern fdtype sp_fd;
+int serialport_config(fdtype fd, int baud);
int serialport_shutdown(void *data);
int serialport_write(const unsigned char *buf, unsigned int writecnt);
int serialport_write_nonblock(const unsigned char *buf, unsigned int writecnt, unsigned int timeout, unsigned int *really_wrote);
--
To view, visit https://review.coreboot.org/23057
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3706f17a94fdf056063f2ad4a5f0a219665cdcbf
Gerrit-Change-Number: 23057
Gerrit-PatchSet: 8
Gerrit-Owner: Shawn Anastasio <shawnanastasio(a)yahoo.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Shawn Anastasio <shawnanastasio(a)yahoo.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Nico Huber has posted comments on this change. ( https://review.coreboot.org/23057 )
Change subject: buspirate_spi: Add support for variable serial speeds
......................................................................
Patch Set 6: Code-Review+2
(1 comment)
I'll take it as is. You can still optimize that delay in a follow-up
if you want.
https://review.coreboot.org/#/c/23057/6/buspirate_spi.c
File buspirate_spi.c:
https://review.coreboot.org/#/c/23057/6/buspirate_spi.c@473
PS6, Line 473: sleep(1);
> It's really just a safe value. […]
Idk, if it's worth it depends pretty much on the optimal value. e.g.
if it's <1ms, it's definitely worth it, if >500ms, rather not.
OTOH, if it's close to 1s, we might not be even safe yet. So it would
be be nice to know, even if we don't optimize it ;)
--
To view, visit https://review.coreboot.org/23057
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3706f17a94fdf056063f2ad4a5f0a219665cdcbf
Gerrit-Change-Number: 23057
Gerrit-PatchSet: 6
Gerrit-Owner: Shawn Anastasio <shawnanastasio(a)yahoo.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Shawn Anastasio <shawnanastasio(a)yahoo.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Fri, 26 Jan 2018 15:22:05 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
Hello Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/23225
to look at the new patch set (#3).
Change subject: Whitelist Lenovo Thinkpad X220
......................................................................
Whitelist Lenovo Thinkpad X220
Coreboot uses the subvendor VID/DID of the Thinkpad X220-Tablet for
all X220 variants. Don't specify a dmidecode string so that i can be
used with coreboot on all X220 variants and on X220-Tablet with vendor
firmare.
Change-Id: Idd667da8209a664469c1a909a549d2b625714a78
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M board_enable.c
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/25/23225/3
--
To view, visit https://review.coreboot.org/23225
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idd667da8209a664469c1a909a549d2b625714a78
Gerrit-Change-Number: 23225
Gerrit-PatchSet: 3
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/23225 )
Change subject: Whitelist Lenovo Thinkpad X220
......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/23225/2/board_enable.c
File board_enable.c:
https://review.coreboot.org/#/c/23225/2/board_enable.c@2437
PS2, Line 2437: 0x21DB
coreboot set 0x21db, but vendor set 0x21da...
--
To view, visit https://review.coreboot.org/23225
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd667da8209a664469c1a909a549d2b625714a78
Gerrit-Change-Number: 23225
Gerrit-PatchSet: 2
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 08:19:45 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
David Hendricks has posted comments on this change. ( https://review.coreboot.org/23021 )
Change subject: layout: Add -i <region>[:<file>] support.
......................................................................
Patch Set 13:
(3 comments)
PS13 addresses a couple lingering issues from earlier patch sets.
I also came up with a patch to try and implement the original intended behavior for '-w': https://review.coreboot.org/#/c/flashrom/+/23353/
It's a bit hacky though, though it does allow us to keep the policy decision in the CLI code. There might be a better way.
https://review.coreboot.org/#/c/23021/5/flashrom.c
File flashrom.c:
https://review.coreboot.org/#/c/23021/5/flashrom.c@1484
PS5, Line 1484: const char *region_file = layout->entries[i].file;
:
: if (first_entry_encountered == 0) {
: ms
> Should be kept in do_write_buf_to_file() where the actual file handling […]
Done
https://review.coreboot.org/#/c/23021/5/layout.c
File layout.c:
https://review.coreboot.org/#/c/23021/5/layout.c@217
PS5, Line 217: }
> why? is it necessary? doesn't `file` point into `argv` of main()?
No, I don't think it's necessary. It seems to be a remnant of https://mail.coreboot.org/pipermail/flashrom/2013-October/011729.html, but that may have been based on an even older version of layout.c from CrOS.
https://review.coreboot.org/#/c/23021/5/layout.c@285
PS5, Line 285:
removed this as well since we don't need to allocate memory for layout.entries[i].file.
--
To view, visit https://review.coreboot.org/23021
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99
Gerrit-Change-Number: 23021
Gerrit-PatchSet: 13
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 22 Jan 2018 08:20:54 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/23353 )
Change subject: WIP: Add romentry flags and function to append a single layout entry
......................................................................
Patch Set 1: Verified+1
Build Successful
https://qa.coreboot.org/job/flashrom-customrules/1077/ : SUCCESS
https://qa.coreboot.org/job/flashrom_gerrit/919/ : SUCCESS
--
To view, visit https://review.coreboot.org/23353
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9eb4a7751681c639028d6854d877558daf59e567
Gerrit-Change-Number: 23353
Gerrit-PatchSet: 1
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 22 Jan 2018 08:19:20 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: Yes
