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 <[e-mail]>
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");

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/41779
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I1b3022542f98f114097c6f39254ef4c2cf188c90
Gerrit-Change-Number: 41779
Gerrit-PatchSet: 1
Gerrit-Owner: Shiyu Sun <[e-mail]>
Gerrit-MessageType: newchange

[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



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/41779
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I1b3022542f98f114097c6f39254ef4c2cf188c90
Gerrit-Change-Number: 41779
Gerrit-PatchSet: 1
Gerrit-Owner: Shiyu Sun <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Wed, 27 May 2020 13:33:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[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.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/41779
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I1b3022542f98f114097c6f39254ef4c2cf188c90
Gerrit-Change-Number: 41779
Gerrit-PatchSet: 1
Gerrit-Owner: Shiyu Sun <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Wed, 27 May 2020 15:34:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

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 <[e-mail]>
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
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/41779
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I1b3022542f98f114097c6f39254ef4c2cf188c90
Gerrit-Change-Number: 41779
Gerrit-PatchSet: 2
Gerrit-Owner: Shiyu Sun <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: newpatchset

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?



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/41779
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I1b3022542f98f114097c6f39254ef4c2cf188c90
Gerrit-Change-Number: 41779
Gerrit-PatchSet: 2
Gerrit-Owner: Shiyu Sun <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Daniel Kurtz <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-Comment-Date: Fri, 29 May 2020 08:31:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

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



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/41779
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I1b3022542f98f114097c6f39254ef4c2cf188c90
Gerrit-Change-Number: 41779
Gerrit-PatchSet: 2
Gerrit-Owner: Shiyu Sun <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Daniel Kurtz <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-Comment-Date: Fri, 29 May 2020 12:39:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

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


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/41779
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I1b3022542f98f114097c6f39254ef4c2cf188c90
Gerrit-Change-Number: 41779
Gerrit-PatchSet: 2
Gerrit-Owner: Shiyu Sun <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Daniel Kurtz <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Sam McNally <[e-mail]>
Gerrit-Comment-Date: Tue, 04 Aug 2020 22:23:38 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment