Attention is currently required from: Furquan Shaikh, Francois Toguo Fotso, Subrata Banik, Kane Chen, Patrick Rudolph, Karthik Ramasubramanian. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57684 )
Change subject: soc/intel/common: Add PMC Crashlog PCI driver ......................................................................
Patch Set 7:
(9 comments)
File src/soc/intel/common/block/crashlog/Kconfig:
https://review.coreboot.org/c/coreboot/+/57684/comment/960eed18_145b8be7 PS6, Line 4: Enables crashlog PCI drivers which collect crashlog data on boot,
I believe more than SoC, there should be a flexibility given to mainboards or platform users to choo […]
Done
https://review.coreboot.org/c/coreboot/+/57684/comment/2eb5c5ac_37aaf61a PS6, Line 8: SOC_INTEL_COMMON_BLOCK_CRASHLOG_PMC_TRIGGER_ON_RESET
WDYT about this name, do the Kconfig need to be explicit about whether PMC is the one who is trigger […]
Done
https://review.coreboot.org/c/coreboot/+/57684/comment/0c118ac6_9afcc8d9 PS6, Line 17: SOC_INTEL_COMMON_BLOCK_CRASHLOG_DISABLE
Ideally yes, knowing the this PCI device has two major purpose. […]
SG
File src/soc/intel/common/block/crashlog/pmc_crashlog.c:
https://review.coreboot.org/c/coreboot/+/57684/comment/ad8ad8ce_dbb2bfd1 PS6, Line 30: discovery_response
Why is this here and not in crashlog_lib. […]
Done
https://review.coreboot.org/c/coreboot/+/57684/comment/88c90bc4_821603c6 PS6, Line 53: bool crashlog_copy_pmc_records(void *dest, size_t size) : { : const struct cbmem_entry *entry; : : entry = cbmem_entry_find(CBMEM_ID_PMC_CRASHLOG); : if (!entry || !cbmem_entry_size(entry)) : return false; : : memcpy(dest, cbmem_entry_start(entry), MIN(size, cbmem_entry_size(entry))); : : return true; : }
Is this function really required? The caller is already making calls to `cbmem_entry_find()` and `cb […]
Ack
https://review.coreboot.org/c/coreboot/+/57684/comment/b6f42ec7_b5b34824 PS6, Line 72: if (res == NULL) : res = &placeholder;
Not for this CL, but looks like we have at least one more instance (cse_eop. […]
Ack
https://review.coreboot.org/c/coreboot/+/57684/comment/2f31cbd6_6f726632 PS6, Line 92: Note: The SRAM area is automatically cleared on G3 entry by the PMC
Is this note added to indicate that the crash log data would be empty when booting from G3?
yes
https://review.coreboot.org/c/coreboot/+/57684/comment/259b82f2_c883b17d PS6, Line 330: if (!is_dev_enabled(dev))
Maybe like this ? […]
Done
https://review.coreboot.org/c/coreboot/+/57684/comment/5d248623_8c9378f7 PS6, Line 330: if (!is_dev_enabled(dev)) : return;
`init` shouldn't be called if device is disabled. So, this check is redundant.
Done