Change in flashrom[master]: realtek_mst_i2c_spi.c: Introduce ISP enter param

[Shiyu Sun (Code Review)]

Shiyu Sun has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/46623 )

Change subject: realtek_mst_i2c_spi.c: Introduce ISP enter param
......................................................................

realtek_mst_i2c_spi.c: Introduce ISP enter param

This is needed to avoid attempt entering ISP mode multiple
times. The ISP mode can only exit after a reset, so once the
reset MCU parameter is set to 0, the device will not able to
exit from ISP mode and hence shouldn't enter ISP again on
the next operation.
Without exit ISP mode, the device data, like firmware version,
will not show the correct value, this param will also help
to identify this situation.

Example on update a firmware with A/B udpate:
flashrom -p realtek_mst_i2c_spi:bus=x,reset-mcu=0,enter-isp=1 write fw
flashrom -p realtek_mst_i2c_spi:bus=x,reset-mcu=0,enter-isp=0 write flag
-> then either reset computer to allow update to take effect, or:
flashrom -p realtek_mst_i2c_spi:bus=x,reset-mcu=1,enter-isp=0 command
to trigger the update.

BUG=b:152558985,b:148745673
BRANCH=none
TEST=build

Signed-off-by: Shiyu Sun 
Change-Id: I58931ac8b42ab55829f102d243aea6fcfd632e3e
---
M realtek_mst_i2c_spi.c
1 file changed, 24 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/46623/1

diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c
index 43cf25b..538b07a 100644
--- a/realtek_mst_i2c_spi.c
+++ b/realtek_mst_i2c_spi.c
@@ -432,10 +432,10 @@
    return ret;
 }

-static int get_params(int *i2c_bus, int *reset)
+static int get_params(int *i2c_bus, int *reset, int *enter_isp)
 {
-	char *bus_str = NULL, *reset_str = NULL;
-        int ret = SPI_GENERIC_ERROR;
+	char *bus_str = NULL, *reset_str = NULL, *isp_str = NULL;
+	int ret = SPI_GENERIC_ERROR;

bus_str = extract_programmer_param("bus");
    if (bus_str) {
@@ -478,6 +478,20 @@
    	*reset = 0; /* Default behaviour is no MCU reset on tear-down. */
    free(reset_str);

+	isp_str = extract_programmer_param("enter-isp");
+	if (isp_str) {
+		if (isp_str[0] == '1')
+			*enter_isp = 1;
+		else if (isp_str[0] == '0')
+			*enter_isp = 0;
+		else {
+			msg_perr("%s: Incorrect param format, enter-isp=1 or 0.\n", __func__);
+			ret = SPI_GENERIC_ERROR;
+		}
+	} else
+		*enter_isp = 1; /* Default behaviour is enter ISP on setup. */
+	free(isp_str);
+
 _get_params_failed:
    if (bus_str)
    	free(bus_str);
@@ -488,18 +502,20 @@
 int realtek_mst_i2c_spi_init(void)
 {
    int ret = 0;
-	int i2c_bus = 0, reset = 0;
+	int i2c_bus = 0, reset = 0, enter_isp = 0;

-	if (get_params(&i2c_bus, &reset))
+	if (get_params(&i2c_bus, &reset, &enter_isp))
    	return SPI_GENERIC_ERROR;

int fd = i2c_open(i2c_bus, REGISTER_ADDRESS, 0);
    if (fd < 0)
    	return fd;

-	ret |= realtek_mst_i2c_spi_enter_isp_mode(fd);
-	if (ret)
-		return ret;
+	if (enter_isp) {
+		ret |= realtek_mst_i2c_spi_enter_isp_mode(fd);
+		if (ret)
+			return ret;
+	}

struct realtek_mst_i2c_spi_data *data = calloc(1, sizeof(struct realtek_mst_i2c_spi_data));
    if (!data) {

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46623 )

Change subject: realtek_mst_i2c_spi.c: Introduce ISP enter param
......................................................................

Patch Set 1: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46623 )

Change subject: realtek_mst_i2c_spi.c: Introduce ISP enter param
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/flashrom/+/46623/1/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/46623/1/realtek_mst_i2c_spi.c@514 
PS1, Line 514: 	if (enter_isp) {
I wonder, is there any way to know if we're in ISP mode already?

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46623 )

Change subject: realtek_mst_i2c_spi.c: Introduce ISP enter param
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/flashrom/+/46623/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/46623/1//COMMIT_MSG@18 
PS1, Line 18: Example on update a firmware with A/B udpate:
            : fla
format as:

```
 * $ flashrom..
```
and move onto the TEST= line

https://review.coreboot.org/c/flashrom/+/46623/1//COMMIT_MSG@22 
PS1, Line 22: command
            : to trigger the update.
this is a little unclear for a passing observer. Perhaps just be concrete with `--flash-size`

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46623 )

Change subject: realtek_mst_i2c_spi.c: Introduce ISP enter param
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/flashrom/+/46623/1/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/46623/1/realtek_mst_i2c_spi.c@514 
PS1, Line 514: 	if (enter_isp) {
> I wonder, is there any way to know if we're in ISP mode already?

Seems a little out of scope for this change but Shiyu would need to ask RTK that. I suspect the enter_isp_mode() function would need to be modified to return another exit code >0 depending on the status.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46623 )

Change subject: realtek_mst_i2c_spi.c: Introduce ISP enter param
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/flashrom/+/46623/1/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/46623/1/realtek_mst_i2c_spi.c@514 
PS1, Line 514: 	if (enter_isp) {
> Seems a little out of scope for this change but Shiyu would need to ask RTK that. […]

It would be nice to automatically detect whether entering ISP mode is necessary. That way, there's one less option to potentially be wrong on! 😄

But I agree, this can be done in a separate patch.

[Shiyu Sun (Code Review)]

Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/46623

to look at the new patch set (#2).

Change subject: realtek_mst_i2c_spi.c: Introduce ISP enter param
......................................................................

realtek_mst_i2c_spi.c: Introduce ISP enter param

This is needed to avoid attempt entering ISP mode multiple
times. The ISP mode can only exit after a reset, so once the
reset MCU parameter is set to 0, the device will not able to
exit from ISP mode and hence shouldn't enter ISP again on
the next operation.
Without exit ISP mode, the device data, like firmware version,
will not show the correct value, this param will also help
to identify this situation.

BUG=b:152558985,b:148745673
BRANCH=none
TEST=build and run:
$ flashrom -p realtek_mst_i2c_spi:bus=x,reset-mcu=0,enter-isp=1 \
    -l layout -i PARTITION1:fw -w
$ flashrom -p realtek_mst_i2c_spi:bus=x,reset-mcu=0,enter-isp=0 \
    -l layout -i FLAG1:flag -w
then either reset computer to allow update to take effect, or:
$ flashrom -p realtek_mst_i2c_spi:bus=x,reset-mcu=1,enter-isp=0 \
    --flash-size
to trigger the update.

Signed-off-by: Shiyu Sun 
Change-Id: I58931ac8b42ab55829f102d243aea6fcfd632e3e
---
M realtek_mst_i2c_spi.c
1 file changed, 24 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/46623/2

[Shiyu Sun (Code Review)]

Shiyu Sun has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46623 )

Change subject: realtek_mst_i2c_spi.c: Introduce ISP enter param
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/flashrom/+/46623/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/46623/1//COMMIT_MSG@18 
PS1, Line 18: Example on update a firmware with A/B udpate:
            : fla
> format as: […]

Done

https://review.coreboot.org/c/flashrom/+/46623/1//COMMIT_MSG@22 
PS1, Line 22: command
            : to trigger the update.
> this is a little unclear for a passing observer. […]

Done

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46623 )

Change subject: realtek_mst_i2c_spi.c: Introduce ISP enter param
......................................................................

Patch Set 2: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46623 )

Change subject: realtek_mst_i2c_spi.c: Introduce ISP enter param
......................................................................

Patch Set 2: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46623 )

Change subject: realtek_mst_i2c_spi.c: Introduce ISP enter param
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/46623/1/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/46623/1/realtek_mst_i2c_spi.c@514 
PS1, Line 514: 	if (enter_isp) {
> It would be nice to automatically detect whether entering ISP mode is necessary. […]

Ack

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/flashrom/+/46623 )

Change subject: realtek_mst_i2c_spi.c: Introduce ISP enter param
......................................................................

realtek_mst_i2c_spi.c: Introduce ISP enter param

This is needed to avoid attempt entering ISP mode multiple
times. The ISP mode can only exit after a reset, so once the
reset MCU parameter is set to 0, the device will not able to
exit from ISP mode and hence shouldn't enter ISP again on
the next operation.
Without exit ISP mode, the device data, like firmware version,
will not show the correct value, this param will also help
to identify this situation.

BUG=b:152558985,b:148745673
BRANCH=none
TEST=build and run:
$ flashrom -p realtek_mst_i2c_spi:bus=x,reset-mcu=0,enter-isp=1 \
    -l layout -i PARTITION1:fw -w
$ flashrom -p realtek_mst_i2c_spi:bus=x,reset-mcu=0,enter-isp=0 \
    -l layout -i FLAG1:flag -w
then either reset computer to allow update to take effect, or:
$ flashrom -p realtek_mst_i2c_spi:bus=x,reset-mcu=1,enter-isp=0 \
    --flash-size
to trigger the update.

Signed-off-by: Shiyu Sun 
Change-Id: I58931ac8b42ab55829f102d243aea6fcfd632e3e
Reviewed-on: https://review.coreboot.org/c/flashrom/+/46623
Tested-by: build bot (Jenkins) 
Reviewed-by: Edward O'Callaghan 
Reviewed-by: Angel Pons 
---
M realtek_mst_i2c_spi.c
1 file changed, 24 insertions(+), 8 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 43cf25b..538b07a 100644
--- a/realtek_mst_i2c_spi.c
+++ b/realtek_mst_i2c_spi.c
@@ -432,10 +432,10 @@
    return ret;
 }

-static int get_params(int *i2c_bus, int *reset)
+static int get_params(int *i2c_bus, int *reset, int *enter_isp)
 {
-	char *bus_str = NULL, *reset_str = NULL;
-        int ret = SPI_GENERIC_ERROR;
+	char *bus_str = NULL, *reset_str = NULL, *isp_str = NULL;
+	int ret = SPI_GENERIC_ERROR;

bus_str = extract_programmer_param("bus");
    if (bus_str) {
@@ -478,6 +478,20 @@
    	*reset = 0; /* Default behaviour is no MCU reset on tear-down. */
    free(reset_str);

+	isp_str = extract_programmer_param("enter-isp");
+	if (isp_str) {
+		if (isp_str[0] == '1')
+			*enter_isp = 1;
+		else if (isp_str[0] == '0')
+			*enter_isp = 0;
+		else {
+			msg_perr("%s: Incorrect param format, enter-isp=1 or 0.\n", __func__);
+			ret = SPI_GENERIC_ERROR;
+		}
+	} else
+		*enter_isp = 1; /* Default behaviour is enter ISP on setup. */
+	free(isp_str);
+
 _get_params_failed:
    if (bus_str)
    	free(bus_str);
@@ -488,18 +502,20 @@
 int realtek_mst_i2c_spi_init(void)
 {
    int ret = 0;
-	int i2c_bus = 0, reset = 0;
+	int i2c_bus = 0, reset = 0, enter_isp = 0;

-	if (get_params(&i2c_bus, &reset))
+	if (get_params(&i2c_bus, &reset, &enter_isp))
    	return SPI_GENERIC_ERROR;

int fd = i2c_open(i2c_bus, REGISTER_ADDRESS, 0);
    if (fd < 0)
    	return fd;

-	ret |= realtek_mst_i2c_spi_enter_isp_mode(fd);
-	if (ret)
-		return ret;
+	if (enter_isp) {
+		ret |= realtek_mst_i2c_spi_enter_isp_mode(fd);
+		if (ret)
+			return ret;
+	}

struct realtek_mst_i2c_spi_data *data = calloc(1, sizeof(struct realtek_mst_i2c_spi_data));
    if (!data) {