Hello Duncan Laurie,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/em100/+/48584
to review the following change.
Change subject: Add more traceable SPI commands ......................................................................
Add more traceable SPI commands
Some commands always require 4 byte addresses, some require 3 or 2 depending on the mode, and some don't require an address. Adjust the table accordingly. This is inspired by some code from Duncan.
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: I9002253bf6eb7d60c90cdb75492076d5c5c48f21 --- M trace.c 1 file changed, 47 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/84/48584/1
diff --git a/trace.c b/trace.c index 108691b..46b52b2 100644 --- a/trace.c +++ b/trace.c @@ -110,27 +110,49 @@ };
struct spi_cmd_values spi_command_list[] = { - /* name cmd, addr, pad */ - {"write status register", 0x01, 0, 0}, - {"page program", 0x02, 1, 0}, - {"read", 0x03, 1, 0}, - {"write disable", 0x04, 0, 0}, - {"read status register", 0x05, 0, 0}, - {"write enable", 0x06, 0, 0}, - {"fast read", 0x0b, 1, 1}, - {"EM100 specific", 0x11, 0, 0}, - {"fast dual read", 0x3b, 1, 2}, - {"chip erase", 0x60, 0, 0}, - {"read JEDEC ID", 0x9f, 0, 0}, - {"chip erase", 0xc7, 0, 0}, - {"sector erase", 0xd8, 1, 0}, - {"enter 4-byte address mode", 0xb7, 0, 0}, - {"exit 4-byte address mode", 0xe9, 0, 0}, - {"enter quad i/o mode", 0x35, 0, 0}, - {"exit quad i/o mode", 0xf5, 0, 0}, - {"read SFDP Table", 0x5a, 1, 0}, + /* name cmd, addr, pad */ + {"read SFDP", 0x5a, 1, 0}, + {"write status register", 0x01, 0, 0}, + {"page program", 0x02, 1, 0}, + {"read", 0x03, 1, 0}, + {"write disable", 0x04, 0, 0}, + {"read status register", 0x05, 0, 0}, + {"write enable", 0x06, 0, 0}, + {"fast read", 0x0b, 1, 1}, + {"EM100 specific", 0x11, 0, 0}, + {"fast dual read", 0x3b, 1, 2}, + {"chip erase", 0x60, 0, 0}, + {"read JEDEC ID", 0x9f, 0, 0}, + {"chip erase c7h", 0xc7, 0, 0}, + {"chip erase 60h", 0x60, 0, 0}, + {"sector erase", 0xd8, 1, 0}, + {"dual I/O read", 0xbb, 1, 2}, + {"quad I/O read", 0xeb, 1, 0}, + {"quad read", 0x6b, 1, 0}, + {"quad I/O dt read", 0xed, 1, 0}, + {"quad page program", 0x38, 1, 0}, + {"sector erase", 0x20, 1, 0}, + {"block erase 32KB", 0x52, 1, 0}, + {"block erase 64KB", 0xd8, 1, 0}, + {"enter 4b mode", 0xb7, 0, 0}, + {"exit 4b mode", 0xe9, 0, 0}, + {"read 4b", 0x13, 2, 0}, + {"fast read 4b", 0x0c, 2, 0}, + {"dual I/O read 4b", 0xbc, 2, 0}, + {"dual out read 4b", 0x3c, 2, 0}, + {"quad I/O read 4b", 0xec, 2, 0}, + {"quad out read 4b", 0x6c, 2, 0}, + {"quad I/O dt read 4b", 0xee, 2, 0}, + {"page program 4b", 0x12, 2, 0}, + {"quad page program 4b", 0x3e, 2, 0}, + {"block erase 64KB 4b", 0xdc, 2, 0}, + {"block erase 32KB 4b", 0x5c, 2, 0}, + {"sector erase 4b", 0x21, 2, 0}, + {"enter quad I/O mode", 0x35, 0, 0}, + {"exit quad I/O mode", 0xf5, 0, 0},
- {"unknown command", 0xff, 0, 0} + {"unknown command", 0xff, 0, 0} + };
static struct spi_cmd_values * get_command_vals(uint8_t command) @@ -221,13 +243,17 @@ if (!spi_cmd_vals->uses_address) { j = 1; /* skip command byte */ } else { + /* If address_mode > 1 -> always decode 4 bytes */ + int address_bytes = + (spi_cmd_vals->uses_address == 1) ? address_mode : 4; + if (address_mode == 3) address = (data[i * 8 + 5] << 16) + (data[i * 8 + 6] << 8) + data[i * 8 + 7]; else address = (data[i * 8 + 5] << 24) + (data[i * 8 + 6] << 16) + (data[i * 8 + 7] << 8) + data[i * 8 + 8];
/* skip command, address bytes, and padding */ - j = 1 + address_mode + spi_cmd_vals->pad_bytes; + j = 1 + address_bytes + spi_cmd_vals->pad_bytes; if (j > MAX_TRACE_BLOCKLENGTH) { additional_pad_bytes = j - MAX_TRACE_BLOCKLENGTH;
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48584 )
Change subject: Add more traceable SPI commands ......................................................................
Patch Set 1: Code-Review+2
(3 comments)
https://review.coreboot.org/c/em100/+/48584/1/trace.c File trace.c:
https://review.coreboot.org/c/em100/+/48584/1/trace.c@108 PS1, Line 108: uses_address Since this now means something else (it was used as a boolean before), maybe a rename or using an enum could improve clarity?
https://review.coreboot.org/c/em100/+/48584/1/trace.c@250 PS1, Line 250: address_mode I've no idea, but should this be changed to `address_bytes`?
https://review.coreboot.org/c/em100/+/48584/1/trace.c@281 PS1, Line 281: if (spi_cmd_vals->uses_address) { Sanity check: does this need to be updated?
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/em100/+/48584
to look at the new patch set (#2).
Change subject: Add more traceable SPI commands ......................................................................
Add more traceable SPI commands
Some commands always require 4 byte addresses, some require 3 or 2 depending on the mode, and some don't require an address. Adjust the table accordingly. This is inspired by some code from Duncan.
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: I9002253bf6eb7d60c90cdb75492076d5c5c48f21 --- M trace.c 1 file changed, 96 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/84/48584/2
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48584 )
Change subject: Add more traceable SPI commands ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/em100/+/48584/1/trace.c File trace.c:
https://review.coreboot.org/c/em100/+/48584/1/trace.c@108 PS1, Line 108: uses_address
Since this now means something else (it was used as a boolean before), maybe a rename or using an en […]
done
https://review.coreboot.org/c/em100/+/48584/1/trace.c@250 PS1, Line 250: address_mode
I've no idea, but should this be changed to `address_bytes`?
Yep. Thanks.
https://review.coreboot.org/c/em100/+/48584/1/trace.c@281 PS1, Line 281: if (spi_cmd_vals->uses_address) {
Sanity check: does this need to be updated?
Done
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/em100/+/48584
to look at the new patch set (#3).
Change subject: Add more traceable SPI commands ......................................................................
Add more traceable SPI commands
Some commands always require 4 byte addresses, some require 3 or 2 depending on the mode, and some don't require an address. Adjust the table accordingly. This is inspired by some code from Duncan.
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: I9002253bf6eb7d60c90cdb75492076d5c5c48f21 --- M trace.c 1 file changed, 96 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/84/48584/3
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/em100/+/48584
to look at the new patch set (#4).
Change subject: Add more traceable SPI commands ......................................................................
Add more traceable SPI commands
Some commands always require 4 byte addresses, some require 3 or 2 depending on the mode, and some don't require an address. Adjust the table accordingly. This is inspired by some code from Duncan.
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: I9002253bf6eb7d60c90cdb75492076d5c5c48f21 --- M trace.c 1 file changed, 96 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/84/48584/4
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48584 )
Change subject: Add more traceable SPI commands ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/em100/+/48584/4/trace.c File trace.c:
https://review.coreboot.org/c/em100/+/48584/4/trace.c@264 PS4, Line 264: always decode 4 bytes I am not sure I understand this comment.
https://review.coreboot.org/c/em100/+/48584/4/trace.c@321 PS4, Line 321: address += 16; Isn't this only for address_type != ADDR_NONE? Probably doesn't matter since it will not be used in the for loop above?
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48584 )
Change subject: Add more traceable SPI commands ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/em100/+/48584/4/trace.c File trace.c:
https://review.coreboot.org/c/em100/+/48584/4/trace.c@264 PS4, Line 264: always decode 4 bytes
I am not sure I understand this comment.
I removed the whole comment now, it made sense before I reworked the address selection code but not anymore.
https://review.coreboot.org/c/em100/+/48584/4/trace.c@321 PS4, Line 321: address += 16;
Isn't this only for address_type != ADDR_NONE? Probably doesn't matter since it will not be used in […]
Yes, the address is never printed, so in theory we could decide to only count up here when we are actually printing it.
I had decided to simplify the code and "track" the index unconditionally to make the code a bit more straightforward.
If you feel that's misguidedm I can put the conditional increase back in.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/em100/+/48584
to look at the new patch set (#5).
Change subject: Add more traceable SPI commands ......................................................................
Add more traceable SPI commands
Some commands always require 4 byte addresses, some require 3 or 2 depending on the mode, and some don't require an address. Adjust the table accordingly. This is inspired by some code from Duncan.
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: I9002253bf6eb7d60c90cdb75492076d5c5c48f21 --- M trace.c 1 file changed, 95 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/84/48584/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48584 )
Change subject: Add more traceable SPI commands ......................................................................
Patch Set 5: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48584 )
Change subject: Add more traceable SPI commands ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/em100/+/48584/4/trace.c File trace.c:
https://review.coreboot.org/c/em100/+/48584/4/trace.c@321 PS4, Line 321: address += 16;
Yes, the address is never printed, so in theory we could decide to only count up here when we are ac […]
No, I think it is fine. Just wanted to make sure I understood the intent.
Stefan Reinauer has submitted this change. ( https://review.coreboot.org/c/em100/+/48584 )
Change subject: Add more traceable SPI commands ......................................................................
Add more traceable SPI commands
Some commands always require 4 byte addresses, some require 3 or 2 depending on the mode, and some don't require an address. Adjust the table accordingly. This is inspired by some code from Duncan.
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: I9002253bf6eb7d60c90cdb75492076d5c5c48f21 Reviewed-on: https://review.coreboot.org/c/em100/+/48584 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Furquan Shaikh furquan@google.com --- M trace.c 1 file changed, 95 insertions(+), 43 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/trace.c b/trace.c index 73b38e9..7ab2f5c 100644 --- a/trace.c +++ b/trace.c @@ -102,35 +102,59 @@ return 1; }
+typedef enum { ADDR_NONE, ADDR_NO_OFF_3B, ADDR_3B, ADDR_4B, ADDR_DYNAMIC } address_t; + struct spi_cmd_values { const char *cmd_name; uint8_t cmd; - uint8_t uses_address; + address_t address_type; uint8_t pad_bytes; };
struct spi_cmd_values spi_command_list[] = { - /* name cmd, addr, pad */ - {"write status register", 0x01, 0, 0}, - {"page program", 0x02, 1, 0}, - {"read", 0x03, 1, 0}, - {"write disable", 0x04, 0, 0}, - {"read status register", 0x05, 0, 0}, - {"write enable", 0x06, 0, 0}, - {"fast read", 0x0b, 1, 1}, - {"EM100 specific", 0x11, 0, 0}, - {"fast dual read", 0x3b, 1, 2}, - {"chip erase", 0x60, 0, 0}, - {"read JEDEC ID", 0x9f, 0, 0}, - {"chip erase", 0xc7, 0, 0}, - {"sector erase", 0xd8, 1, 0}, - {"enter 4-byte address mode", 0xb7, 0, 0}, - {"exit 4-byte address mode", 0xe9, 0, 0}, - {"enter quad i/o mode", 0x35, 0, 0}, - {"exit quad i/o mode", 0xf5, 0, 0}, - {"read SFDP Table", 0x5a, 1, 0}, + /* name cmd, addr, pad */ + {"read SFDP", 0x5a, ADDR_NO_OFF_3B, 0}, + {"write status register", 0x01, ADDR_NONE, 0}, + {"page program", 0x02, ADDR_DYNAMIC, 0}, + {"read", 0x03, ADDR_DYNAMIC, 0}, + {"write disable", 0x04, ADDR_NONE, 0}, + {"read status register", 0x05, ADDR_NONE, 0}, + {"write enable", 0x06, ADDR_NONE, 0}, + {"fast read", 0x0b, ADDR_DYNAMIC, 1}, + {"EM100 specific", 0x11, ADDR_NONE, 0}, + {"fast dual read", 0x3b, ADDR_DYNAMIC, 2}, + {"chip erase", 0x60, ADDR_NONE, 0}, + {"read JEDEC ID", 0x9f, ADDR_NONE, 0}, + {"chip erase c7h", 0xc7, ADDR_NONE, 0}, + {"chip erase 60h", 0x60, ADDR_NONE, 0}, + {"sector erase", 0xd8, ADDR_DYNAMIC, 0}, + {"dual I/O read", 0xbb, ADDR_DYNAMIC, 2}, + {"quad I/O read", 0xeb, ADDR_DYNAMIC, 0}, + {"quad read", 0x6b, ADDR_DYNAMIC, 0}, + {"quad I/O dt read", 0xed, ADDR_DYNAMIC, 0}, + {"quad page program", 0x38, ADDR_DYNAMIC, 0}, + {"sector erase", 0x20, ADDR_DYNAMIC, 0}, + {"block erase 32KB", 0x52, ADDR_DYNAMIC, 0}, + {"block erase 64KB", 0xd8, ADDR_DYNAMIC, 0}, + {"enter 4b mode", 0xb7, ADDR_NONE, 0}, + {"exit 4b mode", 0xe9, ADDR_NONE, 0}, + {"read 4b", 0x13, ADDR_4B, 0}, + {"fast read 4b", 0x0c, ADDR_4B, 0}, + {"dual I/O read 4b", 0xbc, ADDR_4B, 0}, + {"dual out read 4b", 0x3c, ADDR_4B, 0}, + {"quad I/O read 4b", 0xec, ADDR_4B, 0}, + {"quad out read 4b", 0x6c, ADDR_4B, 0}, + {"quad I/O dt read 4b", 0xee, ADDR_4B, 0}, + {"page program 4b", 0x12, ADDR_4B, 0}, + {"quad page program 4b", 0x3e, ADDR_4B, 0}, + {"block erase 64KB 4b", 0xdc, ADDR_4B, 0}, + {"block erase 32KB 4b", 0x5c, ADDR_4B, 0}, + {"sector erase 4b", 0x21, ADDR_4B, 0}, + {"enter quad I/O mode", 0x35, ADDR_NONE, 0}, + {"exit quad I/O mode", 0xf5, ADDR_NONE, 0},
- {"unknown command", 0xff, 0, 0} + {"unknown command", 0xff, ADDR_NONE, 0} + };
static struct spi_cmd_values * get_command_vals(uint8_t command) @@ -160,7 +184,7 @@ unsigned int count, i, report; static int outbytes = 0; static int additional_pad_bytes = 0; - static unsigned int address = 0; + static unsigned long address = 0; static unsigned long long timestamp = 0; static unsigned long long start_timestamp = 0; static struct spi_cmd_values *spi_cmd_vals = &spi_command_list[3]; @@ -176,6 +200,7 @@ count = 1022; } for (i = 0; i < count; i++) { + int address_bytes = 0; unsigned int j = additional_pad_bytes; additional_pad_bytes = 0; unsigned char cmd = data[2 + i*8]; @@ -217,23 +242,44 @@ break; }
- /* set up address if used by this command*/ - if (!spi_cmd_vals->uses_address) { - j = 1; /* skip command byte */ - } else { - if (address_mode == 3) - address = (data[i * 8 + 5] << 16) + (data[i * 8 + 6] << 8) + data[i * 8 + 7]; - else - address = (data[i * 8 + 5] << 24) + (data[i * 8 + 6] << 16) + (data[i * 8 + 7] << 8) + data[i * 8 + 8]; + /* skip command byte */ + j = 1;
- /* skip command, address bytes, and padding */ - j = 1 + address_mode + spi_cmd_vals->pad_bytes; - if (j > MAX_TRACE_BLOCKLENGTH) { - additional_pad_bytes = j - - MAX_TRACE_BLOCKLENGTH; - j = MAX_TRACE_BLOCKLENGTH; - } + /* Set up address if used by this command */ + switch (spi_cmd_vals->address_type) { + case ADDR_DYNAMIC: + address_bytes = address_mode; + break; + case ADDR_NO_OFF_3B: + case ADDR_3B: + address_bytes = 3; + break; + case ADDR_4B: + address_bytes = 4; + break; + case ADDR_NONE: + break; } + + if (address_bytes == 3) + address = (data[i * 8 + 5] << 16) + + (data[i * 8 + 6] << 8) + + data[i * 8 + 7]; + else if (address_bytes == 4) + address = (data[i * 8 + 5] << 24) + + (data[i * 8 + 6] << 16) + + (data[i * 8 + 7] << 8) + + data[i * 8 + 8]; + + /* skip address bytes and padding */ + j += address_bytes + spi_cmd_vals->pad_bytes; + + if (j > MAX_TRACE_BLOCKLENGTH) { + additional_pad_bytes = j - + MAX_TRACE_BLOCKLENGTH; + j = MAX_TRACE_BLOCKLENGTH; + } + printf("\nTime: %06lld.%08lld", (timestamp - start_timestamp) / 100000000, @@ -252,20 +298,26 @@
for (; j < blocklen; j++) { if (outbytes == 0) { - if (spi_cmd_vals->uses_address) { + switch (spi_cmd_vals->address_type) { + case ADDR_DYNAMIC: + case ADDR_3B: + case ADDR_4B: printf("\n%08lx : ", - addr_offset + - address); - } else { + addr_offset + address); + break; + case ADDR_NO_OFF_3B: + printf("\n%08lx : ", address); + break; + case ADDR_NONE: printf("\n : "); + break; } } printf("%02x ", data[i * 8 + 4 + j]); outbytes++; if (outbytes == 16) { outbytes = 0; - if (spi_cmd_vals->uses_address) - address += 16; + address += 16; } } // this is because the em100 counts funny
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Stefan Reinauer (Code Review)