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 DID ......................................................................
Patch Set 13:
(3 comments)
Current version implements long tables instead of C code that derives function numbers.
Originally, I thought the only negative impact would be on the diff stat. But after writing all the lines... I think it's more error-prone this way. Long, dull tables to write and to review. However, I'm undecided. If somebody promises to carefully review the tables, that should be fine :)
Let me know what you think. I've left some comments unresolved in case we want to go back to the C code.
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 72: { PCH_DEV_SLOT_PCIE, .offset = 0, .count = 8 },
For consistency can we please add '. […]
Done
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.
Done
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?
None