Maxim Polyakov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35067 )
Change subject: mb/asrock/h110m: fix VR Settings Configuration info ......................................................................
mb/asrock/h110m: fix VR Settings Configuration info
In accordance with changes in the initialization code for Skylake/Kaby Lake CPU [1], the IccMax is set automatically in vr_config.c
The patch adds a comment about this and fixes invalid parameter values in the VR settings information.
[1] https://review.coreboot.org/c/coreboot/+/34937
Change-Id: I6e1aefde135ffce75a5d837348595aa20aff0513 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/mainboard/asrock/h110m/devicetree.cb 1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/35067/1
diff --git a/src/mainboard/asrock/h110m/devicetree.cb b/src/mainboard/asrock/h110m/devicetree.cb index 3067ffe..61d3e66 100644 --- a/src/mainboard/asrock/h110m/devicetree.cb +++ b/src/mainboard/asrock/h110m/devicetree.cb @@ -89,9 +89,10 @@ #| Psi4Enable | 1 | 1 | 1 | 1 | 1 | #| ImonSlope | 0 | 0 | 0 | 0 | 0 | #| ImonOffset | 0 | 0 | 0 | 0 | 0 | - #| IccMax | 7A | 34A | 34A | 35A | 35A | - #| VrVoltageLimit | 1.52V | 1.52V | 1.52V | 1.52V | 1.52V | + #| IccMax* | 0 | 0 | 0 | 0 | 0 | + #| VrVoltageLimit | 0 | 0 | 0 | 0 | 0 | #+----------------+-------+-------+-------------+-------------+-------+ + # * - is set automatically for the KBL-S/KBL-DT CPUs in the vr_config.c register "domain_vr_config[VR_SYSTEM_AGENT]" = "{ .vr_config_enable = 1, \ .psi1threshold = 0x50, \
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35067 )
Change subject: mb/asrock/h110m: fix VR Settings Configuration info ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... File src/mainboard/asrock/h110m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... PS2, Line 93: VrVoltageLimit Is VrVoltageLimit set automatically as well?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35067 )
Change subject: mb/asrock/h110m: fix VR Settings Configuration info ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... File src/mainboard/asrock/h110m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... PS2, Line 93: VrVoltageLimit
Is VrVoltageLimit set automatically as well?
Waiting on a reply about this
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35067 )
Change subject: mb/asrock/h110m: fix VR Settings Configuration info ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... File src/mainboard/asrock/h110m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... PS2, Line 93: VrVoltageLimit
Waiting on a reply about this
No, this parameter is set from the device tree. https://github.com/coreboot/coreboot/blob/master/src/soc/intel/skylake/vr_co...
According to the documentation, this should be set to 1520 (mV).
However, this comment bothers me: "not used on KBL-S and KBL-DT" https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/x...
I have not found information about this yet
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35067 )
Change subject: mb/asrock/h110m: fix VR Settings Configuration info ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... File src/mainboard/asrock/h110m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... PS2, Line 93: VrVoltageLimit
No, this parameter is set from the device tree. […]
Um, Sarien makes use of this limit:
https://github.com/coreboot/coreboot/blob/master/src/mainboard/google/sarien...
I can't find any reference about that parameter being unused on KBL-S or KBL-DT...
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35067 )
Change subject: mb/asrock/h110m: fix VR Settings Configuration info ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35067/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35067/2//COMMIT_MSG@15 PS2, Line 15: [1] https://review.coreboot.org/c/coreboot/+/34937 I would reference the commit hash and summary instead of the gerrit link
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... File src/mainboard/asrock/h110m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... PS2, Line 93: VrVoltageLimit
Um, Sarien makes use of this limit: […]
Oh, I just checked. There's missing PCI IDs for Skylake devices on CB:34937, so I would expand that. The idea is that boards with sockets don't need to specify extra parameters.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35067 )
Change subject: mb/asrock/h110m: fix VR Settings Configuration info ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... File src/mainboard/asrock/h110m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... PS2, Line 93: VrVoltageLimit
Oh, I just checked. There's missing PCI IDs for Skylake devices on CB:34937, so I would expand that. […]
Yes, I just finished work on this patch
Change-Id: I8e517d8230c251e0cd4b1d4f1b9292c3df80cb19 Change-Id: I19766e4f8fab6b48565b65ed4cf13efbc213e654
Hello Angel Pons, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35067
to look at the new patch set (#8).
Change subject: mb/asrock/h110m: fix VR domains configuration ......................................................................
mb/asrock/h110m: fix VR domains configuration
1) VR domains current limit Icc max for Sky/Kaby Lake S is set based on the processor TDP [1]. Updates information about this
2) Sets VR voltage limit to 1.52V, as described in the datasheets [2,3]
[1] Change-Id: I303c5dc8ed03e9a98a834a2acfb400022dfc2fde [2] page 112-119, 6th Generation Intel(R) Processor Families for S-Platforms, Volume 1 of 2, Datasheet, August 2018. Document Number: 332687-008EN [3] 7th Generation Intel(R) Processor Families for S Platforms and Intel(R) Core(TM) X-Series Processor Family Datasheet, Volume 1, December 2018, Document Number: 335195-003
Change-Id: I6e1aefde135ffce75a5d837348595aa20aff0513 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/mainboard/asrock/h110m/devicetree.cb 1 file changed, 6 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/35067/8
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35067 )
Change subject: mb/asrock/h110m: fix VR domains configuration ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... File src/mainboard/asrock/h110m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35067/2/src/mainboard/asrock/h110m/... PS2, Line 93: VrVoltageLimit
Yes, I just finished work on this patch […]
Done
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35067 )
Change subject: mb/asrock/h110m: fix VR domains configuration ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35067/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35067/2//COMMIT_MSG@15 PS2, Line 15: [1] https://review.coreboot.org/c/coreboot/+/34937
I would reference the commit hash and summary instead of the gerrit link
Done
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35067 )
Change subject: mb/asrock/h110m: fix VR domains configuration ......................................................................
Patch Set 8: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35067 )
Change subject: mb/asrock/h110m: fix VR domains configuration ......................................................................
mb/asrock/h110m: fix VR domains configuration
1) VR domains current limit Icc max for Sky/Kaby Lake S is set based on the processor TDP [1]. Updates information about this
2) Sets VR voltage limit to 1.52V, as described in the datasheets [2,3]
[1] Change-Id: I303c5dc8ed03e9a98a834a2acfb400022dfc2fde [2] page 112-119, 6th Generation Intel(R) Processor Families for S-Platforms, Volume 1 of 2, Datasheet, August 2018. Document Number: 332687-008EN [3] 7th Generation Intel(R) Processor Families for S Platforms and Intel(R) Core(TM) X-Series Processor Family Datasheet, Volume 1, December 2018, Document Number: 335195-003
Change-Id: I6e1aefde135ffce75a5d837348595aa20aff0513 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35067 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Georgi pgeorgi@google.com --- M src/mainboard/asrock/h110m/devicetree.cb 1 file changed, 6 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/src/mainboard/asrock/h110m/devicetree.cb b/src/mainboard/asrock/h110m/devicetree.cb index 3067ffe..5370568 100644 --- a/src/mainboard/asrock/h110m/devicetree.cb +++ b/src/mainboard/asrock/h110m/devicetree.cb @@ -89,9 +89,10 @@ #| Psi4Enable | 1 | 1 | 1 | 1 | 1 | #| ImonSlope | 0 | 0 | 0 | 0 | 0 | #| ImonOffset | 0 | 0 | 0 | 0 | 0 | - #| IccMax | 7A | 34A | 34A | 35A | 35A | + #| IccMax* | 0 | 0 | 0 | 0 | 0 | #| VrVoltageLimit | 1.52V | 1.52V | 1.52V | 1.52V | 1.52V | #+----------------+-------+-------+-------------+-------------+-------+ + # * - is set automatically for the KBL-S/KBL-DT CPUs in the vr_config.c register "domain_vr_config[VR_SYSTEM_AGENT]" = "{ .vr_config_enable = 1, \ .psi1threshold = 0x50, \ @@ -102,7 +103,7 @@ .imon_slope = 0x0, \ .imon_offset = 0x0, \ .icc_max = 0x0, \ - .voltage_limit = 0x0 \ + .voltage_limit = 1520 \ }"
register "domain_vr_config[VR_IA_CORE]" = "{ @@ -115,7 +116,7 @@ .imon_slope = 0x0, \ .imon_offset = 0x0, \ .icc_max = 0x0, \ - .voltage_limit = 0x0 \ + .voltage_limit = 1520 \ }"
register "domain_vr_config[VR_GT_UNSLICED]" = "{ @@ -128,7 +129,7 @@ .imon_slope = 0x0, \ .imon_offset = 0x0, \ .icc_max = 0x0 ,\ - .voltage_limit = 0x0 \ + .voltage_limit = 1520 \ }"
register "domain_vr_config[VR_GT_SLICED]" = "{ @@ -141,7 +142,7 @@ .imon_slope = 0x0, \ .imon_offset = 0x0, \ .icc_max = 0x0, \ - .voltage_limit = 0x0 \ + .voltage_limit = 1520 \ }"
register "EnableLan" = "0"
-
Angel Pons (Code Review) -
Maxim Polyakov (Code Review)
-
Patrick Georgi (Code Review)