Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 11:
(6 comments)
Sorry for the delay. Trying to catch up from being out of town. I think the algorithm is a lot clearer now. However, I did provide a suggestion that is completely data driven w/o the need for a callback.
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... PS11, Line 39: to their original function Presumably this is returning the original function number? Can we make that clearer?
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... PS11, Line 54: * enumeration. How do we handle the mapping to the ACPI device names?
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 51: case PCI_DEVICE_ID_INTEL_SPT_LP_PCIE_RP1: /* Fall through. */ I feel like we're being too slick here.
#define PCI_DEVICE_ID_INTEL_SPT_LP_PCIE_RP9>---->-------0x9d18
Masking off the lower nibble matches PCI_DEVICE_ID_INTEL_SPT_LP_PCIE_RP1. Can we just assume the compiler can optimize correctly if we blew out the switch statement to be more exhaustive?
Or if we passed in the slot number wouldn't that be sufficient?
Another idea:
Why encode these in code itself. We could just as easily add a array of 8 dids per pcie_rp_group. The index of the did match == original function number. And there's no callback required. We only need to encode the did array per group and generic code can handle everything else.
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 72: { PCH_DEV_SLOT_PCIE, .offset = 0, .count = 8 }, For consistency can we please add '.slot = ' ?
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 74: 0x00, 0, 0, I think you can just put '0' on its own.
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 92: const struct pcie_rp_mapper pch_h_rp_mapper = { Any reason the pcie_rp_mapper ad pcie_rp_group objects are global?