Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 16:
(11 comments)
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?
n/a
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... PS11, Line 54: * enumeration.
Looks like we map the final BDF to RP01..16 (don't ask me why 16, I don't know […]
Done
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 88: group->offset + 8
I hope the new version explains itself. Let me know if not.
n/a
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 27: It saves us a walk through the capabilites list per device.*/
^
Done
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 28: #define INTEL_RP_CLIST_XCAP 0x40
Individual PCIe capabilities aren't guaranteed to be at the same offset. […]
No guarantee, I know. But this is vendor specific code anyway. I think the assumption is fair and saves us a handful of PCI config reads per device.
Though, now that I have looked and found pci_s_find_capability() ;) At least I wouldn't have to write the code...
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 44: /* Read 1-based absolute port number: */
Could you expand this comment indicating the original port number is in this field of the capabiliti […]
Done
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 47: port_num
1-based comment above means it starts at 1 and counts up? Wouldn't this check be invalid because off […]
Seems correct to me,
if (port_num <= 8) bail out; if (port_num > 16) bail out;
works for 9 :)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 97: "Remapping PCIe Root Port #%u from %s to new function number %u.\n",
Is it helpful to have the old function number in this message?
Why not?
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 49: offset = 8, .count = 4
Well, I guess the count could be dropped if we assume to always have 8 ports per group... […]
Dropped `offset`
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 83: if (CONFIG(SKYLAKE_SOC_PCH_H)) : pcie_rp_update_devicetree(pch_h_rp_groups); : else : pcie_rp_update_devicetree(pch_lp_rp_groups); :
Not sure how that could be realized... […]
Hmmm, it's really just 5 additional PCI-config reads. But we'd always have to assume that the additional PCI-device numbers are not used for something other than RPs...
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. */
This feels like artificially blowing up the code... […]
n/a