Change in coreboot[master]: soc/intel/tigerlake: Configure L1Substates for PCH Root ports
Wonkyu Kim has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39412 )
Change subject: soc/intel/tigerlake: Configure L1Substates for PCH Root ports ......................................................................
soc/intel/tigerlake: Configure L1Substates for PCH Root ports
Set value for PcieRpL1Substates as the values are different in ES1(L1.1: 2) and ES2(MAX: 0 - FSP default or 4)
BUG=none BRANCH=none TEST=Boot up and check FSP log for PCIe config for this values
Signed-off-by: Wonkyu Kim wonkyu.kim@intel.com Change-Id: I66743a29ad182bd49b501ae73b79270a9eb88450 --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_tgl.c 2 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39412/1
diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h index a6bcf08..6a0d290 100644 --- a/src/soc/intel/tigerlake/chip.h +++ b/src/soc/intel/tigerlake/chip.h @@ -122,6 +122,11 @@ * clksrc. */ uint8_t PcieClkSrcClkReq[CONFIG_MAX_PCIE_CLOCKS];
+ /* PCIe RP L1 substate + * 0: FSP default(Max), 1: Disable, 2: L1.1 3: L1.2 4:Max + */ + uint8_t PcieRpL1Substates[CONFIG_MAX_ROOT_PORTS]; + /* SMBus */ uint8_t SmbusEnable;
diff --git a/src/soc/intel/tigerlake/fsp_params_tgl.c b/src/soc/intel/tigerlake/fsp_params_tgl.c index 0dae0fe..f53517d 100644 --- a/src/soc/intel/tigerlake/fsp_params_tgl.c +++ b/src/soc/intel/tigerlake/fsp_params_tgl.c @@ -113,6 +113,12 @@ } }
+ /* RP Configs */ + for (i = 0; i < CONFIG_MAX_ROOT_PORTS; i++) { + if (config->PcieRpL1Substates[i]) + params->PcieRpL1Substates[i] = config->PcieRpL1Substates[i] - 1; + } + /* Enable xDCI controller if enabled in devicetree and allowed */ dev = pcidev_on_root(PCH_DEV_SLOT_XHCI, 1); if (dev) {
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: Code-Review+1
Caveh Jalali 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: Code-Review+1
Srinidhi N Kaushik 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: Code-Review+2
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 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?
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?
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?
https://review.coreboot.org/c/coreboot/+/39412/1/src/soc/intel/tigerlake/fsp... PS1, Line 118: config->PcieRpL1Substates[i] Why is this check required?
https://review.coreboot.org/c/coreboot/+/39412/1/src/soc/intel/tigerlake/fsp... PS1, Line 119: - 1 Why -1?
Caveh Jalali 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:
(1 comment)
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 119: - 1
Why -1?
basically, this is an encoding scheme to avoid introducing a separate boolean override flag for this field. so, encoding 0 means no override. a value > 0 represents the desired override value + 1.
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 1:
(1 comment)
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 119: - 1
basically, this is an encoding scheme to avoid introducing a […]
What does 0 mean for FSP? The FSP headers that I am currently looking at says that 0 means no override, which aligns well with expectations here, but chip.h in this CL seems to say that 0 means max. So, I confused what the values really are.
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/...
Hello Raj Astekar, build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39412
to look at the new patch set (#2).
Change subject: soc/intel/tigerlake: Configure L1Substates for PCH Root ports ......................................................................
soc/intel/tigerlake: Configure L1Substates for PCH Root ports
Set value for PcieRpL1Substates according to devicetree.
BUG=none BRANCH=none TEST=Boot up and check FSP log for PCIe config for this values
Signed-off-by: Wonkyu Kim wonkyu.kim@intel.com Change-Id: I66743a29ad182bd49b501ae73b79270a9eb88450 --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_tgl.c 2 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39412/2
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; }
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.
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.
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: […]
Function should actually be:
int get_l1_substate_control(enum L1_substates_control ctl) { if ((ctl > L1_SS_L1_2) || (ctl == L1_SS_FSP_DEFAULT)) ctl = L1_SS_L1_2; /* .... Use same comment as above for enum .... */ return ctl - 1; }
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:
(1 comment)
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;
This is verified. if we don't assign, FSP use default values. […]
Ack
Hello Raj Astekar, build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39412
to look at the new patch set (#3).
Change subject: soc/intel/tigerlake: Configure L1Substates for PCH Root ports ......................................................................
soc/intel/tigerlake: Configure L1Substates for PCH Root ports
Set value for PcieRpL1Substates according to devicetree.
BUG=none BRANCH=none TEST=Boot up and check FSP log for PCIe config for this values
Signed-off-by: Wonkyu Kim wonkyu.kim@intel.com Change-Id: I66743a29ad182bd49b501ae73b79270a9eb88450 --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_tgl.c 2 files changed, 32 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39412/3
build bot (Jenkins) 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 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39412/3/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/39412/3/src/soc/intel/tigerlake/chi... PS3, Line 131: }PcieRpL1Substates[CONFIG_MAX_ROOT_PORTS]; space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/39412/3/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_tgl.c:
https://review.coreboot.org/c/coreboot/+/39412/3/src/soc/intel/tigerlake/fsp... PS3, Line 136: for (i = 0; i < CONFIG_MAX_ROOT_PORTS; i++) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/39412/3/src/soc/intel/tigerlake/fsp... PS3, Line 137: params->PcieRpL1Substates[i] = get_l1_substate_control(config->PcieRpL1Substates[i]); line over 96 characters
Hello Raj Astekar, build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39412
to look at the new patch set (#4).
Change subject: soc/intel/tigerlake: Configure L1Substates for PCH Root ports ......................................................................
soc/intel/tigerlake: Configure L1Substates for PCH Root ports
Set value for PcieRpL1Substates according to devicetree.
BUG=none BRANCH=none TEST=Boot up and check FSP log for PCIe config for this values
Signed-off-by: Wonkyu Kim wonkyu.kim@intel.com Change-Id: I66743a29ad182bd49b501ae73b79270a9eb88450 --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_tgl.c 2 files changed, 31 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39412/4
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 4: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/39412/3/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/39412/3/src/soc/intel/tigerlake/chi... PS3, Line 131: }PcieRpL1Substates[CONFIG_MAX_ROOT_PORTS];
space required after that close brace '}'
Ack
https://review.coreboot.org/c/coreboot/+/39412/3/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_tgl.c:
https://review.coreboot.org/c/coreboot/+/39412/3/src/soc/intel/tigerlake/fsp... PS3, Line 136: for (i = 0; i < CONFIG_MAX_ROOT_PORTS; i++) {
braces {} are not necessary for single statement blocks
Ack
https://review.coreboot.org/c/coreboot/+/39412/3/src/soc/intel/tigerlake/fsp... PS3, Line 137: params->PcieRpL1Substates[i] = get_l1_substate_control(config->PcieRpL1Substates[i]);
line over 96 characters
Ack
build bot (Jenkins) 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 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39412/4/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_tgl.c:
https://review.coreboot.org/c/coreboot/+/39412/4/src/soc/intel/tigerlake/fsp... PS4, Line 137: params->PcieRpL1Substates[i] = get_l1_substate_control(config->PcieRpL1Substates[i]); line over 96 characters
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 4:
(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
Function should actually be: […]
Done
Hello Raj Astekar, build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39412
to look at the new patch set (#5).
Change subject: soc/intel/tigerlake: Configure L1Substates for PCH Root ports ......................................................................
soc/intel/tigerlake: Configure L1Substates for PCH Root ports
Set value for PcieRpL1Substates according to devicetree.
BUG=none BRANCH=none TEST=Boot up and check FSP log for PCIe config for this values
Signed-off-by: Wonkyu Kim wonkyu.kim@intel.com Change-Id: I66743a29ad182bd49b501ae73b79270a9eb88450 --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_tgl.c 2 files changed, 31 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39412/5
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 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39412/4/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_tgl.c:
https://review.coreboot.org/c/coreboot/+/39412/4/src/soc/intel/tigerlake/fsp... PS4, Line 137: params->PcieRpL1Substates[i] = get_l1_substate_control(config->PcieRpL1Substates[i]);
line over 96 characters
Ack
build bot (Jenkins) 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 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39412/5/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_tgl.c:
https://review.coreboot.org/c/coreboot/+/39412/5/src/soc/intel/tigerlake/fsp... PS5, Line 137: params->PcieRpL1Substates[i] = get_l1_substate_control(config->PcieRpL1Substates[i]); line over 96 characters
Hello Raj Astekar, build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39412
to look at the new patch set (#6).
Change subject: soc/intel/tigerlake: Configure L1Substates for PCH Root ports ......................................................................
soc/intel/tigerlake: Configure L1Substates for PCH Root ports
Set value for PcieRpL1Substates according to devicetree.
BUG=none BRANCH=none TEST=Boot up and check FSP log for PCIe config for this values
Signed-off-by: Wonkyu Kim wonkyu.kim@intel.com Change-Id: I66743a29ad182bd49b501ae73b79270a9eb88450 --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_tgl.c 2 files changed, 32 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39412/6
build bot (Jenkins) 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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39412/6/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_tgl.c:
https://review.coreboot.org/c/coreboot/+/39412/6/src/soc/intel/tigerlake/fsp... PS6, Line 137: params->PcieRpL1Substates[i] = \ Avoid unnecessary line continuations
Hello Raj Astekar, build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39412
to look at the new patch set (#7).
Change subject: soc/intel/tigerlake: Configure L1Substates for PCH Root ports ......................................................................
soc/intel/tigerlake: Configure L1Substates for PCH Root ports
Set value for PcieRpL1Substates according to devicetree.
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.
Chip config parameter values 0: Use FSP UPD default 1: Disable L1 substates 2: Use L1.1 3: Use L1.2 (FSP UPD default)
BUG=none BRANCH=none TEST=Boot up and check FSP log for PCIe config for this values
Signed-off-by: Wonkyu Kim wonkyu.kim@intel.com Change-Id: I66743a29ad182bd49b501ae73b79270a9eb88450 --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_tgl.c 2 files changed, 32 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39412/7
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 7: Code-Review+1
Srinidhi N Kaushik 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 7: Code-Review+2
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 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39412/7/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_tgl.c:
https://review.coreboot.org/c/coreboot/+/39412/7/src/soc/intel/tigerlake/fsp... PS7, Line 137: \ shouldn't be required?
Hello Raj Astekar, build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39412
to look at the new patch set (#8).
Change subject: soc/intel/tigerlake: Configure L1Substates for PCH Root ports ......................................................................
soc/intel/tigerlake: Configure L1Substates for PCH Root ports
Set value for PcieRpL1Substates according to devicetree.
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.
Chip config parameter values 0: Use FSP UPD default 1: Disable L1 substates 2: Use L1.1 3: Use L1.2 (FSP UPD default)
BUG=none BRANCH=none TEST=Boot up and check FSP log for PCIe config for this values
Signed-off-by: Wonkyu Kim wonkyu.kim@intel.com Change-Id: I66743a29ad182bd49b501ae73b79270a9eb88450 --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_tgl.c 2 files changed, 32 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/39412/8
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 8: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39412/7/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_tgl.c:
https://review.coreboot.org/c/coreboot/+/39412/7/src/soc/intel/tigerlake/fsp... PS7, Line 137: \
shouldn't be required?
Ack
Caveh Jalali 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 8: Code-Review+1
Nick Vaccaro 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 8: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39412 )
Change subject: soc/intel/tigerlake: Configure L1Substates for PCH Root ports ......................................................................
soc/intel/tigerlake: Configure L1Substates for PCH Root ports
Set value for PcieRpL1Substates according to devicetree.
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.
Chip config parameter values 0: Use FSP UPD default 1: Disable L1 substates 2: Use L1.1 3: Use L1.2 (FSP UPD default)
BUG=none BRANCH=none TEST=Boot up and check FSP log for PCIe config for this values
Signed-off-by: Wonkyu Kim wonkyu.kim@intel.com Change-Id: I66743a29ad182bd49b501ae73b79270a9eb88450 Reviewed-on: https://review.coreboot.org/c/coreboot/+/39412 Reviewed-by: Caveh Jalali caveh@chromium.org Reviewed-by: Nick Vaccaro nvaccaro@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_tgl.c 2 files changed, 32 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Nick Vaccaro: Looks good to me, approved Caveh Jalali: Looks good to me, but someone else must approve Wonkyu Kim: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h index d23148a..9fc70f8 100644 --- a/src/soc/intel/tigerlake/chip.h +++ b/src/soc/intel/tigerlake/chip.h @@ -122,6 +122,14 @@ * clksrc. */ uint8_t PcieClkSrcClkReq[CONFIG_MAX_PCIE_CLOCKS];
+ /* PCIe RP L1 substate */ + enum L1_substates_control { + L1_SS_FSP_DEFAULT, + L1_SS_DISABLED, + L1_SS_L1_1, + L1_SS_L1_2, + } PcieRpL1Substates[CONFIG_MAX_ROOT_PORTS]; + /* SMBus */ uint8_t SmbusEnable;
diff --git a/src/soc/intel/tigerlake/fsp_params_tgl.c b/src/soc/intel/tigerlake/fsp_params_tgl.c index 14997c5..7230a4c 100644 --- a/src/soc/intel/tigerlake/fsp_params_tgl.c +++ b/src/soc/intel/tigerlake/fsp_params_tgl.c @@ -28,6 +28,25 @@ #include <soc/soc_chip.h> #include <string.h>
+/* + * 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. + * 0: Use FSP UPD default + * 1: Disable L1 substates + * 2: Use L1.1 + * 3: Use L1.2 (FSP UPD default) + */ +static int get_l1_substate_control(enum L1_substates_control ctl) +{ + if ((ctl > L1_SS_L1_2) || (ctl == L1_SS_FSP_DEFAULT)) + ctl = L1_SS_L1_2; + return ctl - 1; +} + static void parse_devicetree(FSP_S_CONFIG *params) { const struct soc_intel_tigerlake_config *config; @@ -113,6 +132,11 @@ } }
+ /* RP Configs */ + for (i = 0; i < CONFIG_MAX_ROOT_PORTS; i++) + params->PcieRpL1Substates[i] = + get_l1_substate_control(config->PcieRpL1Substates[i]); + /* Enable xDCI controller if enabled in devicetree and allowed */ dev = pcidev_on_root(PCH_DEV_SLOT_XHCI, 1); if (dev) {
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Caveh Jalali (Code Review)
-
Furquan Shaikh (Code Review)
-
Nick Vaccaro (Code Review)
-
Patrick Georgi (Code Review)
-
Srinidhi N Kaushik (Code Review)
-
Wonkyu Kim (Code Review)