Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(3 comments)
Tested this again on SKL with an artificial corner-case devicetree. Inconsistencies in `on` settings were correctly discovered, devicetree nodes correctly patched (tested by specifying individual SSIDs and where coreboot wanted to assign them later; actual assignments failed though, because the registers were already locked). Also, no complaints about spurious nodes during PCI enumeration.
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 37: root-port
Ok, looks like I was wrong; thanks! :) but then be consistent at least :P
AFAIK, I've consistently used it hyphenated when describing another noun and not hyphenated when used as a noun. Please let me know if you see any inconsistencies.
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 141: for (dev = *ptr; dev; dev = *ptr) { : if (dev->path.type != DEVICE_PATH_PCI) { : ptr = &dev->sibling; : continue; : } : : offset = 0; : for (group = groups; group->count; ++group) { : if (group->slot == PCI_SLOT(dev->path.pci.devfn)) : break; : offset += group->count; : } : if (!group->count) { : ptr = &dev->sibling; : continue; : } : : if (pcie_rp_update_dev(dev, offset, mapping) < 0) { : /* Unlink disabled device. */ : *ptr = dev->sibling; : dev->sibling = NULL; : } : }
Could be fixed as follows: […]
Rewrote the loop to make the (un)linking better visible. Let me know if it still worries you.
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 132: root-port
nit: root port
Nope, as discussed elsewhere.