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 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39412/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39412/1//COMMIT_MSG@9 PS1, Line 9: as the values are different : in ES1(L1.1: 2) and ES2(MAX: 0 - FSP default or 4)
What does this mean?
For ES1 we need to limit L1.1 for avoiding NVMe reboot issue. (NVMe is not detected after warm reboot) while ES2 doesn't need to limit.
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
Why are both 0 and 4 max? and what do they refer to?
As Caveh explains, we're using +1 value in config variable so that we can use 0(not assigning value from device tree) for non-overriding the FSP UPD.
UPD value 0 mean disable. So if we use config variable 1, 0 will be assigned to FSP UPD. And using config variable 0 means not assigning UPD;use FSP default value(3).
For FSP info, refer below.
* FSP definition: https://chrome-internal.googlesource.com/chromeos/third_party/intel-fsp/tgl/...
typedef enum { PchPcieL1SubstatesDisabled, PchPcieL1SubstatesL1_1, PchPcieL1SubstatesL1_1_2, PchPcieL1SubstatesMax } PCH_PCIE_L1SUBSTATES_CONTROL
* FSP default value(3): https://chrome-internal.googlesource.com/chromeos/third_party/intel-fsp/tgl/...
FSP treats L1.2 and Max same as FSP only checking disable, L1.1 case. https://chrome-internal.googlesource.com/chromeos/third_party/intel-fsp/tgl/...
https://review.coreboot.org/c/coreboot/+/39412/1/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_tgl.c:
https://review.coreboot.org/c/coreboot/+/39412/1/src/soc/intel/tigerlake/fsp... PS1, Line 117: for
Can memcpy be used?
We need to use this condition to use 0 as FSP default value; not overriding PcieRpL1Substates
https://review.coreboot.org/c/coreboot/+/39412/1/src/soc/intel/tigerlake/fsp... PS1, Line 118: config->PcieRpL1Substates[i]
Why is this check required?
We need to use this condition to use 0 as FSP default value; not overriding PcieRpL1Substates
https://review.coreboot.org/c/coreboot/+/39412/1/src/soc/intel/tigerlake/fsp... PS1, Line 119: - 1
What does 0 mean for FSP? The FSP headers that I am currently looking at says that 0 means no overri […]
As Caveh explains, we're using +1 value in config variable so that we can use 0(not assigning value from device tree) for non-overriding the FSP UPD.
UPD value 0 mean disable. So if we use config variable 1, 0 will be assigned to FSP UPD. And using config variable 0 means not assigning UPD;use FSP default value(3).
For FSP info, refer below.
* FSP definition: https://chrome-internal.googlesource.com/chromeos/third_party/intel-fsp/tgl/...
typedef enum { PchPcieL1SubstatesDisabled, PchPcieL1SubstatesL1_1, PchPcieL1SubstatesL1_1_2, PchPcieL1SubstatesMax } PCH_PCIE_L1SUBSTATES_CONTROL
* FSP default value(3): https://chrome-internal.googlesource.com/chromeos/third_party/intel-fsp/tgl/...
FSP treats L1.2 and Max same as FSP only checking disable, L1.1 case. https://chrome-internal.googlesource.com/chromeos/third_party/intel-fsp/tgl/...