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@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;
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41080 )
Change subject: realtek_mst_i2c_spi.c: Fix _spi_write256() as documented ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/flashrom/+/41080/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/41080/1/realtek_mst_i2c_spi.c@130 PS1, Line 130: realtek_mst_i2c_start_program How about ..._run_program (since it waits for completion)?
Hello Sam McNally, build bot (Jenkins), Shiyu Sun, Angel Pons, Edward Hill,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41080
to look at the new patch set (#2).
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.
V.2: Rename 'start_program() -> execute_write()' to clarify the intention and not to overload the term 'program' since the MST actually runs a 'program' itself.
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@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/2
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41080 )
Change subject: realtek_mst_i2c_spi.c: Fix _spi_write256() as documented ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/flashrom/+/41080/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/41080/1/realtek_mst_i2c_spi.c@130 PS1, Line 130: realtek_mst_i2c_start_program
How about ... […]
Done
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41080 )
Change subject: realtek_mst_i2c_spi.c: Fix _spi_write256() as documented ......................................................................
Patch Set 2: Code-Review+2
Hello Sam McNally, build bot (Jenkins), Shiyu Sun, Angel Pons, Edward Hill,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41080
to look at the new patch set (#3).
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.
V.2: Rename 'start_program() -> execute_write()' to clarify the intention and not to overload the term 'program' since the MST actually runs a 'program' itself.
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@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/3
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41080 )
Change subject: realtek_mst_i2c_spi.c: Fix _spi_write256() as documented ......................................................................
Patch Set 3: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41080 )
Change subject: realtek_mst_i2c_spi.c: Fix _spi_write256() as documented ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/flashrom/+/41080/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/41080/3//COMMIT_MSG@11 PS3, Line 11: documention documentation
https://review.coreboot.org/c/flashrom/+/41080/3/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/41080/3/realtek_mst_i2c_spi.c@36 PS3, Line 36: One space as above?
https://review.coreboot.org/c/flashrom/+/41080/3/realtek_mst_i2c_spi.c@387 PS3, Line 387: } Above you do not add {} around the one line branch.
Hello Sam McNally, build bot (Jenkins), Shiyu Sun, Angel Pons, Edward Hill,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41080
to look at the new patch set (#4).
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 documentation details a correct implementation was finally derived.
V.2: Rename 'start_program() -> execute_write()' to clarify the intention and not to overload the term 'program' since the MST actually runs a 'program' itself.
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@google.com --- M realtek_mst_i2c_spi.c 1 file changed, 50 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/80/41080/4
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41080 )
Change subject: realtek_mst_i2c_spi.c: Fix _spi_write256() as documented ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/flashrom/+/41080/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/41080/3//COMMIT_MSG@11 PS3, Line 11: documention
documentation
Done
https://review.coreboot.org/c/flashrom/+/41080/3/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/41080/3/realtek_mst_i2c_spi.c@36 PS3, Line 36:
One space as above?
Done
https://review.coreboot.org/c/flashrom/+/41080/3/realtek_mst_i2c_spi.c@387 PS3, Line 387: }
Above you do not add {} around the one line branch.
Done
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41080 )
Change subject: realtek_mst_i2c_spi.c: Fix _spi_write256() as documented ......................................................................
Patch Set 4: Code-Review+2
Edward O'Callaghan has submitted this change. ( 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 documentation details a correct implementation was finally derived.
V.2: Rename 'start_program() -> execute_write()' to clarify the intention and not to overload the term 'program' since the MST actually runs a 'program' itself.
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@google.com Reviewed-on: https://review.coreboot.org/c/flashrom/+/41080 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, 50 insertions(+), 22 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 682d4a6..2117c3c 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_WRITE_XFER 0xA0 +#define WRITE_XFER_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_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); + 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 the data port register at index 0. + */ + uint8_t wbuf[PAGE_SIZE + 1] = { MCU_DATA_PORT }; + 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,34 @@ 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_execute_write(fd); if (ret) break; }
-fail: + /* TODO: re-enable the write protection? */
return ret;
-
Edward O'Callaghan (Code Review) -
Paul Menzel (Code Review)
-
Sam McNally (Code Review)