Paul Fagerburg has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35920 )
Change subject: helios: Add TEMP_SENSOR4 to DPTF ......................................................................
helios: Add TEMP_SENSOR4 to DPTF
Helios adds TEMP_SENSOR4 to the EC ADC2 pin. Add this to the DPTF.
BRANCH=None BUG=b:142266102 TEST=`emerge-hatch coreboot` Verify that Helios builds correctly.
Change-Id: I3bc19f9b9bd644e134987749ad9a4d875ad8b40a Signed-off-by: Paul Fagerburg pfagerburg@chromium.org --- M src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl 1 file changed, 18 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/35920/1
diff --git a/src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl b/src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl index e3159c8..0013f29 100644 --- a/src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl +++ b/src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl @@ -54,6 +54,17 @@ #define DPTF_TSR2_ACTIVE_AC4 40 #define DPTF_TSR2_ACTIVE_AC5 38
+#define DPTF_TSR3_SENSOR_ID 3 +#define DPTF_TSR3_SENSOR_NAME "CPU" +#define DPTF_TSR3_PASSIVE 85 +#define DPTF_TSR3_CRITICAL 100 +#define DPTF_TSR3_ACTIVE_AC0 0 +#define DPTF_TSR3_ACTIVE_AC1 0 +#define DPTF_TSR3_ACTIVE_AC2 0 +#define DPTF_TSR3_ACTIVE_AC3 0 +#define DPTF_TSR3_ACTIVE_AC4 0 +#define DPTF_TSR3_ACTIVE_AC5 0 + #define DPTF_ENABLE_CHARGER #define DPTF_ENABLE_FAN_CONTROL
@@ -107,6 +118,10 @@ Package () { _SB.DPTF.TFN1, _SB.DPTF.TSR2, 100, 90, 69, 56, 46, 36, 30, 0, 0, 0, 0 + }, + Package () { + _SB.DPTF.TFN1, _SB.DPTF.TSR3, 100, 90, 69, 56, 46, 36, 30, 0, + 0, 0, 0 } })
@@ -122,6 +137,9 @@
/* CPU Throttle Effect on TSR2 */ Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR2, 100, 60, 0, 0, 0, 0 }, + + /* CPU Throttle Effect on TSR3 */ + Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR3, 100, 60, 0, 0, 0, 0 }, })
Name (MPPC, Package ()
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35920 )
Change subject: helios: Add TEMP_SENSOR4 to DPTF ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35920/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl:
https://review.coreboot.org/c/coreboot/+/35920/1/src/mainboard/google/hatch/... PS1, Line 57: #define DPTF_TSR3_SENSOR_ID 3 : #define DPTF_TSR3_SENSOR_NAME "CPU" : #define DPTF_TSR3_PASSIVE 85 : #define DPTF_TSR3_CRITICAL 100 : #define DPTF_TSR3_ACTIVE_AC0 0 : #define DPTF_TSR3_ACTIVE_AC1 0 : #define DPTF_TSR3_ACTIVE_AC2 0 : #define DPTF_TSR3_ACTIVE_AC3 0 : #define DPTF_TSR3_ACTIVE_AC4 0 : #define DPTF_TSR3_ACTIVE_AC5 0 : Where did all the setpoints/thresholds come from?
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35920 )
Change subject: helios: Add TEMP_SENSOR4 to DPTF ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35920/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl:
https://review.coreboot.org/c/coreboot/+/35920/1/src/mainboard/google/hatch/... PS1, Line 57: #define DPTF_TSR3_SENSOR_ID 3 : #define DPTF_TSR3_SENSOR_NAME "CPU" : #define DPTF_TSR3_PASSIVE 85 : #define DPTF_TSR3_CRITICAL 100 : #define DPTF_TSR3_ACTIVE_AC0 0 : #define DPTF_TSR3_ACTIVE_AC1 0 : #define DPTF_TSR3_ACTIVE_AC2 0 : #define DPTF_TSR3_ACTIVE_AC3 0 : #define DPTF_TSR3_ACTIVE_AC4 0 : #define DPTF_TSR3_ACTIVE_AC5 0 :
Where did all the setpoints/thresholds come from?
https://b.corp.google.com/issues/142266102#comment7
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35920 )
Change subject: helios: Add TEMP_SENSOR4 to DPTF ......................................................................
Patch Set 1:
(1 comment)
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35920/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl:
https://review.coreboot.org/c/coreboot/+/35920/1/src/mainboard/google/hatch/... PS1, Line 57: #define DPTF_TSR3_SENSOR_ID 3 : #define DPTF_TSR3_SENSOR_NAME "CPU" : #define DPTF_TSR3_PASSIVE 85 : #define DPTF_TSR3_CRITICAL 100 : #define DPTF_TSR3_ACTIVE_AC0 0 : #define DPTF_TSR3_ACTIVE_AC1 0 : #define DPTF_TSR3_ACTIVE_AC2 0 : #define DPTF_TSR3_ACTIVE_AC3 0 : #define DPTF_TSR3_ACTIVE_AC4 0 : #define DPTF_TSR3_ACTIVE_AC5 0 :
Sorry, I mean could you add more description to the commit message about what this is doing.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35920 )
Change subject: helios: Add TEMP_SENSOR4 to DPTF ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35920/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl:
https://review.coreboot.org/c/coreboot/+/35920/1/src/mainboard/google/hatch/... PS1, Line 57: #define DPTF_TSR3_SENSOR_ID 3 : #define DPTF_TSR3_SENSOR_NAME "CPU" : #define DPTF_TSR3_PASSIVE 85 : #define DPTF_TSR3_CRITICAL 100 : #define DPTF_TSR3_ACTIVE_AC0 0 : #define DPTF_TSR3_ACTIVE_AC1 0 : #define DPTF_TSR3_ACTIVE_AC2 0 : #define DPTF_TSR3_ACTIVE_AC3 0 : #define DPTF_TSR3_ACTIVE_AC4 0 : #define DPTF_TSR3_ACTIVE_AC5 0 :
Sorry, I mean could you add more description to the commit message about what this is doing.
All values were provided by the ODM. The zeros for ACTIVE_AC0 through AC5 are to disable those particular settings.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35920 )
Change subject: helios: Add TEMP_SENSOR4 to DPTF ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35920/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl:
https://review.coreboot.org/c/coreboot/+/35920/1/src/mainboard/google/hatch/... PS1, Line 57: #define DPTF_TSR3_SENSOR_ID 3 : #define DPTF_TSR3_SENSOR_NAME "CPU" : #define DPTF_TSR3_PASSIVE 85 : #define DPTF_TSR3_CRITICAL 100 : #define DPTF_TSR3_ACTIVE_AC0 0 : #define DPTF_TSR3_ACTIVE_AC1 0 : #define DPTF_TSR3_ACTIVE_AC2 0 : #define DPTF_TSR3_ACTIVE_AC3 0 : #define DPTF_TSR3_ACTIVE_AC4 0 : #define DPTF_TSR3_ACTIVE_AC5 0 :
All values were provided by the ODM. […]
The values that are all 0 -- Is it okay to skip setting them?
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35920 )
Change subject: helios: Add TEMP_SENSOR4 to DPTF ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35920/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl:
https://review.coreboot.org/c/coreboot/+/35920/1/src/mainboard/google/hatch/... PS1, Line 57: #define DPTF_TSR3_SENSOR_ID 3 : #define DPTF_TSR3_SENSOR_NAME "CPU" : #define DPTF_TSR3_PASSIVE 85 : #define DPTF_TSR3_CRITICAL 100 : #define DPTF_TSR3_ACTIVE_AC0 0 : #define DPTF_TSR3_ACTIVE_AC1 0 : #define DPTF_TSR3_ACTIVE_AC2 0 : #define DPTF_TSR3_ACTIVE_AC3 0 : #define DPTF_TSR3_ACTIVE_AC4 0 : #define DPTF_TSR3_ACTIVE_AC5 0 :
The values that are all 0 -- Is it okay to skip setting them?
After looking at the underlying code with you and Tim, I think we need to define these anyway. Also, future platform tuning will change those values, so we having them defined now means we have nice placeholders for future use.
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35920 )
Change subject: helios: Add TEMP_SENSOR4 to DPTF ......................................................................
Patch Set 1: Code-Review+2
Shelley Chen has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35920 )
Change subject: helios: Add TEMP_SENSOR4 to DPTF ......................................................................
helios: Add TEMP_SENSOR4 to DPTF
Helios adds TEMP_SENSOR4 to the EC ADC2 pin. Add this to the DPTF.
BRANCH=None BUG=b:142266102 TEST=`emerge-hatch coreboot` Verify that Helios builds correctly.
Change-Id: I3bc19f9b9bd644e134987749ad9a4d875ad8b40a Signed-off-by: Paul Fagerburg pfagerburg@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/35920 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl 1 file changed, 18 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Shelley Chen: Looks good to me, approved
diff --git a/src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl b/src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl index e3159c8..0013f29 100644 --- a/src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl +++ b/src/mainboard/google/hatch/variants/helios/include/variant/acpi/dptf.asl @@ -54,6 +54,17 @@ #define DPTF_TSR2_ACTIVE_AC4 40 #define DPTF_TSR2_ACTIVE_AC5 38
+#define DPTF_TSR3_SENSOR_ID 3 +#define DPTF_TSR3_SENSOR_NAME "CPU" +#define DPTF_TSR3_PASSIVE 85 +#define DPTF_TSR3_CRITICAL 100 +#define DPTF_TSR3_ACTIVE_AC0 0 +#define DPTF_TSR3_ACTIVE_AC1 0 +#define DPTF_TSR3_ACTIVE_AC2 0 +#define DPTF_TSR3_ACTIVE_AC3 0 +#define DPTF_TSR3_ACTIVE_AC4 0 +#define DPTF_TSR3_ACTIVE_AC5 0 + #define DPTF_ENABLE_CHARGER #define DPTF_ENABLE_FAN_CONTROL
@@ -107,6 +118,10 @@ Package () { _SB.DPTF.TFN1, _SB.DPTF.TSR2, 100, 90, 69, 56, 46, 36, 30, 0, 0, 0, 0 + }, + Package () { + _SB.DPTF.TFN1, _SB.DPTF.TSR3, 100, 90, 69, 56, 46, 36, 30, 0, + 0, 0, 0 } })
@@ -122,6 +137,9 @@
/* CPU Throttle Effect on TSR2 */ Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR2, 100, 60, 0, 0, 0, 0 }, + + /* CPU Throttle Effect on TSR3 */ + Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR3, 100, 60, 0, 0, 0, 0 }, })
Name (MPPC, Package ()
-
Furquan Shaikh (Code Review) -
Paul Fagerburg (Code Review)
-
Shelley Chen (Code Review)
-
Tim Wawrzynczak (Code Review)