Attention is currently required from: Francois Toguo Fotso, Tim Wawrzynczak, Nick Vaccaro, Kane Chen, Karthik Ramasubramanian. Furquan Shaikh 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 6:
(9 comments)
File src/soc/intel/common/block/crashlog/Kconfig:
https://review.coreboot.org/c/coreboot/+/57684/comment/66131da2_d2643a03 PS6, Line 4: Enables crashlog PCI drivers which collect crashlog data on boot, Thinking out loud: Should `SOC_INTEL_COMMON_BLOCK_CRASHLOG` be selected by SoCs that support the crashlog block and the device on/off status be actually used to decide if crash collection needs to be enabled or not?
https://review.coreboot.org/c/coreboot/+/57684/comment/236c6650_b78d025c PS6, Line 17: SOC_INTEL_COMMON_BLOCK_CRASHLOG_DISABLE Shouldn't this be done based on dev->enabled status set to `off` in ->enable() callback? Why is a separate Kconfig required?
File src/soc/intel/common/block/crashlog/pmc_crashlog.c:
https://review.coreboot.org/c/coreboot/+/57684/comment/e215deff_b03a78ce PS1, Line 248: en_gen_on_all_reboot
It does appear to be required before collection happens, or else the crashlog flow doesn't work as e […]
That is really weird. Is that expected as per Intel docs?
File src/soc/intel/common/block/crashlog/pmc_crashlog.c:
https://review.coreboot.org/c/coreboot/+/57684/comment/353923f3_5b675606 PS6, Line 30: discovery_response Why is this here and not in crashlog_lib.h? I am guessing this is the response for discovery mechanism: CRASHLOG_DISCOVERY_DESCRIPTOR?
https://review.coreboot.org/c/coreboot/+/57684/comment/6c2601c8_8a12a94a 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 `cbmem_entry_size()` for other things. Can it not do the memcpy based on the information it has already collected? Same for crashlog_copy_cpu_records() in follow-up CL.
https://review.coreboot.org/c/coreboot/+/57684/comment/a4ee2574_8bbe78b4 PS6, Line 72: if (res == NULL) : res = &placeholder; Not for this CL, but looks like we have at least one more instance (cse_eop.c) where we are supplying a dummy/placeholder buffer for reading when the caller doesn't care about the response. Probably better to move this logic to `pmc_send_ipc_cmd()` so that it can drop the reads if caller passes `rbuf` as `NULL`.
https://review.coreboot.org/c/coreboot/+/57684/comment/7ed62046_5579734f 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?
https://review.coreboot.org/c/coreboot/+/57684/comment/1532cc17_05c7acd7 PS6, Line 146: static bool fill_info_from_type(struct device *dev, I am curious about the split of crash log library and the device driver. These functions to parse the info based on discovery mechanism seem like good candidates for the crash log library.
https://review.coreboot.org/c/coreboot/+/57684/comment/85fe67b7_d5afa63d PS6, Line 330: if (!is_dev_enabled(dev)) : return; `init` shouldn't be called if device is disabled. So, this check is redundant.