Change in coreboot[master]: google/chell: Update ICC_MAX configuration

[Matt DeVillier (Code Review)]

Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35958 )

Change subject: google/chell: Update ICC_MAX configuration
......................................................................

google/chell: Update ICC_MAX configuration

Correct ICC_MAX values per SKL-Y EDS spec.

Adapted from chromium commit 1c4e89e8 [Chell: Update ICC_MAX configuration]

Original-Change-Id: Ic660cc6a2d11e995a86a30ddde800d096d93e012
Original-Signed-off-by: Sumeet Pawnikar 
Original-Reviewed-on: https://chromium-review.googlesource.com/593715
Original-Reviewed-by: Duncan Laurie

Change-Id: Ia31ce432cf979d574d84e9205a287f87de5de057
Signed-off-by: Matt DeVillier 
---
M src/mainboard/google/glados/variants/chell/devicetree.cb
1 file changed, 5 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/35958/1

diff --git a/src/mainboard/google/glados/variants/chell/devicetree.cb b/src/mainboard/google/glados/variants/chell/devicetree.cb
index 5b82e5a..89f1c08 100644
--- a/src/mainboard/google/glados/variants/chell/devicetree.cb
+++ b/src/mainboard/google/glados/variants/chell/devicetree.cb
@@ -81,7 +81,7 @@
    #| Psi4Enable     | 1         | 1         | 1           | 1        |
    #| ImonSlope      | 0         | 0         | 0           | 0        |
    #| ImonOffset     | 0         | 0         | 0           | 0        |
-	#| IccMax         | 7A        | 34A       | 35A         | 35A      |
+	#| IccMax         | 4A        | 24A       | 24A         | 24A      |
    #| VrVoltageLimit | 1.52V     | 1.52V     | 1.52V       | 1.52V    |
    #+----------------+-----------+-----------+-------------+----------+
    register "domain_vr_config[VR_SYSTEM_AGENT]" = "{
@@ -93,7 +93,7 @@
    	.psi4enable = 1,
    	.imon_slope = 0x0,
    	.imon_offset = 0x0,
-		.icc_max = VR_CFG_AMP(7),
+		.icc_max = VR_CFG_AMP(4),
    	.voltage_limit = 1520,
    }"

@@ -106,7 +106,7 @@
    	.psi4enable = 1,
    	.imon_slope = 0x0,
    	.imon_offset = 0x0,
-		.icc_max = VR_CFG_AMP(34),
+		.icc_max = VR_CFG_AMP(24),
    	.voltage_limit = 1520,
    }"

@@ -119,7 +119,7 @@
    	.psi4enable = 1,
    	.imon_slope = 0x0,
    	.imon_offset = 0x0,
-		.icc_max = VR_CFG_AMP(35),
+		.icc_max = VR_CFG_AMP(24),
    	.voltage_limit = 1520,
    }"

@@ -132,7 +132,7 @@
    	.psi4enable = 1,
    	.imon_slope = 0x0,
    	.imon_offset = 0x0,
-		.icc_max = VR_CFG_AMP(35),
+		.icc_max = VR_CFG_AMP(24),
    	.voltage_limit = 1520,
    }"

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35958 )

Change subject: google/chell: Update ICC_MAX configuration
......................................................................

Patch Set 1: Code-Review+2

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35958 )

Change subject: google/chell: Update ICC_MAX configuration
......................................................................

Patch Set 1:

is this something we would want to add to vr_config.c as well?

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35958 )

Change subject: google/chell: Update ICC_MAX configuration
......................................................................

Patch Set 1:
> Patch Set 1:
>
> is this something we would want to add to vr_config.c as well?

CB:35167 already does this

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35958 )

Change subject: google/chell: Update ICC_MAX configuration
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > is this something we would want to add to vr_config.c as well?
>
> CB:35167 already does this

Oh yes, indeed. Do we need this CR then?

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35958 )

Change subject: google/chell: Update ICC_MAX configuration
......................................................................

Patch Set 1:
> Oh yes, indeed. Do we need this CR then?

as long as the devicetree settings exist, they'll override the defaults in vr_config.c.
I'm guessing the intent is to be able to remove all the devicetree overrides unless they deviate from the SKU defaults. But until then, the devicetree settings need to be correct.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35958 )

Change subject: google/chell: Update ICC_MAX configuration
......................................................................

Patch Set 1:
> Patch Set 1:
> > Oh yes, indeed. Do we need this CR then?
>
> as long as the devicetree settings exist, they'll override the defaults in vr_config.c.
> I'm guessing the intent is to be able to remove all the devicetree overrides unless they deviate from the SKU defaults. But until then, the devicetree settings need to be correct.

Well, that's what I mean - your CR is based on 35167, so 35167 well be get merged before yours and then yours is obsolete :-)

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35958 )

Change subject: google/chell: Update ICC_MAX configuration
......................................................................

Patch Set 1:
> Well, that's what I mean - your CR is based on 35167, so 35167 well be get merged before yours and then yours is obsolete :-)

no, not obsolete. necessary unless all VR config info removed from devicetree. As long as incorrect info in devicetree, will override SKU defaults in vr_config.c

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35958 )

Change subject: google/chell: Update ICC_MAX configuration
......................................................................

Patch Set 1: Code-Review+1
> Patch Set 1:
> > Well, that's what I mean - your CR is based on 35167, so 35167 well be get merged before yours and then yours is obsolete :-)
>
> no, not obsolete. necessary unless all VR config info removed from devicetree. As long as incorrect info in devicetree, will override SKU defaults in vr_config.c

indeed, you're right. my fault :-)

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35958 )

Change subject: google/chell: Update ICC_MAX configuration
......................................................................

google/chell: Update ICC_MAX configuration

Correct ICC_MAX values per SKL-Y EDS spec.

Adapted from chromium commit 1c4e89e8 [Chell: Update ICC_MAX configuration]

Original-Change-Id: Ic660cc6a2d11e995a86a30ddde800d096d93e012
Original-Signed-off-by: Sumeet Pawnikar 
Original-Reviewed-on: https://chromium-review.googlesource.com/593715
Original-Reviewed-by: Duncan Laurie

Change-Id: Ia31ce432cf979d574d84e9205a287f87de5de057
Signed-off-by: Matt DeVillier 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35958
Tested-by: build bot (Jenkins) 
Reviewed-by: Aaron Durbin 
Reviewed-by: Michael Niewöhner
---
M src/mainboard/google/glados/variants/chell/devicetree.cb
1 file changed, 5 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved
  Michael Niewöhner: Looks good to me, but someone else must approve

diff --git a/src/mainboard/google/glados/variants/chell/devicetree.cb b/src/mainboard/google/glados/variants/chell/devicetree.cb
index 5b82e5a..89f1c08 100644
--- a/src/mainboard/google/glados/variants/chell/devicetree.cb
+++ b/src/mainboard/google/glados/variants/chell/devicetree.cb
@@ -81,7 +81,7 @@
    #| Psi4Enable     | 1         | 1         | 1           | 1        |
    #| ImonSlope      | 0         | 0         | 0           | 0        |
    #| ImonOffset     | 0         | 0         | 0           | 0        |
-	#| IccMax         | 7A        | 34A       | 35A         | 35A      |
+	#| IccMax         | 4A        | 24A       | 24A         | 24A      |
    #| VrVoltageLimit | 1.52V     | 1.52V     | 1.52V       | 1.52V    |
    #+----------------+-----------+-----------+-------------+----------+
    register "domain_vr_config[VR_SYSTEM_AGENT]" = "{
@@ -93,7 +93,7 @@
    	.psi4enable = 1,
    	.imon_slope = 0x0,
    	.imon_offset = 0x0,
-		.icc_max = VR_CFG_AMP(7),
+		.icc_max = VR_CFG_AMP(4),
    	.voltage_limit = 1520,
    }"

@@ -106,7 +106,7 @@
    	.psi4enable = 1,
    	.imon_slope = 0x0,
    	.imon_offset = 0x0,
-		.icc_max = VR_CFG_AMP(34),
+		.icc_max = VR_CFG_AMP(24),
    	.voltage_limit = 1520,
    }"

@@ -119,7 +119,7 @@
    	.psi4enable = 1,
    	.imon_slope = 0x0,
    	.imon_offset = 0x0,
-		.icc_max = VR_CFG_AMP(35),
+		.icc_max = VR_CFG_AMP(24),
    	.voltage_limit = 1520,
    }"

@@ -132,7 +132,7 @@
    	.psi4enable = 1,
    	.imon_slope = 0x0,
    	.imon_offset = 0x0,
-		.icc_max = VR_CFG_AMP(35),
+		.icc_max = VR_CFG_AMP(24),
    	.voltage_limit = 1520,
    }"