Change in flashrom[master]: realtek_mst_i2c_spi.c: Add ISP mode check

[Shiyu Sun (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Add ISP mode check
......................................................................

realtek_mst_i2c_spi.c: Add ISP mode check

Check ISP mode before doing reset and add waiting
after the enter ISP mode command.

BUG=None
TEST=build and run mst commands

Signed-off-by: Shiyu Sun 
Change-Id: Ib1ab8370eb6335a77bb293fc98a8ab7be465db4f
---
M realtek_mst_i2c_spi.c
1 file changed, 27 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/48662/1

diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c
index 538b07a..0126157 100644
--- a/realtek_mst_i2c_spi.c
+++ b/realtek_mst_i2c_spi.c
@@ -32,7 +32,7 @@
 #define MAX_SPI_WAIT_RETRIES	1000

#define MCU_MODE 0x6F
-#define 	ENTER_ISP_MODE 0x80
+#define 	MCU_ISP_MODE_MASK 0x80
 #define 	START_WRITE_XFER 0xA0
 #define 	WRITE_XFER_STATUS_MASK  0x20

@@ -117,10 +117,30 @@
    return (val & mask) != target ? SPI_GENERIC_ERROR : ret;
 }

+static int realtek_mst_i2c_spi_in_isp_mode(int fd)
+{
+	uint8_t mcu_mode_val;
+	int ret = realtek_mst_i2c_spi_read_register(fd, MCU_MODE, &mcu_mode_val);
+	if (ret || (mcu_mode_val & MCU_ISP_MODE_MASK) == 0)
+		return 0;
+	return 1;
+}
+
 static int realtek_mst_i2c_spi_enter_isp_mode(int fd)
 {
-	int ret = realtek_mst_i2c_spi_write_register(fd, MCU_MODE, ENTER_ISP_MODE);
+	int ret = realtek_mst_i2c_spi_write_register(fd, MCU_MODE, MCU_ISP_MODE_MASK);

+	/* wait for isp enter */
+	int retry = 10;
+	while (retry-- > 0) {
+		if (realtek_mst_i2c_spi_in_isp_mode(fd) == 1)
+			break;
+		struct timespec wait_200ms = { 0, (unsigned)2e8 };
+		nanosleep(&wait_200ms, NULL);
+	}
+
+	if (retry < 0)
+		return SPI_GENERIC_ERROR;
    // set internal osc divider register to default to speed up MCU
    // 0x06A0 = 0x74
    ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F);
@@ -140,6 +160,11 @@

static int realtek_mst_i2c_spi_reset_mpu(int fd)
 {
+	if (realtek_mst_i2c_spi_in_isp_mode(fd) == 0) {
+		msg_perr("%s: MST not in ISP mode, reset will not happen.\n", __func__);
+		return SPI_GENERIC_ERROR;
+	}
+
    int ret = 0;
    // 0xFFEE[1] = 1;
    uint8_t val = 0;

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Add ISP mode check
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/flashrom/+/48662/1/realtek_mst_i2c_spi.c@133 
PS1, Line 133: 	/* wait for isp enter */
             : 	int retry = 10;
             : 	while (retry-- > 0) {
             : 		if (realtek_mst_i2c_spi_in_isp_mode(fd) == 1)
             : 			break;
             : 		struct timespec wait_200ms = { 0, (unsigned)2e8 };
             : 		nanosleep(&wait_200ms, NULL);
             : 	}
             : 
             : 	if (retry < 0)
             : 		return SPI_GENERIC_ERROR;
I think this is re-implementing,
 `realtek_mst_i2c_spi_wait_command_done(fd, MCU_MODE, MCU_ISP_MODE_MASK, 0, 1);`

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Add ISP mode check
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/flashrom/+/48662/1/realtek_mst_i2c_spi.c@163 
PS1, Line 163: 	if (realtek_mst_i2c_spi_in_isp_mode(fd) == 0) {
             : 		msg_perr("%s: MST not in ISP mode, reset will not happen.\n", __func__);
             : 		return SPI_GENERIC_ERROR;
             : 	}
just inline this,

```
    uint8_t mcu_mode_val;
    int ret = realtek_mst_i2c_spi_read_register(fd, MCU_MODE, &mcu_mode_val);
    if (ret || (mcu_mode_val & MCU_ISP_MODE_MASK) == 0) {
    	msg_perr("%s: MST not in ISP mode, cannot perform MCU reset.\n", __func__);
    	return SPI_GENERIC_ERROR;
    }
```

just as an aside for future reference; A return value of >0 usually indicates some success with value, =0 is success without value and <0 indicates failure. It's a bit unconventional to use =0 to mean error as is the case with `realtek_mst_i2c_spi_in_isp_mode()`.

[Shiyu Sun (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: realtek_mst_i2c_spi.c: Add ISP mode check
......................................................................

realtek_mst_i2c_spi.c: Add ISP mode check

Check ISP mode before doing reset and add waiting
after the enter ISP mode command.

BUG=None
TEST=build and run mst commands

Signed-off-by: Shiyu Sun 
Change-Id: Ib1ab8370eb6335a77bb293fc98a8ab7be465db4f
---
M realtek_mst_i2c_spi.c
1 file changed, 21 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/48662/2

[Shiyu Sun (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: realtek_mst_i2c_spi.c: Add ISP mode check
......................................................................

realtek_mst_i2c_spi.c: Add ISP mode check

Check ISP mode before doing reset and add waiting
after the enter ISP mode command.

BUG=None
TEST=build and run mst commands

Signed-off-by: Shiyu Sun 
Change-Id: Ib1ab8370eb6335a77bb293fc98a8ab7be465db4f
---
M realtek_mst_i2c_spi.c
1 file changed, 14 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/48662/3

[Shiyu Sun (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Add ISP mode check
......................................................................

Patch Set 3:

(2 comments)

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

https://review.coreboot.org/c/flashrom/+/48662/1/realtek_mst_i2c_spi.c@133 
PS1, Line 133: 	/* wait for isp enter */
             : 	int retry = 10;
             : 	while (retry-- > 0) {
             : 		if (realtek_mst_i2c_spi_in_isp_mode(fd) == 1)
             : 			break;
             : 		struct timespec wait_200ms = { 0, (unsigned)2e8 };
             : 		nanosleep(&wait_200ms, NULL);
             : 	}
             : 
             : 	if (retry < 0)
             : 		return SPI_GENERIC_ERROR;
> I think this is re-implementing, […]

Done

https://review.coreboot.org/c/flashrom/+/48662/1/realtek_mst_i2c_spi.c@163 
PS1, Line 163: 	if (realtek_mst_i2c_spi_in_isp_mode(fd) == 0) {
             : 		msg_perr("%s: MST not in ISP mode, reset will not happen.\n", __func__);
             : 		return SPI_GENERIC_ERROR;
             : 	}
> just inline this, […]

I was thinking use the return value as True/False to present whether the device is in the ISP mode. But probably that should be a parameter instead of return value.

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Add ISP mode check
......................................................................

Patch Set 3: Code-Review+2

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Add ISP mode check
......................................................................

Patch Set 3: -Code-Review

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Add ISP mode check
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/flashrom/+/48662/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/48662/3//COMMIT_MSG@13 
PS3, Line 13: TEST=build and run mst commands
Test your changes each time you revise the patch otherwise this line isn't valid.

https://review.coreboot.org/c/flashrom/+/48662/3/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/48662/3/realtek_mst_i2c_spi.c@124 
PS3, Line 124: MCU_ISP_MODE_MASK, MCU_ISP_MODE_MASK
this isn't right, you have mask used for the target.

[Shiyu Sun (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Add ISP mode check
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/flashrom/+/48662/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/48662/3//COMMIT_MSG@13 
PS3, Line 13: TEST=build and run mst commands
> Test your changes each time you revise the patch otherwise this line isn't valid.

I believe I did try this for every change I have made. Build, copy to DUT and test with multiple commands.

https://review.coreboot.org/c/flashrom/+/48662/3/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/48662/3/realtek_mst_i2c_spi.c@124 
PS3, Line 124: MCU_ISP_MODE_MASK, MCU_ISP_MODE_MASK
> this isn't right, you have mask used for the target.

I believe we are waiting for the bit 7 to reach 1 so we do expect to have '(val & MCU_ISP_MODE_MASK) = MCU_ISP_MODE_MASK' as the enter ISP success. Does it means I have to use 0x80 directly here?

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Add ISP mode check
......................................................................

Patch Set 3: Code-Review+2

(2 comments)

https://review.coreboot.org/c/flashrom/+/48662/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/48662/3//COMMIT_MSG@13 
PS3, Line 13: TEST=build and run mst commands
> I believe I did try this for every change I have made. […]

If it works then that's good enough for me, Ack.

https://review.coreboot.org/c/flashrom/+/48662/3/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/48662/3/realtek_mst_i2c_spi.c@124 
PS3, Line 124: MCU_ISP_MODE_MASK, MCU_ISP_MODE_MASK
> I believe we are waiting for the bit 7 to reach 1 so we do expect to have '(val & MCU_ISP_MODE_MASK) […]

Ack.

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/48662 )

Change subject: realtek_mst_i2c_spi.c: Add ISP mode check
......................................................................

realtek_mst_i2c_spi.c: Add ISP mode check

Check ISP mode before doing reset and add waiting
after the enter ISP mode command.

BUG=None
TEST=build and run mst commands

Signed-off-by: Shiyu Sun 
Change-Id: Ib1ab8370eb6335a77bb293fc98a8ab7be465db4f
Reviewed-on: https://review.coreboot.org/c/flashrom/+/48662
Tested-by: build bot (Jenkins) 
Reviewed-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 14 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Edward O'Callaghan: Looks good to me, approved

diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c
index 538b07a..80caf86 100644
--- a/realtek_mst_i2c_spi.c
+++ b/realtek_mst_i2c_spi.c
@@ -32,7 +32,7 @@
 #define MAX_SPI_WAIT_RETRIES	1000

#define MCU_MODE 0x6F
-#define 	ENTER_ISP_MODE 0x80
+#define 	MCU_ISP_MODE_MASK 0x80
 #define 	START_WRITE_XFER 0xA0
 #define 	WRITE_XFER_STATUS_MASK  0x20

@@ -119,7 +119,12 @@

static int realtek_mst_i2c_spi_enter_isp_mode(int fd)
 {
-	int ret = realtek_mst_i2c_spi_write_register(fd, MCU_MODE, ENTER_ISP_MODE);
+	int ret = realtek_mst_i2c_spi_write_register(fd, MCU_MODE, MCU_ISP_MODE_MASK);
+	/* wait for ISP mode enter success */
+	ret |= realtek_mst_i2c_spi_wait_command_done(fd, MCU_MODE, MCU_ISP_MODE_MASK, MCU_ISP_MODE_MASK, 1);
+
+	if (ret)
+		return ret;

// set internal osc divider register to default to speed up MCU
    // 0x06A0 = 0x74
@@ -140,7 +145,13 @@

static int realtek_mst_i2c_spi_reset_mpu(int fd)
 {
-	int ret = 0;
+	uint8_t mcu_mode_val;
+	int ret = realtek_mst_i2c_spi_read_register(fd, MCU_MODE, &mcu_mode_val);
+	if (ret || (mcu_mode_val & MCU_ISP_MODE_MASK) == 0) {
+		msg_perr("%s: MST not in ISP mode, cannot perform MCU reset.\n", __func__);
+		return SPI_GENERIC_ERROR;
+	}
+
    // 0xFFEE[1] = 1;
    uint8_t val = 0;
    ret |= realtek_mst_i2c_spi_read_register(fd, 0xEE, &val);