Benjamin Doron 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 11:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39538/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39538/6//COMMIT_MSG@9 PS6, Line 9: Port of CB:39412 for Skylake.
Port commit 84b4882b (soc/intel/tigerlake: Configure L1Substates for PCH Root ports), CB:39412) to S […]
Done
https://review.coreboot.org/c/coreboot/+/39538/6//COMMIT_MSG@10 PS6, Line 10: to devicetree to allow boards to set these options
Please add a dot/period at the end of sentences please.
Done
https://review.coreboot.org/c/coreboot/+/39538/6//COMMIT_MSG@12 PS6, Line 12: Chip config parameter PcieRpL1Substates uses (UPD value + 1) : because UPD value of 0 for PcieRpL1Substates means disabled for FSP. : In order to ensure that mainboard setting does not disable L1 substates : incorrectly, chip config parameter values are offset by 1 with 0 meaning : use FSP UPD default.
Please re-flow for 72 characters.
Done
https://review.coreboot.org/c/coreboot/+/39538/6/src/soc/intel/skylake/chip.... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/39538/6/src/soc/intel/skylake/chip.... PS6, Line 135: * Chip config parameter PcieRpL1Substates uses (UPD value + 1) : * because UPD value of 0 for PcieRpL1Substates means disabled for FSP. : * In order to ensure that mainboard setting does not disable L1 substates : * incorrectly, chip config parameter values are offset by 1 with 0 meaning : * use FSP UPD default. get_l1_substate_control() ensures that the right UPD : * value is set in fsp_params.
Please re-flow for 96 characters.
Done
https://review.coreboot.org/c/coreboot/+/39538/6/src/soc/intel/skylake/chip.... PS6, Line 224: get_l1_substate_control(config->PcieRpL1Substates[i]);
Why not combine both loops?
Done