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 17:
(5 comments)
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 28: #define INTEL_RP_CLIST_XCAP 0x40
Understood it's vendor code. […]
I don't think the extra config reads hurt. So let's keep the pci_s_find_capability(). Now that it's written, I kind of like it.
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 47: port_num
That's fair. I think I was reading it as a positive as opposed to an error.
Ack
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",
I was meaning should we add it to the message.
Gotcha. The original function number is part of the `dev_path(dev)`, that is still unpatched.
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
There's only pci functions under a slot.
Right, 8 is the maximum per PCI device. It's not always 8, though. APL, for instance, has 4 + 2 groups.
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); : Well, we know that LP doesn't have 1b.*, so it's safe. One version is more explicit, the other saves us a few lines. I have no strong preference.
Hah! I found something, what about checking for "Port Type (DT)" in XCAP? That could be done for each first port per group for example
We already do that for every port. If it wasn't set to root port, it would we invalid to read LCAP :)