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

Change in flashrom[master]: pci2: add read/write/mask byte/long mmio accessors
by Angel Pons (Code Review) 11 May '20

11 May '20

Angel Pons has uploaded a new patch set (#3) to the change originally created by Luc Verhaegen. ( https://review.coreboot.org/c/flashrom/+/29080 ) Change subject: pci2: add read/write/mask byte/long mmio accessors ...................................................................... pci2: add read/write/mask byte/long mmio accessors As for the mask command: For those "embedded developers" who grew up with the awkward x86 in/out assembly instructions with the illogical ordering it will seem alien. As a graphics driver developer who has had to deal with thousands of registers at a time, mostly setting a few bits at a time, i found the increased readability and debuggability of a mask command invaluable. Once the ati_spi code is added, it will be clear just how effective this command is. As for the lack of automatic rollback: - it makes little sense to roll back every single register written. One could imagine a scenario where this would perfectly undo the flash being written. - it makes little sense to roll back every mask command: Multiple mask commands are likely to touch the same register. - engines, even simple spi engines, are triggered by a single bit or a single register being written. Automatic rollback would trigger engines before the fitting registers are written. - reading might also trigger an engine. How does one roll back those? How, if the register is automatically read before it is written, are we dealing with that? Instead, by providing programmer internal save/restore functions, the programmer gets full control over what is saved and restored, and the order in which those happen. The fact that we automatically retain the device match private, should make it easier to have device specific save/restore callbacks. Change-Id: I5c3f57e9c510337bb81be30aa6c143840a349a60 Signed-off-by: Luc Verhaegen <libv(a)skynet.be> --- M pcidev.c M programmer.h 2 files changed, 101 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/80/29080/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/29080 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5c3f57e9c510337bb81be30aa6c143840a349a60 Gerrit-Change-Number: 29080 Gerrit-PatchSet: 3 Gerrit-Owner: Luc Verhaegen <libv(a)skynet.be> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: add newer pci infrastructure
by Angel Pons (Code Review) 11 May '20

11 May '20

Angel Pons has uploaded a new patch set (#3) to the change originally created by Luc Verhaegen. ( https://review.coreboot.org/c/flashrom/+/29078 ) Change subject: add newer pci infrastructure ...................................................................... add newer pci infrastructure This code does not negatively affect existing pci support and existing pci devices, it introduces parallel infrastructure which: - allows users to define a private which could be device specific. This avoids doing multiple lookups, which massively helps the ati spi driver which will support hundreds of devices. - uses linux sysfs pci infrastructure for enable/disable and mapping resources. This circumvents the security restrictions surrounding /dev/mem, but means that support for other operating systems still needs to be cobbled together. - looks up device names through libpci, keeping us from maintaining a separate names list. Change-Id: I59af37cba5cb78014e0714c654db2501151f605e Signed-off-by: Luc Verhaegen <libv(a)skynet.be> --- M pcidev.c M programmer.h 2 files changed, 165 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/29078/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/29078 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I59af37cba5cb78014e0714c654db2501151f605e Gerrit-Change-Number: 29078 Gerrit-PatchSet: 3 Gerrit-Owner: Luc Verhaegen <libv(a)skynet.be> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Luc Verhaegen <libv(a)skynet.be> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: spi25: Debug flashrom crash when Write Protect is ON
by Edward O'Callaghan (Code Review) 10 May '20

10 May '20

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/40468 ) Change subject: spi25: Debug flashrom crash when Write Protect is ON ...................................................................... spi25: Debug flashrom crash when Write Protect is ON When hardware write protect is applied, flashrom crashed and generate coredump. spi_disable_blockprotect_generic() calls flash->chip->printlock() method when disable was failed, but this method is optional, can be NULL depends on type of flashrom chip. NULL pointer check before call is added to avoid crash. BRANCH=none BUG=b:129083894 TEST=Run on Mistral P2 (On CR50 console, run "wp disable") flashrom --wp-range 0 0x400000 flashrom --wp-enable (On CR50 console, run "wp enable") flashrom -r /tmp/test.bin Verify "Block protection could not be disabled!" is shown, but flash read completes. Signed-off-by: Yuji Sasaki <sasakiy(a)chromium.org> Change-Id: I81094ab5f16a85871fc9869a2e285eddbbbdec4e Reviewed-on: https://chromium-review.googlesource.com/1535140 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator(a)appspot.gserviceaccount.com> Tested-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Stefan Reinauer <reinauer(a)google.com> Reviewed-by: SANTHOSH JANARDHANA HASSAN <sahassan(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/40468 Reviewed-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M spi25_statusreg.c 1 file changed, 2 insertions(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved diff --git a/spi25_statusreg.c b/spi25_statusreg.c index 4b9b2a9..34f9ad4 100644 --- a/spi25_statusreg.c +++ b/spi25_statusreg.c @@ -168,7 +168,8 @@ status = spi_read_status_register(flash); if ((status & bp_mask) != 0) { msg_cerr("Block protection could not be disabled!\n"); - flash->chip->printlock(flash); + if (flash->chip->printlock) + flash->chip->printlock(flash); return 1; } msg_cdbg("disabled.\n"); -- To view, visit https://review.coreboot.org/c/flashrom/+/40468 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I81094ab5f16a85871fc9869a2e285eddbbbdec4e Gerrit-Change-Number: 40468 Gerrit-PatchSet: 5 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: SANTHOSH JANARDHANA HASSAN <sahassan(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Yuji Sasaki <sasakiy(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-CC: Yuji Sasaki <sasakiy(a)google.com> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: realtek_mst_i2c_spi.c: Show MCU running fw version
by Edward O'Callaghan (Code Review) 08 May '20

08 May '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/41158 ) Change subject: realtek_mst_i2c_spi.c: Show MCU running fw version ...................................................................... realtek_mst_i2c_spi.c: Show MCU running fw version NOT-FOR-MERGE yet. Change-Id: I85eafd95833f7a280c46947e280f967489d1b8d1 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M realtek_mst_i2c_spi.c 1 file changed, 26 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/58/41158/1 diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 5c1b901..68ee1a7 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -146,6 +146,28 @@ return ret; } +static int realtek_mst_i2c_spi_version(int fd) +{ + int ret = 0; + + uint8_t command[] = { 0x70, 0x04 }; + ret = realtek_mst_i2c_spi_write_data(fd, REGISTER_ADDRESS, command, 2); + uint8_t buf[3]; + ret |= realtek_mst_i2c_spi_read_data(fd, REGISTER_ADDRESS, buf, 3); + printf("(flash): projectID = %d\n", ((buf[1] & 0xC0) >> 6) + (buf[0] << 2)); + printf("(flash): version = %d.%d\n", (buf[1] & 0x3F), buf[2]); + + uint8_t x0, x1, x2; + ret |= realtek_mst_i2c_spi_read_register(fd, 0x03, &x0); + ret |= realtek_mst_i2c_spi_read_register(fd, 0x04, &x1); + ret |= realtek_mst_i2c_spi_read_register(fd, 0x07, &x2); + + printf("(reg): projectID = %d\n", ((x1 & 0xC0) >> 6) + (x0 << 2)); + printf("(reg): version = %d.%d\n", (x1 & 0x3F), x2); + + return ret; +} + static int realtek_mst_i2c_spi_toggle_protection(int fd, int enable) { int ret = 0; @@ -461,6 +483,10 @@ if (ret) return ret; + ret |= realtek_mst_i2c_spi_version(fd); + if (ret) + return ret; + struct realtek_mst_i2c_spi_data *data = calloc(1, sizeof(struct realtek_mst_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/+/41158 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I85eafd95833f7a280c46947e280f967489d1b8d1 Gerrit-Change-Number: 41158 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 1

Change in flashrom[master]: Subject: raiden_debug_spi.c: Disable USB retry during some error codes
by Brian Nemec (Code Review) 08 May '20

08 May '20

Hello Brian Nemec, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/41152 to review the following change. Change subject: Subject: raiden_debug_spi.c: Disable USB retry during some error codes ...................................................................... Subject: raiden_debug_spi.c: Disable USB retry during some error codes Enables the USB SPI transfer retry mechanism when the write count error code is returned. This error code can indicate a recoverable transfer failure. BUG=b:153887087 BRANCH=none TEST=Manual testing of ServoMicro and Flashrom when performing reads, writes, and verifications of the EC firmware on Nami. TEST=Modified servo micro to produce periodic errors when reading the packet length to verify the recovery is successful. Change-Id: I9e6b2ccec0b06aab0d6920f1bddf108058e5d6b1 --- M raiden_debug_spi.c 1 file changed, 7 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/52/41152/1 diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index 2127f69..ac74c0b 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -188,9 +188,13 @@ static bool retry_recovery(int error_code) { if (error_code < 0x10000) { - /* Handle error codes returned from the device. */ - if (USB_SPI_WRITE_COUNT_INVALID <= error_code && - error_code <= USB_SPI_DISABLED) { + /* + * Handle error codes returned from the device. USB_SPI_TIMEOUT, + * USB_SPI_BUSY, and USB_SPI_WRITE_COUNT_INVALID have been observed + * during transfer errors to the device and can be recovered. + */ + if (USB_SPI_READ_COUNT_INVALID <= error_code && + error_code <= USB_SPI_DISABLED) { return false; } } else if (usb_device_is_libusb_error(error_code)) { -- To view, visit https://review.coreboot.org/c/flashrom/+/41152 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9e6b2ccec0b06aab0d6920f1bddf108058e5d6b1 Gerrit-Change-Number: 41152 Gerrit-PatchSet: 1 Gerrit-Owner: Brian Nemec <bnemec(a)google.com> Gerrit-Reviewer: Brian Nemec <bnemec(a)chromium.org> Gerrit-MessageType: newchange

3 6

Change in flashrom[master]: realtek_mst_i2c_spi.c: Remove dead code
by Edward O'Callaghan (Code Review) 07 May '20

07 May '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/41124 ) Change subject: realtek_mst_i2c_spi.c: Remove dead code ...................................................................... realtek_mst_i2c_spi.c: Remove dead code Turns out the MST likely doesn't need these so-called defaults to be written for the purposes of spi flashing. BUG=b:152558985,b:148745673 BRANCH=none TEST=builds Change-Id: Ieb938cf0805b22692d61db23795208c9be962b60 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M realtek_mst_i2c_spi.c 1 file changed, 0 insertions(+), 33 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/24/41124/1 diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 863ace5..64458dd 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -145,33 +145,6 @@ return ret; } -#if 0 -static int realtek_mst_i2c_spi_set_defaults(int fd) -{ - // 0xFF1B = 0x02; - int ret = realtek_mst_i2c_spi_write_register(fd, 0x1B, 0x02); - ret = realtek_mst_i2c_spi_write_register(fd, 0x1C, 0x30); - ret = realtek_mst_i2c_spi_write_register(fd, 0x1D, 0x1C); - ret = realtek_mst_i2c_spi_write_register(fd, 0x1E, 0x02); - ret = realtek_mst_i2c_spi_write_register(fd, 0x1F, 0x00); - - ret = realtek_mst_i2c_spi_write_register(fd, 0x20, 0x1C); - ret = realtek_mst_i2c_spi_write_register(fd, 0x2C, 0x02); - ret = realtek_mst_i2c_spi_write_register(fd, 0x2D, 0x00); - ret = realtek_mst_i2c_spi_write_register(fd, 0x2E, 0x1C); - - ret = realtek_mst_i2c_spi_write_register(fd, 0x62, 0x06); - ret = realtek_mst_i2c_spi_write_register(fd, 0x6A, 0x03); - ret = realtek_mst_i2c_spi_write_register(fd, 0x6B, 0x0B); - ret = realtek_mst_i2c_spi_write_register(fd, 0x6C, 0x00); - - ret = realtek_mst_i2c_spi_write_register(fd, 0xED, 0x88); - ret = realtek_mst_i2c_spi_write_register(fd, 0xEE, 0x04); - - return ret; -} -#endif - static int realtek_mst_i2c_spi_disable_protection(int fd) { int ret = 0; @@ -484,12 +457,6 @@ ret |= realtek_mst_i2c_spi_enter_isp_mode(fd); if (ret) return ret; -// XXX: maybe make into a mode:defaults cli param? -#if 0 - ret |= realtek_mst_i2c_spi_set_defaults(fd); - if (ret) - return ret; -#endif struct realtek_mst_i2c_spi_data *data = calloc(1, sizeof(struct realtek_mst_i2c_spi_data)); if (!data) { -- To view, visit https://review.coreboot.org/c/flashrom/+/41124 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieb938cf0805b22692d61db23795208c9be962b60 Gerrit-Change-Number: 41124 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

2 2

Change in flashrom[master]: realtek_mst_i2c_spi.c: Fix cmd timeout issue
by Edward O'Callaghan (Code Review) 07 May '20

07 May '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/41123 ) Change subject: realtek_mst_i2c_spi.c: Fix cmd timeout issue ...................................................................... realtek_mst_i2c_spi.c: Fix cmd timeout issue Chip erasures take much longer than sector and bank erasures. Allow the wait loop helper to multiply the max timeout in this very specific case while quickly timeout for other ops that are expected to be shorter. BUG=b:152558985,b:148745673 BRANCH=none TEST=flashrom -p realtek_mst_i2c_spi:bus=8 -E && flashrom -p realtek_mst_i2c_spi:bus=8 -w foo (cycle).. Change-Id: I4a36aa3101827e69eb244775d25bbb476d4bb780 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M realtek_mst_i2c_spi.c 1 file changed, 11 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/41123/1 diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 2117c3c..863ace5 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -96,21 +96,22 @@ return ret ? SPI_GENERIC_ERROR : 0; } -static int realtek_mst_i2c_spi_wait_command_done(int fd, unsigned int offset, int mask, int target) +static int realtek_mst_i2c_spi_wait_command_done(int fd, unsigned int offset, int mask, + int target, int multiplier) { uint8_t val; int tried = 0; int ret = 0; do { ret |= realtek_mst_i2c_spi_read_register(fd, offset, &val); - } while (!ret && ((val & mask) != target) && ++tried < MAX_SPI_WAIT_RETRIES); + } while (!ret && ((val & mask) != target) && ++tried < (MAX_SPI_WAIT_RETRIES*multiplier)); if (tried == MAX_SPI_WAIT_RETRIES) { msg_perr("%s: Time out on sending command.\n", __func__); return -MAX_SPI_WAIT_RETRIES; } - return (val & mask) ? SPI_GENERIC_ERROR : ret; + return (val & mask) != target ? SPI_GENERIC_ERROR : ret; } static int realtek_mst_i2c_spi_enter_isp_mode(int fd) @@ -130,7 +131,7 @@ static int realtek_mst_i2c_execute_write(int fd) { int ret = realtek_mst_i2c_spi_write_register(fd, MCU_MODE, START_WRITE_XFER); - ret |= realtek_mst_i2c_spi_wait_command_done(fd, MCU_MODE, WRITE_XFER_STATUS_MASK, 0); + ret |= realtek_mst_i2c_spi_wait_command_done(fd, MCU_MODE, WRITE_XFER_STATUS_MASK, 0, 1); return ret; } @@ -202,6 +203,7 @@ unsigned char *readarr) { unsigned i; + int max_timeout_mul = 1; int ret = 0; if (writecnt > 4 || readcnt > 3 || writecnt == 0) { @@ -239,8 +241,9 @@ ctrl_reg_val |= (2 << 5); break; /* Erasures require BIT7 && BIT5 set. */ - case JEDEC_CE_60: case JEDEC_CE_C7: + max_timeout_mul *= 20; /* chip erasures take much longer! */ + case JEDEC_CE_60: case JEDEC_BE_52: case JEDEC_BE_D8: case JEDEC_BE_D7: @@ -261,7 +264,7 @@ if (ret) return ret; - ret = realtek_mst_i2c_spi_wait_command_done(fd, 0x60, 0x01, 0); + ret = realtek_mst_i2c_spi_wait_command_done(fd, 0x60, 0x01, 0, max_timeout_mul); if (ret) return ret; @@ -321,7 +324,7 @@ if (ret) return ret; - ret = realtek_mst_i2c_spi_wait_command_done(fd, 0x60, 0x01, 0); + ret = realtek_mst_i2c_spi_wait_command_done(fd, 0x60, 0x01, 0, 1); if (ret) return ret; @@ -373,7 +376,7 @@ break; /* Wait for empty buffer. */ - ret |= realtek_mst_i2c_spi_wait_command_done(fd, MCU_MODE, 0x10, 0x10); + ret |= realtek_mst_i2c_spi_wait_command_done(fd, MCU_MODE, 0x10, 0x10, 1); if (ret) break; -- To view, visit https://review.coreboot.org/c/flashrom/+/41123 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4a36aa3101827e69eb244775d25bbb476d4bb780 Gerrit-Change-Number: 41123 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

2 3

Change in flashrom[master]: realtek_mst_i2c_spi.c: Fix wp gpio toggle
by Edward O'Callaghan (Code Review) 07 May '20

07 May '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/41125 ) Change subject: realtek_mst_i2c_spi.c: Fix wp gpio toggle ...................................................................... realtek_mst_i2c_spi.c: Fix wp gpio toggle Allow for toggling the GPIO pin 108 that is routed to the WP pin in the reference design. This need not always apply however is largely harmless for a better out-of-the-box experience. BUG=b:152558985,b:148745673 BRANCH=none TEST=builds && A double cycle of erase+write works. Change-Id: I825df3179cdd16f7c7ecf556b5ca29bfe6394346 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M realtek_mst_i2c_spi.c 1 file changed, 8 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/25/41125/1 diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 64458dd..6cb87fe 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -34,6 +34,7 @@ #define MCU_MODE 0x6F #define ENTER_ISP_MODE 0x80 #define START_WRITE_XFER 0xA0 +#define BUF_XFER_STATUS_MASK 0x10 #define WRITE_XFER_STATUS_MASK 0x20 #define MCU_DATA_PORT 0x70 @@ -145,7 +146,7 @@ return ret; } -static int realtek_mst_i2c_spi_disable_protection(int fd) +static int realtek_mst_i2c_spi_toggle_protection(int fd, int enable) { int ret = 0; uint8_t val = 0; @@ -164,8 +165,9 @@ ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, (val & 0xF8) | 0x01); /* Set pin value to high, 0xFFD7[0] = 1. */ + uint8_t en = enable ? 0x00 : 0x01; ret |= realtek_mst_i2c_spi_read_register(fd, 0xD7, &val); - ret |= realtek_mst_i2c_spi_write_register(fd, 0xD7, (val & 0xFE) | 0x01); + ret |= realtek_mst_i2c_spi_write_register(fd, 0xD7, (val & 0xFE) | en); return ret; } @@ -331,7 +333,7 @@ if (fd < 0) return SPI_GENERIC_ERROR; - ret = realtek_mst_i2c_spi_disable_protection(fd); + ret = realtek_mst_i2c_spi_toggle_protection(fd, 0); if (ret) return ret; @@ -349,7 +351,8 @@ break; /* Wait for empty buffer. */ - ret |= realtek_mst_i2c_spi_wait_command_done(fd, MCU_MODE, 0x10, 0x10, 1); + ret |= realtek_mst_i2c_spi_wait_command_done(fd, MCU_MODE, + BUF_XFER_STATUS_MASK, 0x10, 1); if (ret) break; @@ -362,8 +365,7 @@ break; } - - /* TODO: re-enable the write protection? */ + ret |= realtek_mst_i2c_spi_toggle_protection(fd, 1); return ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/41125 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I825df3179cdd16f7c7ecf556b5ca29bfe6394346 Gerrit-Change-Number: 41125 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

2 2

Change in flashrom[master]: realtek_mst_i2c_spi.c: Fix _spi_write256() as documented
by Edward O'Callaghan (Code Review) 06 May '20

06 May '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/41080 ) Change subject: realtek_mst_i2c_spi.c: Fix _spi_write256() as documented ...................................................................... realtek_mst_i2c_spi.c: Fix _spi_write256() as documented Turns out broken erasures highlighted some of the issues in the write256 implementation. After a fair amount of time deciphering scarce documention details a correct implementation was finally derived. BUG=b:152558985,b:148745673 BRANCH=none TEST=flashrom -p realtek_mst_i2c_spi:bus=8 -E && flashrom -p realtek_mst_i2c_spi:bus=8 -w foo && flashrom -p realtek_mst_i2c_spi:bus=8 -r foo && hexdump -C foo Change-Id: If61ff95697f886d3301a907b76283322c39ef5c7 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M realtek_mst_i2c_spi.c 1 file changed, 51 insertions(+), 22 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/80/41080/1 diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 682d4a6..6a3bac5 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -33,6 +33,8 @@ #define MCU_MODE 0x6F #define ENTER_ISP_MODE 0x80 +#define START_PROGRAM 0xA0 +#define PROGRAM_STATUS_MASK 0x20 #define MCU_DATA_PORT 0x70 @@ -94,14 +96,14 @@ return ret ? SPI_GENERIC_ERROR : 0; } -static int realtek_mst_i2c_spi_wait_command_done(int fd, unsigned int offset, int mask) +static int realtek_mst_i2c_spi_wait_command_done(int fd, unsigned int offset, int mask, int target) { uint8_t val; int tried = 0; int ret = 0; do { ret |= realtek_mst_i2c_spi_read_register(fd, offset, &val); - } while (!ret && (val & mask) && ++tried < MAX_SPI_WAIT_RETRIES); + } while (!ret && ((val & mask) != target) && ++tried < MAX_SPI_WAIT_RETRIES); if (tried == MAX_SPI_WAIT_RETRIES) { msg_perr("%s: Time out on sending command.\n", __func__); @@ -125,6 +127,13 @@ return ret; } +static int realtek_mst_i2c_start_program(int fd) +{ + int ret = realtek_mst_i2c_spi_write_register(fd, MCU_MODE, START_PROGRAM); + ret |= realtek_mst_i2c_spi_wait_command_done(fd, MCU_MODE, PROGRAM_STATUS_MASK, 0); + return ret; +} + static int realtek_mst_i2c_spi_reset_mpu(int fd) { int ret = 0; @@ -252,7 +261,7 @@ if (ret) return ret; - ret = realtek_mst_i2c_spi_wait_command_done(fd, 0x60, 0x01); + ret = realtek_mst_i2c_spi_wait_command_done(fd, 0x60, 0x01, 0); if (ret) return ret; @@ -272,6 +281,21 @@ return ret ? SPI_GENERIC_ERROR : 0; } +static int realtek_mst_i2c_spi_write_page(int fd, uint8_t reg, const uint8_t *buf, unsigned int len) +{ + /** + * Using static buffer with maximum possible size, + * extra byte is needed for prefixing zero at index 0. + */ + uint8_t wbuf[PAGE_SIZE + 1] = { 0x70 }; + if (len > PAGE_SIZE) + return SPI_GENERIC_ERROR; + + memcpy(&wbuf[1], buf, len); + + return realtek_mst_i2c_spi_write_data(fd, REGISTER_ADDRESS, wbuf, len + 1); +} + static int realtek_mst_i2c_spi_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { @@ -297,7 +321,7 @@ if (ret) return ret; - ret = realtek_mst_i2c_spi_wait_command_done(fd, 0x60, 0x01); + ret = realtek_mst_i2c_spi_wait_command_done(fd, 0x60, 0x01, 0); if (ret) return ret; @@ -335,30 +359,35 @@ if (ret) return ret; - start--; - ret |= realtek_mst_i2c_spi_write_register(fd, 0x60, 0x46); // ** - ret |= realtek_mst_i2c_spi_write_register(fd, 0x61, OPCODE_WRITE); - uint8_t block_idx = start >> 16; - uint8_t page_idx = start >> 8; - uint8_t byte_idx = start; - ret |= realtek_mst_i2c_spi_map_page(fd, block_idx, page_idx, byte_idx); - ret |= realtek_mst_i2c_spi_write_register(fd, 0x6a, 0x03); - ret |= realtek_mst_i2c_spi_write_register(fd, 0x60, 0x47); // ** - if (ret) - goto fail; - - ret = realtek_mst_i2c_spi_wait_command_done(fd, 0x60, 0x01); - if (ret) - goto fail; + ret |= realtek_mst_i2c_spi_write_register(fd, 0x6D, 0x02); /* write opcode */ + ret |= realtek_mst_i2c_spi_write_register(fd, 0x71, (PAGE_SIZE - 1)); /* fit len=256 */ for (i = 0; i < len; i += PAGE_SIZE) { - ret |= realtek_mst_i2c_spi_write_data(fd, REGISTER_ADDRESS, - (uint8_t *)buf + i, min(len - i, PAGE_SIZE)); + uint16_t page_len = min(len - i, PAGE_SIZE); + if (len - i < PAGE_SIZE) + ret |= realtek_mst_i2c_spi_write_register(fd, 0x71, page_len-1); + uint8_t block_idx = (start + i) >> 16; + uint8_t page_idx = (start + i) >> 8; + ret |= realtek_mst_i2c_spi_map_page(fd, block_idx, page_idx, 0); if (ret) break; + + /* Wait for empty buffer. */ + ret |= realtek_mst_i2c_spi_wait_command_done(fd, MCU_MODE, 0x10, 0x10); + if (ret) + break; + + ret |= realtek_mst_i2c_spi_write_page(fd, MCU_DATA_PORT, + buf + i, page_len); + if (ret) + break; + ret |= realtek_mst_i2c_start_program(fd); + if (ret) { + break; + } } -fail: + /* TODO: re-enable the write protection? */ return ret; -- To view, visit https://review.coreboot.org/c/flashrom/+/41080 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If61ff95697f886d3301a907b76283322c39ef5c7 Gerrit-Change-Number: 41080 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

3 11

Change in flashrom[master]: realtek_mst_i2c_spi.c: Fix _spi_send_command cb for erasures
by Edward O'Callaghan (Code Review) 06 May '20

06 May '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/41079 ) Change subject: realtek_mst_i2c_spi.c: Fix _spi_send_command cb for erasures ...................................................................... realtek_mst_i2c_spi.c: Fix _spi_send_command cb for erasures Before issuing SPI opcodes into 0x61 the top three BITS of 0x60 need to be carefully crafted. Correctly craft these in the case of SPI erasures and document this registers expectations. Clean up remaining debug comments while we are here. BUG=b:152558985,b:148745673 BRANCH=none TEST=flashrom -p realtek_mst_i2c_spi:bus=8 -E && flashrom -p realtek_mst_i2c_spi:bus=8 -r foo && hexdump -C foo Change-Id: Ib11ba8f63b11a1c5ebaa68deb7971648de8c2ecd Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M realtek_mst_i2c_spi.c 1 file changed, 40 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/41079/1 diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 2dd4936..682d4a6 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -196,21 +196,55 @@ int ret = 0; if (writecnt > 4 || readcnt > 3 || writecnt == 0) { - //msg_perr("%s: Invalid read/write count for send command.\n", __func__); return SPI_GENERIC_ERROR; } - //printf("%s: writearr[0]=0x%x\n", __func__, writearr[0]); int fd = get_fd_from_context(flash); if (fd < 0) return SPI_GENERIC_ERROR; - /* First byte of writearr should be the command value, followed by the value to write. */ - // 0xFF60 = cmd; + /* First byte of writearr should be the spi opcode value, followed by the value to write. */ writecnt--; - uint8_t ctrl_reg_val = (writecnt << 3) | (readcnt << 1) | (2 << 5); + + /** + * Before dispatching a SPI opcode the MCU register 0x60 requires + * the following configuration byte set: + * + * BIT0 - start [0] , end [1]. + * BITS[1-4] - counts. + * BITS[5-7] - opcode type. + * + * | bit7 | bit6 | bit5 | + * +------+------+------+ + * | 0 | 1 | 0 | ~ JEDEC_RDID,REMS,READ + * | 0 | 1 | 1 | ~ JEDEC_WRSR + * | 1 | 0 | 1 | ~ JEDEC_.. erasures. + */ + uint8_t ctrl_reg_val = (writecnt << 3) | (readcnt << 1); + switch (writearr[0]) { + /* WREN isn't a supported somehow? ignore it. */ + case JEDEC_WREN: return 0; + /* WRSR requires BIT6 && BIT5 set. */ + case JEDEC_WRSR: + ctrl_reg_val |= (1 << 5); + ctrl_reg_val |= (2 << 5); + break; + /* Erasures require BIT7 && BIT5 set. */ + case JEDEC_CE_60: + case JEDEC_CE_C7: + case JEDEC_BE_52: + case JEDEC_BE_D8: + case JEDEC_BE_D7: + case JEDEC_SE: + ctrl_reg_val |= (1 << 5); + ctrl_reg_val |= (4 << 5); + break; + default: + /* Otherwise things like RDID,REMS,READ require BIT6 */ + ctrl_reg_val |= (2 << 5); + } ret |= realtek_mst_i2c_spi_write_register(fd, 0x60, ctrl_reg_val); - ret |= realtek_mst_i2c_spi_write_register(fd, 0x61, writearr[0]); + ret |= realtek_mst_i2c_spi_write_register(fd, 0x61, writearr[0]); /* opcode */ for (i = 0; i < writecnt; ++i) ret |= realtek_mst_i2c_spi_write_register(fd, 0x64 + i, writearr[i + 1]); @@ -225,9 +259,6 @@ for (i = 0; i < readcnt; ++i) ret |= realtek_mst_i2c_spi_read_register(fd, 0x67 + i, &readarr[i]); - //for (i = 0; i< readcnt; i++) - // printf("DEBUG: readarr[%d]=0x%02x\n", i, readarr[i]); - return ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/41079 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib11ba8f63b11a1c5ebaa68deb7971648de8c2ecd Gerrit-Change-Number: 41079 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

2 3

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2020