Stefan Reinauer has uploaded this change for review. ( https://review.coreboot.org/c/em100/+/48553 )
Change subject: Support 4-byte address mode in SPI traces ......................................................................
Support 4-byte address mode in SPI traces
- Track address mode globally, so we can access the user presets when reading SPI traces. - Enable address mode switching based on the SPI opcodes 0xB7 / 0xE9 - Read additional address byte when appropriate and skip it in the data stream - Skip trace packets without valid data
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Change-Id: Iec9f36356eb7e612a909855cda85dc0ce1b5adc0 --- M em100.c M em100.h M trace.c 3 files changed, 23 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/53/48553/1
diff --git a/em100.c b/em100.c index c4b6c01..ca6e683 100644 --- a/em100.c +++ b/em100.c @@ -29,6 +29,7 @@ TFILE *configs; char *database_version; int debug = 0; +int address_mode = 3;
volatile sig_atomic_t exit_requested = 0; static struct em100 em100_state; @@ -835,7 +836,6 @@ int compatibility = 0; int bus = 0, device = 0; int firmware_is_dpfw = 0; - int address_mode = 0; unsigned int serial_number = 0; unsigned long address_offset = 0; unsigned int spi_start_address = 0; diff --git a/em100.h b/em100.h index d49339b..ecb74d2 100644 --- a/em100.h +++ b/em100.h @@ -197,6 +197,7 @@ #define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0])) char *get_em100_file(const char *name); extern int debug; +extern int address_mode; extern volatile sig_atomic_t exit_requested;
/* Chips */ diff --git a/trace.c b/trace.c index 0c6cdfd..108691b 100644 --- a/trace.c +++ b/trace.c @@ -128,6 +128,7 @@ {"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},
{"unknown command", 0xff, 0, 0} }; @@ -178,6 +179,11 @@ unsigned int j = additional_pad_bytes; additional_pad_bytes = 0; unsigned char cmd = data[2 + i*8]; + + if (cmd == 0x00) { + /* packet without valid data */ + continue; + } if (cmd == 0xff) { /* timestamp */ timestamp = data[2 + i*8 + 2]; @@ -201,16 +207,27 @@ if (counter == 0) start_timestamp = timestamp;
+ /* Special commands */ + switch (spi_command) { + case 0xb7: + address_mode = 4; + break; + case 0xe9: + address_mode = 3; + break; + } + /* set up address if used by this command*/ if (!spi_cmd_vals->uses_address) { j = 1; /* skip command byte */ } else { - address = (data[i * 8 + 5] << 16) + - (data[i * 8 + 6] << 8) + - data[i * 8 + 7]; + 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 = 4 + spi_cmd_vals->pad_bytes; + j = 1 + address_mode + 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/+/48553 )
Change subject: Support 4-byte address mode in SPI traces ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/em100/+/48553/1/trace.c File trace.c:
https://review.coreboot.org/c/em100/+/48553/1/trace.c@210 PS1, Line 210: /* Special commands */ : switch (spi_command) { : case 0xb7: : address_mode = 4; : break; : case 0xe9: : address_mode = 3; : break; : } Huh, this part is indented with spaces
https://review.coreboot.org/c/em100/+/48553/1/trace.c@224 PS1, Line 224: 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]; How about:
address = 0; for (int n = 0; n < address_mode; n++) { address <<= 8; address |= data[i * 8 + 4 + n]; }
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Duncan Laurie, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/em100/+/48553
to look at the new patch set (#2).
Change subject: Support 4-byte address mode in SPI traces ......................................................................
Support 4-byte address mode in SPI traces
- Track address mode globally, so we can access the user presets when reading SPI traces. - Enable address mode switching based on the SPI opcodes 0xB7 / 0xE9 - Read additional address byte when appropriate and skip it in the data stream - Skip trace packets without valid data
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Change-Id: Iec9f36356eb7e612a909855cda85dc0ce1b5adc0 --- M em100.c M em100.h M trace.c 3 files changed, 23 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/53/48553/2
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48553 )
Change subject: Support 4-byte address mode in SPI traces ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/em100/+/48553/1/trace.c File trace.c:
https://review.coreboot.org/c/em100/+/48553/1/trace.c@210 PS1, Line 210: /* Special commands */ : switch (spi_command) { : case 0xb7: : address_mode = 4; : break; : case 0xe9: : address_mode = 3; : break; : }
Huh, this part is indented with spaces
Done
https://review.coreboot.org/c/em100/+/48553/1/trace.c@224 PS1, Line 224: 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];
How about: […]
Besides the off by one error there, I'm not sure that this contributes to the readability of the code, although it is kind of neat. I had it in there and removed it again, for now.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48553 )
Change subject: Support 4-byte address mode in SPI traces ......................................................................
Patch Set 3: Code-Review+2
Stefan Reinauer has submitted this change. ( https://review.coreboot.org/c/em100/+/48553 )
Change subject: Support 4-byte address mode in SPI traces ......................................................................
Support 4-byte address mode in SPI traces
- Track address mode globally, so we can access the user presets when reading SPI traces. - Enable address mode switching based on the SPI opcodes 0xB7 / 0xE9 - Read additional address byte when appropriate and skip it in the data stream - Skip trace packets without valid data
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Change-Id: Iec9f36356eb7e612a909855cda85dc0ce1b5adc0 Reviewed-on: https://review.coreboot.org/c/em100/+/48553 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M em100.c M em100.h M trace.c 3 files changed, 23 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/em100.c b/em100.c index ffed4d7..4b0a121 100644 --- a/em100.c +++ b/em100.c @@ -29,6 +29,7 @@ TFILE *configs; char *database_version; int debug = 0; +int address_mode = 3;
volatile sig_atomic_t exit_requested = 0; static struct em100 em100_state; @@ -835,7 +836,6 @@ int compatibility = 0; int bus = 0, device = 0; int firmware_is_dpfw = 0; - int address_mode = 0; unsigned int serial_number = 0; unsigned long address_offset = 0; unsigned int spi_start_address = 0; diff --git a/em100.h b/em100.h index d49339b..ecb74d2 100644 --- a/em100.h +++ b/em100.h @@ -197,6 +197,7 @@ #define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0])) char *get_em100_file(const char *name); extern int debug; +extern int address_mode; extern volatile sig_atomic_t exit_requested;
/* Chips */ diff --git a/trace.c b/trace.c index 0c6cdfd..73b38e9 100644 --- a/trace.c +++ b/trace.c @@ -128,6 +128,7 @@ {"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},
{"unknown command", 0xff, 0, 0} }; @@ -178,6 +179,11 @@ unsigned int j = additional_pad_bytes; additional_pad_bytes = 0; unsigned char cmd = data[2 + i*8]; + + if (cmd == 0x00) { + /* packet without valid data */ + continue; + } if (cmd == 0xff) { /* timestamp */ timestamp = data[2 + i*8 + 2]; @@ -201,16 +207,27 @@ if (counter == 0) start_timestamp = timestamp;
+ /* Special commands */ + switch (spi_command) { + case 0xb7: + address_mode = 4; + break; + case 0xe9: + address_mode = 3; + break; + } + /* set up address if used by this command*/ if (!spi_cmd_vals->uses_address) { j = 1; /* skip command byte */ } else { - address = (data[i * 8 + 5] << 16) + - (data[i * 8 + 6] << 8) + - data[i * 8 + 7]; + 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 = 4 + spi_cmd_vals->pad_bytes; + j = 1 + address_mode + spi_cmd_vals->pad_bytes; if (j > MAX_TRACE_BLOCKLENGTH) { additional_pad_bytes = j - MAX_TRACE_BLOCKLENGTH;
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Stefan Reinauer (Code Review)