Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39538 )
Change subject: soc/intel/skylake: Configure L1 substates for PCH root ports ......................................................................
Patch Set 25:
Do you think that L1 substates should be guarded by that Kconfig?
it wouldn't hurt to add 'CONFIG(PCIEXP_L1_SUB_STATE) &&' to the front of the if statement
That's not the config that I meant. As Nico said, coreboot can enable L1 substates if the FSP says that it's capable. We can use this UPD to get finer-grained control over which devices have L1SS enabled. This may be useful if, for example, only one devices misbehaves with L1SS. In other words, as I understand it, "PCIEXP_L1_SUB_STATE" depends on the FSP UPD (per root-port).
One dependency for L1 substates is clock PM
Should L1SS be guarded by PCIEXP_CLK_PM?
It's all a bit foggy as these Kconfigs don't directly reflect hardware state. PCIEXP_CLK_PM makes us enable a feature in the downstream device of a link. This needs a working clock request signal. Same with L1SS (but this also checks support in the upstream device). But L1SS doesn't depend on the other feature. What we'd probably need would be a Kconfig that says ASSUME_CLK_REQ_SIGNALS, that would be something to depend on. But I don't think it's worth the trouble. Also, would be much nicer to directly tag links in the devicetree to have clock request signals.
Now if FSP does the right thing if we say a port supports L1SS but no ASPM is a different story. But I don't expect any trouble if it doesn't filter the case, L1SS would just not work, I guess.
Btw. there won't be a +2 from me as long as there is the confusing CamelCase thing (same option name as FSP but with different encoding). But I'm not blocking it.