Attention is currently required from: Felix Singer, Nico Huber, Jeremy Soller. Tim Crawford has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49104 )
Change subject: soc/intel/cannonlake: Allow setting PCIe subsystem IDs after FSP SiliconInit ......................................................................
Patch Set 5:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49104/comment/b68b2fe9_89c79092 PS5, Line 14: the TigerLake FSP Integration Guide.
I couldn't find it, can you be more specific? […]
I will clarify the it to be the table override behavior in the message, and reference the section in the doc.
https://review.coreboot.org/c/coreboot/+/49104/comment/4b2b1e6d_3ee1e368 PS5, Line 23: Tested by checking lspci output on System76 galp3-c, oryp5, oryp6.
I can't find any of them in the tree. […]
These are WHL (galp3-c), CFL (oryp5, CB:47892), and CML (oryp6, CB:47768). I will specify the FSP in the message.
File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/49104/comment/3ad9a3f4_319b49d2 PS5, Line 552: 0
This looks like a valid address to me: offset 0 of PCI 0:00.0, or did I […]
You're right. The PCI ID database doesn't list anything using it, but it does appear to be valid.
https://review.coreboot.org/c/coreboot/+/49104/comment/e5fb590d_e11cf0e4 PS5, Line 561: dev = pcidev_path_on_root(PCH_DEVFN_XHCI); : if (dev) : pci_dev_set_subsystem(dev, dev->subsystem_vendor, : dev->subsystem_device); : : dev = pcidev_path_on_root(PCH_DEVFN_HDA); : if (dev) : pci_dev_set_subsystem(dev, dev->subsystem_vendor, : dev->subsystem_device);
Alternatively to a dummy entry, why not move this into the table?
Yes. Based on your other comments, I believe this is the better option. Moving them into the table would comply with what little documentation there is instead of relying on unknown behavior of the FSP.