Krzysztof M Sywula has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32026
Change subject: soc/intel/cannonlake: FSP UPD update ......................................................................
soc/intel/cannonlake: FSP UPD update
Exposed FSP params: SlpS0WithGbeSupport, PchPmSlpS0VmRuntimeControl, PchPmSlpS0VmRuntimeControl, PchPmSlpS0VmRuntimeControl. Their values can be controled from devicetree.cb.
Change-Id: I02aaf0b77b8fc1555a3a424c02acfada21707d0e Signed-off-by: Krzysztof Sywula krzysztof.m.sywula@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/32026/1
diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index b4d78f3..72fb45a 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -138,6 +138,12 @@ uint8_t SataPortsEnable[8]; uint8_t SataPortsDevSlp[8];
+ /* Power management related */ + uint8_t SlpS0WithGbeSupport; + uint8_t PchPmSlpS0VmRuntimeControl; + uint8_t PchPmSlpS0Vm070VSupport; + uint8_t PchPmSlpS0Vm075VSupport; + /* Audio related */ uint8_t PchHdaDspEnable;
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index 77d82d6..58dab3f 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -164,6 +164,12 @@ else params->PchLanEnable = dev->enabled;
+ /* Power management related */ + params->SlpS0WithGbeSupport = config->SlpS0WithGbeSupport; + params->PchPmSlpS0VmRuntimeControl = config->PchPmSlpS0VmRuntimeControl; + params->PchPmSlpS0Vm070VSupport = config->PchPmSlpS0Vm070VSupport; + params->PchPmSlpS0Vm075VSupport = config->PchPmSlpS0Vm075VSupport; + /* Audio */ params->PchHdaDspEnable = config->PchHdaDspEnable; params->PchHdaAudioLinkHda = config->PchHdaAudioLinkHda;
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: FSP UPD update ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/32026/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32026/1//COMMIT_MSG@7 PS1, Line 7: FSP UPD update That's not an UPD update, change to something else.
https://review.coreboot.org/#/c/32026/1//COMMIT_MSG@12 PS1, Line 12: Better list how that got tested.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: FSP UPD update ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/chip.h@142 PS1, Line 142: uint8_t SlpS0WithGbeSupport; : uint8_t PchPmSlpS0VmRuntimeControl; : uint8_t PchPmSlpS0Vm070VSupport; : uint8_t PchPmSlpS0Vm075VSupport; Can you please add comments indicating what each param does?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: FSP UPD update ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32026/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32026/1//COMMIT_MSG@7 PS1, Line 7: FSP UPD update Please use verbs (in imperative mood) to make it a statement. See `git log --oneline` to get a feeling.
Update FSP UPD
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: FSP UPD update ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 168: SlpS0WithGbeSupport Could this be set based on s0ix_enable+PchLanEnable?
The default seems to be enabled except on WHL V0, do we need to account for that in here as well to not try and force it on with WHL V0?
"Default is 0 when paired with WHL V0 stepping CPU and 1 for all other CPUs."
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 169: PchPmSlpS0VmRuntimeControl What is the downside of enabling these options when config->s0ix_enable is set?
Krzysztof M Sywula has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: FSP UPD update ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 169: PchPmSlpS0VmRuntimeControl
What is the downside of enabling these options when config->s0ix_enable is set?
As far as I know voltage margining is only used for debug/validation purposes. Not sure if that should ever be auto configured. I see in coreboot sources some of these options are left as is (so they are fully controlled from appropriate devicetree.cb). Some other options are auto-configured wihtin if/else logic. I honestly don't know what's better in this case.
Now, addressing your exact question, I think s0ix_enable should not control these params because from end user point of view it's perfectly feasible to have either case: s0ix_enable=1 ; PchPmSlpS0VmRuntimeControl=0 or s0ix_enable=1 ; PchPmSlpS0VmRuntimeControl=1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: FSP UPD update ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 169: PchPmSlpS0VmRuntimeControl
As far as I know voltage margining is only used for debug/validation purposes. […]
Isn't this dependent on the mainboard design? i.e. is the board designed to support voltage margining in S0ix? And so it will have an end user impact in terms of power consumption in S0ix? I am curious to understand why this is only a debug/validation thing?
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: FSP UPD update ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 169: PchPmSlpS0VmRuntimeControl
Isn't this dependent on the mainboard design? i.e. […]
I agree with Furquan and I don't believe that's debug/validation thing.
Krzysztof M Sywula has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: FSP UPD update ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 169: PchPmSlpS0VmRuntimeControl
I agree with Furquan and I don't believe that's debug/validation thing.
Unless we have hardware/electrical engineer say otherwise, I'm gonna go with what google offers:
"Margining is a test procedure that determines the "safety margin." A parameter is varied to determine the device's sensitivity or ability to perform given a range of inputs. A large number of parts can be characterized to determine a safe range for the specification, to guarantee performance and yield."
or
"Voltage margining is a means of verifying the robustness of a product by intentionally adjusting its supply voltages to their limits and then evaluating the product's performance to ensure that it still meets its specifications at the power supply's extremes."
Again, I don't believe that end customer (person who obtains fully finished device) would ever want voltage margining enabled. S0ix should not be tied in any way to voltage margining. Voltage margining should only be accessible to engineers working on a product.
Hello Thejaswani Putta, Patrick Rudolph, Subrata Banik, Roy Mingi Park, Duncan Laurie, Bora Guvendik, Lijian Zhao, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32026
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Expose FSP params to be adjusted from devicetree.cb ......................................................................
soc/intel/cannonlake: Expose FSP params to be adjusted from devicetree.cb
PchPmSlpS0VmRuntimeControl, PchPmSlpS0Vm070VSupport, PchPmSlpS0Vm075VSupport: configure voltage margining SlpS0WithGbeSupport: enable/disable SLP_S0 with GBE support
TEST=Configure params in appropriate devicetree.cb, boot the board with debug FSP and compare if changes are reflected
Change-Id: I02aaf0b77b8fc1555a3a424c02acfada21707d0e Signed-off-by: Krzysztof Sywula krzysztof.m.sywula@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/32026/2
Krzysztof M Sywula has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: Expose FSP params to be adjusted from devicetree.cb ......................................................................
Patch Set 2:
Patch Set 1:
(1 comment)
I refreshed whole commit message in patchset 3 addressing your and previous comments.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: Expose FSP params to be adjusted from devicetree.cb ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 169: PchPmSlpS0VmRuntimeControl
Unless we have hardware/electrical engineer say otherwise, I'm gonna go with what google offers:
Can you please get confirmation from hardware/FSP team about what voltage margining really means in this case w.r.t. S0ix? My past experience with voltage margining on KBL was very different than the definitions outlined above.
Hello Thejaswani Putta, Patrick Rudolph, Subrata Banik, Roy Mingi Park, Duncan Laurie, Bora Guvendik, build bot (Jenkins), Lijian Zhao,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32026
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Configure voltage margining policies ......................................................................
soc/intel/cannonlake: Configure voltage margining policies
For systems that integrate GbE controllers, following parameters should be configured: SlpS0WithGbeSupport: enable PchPmSlpS0VmRuntimeControl: disable, PchPmSlpS0Vm070VSupport: disable, PchPmSlpS0Vm075VSupport: disable.
TEST=boot on any GbE supported WHL platform
Change-Id: I02aaf0b77b8fc1555a3a424c02acfada21707d0e Signed-off-by: Krzysztof Sywula krzysztof.m.sywula@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 17 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/32026/3
Krzysztof M Sywula has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: Configure voltage margining policies ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 169: PchPmSlpS0VmRuntimeControl
Unless we have hardware/electrical engineer say otherwise, I'm gonna go with what google offers: […]
I became aware of a documentation that specifies how these parameters should be configured for GbE supported WHL platforms. Now it makes much more sense to configure it the way you and Duncan Laurie suggested.
Krzysztof M Sywula has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: Configure voltage margining policies ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 168: SlpS0WithGbeSupport
Could this be set based on s0ix_enable+PchLanEnable? […]
"Could this be set based on s0ix_enable+PchLanEnable?" - yes, done "The default seems to be enabled except on WHL V0, do we need to account for that in here as well to not try and force it on with WHL V0?" - no, as per documentation, SlpS0WithGbeSupport should be TRUE (1) for GbE enabled platforms, W0 and V0 steppings.
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 169: PchPmSlpS0VmRuntimeControl
I became aware of a documentation that specifies how these parameters should be configured for GbE s […]
Done
Krzysztof M Sywula has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: Configure voltage margining policies ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/32026/1/src/soc/intel/cannonlake/chip.h@142 PS1, Line 142: uint8_t SlpS0WithGbeSupport; : uint8_t PchPmSlpS0VmRuntimeControl; : uint8_t PchPmSlpS0Vm070VSupport; : uint8_t PchPmSlpS0Vm075VSupport;
Can you please add comments indicating what each param does?
Done
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: Configure voltage margining policies ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32026 )
Change subject: soc/intel/cannonlake: Configure voltage margining policies ......................................................................
soc/intel/cannonlake: Configure voltage margining policies
For systems that integrate GbE controllers, following parameters should be configured: SlpS0WithGbeSupport: enable PchPmSlpS0VmRuntimeControl: disable, PchPmSlpS0Vm070VSupport: disable, PchPmSlpS0Vm075VSupport: disable.
TEST=boot on any GbE supported WHL platform
Change-Id: I02aaf0b77b8fc1555a3a424c02acfada21707d0e Signed-off-by: Krzysztof Sywula krzysztof.m.sywula@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32026 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Duncan Laurie dlaurie@chromium.org --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 17 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index 5d9c744..a81a7c1 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -138,6 +138,15 @@ uint8_t SataPortsEnable[8]; uint8_t SataPortsDevSlp[8];
+ /* Enable/Disable SLP_S0 with GBE Support. 0: disable, 1: enable */ + uint8_t SlpS0WithGbeSupport; + /* SLP_S0 Voltage Margining Policy. 0: disable, 1: enable */ + uint8_t PchPmSlpS0VmRuntimeControl; + /* SLP_S0 Voltage Margining 0.70V Policy. 0: disable, 1: enable */ + uint8_t PchPmSlpS0Vm070VSupport; + /* SLP_S0 Voltage Margining 0.75V Policy. 0: disable, 1: enable */ + uint8_t PchPmSlpS0Vm075VSupport; + /* Audio related */ uint8_t PchHdaDspEnable;
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index 1a3b4fb..6173403 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -161,8 +161,15 @@ dev = dev_find_slot(0, PCH_DEVFN_GBE); if (!dev) params->PchLanEnable = 0; - else + else { params->PchLanEnable = dev->enabled; + if (config->s0ix_enable) { + params->SlpS0WithGbeSupport = 1; + params->PchPmSlpS0VmRuntimeControl = 0; + params->PchPmSlpS0Vm070VSupport = 0; + params->PchPmSlpS0Vm075VSupport = 0; + } + }
/* Audio */ params->PchHdaDspEnable = config->PchHdaDspEnable;
-
Duncan Laurie (Code Review) -
Furquan Shaikh (Code Review)
-
Krzysztof M Sywula (Code Review)
-
Lijian Zhao (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)