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 10:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 21: unsigned int offset; It's not clear what these fields represent.
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 25: /* Zero-terminated list */ Zero in what field? Looks like it's 'slot'. I think we should be more explicit in the comments.
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 34: Could you please add a description?
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 32: < Shouldn't this be <= ? There are 8 functions possible, right?
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 79: 24 why 24?
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 88: group->offset + 8 Could you please explain this check?