Attention is currently required from: Francois Toguo Fotso, Martin Roth, Tim Wawrzynczak, Patrick Rudolph. Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49943 )
Change subject: This change implements CrashLog for intel TGL. ......................................................................
Patch Set 3:
(10 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49943/comment/df2ea118_18caa08a PS3, Line 7: This change implements CrashLog for intel TGL. soc/intel/tigerlake: Add CrashLog reporting
Patchset:
PS3: First review pass, mostly stylistic remarks. The interface to the broader coreboot codebase seems reasonable. I didn't do a deep dive on the internals of the actual crashlog fetching because I'm entirely unfamiliar with tigerlake.
Tim, any comments? Anybody else who should review this patch train?
File src/soc/intel/tigerlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/49943/comment/7cfa0a59_5bf0866b PS3, Line 417: printk(BIOS_DEBUG, "Crashlog is disabled, Skipping it.\n"); Don't think we need this?
https://review.coreboot.org/c/coreboot/+/49943/comment/21b2b068_768ec95a PS3, Line 422: printk(BIOS_DEBUG, "Crashlog discovery result: crashlog not found\n"); maybe make this BIOS_SPEW? From looking at the rest of the commit, having no crashlog seems perfectly reasonable (e.g. after G3)
https://review.coreboot.org/c/coreboot/+/49943/comment/7f0de374_1d1b6a13 PS3, Line 428: dicovery "discovery". Also, differ from what: the reported size here? In that case, maybe "discovery tables can be larger than the actual valid collected data"?
File src/soc/intel/tigerlake/crashlog_lib.c:
https://review.coreboot.org/c/coreboot/+/49943/comment/561c2894_cf60fd40 PS3, Line 80: extra whitespace (also at least 4 times more in this file)
https://review.coreboot.org/c/coreboot/+/49943/comment/d50e90c4_ee7ebaf2 PS3, Line 379: extraneous space
https://review.coreboot.org/c/coreboot/+/49943/comment/12001f5c_bafcb7bc PS3, Line 428: * we usually leave a space between /* and the comment.
File src/soc/intel/tigerlake/include/soc/crashlog_def.h:
https://review.coreboot.org/c/coreboot/+/49943/comment/da5b6174_21b2231c PS3, Line 1: GPL-2.0-only The license text below says "or any later version". In any case, we prefer to have only SPDX identifiers in source files, and record the authorship/copyright through git history and AUTHORS file (where Intel is already listed).
https://review.coreboot.org/c/coreboot/+/49943/comment/b741555f_cee6098a PS3, Line 30: #define BIT_31_SET 0x80000000 some places across the code base call this BIT31, and we also have a BIT(x) macro in types.h. Pick one of those please
-
Patrick Georgi (Code Review)