Attention is currently required from: Jason Glenesk, Raul Rangel, Martin Roth, Marshall Dawson, Felix Held. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51750 )
Change subject: soc/amd/common: Add ESPI_DEBUG_PRINT macro & Kconfig option ......................................................................
Patch Set 3:
(3 comments)
Patchset:
PS3:
I want to add a bunch more messages with error reporting. This is just the start of that. […]
This sounds very weird. Beside one instance that is disabled by default, the `espi_util.c` doesn't contain any BIOS_DEBUG or BIOS_SPEW messages yet. And the idea of SPEW is to be overwhelming anyway:
* Log level for intricacies of a method. Messages might contain raw * data and will produce large logs. Developers should try to make sure * that this level is not useful to anyone besides developers.
Please don't be afraid to make use of all 8 log levels. I'd argue that only if these aren't enough (or there are other technical reasons like the concurrency of SMM), we should add additional debug options.
Also, you mentioned error reporting, how does that not belong in any log by default?
In any case, please add your reasoning to the commit message.
File src/soc/amd/common/block/include/amdblocks/espi.h:
https://review.coreboot.org/c/coreboot/+/51750/comment/01d0776a_5b2101af PS3, Line 13: #endif
I don't see how this is any different. […]
The idea is to let the compiler compile it (build test). The linker would remove it later. Alternatively, you can add files to configs/ that test all your debug code.
File src/soc/amd/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/51750/comment/987d897b_4512f973 PS3, Line 24: DEBUG_ESPI
Maybe this should be changed to ESPI_DEBUG, the same as the macro
I just noticed, ESPI_DEBUG exists already and is what you actually use in this patch. In its current state, ESPI_DEBUG is dead, though. No prompt, no user.