Change in flashrom[master]: lspcon_i2c_spi.c: Add debug message that shows the active block.

[Shiyu Sun (Code Review)]

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

Change subject: lspcon_i2c_spi.c: Add debug message that shows the active block.
......................................................................

lspcon_i2c_spi.c: Add debug message that shows the active block.

This is all for debug propose to verify A/B update work as
expected.

BUG=b:148746232
BRANCH=none
TEST=flashrom -p lspcon_i2c_spi:bus=7 --flash-size -V
    verified the related message appears.

Signed-off-by: Shiyu Sun 
Change-Id: I1b3022542f98f114097c6f39254ef4c2cf188c90
---
M lspcon_i2c_spi.c
1 file changed, 25 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/41779/1

diff --git a/lspcon_i2c_spi.c b/lspcon_i2c_spi.c
index 7b9f1c0..e5e5dc9 100644
--- a/lspcon_i2c_spi.c
+++ b/lspcon_i2c_spi.c
@@ -27,6 +27,7 @@

#define REGISTER_ADDRESS	(0x94 >> 1)
 #define PAGE_ADDRESS		(0x9e >> 1)
+#define ACTIVE_BLOCK_ADDRESS   (0x9a >> 1)
 #define PAGE_SIZE		256
 #define MAX_SPI_WAIT_RETRIES	1000

@@ -472,6 +473,27 @@
    return ret;
 }

+static void lspcon_i2c_spi_boot_block_info(int fd)
+{
+	uint8_t block_id = 0;
+	int ret = 0;
+	ret |= lspcon_i2c_spi_write_register(fd, 0xbc, 0xc0);
+	ret |= lspcon_i2c_spi_write_register(fd, 0xbc, 0x40);
+	ret |= lspcon_i2c_spi_write_register(fd, 0x8e, 0x00);
+	ret |= lspcon_i2c_spi_write_register(fd, 0x8f, 0x00);
+	struct timespec wait_10ms = { 0, (unsigned)1e7 };
+	nanosleep(&wait_10ms, NULL);
+
+	uint8_t command[] = { 0x0e };
+	ret |= lspcon_i2c_spi_write_data(fd, ACTIVE_BLOCK_ADDRESS, command, 1);
+	ret |= lspcon_i2c_spi_read_data(fd, ACTIVE_BLOCK_ADDRESS, &block_id, 1);
+	if (ret) {
+		msg_pdbg("Failed to get LSPCON boot block information.\n");
+	} else {
+		msg_pdbg("LSPCON is booting from block #%d.\n", block_id);
+	}
+}
+
 int lspcon_i2c_spi_init(void)
 {
    int lspcon_i2c_spi_bus = get_bus();
@@ -489,6 +511,9 @@
    	return ret;
    }

+	/* Show current activate block information for debug propose. */
+	lspcon_i2c_spi_boot_block_info(fd);
+
    struct lspcon_i2c_spi_data *data = calloc(1, sizeof(struct lspcon_i2c_spi_data));
    if (!data) {
    	msg_perr("Unable to allocate space for extra SPI master data.\n");

[Shiyu Sun (Code Review)]

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

Change subject: lspcon_i2c_spi.c: Add debug message that shows the active block.
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/flashrom/+/41779/1/lspcon_i2c_spi.c 
File lspcon_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/41779/1/lspcon_i2c_spi.c@30 
PS1, Line 30: #define ACTIVE_BLOCK_ADDRESS   (0x9a >> 1)
will fix this indentation

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41779 )

Change subject: lspcon_i2c_spi.c: Add debug message that shows the active block.
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/flashrom/+/41779/1//COMMIT_MSG@7 
PS1, Line 7: lspcon_i2c_spi.c: Add debug message that shows the active block.
Please remove the period/dot at the end of the git commit message summary.

Change in flashrom[master]: lspcon_i2c_spi.c: Add debug message that shows the active block

[Shiyu Sun (Code Review)]

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

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

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

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

Change subject: lspcon_i2c_spi.c: Add debug message that shows the active block
......................................................................

lspcon_i2c_spi.c: Add debug message that shows the active block

This is all for debug propose to verify A/B update work as
expected.

BUG=b:148746232
BRANCH=none
TEST=flashrom -p lspcon_i2c_spi:bus=7 --flash-size -V
    verified the related message appears.

Signed-off-by: Shiyu Sun 
Change-Id: I1b3022542f98f114097c6f39254ef4c2cf188c90
---
M lspcon_i2c_spi.c
1 file changed, 25 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/41779/2

Change in flashrom[master]: lspcon_i2c_spi.c: Add debug message that shows the active block

[Shiyu Sun (Code Review)]

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

Change subject: lspcon_i2c_spi.c: Add debug message that shows the active block
......................................................................

Patch Set 2:

(2 comments)

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

https://review.coreboot.org/c/flashrom/+/41779/1//COMMIT_MSG@7 
PS1, Line 7: lspcon_i2c_spi.c: Add debug message that shows the active block.
> Please remove the period/dot at the end of the git commit message summary.

Done

https://review.coreboot.org/c/flashrom/+/41779/1/lspcon_i2c_spi.c 
File lspcon_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/41779/1/lspcon_i2c_spi.c@30 
PS1, Line 30: #define ACTIVE_BLOCK_ADDRESS   (0x9a >> 1)
> will fix this indentation

Done

Change in flashrom[master]: lspcon_i2c_spi.c: Add debug message that shows the active block

[Edward O'Callaghan (Code Review)]

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

Change subject: lspcon_i2c_spi.c: Add debug message that shows the active block
......................................................................

Patch Set 2:

(7 comments)

https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c 
File lspcon_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@476 
PS2, Line 476: static void lspcon_i2c_spi_boot_block_info(int fd)
move this function after `lspcon_i2c_spi_map_page(int fd, unsigned int offset)`

https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@480 
PS2, Line 480: 	ret |= lspcon_i2c_spi_write_register(fd, 0xbc, 0xc0);
             : 	ret |= lspcon_i2c_spi_write_register(fd, 0xbc, 0x40);
The first two magics programmed here are equiv to `lspcon_i2c_spi_reset_mpu_stop(int fd) {}` and so not needed.

https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@482 
PS2, Line 482: 	ret |= lspcon_i2c_spi_write_register(fd, 0x8e, 0x00);
             : 	ret |= lspcon_i2c_spi_write_register(fd, 0x8f, 0x00);
Put a comment above, `/* Set rom addr to beginning. */`

and,

```
#define ROMADDR_BYTE1		0x8e
#define ROMADDR_BYTE2		0x8f
```

so you can just use `lspcon_i2c_spi_map_page(int fd, unsigned int offset)`

https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@484 
PS2, Line 484: struct timespec wait_10ms = { 0, (unsigned)1e7 };
I suspect this wait is for the unnecessary MPU reset above, so you can get rid of it here.

https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@487 
PS2, Line 487: 0x0e
Perhaps this should be a #define or have a inline comment /* .. */ that has the name of what the vendor calls this special command magic.

https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@490 
PS2, Line 490:  {
{} unnecessary for single lines.

https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@491 
PS2, Line 491: pdbg("F
msg_perr() and use `%s: Failed to..` with __func__.

As mentioned before, consider yourself as a user seeing the word "Error/Failed" in a log - what are you going to grep for?

Change in flashrom[master]: lspcon_i2c_spi.c: Add debug message that shows the active block

[Sam McNally (Code Review)]

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

Change subject: lspcon_i2c_spi.c: Add debug message that shows the active block
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c 
File lspcon_i2c_spi.c:

https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@482 
PS2, Line 482: 	ret |= lspcon_i2c_spi_write_register(fd, 0x8e, 0x00);
             : 	ret |= lspcon_i2c_spi_write_register(fd, 0x8f, 0x00);
> Put a comment above, `/* Set rom addr to beginning. */` […]

Why is this being done?

https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@487 
PS2, Line 487: 0x0e
> Perhaps this should be a #define or have a inline comment /* .. […]

It's just the register being read.  There probably should be more generic i2c register read/write functions that don't hard-code the i2c address for cases like this.

Change in flashrom[master]: lspcon_i2c_spi.c: Add debug message that shows the active block

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41779 )

Change subject: lspcon_i2c_spi.c: Add debug message that shows the active block
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/41779/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/41779/2//COMMIT_MSG@9 
PS2, Line 9: work
works

Change in flashrom[master]: lspcon_i2c_spi.c: Add debug message that shows the active block

[Angel Pons (Code Review)]

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

Change subject: lspcon_i2c_spi.c: Add debug message that shows the active block
......................................................................

Patch Set 2: Code-Review+1