Change in coreboot[master]: soc/intel/skylake: Configure ASPM and L1 substates for PCH root ports
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39538 )
Change subject: soc/intel/skylake: Configure ASPM and L1 substates for PCH root ports ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39538/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.h:
https://review.coreboot.org/c/coreboot/+/39538/14/src/soc/intel/skylake/chip... PS14, Line 286: PcieRpAspm
I see the issue. In your opinion, should they be renamed?
Sure, that's what I meant.
Regarding CamelCase, there doesn't seem to be an aversion to following FSP identifiers when it comes to UPDs.
Yes and it often turned out to be a bad idea. Sometimes the UPDs collide with some of coreboot's code or concepts. But the people adding their copies to `chip.h` don't know coreboot, and don't care if they create a shim around FSP instead of a proper coreboot.
In this case, though, I really just wanted to point out that you use the same name with different values. Actually, I don't expect anybody to fall into this trap. But everytime somebody does some- thing like this, it's another bad example in the tree.
-
Nico Huber (Code Review)