Attention is currently required from: Paul Menzel, Tim Wawrzynczak. Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52454 )
Change subject: soc/intel/alderlake: Add CrashLog implementation for Intel ADL ......................................................................
Patch Set 5:
(16 comments)
This change is ready for review.
Commit Message:
https://review.coreboot.org/c/coreboot/+/52454/comment/69757d1e_b79868db PS3, Line 9: This enables CrashLog for Intel ADL based platform.
What is different from the other generations that so much new code has to be added? What changed?
These are ADL specific changes. Changes in soc/intel/.. cannot be shared between different SOC. Only the ones in common can, and there are no changes in the common code here.
https://review.coreboot.org/c/coreboot/+/52454/comment/c361e90f_d62368b6 PS3, Line 12: sucessfully
successfully
Could you please explain what the issue is here?
https://review.coreboot.org/c/coreboot/+/52454/comment/e0766ea0_e5a898fd PS3, Line 12: CrashLog data generated, extracted, processed
Is it documented somewhere, how to do that?
Yes, there is a User guide uploaded i a bug number mentioned in the commit message
https://review.coreboot.org/c/coreboot/+/52454/comment/1335ec43_41cb105b PS3, Line 13:
Please paste the new log messages added in this commit.
cpu discovery table offset: 0x6030.
Above is the newly added log message. But it is not clear to me why it needs to be added to the commit message, since this CL is not about adding a new log message but rather enabling/implementing the feature.
File src/soc/intel/alderlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/52454/comment/e6e5bd19_a8889d1a PS3, Line 302: select ACPI_BERT
Why is that not selected in the common code?
Because the common code applies to all SOCs, and this feature is not implemented and enabled on all intel SOC.
File src/soc/intel/alderlake/crashlog.c:
https://review.coreboot.org/c/coreboot/+/52454/comment/ac4be17a_e6f3f840 PS3, Line 55: BIOS_DEBUG
Why not level INFO?
Done
https://review.coreboot.org/c/coreboot/+/52454/comment/9d81d0c0_808cc046 PS3, Line 77: mode :
Please remove the space before the colon.
Done
https://review.coreboot.org/c/coreboot/+/52454/comment/c00867bd_6d3cec75 PS3, Line 77: crashLog
Please use consistent spelling. Above all lowercase is used, some where else it’s CrashLog.
Done
https://review.coreboot.org/c/coreboot/+/52454/comment/d00e0de3_8f310fba PS3, Line 106: printk(BIOS_ERR, "Invalid TEL_CFG_BAR value %d:\n",
Why a colon in the end? What is printed afterward? As it’s an error message please add the consequen […]
Done
https://review.coreboot.org/c/coreboot/+/52454/comment/9dce1e82_6114a154 PS3, Line 121:
Please remove the blank line.
Done
https://review.coreboot.org/c/coreboot/+/52454/comment/bb6363f3_3a99825e PS3, Line 123: printk(BIOS_ERR, "PMC SSRAM PCI device is disabled.\n");
Please add how it can be enabled.
Done
https://review.coreboot.org/c/coreboot/+/52454/comment/cc12ed21_4cfb4370 PS3, Line 146: printk(BIOS_DEBUG, "Read invalid pcie_cap_id value: : 0x%x\n",
Is the second colon needed?
Done
https://review.coreboot.org/c/coreboot/+/52454/comment/66cfbe18_6081a25a PS3, Line 186: i < cpu_cl_disc_tab.header.fields.count ;
Only space after <, and no space before ;.
Done
https://review.coreboot.org/c/coreboot/+/52454/comment/30a25ab6_9f989175 PS3, Line 191: printk(BIOS_DEBUG, "cpu_crashlog_discovery_table buffer: 0x%x size:"
Please add a space after the last :.
Done
https://review.coreboot.org/c/coreboot/+/52454/comment/3f8cae66_98585272 PS3, Line 207: capability not
Only one space.
Done
File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/52454/comment/3712828a_0bab0fab PS3, Line 211: crashLog
CrashLog
Done