Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39412 )
Change subject: soc/intel/tigerlake: Configure L1Substates for PCH Root ports ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39412/1/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/39412/1/src/soc/intel/tigerlake/chi... PS1, Line 126: 4:Max Can you please update the comment here to make it clear:
Chip config parameter PcieRpL1Substates uses (UPD value + 1) because UPD value of 0 for PcieRpL1Substates means disabled for FSP. In order to ensure that mainboards that miss setting the chip config parameter do not incorrectly disable L1 substates, 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. 0: Use FSP UPD default 1: Disable L1 substates 2: Use L1.1 3: Use L1.2 (FSP UPD default)
And add an enum for the same: enum L1_substates_control { L1_SS_FSP_DEFAULT, L1_SS_DISABLED, L1_SS_L1_1, L1_SS_L1_2, };
And then you can add a helper function in fsp_params.c to get the required L1 substate value:
int get_l1_substate_control(enum L1_substates_control ctl) { if (ctl > L1_SS_L1_2) ctl = L1_SS_L1_2; /* .... Use same comment as above for enum .... */ return ctl - 1; }
FSP treats L1.2 and Max same
That is because FSP treats Max as end of enum and not really a valid L1 Substate control value.