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@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; }
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41125 )
Change subject: realtek_mst_i2c_spi.c: Fix wp gpio toggle ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/flashrom/+/41125/2/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/41125/2/realtek_mst_i2c_spi.c@167 PS2, Line 167: /* Set pin value to high, 0xFFD7[0] = 1. */ Update the comment.
https://review.coreboot.org/c/flashrom/+/41125/2/realtek_mst_i2c_spi.c@337 PS2, Line 337: ret = realtek_mst_i2c_spi_toggle_protection(fd, 0); This seems like a strange place to do this considering this write protect should control whether the flash chip's write protect can be disabled.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41125 )
Change subject: realtek_mst_i2c_spi.c: Fix wp gpio toggle ......................................................................
Patch Set 2: Code-Review-2
Patch Set 2:
(2 comments)
Do you think it is worth even keeping this gpio self-wp assertion function logic? Maybe the gpio mask should be a param as well if it is to be done properly or maybe it is actually out of scope of flashrom? It's sort of weird case because of the chimera properties of being both a spi master to a slave spi flash and at the same time a MCU.
-2 for now until the direction is fully decided or if we learn more about the chip soon.
-
Edward O'Callaghan (Code Review) -
Sam McNally (Code Review)