Hello Duncan Laurie,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/em100/+/48599
to review the following change.
Change subject: em100: Add trace console command ......................................................................
em100: Add trace console command
Add a command which allows a realtime console output from em100 when using the CONFIG_SPI_FLASH_CONSOLE option in coreboot.
Provide the offset and lenght of the region defined in the FMAP and it will watch for the host to write to that area, and when it sees a write it will print it to stdout.
Change-Id: I78162585bcd73a3bb3a8aa590f43a96092946109 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org --- M em100.c M em100.h M trace.c 3 files changed, 135 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/99/48599/1
diff --git a/em100.c b/em100.c index 4b0a121..f808ed6 100644 --- a/em100.c +++ b/em100.c @@ -787,6 +787,8 @@ {"list-devices", 0, 0, 'l'}, {"update-files", 0, 0, 'U'}, {"terminal", 0, 0, 'T'}, + {"traceconsole", 0, 0, 'R'}, + {"length", 1, 0, 'L'}, {"compatible", 0, 0, 'C'}, {NULL, 0, 0, 0} }; @@ -807,6 +809,8 @@ " -t|--trace: trace mode\n" " -O|--offset HEX_VAL: address offset for trace mode\n" " -T|--terminal: terminal mode\n" + " -R|--traceconsole: trace console mode\n" + " -L|--length HEX_VAL: length of buffer for traceconsole mode\n" " -F|--firmware-update FILE|auto: update EM100pro firmware (dangerous)\n" " -f|--firmware-dump FILE: export raw EM100pro firmware to file\n" " -g|--firmware-write FILE: export EM100pro firmware to DPFW file\n" @@ -832,18 +836,18 @@ const char *firmware_in = NULL, *firmware_out = NULL; const char *holdpin = NULL; int do_start = 0, do_stop = 0; - int verify = 0, trace = 0, terminal=0; + int verify = 0, trace = 0, terminal=0, traceconsole=0; int compatibility = 0; int bus = 0, device = 0; int firmware_is_dpfw = 0; unsigned int serial_number = 0; - unsigned long address_offset = 0; + unsigned long address_offset = 0, address_length = 0; unsigned int spi_start_address = 0; const char *voltage = NULL; struct sigaction signal_action; struct em100 *em100 = &em100_state;
- while ((opt = getopt_long(argc, argv, "c:d:a:m:u:rsvtO:F:f:g:S:V:p:DCx:lUhT", + while ((opt = getopt_long(argc, argv, "c:d:a:m:u:rsvtO:F:f:g:S:V:p:DCx:lUhTRL:", longopts, &idx)) != -1) { switch (opt) { case 'c': @@ -919,6 +923,12 @@ case 'U': update_all_files(); return 0; + case 'R': + traceconsole = 1; + break; + case 'L': + sscanf(optarg, "%lx", &address_length); + break; case 'C': compatibility = 1; break; @@ -1206,7 +1216,7 @@ set_state(em100, 1); }
- if (trace || terminal) { + if (trace || terminal || traceconsole) { int usb_errors = 0;
if ((holdpin == NULL) && (!set_hold_pin_state(em100, 3))) { @@ -1220,7 +1230,7 @@
printf ("Starting ");
- if (trace) { + if (trace || traceconsole) { reset_spi_trace(em100); printf("trace%s", terminal ? " & " : ""); } @@ -1234,13 +1244,18 @@
while (!exit_requested && usb_errors < MAX_USB_ERRORS) { int ret; - if (trace) { + + if (traceconsole) + ret = read_spi_trace_console(em100, address_offset, address_length); + else if (trace) ret = read_spi_trace(em100, terminal, address_offset); - } else { + else if (terminal) ret = read_spi_terminal(em100, 0); - } + if (ret == 0) usb_errors++; + if (ret < 0) + break; } if (usb_errors >= MAX_USB_ERRORS) printf("Error: Bailed out with too many USB errors.\n"); diff --git a/em100.h b/em100.h index ecb74d2..bf1bb87 100644 --- a/em100.h +++ b/em100.h @@ -172,6 +172,8 @@ unsigned long addr_offset); int read_spi_terminal(struct em100 *em100, int print_counter); int init_spi_terminal(struct em100 *em100); +int read_spi_trace_console(struct em100 *em100, unsigned long addr_offset, + unsigned long addr_len);
/* Archive handling */ typedef struct { diff --git a/trace.c b/trace.c index 88265b5..7fe6ab8 100644 --- a/trace.c +++ b/trace.c @@ -305,6 +305,116 @@ return 1; }
+int read_spi_trace_console(struct em100 *em100, unsigned long addr_offset, + unsigned long addr_len) +{ + unsigned char reportdata[REPORT_BUFFER_COUNT][REPORT_BUFFER_LENGTH] = + {{0}}; + unsigned char *data; + unsigned int count, i, report; + static int additional_pad_bytes = 0; + static unsigned int address = 0; + static int do_write = 0; + static struct spi_cmd_values *spi_cmd_vals = &spi_command_list[3]; + int addr_bytes = 4; + + if (addr_offset == 0) { + printf("Address offset for console buffer required\n"); + return -1; + } + if (addr_len == 0) { + printf("Console buffer length required\n"); + return -1; + } + + if (!read_report_buffer(em100, reportdata)) + return -1; + + for (report = 0; report < REPORT_BUFFER_COUNT; report++) { + data = &reportdata[report][0]; + count = (data[0] << 8) | data[1]; + if (!count) + continue; + if (count > 1023) { + printf("Warning: EM100pro sends too much data: %d.\n", count); + count = 1023; + } + for (i = 0; i < count; i++) { + unsigned int j = additional_pad_bytes; + unsigned int address_bytes = 0; + additional_pad_bytes = 0; + unsigned char cmd = data[2 + i*8]; + + /* from here, it must be data */ + if (cmd != cmdid) { + unsigned char spi_command = data[i * 8 + 4]; + spi_cmd_vals = get_command_vals(spi_command); + + /* new command */ + cmdid = cmd; + + /* set up address if used by this command */ + if (!spi_cmd_vals->uses_address) { + j = 1; /* skip command byte */ + } else { + /* If address_mode > 1 -> always decode 4 bytes */ + 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 = address_bytes + 1 + spi_cmd_vals->pad_bytes; + if (j > MAX_TRACE_BLOCKLENGTH) { + additional_pad_bytes = j - + MAX_TRACE_BLOCKLENGTH; + j = MAX_TRACE_BLOCKLENGTH; + } + } + +#if 0 + if (spi_cmd_vals->uses_address) + printf("0x%02x @ 0x%08x (%s)\n", + spi_command, address, spi_cmd_vals->cmd_name); + else + printf("0x%02x (%s)\n", + spi_command, spi_cmd_vals->cmd_name); +#endif + + curpos = 0; + if (spi_command == 0x02) + do_write = 1; + else + do_write = 0; + } + + if (do_write == 0 || + (spi_cmd_vals->uses_address == 0 || + address < addr_offset || + address > (addr_offset + addr_len))) { + curpos = data[2 + i*8 + 1] + 0x10; + continue; + } + + /* this exploits 8bit wrap around in curpos */ + unsigned char blocklen = (data[2 + i*8 + 1] - curpos); + blocklen /= 8; + + for (; j < blocklen; j++) { + printf("%c", data[i * 8 + addr_bytes + j]); + } + + /* this is because the em100 counts funny */ + curpos = data[2 + i*8 + 1] + 0x10; + fflush(stdout); + } + } + return 1; +} + #define UFIFO_SIZE 512 #define UFIFO_TIMEOUT 0x00
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48599 )
Change subject: em100: Add trace console command ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/em100/+/48599/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/em100/+/48599/1//COMMIT_MSG@12 PS1, Line 12: lenght length
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/em100/+/48599
to look at the new patch set (#2).
Change subject: em100: Add SPI trace console ......................................................................
em100: Add SPI trace console
Add a command which allows a realtime console output from em100 when using the CONFIG_SPI_FLASH_CONSOLE option in coreboot.
Provide the offset and lenght of the region defined in the FMAP and it will watch for the host to write to that area, and when it sees a write it will print it to stdout.
Unlike the already implemented SPI terminal function, this does not rely on the host sending EM100 specific commands (that might be filtered out by the SPI controller or the ME)
Change-Id: I78162585bcd73a3bb3a8aa590f43a96092946109 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org --- M em100.c M em100.h M trace.c 3 files changed, 162 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/99/48599/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48599 )
Change subject: em100: Add SPI trace console ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/em100/+/48599/2/trace.c File trace.c:
https://review.coreboot.org/c/em100/+/48599/2/trace.c@345 PS2, Line 345: 4 Shouldn't this be 0?
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/em100/+/48599
to look at the new patch set (#3).
Change subject: em100: Add SPI trace console ......................................................................
em100: Add SPI trace console
Add a command which allows a realtime console output from em100 when using the CONFIG_SPI_FLASH_CONSOLE option in coreboot.
Provide the offset and lenght of the region defined in the FMAP and it will watch for the host to write to that area, and when it sees a write it will print it to stdout.
Unlike the already implemented SPI terminal function, this does not rely on the host sending EM100 specific commands (that might be filtered out by the SPI controller or the ME)
Change-Id: I78162585bcd73a3bb3a8aa590f43a96092946109 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org --- M em100.c M em100.h M trace.c 3 files changed, 162 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/99/48599/3
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48599 )
Change subject: em100: Add SPI trace console ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/em100/+/48599/2/trace.c File trace.c:
https://review.coreboot.org/c/em100/+/48599/2/trace.c@345 PS2, Line 345: 4
Shouldn't this be 0?
I'll be .... yes it should.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/em100/+/48599
to look at the new patch set (#4).
Change subject: em100: Add SPI trace console ......................................................................
em100: Add SPI trace console
Add a command which allows a realtime console output from em100 when using the CONFIG_SPI_FLASH_CONSOLE option in coreboot.
Provide the offset and lenght of the region defined in the FMAP and it will watch for the host to write to that area, and when it sees a write it will print it to stdout.
Unlike the already implemented SPI terminal function, this does not rely on the host sending EM100 specific commands (that might be filtered out by the SPI controller or the ME)
Change-Id: I78162585bcd73a3bb3a8aa590f43a96092946109 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org --- M em100.c M em100.h M trace.c 3 files changed, 161 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/99/48599/4
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48599 )
Change subject: em100: Add SPI trace console ......................................................................
Patch Set 4: Code-Review+2
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/em100/+/48599
to look at the new patch set (#5).
Change subject: em100: Add SPI trace console ......................................................................
em100: Add SPI trace console
Add a command which allows a realtime console output from em100 when using the CONFIG_SPI_FLASH_CONSOLE option in coreboot.
Provide the offset and length of the region defined in the FMAP and it will watch for the host to write to that area, and when it sees a write it will print it to stdout.
Unlike the already implemented SPI terminal function, this does not rely on the host sending EM100 specific commands (that might be filtered out by the SPI controller or the ME)
Change-Id: I78162585bcd73a3bb3a8aa590f43a96092946109 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org --- M em100.c M em100.h M trace.c 3 files changed, 161 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/99/48599/5
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48599 )
Change subject: em100: Add SPI trace console ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/em100/+/48599/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/em100/+/48599/1//COMMIT_MSG@12 PS1, Line 12: lenght
length
Done
Stefan Reinauer has submitted this change. ( https://review.coreboot.org/c/em100/+/48599 )
Change subject: em100: Add SPI trace console ......................................................................
em100: Add SPI trace console
Add a command which allows a realtime console output from em100 when using the CONFIG_SPI_FLASH_CONSOLE option in coreboot.
Provide the offset and length of the region defined in the FMAP and it will watch for the host to write to that area, and when it sees a write it will print it to stdout.
Unlike the already implemented SPI terminal function, this does not rely on the host sending EM100 specific commands (that might be filtered out by the SPI controller or the ME)
Change-Id: I78162585bcd73a3bb3a8aa590f43a96092946109 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Reviewed-on: https://review.coreboot.org/c/em100/+/48599 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M em100.c M em100.h M trace.c 3 files changed, 161 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/em100.c b/em100.c index 4b0a121..f808ed6 100644 --- a/em100.c +++ b/em100.c @@ -787,6 +787,8 @@ {"list-devices", 0, 0, 'l'}, {"update-files", 0, 0, 'U'}, {"terminal", 0, 0, 'T'}, + {"traceconsole", 0, 0, 'R'}, + {"length", 1, 0, 'L'}, {"compatible", 0, 0, 'C'}, {NULL, 0, 0, 0} }; @@ -807,6 +809,8 @@ " -t|--trace: trace mode\n" " -O|--offset HEX_VAL: address offset for trace mode\n" " -T|--terminal: terminal mode\n" + " -R|--traceconsole: trace console mode\n" + " -L|--length HEX_VAL: length of buffer for traceconsole mode\n" " -F|--firmware-update FILE|auto: update EM100pro firmware (dangerous)\n" " -f|--firmware-dump FILE: export raw EM100pro firmware to file\n" " -g|--firmware-write FILE: export EM100pro firmware to DPFW file\n" @@ -832,18 +836,18 @@ const char *firmware_in = NULL, *firmware_out = NULL; const char *holdpin = NULL; int do_start = 0, do_stop = 0; - int verify = 0, trace = 0, terminal=0; + int verify = 0, trace = 0, terminal=0, traceconsole=0; int compatibility = 0; int bus = 0, device = 0; int firmware_is_dpfw = 0; unsigned int serial_number = 0; - unsigned long address_offset = 0; + unsigned long address_offset = 0, address_length = 0; unsigned int spi_start_address = 0; const char *voltage = NULL; struct sigaction signal_action; struct em100 *em100 = &em100_state;
- while ((opt = getopt_long(argc, argv, "c:d:a:m:u:rsvtO:F:f:g:S:V:p:DCx:lUhT", + while ((opt = getopt_long(argc, argv, "c:d:a:m:u:rsvtO:F:f:g:S:V:p:DCx:lUhTRL:", longopts, &idx)) != -1) { switch (opt) { case 'c': @@ -919,6 +923,12 @@ case 'U': update_all_files(); return 0; + case 'R': + traceconsole = 1; + break; + case 'L': + sscanf(optarg, "%lx", &address_length); + break; case 'C': compatibility = 1; break; @@ -1206,7 +1216,7 @@ set_state(em100, 1); }
- if (trace || terminal) { + if (trace || terminal || traceconsole) { int usb_errors = 0;
if ((holdpin == NULL) && (!set_hold_pin_state(em100, 3))) { @@ -1220,7 +1230,7 @@
printf ("Starting ");
- if (trace) { + if (trace || traceconsole) { reset_spi_trace(em100); printf("trace%s", terminal ? " & " : ""); } @@ -1234,13 +1244,18 @@
while (!exit_requested && usb_errors < MAX_USB_ERRORS) { int ret; - if (trace) { + + if (traceconsole) + ret = read_spi_trace_console(em100, address_offset, address_length); + else if (trace) ret = read_spi_trace(em100, terminal, address_offset); - } else { + else if (terminal) ret = read_spi_terminal(em100, 0); - } + if (ret == 0) usb_errors++; + if (ret < 0) + break; } if (usb_errors >= MAX_USB_ERRORS) printf("Error: Bailed out with too many USB errors.\n"); diff --git a/em100.h b/em100.h index ecb74d2..bf1bb87 100644 --- a/em100.h +++ b/em100.h @@ -172,6 +172,8 @@ unsigned long addr_offset); int read_spi_terminal(struct em100 *em100, int print_counter); int init_spi_terminal(struct em100 *em100); +int read_spi_trace_console(struct em100 *em100, unsigned long addr_offset, + unsigned long addr_len);
/* Archive handling */ typedef struct { diff --git a/trace.c b/trace.c index 2afdbad..df3fdd9 100644 --- a/trace.c +++ b/trace.c @@ -330,6 +330,142 @@ return 1; }
+int read_spi_trace_console(struct em100 *em100, unsigned long addr_offset, + unsigned long addr_len) +{ + unsigned char reportdata[REPORT_BUFFER_COUNT][REPORT_BUFFER_LENGTH] = + {{0}}; + unsigned char *data; + unsigned int count, i, report; + static int additional_pad_bytes = 0; + static unsigned int address = 0; + static int do_write = 0; + static struct spi_cmd_values *spi_cmd_vals = &spi_command_list[3]; + int addr_bytes = 0; + + if (addr_offset == 0) { + printf("Address offset for console buffer required\n"); + return -1; + } + if (addr_len == 0) { + printf("Console buffer length required\n"); + return -1; + } + + if (!read_report_buffer(em100, reportdata)) + return -1; + + for (report = 0; report < REPORT_BUFFER_COUNT; report++) { + data = &reportdata[report][0]; + count = (data[0] << 8) | data[1]; + if (!count) + continue; + if (count > 1023) { + printf("Warning: EM100pro sends too much data: %d.\n", count); + count = 1023; + } + for (i = 0; i < count; i++) { + unsigned int j = additional_pad_bytes; + unsigned int address_bytes = 0; + additional_pad_bytes = 0; + unsigned char cmd = data[2 + i*8]; + + /* from here, it must be data */ + if (cmd != cmdid) { + unsigned char spi_command = data[i * 8 + 4]; + spi_cmd_vals = get_command_vals(spi_command); + + /* new command */ + cmdid = cmd; + + /* Special commands */ + switch (spi_command) { + case 0xb7: + address_mode = 4; + break; + case 0xe9: + address_mode = 3; + break; + } + + /* skip command byte */ + j = 1; + + /* 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; + } + +#if 0 + if (spi_cmd_vals->address_type != ADDR_NONE) + printf("0x%02x @ 0x%08x (%s)\n", + spi_command, address, spi_cmd_vals->cmd_name); + else + printf("0x%02x (%s)\n", + spi_command, spi_cmd_vals->cmd_name); +#endif + + curpos = 0; + if (spi_command == 0x02) + do_write = 1; + else + do_write = 0; + } + + if (do_write == 0 || + (spi_cmd_vals->address_type == ADDR_NONE || + address < addr_offset || + address > (addr_offset + addr_len))) { + curpos = data[2 + i*8 + 1] + 0x10; + continue; + } + + /* this exploits 8bit wrap around in curpos */ + unsigned char blocklen = (data[2 + i*8 + 1] - curpos); + blocklen /= 8; + + for (; j < blocklen; j++) { + printf("%c", data[i * 8 + addr_bytes + j]); + } + + /* this is because the em100 counts funny */ + curpos = data[2 + i*8 + 1] + 0x10; + fflush(stdout); + } + } + return 1; +} + #define UFIFO_SIZE 512 #define UFIFO_TIMEOUT 0x00
-
Furquan Shaikh (Code Review) -
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)