Attention is currently required from: Jason Glenesk, Raul Rangel, Nico Huber, Marshall Dawson, Paul Menzel, Julius Werner, Nikolai Vyssotski, Fred Reitberger, Felix Held. Alexey Buyanov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61311 )
Change subject: console: Add IO port serial logging ......................................................................
Patch Set 5:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/61311/comment/d9fdf710_dbdcf418 PS4, Line 9: IO ports can be used for debug data output as an : alternative for UART. Protocols used to output this data can : vary. The patch ports a custom version of such a protocol to : coreboot debug log printing routines. : Use cases may include UART problems bypassing or using of systems : without UART.
Thank you, will do.
Done
https://review.coreboot.org/c/coreboot/+/61311/comment/0ed2a1f2_d41805b6 PS4, Line 15:
Hi, Paul, this was tested by receiving data through port 80 and then interpreting it as per the prot […]
Done
Patchset:
PS5: Hello, please see my responds below and let me know if they are reasonable. Thanks, Alex.
File src/arch/x86/include/arch/io.h:
https://review.coreboot.org/c/coreboot/+/61311/comment/fc9434d2_efcefd42 PS5, Line 8: 0x5f535452ul
This is really a SIMNow sentinel value. […]
Ack
File src/arch/x86/post.c:
https://review.coreboot.org/c/coreboot/+/61311/comment/459e3675_27036252 PS5, Line 10: if (CONFIG(CONSOLE_AMD_IO_PORT)) : outl(AMD_IO_PORT_DATA_END, CONFIG_POST_IO_PORT);
Instead of modifying the common code, can we modify the simnow_tx_byte method to write the BEGIN byt […]
I started with this approach, but soon realized that not only performance affected (in current version of the patch simnow is on par with real HW), but also the logs are much harder to read as postcodes are getting mixed in. If sentinel values added per line and not per byte, then the logs are identical.
File src/console/Kconfig:
https://review.coreboot.org/c/coreboot/+/61311/comment/9aaeaa81_b1738d89 PS5, Line 489: config CONSOLE_AMD_IO_PORT
Why is that AMD specific? (If so, it needs to be clarified in the commit message. […]
The sentinel values are AMD specific, I will correct the commit message.
File src/console/printk.c:
https://review.coreboot.org/c/coreboot/+/61311/comment/fadb3b33_bbd267c0 PS4, Line 112: outl(AMD_IO_PORT_DATA_BEGIN, CONFIG_POST_IO_PORT);
Hi, Nico and Julius, thank you very much for reviewing. […]
Done
File src/include/console/io_port.h:
PS5:
Can we rename this to simnow. […]
The feature is not Simnow specific, it can be used with real HW. The io_port.h is to have one more way to output serial logs, this part is not AMD specific, community may also use this and adjust the code so they use io_port.h too.
File src/soc/amd/sabrina/bootblock.c:
https://review.coreboot.org/c/coreboot/+/61311/comment/eb90c714_3a5790d9 PS5, Line 39: if (CONFIG(CONSOLE_AMD_IO_PORT)) : outl(AMD_IO_PORT_DATA_BEGIN, CONFIG_POST_IO_PORT);
This should be done as part of the console_init
Ack