Change in ...coreboot[master]: mb/google/sarien/variants/sarien: Update dptf.asl for EVT1
John Su has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31170
Change subject: mb/google/sarien/variants/sarien: Update dptf.asl for EVT1 ......................................................................
mb/google/sarien/variants/sarien: Update dptf.asl for EVT1
Follow thermal table (b:123383634 comment#1) for EVT1 tunning.
BUG=b:123383634 BRANCH=sarien TEST=Built and tested on sarien system
Change-Id: I22908e4bf39aedb8cf31a9060084f6f36bff56ca Signed-off-by: John Su john_su@compal.corp-partner.google.com --- M src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl 1 file changed, 15 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/31170/1
diff --git a/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl b/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl index d6a1274..66839a0 100644 --- a/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl +++ b/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl @@ -13,42 +13,42 @@ * GNU General Public License for more details. */
-#define DPTF_CPU_PASSIVE 80 +#define DPTF_CPU_PASSIVE 99 #define DPTF_CPU_CRITICAL 100
/* 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 71 +#define DPTF_TSR0_CRITICAL 100
/* 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 56 +#define DPTF_TSR1_CRITICAL 100
/* 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 90 +#define DPTF_TSR2_CRITICAL 100
#undef DPTF_ENABLE_FAN_CONTROL #undef DPTF_ENABLE_CHARGER
Name (DTRT, Package () { /* CPU Throttle Effect on CPU */ - Package () { _SB.PCI0.TCPU, _SB.PCI0.TCPU, 100, 10, 0, 0, 0, 0 }, + Package () { _SB.PCI0.TCPU, _SB.PCI0.TCPU, 5000, 10, 0, 0, 0, 0 },
/* CPU Throttle Effect on Skin (TSR0) */ - Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR0, 100, 600, 0, 0, 0, 0 }, + Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR0, 200, 10, 0, 0, 0, 0 },
/* CPU Throttle Effect on DDR (TSR1) */ - Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR1, 100, 90, 0, 0, 0, 0 }, + Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR1, 2000, 10, 0, 0, 0, 0 },
/* CPU Throttle Effect on Ambient (TSR2) */ - Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR2, 100, 600, 0, 0, 0, 0 }, + Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR2, 200, 10, 0, 0, 0, 0 }, })
Name (MPPC, Package () @@ -58,16 +58,16 @@ 0, /* PowerLimitIndex, 0 for Power Limit 1 */ 3000, /* PowerLimitMinimum */ 25000, /* PowerLimitMaximum */ - 28000, /* TimeWindowMinimum */ - 32000, /* TimeWindowMaximum */ + 10000, /* TimeWindowMinimum */ + 10000, /* TimeWindowMaximum */ 100 /* StepSize */ }, Package () { /* Power Limit 2 */ 1, /* PowerLimitIndex, 1 for Power Limit 2 */ - 15000, /* PowerLimitMinimum */ + 3000, /* PowerLimitMinimum */ 51000, /* PowerLimitMaximum */ 28000, /* TimeWindowMinimum */ - 32000, /* TimeWindowMaximum */ + 28000, /* TimeWindowMaximum */ 100 /* StepSize */ } })
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien/variants/sarien: Update dptf.asl for EVT1 ......................................................................
Patch Set 1: Code-Review+1
EricR Lai has uploaded a new patch set (#2) to the change originally created by John Su. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien: Update dptf.asl for EVT1 ......................................................................
mb/google/sarien: Update dptf.asl for EVT1
Follow thermal table (b:123383634 comment#1) for EVT1 tunning.
BUG=b:123383634 BRANCH=master TEST=Built and tested on sarien system
Change-Id: I22908e4bf39aedb8cf31a9060084f6f36bff56ca Signed-off-by: John Su john_su@compal.corp-partner.google.com --- M src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl 1 file changed, 15 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/31170/2
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien: Update dptf.asl for EVT1 ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/31170/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31170/2//COMMIT_MSG@7 PS2, Line 7: dptf.asl Update thermal configuration for DPTF
https://review.coreboot.org/#/c/31170/2//COMMIT_MSG@12 PS2, Line 12: BRANCH=master BRANCH is not needed here, all coreboot changes in master branch.
Hello Ivy Jian, Chris Zhou, EricR Lai, Sumeet R Pawnikar, Paul Menzel, Duncan Laurie, Van Chen, Frank Wu, Lijian Zhao, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31170
to look at the new patch set (#3).
Change subject: mb/google/sarien/variants/sarien: Update thermal configuration for DPTF ......................................................................
mb/google/sarien/variants/sarien: Update thermal configuration for DPTF
Follow thermal table (b:123383634 comment#1) for EVT1 tunning.
BUG=b:123383634 BRANCH=sarien TEST=Built and tested on sarien system
Change-Id: I22908e4bf39aedb8cf31a9060084f6f36bff56ca Signed-off-by: John Su john_su@compal.corp-partner.google.com --- M src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl 1 file changed, 15 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/31170/3
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien/variants/sarien: Update thermal configuration for DPTF ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/31170/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31170/2//COMMIT_MSG@7 PS2, Line 7: dptf.asl
Update thermal configuration for DPTF
Done
https://review.coreboot.org/#/c/31170/2//COMMIT_MSG@12 PS2, Line 12: BRANCH=master
BRANCH is not needed here, all coreboot changes in master branch.
Done
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien/variants/sarien: Update thermal configuration for DPTF ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31170/3/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl:
https://review.coreboot.org/#/c/31170/3/src/mainboard/google/sarien/variants... PS3, Line 16: 99 any reason for keeping this passive 99 ? If passive is at 99 than I do not see any point of keeping critical at 100. This might shutdown system abruptly in heavy load scenarios. There should be some temperature difference between passive and critical limits. I would suggest to keep passive 95 and keep critical 105.
EricR Lai has uploaded a new patch set (#4) to the change originally created by John Su. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien/variants/sarien: Update thermal configuration for DPTF ......................................................................
mb/google/sarien/variants/sarien: Update thermal configuration for DPTF
Follow thermal table (b:123383634 comment#1) for EVT1 tunning.
BUG=b:123383634 TEST=Built and tested on sarien system
Change-Id: I22908e4bf39aedb8cf31a9060084f6f36bff56ca Signed-off-by: John Su john_su@compal.corp-partner.google.com --- M src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl 1 file changed, 15 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/31170/4
Hello Ivy Jian, Chris Zhou, EricR Lai, Sumeet R Pawnikar, Paul Menzel, Duncan Laurie, Van Chen, Frank Wu, build bot (Jenkins), Lijian Zhao, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31170
to look at the new patch set (#5).
Change subject: mb/google/sarien/variants/sarien: Update thermal configuration for DPTF ......................................................................
mb/google/sarien/variants/sarien: Update thermal configuration for DPTF
Follow thermal table (b:123383634 comment#1) for EVT1 tunning.
BUG=b:123383634 TEST=Built and tested on sarien system
Change-Id: I22908e4bf39aedb8cf31a9060084f6f36bff56ca Signed-off-by: John Su john_su@compal.corp-partner.google.com --- M src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl 1 file changed, 16 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/31170/5
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien/variants/sarien: Update thermal configuration for DPTF ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31170/3/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl:
https://review.coreboot.org/#/c/31170/3/src/mainboard/google/sarien/variants... PS3, Line 16: 99
any reason for keeping this passive 99 ? […]
Done
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien/variants/sarien: Update thermal configuration for DPTF ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31170/3/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl:
https://review.coreboot.org/#/c/31170/3/src/mainboard/google/sarien/variants... PS3, Line 16: 99
Done
Please, explain why it's set to 99 ? Why not 95 or 90 ?
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien/variants/sarien: Update thermal configuration for DPTF ......................................................................
Patch Set 5:
Patch Set 5:
(1 comment)
Sumeet, Modified in Patch Set5.
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien/variants/sarien: Update thermal configuration for DPTF ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
(1 comment)
Sumeet, Modified in Patch Set5.
Hi Summet Could you help to review again? Thanks
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien/variants/sarien: Update thermal configuration for DPTF ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/31170/5/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl:
https://review.coreboot.org/#/c/31170/5/src/mainboard/google/sarien/variants... PS5, Line 42: 5000 I am not clear on this. Please, explain why this is such a high number.
https://review.coreboot.org/#/c/31170/5/src/mainboard/google/sarien/variants... PS5, Line 67: 3000 How did you arrive to this number ? I would suggest to keep this at least same as PL1 minimum i.e. 25W.
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien/variants/sarien: Update thermal configuration for DPTF ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/31170/5/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl:
https://review.coreboot.org/#/c/31170/5/src/mainboard/google/sarien/variants... PS5, Line 42: 5000
I am not clear on this. Please, explain why this is such a high number.
Setting 5000 for CPU Influence(mw), because this is our test result, due to CPU at performance behavior will arise to 51W, and it will meet to Tcc offset 5 & PSV95, so we need set CPU 5000 Influence(mw) to quickly pull down power, thanks.
https://review.coreboot.org/#/c/31170/5/src/mainboard/google/sarien/variants... PS5, Line 67: 3000
How did you arrive to this number ? I would suggest to keep this at least same as PL1 minimum i.e. […]
Set PL1 MIN 3000 is for skin test, when we test high temperature ambient, we want CPU power get lower to avoid system skin test fail, and 3000 is the best that our result, thanks.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien/variants/sarien: Update thermal configuration for DPTF ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31170 )
Change subject: mb/google/sarien/variants/sarien: Update thermal configuration for DPTF ......................................................................
mb/google/sarien/variants/sarien: Update thermal configuration for DPTF
Follow thermal table (b:123383634 comment#1) for EVT1 tunning.
BUG=b:123383634 TEST=Built and tested on sarien system
Change-Id: I22908e4bf39aedb8cf31a9060084f6f36bff56ca Signed-off-by: John Su john_su@compal.corp-partner.google.com Reviewed-on: https://review.coreboot.org/c/31170 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com --- M src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl 1 file changed, 16 insertions(+), 16 deletions(-)
Approvals: build bot (Jenkins): Verified Sumeet R Pawnikar: Looks good to me, approved
diff --git a/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl b/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl index d6a1274..be052f8 100644 --- a/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl +++ b/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/dptf.asl @@ -13,42 +13,42 @@ * GNU General Public License for more details. */
-#define DPTF_CPU_PASSIVE 80 -#define DPTF_CPU_CRITICAL 100 +#define DPTF_CPU_PASSIVE 95 +#define DPTF_CPU_CRITICAL 105
/* 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 71 +#define DPTF_TSR0_CRITICAL 100
/* 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 56 +#define DPTF_TSR1_CRITICAL 100
/* 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 90 +#define DPTF_TSR2_CRITICAL 100
#undef DPTF_ENABLE_FAN_CONTROL #undef DPTF_ENABLE_CHARGER
Name (DTRT, Package () { /* CPU Throttle Effect on CPU */ - Package () { _SB.PCI0.TCPU, _SB.PCI0.TCPU, 100, 10, 0, 0, 0, 0 }, + Package () { _SB.PCI0.TCPU, _SB.PCI0.TCPU, 5000, 10, 0, 0, 0, 0 },
/* CPU Throttle Effect on Skin (TSR0) */ - Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR0, 100, 600, 0, 0, 0, 0 }, + Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR0, 200, 10, 0, 0, 0, 0 },
/* CPU Throttle Effect on DDR (TSR1) */ - Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR1, 100, 90, 0, 0, 0, 0 }, + Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR1, 2000, 10, 0, 0, 0, 0 },
/* CPU Throttle Effect on Ambient (TSR2) */ - Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR2, 100, 600, 0, 0, 0, 0 }, + Package () { _SB.PCI0.TCPU, _SB.DPTF.TSR2, 200, 10, 0, 0, 0, 0 }, })
Name (MPPC, Package () @@ -58,16 +58,16 @@ 0, /* PowerLimitIndex, 0 for Power Limit 1 */ 3000, /* PowerLimitMinimum */ 25000, /* PowerLimitMaximum */ - 28000, /* TimeWindowMinimum */ - 32000, /* TimeWindowMaximum */ + 10000, /* TimeWindowMinimum */ + 10000, /* TimeWindowMaximum */ 100 /* StepSize */ }, Package () { /* Power Limit 2 */ 1, /* PowerLimitIndex, 1 for Power Limit 2 */ - 15000, /* PowerLimitMinimum */ + 3000, /* PowerLimitMinimum */ 51000, /* PowerLimitMaximum */ 28000, /* TimeWindowMinimum */ - 32000, /* TimeWindowMaximum */ + 28000, /* TimeWindowMaximum */ 100 /* StepSize */ } })
-
EricR Lai (Code Review) -
John Su (Code Review)
-
Lijian Zhao (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Sumeet R Pawnikar (Code Review)