Change in ...coreboot[master]: mb/google/sarien/variants/arcada: Update thermal configuration for DPTF

[Casper Chang (Code Review)]

Casper Chang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31331

Change subject: mb/google/sarien/variants/arcada: Update thermal configuration for DPTF
......................................................................

mb/google/sarien/variants/arcada: Update thermal configuration for DPTF

Update dptf for arcada EVT.

BUG=b:123924662
TEST=Built and tested on arcada system

Change-Id: Ieed8021b83776fdb6320ff89b57c8d2747667fd5
---
M src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl
1 file changed, 11 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/31331/1

diff --git a/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl b/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl
index f54a7c1..6fa06c7 100644
--- a/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl
+++ b/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl
@@ -13,26 +13,26 @@
  * GNU General Public License for more details.
  */

-#define DPTF_CPU_PASSIVE	80
-#define DPTF_CPU_CRITICAL	100
+#define DPTF_CPU_PASSIVE	96
+#define DPTF_CPU_CRITICAL	103

/* Skin Sensor for CPU VR temperature monitor */
 #define DPTF_TSR0_SENSOR_ID	1
 #define DPTF_TSR0_SENSOR_NAME	"Skin"
-#define DPTF_TSR0_PASSIVE	55
-#define DPTF_TSR0_CRITICAL	70
+#define DPTF_TSR0_PASSIVE	56
+#define DPTF_TSR0_CRITICAL	108

/* Memory Sensor for DDR temperature monitor */
 #define DPTF_TSR1_SENSOR_ID	2
 #define DPTF_TSR1_SENSOR_NAME	"DDR"
-#define DPTF_TSR1_PASSIVE	55
-#define DPTF_TSR1_CRITICAL	80
+#define DPTF_TSR1_PASSIVE	70
+#define DPTF_TSR1_CRITICAL	95

/* M.2 Sensor for Ambient temperature monitor */
 #define DPTF_TSR2_SENSOR_ID	3
 #define DPTF_TSR2_SENSOR_NAME	"Ambient"
-#define DPTF_TSR2_PASSIVE	55
-#define DPTF_TSR2_CRITICAL	70
+#define DPTF_TSR2_PASSIVE	50
+#define DPTF_TSR2_CRITICAL	95

#undef DPTF_ENABLE_FAN_CONTROL
 #undef DPTF_ENABLE_CHARGER
@@ -57,9 +57,9 @@
    Package () {	/* Power Limit 1 */
    	0,	/* PowerLimitIndex, 0 for Power Limit 1 */
    	3000,	/* PowerLimitMinimum */
-		25000,	/* PowerLimitMaximum */
+		21000,	/* PowerLimitMaximum */
    	28000,	/* TimeWindowMinimum */
-		32000,	/* TimeWindowMaximum */
+		28000,	/* TimeWindowMaximum */
    	100	/* StepSize */
    },
    Package () {	/* Power Limit 2 */
@@ -67,7 +67,7 @@
    	15000,	/* PowerLimitMinimum */
    	51000,	/* PowerLimitMaximum */
    	28000,	/* TimeWindowMinimum */
-		32000,	/* TimeWindowMaximum */
+		28000,	/* TimeWindowMaximum */
    	100	/* StepSize */
    }
 })

[Casper Chang (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#2).

Change subject: mb/google/sarien/variants/arcada: Update thermal configuration for DPTF
......................................................................

mb/google/sarien/variants/arcada: Update thermal configuration for DPTF

Update dptf for arcada EVT.

BUG=b:123924662
TEST=Built and tested on arcada system

Signed-off-by: Casper Chang 
Change-Id: Ieed8021b83776fdb6320ff89b57c8d2747667fd5
---
M src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl
1 file changed, 11 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/31331/2

[Sumeet R Pawnikar (Code Review)]

Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31331 )

Change subject: mb/google/sarien/variants/arcada: Update thermal configuration for DPTF
......................................................................

Patch Set 2:

(5 comments)

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
File src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl:

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
PS2, Line 23: 108
Please, explain why it's set such a high value 108 for Skin sensor ?

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
PS2, Line 35: 95
Please, explain why it's set such a high value 95 for Ambient sensor ?

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
PS2, Line 60: 21
What's the reason for reducing it from previous value of 25W to this new value as 21W ? Did you observe any issue with previous value ?

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
PS2, Line 62: 28000
How did you arrive to this particular number ?

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
PS2, Line 70: 28000
same query as previous one.

[Casper Chang (Code Review)]

Casper Chang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31331 )

Change subject: mb/google/sarien/variants/arcada: Update thermal configuration for DPTF
......................................................................

Patch Set 2:

(5 comments)

Please kindly check the comment.

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
File src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl:

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
PS2, Line 23: 108
> Please, explain why it's set such a high value 108 for Skin sensor ?

The purpose of TSR0 and TSR2 critical would be preventing from serious issue like casing melting. That’s the reason we set such high value for sensors and it’s based on bandon’s (leading Windows project) experience.

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
PS2, Line 35: 95
> Please, explain why it's set such a high value 95 for Ambient sensor ?

Same as previous one.

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
PS2, Line 60: 21
> What's the reason for reducing it from previous value of 25W to this new value as 21W ? Did you obse […]

The value was leveraged from Bandon (leading Windows project), the 21 W would be max dynamic PL1 with same CPU type and thermal solutions.

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
PS2, Line 62: 28000
> How did you arrive to this particular number ?

same as previous one.

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
PS2, Line 70: 28000
> same query as previous one.

same as previous one.

[Casper Chang (Code Review)]

Casper Chang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31331 )

Change subject: mb/google/sarien/variants/arcada: Update thermal configuration for DPTF
......................................................................

Patch Set 2:

Hi Sumeet, May we have your comment? 
Thanks.

[Sumeet R Pawnikar (Code Review)]

Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31331 )

Change subject: mb/google/sarien/variants/arcada: Update thermal configuration for DPTF
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
File src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl:

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
PS2, Line 23: 108
> The purpose of TSR0 and TSR2 critical would be preventing from serious issue like casing melting. […]

If intention here to prevent any high temperature serious issue, in that case, you should set this critical value to lower value <100. I not clear on keeping this such a high value, how will you prevent serious issue as you mentioned above.

[Casper Chang (Code Review)]

Casper Chang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31331 )

Change subject: mb/google/sarien/variants/arcada: Update thermal configuration for DPTF
......................................................................

Patch Set 2:

(1 comment)

Please check the comment.

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
File src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl:

https://review.coreboot.org/#/c/31331/2/src/mainboard/google/sarien/variants... 
PS2, Line 23: 108
> If intention here to prevent any high temperature serious issue, in that case, you should set this c […]

The thermal sensor is built on MB (have distance from casing) and due to different location it will have different heat shock. If it locate nearby CPU, value lower than 100 degree will easily be triggered when CPU turbo.
This value is based on Bandon (leading Windows project) setting, for Arcada, we will fine tune the value at the next stage.

[Sumeet R Pawnikar (Code Review)]

Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31331 )

Change subject: mb/google/sarien/variants/arcada: Update thermal configuration for DPTF
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31331 )

Change subject: mb/google/sarien/variants/arcada: Update thermal configuration for DPTF
......................................................................

mb/google/sarien/variants/arcada: Update thermal configuration for DPTF

Update dptf for arcada EVT.

BUG=b:123924662
TEST=Built and tested on arcada system

Signed-off-by: Casper Chang 
Change-Id: Ieed8021b83776fdb6320ff89b57c8d2747667fd5
Reviewed-on: https://review.coreboot.org/c/31331
Tested-by: build bot (Jenkins) 
Reviewed-by: Sumeet R Pawnikar 
---
M src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl
1 file changed, 11 insertions(+), 11 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Sumeet R Pawnikar: Looks good to me, approved

diff --git a/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl b/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl
index f54a7c1..6fa06c7 100644
--- a/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl
+++ b/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/dptf.asl
@@ -13,26 +13,26 @@
  * GNU General Public License for more details.
  */

-#define DPTF_CPU_PASSIVE	80
-#define DPTF_CPU_CRITICAL	100
+#define DPTF_CPU_PASSIVE	96
+#define DPTF_CPU_CRITICAL	103

/* Skin Sensor for CPU VR temperature monitor */
 #define DPTF_TSR0_SENSOR_ID	1
 #define DPTF_TSR0_SENSOR_NAME	"Skin"
-#define DPTF_TSR0_PASSIVE	55
-#define DPTF_TSR0_CRITICAL	70
+#define DPTF_TSR0_PASSIVE	56
+#define DPTF_TSR0_CRITICAL	108

/* Memory Sensor for DDR temperature monitor */
 #define DPTF_TSR1_SENSOR_ID	2
 #define DPTF_TSR1_SENSOR_NAME	"DDR"
-#define DPTF_TSR1_PASSIVE	55
-#define DPTF_TSR1_CRITICAL	80
+#define DPTF_TSR1_PASSIVE	70
+#define DPTF_TSR1_CRITICAL	95

/* M.2 Sensor for Ambient temperature monitor */
 #define DPTF_TSR2_SENSOR_ID	3
 #define DPTF_TSR2_SENSOR_NAME	"Ambient"
-#define DPTF_TSR2_PASSIVE	55
-#define DPTF_TSR2_CRITICAL	70
+#define DPTF_TSR2_PASSIVE	50
+#define DPTF_TSR2_CRITICAL	95

#undef DPTF_ENABLE_FAN_CONTROL
 #undef DPTF_ENABLE_CHARGER
@@ -57,9 +57,9 @@
    Package () {	/* Power Limit 1 */
    	0,	/* PowerLimitIndex, 0 for Power Limit 1 */
    	3000,	/* PowerLimitMinimum */
-		25000,	/* PowerLimitMaximum */
+		21000,	/* PowerLimitMaximum */
    	28000,	/* TimeWindowMinimum */
-		32000,	/* TimeWindowMaximum */
+		28000,	/* TimeWindowMaximum */
    	100	/* StepSize */
    },
    Package () {	/* Power Limit 2 */
@@ -67,7 +67,7 @@
    	15000,	/* PowerLimitMinimum */
    	51000,	/* PowerLimitMaximum */
    	28000,	/* TimeWindowMinimum */
-		32000,	/* TimeWindowMaximum */
+		28000,	/* TimeWindowMaximum */
    	100	/* StepSize */
    }
 })