Attention is currently required from: Francois Toguo Fotso, Tim Wawrzynczak, Karthik Ramasubramanian. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57785 )
Change subject: soc/intel/block: Add small crashlog helper library ......................................................................
Patch Set 3:
(9 comments)
File src/soc/intel/common/block/crashlog/crashlog_lib.h:
https://review.coreboot.org/c/coreboot/+/57785/comment/b605a412_822e2578 PS3, Line 16: 0x04 Curious why this is 0x04 v/s 0x4 for DVSEC_VSEC0_OFFSET? Is it because the other field is 8-bit and this is 16-bit?
https://review.coreboot.org/c/coreboot/+/57785/comment/6b773ece_8c7e83ca PS3, Line 41: union discovery_info { Some of the structures/unions seem to be unused in this CL. I am guessing this is primarily for setting up the library so that they can be used in follow-up CLs. Don't you need some helper functions to set these up though?
https://review.coreboot.org/c/coreboot/+/57785/comment/7324ffca_5a33ab7c PS3, Line 89: version_id Are the definitions of vsec1 and vsec2 dependent on version_id? Also, header size?
https://review.coreboot.org/c/coreboot/+/57785/comment/af2cdca1_264cdeb4 PS3, Line 97: discovery_type Is this discovery_type or is this dvsec_id?
https://review.coreboot.org/c/coreboot/+/57785/comment/d907ef9e_d2814f5c PS3, Line 130: struct const struct
https://review.coreboot.org/c/coreboot/+/57785/comment/f2c37de4_f622d2a8 PS3, Line 131: struct const struct
https://review.coreboot.org/c/coreboot/+/57785/comment/06ae2397_7d17e28a PS3, Line 132: offset offset from start of SRAM?
File src/soc/intel/common/block/crashlog/crashlog_lib.c:
https://review.coreboot.org/c/coreboot/+/57785/comment/e221fe45_c1bebcfd PS3, Line 24: pos = pciexp_find_extended_cap(dev, PCIE_EXT_CAP_DVSEC_ID); nit: Can be rearranged to skip writing this twice:
``` pos = 0;
while (1) { pos = pciexp_find_next_extended_cap(dev, PCIE_EXT_CAP_DVSEC_ID, pos); if (!pos) break;
... } ```
This is under the assumption that `pciexp_find_next_extended_cap()` will be updated to handle 0 as special case.
https://review.coreboot.org/c/coreboot/+/57785/comment/9c4fcfff_090b0d3b PS3, Line 37: fill_crashlog_table_from_dvsec Don't you need similar fill_crashlog_table_* functions for other discovery mechanisms as well?