Wonkyu Kim 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:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39412/2/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/39412/2/src/soc/intel/tigerlake/chi... PS2, Line 126: 0: FSP default(Max), 1: Disable, 2: L1.1 3: L1.2 4:Max
Could this be an enum? It would make the devicetree options less confusing
Ack
https://review.coreboot.org/c/coreboot/+/39412/2/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_tgl.c:
https://review.coreboot.org/c/coreboot/+/39412/2/src/soc/intel/tigerlake/fsp... PS2, Line 118: if (config->PcieRpL1Substates[i]) : params->PcieRpL1Substates[i] = config->PcieRpL1Substates[i] - 1;
I don't know if this will work as intended. […]
This is verified. if we don't assign, FSP use default values. But for safe( in case new FSP change default value), it's better assign value. And I think it's better not use MAX(3) as it means same as 2(L1.2) for avoiding confusion. I'll update code to use enum value.