Change in coreboot[master]: soc/intel/cannonlake: FSP UPDs settings are wrong when GBE is disabled
Kane Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35595 )
Change subject: soc/intel/cannonlake: FSP UPDs settings are wrong when GBE is disabled ......................................................................
soc/intel/cannonlake: FSP UPDs settings are wrong when GBE is disabled
The previous code actually set SlpS0WithGbeSupport even GBE is disabled in device tree.
This change will config PchPmSlpS0VmRuntimeControl, PchPmSlpS0Vm070VSupport, PchPmSlpS0Vm075VSupport by device tree.
SlpS0WithGbeSupport will be set only when s0ix and gbe are enabled.
BUG=b:134092071 TEST=Run suspend_stress_test on kohaku and pass 100 cycles
Change-Id: I154a4e6970f99360aeb880d576eb61528034f7b6 Signed-off-by: Kane Chen kane.chen@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/35595/1
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index 76d40aa..e500a75 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -193,13 +193,19 @@ #endif }
+ params->SlpS0WithGbeSupport = 0; + params->PchPmSlpS0VmRuntimeControl = config->PchPmSlpS0VmRuntimeControl; + params->PchPmSlpS0Vm070VSupport = config->PchPmSlpS0Vm070VSupport; + params->PchPmSlpS0Vm075VSupport = config->PchPmSlpS0Vm075VSupport; + /* Lan */ dev = pcidev_path_on_root(PCH_DEVFN_GBE); if (!dev) params->PchLanEnable = 0; else { params->PchLanEnable = dev->enabled; - if (config->s0ix_enable) { + /* Override the settings when GBE and s0ix are enabled */ + if (config->s0ix_enable && params->PchLanEnable) { params->SlpS0WithGbeSupport = 1; params->PchPmSlpS0VmRuntimeControl = 0; params->PchPmSlpS0Vm070VSupport = 0;
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35595 )
Change subject: soc/intel/cannonlake: FSP UPDs settings are wrong when GBE is disabled ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35595/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35595/1//COMMIT_MSG@7 PS1, Line 7: soc/intel/cannonlake: FSP UPDs settings are wrong when GBE is disabled Please make this a statement about the change, and not the problem.
Fix FSP UPDs with disabled GBE
https://review.coreboot.org/c/coreboot/+/35595/1//COMMIT_MSG@9 PS1, Line 9: even even when?
https://review.coreboot.org/c/coreboot/+/35595/1//COMMIT_MSG@10 PS1, Line 10: device tree. What affects does this have? Does something not work?
https://review.coreboot.org/c/coreboot/+/35595/1/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/35595/1/src/soc/intel/cannonlake/fs... PS1, Line 207: /* Override the settings when GBE and s0ix are enabled */ I believe the comment is not necessary, as the code is self-explanatory. If you want to keep it, please order the conditions in the comment to the if statement condition.
Hello Patrick Rudolph, Subrata Banik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35595
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE
The previous code actually set SlpS0WithGbeSupport even when GBE is disabled in device tree and could cause power consumption in s0ix.
This change will config PchPmSlpS0VmRuntimeControl, PchPmSlpS0Vm070VSupport, PchPmSlpS0Vm075VSupport by device tree.
SlpS0WithGbeSupport will be set only when s0ix and gbe are enabled.
BUG=b:134092071 TEST=Run suspend_stress_test on kohaku and pass 100 cycles
Change-Id: I154a4e6970f99360aeb880d576eb61528034f7b6 Signed-off-by: Kane Chen kane.chen@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 6 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/35595/2
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35595 )
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35595/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35595/1//COMMIT_MSG@7 PS1, Line 7: soc/intel/cannonlake: FSP UPDs settings are wrong when GBE is disabled
Please make this a statement about the change, and not the problem. […]
Done
https://review.coreboot.org/c/coreboot/+/35595/1//COMMIT_MSG@9 PS1, Line 9: even
even when?
Done
https://review.coreboot.org/c/coreboot/+/35595/1//COMMIT_MSG@10 PS1, Line 10: device tree.
What affects does this have? Does something not work?
Done
https://review.coreboot.org/c/coreboot/+/35595/1/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/35595/1/src/soc/intel/cannonlake/fs... PS1, Line 207: /* Override the settings when GBE and s0ix are enabled */
I believe the comment is not necessary, as the code is self-explanatory. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35595 )
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... PS2, Line 196: SlpS0WithGbeSupport Isn't the default for this 0?
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... PS2, Line 207: && params->PchLanEnable Just checking this should have been sufficient to not set SlpS0WithGbeSupport, right? Do you still need the lines 196-199?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35595 )
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... PS2, Line 207: && params->PchLanEnable
Just checking this should have been sufficient to not set SlpS0WithGbeSupport, right? Do you still n […]
Aah I see in a follow up CL, you are actually using the Vm settings. I think I had this question before but never got a good answer.
How is Voltage margining tied to GbE? Is it that if you have GbE enabled, you need to ignore its state in S0ix and hence disable any kind of voltage margining? Otherwise, you might enable some voltage margining?
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35595 )
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... PS2, Line 207: && params->PchLanEnable
Aah I see in a follow up CL, you are actually using the Vm settings. […]
Hi Furquan, I think it's due to PchPmSlpS0VmSupports UPDs will enable some clk gating settings in CPPM hence it would make slps0 gbe not working.
The PchPmSlpS0VmSupport UPDs are used to tell FSP that HW design can support VCCPRIM_CORE Low Voltage Mode so that certain power gating settings can be enabled.
For more details, we can go back to issue tracker to discussed
Thanks
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35595 )
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... PS2, Line 207: && params->PchLanEnable
For more details, we can go back to issue tracker to discussed
Can you please raise a bug and put details about when the settings need to be done and what advantage you are seeing with it?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35595 )
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
Patch Set 2:
(2 comments)
I have reviewed this CL again and posted couple of comments. Can you please take a look?
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... PS2, Line 196: SlpS0WithGbeSupport
Isn't the default for this 0?
This line should not be required, right?
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... PS2, Line 207: && params->PchLanEnable
For more details, we can go back to issue tracker to discussed […]
So, as per discussion it looks like: 1. VmControl needs to be set as per board design to allow voltage margining in S0ix to lower power consumption. 2. But, if GbE is enabled, voltage magining cannot be enabled and so the Vm control UPDs need to be set to 0.
Can you please add the comment here indicating this?
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35595 )
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... PS2, Line 196: SlpS0WithGbeSupport
This line should not be required, right?
Hi Fuquan, I think the default of this UPD is 1 (enable)
# !BSF NAME:{SlpS0WithGbeSupport} TYPE:{Combo} OPTION:{$EN_DIS} # !BSF HELP:{Enable/Disable SLP_S0 with GBE Support. Default is 0 for PCH-LP, WHL V0 Stepping CPU and 1 for PCH-H Series. 0: Disable, 1: Enable} gPlatformFspPkgTokenSpaceGuid.SlpS0WithGbeSupport | * | 0x01 | 0x01
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... PS2, Line 207: && params->PchLanEnable
So, as per discussion it looks like: […]
ok, i will add comment here. thanks.
Hello Patrick Rudolph, Subrata Banik, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35595
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE
The previous code actually set SlpS0WithGbeSupport even when GBE is disabled in device tree and could cause power consumption in s0ix.
This change will config PchPmSlpS0VmRuntimeControl, PchPmSlpS0Vm070VSupport, PchPmSlpS0Vm075VSupport by device tree.
SlpS0WithGbeSupport will be set only when s0ix and gbe are enabled.
BUG=b:134092071 TEST=Run suspend_stress_test on kohaku and pass 100 cycles
Change-Id: I154a4e6970f99360aeb880d576eb61528034f7b6 Signed-off-by: Kane Chen kane.chen@intel.com --- M 3rdparty/blobs M 3rdparty/intel-microcode M src/soc/intel/cannonlake/fsp_params.c 3 files changed, 14 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/35595/3
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35595 )
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35595/3/3rdparty/intel-microcode File 3rdparty/intel-microcode:
https://review.coreboot.org/c/coreboot/+/35595/3/3rdparty/intel-microcode@1 PS3, Line 1: mmit ee319ae7bc59e88b60142f40a9ec1b46656de4db sorry, i will fix it.
Hello Patrick Rudolph, Subrata Banik, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35595
to look at the new patch set (#4).
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE
The previous code actually set SlpS0WithGbeSupport even when GBE is disabled in device tree and could cause power consumption in s0ix.
This change will config PchPmSlpS0VmRuntimeControl, PchPmSlpS0Vm070VSupport, PchPmSlpS0Vm075VSupport by device tree.
SlpS0WithGbeSupport will be set only when s0ix and gbe are enabled.
BUG=b:134092071 TEST=Run suspend_stress_test on kohaku and pass 100 cycles
Change-Id: I154a4e6970f99360aeb880d576eb61528034f7b6 Signed-off-by: Kane Chen kane.chen@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 12 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/35595/4
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35595 )
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
Patch Set 4: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35595 )
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35595/3/3rdparty/intel-microcode File 3rdparty/intel-microcode:
https://review.coreboot.org/c/coreboot/+/35595/3/3rdparty/intel-microcode@1 PS3, Line 1: mmit ee319ae7bc59e88b60142f40a9ec1b46656de4db
sorry, i will fix it.
Done
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... PS2, Line 196: SlpS0WithGbeSupport
Hi Fuquan, […]
Ack
https://review.coreboot.org/c/coreboot/+/35595/2/src/soc/intel/cannonlake/fs... PS2, Line 207: && params->PchLanEnable
ok, i will add comment here. […]
Done
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35595 )
Change subject: soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE ......................................................................
soc/intel/cannonlake: Fix FSP UPDs settings with disabled GBE
The previous code actually set SlpS0WithGbeSupport even when GBE is disabled in device tree and could cause power consumption in s0ix.
This change will config PchPmSlpS0VmRuntimeControl, PchPmSlpS0Vm070VSupport, PchPmSlpS0Vm075VSupport by device tree.
SlpS0WithGbeSupport will be set only when s0ix and gbe are enabled.
BUG=b:134092071 TEST=Run suspend_stress_test on kohaku and pass 100 cycles
Change-Id: I154a4e6970f99360aeb880d576eb61528034f7b6 Signed-off-by: Kane Chen kane.chen@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35595 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 12 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index b580620..74884fd 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -192,6 +192,10 @@ sizeof(params->SataPortsDevSlpResetConfig)); #endif } + params->SlpS0WithGbeSupport = 0; + params->PchPmSlpS0VmRuntimeControl = config->PchPmSlpS0VmRuntimeControl; + params->PchPmSlpS0Vm070VSupport = config->PchPmSlpS0Vm070VSupport; + params->PchPmSlpS0Vm075VSupport = config->PchPmSlpS0Vm075VSupport;
/* Lan */ dev = pcidev_path_on_root(PCH_DEVFN_GBE); @@ -199,7 +203,14 @@ params->PchLanEnable = 0; else { params->PchLanEnable = dev->enabled; - if (config->s0ix_enable) { + if (config->s0ix_enable && params->PchLanEnable) { + /* + * The VmControl UPDs need to be set as per board + * design to allow voltage margining in S0ix to lower + * power consumption. + * But if GbE is enabled, voltage magining cannot be + * enabled, so the Vm control UPDs need to be set to 0. + */ params->SlpS0WithGbeSupport = 1; params->PchPmSlpS0VmRuntimeControl = 0; params->PchPmSlpS0Vm070VSupport = 0;
-
Furquan Shaikh (Code Review) -
Kane Chen (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)