May 2020 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: sb600spi.c: Don't access spibar directly
by Edward O'Callaghan (Code Review) 26 Nov '20

26 Nov '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/36433 ) Change subject: sb600spi.c: Don't access spibar directly ...................................................................... sb600spi.c: Don't access spibar directly Constrain the manner in which the global state of spibar is set and then subsequently accessed. This makes it easier to write unit-tests and instrument the code. Change-Id: Ic26372b9c1baebb20716eea1db1e942239ed3e48 Signed-off-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M sb600spi.c 1 file changed, 53 insertions(+), 38 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/36433/1 diff --git a/sb600spi.c b/sb600spi.c index 23b36ee..7bdd11f 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -40,7 +40,13 @@ *}; */ -static uint8_t *sb600_spibar = NULL; +static uint8_t *g_sb600_spibar = NULL; + +static inline uint8_t * get_spibar(void) +{ + return g_sb600_spibar; +} + enum amd_chipset { CHIPSET_AMD_UNKNOWN, CHIPSET_SB6XX, @@ -180,16 +186,18 @@ static void reset_internal_fifo_pointer(void) { - mmio_writeb(mmio_readb(sb600_spibar + 2) | 0x10, sb600_spibar + 2); + uint8_t *spibar = get_spibar(); + mmio_writeb(mmio_readb(spibar + 2) | 0x10, spibar + 2); /* FIXME: This loop needs a timeout and a clearer message. */ - while (mmio_readb(sb600_spibar + 0xD) & 0x7) + while (mmio_readb(spibar + 0xD) & 0x7) msg_pspew("reset\n"); } static int compare_internal_fifo_pointer(uint8_t want) { - uint8_t have = mmio_readb(sb600_spibar + 0xd) & 0x07; + uint8_t *spibar = get_spibar(); + uint8_t have = mmio_readb(spibar + 0xd) & 0x07; want %= FIFO_SIZE_OLD; if (have != want) { msg_perr("AMD SPI FIFO pointer corruption! Pointer is %d, wanted %d\n", have, want); @@ -224,8 +232,9 @@ static void execute_command(void) { msg_pspew("Executing... "); - mmio_writeb(mmio_readb(sb600_spibar + 2) | 1, sb600_spibar + 2); - while (mmio_readb(sb600_spibar + 2) & 1) + uint8_t *spibar = get_spibar(); + mmio_writeb(mmio_readb(spibar + 2) | 1, spibar + 2); + while (mmio_readb(spibar + 2) & 1) ; msg_pspew("done\n"); } @@ -239,7 +248,8 @@ unsigned char cmd = *writearr++; writecnt--; msg_pspew("%s, cmd=0x%02x, writecnt=%d, readcnt=%d\n", __func__, cmd, writecnt, readcnt); - mmio_writeb(cmd, sb600_spibar + 0); + uint8_t *spibar = get_spibar(); + mmio_writeb(cmd, spibar + 0); int ret = check_readwritecnt(flash, writecnt, readcnt); if (ret != 0) @@ -253,14 +263,14 @@ */ unsigned int readoffby1 = (writecnt > 0) ? 0 : 1; uint8_t readwrite = (readcnt + readoffby1) << 4 | (writecnt); - mmio_writeb(readwrite, sb600_spibar + 1); + mmio_writeb(readwrite, spibar + 1); reset_internal_fifo_pointer(); msg_pspew("Filling FIFO: "); unsigned int count; for (count = 0; count < writecnt; count++) { msg_pspew("[%02x]", writearr[count]); - mmio_writeb(writearr[count], sb600_spibar + 0xC); + mmio_writeb(writearr[count], spibar + 0xC); } msg_pspew("\n"); if (compare_internal_fifo_pointer(writecnt)) @@ -291,7 +301,7 @@ /* Skip the bytes we sent. */ msg_pspew("Skipping: "); for (count = 0; count < writecnt; count++) { - msg_pspew("[%02x]", mmio_readb(sb600_spibar + 0xC)); + msg_pspew("[%02x]", mmio_readb(spibar + 0xC)); } msg_pspew("\n"); if (compare_internal_fifo_pointer(writecnt)) @@ -299,14 +309,14 @@ msg_pspew("Reading FIFO: "); for (count = 0; count < readcnt; count++) { - readarr[count] = mmio_readb(sb600_spibar + 0xC); + readarr[count] = mmio_readb(spibar + 0xC); msg_pspew("[%02x]", readarr[count]); } msg_pspew("\n"); if (compare_internal_fifo_pointer(writecnt+readcnt)) return SPI_PROGRAMMER_ERROR; - if (mmio_readb(sb600_spibar + 1) != readwrite) { + if (mmio_readb(spibar + 1) != readwrite) { msg_perr("Unexpected change in AMD SPI read/write count!\n"); msg_perr("Something else is accessing the flash chip and causes random corruption.\n" "Please stop all applications and drivers and IPMI which access the flash chip.\n"); @@ -325,21 +335,22 @@ unsigned char cmd = *writearr++; writecnt--; msg_pspew("%s, cmd=0x%02x, writecnt=%d, readcnt=%d\n", __func__, cmd, writecnt, readcnt); - mmio_writeb(cmd, sb600_spibar + 0); + uint8_t *spibar = get_spibar(); + mmio_writeb(cmd, spibar + 0); int ret = check_readwritecnt(flash, writecnt, readcnt); if (ret != 0) return ret; /* Use the extended TxByteCount and RxByteCount registers. */ - mmio_writeb(writecnt, sb600_spibar + 0x48); - mmio_writeb(readcnt, sb600_spibar + 0x4b); + mmio_writeb(writecnt, spibar + 0x48); + mmio_writeb(readcnt, spibar + 0x4b); msg_pspew("Filling buffer: "); unsigned int count; for (count = 0; count < writecnt; count++) { msg_pspew("[%02x]", writearr[count]); - mmio_writeb(writearr[count], sb600_spibar + 0x80 + count); + mmio_writeb(writearr[count], spibar + 0x80 + count); } msg_pspew("\n"); @@ -347,7 +358,7 @@ msg_pspew("Reading buffer: "); for (count = 0; count < readcnt; count++) { - readarr[count] = mmio_readb(sb600_spibar + 0x80 + (writecnt + count) % FIFO_SIZE_YANGTZE); + readarr[count] = mmio_readb(spibar + 0x80 + (writecnt + count) % FIFO_SIZE_YANGTZE); msg_pspew("[%02x]", readarr[count]); } msg_pspew("\n"); @@ -375,16 +386,17 @@ { bool success = false; uint8_t speed = spispeed->speed; + uint8_t *spibar = get_spibar(); msg_pdbg("Setting SPI clock to %s (0x%x).\n", spispeed->name, speed); if (amd_gen >= CHIPSET_YANGTZE) { - rmmio_writew((speed << 12) | (speed << 8) | (speed << 4) | speed, sb600_spibar + 0x22); - uint16_t tmp = mmio_readw(sb600_spibar + 0x22); + rmmio_writew((speed << 12) | (speed << 8) | (speed << 4) | speed, spibar + 0x22); + uint16_t tmp = mmio_readw(spibar + 0x22); success = (((tmp >> 12) & 0xf) == speed && ((tmp >> 8) & 0xf) == speed && ((tmp >> 4) & 0xf) == speed && ((tmp >> 0) & 0xf) == speed); } else { - rmmio_writeb((mmio_readb(sb600_spibar + 0xd) & ~(0x3 << 4)) | (speed << 4), sb600_spibar + 0xd); - success = (speed == ((mmio_readb(sb600_spibar + 0xd) >> 4) & 0x3)); + rmmio_writeb((mmio_readb(spibar + 0xd) & ~(0x3 << 4)) | (speed << 4), spibar + 0xd); + success = (speed == ((mmio_readb(spibar + 0xd) >> 4) & 0x3)); } if (!success) { @@ -396,11 +408,12 @@ static int set_mode(struct pci_dev *dev, uint8_t read_mode) { - uint32_t tmp = mmio_readl(sb600_spibar + 0x00); + uint8_t *spibar = get_spibar(); + uint32_t tmp = mmio_readl(spibar + 0x00); tmp &= ~(0x6 << 28 | 0x1 << 18); /* Clear mode bits */ tmp |= ((read_mode & 0x6) << 28) | ((read_mode & 0x1) << 18); - rmmio_writel(tmp, sb600_spibar + 0x00); - if (tmp != mmio_readl(sb600_spibar + 0x00)) + rmmio_writel(tmp, spibar + 0x00); + if (tmp != mmio_readl(spibar + 0x00)) return 1; return 0; } @@ -409,6 +422,7 @@ { uint32_t tmp; uint8_t spispeed_idx = 3; /* Default to 16.5 MHz */ + uint8_t *spibar = get_spibar(); char *spispeed = extract_programmer_param("spispeed"); if (spispeed != NULL) { @@ -450,7 +464,7 @@ "Normal (up to 66 MHz)", /* 6 */ "Fast Read", /* 7 (Not defined in the Bolton datasheet.) */ }; - tmp = mmio_readl(sb600_spibar + 0x00); + tmp = mmio_readl(spibar + 0x00); uint8_t read_mode = ((tmp >> 28) & 0x6) | ((tmp >> 18) & 0x1); msg_pdbg("SpiReadMode=%s (%i)\n", spireadmodes[read_mode], read_mode); if (read_mode != 6) { @@ -463,11 +477,11 @@ } if (amd_gen >= CHIPSET_YANGTZE) { - tmp = mmio_readb(sb600_spibar + 0x20); + tmp = mmio_readb(spibar + 0x20); msg_pdbg("UseSpi100 is %sabled\n", (tmp & 0x1) ? "en" : "dis"); if ((tmp & 0x1) == 0) { - rmmio_writeb(tmp | 0x1, sb600_spibar + 0x20); - tmp = mmio_readb(sb600_spibar + 0x20) & 0x1; + rmmio_writeb(tmp | 0x1, spibar + 0x20); + tmp = mmio_readb(spibar + 0x20) & 0x1; if (tmp == 0) { msg_perr("Enabling Spi100 failed.\n"); return 1; @@ -475,7 +489,7 @@ msg_pdbg("Enabling Spi100 succeeded.\n"); } - tmp = mmio_readw(sb600_spibar + 0x22); /* SPI 100 Speed Config */ + tmp = mmio_readw(spibar + 0x22); /* SPI 100 Speed Config */ msg_pdbg("NormSpeedNew is %s\n", spispeeds[(tmp >> 12) & 0xf].name); msg_pdbg("FastSpeedNew is %s\n", spispeeds[(tmp >> 8) & 0xf].name); msg_pdbg("AltSpeedNew is %s\n", spispeeds[(tmp >> 4) & 0xf].name); @@ -483,15 +497,15 @@ } } else { if (amd_gen >= CHIPSET_SB89XX && amd_gen <= CHIPSET_HUDSON234) { - bool fast_read = (mmio_readl(sb600_spibar + 0x00) >> 18) & 0x1; + bool fast_read = (mmio_readl(spibar + 0x00) >> 18) & 0x1; msg_pdbg("Fast Reads are %sabled\n", fast_read ? "en" : "dis"); if (fast_read) { msg_pdbg("Disabling them temporarily.\n"); - rmmio_writel(mmio_readl(sb600_spibar + 0x00) & ~(0x1 << 18), - sb600_spibar + 0x00); + rmmio_writel(mmio_readl(spibar + 0x00) & ~(0x1 << 18), + spibar + 0x00); } } - tmp = (mmio_readb(sb600_spibar + 0xd) >> 4) & 0x3; + tmp = (mmio_readb(spibar + 0xd) >> 4) & 0x3; msg_pdbg("NormSpeed is %s\n", spispeeds[tmp].name); } return set_speed(dev, amd_gen, &spispeeds[spispeed_idx]); @@ -581,14 +595,14 @@ return 0; /* Physical memory has to be mapped at page (4k) boundaries. */ - sb600_spibar = rphysmap("SB600 SPI registers", tmp & 0xfffff000, 0x1000); - if (sb600_spibar == ERROR_PTR) + g_sb600_spibar = rphysmap("SB600 SPI registers", tmp & 0xfffff000, 0x1000); + if (g_sb600_spibar == ERROR_PTR) return ERROR_FATAL; /* The low bits of the SPI base address are used as offset into * the mapped page. */ - sb600_spibar += tmp & 0xfff; + g_sb600_spibar += tmp & 0xfff; int amd_gen = determine_generation(dev); if (amd_gen < 0) @@ -649,7 +663,8 @@ * * <1> see handle_speed */ - tmp = mmio_readl(sb600_spibar + 0x00); + uint8_t *spibar = get_spibar(); + tmp = mmio_readl(spibar + 0x00); msg_pdbg("(0x%08" PRIx32 ") SpiArbEnable=%i", tmp, (tmp >> 19) & 0x1); if (amd_gen >= CHIPSET_YANGTZE) msg_pdbg(", IllegalAccess=%i", (tmp >> 21) & 0x1); @@ -679,7 +694,7 @@ } if (amd_gen >= CHIPSET_SB89XX) { - tmp = mmio_readb(sb600_spibar + 0x1D); + tmp = mmio_readb(spibar + 0x1D); msg_pdbg("Using SPI_CS%d\n", tmp & 0x3); /* FIXME: Handle SpiProtect* configuration on Yangtze. */ } -- To view, visit https://review.coreboot.org/c/flashrom/+/36433 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic26372b9c1baebb20716eea1db1e942239ed3e48 Gerrit-Change-Number: 36433 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

2 2

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state.
by Edward O'Callaghan (Code Review) 25 Nov '20

25 Nov '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/36432 ) Change subject: sb600spi.c: Remove 'amd_gen' out of global state. ...................................................................... sb600spi.c: Remove 'amd_gen' out of global state. Have 'determine_generation()' explicitly return 'amd_gen' and then pass the state into what requires it. Thus making the code more pure, easier to read and more unit-testable. Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a Signed-off-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M sb600spi.c 1 file changed, 12 insertions(+), 13 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/36432/1 diff --git a/sb600spi.c b/sb600spi.c index 8daaf03..23b36ee 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -51,7 +51,6 @@ CHIPSET_YANGTZE, CHIPSET_PROMONTORY, }; -static enum amd_chipset amd_gen = CHIPSET_AMD_UNKNOWN; #define FIFO_SIZE_OLD 8 #define FIFO_SIZE_YANGTZE 71 @@ -110,7 +109,7 @@ /* Determine the chipset's version and identify the respective SMBUS device. */ static int determine_generation(struct pci_dev *dev) { - amd_gen = CHIPSET_AMD_UNKNOWN; + enum amd_chipset amd_gen = CHIPSET_AMD_UNKNOWN; msg_pdbg2("Trying to determine the generation of the SPI interface... "); if (dev->device_id == 0x438d) { amd_gen = CHIPSET_SB6XX; @@ -173,11 +172,10 @@ "Please report this to flashrom(a)flashrom.org and include this log and\n" "the output of lspci -nnvx, thanks!\n", __func__, dev->vendor_id, dev->device_id); - if (amd_gen == CHIPSET_AMD_UNKNOWN) { + if (amd_gen == CHIPSET_AMD_UNKNOWN) msg_perr("Could not determine chipset generation."); - return -1; - } - return 0; + + return amd_gen; } static void reset_internal_fifo_pointer(void) @@ -373,7 +371,7 @@ { "800 kHz", 0x07 }, }; -static int set_speed(struct pci_dev *dev, const struct spispeed *spispeed) +static int set_speed(struct pci_dev *dev, enum amd_chipset amd_gen, const struct spispeed *spispeed) { bool success = false; uint8_t speed = spispeed->speed; @@ -407,7 +405,7 @@ return 0; } -static int handle_speed(struct pci_dev *dev) +static int handle_speed(struct pci_dev *dev, enum amd_chipset amd_gen) { uint32_t tmp; uint8_t spispeed_idx = 3; /* Default to 16.5 MHz */ @@ -496,10 +494,10 @@ tmp = (mmio_readb(sb600_spibar + 0xd) >> 4) & 0x3; msg_pdbg("NormSpeed is %s\n", spispeeds[tmp].name); } - return set_speed(dev, &spispeeds[spispeed_idx]); + return set_speed(dev, amd_gen, &spispeeds[spispeed_idx]); } -static int handle_imc(struct pci_dev *dev) +static int handle_imc(struct pci_dev *dev, enum amd_chipset amd_gen) { /* Handle IMC everywhere but sb600 which does not have one. */ if (amd_gen == CHIPSET_SB6XX) @@ -592,7 +590,8 @@ */ sb600_spibar += tmp & 0xfff; - if (determine_generation(dev) < 0) + int amd_gen = determine_generation(dev); + if (amd_gen < 0) return ERROR_NONFATAL; /* How to read the following table and similar ones in this file: @@ -725,10 +724,10 @@ return 0; } - if (handle_speed(dev) != 0) + if (handle_speed(dev, amd_gen) != 0) return ERROR_FATAL; - if (handle_imc(dev) != 0) + if (handle_imc(dev, amd_gen) != 0) return ERROR_FATAL; /* Starting with Yangtze the SPI controller got a different interface with a much bigger buffer. */ -- To view, visit https://review.coreboot.org/c/flashrom/+/36432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a Gerrit-Change-Number: 36432 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

5 15

Change in flashrom[master]: tests/: [RFC]: Implement unit-testing using mtest
by Edward O'Callaghan (Code Review) 24 Nov '20

24 Nov '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/38954 ) Change subject: tests/: [RFC]: Implement unit-testing using mtest ...................................................................... tests/: [RFC]: Implement unit-testing using mtest The following allows the use of a ultra minimal unit-testing framework and includes an example that exercises the libflashrom init self-check and tear-down. Some work is needed to get include paths into a healthy state so that inner logic can be instrumented however this sets the tone. Change-Id: I18d68d3ace32b5be264aae2988a8ed84c779be43 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M meson.build A tests/meson.build A tests/selfcheck.c 3 files changed, 60 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/54/38954/1 diff --git a/meson.build b/meson.build index 375089c..8a998d0 100644 --- a/meson.build +++ b/meson.build @@ -402,3 +402,10 @@ ) subdir('util') + +# unit-test framework +munit_dep = dependency( + 'munit', + fallback: ['munit', 'munit_dep'] +) +subdir('tests') diff --git a/tests/meson.build b/tests/meson.build new file mode 100644 index 0000000..af65a81 --- /dev/null +++ b/tests/meson.build @@ -0,0 +1,12 @@ +root_includes = include_directories(['../', '../subprojects']) + +srcs = files('selfcheck.c') + +flashrom_tests = executable('flashrom_unit_tests', + srcs, + include_directories : root_includes, + dependencies : munit_dep, + link_with : flashrom +) + +test('munit test flashrom', flashrom_tests) diff --git a/tests/selfcheck.c b/tests/selfcheck.c new file mode 100644 index 0000000..abe88b1 --- /dev/null +++ b/tests/selfcheck.c @@ -0,0 +1,41 @@ +#include <stdlib.h> + +#include "munit/munit.h" + +#include "libflashrom.h" + + +MunitResult self_check_test(const MunitParameter params[], void* user_data_or_fixture) +{ + //Programmer table miscompilation! + munit_assert(flashrom_init(1 /*perform_selfcheck*/) == 0); + munit_assert(flashrom_shutdown() == 0); + + return MUNIT_OK; +} + +MunitTest tests[] = { + { + (char *)"/self_check", /* name */ + self_check_test, /* test */ + NULL, /* setup */ + NULL, /* tear_down */ + MUNIT_TEST_OPTION_NONE, /* options */ + NULL /* parameters */ + }, + /* Mark the end of the array with an entry where the test + * function is NULL */ + { NULL, NULL, NULL, NULL, MUNIT_TEST_OPTION_NONE, NULL } +}; + +static const MunitSuite suite = { + (char *)"/basic", /* name */ + tests, /* tests */ + NULL, /* suites */ + 1, /* iterations */ + MUNIT_SUITE_OPTION_NONE /* options */ +}; + +int main (int argc, char* argv[]) { + return munit_suite_main(&suite, (char*)"flashrom", argc, argv); +} -- To view, visit https://review.coreboot.org/c/flashrom/+/38954 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I18d68d3ace32b5be264aae2988a8ed84c779be43 Gerrit-Change-Number: 38954 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 2

Change in flashrom[master]: programmer: Introduce programmer alias mechanism
by Edward O'Callaghan (Code Review) 27 Oct '20

27 Oct '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/38671 ) Change subject: programmer: Introduce programmer alias mechanism ...................................................................... programmer: Introduce programmer alias mechanism Originally introduced in the downstream commit ba0827acee4da6740133345f43aae386923bbaad . This was originally authored by David Hendricks so that the various flash peripheral in ChromeOS devices can be flashed. For example, flashrom -p ec -r ec.bin flashrom -p host -r host.bin Note that this requires, `CONFIG_DEFAULT_PROGRAMMER=PROGRAMMER_INTERNAL make` to be defined as build-time. BUG=b:148755493 BRANCH=none TEST=ran ./flashrom -p host --flash-name Change-Id: I73766451cd8900a9e0fe08efc7a4d81b2a35ac8d Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M cli_classic.c M it85spi.c M linux_mtd.c M linux_spi.c M programmer.c M programmer.h 6 files changed, 98 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/71/38671/1 diff --git a/cli_classic.c b/cli_classic.c index 73cc417..d58562b 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -329,13 +329,51 @@ break; } } - if (prog == PROGRAMMER_INVALID) { + + for (i = 0; aliases[i].name; i++) { + name = aliases[i].name; + namelen = strlen(aliases[i].name); + + if (strncmp(optarg, name, namelen)) + continue; + + switch (optarg[namelen]) { + case ':': + pparam = strdup(optarg + namelen + 1); + if (!strlen(pparam)) { + free(pparam); + pparam = NULL; + } + break; + case '\0': + break; + default: + /* The continue refers to the for-loop. + * It is here to be able to + * differentiate between foo and foobar. + */ + continue; + } + + alias = &aliases[i]; + msg_gdbg("Programmer alias: \"%s\", parameter: " + " \"%s\",\n", alias->name, pparam); + break; + } + + if ((prog == PROGRAMMER_INVALID) && !alias) { fprintf(stderr, "Error: Unknown programmer \"%s\". Valid choices are:\n", optarg); list_programmers_linebreak(0, 80, 0); msg_ginfo(".\n"); cli_classic_abort_usage(NULL); } + + if ((prog != PROGRAMMER_INVALID) && alias) { + fprintf(stderr, "Error: Alias cannot be used " + "with programmer name.\n"); + cli_classic_abort_usage(NULL); + } break; case 'R': /* print_version() is always called during startup. */ diff --git a/it85spi.c b/it85spi.c index 5ce9193..47e2337 100644 --- a/it85spi.c +++ b/it85spi.c @@ -289,6 +289,9 @@ { int ret; + if (!programming_ec()) + return 1; + if (!(internal_buses_supported & BUS_FWH)) { msg_pdbg("%s():%d buses not support FWH\n", __func__, __LINE__); return 1; diff --git a/linux_mtd.c b/linux_mtd.c index d2df95e..bee7cc5 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -363,6 +363,9 @@ int dev_num = 0; int ret = 1; + if (!programming_host()) + return 1; + param = extract_programmer_param("dev"); if (param) { char *endptr; diff --git a/linux_spi.c b/linux_spi.c index aa73c18..221a36e 100644 --- a/linux_spi.c +++ b/linux_spi.c @@ -78,6 +78,14 @@ const uint8_t mode = SPI_MODE_0; const uint8_t bits = 8; + /* + * FIXME: There might be other programmers with flash memory (such as + * an EC) connected via SPI. For now we rely on the device's driver to + * distinguish it and assume generic SPI implies host. + */ + if (!programming_host()) + return 1; + p = extract_programmer_param("spispeed"); if (p && strlen(p)) { speed_hz = (uint32_t)strtoul(p, &endp, 10) * 1000; diff --git a/programmer.c b/programmer.c index f4b4384..ffad784 100644 --- a/programmer.c +++ b/programmer.c @@ -128,6 +128,13 @@ return 0; } +struct programmer_alias aliases[] = { + { "ec", ALIAS_EC }, + { "host", ALIAS_HOST }, + { NULL }, +}; +struct programmer_alias *alias; + enum chipbustype get_buses_supported(void) { int i; diff --git a/programmer.h b/programmer.h index 3cf53b9..a7bd947 100644 --- a/programmer.h +++ b/programmer.h @@ -130,6 +130,44 @@ PROGRAMMER_INVALID /* This must always be the last entry. */ }; +enum alias_type { + ALIAS_NONE = 0, /* no alias (default) */ + ALIAS_EC, /* embedded controller */ + ALIAS_HOST, /* chipset / PCH / SoC / etc. */ +}; + +struct programmer_alias { + const char *name; + enum alias_type type; +}; + +extern struct programmer_alias *alias; +extern struct programmer_alias aliases[]; + +/** + * This function returns 'true' if current flashrom invocation is programming + * NONE. + */ +static inline int programming_none(void) { + return alias && (alias->type == ALIAS_NONE); +} + +/** + * This function returns 'true' if current flashrom invocation is programming + * the EC. + */ +static inline int programming_ec(void) { + return alias && (alias->type == ALIAS_EC); +} + +/** + * This function returns 'true' if current flashrom invocation is programming + * the HOST. + */ +static inline int programming_host(void) { + return alias && (alias->type == ALIAS_HOST); +} + enum programmer_type { PCI = 1, /* to detect uninitialized values */ USB, -- To view, visit https://review.coreboot.org/c/flashrom/+/38671 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I73766451cd8900a9e0fe08efc7a4d81b2a35ac8d Gerrit-Change-Number: 38671 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

4 10

Change in flashrom[master]: spi: Allow cached ID bytes to be cleared
by Edward O'Callaghan (Code Review) 26 Aug '20

26 Aug '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/41399 ) Change subject: spi: Allow cached ID bytes to be cleared ...................................................................... spi: Allow cached ID bytes to be cleared This does two things: 1. Removes the static variables from various ID functions and moves them to a single struct. 2. Adds a function to clear the struct. The idea of the original caching mechanism introduced years ago was simply to speed up probe_flash() by not issuing the same read ID commands dozens or hundreds of times for a single chip. That implementation assumed that we would only call probe_flash() once. However, there are cases when we want to call probe_flash() multiple times, for example, if using an external programmer and using different voltages (for probing the chip). This patch is extremely similar to the original works of: `commit 57b7524b1448189d3630d6c4735e60dbbdf14d51`. Author: David Hendricks <dhendrix(a)chromium.org> && `commit 7f7c711ee1a41649607f81f1533e5135fc1361fc`. Author: David Hendricks <dhendrix(a)chromium.org> BUG=b:15656443 BRANCH=none TEST=none Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M spi.h M spi25.c 2 files changed, 50 insertions(+), 26 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/99/41399/1 diff --git a/spi.h b/spi.h index 3f45038..c1ca46a 100644 --- a/spi.h +++ b/spi.h @@ -192,4 +192,6 @@ #define SPI_FLASHROM_BUG -5 #define SPI_PROGRAMMER_ERROR -6 +void clear_spi_id_cache(void); + #endif /* !__SPI_H__ */ diff --git a/spi25.c b/spi25.c index fb91903..b4fbe6d 100644 --- a/spi25.c +++ b/spi25.c @@ -27,6 +27,26 @@ #include "programmer.h" #include "spi.h" +enum id_type { + RDID, + RDID4, + REMS, + RES2, + RES3, + NUM_ID_TYPES, +}; + +static struct { + int is_cached; + unsigned char bytes[4]; /* enough to hold largest ID type */ +} id_cache[NUM_ID_TYPES]; + +void clear_spi_id_cache(void) +{ + memset(id_cache, 0, sizeof(id_cache)); + return; +} + static int spi_rdid(struct flashctx *flash, unsigned char *readarr, int bytes) { static const unsigned char cmd[JEDEC_RDID_OUTSIZE] = { JEDEC_RDID }; @@ -138,18 +158,20 @@ static int probe_spi_rdid_generic(struct flashctx *flash, int bytes) { const struct flashchip *chip = flash->chip; - unsigned char readarr[4]; uint32_t id1; uint32_t id2; + enum id_type idty = bytes == 3 ? RDID : RDID4; - const int ret = spi_rdid(flash, readarr, bytes); - if (ret == SPI_INVALID_LENGTH) - msg_cinfo("%d byte RDID not supported on this SPI controller\n", bytes); - if (ret) - return 0; + if (!id_cache[idty].is_cached) { + const int ret = spi_rdid(flash, id_cache[idty].bytes, bytes); + if (ret == SPI_INVALID_LENGTH) + msg_cinfo("%d byte RDID not supported on this SPI controller\n", bytes); + if (ret) + return 0; + id_cache[idty].is_cached = 1; + } - rdid_get_ids(readarr, bytes, &id1, &id2); - + rdid_get_ids(id_cache[idty].bytes, bytes, &id1, &id2); return compare_id(chip, id1, id2); } @@ -166,16 +188,16 @@ int probe_spi_rems(struct flashctx *flash) { const struct flashchip *chip = flash->chip; - unsigned char readarr[JEDEC_REMS_INSIZE]; uint32_t id1, id2; - if (spi_rems(flash, readarr)) { - return 0; + if (!id_cache[REMS].is_cached) { + if (spi_rems(flash, id_cache[REMS].bytes)) + return 0; + id_cache[REMS].is_cached = 1; } - id1 = readarr[0]; - id2 = readarr[1]; - + id1 = id_cache[REMS].bytes[0]; + id2 = id_cache[REMS].bytes[1]; return compare_id(chip, id1, id2); } @@ -222,16 +244,16 @@ int probe_spi_res2(struct flashctx *flash) { - unsigned char readarr[2]; uint32_t id1, id2; - if (spi_res(flash, readarr, 2)) { - return 0; + if (!id_cache[RES2].is_cached) { + if (spi_res(flash, id_cache[RES2].bytes, 2)) + return 0; + id_cache[RES2].is_cached = 1; } - id1 = readarr[0]; - id2 = readarr[1]; - + id1 = id_cache[RES2].bytes[0]; + id2 = id_cache[RES2].bytes[1]; msg_cdbg("%s: id1 0x%x, id2 0x%x\n", __func__, id1, id2); if (id1 != flash->chip->manufacture_id || id2 != flash->chip->model_id) @@ -242,16 +264,16 @@ int probe_spi_res3(struct flashctx *flash) { - unsigned char readarr[3]; uint32_t id1, id2; - if (spi_res(flash, readarr, 3)) { - return 0; + if (!id_cache[RES3].is_cached) { + if (spi_res(flash, id_cache[RES3].bytes, 3)) + return 0; + id_cache[RES3].is_cached = 1; } - id1 = (readarr[0] << 8) | readarr[1]; - id2 = readarr[2]; - + id1 = (id_cache[RES3].bytes[0] << 8) | id_cache[RES3].bytes[1]; + id2 = id_cache[RES3].bytes[3]; msg_cdbg("%s: id1 0x%x, id2 0x%x\n", __func__, id1, id2); if (id1 != flash->chip->manufacture_id || id2 != flash->chip->model_id) -- To view, visit https://review.coreboot.org/c/flashrom/+/41399 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I879cb08dbe66db9ab0c3b8a7f93b04fe1c5980f4 Gerrit-Change-Number: 41399 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

2 4

Change in flashrom[master]: spi25.c: Factor out rdid_get_ids() and compare_id()
by Edward O'Callaghan (Code Review) 26 Aug '20

26 Aug '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/41398 ) Change subject: spi25.c: Factor out rdid_get_ids() and compare_id() ...................................................................... spi25.c: Factor out rdid_get_ids() and compare_id() This is in preparation for implementing a cache for the probe results of RDID and REMS (3&4-byte variant) commands. The intention is to make probing of SPI rom's slightly faster, a few 10's of ms dependant upon the spi master used. BUG=b:15656443 BRANCH=none TEST=builds Change-Id: I1556e97a7c70425069e3d1dc0d5daf0aeec4e7bf Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M spi25.c 1 file changed, 30 insertions(+), 32 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/98/41398/1 diff --git a/spi25.c b/spi25.c index 0a939e7..fb91903 100644 --- a/spi25.c +++ b/spi25.c @@ -93,19 +93,9 @@ return spi_send_command(flash, sizeof(cmd), 0, cmd, NULL); } -static int probe_spi_rdid_generic(struct flashctx *flash, int bytes) +static void rdid_get_ids(unsigned char *readarr, int bytes, + uint32_t *id1, uint32_t *id2) { - const struct flashchip *chip = flash->chip; - unsigned char readarr[4]; - uint32_t id1; - uint32_t id2; - - const int ret = spi_rdid(flash, readarr, bytes); - if (ret == SPI_INVALID_LENGTH) - msg_cinfo("%d byte RDID not supported on this SPI controller\n", bytes); - if (ret) - return 0; - if (!oddparity(readarr[0])) msg_cdbg("RDID byte 0 parity violation. "); @@ -115,17 +105,20 @@ if (readarr[0] == 0x7f) { if (!oddparity(readarr[1])) msg_cdbg("RDID byte 1 parity violation. "); - id1 = (readarr[0] << 8) | readarr[1]; - id2 = readarr[2]; + *id1 = (readarr[0] << 8) | readarr[1]; + *id2 = readarr[2]; if (bytes > 3) { - id2 <<= 8; - id2 |= readarr[3]; + *id2 <<= 8; + *id2 |= readarr[3]; } } else { - id1 = readarr[0]; - id2 = (readarr[1] << 8) | readarr[2]; + *id1 = readarr[0]; + *id2 = (readarr[1] << 8) | readarr[2]; } +} +static int compare_id(const struct flashchip *chip, uint32_t id1, uint32_t id2) +{ msg_cdbg("%s: id1 0x%02x, id2 0x%02x\n", __func__, id1, id2); if (id1 == chip->manufacture_id && id2 == chip->model_id) @@ -142,6 +135,24 @@ return 0; } +static int probe_spi_rdid_generic(struct flashctx *flash, int bytes) +{ + const struct flashchip *chip = flash->chip; + unsigned char readarr[4]; + uint32_t id1; + uint32_t id2; + + const int ret = spi_rdid(flash, readarr, bytes); + if (ret == SPI_INVALID_LENGTH) + msg_cinfo("%d byte RDID not supported on this SPI controller\n", bytes); + if (ret) + return 0; + + rdid_get_ids(readarr, bytes, &id1, &id2); + + return compare_id(chip, id1, id2); +} + int probe_spi_rdid(struct flashctx *flash) { return probe_spi_rdid_generic(flash, 3); @@ -165,20 +176,7 @@ id1 = readarr[0]; id2 = readarr[1]; - msg_cdbg("%s: id1 0x%x, id2 0x%x\n", __func__, id1, id2); - - if (id1 == chip->manufacture_id && id2 == chip->model_id) - return 1; - - /* Test if this is a pure vendor match. */ - if (id1 == chip->manufacture_id && GENERIC_DEVICE_ID == chip->model_id) - return 1; - - /* Test if there is any vendor ID. */ - if (GENERIC_MANUF_ID == chip->manufacture_id && id1 != 0xff && id1 != 0x00) - return 1; - - return 0; + return compare_id(chip, id1, id2); } int probe_spi_res1(struct flashctx *flash) -- To view, visit https://review.coreboot.org/c/flashrom/+/41398 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1556e97a7c70425069e3d1dc0d5daf0aeec4e7bf Gerrit-Change-Number: 41398 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

3 4

Change in ...flashrom[master]: dediprog: Use 4BA page program without 0x12
by David Hendricks (Code Review) 25 Aug '20

25 Aug '20

David Hendricks has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/30521 Change subject: dediprog: Use 4BA page program without 0x12 ...................................................................... dediprog: Use 4BA page program without 0x12 It's not clear exactly what the difference is between WRITE_MODE_4B_ADDR_256B_PAGE_PGM with and without _0x12 from the SF600 command spec, but the former seems to work more reliably. Tested using an SF600 (FW v7.2.21), IS25LP256D, W25Q256JV, and MX25L25735. Change-Id: I3cb37ec8838f5bb02948ed52b4a2906fa033cf83 Signed-off-by: David Hendricks <david.hendricks(a)gmail.com> --- M dediprog.c 1 file changed, 2 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/30521/1 diff --git a/dediprog.c b/dediprog.c index 2a190d2..303f1d5 100644 --- a/dediprog.c +++ b/dediprog.c @@ -398,7 +398,8 @@ } } else if (dedi_spi_cmd == WRITE_MODE_PAGE_PGM) { if (flash->chip->feature_bits & FEATURE_4BA_WRITE) { - data_packet[3] = WRITE_MODE_4B_ADDR_256B_PAGE_PGM_0x12; +// data_packet[3] = WRITE_MODE_4B_ADDR_256B_PAGE_PGM_0x12; + data_packet[3] = WRITE_MODE_4B_ADDR_256B_PAGE_PGM; data_packet[4] = JEDEC_BYTE_PROGRAM_4BA; use_4ba = true; } else if (flash->in_4ba_mode) { -- To view, visit https://review.coreboot.org/c/flashrom/+/30521 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3cb37ec8838f5bb02948ed52b4a2906fa033cf83 Gerrit-Change-Number: 30521 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: newchange

1 1

Change in flashrom[master]: udelay.c: Enable providing delay calibration value through CLI
by Hrvoje Čavrak (Code Review) 17 Aug '20

17 Aug '20

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 <hrvoje(a)hrvoje.org> --- 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) { -- To view, visit https://review.coreboot.org/c/flashrom/+/39841 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iea2a7f62300663bc0a32ed4abced57c8c55c90c8 Gerrit-Change-Number: 39841 Gerrit-PatchSet: 1 Gerrit-Owner: Hrvoje Čavrak <github(a)hrvoje.org> Gerrit-MessageType: newchange

3 7

Change in flashrom[master]: lspcon_i2c_spi.c: Add debug message that shows the active block.
by Shiyu Sun (Code Review) 05 Aug '20

05 Aug '20

Shiyu Sun has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/41779 ) Change subject: lspcon_i2c_spi.c: Add debug message that shows the active block. ...................................................................... lspcon_i2c_spi.c: Add debug message that shows the active block. This is all for debug propose to verify A/B update work as expected. BUG=b:148746232 BRANCH=none TEST=flashrom -p lspcon_i2c_spi:bus=7 --flash-size -V verified the related message appears. Signed-off-by: Shiyu Sun <sshiyu(a)chromium.org> Change-Id: I1b3022542f98f114097c6f39254ef4c2cf188c90 --- M lspcon_i2c_spi.c 1 file changed, 25 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/41779/1 diff --git a/lspcon_i2c_spi.c b/lspcon_i2c_spi.c index 7b9f1c0..e5e5dc9 100644 --- a/lspcon_i2c_spi.c +++ b/lspcon_i2c_spi.c @@ -27,6 +27,7 @@ #define REGISTER_ADDRESS (0x94 >> 1) #define PAGE_ADDRESS (0x9e >> 1) +#define ACTIVE_BLOCK_ADDRESS (0x9a >> 1) #define PAGE_SIZE 256 #define MAX_SPI_WAIT_RETRIES 1000 @@ -472,6 +473,27 @@ return ret; } +static void lspcon_i2c_spi_boot_block_info(int fd) +{ + uint8_t block_id = 0; + int ret = 0; + ret |= lspcon_i2c_spi_write_register(fd, 0xbc, 0xc0); + ret |= lspcon_i2c_spi_write_register(fd, 0xbc, 0x40); + ret |= lspcon_i2c_spi_write_register(fd, 0x8e, 0x00); + ret |= lspcon_i2c_spi_write_register(fd, 0x8f, 0x00); + struct timespec wait_10ms = { 0, (unsigned)1e7 }; + nanosleep(&wait_10ms, NULL); + + uint8_t command[] = { 0x0e }; + ret |= lspcon_i2c_spi_write_data(fd, ACTIVE_BLOCK_ADDRESS, command, 1); + ret |= lspcon_i2c_spi_read_data(fd, ACTIVE_BLOCK_ADDRESS, &block_id, 1); + if (ret) { + msg_pdbg("Failed to get LSPCON boot block information.\n"); + } else { + msg_pdbg("LSPCON is booting from block #%d.\n", block_id); + } +} + int lspcon_i2c_spi_init(void) { int lspcon_i2c_spi_bus = get_bus(); @@ -489,6 +511,9 @@ return ret; } + /* Show current activate block information for debug propose. */ + lspcon_i2c_spi_boot_block_info(fd); + struct lspcon_i2c_spi_data *data = calloc(1, sizeof(struct lspcon_i2c_spi_data)); if (!data) { msg_perr("Unable to allocate space for extra SPI master data.\n"); -- To view, visit https://review.coreboot.org/c/flashrom/+/41779 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1b3022542f98f114097c6f39254ef4c2cf188c90 Gerrit-Change-Number: 41779 Gerrit-PatchSet: 1 Gerrit-Owner: Shiyu Sun <sshiyu(a)google.com> Gerrit-MessageType: newchange

5 8

Change in flashrom[master]: raiden_debug_spi.c: Adds support for USB SPI protocol V2
by Brian Nemec (Code Review) 21 Jul '20

21 Jul '20

Hello Brian Nemec, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/41533 to review the following change. Change subject: raiden_debug_spi.c: Adds support for USB SPI protocol V2 ...................................................................... raiden_debug_spi.c: Adds support for USB SPI protocol V2 Adds support for the USB SPI V2 protocol and documentation of it. The protocol version number uses the bInterfaceProtocol field in USB to identify which device to use, this enables us to support both V1 and V2 with the same host. The USB SPI V2 protocol adds the ability to perform multi-packet USB SPI transfers. This results in fewer USB messages exchanged and faster flashing speeds. BUG=b:139058552 BRANCH=none TEST=Manual testing of ServoMicro and Flashrom when performing reads, writes, and verification of the EC firmware on Nami with a USB SPI V1 protocol device TEST=Manual testing of ServoMicro and Flashrom when performing reads, writes, and verification of the EC firmware on Nami with a USB SPI V2 protocol device TEST=Builds Signed-off-by: Brian J. Nemec <bnemec(a)chromium.com> Change-Id: Ie356c63b521c0cc11a4946ffac128ec7139f0bec --- M raiden_debug_spi.c 1 file changed, 470 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/41533/1 diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index 6cb32b3..5e07967 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -48,7 +48,10 @@ * include/usb_spi.h * common/usb_spi.c * - * bInterfaceProtocol determines which protocol is used by the USB SPI device. + * 2 Versions of the protocol exist in deployed devices: + * The raiden_debug_spi.c interface used will switch between them automatically + * depending on the protocol version reported in the bInterfaceProtocol + * * * * USB SPI Version 1: @@ -117,8 +120,191 @@ * 0x20001-0x20063 Lower bits store the positive value representation * of the libusb_error enum. See the libusb documentation: * http://libusb.sourceforge.net/api-1.0/group__misc.html + * + * + * + * USB SPI Version 2: + * + * USB SPI version 2 adds support for larger SPI transfers and reduces the + * number of USB packets transferred. This improves performance when + * writing or reading large chunks of memory from a device. A packet ID + * field is used to distinguish the different packet types. Additional + * packets have been included to query the device for it's configuration + * allowing the interface to be used on platforms with different SPI + * limitations. It includes validation and a packet to recover from the + * situations where USB packets are lost. + * + * + * Example: USB SPI request with 128 byte write and 0 byte read. + * + * Packet #1 Host to Device: + * packet id = PACKET_ID_V2_COMMAND + * write count = 128 + * read count = 0 + * payload = First 58 bytes from the write buffer, + * start is byte 0 + * + * Packet #2 Host to Device: + * packet id = PACKET_ID_V2_COMMAND_CONTINUE + * data index = 58 + * payload = next 60 bytes from the write buffer, + * starting at byte 58 + * + * Packet #3 Host to Device: + * packet id = PACKET_ID_V2_COMMAND_CONTINUE + * data index = 118 + * payload = next 10 bytes from the write buffer, + * starting at byte 118 + * + * Packet #4 Device to Host: + * packet id = PACKET_ID_V2_RESPONSE + * status code = status code from device + * payload = 0 bytes + * + * Command Packet (Host to Device): + * + * Start of USB SPI command, containing the number of bytes to write and + * read and a payload of bytes to write. If the payload is unable to fit + * in one USB packet, it contains the first 58 bytes. + * + * +----------------+------------------+-----------------+------------------------+ + * | packet id : 2B | write count : 2B | read count : 2B | write payload : <= 58B | + * +----------------+------------------+-----------------+------------------------+ + * + * packet id: 2 byte enum defined by packet_id_type + * Valid values packet id = PACKET_ID_V2_COMMAND + * + * write count: 2 byte, zero based count of bytes to write + * + * read count: 2 byte, zero based count of bytes to read + * + * write payload: Up to 62 bytes of data to write to SPI, the total + * length of all TX packets must match write count. + * Due to data alignment constraints, this must be an + * even number of bytes unless this is the final packet. + * + * + * Response Packet (Device to Host): + * + * Start of the USB SPI response, containing the status code and + * any bytes read in the payload. If read buffer can not fit in + * a single packet, it represents the first 60 bytes. + * + * +----------------+------------------+-----------------------+ + * | packet id : 2B | status code : 2B | read payload : <= 60B | + * +----------------+------------------+-----------------------+ + * + * packet id: 2 byte enum defined by packet_id_type + * Valid values packet id = PACKET_ID_V2_RESPONSE + * + * status code: 2 byte status code + * 0x0000: Success + * 0x0001: SPI timeout + * 0x0002: Busy, try again + * This can happen if someone else has acquired the shared memory + * buffer that the SPI driver uses as /dev/null + * 0x0003: Write count invalid. The byte limit is platform specific + * and is set during the configure USB SPI response. + * 0x0004: Read count invalid. The byte limit is platform specific + * and is set during the configure USB SPI response. + * 0x0005: The SPI bridge is disabled. + * 0x0006: The RX continue packet's data index is invalid. This + * can indicate a USB transfer failure to the device. + * 0x0007: The RX endpoint has received more data than write count. + * This can indicate a USB transfer failure to the device. + * 0x0008: An unexpected packet arrived that the device could not + * process. + * 0x8000: Unknown error mask + * The bottom 15 bits will contain the bottom 14 bits from the EC + * error code. + * + * read payload: Up to 62 bytes of data read from SPI, the total + * length of all RX packets must match read count + * unless an error status was returned. Due to data + * alignment constraints, this must be a even number + * of bytes unless this is the final packet. + * + * + * Continue Packet (Bidirectional): + * + * Continuation packet for the writes and read buffers. Both packets + * follow the same format, a data index counts the number of bytes + * previously transferred in the USB SPI transfer and a payload of bytes. + * + * +----------------+-----------------+-------------------------------+ + * | packet id : 2B | data index : 2B | write / read payload : <= 60B | + * +----------------+-----------------+-------------------------------+ + * + * packet id: 2 byte enum defined by packet_id_type + * The packet id has 2 values depending on direction: + * packet id = PACKET_ID_V2_COMMAND_CONTINUE indicates the + * packet is being transmitted from the host to the device + * packet id = PACKET_ID_V2_RESPONSE_CONTINUE indicates the + * packet is being transmitted from the device to the host. + * + * data index: The data index indicates the number of bytes in the + * read or write buffers have already been transmitted. + * It is used to validate that no packets have been dropped + * and that the prior packets have been correctly decoded. + * this value corresponds to the position in the destination + * destination to start copying the payload into. + * + * read and write payload: + * Contains up to 60 bytes of payload data to transfer to + * the SPI write buffer or from the SPI read buffer. + * + * + * Command Get Configuration Packet (Host to Device): + * + * Query the device to request it's USB SPI configuration indicating + * the number of bytes it can write and read. + * + * +----------------+ + * | packet id : 2B | + * +----------------+ + * + * packet id: 2 byte enum PACKET_ID_V2_COMMAND_GET_USB_SPI_CONFIG + * + * Response Configuration Packet (Device to Host): + * + * Response packet form the device to report the maximum write and + * read size supported by the device. + * + * +----------------+----------------------+---------------------+ + * | packet id : 2B | max write count : 2B | max read count : 2B | + * +----------------+----------------------+---------------------+ + * + * packet id: 2 byte enum PACKET_ID_V2_COMMAND_GET_USB_SPI_CONFIG + * + * max write count : 2 byte count of the maximum number of bytes + * the device can write to SPI in one transaction. + * + * max read count : 2 byte count of the maximum number of bytes + * the device can read from SPI in one transaction. + * + * Command Restart Response Packet (Host to Device): + * + * Command to restart the response transfer from the device. This enables + * the host to recover from a lost packet when reading the response + * without restarting the SPI transfer. + * + * +----------------+ + * | packet id : 2B | + * +----------------+ + * + * packet id: 2 byte enum PACKET_ID_V2_COMMAND_GET_USB_SPI_CONFIG + * + * send_command return codes have the following format: + * + * 0x00000: Status code success. + * 0x00001-0x0FFFF: Error code returned by the USB SPI device. + * 0x10001-0x1FFFF: USB SPI Host error codes + * 0x20001-0x20063 Lower bits store the positive value representation + * of the libusb_error enum. See the libusb documentation: + * http://libusb.sourceforge.net/api-1.0/group__misc.html */ + #include "programmer.h" #include "spi.h" #include "usb_device.h" @@ -138,6 +324,7 @@ #define GOOGLE_VID (0x18D1) #define GOOGLE_RAIDEN_SPI_SUBCLASS (0x51) #define GOOGLE_RAIDEN_SPI_PROTOCOL_V1 (0x01) +#define GOOGLE_RAIDEN_SPI_PROTOCOL_V2 (0x02) enum { /* The host failed to transfer the data with no libusb error. */ @@ -185,6 +372,7 @@ */ #define WRITE_RETRY_ATTEMPTS (3) #define READ_RETRY_ATTEMPTS (3) +#define GET_CONFIG_RETRY_ATTEMPTS (3) #define RETRY_INTERVAL_US (100 * 1000) /* @@ -200,6 +388,17 @@ * All of the USB SPI packets have size equal to the max USB packet size of 64B */ #define PAYLOAD_SIZE_V1 (62) +#define PACKET_HEADER_SIZE (2) + +#define HOST_VERSION (1) + +#define PAYLOAD_SIZE_V2_START (58) + +#define PAYLOAD_SIZE_V2_RESPONSE (60) + +#define PAYLOAD_SIZE_V2_CONTINUE (60) + +#define PAYLOAD_SIZE_V2_ERROR (60) #define SPI_TRANSFER_V1_MAX (PAYLOAD_SIZE_V1) @@ -224,10 +423,86 @@ usb_spi_response_v1_t response; } __attribute__((packed)) usb_spi_packet_v1_t; +/* + * Version 2 protocol specific attributes + */ + +enum packet_id_type { + + /* Request USB SPI configuration data from device. */ + PACKET_ID_V2_COMMAND_GET_USB_SPI_CONFIG = 0, + /* USB SPI configuration data from device. */ + PACKET_ID_V2_RESPONSE_USB_SPI_CONFIG = 1, + + /* Start a USB SPI transfer and deliver first packet of data to write. */ + PACKET_ID_V2_COMMAND = 2, + /* Additional packets containing write payload. */ + PACKET_ID_V2_COMMAND_CONTINUE = 3, + + /* + * Request the device restart the response enabling us to recover from + * packet loss without another SPI transfer. + */ + PACKET_ID_V2_COMMAND_RESTART_RESPONSE = 4, + + /* First packet of USB SPI response with status code and read payload. */ + PACKET_ID_V2_RESPONSE = 5, + /* Additional packets containing read payload. */ + PACKET_ID_V2_RESPONSE_CONTINUE = 6, +}; + +typedef struct { + int16_t packet_id; +} __attribute__((packed)) usb_spi_command_get_configuration_v2_t; + +typedef struct { + int16_t packet_id; + uint16_t max_write_count; + uint16_t max_read_count; +} __attribute__((packed)) usb_spi_response_configuration_v2_t; + +typedef struct { + int16_t packet_id; + int16_t write_count; + /* -1 Indicates readback all on halfduplex compliant devices. */ + int16_t read_count; + uint8_t data[PAYLOAD_SIZE_V2_START]; +} __attribute__((packed)) usb_spi_command_v2_t; + +typedef struct { + int16_t packet_id; + uint16_t status_code; + uint8_t data[PAYLOAD_SIZE_V2_RESPONSE]; +} __attribute__((packed)) usb_spi_response_v2_t; + +typedef struct { + int16_t packet_id; + uint16_t data_index; + uint8_t data[PAYLOAD_SIZE_V2_CONTINUE]; +} __attribute__((packed)) usb_spi_continue_v2_t; + +typedef struct { + int16_t packet_id; +} __attribute__((packed)) usb_spi_command_restart_response_v2_t; + +typedef struct { + union { + int16_t packet_id; + usb_spi_command_get_configuration_v2_t command_get_config; + usb_spi_response_configuration_v2_t response_config; + usb_spi_command_restart_response_v2_t restart_response; + usb_spi_command_v2_t command; + usb_spi_response_v2_t response; + usb_spi_continue_v2_t command_continue; + usb_spi_continue_v2_t response_continue; + }; +} __attribute__((packed)) usb_spi_packet_v2_t; + typedef struct { union { uint8_t bytes[MAX_USB_PACKET_SIZE]; usb_spi_packet_v1_t packet_v1; + usb_spi_packet_v2_t packet_v2; }; /* * By storing the number of bytes in the header and knowing that the @@ -427,6 +702,177 @@ return status; } +/* + * Version 2 Protocol + */ + +int get_spi_size_v2(struct raiden_debug_spi_data *ctx_data) +{ + int status; + usb_spi_packet_ctx_t response_config; + + usb_spi_packet_ctx_t command_get_config = { + .header_size = sizeof(usb_spi_command_get_configuration_v2_t), + .packet_size = sizeof(usb_spi_command_get_configuration_v2_t), + .packet_v2.packet_id = PACKET_ID_V2_COMMAND_GET_USB_SPI_CONFIG + }; + + for (int config_attempt = 0; config_attempt < GET_CONFIG_RETRY_ATTEMPTS; + config_attempt++) { + + status = transmit_packet(ctx_data, &command_get_config); + if (status) { + msg_perr("Raiden: Failed to transmit get config\n" + " config attempt = %d\n" + " status = %d\n", + config_attempt + 1, status); + programmer_delay(RETRY_INTERVAL_US); + continue; + } + + status = receive_packet(ctx_data, &response_config); + if (status) { + msg_perr("Raiden: Failed to receive packet\n" + " config attempt = %d\n" + " status = %d\n", + config_attempt + 1, status); + programmer_delay(RETRY_INTERVAL_US); + continue; + } + + /* + * Perform validation on the packet received to verify it is a valid + * configuration. If it is, we are ready to perform transfers. + */ + if ((response_config.packet_v2.packet_id == + PACKET_ID_V2_RESPONSE_USB_SPI_CONFIG) || + (response_config.packet_size == + sizeof(usb_spi_response_configuration_v2_t))) { + + /* Set the parameters from the configuration. */ + ctx_data->max_spi_write_count = + response_config.packet_v2.response_config.max_write_count; + ctx_data->max_spi_read_count = + response_config.packet_v2.response_config.max_read_count; + return status; + } + + msg_perr("Raiden: Packet is not a valid config\n" + " config attempt = %d\n" + " packet id = %d\n" + " packet size = %d\n", + config_attempt + 1, + response_config.packet_v2.packet_id, + response_config.packet_size); + programmer_delay(RETRY_INTERVAL_US); + } + return USB_SPI_HOST_INIT_FAILURE; +} + +int restart_response_v2(const struct raiden_debug_spi_data *ctx_data) +{ + int status; + + usb_spi_packet_ctx_t restart_response = { + .header_size = sizeof(usb_spi_command_restart_response_v2_t), + .packet_size = sizeof(usb_spi_command_restart_response_v2_t), + .packet_v2.packet_id = PACKET_ID_V2_COMMAND_RESTART_RESPONSE + }; + + status = transmit_packet(ctx_data, &restart_response); + return status; +} + + +int write_command_v2(const struct raiden_debug_spi_data * ctx_data, + usb_spi_transmit_ctx_t* write, + usb_spi_receive_ctx_t* read) +{ + int status; + usb_spi_packet_ctx_t continue_packet; + + usb_spi_packet_ctx_t start_usb_spi_packet = { + .header_size = offsetof(usb_spi_command_v2_t, data), + .packet_v2.command.packet_id = PACKET_ID_V2_COMMAND, + .packet_v2.command.write_count = write->transmit_size, + .packet_v2.command.read_count = read->receive_size + }; + + fill_usb_packet(&start_usb_spi_packet, write); + status = transmit_packet(ctx_data, &start_usb_spi_packet); + if (status) { + return status; + } + + while (write->transmit_index < write->transmit_size) { + /* Transmit any continue packets. */ + continue_packet.header_size = offsetof(usb_spi_continue_v2_t, data); + continue_packet.packet_v2.command_continue.packet_id = + PACKET_ID_V2_COMMAND_CONTINUE; + continue_packet.packet_v2.command_continue.data_index = + write->transmit_index; + + fill_usb_packet(&continue_packet, write); + + status = transmit_packet(ctx_data, &continue_packet); + if (status) { + return status; + } + } + return status; +} + +int read_response_v2(const struct raiden_debug_spi_data * ctx_data, + usb_spi_transmit_ctx_t* write, + usb_spi_receive_ctx_t* read) +{ + int status = -1; + usb_spi_packet_ctx_t response; + + /* Receive the payload to the servo micro. */ + while (read->receive_index < read->receive_size) { + + status = receive_packet(ctx_data, &response); + if (status) { + /* Return the transfer error. */ + return status; + } + if (response.packet_v2.packet_id == PACKET_ID_V2_RESPONSE) { + /* + * The host should only see this packet if an error occurs + * on the device or if it's the first response packet. + */ + if (response.packet_v2.response.status_code) { + return response.packet_v2.response.status_code; + } + if (read->receive_index) { + msg_perr("Raiden: Unexpected packet id = %d", + response.packet_v2.response.packet_id); + return USB_SPI_HOST_RX_UNEXPECTED_PACKET; + } + response.header_size = offsetof(usb_spi_response_v2_t, data); + } if (response.packet_v2.packet_id == + PACKET_ID_V2_RESPONSE_CONTINUE) { + + /* We validate that no packets were missed. */ + if (read->receive_index != + response.packet_v2.response_continue.data_index) { + return USB_SPI_HOST_RX_BAD_DATA_INDEX; + } + response.header_size = offsetof(usb_spi_continue_v2_t, data); + } else { + msg_perr("Raiden: Unexpected packet id = %d", + response.packet_v2.packet_id); + return USB_SPI_HOST_RX_UNEXPECTED_PACKET; + } + status = read_usb_packet(read, &response); + if (status) { + return status; + } + } + return status; +} + static int send_command(const struct flashctx *flash, unsigned int write_count, unsigned int read_count, @@ -464,6 +910,8 @@ write_ctx.transmit_index = 0; if (ctx_data->protocol_version == GOOGLE_RAIDEN_SPI_PROTOCOL_V1) { status = write_command_v1(ctx_data, &write_ctx, &read_ctx); + } else { + status = write_command_v2(ctx_data, &write_ctx, &read_ctx); } if (status) { @@ -490,6 +938,8 @@ read_ctx.receive_index = 0; if (ctx_data->protocol_version == GOOGLE_RAIDEN_SPI_PROTOCOL_V1) { status = read_response_v1(ctx_data, &write_ctx, &read_ctx); + } else { + status = read_response_v2(ctx_data, &write_ctx, &read_ctx); } if (status == 0) { @@ -513,6 +963,15 @@ /* Reattempting will not result in a recovery. */ return status; } + /* + * Protocol version 2 includes multi-packet messages, we can + * restart only the response operation without performing SPI + * transfer. + */ + if (ctx_data->protocol_version == + GOOGLE_RAIDEN_SPI_PROTOCOL_V2) { + restart_response_v2(ctx_data); + } programmer_delay(RETRY_INTERVAL_US); } else { /* We were successful at performing the SPI transfer. */ @@ -579,6 +1038,7 @@ static int configure_protocol(struct spi_master *ctx_spi) { + int status = 0; struct raiden_debug_spi_data *ctx_data = (struct raiden_debug_spi_data *)ctx_spi->data; @@ -593,6 +1053,15 @@ ctx_data->max_spi_write_count= SPI_TRANSFER_V1_MAX; ctx_data->max_spi_read_count = SPI_TRANSFER_V1_MAX; break; + case GOOGLE_RAIDEN_SPI_PROTOCOL_V2: + /* + * Protocol V2 requires the host to query the device for + * it's maximum read and write sizes + */ + status = get_spi_size_v2(ctx_data); + if (status) { + return status; + } default: msg_pdbg("Raiden: Unknown USB SPI protocol version = %d", ctx_data->protocol_version); @@ -692,7 +1161,6 @@ usb_match_value_default(&match.vid, GOOGLE_VID); usb_match_value_default(&match.class, LIBUSB_CLASS_VENDOR_SPEC); usb_match_value_default(&match.subclass, GOOGLE_RAIDEN_SPI_SUBCLASS); - usb_match_value_default(&match.protocol, GOOGLE_RAIDEN_SPI_PROTOCOL_V1); ret = LIBUSB(libusb_init(NULL)); if (ret != 0) { -- To view, visit https://review.coreboot.org/c/flashrom/+/41533 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie356c63b521c0cc11a4946ffac128ec7139f0bec Gerrit-Change-Number: 41533 Gerrit-PatchSet: 1 Gerrit-Owner: Brian Nemec <bnemec(a)google.com> Gerrit-Reviewer: Brian Nemec <bnemec(a)chromium.org> Gerrit-MessageType: newchange

3 34

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2020