September 2021 - flashrom-gerrit

Change in flashrom[master]: ich_descriptors: Increase ISL ranges that can be checked
by David Hendricks (Code Review) 14 Sep '21

14 Sep '21

David Hendricks has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/57633 ) Change subject: ich_descriptors: Increase ISL ranges that can be checked ...................................................................... ich_descriptors: Increase ISL ranges that can be checked This patch does the following: - Checks for Ibex Peak more explicitly, as ifdtool currently does. - Add check for EBG/LBG, which has a much larger ISL/PSL value than other chipsets currently listed. - Unconditionally print the "peculiar flash descriptor" warning if none of the chipsets are detected explicitly, since checking `!= 16` is redundant with the above checks. Change-Id: I2b6301c6cd08f6822340d61a3454b31fddc3be52 Signed-off-by: David Hendricks <ddaveh(a)amazon.com> --- M ich_descriptors.c 1 file changed, 10 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/57633/1 diff --git a/ich_descriptors.c b/ich_descriptors.c index aba7528..7589510 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -933,13 +933,22 @@ return CHIPSET_ICH9; if (content->ISL <= 10) return CHIPSET_ICH10; + if (content->ISL <= 16) + return CHIPSET_5_SERIES_IBEX_PEAK; if (content->FLMAP2 == 0) { if (content->ISL == 19) return CHIPSET_APOLLO_LAKE; warn_peculiar_desc(content->ISL != 23, "Gemini Lake"); return CHIPSET_GEMINI_LAKE; } - warn_peculiar_desc(content->ISL != 16, "Ibex Peak"); + /* Emmitsburg has 80, Lewisburg on Purley and Whitley have 139 */ + if (content->ISL <= 139) + return CHIPSET_C620_SERIES_LEWISBURG; + /* + * Ibex Peak is historically used as the default when other + * chipsets are not identified. + */ + warn_peculiar_desc(true, "Ibex Peak"); return CHIPSET_5_SERIES_IBEX_PEAK; } else if (upper->MDTBA == 0x00) { if (content->ICCRIBA < 0x31 && content->FMSBA < 0x30) { -- To view, visit https://review.coreboot.org/c/flashrom/+/57633 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2b6301c6cd08f6822340d61a3454b31fddc3be52 Gerrit-Change-Number: 57633 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: Revert "Add support for Intel Emmitsburg PCH"
by Edward O'Callaghan (Code Review) 14 Sep '21

14 Sep '21

Attention is currently required from: Arthur Heymans, Nico Huber, Jonathan Zhang, Johnny Lin, David Hendricks, Christian Walter, Stefan Reinauer, Deomid "rojer" Ryabkov, Tim Chu. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57589 ) Change subject: Revert "Add support for Intel Emmitsburg PCH" ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Hi David, […] Sorry all for the double post, one thing that I forgot to mention. Is there room here to perhaps write a unit-test as well as `guess_ich_chipset()` seems to be a pure function without side-effects? -- To view, visit https://review.coreboot.org/c/flashrom/+/57589 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4ebb7a931cfa66276df2f762c63e6d092d6b3d5a Gerrit-Change-Number: 57589 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: David Hendricks Gerrit-Attention: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Tue, 14 Sep 2021 03:23:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "Add support for Intel Emmitsburg PCH"
by Edward O'Callaghan (Code Review) 14 Sep '21

14 Sep '21

Attention is currently required from: Arthur Heymans, Nico Huber, Jonathan Zhang, Johnny Lin, David Hendricks, Christian Walter, Stefan Reinauer, Deomid "rojer" Ryabkov, Tim Chu. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57589 ) Change subject: Revert "Add support for Intel Emmitsburg PCH" ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: > > > The patch was reviewed and tested by multiple parties - you were one of them! Your comments didn […] Hi David, I think there is a bit of cross talking here. David => Nico is correct that we should revert and re-land with it fixed correctly. Obviously the breakage wasn't intentional and communication was got lost there with Nico's unanswered question. Let's just let the revert go though please, please remember commits are just hashes and are free we don't need to fight over this and we can re-land this once you addressed all the comments in a follow up patch that fixes the concerns. 😊 Nico => It is sufficient to say revert-first upon breakage, you are absolutely right there. Just no need to threaten forkage whenever someone makes a mistake, I would imagine David does care about your concern already. Your technical arguments are sufficient on their own merit they just need to be understood by the reader. I think what happened here is that you can see something obvious to you but not obvious to David. Let's be patient and care about one another as humans first. -- To view, visit https://review.coreboot.org/c/flashrom/+/57589 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4ebb7a931cfa66276df2f762c63e6d092d6b3d5a Gerrit-Change-Number: 57589 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: David Hendricks Gerrit-Attention: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Tue, 14 Sep 2021 02:06:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "Add support for Intel Emmitsburg PCH"
by David Hendricks (Code Review) 14 Sep '21

14 Sep '21

Attention is currently required from: Arthur Heymans, Nico Huber, Jonathan Zhang, Johnny Lin, Christian Walter, Stefan Reinauer, Edward O'Callaghan, David Hendricks, Deomid "rojer" Ryabkov, Tim Chu. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57589 ) Change subject: Revert "Add support for Intel Emmitsburg PCH" ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > > The patch was reviewed and tested by multiple parties - you were one of them! Your comments didn't contain any useful guidance for how you'd like to see this implemented, nor did you raise any specific, actionable concerns. > > I asked for something. I didn't get an answer. I'm too busy > to take on all details of a patch before the basic questions > are answered. Usually that turns into wasted time. You asked about freq_read and ICCRIBA, neither of which that patch touched or changed as a consequence of that patch. You didn't explain why those might be relevant, either. We're all too busy to go on wild goose chases. > Except that somebody forgot to write consistent code. > > if (x <= 80) > return; That's not quite stating it accurately. Consider the flow of the outer if-statment: https://review.coreboot.org/plugins/gitiles/flashrom/+/refs/heads/master/ic… Just like in the other cases where we check a range of ISL values at successively larger values, if the ISL is greater-than the highest value we compare with then the outer if-statement will end by returning the default. It happens that in this case the last value we compare with was quite high, perhaps due to it being a server chip. > > ...x != 16... The `warn_peculiar_desc(content->ISL != 16, "Ibex Peak")` prints a warning that we're assuming Ibex Peak for compatibility, and then outer if-statement finishes by returning CHIPSET_5_SERIES_IBEX_PEAK as a default. This is not deliberately selecting Ibex Peak, it's choosing an arbitrary default based on failure to detect other chipsets. > We can also argue, based on the broken code, that the consistent > code wasn't good anyway, ofc. Adding EBG in a consistent manner is what got us here :-P What we really need to do is remove brittle assumptions that cause problems when new hardware is added. ifdtool already does that for the Ibex Peak case, though it still allows a range of ISL values to match: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/ut… Perhaps we can stick to allowing a range of ISL values for older hardware where we don't have documentation or IFDs handy to look for an exact value to compare with, and be more careful about adding values to match newer hardware. > Let's be serious again. Yes, please. I've sent two patches to fix the problems mentioned above: CB:57580 and CB:57581. If you'd like you can add this revert patch into that patch chain and rebase the others on top of it, and we'll have three patches to merge instead of only two. LMK how you'd like to proceed. -- To view, visit https://review.coreboot.org/c/flashrom/+/57589 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4ebb7a931cfa66276df2f762c63e6d092d6b3d5a Gerrit-Change-Number: 57589 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: David Hendricks Gerrit-Attention: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Tue, 14 Sep 2021 01:46:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dediprog.c: Drop dediprog_ prefix for spi data struct members
by Nico Huber (Code Review) 13 Sep '21

13 Sep '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/56415 ) Change subject: dediprog.c: Drop dediprog_ prefix for spi data struct members ...................................................................... dediprog.c: Drop dediprog_ prefix for spi data struct members The name of the struct type already contains dp_ prefix, so prefix doesn't need to be repeated in members name. BUG=b:185191942 TEST=builds and ninja test Change-Id: I688d50926b78a6c3f1c5a8ba4ef88a0d5b495bd0 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/56415 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M dediprog.c 1 file changed, 54 insertions(+), 60 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/dediprog.c b/dediprog.c index 942cc26..a1d9972 100644 --- a/dediprog.c +++ b/dediprog.c @@ -156,11 +156,11 @@ struct dediprog_data { struct libusb_context *usb_ctx; - libusb_device_handle *dediprog_handle; - int dediprog_in_endpoint; - int dediprog_out_endpoint; - int dediprog_firmwareversion; - enum dediprog_devtype dediprog_devicetype; + libusb_device_handle *handle; + int in_endpoint; + int out_endpoint; + int firmwareversion; + enum dediprog_devtype devicetype; }; #if defined(LIBUSB_MAJOR) && defined(LIBUSB_MINOR) && defined(LIBUSB_MICRO) && \ @@ -182,17 +182,17 @@ static enum protocol protocol(const struct dediprog_data *dp_data) { /* Firmware version < 5.0.0 is handled explicitly in some cases. */ - switch (dp_data->dediprog_devicetype) { + switch (dp_data->devicetype) { case DEV_SF100: case DEV_SF200: - if (dp_data->dediprog_firmwareversion < FIRMWARE_VERSION(5, 5, 0)) + if (dp_data->firmwareversion < FIRMWARE_VERSION(5, 5, 0)) return PROTOCOL_V1; else return PROTOCOL_V2; case DEV_SF600: - if (dp_data->dediprog_firmwareversion < FIRMWARE_VERSION(6, 9, 0)) + if (dp_data->firmwareversion < FIRMWARE_VERSION(6, 9, 0)) return PROTOCOL_V1; - else if (dp_data->dediprog_firmwareversion <= FIRMWARE_VERSION(7, 2, 21)) + else if (dp_data->firmwareversion <= FIRMWARE_VERSION(7, 2, 21)) return PROTOCOL_V2; else return PROTOCOL_V3; @@ -270,16 +270,16 @@ int target_leds, ret; if (protocol(dp_data) >= PROTOCOL_V2) { target_leds = (leds ^ 7) << 8; - ret = dediprog_write(dp_data->dediprog_handle, CMD_SET_IO_LED, target_leds, 0, NULL, 0); + ret = dediprog_write(dp_data->handle, CMD_SET_IO_LED, target_leds, 0, NULL, 0); } else { - if (dp_data->dediprog_firmwareversion < FIRMWARE_VERSION(5, 0, 0)) { + if (dp_data->firmwareversion < FIRMWARE_VERSION(5, 0, 0)) { target_leds = ((leds & LED_ERROR) >> 2) | ((leds & LED_PASS) << 2); } else { target_leds = leds; } target_leds ^= 7; - ret = dediprog_write(dp_data->dediprog_handle, CMD_SET_IO_LED, 0x9, target_leds, NULL, 0); + ret = dediprog_write(dp_data->handle, CMD_SET_IO_LED, 0x9, target_leds, NULL, 0); } if (ret != 0x0) { @@ -352,7 +352,7 @@ static int dediprog_set_spi_speed(unsigned int spispeed_idx, const struct dediprog_data *dp_data) { - if (dp_data->dediprog_firmwareversion < FIRMWARE_VERSION(5, 0, 0)) { + if (dp_data->firmwareversion < FIRMWARE_VERSION(5, 0, 0)) { msg_pwarn("Skipping to set SPI speed because firmware is too old.\n"); return 0; } @@ -360,7 +360,7 @@ const struct dediprog_spispeeds *spispeed = &spispeeds[spispeed_idx]; msg_pdbg("SPI speed is %sHz\n", spispeed->name); - int ret = dediprog_write(dp_data->dediprog_handle, CMD_SET_SPI_CLK, spispeed->speed, 0, NULL, 0); + int ret = dediprog_write(dp_data->handle, CMD_SET_SPI_CLK, spispeed->speed, 0, NULL, 0); if (ret != 0x0) { msg_perr("Command Set SPI Speed 0x%x failed!\n", spispeed->speed); return 1; @@ -484,8 +484,7 @@ if (prepare_rw_cmd(flash, data_packet, count, READ_MODE_STD, &value, &idx, start, 1)) return 1; - int ret = dediprog_write(dp_data->dediprog_handle, - CMD_READ, value, idx, data_packet, sizeof(data_packet)); + int ret = dediprog_write(dp_data->handle, CMD_READ, value, idx, data_packet, sizeof(data_packet)); if (ret != (int)sizeof(data_packet)) { msg_perr("Command Read SPI Bulk failed, %i %s!\n", ret, libusb_error_name(ret)); return 1; @@ -513,8 +512,7 @@ (status.queued_idx - status.finished_idx) < DEDIPROG_ASYNC_TRANSFERS) { transfer = transfers[status.queued_idx % DEDIPROG_ASYNC_TRANSFERS]; - libusb_fill_bulk_transfer(transfer, - dp_data->dediprog_handle, 0x80 | dp_data->dediprog_in_endpoint, + libusb_fill_bulk_transfer(transfer, dp_data->handle, 0x80 | dp_data->in_endpoint, (unsigned char *)buf + status.queued_idx * chunksize, chunksize, dediprog_bulk_read_cb, &status, DEFAULT_TIMEOUT); transfer->flags |= LIBUSB_TRANSFER_SHORT_NOT_OK; @@ -644,8 +642,7 @@ unsigned int value, idx; if (prepare_rw_cmd(flash, data_packet, count, dedi_spi_cmd, &value, &idx, start, 0)) return 1; - int ret = dediprog_write(dp_data->dediprog_handle, - CMD_WRITE, value, idx, data_packet, sizeof(data_packet)); + int ret = dediprog_write(dp_data->handle, CMD_WRITE, value, idx, data_packet, sizeof(data_packet)); if (ret != (int)sizeof(data_packet)) { msg_perr("Command Write SPI Bulk failed, %s!\n", libusb_error_name(ret)); return 1; @@ -657,8 +654,7 @@ memcpy(usbbuf, buf + i * chunksize, chunksize); memset(usbbuf + chunksize, 0xff, sizeof(usbbuf) - chunksize); // fill up with 0xFF int transferred; - ret = libusb_bulk_transfer(dp_data->dediprog_handle, dp_data->dediprog_out_endpoint, - usbbuf, 512, &transferred, + ret = libusb_bulk_transfer(dp_data->handle, dp_data->out_endpoint, usbbuf, 512, &transferred, DEFAULT_TIMEOUT); if ((ret < 0) || (transferred != 512)) { msg_perr("SPI bulk write failed, expected %i, got %s!\n", 512, libusb_error_name(ret)); @@ -761,7 +757,7 @@ idx = readcnt ? 0x1 : 0x0; // Indicate if we require a read value = 0; } - ret = dediprog_write(dp_data->dediprog_handle, CMD_TRANSCEIVE, value, idx, writearr, writecnt); + ret = dediprog_write(dp_data->handle, CMD_TRANSCEIVE, value, idx, writearr, writecnt); if (ret != (int)writecnt) { msg_perr("Send SPI failed, expected %i, got %i %s!\n", writecnt, ret, libusb_error_name(ret)); @@ -788,7 +784,7 @@ } ret = dediprog_read(dp_data->dediprog_handle, CMD_TRANSCEIVE, value, idx, readarr, readcnt); */ - ret = dediprog_read(dp_data->dediprog_handle, CMD_TRANSCEIVE, 0, 0, readarr, readcnt); + ret = dediprog_read(dp_data->handle, CMD_TRANSCEIVE, 0, 0, readarr, readcnt); if (ret != (int)readcnt) { msg_perr("Receive SPI failed, expected %i, got %i %s!\n", readcnt, ret, libusb_error_name(ret)); return 1; @@ -802,7 +798,7 @@ char buf[0x11]; /* Command Receive Device String. */ - ret = dediprog_read(dp_data->dediprog_handle, CMD_READ_PROG_INFO, 0, 0, (uint8_t *)buf, 0x10); + ret = dediprog_read(dp_data->handle, CMD_READ_PROG_INFO, 0, 0, (uint8_t *)buf, 0x10); if (ret != 0x10) { msg_perr("Incomplete/failed Command Receive Device String!\n"); return 1; @@ -810,11 +806,11 @@ buf[0x10] = '\0'; msg_pdbg("Found a %s\n", buf); if (memcmp(buf, "SF100", 0x5) == 0) - dp_data->dediprog_devicetype = DEV_SF100; + dp_data->devicetype = DEV_SF100; else if (memcmp(buf, "SF200", 0x5) == 0) - dp_data->dediprog_devicetype = DEV_SF200; + dp_data->devicetype = DEV_SF200; else if (memcmp(buf, "SF600", 0x5) == 0) - dp_data->dediprog_devicetype = DEV_SF600; + dp_data->devicetype = DEV_SF600; else { msg_perr("Device not a SF100, SF200, or SF600!\n"); return 1; @@ -823,7 +819,7 @@ int sfnum; int fw[3]; if (sscanf(buf, "SF%d V:%d.%d.%d ", &sfnum, &fw[0], &fw[1], &fw[2]) != 4 || - sfnum != (int)dp_data->dediprog_devicetype) { + sfnum != (int)dp_data->devicetype) { msg_perr("Unexpected firmware version string '%s'\n", buf); return 1; } @@ -833,7 +829,7 @@ return 1; } - dp_data->dediprog_firmwareversion = FIRMWARE_VERSION(fw[0], fw[1], fw[2]); + dp_data->firmwareversion = FIRMWARE_VERSION(fw[0], fw[1], fw[2]); if (protocol(dp_data) == PROTOCOL_UNKNOWN) { msg_perr("Internal error: Unable to determine protocol version.\n"); return 1; @@ -899,12 +895,11 @@ { int ret; - if (dp_data->dediprog_devicetype != DEV_SF600) + if (dp_data->devicetype != DEV_SF600) return 0; msg_pdbg2("Disabling standalone mode.\n"); - ret = dediprog_write(dp_data->dediprog_handle, - CMD_SET_STANDALONE, LEAVE_STANDALONE_MODE, 0, NULL, 0); + ret = dediprog_write(dp_data->handle, CMD_SET_STANDALONE, LEAVE_STANDALONE_MODE, 0, NULL, 0); if (ret) { msg_perr("Failed to disable standalone mode: %s\n", libusb_error_name(ret)); return 1; @@ -1018,17 +1013,17 @@ struct dediprog_data *dp_data = data; /* URB 28. Command Set SPI Voltage to 0. */ - if (dediprog_set_spi_voltage(dp_data->dediprog_handle, 0x0)) { + if (dediprog_set_spi_voltage(dp_data->handle, 0x0)) { ret = 1; goto out; } - if (libusb_release_interface(dp_data->dediprog_handle, 0)) { + if (libusb_release_interface(dp_data->handle, 0)) { msg_perr("Could not release USB interface!\n"); ret = 1; goto out; } - libusb_close(dp_data->dediprog_handle); + libusb_close(dp_data->handle); libusb_exit(dp_data->usb_ctx); out: free(data); @@ -1058,25 +1053,24 @@ const uint16_t pid = devs_dediprog[0].device_id; int ret; - dp_data->dediprog_handle = usb_dev_get_by_vid_pid_number(dp_data->usb_ctx, - vid, pid, (unsigned int) index); - if (!dp_data->dediprog_handle) { + dp_data->handle = usb_dev_get_by_vid_pid_number(dp_data->usb_ctx, vid, pid, (unsigned int) index); + if (!dp_data->handle) { msg_perr("Could not find a Dediprog programmer on USB.\n"); libusb_exit(dp_data->usb_ctx); return -1; } - ret = libusb_set_configuration(dp_data->dediprog_handle, 1); + ret = libusb_set_configuration(dp_data->handle, 1); if (ret != 0) { msg_perr("Could not set USB device configuration: %i %s\n", ret, libusb_error_name(ret)); - libusb_close(dp_data->dediprog_handle); + libusb_close(dp_data->handle); return -2; } - ret = libusb_claim_interface(dp_data->dediprog_handle, 0); + ret = libusb_claim_interface(dp_data->handle, 0); if (ret < 0) { msg_perr("Could not claim USB device interface %i: %i %s\n", 0, ret, libusb_error_name(ret)); - libusb_close(dp_data->dediprog_handle); + libusb_close(dp_data->handle); return -2; } return 0; @@ -1208,8 +1202,8 @@ msg_perr("Unable to allocate space for SPI master data\n"); return 1; } - dp_data->dediprog_firmwareversion = FIRMWARE_VERSION(0, 0, 0); - dp_data->dediprog_devicetype = DEV_UNKNOWN; + dp_data->firmwareversion = FIRMWARE_VERSION(0, 0, 0); + dp_data->devicetype = DEV_UNKNOWN; /* Here comes the USB stuff. */ libusb_init(&dp_data->usb_ctx); @@ -1236,17 +1230,17 @@ * device is in use by another instance of flashrom), * the device is skipped and the next device is tried. */ - found_id = dediprog_read_id(dp_data->dediprog_handle); + found_id = dediprog_read_id(dp_data->handle); if (found_id < 0) { msg_perr("Could not read id.\n"); - libusb_release_interface(dp_data->dediprog_handle, 0); - libusb_close(dp_data->dediprog_handle); + libusb_release_interface(dp_data->handle, 0); + libusb_close(dp_data->handle); continue; } msg_pinfo("Found dediprog id SF%06d.\n", found_id); if (found_id != id) { - libusb_release_interface(dp_data->dediprog_handle, 0); - libusb_close(dp_data->dediprog_handle); + libusb_release_interface(dp_data->handle, 0); + libusb_close(dp_data->handle); continue; } break; @@ -1255,7 +1249,7 @@ if (dediprog_open(usedevice, dp_data)) { goto init_err_exit; } - found_id = dediprog_read_id(dp_data->dediprog_handle); + found_id = dediprog_read_id(dp_data->handle); } if (found_id >= 0) { @@ -1265,21 +1259,21 @@ /* Try reading the devicestring. If that fails and the device is old (FW < 6.0.0, which we can not know) * then we need to try the "set voltage" command and then attempt to read the devicestring again. */ if (dediprog_check_devicestring(dp_data)) { - if (dediprog_set_voltage(dp_data->dediprog_handle)) + if (dediprog_set_voltage(dp_data->handle)) goto init_err_cleanup_exit; if (dediprog_check_devicestring(dp_data)) goto init_err_cleanup_exit; } /* SF100/SF200 uses one in/out endpoint, SF600 uses separate in/out endpoints */ - dp_data->dediprog_in_endpoint = 2; - switch (dp_data->dediprog_devicetype) { + dp_data->in_endpoint = 2; + switch (dp_data->devicetype) { case DEV_SF100: case DEV_SF200: - dp_data->dediprog_out_endpoint = 2; + dp_data->out_endpoint = 2; break; default: - dp_data->dediprog_out_endpoint = 1; + dp_data->out_endpoint = 1; break; } @@ -1289,9 +1283,9 @@ dediprog_set_leds(LED_ALL, dp_data); /* Select target/socket, frequency and VCC. */ - if (set_target_flash(dp_data->dediprog_handle, target) || + if (set_target_flash(dp_data->handle, target) || dediprog_set_spi_speed(spispeed_idx, dp_data) || - dediprog_set_spi_voltage(dp_data->dediprog_handle, millivolt)) { + dediprog_set_spi_voltage(dp_data->handle, millivolt)) { dediprog_set_leds(LED_ERROR, dp_data); goto init_err_cleanup_exit; } @@ -1299,8 +1293,8 @@ if (dediprog_standalone_mode(dp_data)) goto init_err_cleanup_exit; - if ((dp_data->dediprog_devicetype == DEV_SF100) || - (dp_data->dediprog_devicetype == DEV_SF600 && protocol(dp_data) == PROTOCOL_V3)) + if ((dp_data->devicetype == DEV_SF100) || + (dp_data->devicetype == DEV_SF600 && protocol(dp_data) == PROTOCOL_V3)) spi_master_dediprog.features &= ~SPI_MASTER_NO_4BA_MODES; if (protocol(dp_data) >= PROTOCOL_V2) -- To view, visit https://review.coreboot.org/c/flashrom/+/56415 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I688d50926b78a6c3f1c5a8ba4ef88a0d5b495bd0 Gerrit-Change-Number: 56415 Gerrit-PatchSet: 7 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: dediprog.c: Refactor singleton states into reentrant pattern
by Nico Huber (Code Review) 13 Sep '21

13 Sep '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/56414 ) Change subject: dediprog.c: Refactor singleton states into reentrant pattern ...................................................................... dediprog.c: Refactor singleton states into reentrant pattern Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver. This is one of the steps on the way to move spi_master data memory management behind the initialisation API, for more context see other patches under the same topic "register_master_api". BUG=b:185191942 TEST=builds and ninja test Change-Id: I72085e750af97b94dfa94f2ebf2a134e41a2ec8d Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/56414 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M dediprog.c 1 file changed, 152 insertions(+), 114 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/dediprog.c b/dediprog.c index 323dcd0..942cc26 100644 --- a/dediprog.c +++ b/dediprog.c @@ -43,10 +43,6 @@ #define REQTYPE_OTHER_IN (LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_OTHER) /* 0xC3 */ #define REQTYPE_EP_OUT (LIBUSB_ENDPOINT_OUT | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_ENDPOINT) /* 0x42 */ #define REQTYPE_EP_IN (LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_ENDPOINT) /* 0xC2 */ -static struct libusb_context *usb_ctx; -static libusb_device_handle *dediprog_handle; -static int dediprog_in_endpoint; -static int dediprog_out_endpoint; enum dediprog_devtype { DEV_UNKNOWN = 0, @@ -158,8 +154,14 @@ {0}, }; -static int dediprog_firmwareversion = FIRMWARE_VERSION(0, 0, 0); -static enum dediprog_devtype dediprog_devicetype = DEV_UNKNOWN; +struct dediprog_data { + struct libusb_context *usb_ctx; + libusb_device_handle *dediprog_handle; + int dediprog_in_endpoint; + int dediprog_out_endpoint; + int dediprog_firmwareversion; + enum dediprog_devtype dediprog_devicetype; +}; #if defined(LIBUSB_MAJOR) && defined(LIBUSB_MINOR) && defined(LIBUSB_MICRO) && \ LIBUSB_MAJOR <= 1 && LIBUSB_MINOR == 0 && LIBUSB_MICRO < 9 @@ -177,20 +179,20 @@ } #endif -static enum protocol protocol(void) +static enum protocol protocol(const struct dediprog_data *dp_data) { /* Firmware version < 5.0.0 is handled explicitly in some cases. */ - switch (dediprog_devicetype) { + switch (dp_data->dediprog_devicetype) { case DEV_SF100: case DEV_SF200: - if (dediprog_firmwareversion < FIRMWARE_VERSION(5, 5, 0)) + if (dp_data->dediprog_firmwareversion < FIRMWARE_VERSION(5, 5, 0)) return PROTOCOL_V1; else return PROTOCOL_V2; case DEV_SF600: - if (dediprog_firmwareversion < FIRMWARE_VERSION(6, 9, 0)) + if (dp_data->dediprog_firmwareversion < FIRMWARE_VERSION(6, 9, 0)) return PROTOCOL_V1; - else if (dediprog_firmwareversion <= FIRMWARE_VERSION(7, 2, 21)) + else if (dp_data->dediprog_firmwareversion <= FIRMWARE_VERSION(7, 2, 21)) return PROTOCOL_V2; else return PROTOCOL_V3; @@ -215,7 +217,9 @@ ++status->finished_idx; } -static int dediprog_bulk_read_poll(const struct dediprog_transfer_status *const status, const int finish) +static int dediprog_bulk_read_poll(struct libusb_context *usb_ctx, + const struct dediprog_transfer_status *const status, + const int finish) { if (status->finished_idx >= status->queued_idx) return 0; @@ -231,13 +235,17 @@ return 0; } -static int dediprog_read(enum dediprog_cmds cmd, unsigned int value, unsigned int idx, uint8_t *bytes, size_t size) +static int dediprog_read(libusb_device_handle *dediprog_handle, + enum dediprog_cmds cmd, unsigned int value, unsigned int idx, + uint8_t *bytes, size_t size) { return libusb_control_transfer(dediprog_handle, REQTYPE_EP_IN, cmd, value, idx, (unsigned char *)bytes, size, DEFAULT_TIMEOUT); } -static int dediprog_write(enum dediprog_cmds cmd, unsigned int value, unsigned int idx, const uint8_t *bytes, size_t size) +static int dediprog_write(libusb_device_handle *dediprog_handle, + enum dediprog_cmds cmd, unsigned int value, unsigned int idx, + const uint8_t *bytes, size_t size) { return libusb_control_transfer(dediprog_handle, REQTYPE_EP_OUT, cmd, value, idx, (unsigned char *)bytes, size, DEFAULT_TIMEOUT); @@ -245,7 +253,7 @@ /* This function sets the GPIOs connected to the LEDs as well as IO1-IO4. */ -static int dediprog_set_leds(int leds) +static int dediprog_set_leds(int leds, const struct dediprog_data *dp_data) { if (leds < LED_NONE || leds > LED_ALL) leds = LED_ALL; @@ -260,18 +268,18 @@ * FIXME: take IO pins into account */ int target_leds, ret; - if (protocol() >= PROTOCOL_V2) { + if (protocol(dp_data) >= PROTOCOL_V2) { target_leds = (leds ^ 7) << 8; - ret = dediprog_write(CMD_SET_IO_LED, target_leds, 0, NULL, 0); + ret = dediprog_write(dp_data->dediprog_handle, CMD_SET_IO_LED, target_leds, 0, NULL, 0); } else { - if (dediprog_firmwareversion < FIRMWARE_VERSION(5, 0, 0)) { + if (dp_data->dediprog_firmwareversion < FIRMWARE_VERSION(5, 0, 0)) { target_leds = ((leds & LED_ERROR) >> 2) | ((leds & LED_PASS) << 2); } else { target_leds = leds; } target_leds ^= 7; - ret = dediprog_write(CMD_SET_IO_LED, 0x9, target_leds, NULL, 0); + ret = dediprog_write(dp_data->dediprog_handle, CMD_SET_IO_LED, 0x9, target_leds, NULL, 0); } if (ret != 0x0) { @@ -282,7 +290,7 @@ return 0; } -static int dediprog_set_spi_voltage(int millivolt) +static int dediprog_set_spi_voltage(libusb_device_handle *dediprog_handle, int millivolt) { int ret; uint16_t voltage_selector; @@ -312,7 +320,7 @@ /* Wait some time as the original driver does. */ programmer_delay(200 * 1000); } - ret = dediprog_write(CMD_SET_VCC, voltage_selector, 0, NULL, 0); + ret = dediprog_write(dediprog_handle, CMD_SET_VCC, voltage_selector, 0, NULL, 0); if (ret != 0x0) { msg_perr("Command Set SPI Voltage 0x%x failed!\n", voltage_selector); @@ -342,9 +350,9 @@ { NULL, 0x0 }, }; -static int dediprog_set_spi_speed(unsigned int spispeed_idx) +static int dediprog_set_spi_speed(unsigned int spispeed_idx, const struct dediprog_data *dp_data) { - if (dediprog_firmwareversion < FIRMWARE_VERSION(5, 0, 0)) { + if (dp_data->dediprog_firmwareversion < FIRMWARE_VERSION(5, 0, 0)) { msg_pwarn("Skipping to set SPI speed because firmware is too old.\n"); return 0; } @@ -352,7 +360,7 @@ const struct dediprog_spispeeds *spispeed = &spispeeds[spispeed_idx]; msg_pdbg("SPI speed is %sHz\n", spispeed->name); - int ret = dediprog_write(CMD_SET_SPI_CLK, spispeed->speed, 0, NULL, 0); + int ret = dediprog_write(dp_data->dediprog_handle, CMD_SET_SPI_CLK, spispeed->speed, 0, NULL, 0); if (ret != 0x0) { msg_perr("Command Set SPI Speed 0x%x failed!\n", spispeed->speed); return 1; @@ -364,6 +372,8 @@ struct flashctx *const flash, uint8_t *data_packet, unsigned int count, uint8_t dedi_spi_cmd, unsigned int *value, unsigned int *idx, unsigned int start, int is_read) { + const struct dediprog_data *dp_data = flash->mst->spi.data; + if (count >= 1 << 16) { msg_perr("%s: Unsupported transfer length of %u blocks! " "Please report a bug at flashrom(a)flashrom.org\n", @@ -378,7 +388,7 @@ data_packet[3] = dedi_spi_cmd; /* Read/Write Mode (currently READ_MODE_STD, WRITE_MODE_PAGE_PGM or WRITE_MODE_2B_AAI) */ data_packet[4] = 0; /* "Opcode". Specs imply necessity only for READ_MODE_4B_ADDR_FAST and WRITE_MODE_4B_ADDR_256B_PAGE_PGM */ - if (protocol() >= PROTOCOL_V2) { + if (protocol(dp_data) >= PROTOCOL_V2) { if (is_read && flash->chip->feature_bits & FEATURE_4BA_FAST_READ) { data_packet[3] = READ_MODE_4B_ADDR_FAST_0x0C; data_packet[4] = JEDEC_READ_4BA_FAST; @@ -394,7 +404,7 @@ data_packet[7] = (start >> 8) & 0xff; data_packet[8] = (start >> 16) & 0xff; data_packet[9] = (start >> 24) & 0xff; - if (protocol() >= PROTOCOL_V3) { + if (protocol(dp_data) >= PROTOCOL_V3) { if (is_read) { data_packet[10] = 0x00; /* address length (3 or 4) */ data_packet[11] = 0x00; /* dummy cycle / 2 */ @@ -435,6 +445,7 @@ static int dediprog_spi_bulk_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { int err = 1; + const struct dediprog_data *dp_data = flash->mst->spi.data; /* chunksize must be 512, other sizes will NOT work at all. */ const unsigned int chunksize = 512; @@ -454,7 +465,7 @@ } int command_packet_size; - switch (protocol()) { + switch (protocol(dp_data)) { case PROTOCOL_V1: command_packet_size = 5; break; @@ -473,7 +484,8 @@ if (prepare_rw_cmd(flash, data_packet, count, READ_MODE_STD, &value, &idx, start, 1)) return 1; - int ret = dediprog_write(CMD_READ, value, idx, data_packet, sizeof(data_packet)); + int ret = dediprog_write(dp_data->dediprog_handle, + CMD_READ, value, idx, data_packet, sizeof(data_packet)); if (ret != (int)sizeof(data_packet)) { msg_perr("Command Read SPI Bulk failed, %i %s!\n", ret, libusb_error_name(ret)); return 1; @@ -501,7 +513,8 @@ (status.queued_idx - status.finished_idx) < DEDIPROG_ASYNC_TRANSFERS) { transfer = transfers[status.queued_idx % DEDIPROG_ASYNC_TRANSFERS]; - libusb_fill_bulk_transfer(transfer, dediprog_handle, 0x80 | dediprog_in_endpoint, + libusb_fill_bulk_transfer(transfer, + dp_data->dediprog_handle, 0x80 | dp_data->dediprog_in_endpoint, (unsigned char *)buf + status.queued_idx * chunksize, chunksize, dediprog_bulk_read_cb, &status, DEFAULT_TIMEOUT); transfer->flags |= LIBUSB_TRANSFER_SHORT_NOT_OK; @@ -513,11 +526,11 @@ } ++status.queued_idx; } - if (dediprog_bulk_read_poll(&status, 0)) + if (dediprog_bulk_read_poll(dp_data->usb_ctx, &status, 0)) goto err_free; } /* Wait for transfers to finish. */ - if (dediprog_bulk_read_poll(&status, 1)) + if (dediprog_bulk_read_poll(dp_data->usb_ctx, &status, 0)) goto err_free; /* Check if everything has been transmitted. */ if ((status.finished_idx < count) || status.error) @@ -526,7 +539,7 @@ err = 0; err_free: - dediprog_bulk_read_poll(&status, 1); + dediprog_bulk_read_poll(dp_data->usb_ctx, &status, 1); for (i = 0; i < DEDIPROG_ASYNC_TRANSFERS; ++i) if (transfers[i]) libusb_free_transfer(transfers[i]); return err; @@ -539,8 +552,9 @@ const unsigned int chunksize = 0x200; unsigned int residue = start % chunksize ? min(len, chunksize - start % chunksize) : 0; unsigned int bulklen; + const struct dediprog_data *dp_data = flash->mst->spi.data; - dediprog_set_leds(LED_BUSY); + dediprog_set_leds(LED_BUSY, dp_data); if (residue) { msg_pdbg("Slow read for partial block from 0x%x, length 0x%x\n", @@ -566,10 +580,10 @@ goto err; } - dediprog_set_leds(LED_PASS); + dediprog_set_leds(LED_PASS, dp_data); return 0; err: - dediprog_set_leds(LED_ERROR); + dediprog_set_leds(LED_ERROR, dp_data); return ret; } @@ -588,6 +602,7 @@ * space in a USB bulk transfer must be filled with 0xff padding. */ const unsigned int count = len / chunksize; + const struct dediprog_data *dp_data = flash->mst->spi.data; /* * We should change this check to @@ -611,7 +626,7 @@ return 0; int command_packet_size; - switch (protocol()) { + switch (protocol(dp_data)) { case PROTOCOL_V1: command_packet_size = 5; break; @@ -629,7 +644,8 @@ unsigned int value, idx; if (prepare_rw_cmd(flash, data_packet, count, dedi_spi_cmd, &value, &idx, start, 0)) return 1; - int ret = dediprog_write(CMD_WRITE, value, idx, data_packet, sizeof(data_packet)); + int ret = dediprog_write(dp_data->dediprog_handle, + CMD_WRITE, value, idx, data_packet, sizeof(data_packet)); if (ret != (int)sizeof(data_packet)) { msg_perr("Command Write SPI Bulk failed, %s!\n", libusb_error_name(ret)); return 1; @@ -641,7 +657,8 @@ memcpy(usbbuf, buf + i * chunksize, chunksize); memset(usbbuf + chunksize, 0xff, sizeof(usbbuf) - chunksize); // fill up with 0xFF int transferred; - ret = libusb_bulk_transfer(dediprog_handle, dediprog_out_endpoint, usbbuf, 512, &transferred, + ret = libusb_bulk_transfer(dp_data->dediprog_handle, dp_data->dediprog_out_endpoint, + usbbuf, 512, &transferred, DEFAULT_TIMEOUT); if ((ret < 0) || (transferred != 512)) { msg_perr("SPI bulk write failed, expected %i, got %s!\n", 512, libusb_error_name(ret)); @@ -659,8 +676,9 @@ const unsigned int chunksize = flash->chip->page_size; unsigned int residue = start % chunksize ? chunksize - start % chunksize : 0; unsigned int bulklen; + const struct dediprog_data *dp_data = flash->mst->spi.data; - dediprog_set_leds(LED_BUSY); + dediprog_set_leds(LED_BUSY, dp_data); if (chunksize != 256) { msg_pdbg("Page sizes other than 256 bytes are unsupported as " @@ -675,7 +693,7 @@ /* No idea about the real limit. Maybe 16 including command and address, maybe more. */ ret = spi_write_chunked(flash, buf, start, residue, 11); if (ret) { - dediprog_set_leds(LED_ERROR); + dediprog_set_leds(LED_ERROR, dp_data); return ret; } } @@ -684,7 +702,7 @@ bulklen = (len - residue) / chunksize * chunksize; ret = dediprog_spi_bulk_write(flash, buf + residue, chunksize, start + residue, bulklen, dedi_spi_cmd); if (ret) { - dediprog_set_leds(LED_ERROR); + dediprog_set_leds(LED_ERROR, dp_data); return ret; } @@ -695,12 +713,12 @@ ret = spi_write_chunked(flash, buf + residue + bulklen, start + residue + bulklen, len, 11); if (ret) { - dediprog_set_leds(LED_ERROR); + dediprog_set_leds(LED_ERROR, dp_data); return ret; } } - dediprog_set_leds(LED_PASS); + dediprog_set_leds(LED_PASS, dp_data); return 0; } @@ -721,6 +739,7 @@ unsigned char *readarr) { int ret; + const struct dediprog_data *dp_data = flash->mst->spi.data; msg_pspew("%s, writecnt=%i, readcnt=%i\n", __func__, writecnt, readcnt); if (writecnt > flash->mst->spi.max_data_write) { @@ -735,14 +754,14 @@ unsigned int idx, value; /* New protocol has options and timeout combined as value while the old one used the value field for * timeout and the index field for options. */ - if (protocol() >= PROTOCOL_V2) { + if (protocol(dp_data) >= PROTOCOL_V2) { idx = 0; value = readcnt ? 0x1 : 0x0; // Indicate if we require a read } else { idx = readcnt ? 0x1 : 0x0; // Indicate if we require a read value = 0; } - ret = dediprog_write(CMD_TRANSCEIVE, value, idx, writearr, writecnt); + ret = dediprog_write(dp_data->dediprog_handle, CMD_TRANSCEIVE, value, idx, writearr, writecnt); if (ret != (int)writecnt) { msg_perr("Send SPI failed, expected %i, got %i %s!\n", writecnt, ret, libusb_error_name(ret)); @@ -767,9 +786,9 @@ idx = (0 & 0xFF); // Lower byte is option (0x01 = require SR, 0x02 keep CS low) value = min(read_timeout, 0xFF); // Possibly two bytes but we play safe here } - ret = dediprog_read(CMD_TRANSCEIVE, value, idx, readarr, readcnt); + ret = dediprog_read(dp_data->dediprog_handle, CMD_TRANSCEIVE, value, idx, readarr, readcnt); */ - ret = dediprog_read(CMD_TRANSCEIVE, 0, 0, readarr, readcnt); + ret = dediprog_read(dp_data->dediprog_handle, CMD_TRANSCEIVE, 0, 0, readarr, readcnt); if (ret != (int)readcnt) { msg_perr("Receive SPI failed, expected %i, got %i %s!\n", readcnt, ret, libusb_error_name(ret)); return 1; @@ -777,13 +796,13 @@ return 0; } -static int dediprog_check_devicestring(void) +static int dediprog_check_devicestring(struct dediprog_data *dp_data) { int ret; char buf[0x11]; /* Command Receive Device String. */ - ret = dediprog_read(CMD_READ_PROG_INFO, 0, 0, (uint8_t *)buf, 0x10); + ret = dediprog_read(dp_data->dediprog_handle, CMD_READ_PROG_INFO, 0, 0, (uint8_t *)buf, 0x10); if (ret != 0x10) { msg_perr("Incomplete/failed Command Receive Device String!\n"); return 1; @@ -791,11 +810,11 @@ buf[0x10] = '\0'; msg_pdbg("Found a %s\n", buf); if (memcmp(buf, "SF100", 0x5) == 0) - dediprog_devicetype = DEV_SF100; + dp_data->dediprog_devicetype = DEV_SF100; else if (memcmp(buf, "SF200", 0x5) == 0) - dediprog_devicetype = DEV_SF200; + dp_data->dediprog_devicetype = DEV_SF200; else if (memcmp(buf, "SF600", 0x5) == 0) - dediprog_devicetype = DEV_SF600; + dp_data->dediprog_devicetype = DEV_SF600; else { msg_perr("Device not a SF100, SF200, or SF600!\n"); return 1; @@ -804,7 +823,7 @@ int sfnum; int fw[3]; if (sscanf(buf, "SF%d V:%d.%d.%d ", &sfnum, &fw[0], &fw[1], &fw[2]) != 4 || - sfnum != (int)dediprog_devicetype) { + sfnum != (int)dp_data->dediprog_devicetype) { msg_perr("Unexpected firmware version string '%s'\n", buf); return 1; } @@ -814,8 +833,8 @@ return 1; } - dediprog_firmwareversion = FIRMWARE_VERSION(fw[0], fw[1], fw[2]); - if (protocol() == PROTOCOL_UNKNOWN) { + dp_data->dediprog_firmwareversion = FIRMWARE_VERSION(fw[0], fw[1], fw[2]); + if (protocol(dp_data) == PROTOCOL_UNKNOWN) { msg_perr("Internal error: Unable to determine protocol version.\n"); return 1; } @@ -831,7 +850,7 @@ * much different. * @return the id on success, -1 on failure */ -static int dediprog_read_id(void) +static int dediprog_read_id(libusb_device_handle *dediprog_handle) { int ret; uint8_t buf[3]; @@ -859,7 +878,7 @@ /* This command presumably sets the voltage for the SF100 itself (not the SPI flash). * Only use dediprog_set_voltage on SF100 programmers with firmware older * than V6.0.0. Newer programmers (including all SF600s) do not support it. */ -static int dediprog_set_voltage(void) +static int dediprog_set_voltage(libusb_device_handle *dediprog_handle) { unsigned char buf[1] = {0}; int ret = libusb_control_transfer(dediprog_handle, REQTYPE_OTHER_IN, CMD_SET_VOLTAGE, 0x0, 0x0, @@ -876,15 +895,16 @@ return 0; } -static int dediprog_standalone_mode(void) +static int dediprog_standalone_mode(const struct dediprog_data *dp_data) { int ret; - if (dediprog_devicetype != DEV_SF600) + if (dp_data->dediprog_devicetype != DEV_SF600) return 0; msg_pdbg2("Disabling standalone mode.\n"); - ret = dediprog_write(CMD_SET_STANDALONE, LEAVE_STANDALONE_MODE, 0, NULL, 0); + ret = dediprog_write(dp_data->dediprog_handle, + CMD_SET_STANDALONE, LEAVE_STANDALONE_MODE, 0, NULL, 0); if (ret) { msg_perr("Failed to disable standalone mode: %s\n", libusb_error_name(ret)); return 1; @@ -898,7 +918,7 @@ * Present in eng_detect_blink.log with firmware 3.1.8 * Always preceded by Command Receive Device String */ -static int dediprog_command_b(void) +static int dediprog_command_b(libusb_device_handle *dediprog_handle) { int ret; char buf[0x3]; @@ -919,9 +939,9 @@ } #endif -static int set_target_flash(enum dediprog_target target) +static int set_target_flash(libusb_device_handle *dediprog_handle, enum dediprog_target target) { - int ret = dediprog_write(CMD_SET_TARGET, target, 0, NULL, 0); + int ret = dediprog_write(dediprog_handle, CMD_SET_TARGET, target, 0, NULL, 0); if (ret != 0) { msg_perr("set_target_flash failed (%s)!\n", libusb_error_name(ret)); return 1; @@ -931,7 +951,7 @@ #if 0 /* Returns true if the button is currently pressed. */ -static bool dediprog_get_button(void) +static bool dediprog_get_button(libusb_device_handle *dediprog_handle) { char buf[1]; int ret = usb_control_msg(dediprog_handle, REQTYPE_EP_IN, CMD_GET_BUTTON, 0, 0, @@ -994,20 +1014,25 @@ static int dediprog_shutdown(void *data) { - dediprog_devicetype = DEV_UNKNOWN; + int ret = 0; + struct dediprog_data *dp_data = data; /* URB 28. Command Set SPI Voltage to 0. */ - if (dediprog_set_spi_voltage(0x0)) - return 1; - - if (libusb_release_interface(dediprog_handle, 0)) { - msg_perr("Could not release USB interface!\n"); - return 1; + if (dediprog_set_spi_voltage(dp_data->dediprog_handle, 0x0)) { + ret = 1; + goto out; } - libusb_close(dediprog_handle); - libusb_exit(usb_ctx); - return 0; + if (libusb_release_interface(dp_data->dediprog_handle, 0)) { + msg_perr("Could not release USB interface!\n"); + ret = 1; + goto out; + } + libusb_close(dp_data->dediprog_handle); + libusb_exit(dp_data->usb_ctx); +out: + free(data); + return ret; } static struct spi_master spi_master_dediprog = { @@ -1027,30 +1052,31 @@ * @index index of the USB device * @return 0 for success, -1 for error, -2 for busy device */ -static int dediprog_open(int index) +static int dediprog_open(int index, struct dediprog_data *dp_data) { const uint16_t vid = devs_dediprog[0].vendor_id; const uint16_t pid = devs_dediprog[0].device_id; int ret; - dediprog_handle = usb_dev_get_by_vid_pid_number(usb_ctx, vid, pid, (unsigned int) index); - if (!dediprog_handle) { + dp_data->dediprog_handle = usb_dev_get_by_vid_pid_number(dp_data->usb_ctx, + vid, pid, (unsigned int) index); + if (!dp_data->dediprog_handle) { msg_perr("Could not find a Dediprog programmer on USB.\n"); - libusb_exit(usb_ctx); + libusb_exit(dp_data->usb_ctx); return -1; } - ret = libusb_set_configuration(dediprog_handle, 1); + ret = libusb_set_configuration(dp_data->dediprog_handle, 1); if (ret != 0) { msg_perr("Could not set USB device configuration: %i %s\n", ret, libusb_error_name(ret)); - libusb_close(dediprog_handle); + libusb_close(dp_data->dediprog_handle); return -2; } - ret = libusb_claim_interface(dediprog_handle, 0); + ret = libusb_claim_interface(dp_data->dediprog_handle, 0); if (ret < 0) { msg_perr("Could not claim USB device interface %i: %i %s\n", 0, ret, libusb_error_name(ret)); - libusb_close(dediprog_handle); + libusb_close(dp_data->dediprog_handle); return -2; } return 0; @@ -1177,19 +1203,27 @@ } free(target_str); - /* Here comes the USB stuff. */ - libusb_init(&usb_ctx); - if (!usb_ctx) { - msg_perr("Could not initialize libusb!\n"); + struct dediprog_data *dp_data = calloc(1, sizeof(*dp_data)); + if (!dp_data) { + msg_perr("Unable to allocate space for SPI master data\n"); return 1; } + dp_data->dediprog_firmwareversion = FIRMWARE_VERSION(0, 0, 0); + dp_data->dediprog_devicetype = DEV_UNKNOWN; + + /* Here comes the USB stuff. */ + libusb_init(&dp_data->usb_ctx); + if (!dp_data->usb_ctx) { + msg_perr("Could not initialize libusb!\n"); + goto init_err_exit; + } if (id != -1) { for (i = 0; ; i++) { - ret = dediprog_open(i); + ret = dediprog_open(i, dp_data); if (ret == -1) { /* no dev */ - return 1; + goto init_err_exit; } else if (ret == -2) { /* busy dev */ continue; @@ -1202,26 +1236,26 @@ * device is in use by another instance of flashrom), * the device is skipped and the next device is tried. */ - found_id = dediprog_read_id(); + found_id = dediprog_read_id(dp_data->dediprog_handle); if (found_id < 0) { msg_perr("Could not read id.\n"); - libusb_release_interface(dediprog_handle, 0); - libusb_close(dediprog_handle); + libusb_release_interface(dp_data->dediprog_handle, 0); + libusb_close(dp_data->dediprog_handle); continue; } msg_pinfo("Found dediprog id SF%06d.\n", found_id); if (found_id != id) { - libusb_release_interface(dediprog_handle, 0); - libusb_close(dediprog_handle); + libusb_release_interface(dp_data->dediprog_handle, 0); + libusb_close(dp_data->dediprog_handle); continue; } break; } } else { - if (dediprog_open(usedevice)) { - return 1; + if (dediprog_open(usedevice, dp_data)) { + goto init_err_exit; } - found_id = dediprog_read_id(); + found_id = dediprog_read_id(dp_data->dediprog_handle); } if (found_id >= 0) { @@ -1230,55 +1264,59 @@ /* Try reading the devicestring. If that fails and the device is old (FW < 6.0.0, which we can not know) * then we need to try the "set voltage" command and then attempt to read the devicestring again. */ - if (dediprog_check_devicestring()) { - if (dediprog_set_voltage()) + if (dediprog_check_devicestring(dp_data)) { + if (dediprog_set_voltage(dp_data->dediprog_handle)) goto init_err_cleanup_exit; - if (dediprog_check_devicestring()) + if (dediprog_check_devicestring(dp_data)) goto init_err_cleanup_exit; } /* SF100/SF200 uses one in/out endpoint, SF600 uses separate in/out endpoints */ - dediprog_in_endpoint = 2; - switch (dediprog_devicetype) { + dp_data->dediprog_in_endpoint = 2; + switch (dp_data->dediprog_devicetype) { case DEV_SF100: case DEV_SF200: - dediprog_out_endpoint = 2; + dp_data->dediprog_out_endpoint = 2; break; default: - dediprog_out_endpoint = 1; + dp_data->dediprog_out_endpoint = 1; break; } /* Set all possible LEDs as soon as possible to indicate activity. * Because knowing the firmware version is required to set the LEDs correctly we need to this after * dediprog_check_devicestring() has queried the device. */ - dediprog_set_leds(LED_ALL); + dediprog_set_leds(LED_ALL, dp_data); /* Select target/socket, frequency and VCC. */ - if (set_target_flash(target) || - dediprog_set_spi_speed(spispeed_idx) || - dediprog_set_spi_voltage(millivolt)) { - dediprog_set_leds(LED_ERROR); + if (set_target_flash(dp_data->dediprog_handle, target) || + dediprog_set_spi_speed(spispeed_idx, dp_data) || + dediprog_set_spi_voltage(dp_data->dediprog_handle, millivolt)) { + dediprog_set_leds(LED_ERROR, dp_data); goto init_err_cleanup_exit; } - if (dediprog_standalone_mode()) + if (dediprog_standalone_mode(dp_data)) goto init_err_cleanup_exit; - if ((dediprog_devicetype == DEV_SF100) || - (dediprog_devicetype == DEV_SF600 && protocol() == PROTOCOL_V3)) + if ((dp_data->dediprog_devicetype == DEV_SF100) || + (dp_data->dediprog_devicetype == DEV_SF600 && protocol(dp_data) == PROTOCOL_V3)) spi_master_dediprog.features &= ~SPI_MASTER_NO_4BA_MODES; - if (protocol() >= PROTOCOL_V2) + if (protocol(dp_data) >= PROTOCOL_V2) spi_master_dediprog.features |= SPI_MASTER_4BA; - if (register_spi_master(&spi_master_dediprog, NULL) || dediprog_set_leds(LED_NONE)) + if (register_spi_master(&spi_master_dediprog, dp_data) || dediprog_set_leds(LED_NONE, dp_data)) return 1; /* shutdown function does cleanup */ return 0; init_err_cleanup_exit: - dediprog_shutdown(NULL); + dediprog_shutdown(dp_data); + return 1; + +init_err_exit: + free(dp_data); return 1; } -- To view, visit https://review.coreboot.org/c/flashrom/+/56414 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I72085e750af97b94dfa94f2ebf2a134e41a2ec8d Gerrit-Change-Number: 56414 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: Revert "Add support for Intel Emmitsburg PCH"
by Nico Huber (Code Review) 13 Sep '21

13 Sep '21

Attention is currently required from: Arthur Heymans, Jonathan Zhang, Johnny Lin, David Hendricks, Christian Walter, Stefan Reinauer, Edward O'Callaghan, Deomid "rojer" Ryabkov, Tim Chu. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57589 ) Change subject: Revert "Add support for Intel Emmitsburg PCH" ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > The patch was reviewed and tested by multiple parties - you were one of them! Your comments didn't contain any useful guidance for how you'd like to see this implemented, nor did you raise any specific, actionable concerns. I asked for something. I didn't get an answer. I'm too busy to take on all details of a patch before the basic questions are answered. Usually that turns into wasted time. If some- body else wants to take the responsibility that's ok for me, *if* they act responsibly when something goes wrong. Apologizing and asking for help is also ok. But making wild justifications whilst still not reading the code (see below) is not, IMO. > We went with the approach that aligned with how the code was already implemented Except that somebody forgot to write consistent code. if (x <= 80) return; ...x != 16... Makes no sense, no matter the context, documentation or anything. That you still haven't realized that tells me that the patch shouldn't have been merged. Reverting it would fix that. > and worked when testing on real hardware. Great works-for-me attitude! And of course if the new hardware works, we can skip the review and don't have to care about older hardware. We can also argue, based on the broken code, that the consistent code wasn't good anyway, ofc. Let's be serious again. If we can't agree that everything gets properly reviewed *before* it's merged (afterwards nobody will do it, you just proved that), I'm really done with this branch. -- To view, visit https://review.coreboot.org/c/flashrom/+/57589 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4ebb7a931cfa66276df2f762c63e6d092d6b3d5a Gerrit-Change-Number: 57589 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: David Hendricks Gerrit-Attention: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Mon, 13 Sep 2021 21:42:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors: Explicitly check for Ibex Peak ISL value
by David Hendricks (Code Review) 13 Sep '21

13 Sep '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, David Hendricks, Angel Pons, Arthur Heymans. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57580 ) Change subject: ich_descriptors: Explicitly check for Ibex Peak ISL value ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Please mention all changes (and affected paths) in the commit message and argue why they are necessary. Code snippet #1: > if (content->ISL == 16) > return CHIPSET_5_SERIES_IBEX_PEAK; Commit message: This adds an explicit check for Ibex Peak when guessing chipset from IFD contents Rationale: This makes it so that we positively identify Ibex Peak, using the ISL value you've confirmed, instead of guessing based on other chipsets not being detected. Code snippet #2: > /* Ibex Peak is historically used as the default */ Commit message: > adds a comment that Ibex Peak is historically the default when other chipsets are not detected. Rationale: This adds some context to explain why we return CHIPSET_5_SERIES_IBEX_PEAK instead of something else, such as CHIPSET_ICH_UNKNOWN. Hopefully some documentation will avoid this sort of friction in the future. -- To view, visit https://review.coreboot.org/c/flashrom/+/57580 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8a880251a393666757f3879629a165b7fe3533ef Gerrit-Change-Number: 57580 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: David Hendricks Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Mon, 13 Sep 2021 20:59:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "Add support for Intel Emmitsburg PCH"
by David Hendricks (Code Review) 13 Sep '21

13 Sep '21

Attention is currently required from: Arthur Heymans, Nico Huber, Jonathan Zhang, Johnny Lin, Christian Walter, Stefan Reinauer, Edward O'Callaghan, David Hendricks, Deomid "rojer" Ryabkov, Tim Chu. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57589 ) Change subject: Revert "Add support for Intel Emmitsburg PCH" ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Hi David, as per https://doc.coreboot.org/getting_started/gerrit_guidelines.html, > please provide a concrete recommendation here. 1. Explicitly check for Ibex Peak instead of relying on dubious default behavior: CB:57580 2. Start using `==` to check ISL/PSL values, like I've done in CB:57581. As Angel pointed out, item 2 by itself should be sufficient in this case if we don't want the default behavior, which is to assume Ibex Peak, to change. However, since Ibex Peak was introduced in 2009 and has been obsolete for years we probably should considering changing the default to something more recent. > What exactly would you like to see > that will fix all the issues related to the merge of an unreviewed, regressing > commit The patch was reviewed and tested by multiple parties - you were one of them! Your comments didn't contain any useful guidance for how you'd like to see this implemented, nor did you raise any specific, actionable concerns. We went with the approach that aligned with how the code was already implemented and worked when testing on real hardware. -- To view, visit https://review.coreboot.org/c/flashrom/+/57589 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4ebb7a931cfa66276df2f762c63e6d092d6b3d5a Gerrit-Change-Number: 57589 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: David Hendricks Gerrit-Attention: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Mon, 13 Sep 2021 19:22:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "Add support for Intel Emmitsburg PCH"
by Nico Huber (Code Review) 13 Sep '21

13 Sep '21

Attention is currently required from: Arthur Heymans, Jonathan Zhang, Johnny Lin, Christian Walter, Stefan Reinauer, Edward O'Callaghan, David Hendricks, Deomid "rojer" Ryabkov, Tim Chu. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57589 ) Change subject: Revert "Add support for Intel Emmitsburg PCH" ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Hi David, as per https://doc.coreboot.org/getting_started/gerrit_guidelines.html, please provide a concrete recommendation here. What exactly would you like to see that will fix all the issues related to the merge of an unreviewed, regressing commit without letting the community do the job that the author was paid for. -- To view, visit https://review.coreboot.org/c/flashrom/+/57589 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4ebb7a931cfa66276df2f762c63e6d092d6b3d5a Gerrit-Change-Number: 57589 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: David Hendricks Gerrit-Attention: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Mon, 13 Sep 2021 18:05:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2021