Change in coreboot[master]: mb/google/kohaku: Update DPTF parameters and TCC offset setting

[shkim (Code Review)]

Hello Seunghwan Kim,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/35532

to review the following change.

Change subject: mb/google/kohaku: Update DPTF parameters and TCC offset setting
......................................................................

mb/google/kohaku: Update DPTF parameters and TCC offset setting

This change applies fine-tuned DPTF parameters and TCC offset setting
for kohaku.

BUG=b:137688474
BRANCH=none
TEST=built and verified the setting values

Change-Id: I92e268b2e07ca5a04e29bda84ddb8fc21eb23251
Signed-off-by: Seunghwan Kim 
---
M src/mainboard/google/hatch/variants/kohaku/include/variant/acpi/dptf.asl
M src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h
M src/mainboard/google/hatch/variants/kohaku/overridetree.cb
3 files changed, 27 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/35532/1

diff --git a/src/mainboard/google/hatch/variants/kohaku/include/variant/acpi/dptf.asl b/src/mainboard/google/hatch/variants/kohaku/include/variant/acpi/dptf.asl
index 375be8e..13a501a 100644
--- a/src/mainboard/google/hatch/variants/kohaku/include/variant/acpi/dptf.asl
+++ b/src/mainboard/google/hatch/variants/kohaku/include/variant/acpi/dptf.asl
@@ -13,28 +13,36 @@
  * GNU General Public License for more details.
  */

-#define DPTF_CPU_PASSIVE	95
+#define DPTF_CPU_PASSIVE	50
 #define DPTF_CPU_CRITICAL	105

#define DPTF_TSR0_SENSOR_ID	0
 #define DPTF_TSR0_SENSOR_NAME	"Thermal Sensor - Charger"
-#define DPTF_TSR0_PASSIVE	49
-#define DPTF_TSR0_CRITICAL	75
+#define DPTF_TSR0_PASSIVE	45
+#define DPTF_TSR0_CRITICAL	90
+#define DPTF_TSR0_TABLET_PASSIVE        32
+#define DPTF_TSR0_TABLET_CRITICAL       90

#define DPTF_TSR1_SENSOR_ID	1
 #define DPTF_TSR1_SENSOR_NAME	"Thermal Sensor - 5V"
-#define DPTF_TSR1_PASSIVE	65
-#define DPTF_TSR1_CRITICAL	75
+#define DPTF_TSR1_PASSIVE	45
+#define DPTF_TSR1_CRITICAL	90
+#define DPTF_TSR1_TABLET_PASSIVE        32
+#define DPTF_TSR1_TABLET_CRITICAL       90

#define DPTF_TSR2_SENSOR_ID	2
 #define DPTF_TSR2_SENSOR_NAME	"Thermal Sensor - IA"
-#define DPTF_TSR2_PASSIVE	65
-#define DPTF_TSR2_CRITICAL	75
+#define DPTF_TSR2_PASSIVE	45
+#define DPTF_TSR2_CRITICAL	90
+#define DPTF_TSR2_TABLET_PASSIVE        32
+#define DPTF_TSR2_TABLET_CRITICAL       90

#define DPTF_TSR3_SENSOR_ID	3
 #define DPTF_TSR3_SENSOR_NAME	"Thermal Sensor - GT"
-#define DPTF_TSR3_PASSIVE	65
-#define DPTF_TSR3_CRITICAL	75
+#define DPTF_TSR3_PASSIVE	45
+#define DPTF_TSR3_CRITICAL	90
+#define DPTF_TSR3_TABLET_PASSIVE        32
+#define DPTF_TSR3_TABLET_CRITICAL       90

#define DPTF_ENABLE_CHARGER

@@ -48,19 +56,19 @@

Name (DTRT, Package () {
    /* CPU Throttle Effect on CPU */
-	Package () { _SB.PCI0.TCPU, _SB.PCI0.TCPU, 100, 50, 0, 0, 0, 0 },
+	Package () { _SB.PCI0.TCPU, _SB.PCI0.TCPU, 100, 10, 0, 0, 0, 0 },

/* CPU Throttle Effect on 5V (TSR1) */
-	Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR1, 100, 60, 0, 0, 0, 0 },
+	Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR1, 100, 10, 0, 0, 0, 0 },

/* Charger Throttle Effect on Charger (TSR0) */
    Package () { _SB.DPTF.TCHG, _SB.DPTF.TSR0, 100, 94, 0, 0, 0, 0 },

/* CPU Throttle Effect on IA (TSR2) */
-	Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR2, 100, 60, 0, 0, 0, 0 },
+	Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR2, 100, 10, 0, 0, 0, 0 },

/* CPU Throttle Effect on GT (TSR3) */
-	Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR3, 100, 60, 0, 0, 0, 0 },
+	Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR3, 100, 10, 0, 0, 0, 0 },
 })

Name (MPPC, Package ()
@@ -68,8 +76,8 @@
    0x2,		/* Revision */
    Package () {	/* Power Limit 1 */
    	0,	/* PowerLimitIndex, 0 for Power Limit 1 */
-		8000,	/* PowerLimitMinimum */
-		15000,	/* PowerLimitMaximum */
+		7000,	/* PowerLimitMinimum */
+		9000,	/* PowerLimitMaximum */
    	28000,	/* TimeWindowMinimum */
    	28000,	/* TimeWindowMaximum */
    	250	/* StepSize */
diff --git a/src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h b/src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h
index 768987d..84e050e 100644
--- a/src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h
+++ b/src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h
@@ -18,4 +18,6 @@

#include <baseboard/ec.h>

+#define EC_ENABLE_MULTIPLE_DPTF_PROFILES
+
 #endif
diff --git a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
index ff6d817..8c7bb1f 100644
--- a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
+++ b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
@@ -2,6 +2,8 @@
    register "tdp_pl1_override" = "8"
    register "tdp_pl2_override" = "51"

+	register "tcc_offset" = "35"	# TCC of 65C
+
    register "SerialIoDevMode" = "{
    	[PchSerialIoIndexI2C0] = PchSerialIoPci,
    	[PchSerialIoIndexI2C1] = PchSerialIoPci,

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35532 )

Change subject: mb/google/kohaku: Update DPTF parameters and TCC offset setting
......................................................................

Patch Set 1: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35532 )

Change subject: mb/google/kohaku: Update DPTF parameters and TCC offset setting
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35532/1/src/mainboard/google/hatch/... 
File src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h:

https://review.coreboot.org/c/coreboot/+/35532/1/src/mainboard/google/hatch/... 
PS1, Line 21: EC_ENABLE_MULTIPLE_DPTF_PROFILES
This is not mentioned in commit message.

https://review.coreboot.org/c/coreboot/+/35532/1/src/mainboard/google/hatch/... 
File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:

https://review.coreboot.org/c/coreboot/+/35532/1/src/mainboard/google/hatch/... 
PS1, Line 2: 8
Shouldn't this be updated to match what is in asl file?

[SH Kim (Code Review)]

SH Kim has uploaded a new patch set (#2) to the change originally created by shkim. ( https://review.coreboot.org/c/coreboot/+/35532 )

Change subject: mb/google/kohaku: Update DPTF parameters and TCC offset setting
......................................................................

mb/google/kohaku: Update DPTF parameters and TCC offset setting

This change applies fine-tuned DPTF parameters and TCC offset setting
for kohaku.

Enable EC_ENABLE_MULTIPLE_DPTF_PROFILES also since Kohaku needs it for
tablet mode.

BUG=b:137688474
BRANCH=none
TEST=built and verified the setting values

Change-Id: I92e268b2e07ca5a04e29bda84ddb8fc21eb23251
Signed-off-by: Seunghwan Kim 
---
M src/mainboard/google/hatch/variants/kohaku/include/variant/acpi/dptf.asl
M src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h
M src/mainboard/google/hatch/variants/kohaku/overridetree.cb
3 files changed, 27 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/35532/2

[SH Kim (Code Review)]

SH Kim has uploaded a new patch set (#3) to the change originally created by shkim. ( https://review.coreboot.org/c/coreboot/+/35532 )

Change subject: mb/google/kohaku: Update DPTF parameters and TCC offset setting
......................................................................

mb/google/kohaku: Update DPTF parameters and TCC offset setting

This change applies fine-tuned DPTF parameters and TCC offset setting
for kohaku. Also enables EC_ENABLE_MULTIPLE_DPTF_PROFILES for tablet
mode.

BUG=b:137688474
BRANCH=none
TEST=built and verified the setting values

Change-Id: I92e268b2e07ca5a04e29bda84ddb8fc21eb23251
Signed-off-by: Seunghwan Kim 
---
M src/mainboard/google/hatch/variants/kohaku/include/variant/acpi/dptf.asl
M src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h
M src/mainboard/google/hatch/variants/kohaku/overridetree.cb
3 files changed, 27 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/35532/3

[SH Kim (Code Review)]

SH Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35532 )

Change subject: mb/google/kohaku: Update DPTF parameters and TCC offset setting
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35532/1/src/mainboard/google/hatch/... 
File src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h:

https://review.coreboot.org/c/coreboot/+/35532/1/src/mainboard/google/hatch/... 
PS1, Line 21: EC_ENABLE_MULTIPLE_DPTF_PROFILES
> This is not mentioned in commit message.

Done

https://review.coreboot.org/c/coreboot/+/35532/1/src/mainboard/google/hatch/... 
File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:

https://review.coreboot.org/c/coreboot/+/35532/1/src/mainboard/google/hatch/... 
PS1, Line 2: 8
> Shouldn't this be updated to match what is in asl file?

We would use 8W for this. (Thermal guy's feedback)

[Patrick Georgi (Code Review)]

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

Change subject: mb/google/kohaku: Update DPTF parameters and TCC offset setting
......................................................................

mb/google/kohaku: Update DPTF parameters and TCC offset setting

This change applies fine-tuned DPTF parameters and TCC offset setting
for kohaku. Also enables EC_ENABLE_MULTIPLE_DPTF_PROFILES for tablet
mode.

BUG=b:137688474
BRANCH=none
TEST=built and verified the setting values

Change-Id: I92e268b2e07ca5a04e29bda84ddb8fc21eb23251
Signed-off-by: Seunghwan Kim 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35532
Tested-by: build bot (Jenkins) 
Reviewed-by: Shelley Chen 
---
M src/mainboard/google/hatch/variants/kohaku/include/variant/acpi/dptf.asl
M src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h
M src/mainboard/google/hatch/variants/kohaku/overridetree.cb
3 files changed, 27 insertions(+), 15 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Shelley Chen: Looks good to me, approved

diff --git a/src/mainboard/google/hatch/variants/kohaku/include/variant/acpi/dptf.asl b/src/mainboard/google/hatch/variants/kohaku/include/variant/acpi/dptf.asl
index 375be8e..13a501a 100644
--- a/src/mainboard/google/hatch/variants/kohaku/include/variant/acpi/dptf.asl
+++ b/src/mainboard/google/hatch/variants/kohaku/include/variant/acpi/dptf.asl
@@ -13,28 +13,36 @@
  * GNU General Public License for more details.
  */

-#define DPTF_CPU_PASSIVE	95
+#define DPTF_CPU_PASSIVE	50
 #define DPTF_CPU_CRITICAL	105

#define DPTF_TSR0_SENSOR_ID	0
 #define DPTF_TSR0_SENSOR_NAME	"Thermal Sensor - Charger"
-#define DPTF_TSR0_PASSIVE	49
-#define DPTF_TSR0_CRITICAL	75
+#define DPTF_TSR0_PASSIVE	45
+#define DPTF_TSR0_CRITICAL	90
+#define DPTF_TSR0_TABLET_PASSIVE        32
+#define DPTF_TSR0_TABLET_CRITICAL       90

#define DPTF_TSR1_SENSOR_ID	1
 #define DPTF_TSR1_SENSOR_NAME	"Thermal Sensor - 5V"
-#define DPTF_TSR1_PASSIVE	65
-#define DPTF_TSR1_CRITICAL	75
+#define DPTF_TSR1_PASSIVE	45
+#define DPTF_TSR1_CRITICAL	90
+#define DPTF_TSR1_TABLET_PASSIVE        32
+#define DPTF_TSR1_TABLET_CRITICAL       90

#define DPTF_TSR2_SENSOR_ID	2
 #define DPTF_TSR2_SENSOR_NAME	"Thermal Sensor - IA"
-#define DPTF_TSR2_PASSIVE	65
-#define DPTF_TSR2_CRITICAL	75
+#define DPTF_TSR2_PASSIVE	45
+#define DPTF_TSR2_CRITICAL	90
+#define DPTF_TSR2_TABLET_PASSIVE        32
+#define DPTF_TSR2_TABLET_CRITICAL       90

#define DPTF_TSR3_SENSOR_ID	3
 #define DPTF_TSR3_SENSOR_NAME	"Thermal Sensor - GT"
-#define DPTF_TSR3_PASSIVE	65
-#define DPTF_TSR3_CRITICAL	75
+#define DPTF_TSR3_PASSIVE	45
+#define DPTF_TSR3_CRITICAL	90
+#define DPTF_TSR3_TABLET_PASSIVE        32
+#define DPTF_TSR3_TABLET_CRITICAL       90

#define DPTF_ENABLE_CHARGER

@@ -48,19 +56,19 @@

Name (DTRT, Package () {
    /* CPU Throttle Effect on CPU */
-	Package () { _SB.PCI0.TCPU, _SB.PCI0.TCPU, 100, 50, 0, 0, 0, 0 },
+	Package () { _SB.PCI0.TCPU, _SB.PCI0.TCPU, 100, 10, 0, 0, 0, 0 },

/* CPU Throttle Effect on 5V (TSR1) */
-	Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR1, 100, 60, 0, 0, 0, 0 },
+	Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR1, 100, 10, 0, 0, 0, 0 },

/* Charger Throttle Effect on Charger (TSR0) */
    Package () { _SB.DPTF.TCHG, _SB.DPTF.TSR0, 100, 94, 0, 0, 0, 0 },

/* CPU Throttle Effect on IA (TSR2) */
-	Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR2, 100, 60, 0, 0, 0, 0 },
+	Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR2, 100, 10, 0, 0, 0, 0 },

/* CPU Throttle Effect on GT (TSR3) */
-	Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR3, 100, 60, 0, 0, 0, 0 },
+	Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR3, 100, 10, 0, 0, 0, 0 },
 })

Name (MPPC, Package ()
@@ -68,8 +76,8 @@
    0x2,		/* Revision */
    Package () {	/* Power Limit 1 */
    	0,	/* PowerLimitIndex, 0 for Power Limit 1 */
-		8000,	/* PowerLimitMinimum */
-		15000,	/* PowerLimitMaximum */
+		7000,	/* PowerLimitMinimum */
+		9000,	/* PowerLimitMaximum */
    	28000,	/* TimeWindowMinimum */
    	28000,	/* TimeWindowMaximum */
    	250	/* StepSize */
diff --git a/src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h b/src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h
index 768987d..84e050e 100644
--- a/src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h
+++ b/src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h
@@ -18,4 +18,6 @@

#include <baseboard/ec.h>

+#define EC_ENABLE_MULTIPLE_DPTF_PROFILES
+
 #endif
diff --git a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
index ff6d817..8c7bb1f 100644
--- a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
+++ b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
@@ -2,6 +2,8 @@
    register "tdp_pl1_override" = "8"
    register "tdp_pl2_override" = "51"

+	register "tcc_offset" = "35"	# TCC of 65C
+
    register "SerialIoDevMode" = "{
    	[PchSerialIoIndexI2C0] = PchSerialIoPci,
    	[PchSerialIoIndexI2C1] = PchSerialIoPci,