Change in flashrom[master]: realtek_mst_i2c_spi.c: Update wp disable function and add param
Shiyu Sun has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update wp disable function and add param ......................................................................
realtek_mst_i2c_spi.c: Update wp disable function and add param
The write protection trigger has been updated by the vendor so update here as well. Also added parameter to allower user to decide turn off protection or not before an operation.
Change-Id: I1aed0086f917e31bebb51857ad5cce138158fe82 Signed-off-by: Shiyu Sun sshiyu@chromium.org --- M realtek_mst_i2c_spi.c 1 file changed, 43 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/46089/1
diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index d1f7227..2e5872e 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -146,27 +146,36 @@ return ret; }
-static int realtek_mst_i2c_spi_disable_protection(int fd) +static int realtek_mst_i2c_spi_disable_hw_protection(int fd) { int ret = 0; uint8_t val = 0; - // 0xAB[2:0] = b001
+ // Configure Pin to Push-Pull GPIO ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, 0x10); - ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0xAB); + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x4F);
ret |= realtek_mst_i2c_spi_read_register(fd, 0xF5, &val);
ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, 0x10); - ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0xAB); + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x4F);
- ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, (val & 0xF8) | 0x01); + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, (val & 0xF0) | 0x01);
- /* Set pin value to high, 0xFFD7[0] = 1. */ - ret |= realtek_mst_i2c_spi_read_register(fd, 0xD7, &val); - ret |= realtek_mst_i2c_spi_write_register(fd, 0xD7, (val & 0xFE) | 0x01); + // Set Pin Value to High + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, 0xFE); + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x3F); + + ret |= realtek_mst_i2c_spi_read_register(fd, 0xF5, &val); + + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, 0xFE); + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x3F); + + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, (val & 0xFE) | 0x01);
return ret; } @@ -333,10 +342,6 @@ if (fd < 0) return SPI_GENERIC_ERROR;
- ret = realtek_mst_i2c_spi_disable_protection(fd); - if (ret) - return ret; - 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 */
@@ -404,9 +409,9 @@ return ret; }
-static int get_params(int *i2c_bus, int *reset) +static int get_params(int *i2c_bus, int *reset, int *wp_disable) { - char *bus_str = NULL, *reset_str = NULL; + char *bus_str = NULL, *reset_str = NULL, *wp_str = NULL; int ret = SPI_GENERIC_ERROR;
bus_str = extract_programmer_param("bus"); @@ -450,6 +455,20 @@ *reset = 0; /* Default behaviour is no MCU reset on tear-down. */ free(reset_str);
+ wp_str = extract_programmer_param("wp-disable"); + if (wp_str) { + if (wp_str[0] == '1') + *wp_disable = 1; + else if (wp_str[0] == '0') + *wp_disable = 0; + else { + msg_perr("%s: Incorrect param format, wp-disable=1 or 0.\n", __func__); + ret = SPI_GENERIC_ERROR; + } + } else + *wp_disable = 0; /* Default behaviour is not have hardware write protect disabled. */ + free(wp_str); + _get_params_failed: if (bus_str) free(bus_str); @@ -460,9 +479,9 @@ int realtek_mst_i2c_spi_init(void) { int ret = 0; - int i2c_bus = 0, reset = 0; + int i2c_bus = 0, reset = 0, wp_disable = 0;
- if (get_params(&i2c_bus, &reset)) + if (get_params(&i2c_bus, &reset, &wp_disable)) return SPI_GENERIC_ERROR;
int fd = i2c_open(i2c_bus, REGISTER_ADDRESS, 0); @@ -473,6 +492,14 @@ if (ret) return ret;
+ if (wp_disable) { + ret |= realtek_mst_i2c_spi_disable_hw_protection(fd); + if (ret != 0) { + msg_perr("%s: failed to disable the write protection.\n", __func__); + 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");
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update wp disable function and add param ......................................................................
Patch Set 1: Code-Review-1
(7 comments)
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@a336 PS1, Line 336: ret = realtek_mst_i2c_spi_disable_protection(fd); : if (ret) : return ret; This change should just generalise this functions implementation and not too much else. A second follow up patch then leverages those generalisations.
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@149 PS1, Line 149: disable this function shouldn't be just _disable_, it should toggle and be parametric with a bool to toggle the bit mask '0x01' as seen below.
`static int realtek_mst_i2c_spi_toggle_gpioXX_strap(int fd, bool toggle)` where XX denotes the gpio #, or if you can indentify it below perhaps have that as a param as well.
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@154 PS1, Line 154: // Configure Pin to Push-Pull GPIO `/* 0x4F[2:0] = b001 */`
what do you mean push-pull? avoid these C++ style comments btw as it is inconsistent.
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@165 PS1, Line 165: 0xF0 these magics getting changed, looks like they should have a #define
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@167 PS1, Line 167: // Set Pin Value to High the comment was fine and very informative before, leave it.
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@178 PS1, Line 178: 0xFE 0xF0 0xFE
there is a pattern here.
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@495 PS1, Line 495: if (wp_disable) { : ret |= realtek_mst_i2c_spi_disable_hw_protection(fd); : if (ret != 0) { : msg_perr("%s: failed to disable the write protection.\n", __func__); : return ret; : } : } : I don't think wp should be toggled via this mechanism by using a bespoke spi master parameter. Rather it should be via the --wp-[en|dis]able parameter. Just as I mentioned before, perhaps we could catch the request in the spi_send_command call back and dispatch to the local implementation detail internally here.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update wp disable function and add param ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@165 PS1, Line 165: 0xF0
these magics getting changed, looks like they should have a #define
Specifically, 0xF8 -> 0xF0 seems suspect to change that mask, why are we touching the high bit?
You deleted the above comment but are you sure you understood it? `// 0xAB[2:0] = b001` if I draw it out here it maybe more clear:
``` 0xF8 - 1111 1 000 0xF0 - 1111 0 000 ```
this would mean we are overwriting 0x4F[3:0] worth of `val` and not 0x4F[2:0], is that intended and if so why?
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@167 PS1, Line 167: // Set Pin Value to High
the comment was fine and very informative before, leave it.
`/* Toggle pin value, 0xFFF5[0] = |toggle|. */`
and move the comment above the actual masking below.
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@176 PS1, Line 176: 0x3F `#define GPIO_CONF 0x3F` `#define GPIO_DIRECTION 0x4F`
or something to that effect. Probably the pin number is encoded somewhere here that you can identify to fully see the whole picture that is happening here?
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@178 PS1, Line 178: 0xFE
0xF0 […]
`0xFE - 1111 111 0`
in other words "keep the rest of val the same and just mask off whatever was in the low bit and or the 0 with 0x01 to toggle it".
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update wp disable function and add param ......................................................................
Patch Set 1:
(9 comments)
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@154 PS1, Line 154: // Configure Pin to Push-Pull GPIO
`/* 0x4F[2:0] = b001 */` […]
/* read 0x4F into val. */
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@155 PS1, Line 155: ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, 0x10); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x4F); : : ret |= realtek_mst_i2c_spi_read_register(fd, 0xF5, &val); : /* write 0x4F[3:0] = b0001 */
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@162 PS1, Line 162: 0x10 Q: Need to find out where 0x10 is coming from?
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@165 PS1, Line 165: 0xF0
Specifically, 0xF8 -> 0xF0 seems suspect to change that mask, why are we touching the high bit? […]
Q: Need to ask what this low byte represents precisely?
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@167 PS1, Line 167: // Set Pin Value to High
`/* Toggle pin value, 0xFFF5[0] = |toggle|. */` […]
/* read 0x3F into val. */
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@174 PS1, Line 174: /* write 0x3F[1:0] = b|toggle| */
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@175 PS1, Line 175: 0xFE Q: like 0x10 above, should know a bit better where this value came from?
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@176 PS1, Line 176: 0x3F
`#define GPIO_CONF 0x3F` […]
Actually in retrospect it looks like #define GPIO_CONFIG 0x4F and #define GPIO_DATA 0x3F
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@178 PS1, Line 178: 0xFE
`0xFE - 1111 111 0` […]
ignore I misread thinking xor but we should know what this register space represents, currently think this is the state value of the pins.
Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/46089
to look at the new patch set (#2).
Change subject: realtek_mst_i2c_spi.c: Update wp disable function and add param ......................................................................
realtek_mst_i2c_spi.c: Update wp disable function and add param
The write protection trigger has been updated by the vendor so update here as well. Also added parameter to allower user to decide turn off protection or not before an operation.
Change-Id: I1aed0086f917e31bebb51857ad5cce138158fe82 Signed-off-by: Shiyu Sun sshiyu@chromium.org --- M realtek_mst_i2c_spi.c 1 file changed, 22 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/46089/2
Shiyu Sun has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update wp disable function and add param ......................................................................
Patch Set 1:
(8 comments)
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@a336 PS1, Line 336: ret = realtek_mst_i2c_spi_disable_protection(fd); : if (ret) : return ret;
This change should just generalise this functions implementation and not too much else. […]
Done
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@149 PS1, Line 149: disable
this function shouldn't be just _disable_, it should toggle and be parametric with a bool to toggle […]
Done
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@154 PS1, Line 154: // Configure Pin to Push-Pull GPIO
/* read 0x4F into val. […]
Done
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@155 PS1, Line 155: ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, 0x10); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x4F); : : ret |= realtek_mst_i2c_spi_read_register(fd, 0xF5, &val); :
/* write 0x4F[3:0] = b0001 */
Done
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@167 PS1, Line 167: // Set Pin Value to High
/* read 0x3F into val. […]
Done
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@174 PS1, Line 174:
/* write 0x3F[1:0] = b|toggle| */
Done
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@176 PS1, Line 176: 0x3F
Actually in retrospect it looks like #define GPIO_CONFIG 0x4F and #define GPIO_DATA 0x3F
Done
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@495 PS1, Line 495: if (wp_disable) { : ret |= realtek_mst_i2c_spi_disable_hw_protection(fd); : if (ret != 0) { : msg_perr("%s: failed to disable the write protection.\n", __func__); : return ret; : } : } :
I don't think wp should be toggled via this mechanism by using a bespoke spi master parameter. […]
As discussed we will revisit this on the following cls. Just removed this for now.
Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/46089
to look at the new patch set (#3).
Change subject: realtek_mst_i2c_spi.c: Update wp disable function ......................................................................
realtek_mst_i2c_spi.c: Update wp disable function
The write protection trigger has been updated by the vendor so update here as well.
Change-Id: I1aed0086f917e31bebb51857ad5cce138158fe82 Signed-off-by: Shiyu Sun sshiyu@chromium.org --- M realtek_mst_i2c_spi.c 1 file changed, 22 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/46089/3
Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/46089
to look at the new patch set (#4).
Change subject: realtek_mst_i2c_spi.c: Update wp disable function ......................................................................
realtek_mst_i2c_spi.c: Update wp disable function
The write protection trigger has been updated by the vendor so update here as well.
Change-Id: I1aed0086f917e31bebb51857ad5cce138158fe82 Signed-off-by: Shiyu Sun sshiyu@chromium.org --- M realtek_mst_i2c_spi.c 1 file changed, 26 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/46089/4
Shiyu Sun has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update wp disable function ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@162 PS1, Line 162: 0x10
Q: Need to find out where 0x10 is coming from?
Done
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@165 PS1, Line 165: 0xF0
Q: Need to ask what this low byte represents precisely?
I think from the Realtek's update it's required that last byte be 0x01 to enable the setting on pin value. Other value would considered wrong.
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@175 PS1, Line 175: 0xFE
Q: like 0x10 above, should know a bit better where this value came from?
Done
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update wp disable function ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/flashrom/+/46089/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/46089/4//COMMIT_MSG@9 PS4, Line 9: The write protection trigger has been updated by the vendor so update : here as well. Please add a little more detail for external reviewers so that everyone understands the code well and folks can look back in git history to understand it as well.
Also please add BUG/BRANCH=puff and TEST lines.
https://review.coreboot.org/c/flashrom/+/46089/4/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/4/realtek_mst_i2c_spi.c@49 PS4, Line 49: HIGH perhaps, 'HIGHBYTE' also leave out the `_WP_` part.
Alternatively, just write `#define GPIO_CONFIG_ADDRESS 0x104F` and use `(GPIO_CONFIG_ADDRESS>>8)` to get the high byte and the cast will auto give you the low byte.
Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/46089
to look at the new patch set (#5).
Change subject: realtek_mst_i2c_spi.c: Update write protection disable function ......................................................................
realtek_mst_i2c_spi.c: Update write protection disable function
The write protection trigger has been updated by the vendor so update here as well. Basically the idea is to push the GPIO pin for write protectoin to low or high. The process would be 1. write to GPIO configuration register to enable write protection pin value update. 2. write to GPIO data register to toggle the actual protection pin value.
BUG=b:152558985,b:148745673 BRANCH=none TEST=builds && verified the write protection get disabled.
Change-Id: I1aed0086f917e31bebb51857ad5cce138158fe82 Signed-off-by: Shiyu Sun sshiyu@chromium.org --- M realtek_mst_i2c_spi.c 1 file changed, 24 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/46089/5
Shiyu Sun has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update write protection disable function ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/flashrom/+/46089/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/46089/4//COMMIT_MSG@9 PS4, Line 9: The write protection trigger has been updated by the vendor so update : here as well.
Please add a little more detail for external reviewers so that everyone understands the code well an […]
Done
https://review.coreboot.org/c/flashrom/+/46089/4/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/4/realtek_mst_i2c_spi.c@49 PS4, Line 49: HIGH
perhaps, 'HIGHBYTE' also leave out the `_WP_` part. […]
I updated to use `#define GPIO_CONFIG_ADDRESS 0x104F`. For low byte case, I know it can auto cast, but still write GPIO_CONFIG_ADDRESS & 0xFF to make it clear. Let's see if that looks good.
Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/46089
to look at the new patch set (#6).
Change subject: realtek_mst_i2c_spi.c: Update write protection toggle function ......................................................................
realtek_mst_i2c_spi.c: Update write protection toggle function
The write protection trigger has been updated by the vendor so update here as well. Basically the idea is to push the GPIO pin for write protectoin to low or high. The process would be 1. write to GPIO configuration register to enable write protection pin value update. 2. write to GPIO data register to toggle the actual protection pin value.
BUG=b:152558985,b:148745673 BRANCH=none TEST=builds && verified the write protection get disabled.
Change-Id: I1aed0086f917e31bebb51857ad5cce138158fe82 Signed-off-by: Shiyu Sun sshiyu@chromium.org --- M realtek_mst_i2c_spi.c 1 file changed, 24 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/46089/6
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update write protection toggle function ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/flashrom/+/46089/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/46089/6//COMMIT_MSG@11 PS6, Line 11: protectoin protection
https://review.coreboot.org/c/flashrom/+/46089/6//COMMIT_MSG@11 PS6, Line 11: The process would be Format this as a list?
The process would be: 1. write to GPIO ... 2. write to GPIO ...
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@151 PS6, Line 151: realtek_mst_i2c_spi_toggle_gpio_88_strap realtek_mst_i2c_spi_toggle_write_protect_gpio
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@157 PS6, Line 157: ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, GPIO_CONFIG_ADDRESS >> 8); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, GPIO_CONFIG_ADDRESS & 0xFF); Maybe turn these groups of three register writes into an auxiliary function?
static int realtek_mst_i2c_spi_select_indexed_register(uint16_t address) { int ret = 0;
ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, address >> 8); ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, address & 0xFF);
return ret; }
Feel free to rename it for accuracy. It's not specific to GPIO, since `realtek_mst_i2c_spi_enter_isp_mode` also does the same thing with another register. Feel free to do it in a follow-up.
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@182 PS6, Line 182: (toggle ? 0x01 : 0x00) This can be simplified as follows:
!!toggle
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update write protection toggle function ......................................................................
Patch Set 6: Code-Review+1
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update write protection toggle function ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@151 PS6, Line 151: realtek_mst_i2c_spi_toggle_gpio_88_strap
realtek_mst_i2c_spi_toggle_write_protect_gpio
I suggested to Shiyu to use this name instead Angel as gpio88 need not be connected to the wp pin of the external spi flash, this just happens to be the reference design.
Since the purpose of the function is general and not specific to toggling wp but rather a general gpio that happens to be routed to the wp pin I think we should keep the name as it is here and instead have a commit what its doing at the call site of the function. For example, the gpio could be routed to a led to indicate programming is occurring and is thus up to the designer. As a result we try to delineate those competing principles out here between function purpose and call site intention.
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@157 PS6, Line 157: ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, GPIO_CONFIG_ADDRESS >> 8); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, GPIO_CONFIG_ADDRESS & 0xFF);
Maybe turn these groups of three register writes into an auxiliary function? […]
That's a good idea and thanks for identifying the pattern Angel.
Shiyu, I think Angel is right here and perhaps if you could prepare a follow up commit that refactors to collapse this pattern out of common places.
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@182 PS6, Line 182: (toggle ? 0x01 : 0x00)
This can be simplified as follows: […]
agreed, and toggle could really be typed as a bool as well imho.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update write protection toggle function ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@157 PS6, Line 157: ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, GPIO_CONFIG_ADDRESS >> 8); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, GPIO_CONFIG_ADDRESS & 0xFF);
That's a good idea and thanks for identifying the pattern Angel. […]
I forgot to add, the func is almost correct but would be:
static int realtek_mst_i2c_spi_select_indexed_register(uint16_t address, uint8_t val) { int ret = 0;
ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, address >> 8); ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, address & 0xFF); ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, val);
return ret; }
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update write protection toggle function ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@151 PS6, Line 151: realtek_mst_i2c_spi_toggle_gpio_88_strap
I suggested to Shiyu to use this name instead Angel as gpio88 need not be connected to the wp pin of […]
I see. Maybe add a comment for clarification?
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@157 PS6, Line 157: ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, GPIO_CONFIG_ADDRESS >> 8); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, GPIO_CONFIG_ADDRESS & 0xFF);
I forgot to add, the func is almost correct but would be: […]
Adding the last write inside the function makes it a write-function. For reads, the second operation on 0xF5 is a read. So there would be two functions:
static int realtek_mst_i2c_spi_read_indexed_register(uint16_t address, uint8_t *val) { int ret = 0;
ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, address >> 8); ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, address & 0xFF); ret |= realtek_mst_i2c_spi_read_register(fd, 0xF5, &val);
return ret; }
static int realtek_mst_i2c_spi_write_indexed_register(uint16_t address, uint8_t val) { int ret = 0;
ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, address >> 8); ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, address & 0xFF); ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, val);
return ret; }
I'm fine with either having a single `select indexed register` function (my suggestion) or having separate `read indexed register` and `write indexed register` functions (Edward's suggestion).
Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/46089
to look at the new patch set (#7).
Change subject: realtek_mst_i2c_spi.c: Update write protection toggle function ......................................................................
realtek_mst_i2c_spi.c: Update write protection toggle function
The write protection trigger has been updated by the vendor so update here as well. Basically the idea is to push the GPIO pin for write protection to low or high.
The process would be: 1. write to GPIO configuration register to enable write protection pin value update. 2. write to GPIO data register to toggle the actual protection pin value.
BUG=b:152558985,b:148745673 BRANCH=none TEST=builds && verified the write protection get disabled.
Change-Id: I1aed0086f917e31bebb51857ad5cce138158fe82 Signed-off-by: Shiyu Sun sshiyu@chromium.org --- M realtek_mst_i2c_spi.c 1 file changed, 39 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/46089/7
Shiyu Sun has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update write protection toggle function ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/flashrom/+/46089/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/46089/6//COMMIT_MSG@11 PS6, Line 11: The process would be
Format this as a list? […]
Done
https://review.coreboot.org/c/flashrom/+/46089/6//COMMIT_MSG@11 PS6, Line 11: protectoin
protection
Done
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@157 PS6, Line 157: ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, GPIO_CONFIG_ADDRESS >> 8); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, GPIO_CONFIG_ADDRESS & 0xFF);
Adding the last write inside the function makes it a write-function. […]
Done
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@182 PS6, Line 182: (toggle ? 0x01 : 0x00)
agreed, and toggle could really be typed as a bool as well imho.
Done
Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/46089
to look at the new patch set (#8).
Change subject: realtek_mst_i2c_spi.c: Update write protection toggle function ......................................................................
realtek_mst_i2c_spi.c: Update write protection toggle function
The write protection trigger has been updated by the vendor so update here as well. Basically the idea is to push the GPIO pin for write protection to low or high.
The process would be: 1. write to GPIO configuration register to enable write protection pin value update. 2. write to GPIO data register to toggle the actual protection pin value.
BUG=b:152558985,b:148745673 BRANCH=none TEST=builds && verified the write protection get disabled.
Change-Id: I1aed0086f917e31bebb51857ad5cce138158fe82 Signed-off-by: Shiyu Sun sshiyu@chromium.org --- M realtek_mst_i2c_spi.c 1 file changed, 39 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/46089/8
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update write protection toggle function ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/flashrom/+/46089/8/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/8/realtek_mst_i2c_spi.c@170 PS8, Line 170: val &val
Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/46089
to look at the new patch set (#9).
Change subject: realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function ......................................................................
realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function
Here we provide a helper function to allow indexing MCU configuration registers. The 0x9F port allows access to these MCU configuration registers followed by the high and then low bytes of the register address we wish to index written into 0xF5 or 0xF4 respectively, a read or write can be made via 0xF5.
For the configuration of GPIO pins on the chip, there are two relevant register address, 0x104F for pin direction (sink input or push-pull in-out) configuratoin and 0xFE3F for pin data values (1 to push-pull and 0 to sink). The reference design uses GPIO 88 to strap the write protection pin and so we provide a function that allows the call site to toggle this state and therefore de-assert hardware write protection of the external spi flash.
BUG=b:152558985,b:148745673 BRANCH=none TEST=builds && verified the write protection get disabled.
Change-Id: I1aed0086f917e31bebb51857ad5cce138158fe82 Signed-off-by: Shiyu Sun sshiyu@chromium.org --- M realtek_mst_i2c_spi.c 1 file changed, 46 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/46089/9
Shiyu Sun has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/flashrom/+/46089/8/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/8/realtek_mst_i2c_spi.c@170 PS8, Line 170: val
&val
It's already take in as a pointer, we don't need the & here?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function ......................................................................
Patch Set 9: Code-Review+2
Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/46089
to look at the new patch set (#10).
Change subject: realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function ......................................................................
realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function
Here we provide a helper function to allow indexing MCU configuration registers. The 0x9F port allows access to these MCU configuration registers followed by the high and then low bytes of the register address we wish to index written into 0xF5 or 0xF4 respectively, a read or write can then be made via 0xF5.
For the configuration of GPIO pins on the chip, there are two relevant register address, 0x104F for pin direction (sink input or push-pull in-out) configuration and 0xFE3F for pin data values (1 to push-pull and 0 to sink). The reference design uses GPIO 88 to strap the write protection pin and so we provide a function that allows the call site to toggle this state and therefore de-assert hardware write protection of the external spi flash.
BUG=b:152558985,b:148745673 BRANCH=none TEST=builds && verified the write protection get disabled.
Change-Id: I1aed0086f917e31bebb51857ad5cce138158fe82 Signed-off-by: Shiyu Sun sshiyu@chromium.org --- M realtek_mst_i2c_spi.c 1 file changed, 46 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/46089/10
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function ......................................................................
Patch Set 10: Code-Review+2
(1 comment)
https://review.coreboot.org/c/flashrom/+/46089/8/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/8/realtek_mst_i2c_spi.c@170 PS8, Line 170: val
It's already take in as a pointer, we don't need the & here?
Yes Shiyu, there's no need to add the & here.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@165 PS1, Line 165: 0xF0
I think from the Realtek's update it's required that last byte be 0x01 to enable the setting on pin […]
Mask went back to 0xFE
Shiyu Sun has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@165 PS1, Line 165: 0xF0
Mask went back to 0xFE
Sorry what does it means? I believe it's intended to put 0xF0 here
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@165 PS1, Line 165: 0xF0
Sorry what does it means? I believe it's intended to put 0xF0 here
Uh, sorry. The original value is 0xF8, this patchset has 0xF0, and the latest patchset has 0xFE. Re-opening until the correct value is confirmed.
Shiyu Sun has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@165 PS1, Line 165: 0xF0
Uh, sorry. The original value is 0xF8, this patchset has 0xF0, and the latest patchset has 0xFE. […]
Hi Angel, I think the switch from 0xF8 to 0xF0 is expected here, the vendor specifically we will need the last byte to be 0x01 in order to enable pin configuration. I have also checked all the patchset and looks like we never use 0xFE here, the only place we do an and with 0xFE is when we write pin value(1 or 0) to register 0xFE3F. Those values are all taken from vendor's suggestion.
So basically when write to 0x104F, we do write (val & 0xF0) | 0x01 and when write to 0xFE3F, we will write (val & 0xFE) | toggle.
Does it make sense?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@165 PS1, Line 165: 0xF0
Hi Angel, I think the switch from 0xF8 to 0xF0 is expected here, the vendor specifically we will nee […]
Kind of (I have no documentation). If last byte needs to be 0x01, then there should not be any mask here. Or did you mean last nibble? (4 bits)
Shiyu Sun has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@165 PS1, Line 165: 0xF0
Kind of (I have no documentation). […]
Ah yes, sorry for the confusion, I meant to say the last 4 bits actually.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@165 PS1, Line 165: 0xF0
Ah yes, sorry for the confusion, I meant to say the last 4 bits actually.
OK, then we're good
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function ......................................................................
realtek_mst_i2c_spi.c: Update GPIO pin 88 toggle function
Here we provide a helper function to allow indexing MCU configuration registers. The 0x9F port allows access to these MCU configuration registers followed by the high and then low bytes of the register address we wish to index written into 0xF5 or 0xF4 respectively, a read or write can then be made via 0xF5.
For the configuration of GPIO pins on the chip, there are two relevant register address, 0x104F for pin direction (sink input or push-pull in-out) configuration and 0xFE3F for pin data values (1 to push-pull and 0 to sink). The reference design uses GPIO 88 to strap the write protection pin and so we provide a function that allows the call site to toggle this state and therefore de-assert hardware write protection of the external spi flash.
BUG=b:152558985,b:148745673 BRANCH=none TEST=builds && verified the write protection get disabled.
Change-Id: I1aed0086f917e31bebb51857ad5cce138158fe82 Signed-off-by: Shiyu Sun sshiyu@chromium.org Reviewed-on: https://review.coreboot.org/c/flashrom/+/46089 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Edward O'Callaghan quasisec@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M realtek_mst_i2c_spi.c 1 file changed, 46 insertions(+), 17 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved
diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index d1f7227..63ff510 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -46,6 +46,8 @@ #define OPCODE_READ 3 #define OPCODE_WRITE 2
+#define GPIO_CONFIG_ADDRESS 0x104F +#define GPIO_VALUE_ADDRESS 0xFE3F
struct realtek_mst_i2c_spi_data { int fd; @@ -146,27 +148,54 @@ return ret; }
-static int realtek_mst_i2c_spi_disable_protection(int fd) +static int realtek_mst_i2c_spi_select_indexed_register(int fd, uint16_t address) +{ + int ret = 0; + + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, address >> 8); + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, address & 0xFF); + + return ret; +} + +static int realtek_mst_i2c_spi_write_indexed_register(int fd, uint16_t address, uint8_t val) +{ + int ret = 0; + + ret |= realtek_mst_i2c_spi_select_indexed_register(fd, address); + ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, val); + + return ret; +} + +static int realtek_mst_i2c_spi_read_indexed_register(int fd, uint16_t address, uint8_t *val) +{ + int ret = 0; + + ret |= realtek_mst_i2c_spi_select_indexed_register(fd, address); + ret |= realtek_mst_i2c_spi_read_register(fd, 0xF5, val); + + return ret; +} + + +/* Toggle the GPIO pin 88, this could be routed to different controls like write + * protection or a led. */ +static int realtek_mst_i2c_spi_toggle_gpio_88_strap(int fd, bool toggle) { int ret = 0; uint8_t val = 0; - // 0xAB[2:0] = b001
- ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); - ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, 0x10); - ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0xAB); + /* Read register 0x104F into val. */ + ret |= realtek_mst_i2c_spi_read_indexed_register(fd, GPIO_CONFIG_ADDRESS, &val); + /* Write 0x104F[3:0] = b0001 to enable the toggle of pin value. */ + ret |= realtek_mst_i2c_spi_write_indexed_register(fd, GPIO_CONFIG_ADDRESS, (val & 0xF0) | 0x01);
- ret |= realtek_mst_i2c_spi_read_register(fd, 0xF5, &val); - - ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); - ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, 0x10); - ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0xAB); - - ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, (val & 0xF8) | 0x01); - - /* Set pin value to high, 0xFFD7[0] = 1. */ - ret |= realtek_mst_i2c_spi_read_register(fd, 0xD7, &val); - ret |= realtek_mst_i2c_spi_write_register(fd, 0xD7, (val & 0xFE) | 0x01); + /* Read register 0xFE3F into val. */ + ret |= realtek_mst_i2c_spi_read_indexed_register(fd, GPIO_VALUE_ADDRESS, &val); + /* Write 0xFE3F[0] = b|toggle| to toggle pin value to low/high. */ + ret |= realtek_mst_i2c_spi_write_indexed_register(fd, GPIO_VALUE_ADDRESS, (val & 0xFE) | toggle);
return ret; } @@ -333,7 +362,7 @@ if (fd < 0) return SPI_GENERIC_ERROR;
- ret = realtek_mst_i2c_spi_disable_protection(fd); + ret = realtek_mst_i2c_spi_toggle_gpio_88_strap(fd, true); if (ret) return ret;
-
Angel Pons (Code Review) -
Edward O'Callaghan (Code Review)
-
Shiyu Sun (Code Review)