Angel Pons 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)
I don't think Tiger Lake FSP is released yet, so it would be nice if the UPD definition could be changed.
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
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. Don't we zero out structs in coreboot? If so, the default value of `params->PcieRpL1Substates[i]` will not be 3, but 0 instead...
If one would want the default value to be 3 instead of 0, I would suggest writing it explicitly instead of subtracting:
for (i = 0; i < CONFIG_MAX_ROOT_PORTS; i++) { /* FSP uses zero as Disable. It's not good for a default value, though */ if (config->PcieRpL1Substates[i]) /* Use non-zero values for explicit choices */ params->PcieRpL1Substates[i] = config->PcieRpL1Substates[i] - 1; else /* For the default value of zero, fall back to a reasonable setting */ params->PcieRpL1Substates[i] = 3; }