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@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;
Hello Sam McNally, build bot (Jenkins), Shiyu Sun, Edward Hill,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41123
to look at the new patch set (#2).
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.
V.2: Fix nonsense fall though warn-err
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@google.com --- M realtek_mst_i2c_spi.c 1 file changed, 12 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/41123/2
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41123 )
Change subject: realtek_mst_i2c_spi.c: Fix cmd timeout issue ......................................................................
Patch Set 2: Code-Review+2
Edward O'Callaghan has submitted this change. ( 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.
V.2: Fix nonsense fall though warn-err
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@google.com Reviewed-on: https://review.coreboot.org/c/flashrom/+/41123 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Sam McNally sammc@google.com --- M realtek_mst_i2c_spi.c 1 file changed, 12 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Sam McNally: Looks good to me, approved
diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 2117c3c..e9ee576 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,10 @@ 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! */ + /* FALLTHRU */ + case JEDEC_CE_60: case JEDEC_BE_52: case JEDEC_BE_D8: case JEDEC_BE_D7: @@ -261,7 +265,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 +325,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 +377,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;
-
Edward O'Callaghan (Code Review) -
Sam McNally (Code Review)