Change in flashrom[master]: realtek_mst_i2c_spi.c: Introduce MCU reset param

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/45674 )

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

realtek_mst_i2c_spi.c: Introduce MCU reset param

Modify the spi master as to not automatically reset the
MCU unless explicitly stated by a param.

Change-Id: Ib70bf7399e7541f30b6905cdb950a6fb7b74ae18
Signed-off-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 15 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/45674/1

diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c
index 6158524..6ac57d5 100644
--- a/realtek_mst_i2c_spi.c
+++ b/realtek_mst_i2c_spi.c
@@ -49,6 +49,7 @@

struct realtek_mst_i2c_spi_data {
    int fd;
+	int reset;
 };

static int realtek_mst_i2c_spi_write_data(int fd, uint16_t addr, void *buf, uint16_t len)
@@ -392,14 +393,15 @@
         struct realtek_mst_i2c_spi_data *realtek_mst_data =
    	(struct realtek_mst_i2c_spi_data *)data;
    int fd = realtek_mst_data->fd;
-	ret |= realtek_mst_i2c_spi_reset_mpu(fd);
+	if (data->reset)
+		ret |= realtek_mst_i2c_spi_reset_mpu(fd);
    i2c_close(fd);
    free(data);

return ret;
 }

-static int get_params(int *i2c_bus)
+static int get_params(int *i2c_bus, int *reset)
 {
    char *bus_str = NULL;
         int ret = SPI_GENERIC_ERROR;
@@ -431,6 +433,12 @@
    } else {
    	msg_perr("%s: Bus number not specified.\n", __func__);
    }
+
+	reset_str = extract_programmer_param("reset-mcu");
+	if (reset_str && reset_str[0] == '1')
+		reset = 1;
+	free(reset_str);
+
 get_params_done:
    if (bus_str)
    	free(bus_str);
@@ -441,9 +449,9 @@
 int realtek_mst_i2c_spi_init(void)
 {
    int ret = 0;
-	int i2c_bus = 0;
+	int i2c_bus = 0, reset = 0;

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

int fd = i2c_open(i2c_bus, REGISTER_ADDRESS, 0);
@@ -451,7 +459,8 @@
    	return fd;

/* Ensure we are in a known state before entering ISP mode */
-	ret |= realtek_mst_i2c_spi_reset_mpu(fd);
+	if (reset)
+		ret |= realtek_mst_i2c_spi_reset_mpu(fd);
    if (ret)
    	return ret;

@@ -466,6 +475,7 @@
    }

data->fd = fd;
+	data->reset = reset;
    ret |= register_shutdown(realtek_mst_i2c_spi_shutdown, data);

spi_master_i2c_realtek_mst.data = data;

[Edward O'Callaghan (Code Review)]

Hello build bot (Jenkins), Shiyu Sun,

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

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

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

realtek_mst_i2c_spi.c: Introduce MCU reset param

Modify the spi master as to not automatically reset the
MCU unless explicitly stated by a param.

Change-Id: Ib70bf7399e7541f30b6905cdb950a6fb7b74ae18
Signed-off-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 15 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/45674/2

[Edward O'Callaghan (Code Review)]

Hello Sam McNally, build bot (Jenkins), Shiyu Sun,

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

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

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

realtek_mst_i2c_spi.c: Introduce MCU reset param

Modify the spi master as to not automatically reset the
MCU unless explicitly stated by a param.

Change-Id: Ib70bf7399e7541f30b6905cdb950a6fb7b74ae18
Signed-off-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 16 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/45674/3

[Angel Pons (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

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

https://review.coreboot.org/c/flashrom/+/45674/3//COMMIT_MSG@10 
PS3, Line 10: MCU unless explicitly stated by a param.
In which cases would one want to reset the MCU?

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/flashrom/+/45674/3//COMMIT_MSG@10 
PS3, Line 10: MCU unless explicitly stated by a param.
> In which cases would one want to reset the MCU?

I suppose this is not unlike 'noreset' in mstarddc_spi.c. Basically the reset causes the MCU to jmp to either the A or B fw image from the bootloader depending on some other flow.

We hope to update the comments in the source once we actually have that side of things pinned down as we do not directly control the firmware environment.

This sort of thing is a little tricky because Flashrom was not really designed to handle the runtime state of the thing it is flashing and so we do not really want to introduce that complexity here only just to be able to tune a very basic artifact of the runtime (i.e., reset after flashing).

[Angel Pons (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/flashrom/+/45674/3//COMMIT_MSG@10 
PS3, Line 10: MCU unless explicitly stated by a param.
> I suppose this is not unlike 'noreset' in mstarddc_spi.c. […]

Ah, I see.

[Edward O'Callaghan (Code Review)]

Hello Sam McNally, build bot (Jenkins), Shiyu Sun, Angel Pons,

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

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

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

realtek_mst_i2c_spi.c: Introduce MCU reset param

Modify the spi master as to not automatically reset the
MCU unless explicitly stated by a param.

Change-Id: Ib70bf7399e7541f30b6905cdb950a6fb7b74ae18
Signed-off-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 16 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/45674/4

[Sam McNally (Code Review)]

Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/45674 )

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/flashrom/+/45674/4/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/4/realtek_mst_i2c_spi.c@438 
PS4, Line 438: 	if (reset_str && reset_str[0] == '1')
Could we treat values outside 0 and 1 as an error?

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/flashrom/+/45674/4/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/4/realtek_mst_i2c_spi.c@438 
PS4, Line 438: 	if (reset_str && reset_str[0] == '1')
> Could we treat values outside 0 and 1 as an error?

yes, we should. Waiting on Shiyu for feedback on where he wants this to go.

[Shiyu Sun (Code Review)]

Shiyu Sun has uploaded a new patch set (#5) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/45674 )

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

realtek_mst_i2c_spi.c: Introduce MCU reset param

Modify the spi master as to not automatically reset the
MCU unless explicitly stated by a param.

Change-Id: Ib70bf7399e7541f30b6905cdb950a6fb7b74ae18
Signed-off-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 37 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/45674/5

[Shiyu Sun (Code Review)]

Shiyu Sun has uploaded a new patch set (#6) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/45674 )

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

realtek_mst_i2c_spi.c: Introduce MCU reset param

Modify the spi master as to not automatically reset the
MCU unless explicitly stated by a param.

Change-Id: Ib70bf7399e7541f30b6905cdb950a6fb7b74ae18
Signed-off-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 37 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/45674/6

[Shiyu Sun (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/flashrom/+/45674/4/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/4/realtek_mst_i2c_spi.c@438 
PS4, Line 438: 	if (reset_str && reset_str[0] == '1')
> yes, we should. Waiting on Shiyu for feedback on where he wants this to go.

Updated the code and verified that the reset logic works now.

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 6: Code-Review-2

(2 comments)

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c@a429 
PS6, Line 429: ret = 0;
you cannot remove this because now without the 'reset-mcu' param get_params() will fail which isn't valid.

Better to just put comments on the commit than push over it.

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c@a453 
PS6, Line 453: /* Ensure we are in a known state before entering ISP mode */
this comment still sort of applies on why there is a pre-amble reset before the spi master gets registered into the dispatch mechanism

[Shiyu Sun (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c@a429 
PS6, Line 429: ret = 0;
> you cannot remove this because now without the 'reset-mcu' param get_params() will fail which isn't  […]

Without 'reset-mcu' now on the following else statement we specifically set ret to 0. It's kind of the program success only if both parameter check are passed so we don't want to mark ret to 0 on the first 'bus' check. Does it make sense?
Otherwise we need to set ret = SPI_GENERIC_ERROR; before extract_programmer_param("reset-mcu").

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c@a453 
PS6, Line 453: /* Ensure we are in a known state before entering ISP mode */
> this comment still sort of applies on why there is a pre-amble reset before the spi master gets regi […]

Done

[Shiyu Sun (Code Review)]

Shiyu Sun has uploaded a new patch set (#7) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/45674 )

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

realtek_mst_i2c_spi.c: Introduce MCU reset param

Modify the spi master as to not automatically reset the
MCU unless explicitly stated by a param.

Change-Id: Ib70bf7399e7541f30b6905cdb950a6fb7b74ae18
Signed-off-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 37 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/45674/7

[Shiyu Sun (Code Review)]

Shiyu Sun has uploaded a new patch set (#8) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/45674 )

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

realtek_mst_i2c_spi.c: Introduce MCU reset param

Modify the spi master as to not automatically reset the
MCU unless explicitly stated by a param.

Change-Id: Ib70bf7399e7541f30b6905cdb950a6fb7b74ae18
Signed-off-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 38 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/45674/8

[Shiyu Sun (Code Review)]

Shiyu Sun has uploaded a new patch set (#9) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/45674 )

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

realtek_mst_i2c_spi.c: Introduce MCU reset param

Modify the spi master as to not automatically reset the
MCU unless explicitly stated by a param.

Change-Id: Ib70bf7399e7541f30b6905cdb950a6fb7b74ae18
Signed-off-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 38 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/45674/9

[Shiyu Sun (Code Review)]

Shiyu Sun has uploaded a new patch set (#10) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/45674 )

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

realtek_mst_i2c_spi.c: Introduce MCU reset param

Modify the spi master as to not automatically reset the
MCU unless explicitly stated by a param.

Change-Id: Ib70bf7399e7541f30b6905cdb950a6fb7b74ae18
Signed-off-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 36 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/45674/10

[Shiyu Sun (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c@a429 
PS6, Line 429: ret = 0;
> Without 'reset-mcu' now on the following else statement we specifically set ret to 0. […]

Updated

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c@a429 
PS6, Line 429: ret = 0;
> Updated
> Without 'reset-mcu' now on the following else statement we specifically set ret to 0. It's kind of the program success only if both parameter check are passed so we don't want to mark ret to 0 on the first 'bus' check. Does it make sense?

No. You are changing the behavior. The parameters are a union of state and so mutually exclusive. You are breaking one for the sake of another.

Also I think you missed this comment
> Better to just put comments on the commit than push over it.

Deciding the semantics and behavior via n patch uploaded isn't a good mechanism to reach consensus.

[Angel Pons (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 10:

(4 comments)

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c@a429 
PS6, Line 429: ret = 0;
> > Without 'reset-mcu' now on the following else statement we specifically set ret to 0. […]

The only thing to change around here is to move the `goto` statement to the other branch of the if-block.

https://review.coreboot.org/c/flashrom/+/45674/10/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/10/realtek_mst_i2c_spi.c@444 
PS10, Line 444: if
else if

https://review.coreboot.org/c/flashrom/+/45674/10/realtek_mst_i2c_spi.c@452 
PS10, Line 452: /* It's fine to not set this parameter. */
Please initialize `*reset` here.

https://review.coreboot.org/c/flashrom/+/45674/10/realtek_mst_i2c_spi.c@476 
PS10, Line 476: 	if (reset) {
I see that the MPU is reset here and during programmer shutdown. This means there should be 4 cases to handle:

- Never reset
- Reset before flashing
- Reset after flashing
- Reset both before and after flashing

The current approach only lets one choose between the first and the last option. Should we account for the two central options, too? This can use flexible syntax, too:

if reset_str contains "pre" then
    reset |= RESET_PRE;
 end if;
 if reset_str contains "post" then
    reset |= RESET_POST;
 end if;
 if reset_str contains "both" then
    reset |= RESET_PRE | RESET_POST;
 end if;

Something like this would allow one to request a reset before and after flashing using either "pre,post" or "both". It needs more refining, though: "preposterous" would work as well 😄

[Shiyu Sun (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c@a429 
PS6, Line 429: ret = 0;
> The only thing to change around here is to move the `goto` statement to the other branch of the if-b […]

Yes sorry about that I will stop make change directly here.

[Edward O'Callaghan (Code Review)]

Hello Sam McNally, build bot (Jenkins), Shiyu Sun, Angel Pons,

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

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

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

realtek_mst_i2c_spi.c: Introduce MCU reset param

Modify the spi master as to not automatically reset the
MCU on tear-down unless explicitly stated by a param.

V.2: Remove MCU reset on init as this was only introduced
  when MCU fw requirements for correct flashing were unknown
  however it turns out no MCU fw is required to flash and so
  no MCU reset should occur upon initialization.

Change-Id: Ib70bf7399e7541f30b6905cdb950a6fb7b74ae18
Signed-off-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 26 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/45674/11

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 10: -Code-Review

(3 comments)

https://review.coreboot.org/c/flashrom/+/45674/10/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/10/realtek_mst_i2c_spi.c@444 
PS10, Line 444: if
> else if

Done

https://review.coreboot.org/c/flashrom/+/45674/10/realtek_mst_i2c_spi.c@452 
PS10, Line 452: /* It's fine to not set this parameter. */
> Please initialize `*reset` here.

Done

https://review.coreboot.org/c/flashrom/+/45674/10/realtek_mst_i2c_spi.c@476 
PS10, Line 476: 	if (reset) {
> I see that the MPU is reset here and during programmer shutdown. […]

Hey Angel, thanks for your feedback!

Actually though we should remove the MCU reset from the init all together. This was only put here when we knew far less about the internal fw that runs on the MST. Turns out no reset is needed for correct programming and so no reset should ever be preformed pre-flash.

We only want the binary condition on never reset (by default) or reset after programming tear-down and so I removed it here in the follow up patch. I think there was some mis-communication initially with Shiyu on who is driving this part hence the series of patches that got pushed without a specific direction set in stone.

[Angel Pons (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 11: Code-Review+1

(1 comment)

https://review.coreboot.org/c/flashrom/+/45674/10/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/10/realtek_mst_i2c_spi.c@476 
PS10, Line 476: 	if (reset) {
> Hey Angel, thanks for your feedback! […]

Hi Edward, thanks to you!

I believe removing the pre-reset would be best done in a separate patch, with this one as a follow-up. This way, its commit message can explain why the pre-reset is not required.

[Edward O'Callaghan (Code Review)]

Hello Sam McNally, build bot (Jenkins), Shiyu Sun, Angel Pons,

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

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

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

realtek_mst_i2c_spi.c: Introduce MCU reset param

Modify the spi master as to not automatically reset the
MCU on tear-down unless explicitly stated by a param.

Change-Id: Ib70bf7399e7541f30b6905cdb950a6fb7b74ae18
Signed-off-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 26 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/45674/12

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 12:

(1 comment)

https://review.coreboot.org/c/flashrom/+/45674/10/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/10/realtek_mst_i2c_spi.c@476 
PS10, Line 476: 	if (reset) {
> Hi Edward, thanks to you! […]

No problem, done and rebased this on top.

[Sam McNally (Code Review)]

Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/45674 )

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 12: Code-Review+2

[Angel Pons (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 12: Code-Review+2

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

Patch Set 12:

(2 comments)

https://review.coreboot.org/c/flashrom/+/45674/4/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/4/realtek_mst_i2c_spi.c@438 
PS4, Line 438: 	if (reset_str && reset_str[0] == '1')
> Updated the code and verified that the reset logic works now.

Ack

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c 
File realtek_mst_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/45674/6/realtek_mst_i2c_spi.c@a429 
PS6, Line 429: ret = 0;
> Yes sorry about that I will stop make change directly here.

Ack

[Edward O'Callaghan (Code Review)]

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

Change subject: realtek_mst_i2c_spi.c: Introduce MCU reset param
......................................................................

realtek_mst_i2c_spi.c: Introduce MCU reset param

Modify the spi master as to not automatically reset the
MCU on tear-down unless explicitly stated by a param.

Change-Id: Ib70bf7399e7541f30b6905cdb950a6fb7b74ae18
Signed-off-by: Edward O'Callaghan 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/45674
Reviewed-by: Sam McNally 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M realtek_mst_i2c_spi.c
1 file changed, 26 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved
  Sam McNally: Looks good to me, approved

diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c
index 7867528..e975290 100644
--- a/realtek_mst_i2c_spi.c
+++ b/realtek_mst_i2c_spi.c
@@ -49,6 +49,7 @@

struct realtek_mst_i2c_spi_data {
    int fd;
+	int reset;
 };

static int realtek_mst_i2c_spi_write_data(int fd, uint16_t addr, void *buf, uint16_t len)
@@ -392,16 +393,20 @@
         struct realtek_mst_i2c_spi_data *realtek_mst_data =
    	(struct realtek_mst_i2c_spi_data *)data;
    int fd = realtek_mst_data->fd;
-	ret |= realtek_mst_i2c_spi_reset_mpu(fd);
+	if (realtek_mst_data->reset) {
+		ret |= realtek_mst_i2c_spi_reset_mpu(fd);
+		if (ret != 0)
+			msg_perr("%s: MCU failed to reset on tear-down.\n", __func__);
+	}
    i2c_close(fd);
    free(data);

return ret;
 }

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

bus_str = extract_programmer_param("bus");
@@ -431,6 +436,21 @@
    } else {
    	msg_perr("%s: Bus number not specified.\n", __func__);
    }
+
+	reset_str = extract_programmer_param("reset-mcu");
+	if (reset_str) {
+		if (reset_str[0] == '1')
+			*reset = 1;
+		else if (reset_str[0] == '0')
+			*reset = 0;
+		else {
+			msg_perr("%s: Incorrect param format, reset-mcu=1 or 0.\n", __func__);
+			ret = SPI_GENERIC_ERROR;
+		}
+	} else
+		*reset = 0; /* Default behaviour is no MCU reset on tear-down. */
+	free(reset_str);
+
 get_params_done:
    if (bus_str)
    	free(bus_str);
@@ -441,9 +461,9 @@
 int realtek_mst_i2c_spi_init(void)
 {
    int ret = 0;
-	int i2c_bus = 0;
+	int i2c_bus = 0, reset = 0;

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

int fd = i2c_open(i2c_bus, REGISTER_ADDRESS, 0);
@@ -461,6 +481,7 @@
    }

data->fd = fd;
+	data->reset = reset;
    ret |= register_shutdown(realtek_mst_i2c_spi_shutdown, data);

spi_master_i2c_realtek_mst.data = data;