Attention is currently required from: Erik van den Bogaert, Frans Hendriks, Jan Samek, Jonathon Hall, Michał Żygowski, Nico Huber, Piotr Król.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79958?usp=email )
Change subject: intel skl mainboards: Move PcieRpEnable option below dt entries ......................................................................
Patch Set 6:
(2 comments)
Patchset:
PS6:
Making the PcieRpEnable entries more readable by moving them under the devices to be later removed is a no-op.
No, it's not. "Style changes" are not my concern as well. Please read my commit message again:
*Move the PcieRpEnable option below their related devicetree entries in order to make the review easier when the option is removed.* **Create devicetree entries in case of they don't exist yet.**
This patch fixes the current state of some deviceetrees and it allows CB:79917 (and follows) to be a more or less no-op or to be a style change as it can do a smooth switch over. CB:79917 only removes settings but by doing that it actually changes the state of some mainboards since necessary devicetree entries don't exist. So some PCIe root ports are disabled while they are supposed to be enabled. That wasn't visible to the authors, obviously, which confirms that an intermediate step makes things easier. I've already mentioned that issue in the comments there.
The missing devicetree entries only became visible to me after moving the settings into the devicetree, which is why this patch exists. On the other hand, some root ports are enabled in the devicetree even if the PcieRpEnable setting is not set. So with CB:79917 more root ports are enabled than they should afterwards. Though, enabling more shouldn't break anything I guess.
However, I'm not concerned about merge conflicts or anything. The PcieRpEnable settings can be just removed with a bash one-liner and thus it's easy to update CB:79917 and follows.
File src/mainboard/51nb/x210/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/79958/comment/07c9ffd9_97f5e2bb : PS6, Line 59: register "PcieRpEnable[2]" = "1" # Ethernet controller
This will just create a merge conflict with CB:79917 because e.g. […]
See my other comment. This is not about style changes. Marking this as resolved and the other one as unresolved.