Shiyu Sun has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/48840 )
Change subject: realtek_mst_i2c_spi.c: Update PAGE_SIZE and fix write ......................................................................
realtek_mst_i2c_spi.c: Update PAGE_SIZE and fix write
Update the PAGE_SIZE to 128 as fix r/w on different devices, also fix the write page mapping for it.
BUG=b:147402710 TEST=build and run flashrom to read&write on multiple devices
Signed-off-by: Shiyu Sun sshiyu@chromium.org Change-Id: Ifcdd3548519eb37440e67fcf6206279cff05b159 --- M Makefile M realtek_mst_i2c_spi.c 2 files changed, 5 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/40/48840/1
diff --git a/Makefile b/Makefile index 0498624..a79bb82 100644 --- a/Makefile +++ b/Makefile @@ -737,10 +737,10 @@ CONFIG_STLINKV3_SPI ?= yes
# Disables LSPCON support until the i2c helper supports multiple systems. -CONFIG_LSPCON_I2C_SPI ?= no +CONFIG_LSPCON_I2C_SPI ?= yes
# Disables REALTEK_MST support until the i2c helper supports multiple systems. -CONFIG_REALTEK_MST_I2C_SPI ?= no +CONFIG_REALTEK_MST_I2C_SPI ?= yes
# Always enable dummy tracing for now. CONFIG_DUMMY ?= yes diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 80caf86..dabec70 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -28,7 +28,7 @@
#define MCU_I2C_SLAVE_ADDR 0x94 #define REGISTER_ADDRESS (0x94 >> 1) -#define PAGE_SIZE 256 +#define PAGE_SIZE 128 #define MAX_SPI_WAIT_RETRIES 1000
#define MCU_MODE 0x6F @@ -386,7 +386,8 @@ 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); + uint8_t byte_idx = start + i; + ret |= realtek_mst_i2c_spi_map_page(fd, block_idx, page_idx, byte_idx); if (ret) break;
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/48840
to look at the new patch set (#2).
Change subject: realtek_mst_i2c_spi.c: Update PAGE_SIZE and fix write ......................................................................
realtek_mst_i2c_spi.c: Update PAGE_SIZE and fix write
Update the PAGE_SIZE to 128 as fix r/w on different devices, also fix the write page mapping for it.
BUG=b:147402710 TEST=build and run flashrom to read&write on multiple devices
Signed-off-by: Shiyu Sun sshiyu@chromium.org Change-Id: Ifcdd3548519eb37440e67fcf6206279cff05b159 --- M realtek_mst_i2c_spi.c 1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/40/48840/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48840 )
Change subject: realtek_mst_i2c_spi.c: Update PAGE_SIZE and fix write ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/flashrom/+/48840/2/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/48840/2/realtek_mst_i2c_spi.c@387 PS2, Line 387: uint8_t block_idx = (start + i) >> 16; : uint8_t page_idx = (start + i) >> 8; : uint8_t byte_idx = start + i; I don't like that this code relies on overflow behavior. I would instead explicitly mask the values (`& 0xff`).
Also, wouldn't it be better to pass `start + i` to `realtek_mst_i2c_spi_map_page` and split the values inside the function? I see this pattern on the two places where `realtek_mst_i2c_spi_map_page` is called.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48840 )
Change subject: realtek_mst_i2c_spi.c: Update PAGE_SIZE and fix write ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/flashrom/+/48840/2/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/48840/2/realtek_mst_i2c_spi.c@387 PS2, Line 387: uint8_t block_idx = (start + i) >> 16; : uint8_t page_idx = (start + i) >> 8; : uint8_t byte_idx = start + i;
I don't like that this code relies on overflow behavior. […]
Generally agree with the first point but I think it's out of scope of this change and rather have this merged first to fix the immediate problem of brokenness then follow up with style clean ups.
Same for the second point, although I see what you are saying about consolidating the pattern it is also nice to have realtek_mst_i2c_spi_map_page() with its current signature? The pattern being on both R/W primitives was speculative before as we worked our way though understanding the MCU. Therefore I rather we keep things flexible until everything definitely fully works on all hw variants. Like you said before, we should avoid bolting on too many patches into the same change.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48840 )
Change subject: realtek_mst_i2c_spi.c: Update PAGE_SIZE and fix write ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/flashrom/+/48840/2/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/48840/2/realtek_mst_i2c_spi.c@387 PS2, Line 387: uint8_t block_idx = (start + i) >> 16; : uint8_t page_idx = (start + i) >> 8; : uint8_t byte_idx = start + i;
Generally agree with the first point but I think it's out of scope of this change and rather have th […]
Yeah, I'm fine with doing this on another change. Thanks for acknowledging!
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/48840 )
Change subject: realtek_mst_i2c_spi.c: Update PAGE_SIZE and fix write ......................................................................
realtek_mst_i2c_spi.c: Update PAGE_SIZE and fix write
Update the PAGE_SIZE to 128 as fix r/w on different devices, also fix the write page mapping for it.
BUG=b:147402710 TEST=build and run flashrom to read&write on multiple devices
Signed-off-by: Shiyu Sun sshiyu@chromium.org Change-Id: Ifcdd3548519eb37440e67fcf6206279cff05b159 Reviewed-on: https://review.coreboot.org/c/flashrom/+/48840 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Edward O'Callaghan quasisec@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M realtek_mst_i2c_spi.c 1 file changed, 3 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, but someone else must approve Edward O'Callaghan: Looks good to me, approved
diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 80caf86..dabec70 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -28,7 +28,7 @@
#define MCU_I2C_SLAVE_ADDR 0x94 #define REGISTER_ADDRESS (0x94 >> 1) -#define PAGE_SIZE 256 +#define PAGE_SIZE 128 #define MAX_SPI_WAIT_RETRIES 1000
#define MCU_MODE 0x6F @@ -386,7 +386,8 @@ 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); + uint8_t byte_idx = start + i; + ret |= realtek_mst_i2c_spi_map_page(fd, block_idx, page_idx, byte_idx); if (ret) break;
-
Angel Pons (Code Review) -
Edward O'Callaghan (Code Review)
-
Shiyu Sun (Code Review)