David Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Update PL2 power limit value ......................................................................
mb/google/hatch/var/kindred: Update PL2 power limit value
Update PL2 power limit value to 29W.
BUG=b:140127035 TEST=Build and Boot kindred DVT
Change-Id: Iaefab3011224d6dcf561b437bd34f54a62cf1287 Signed-off-by: David Wu david_wu@quanta.corp-partner.google.com --- M src/mainboard/google/hatch/variants/kindred/include/variant/acpi/dptf.asl M src/mainboard/google/hatch/variants/kindred/overridetree.cb 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/37823/1
diff --git a/src/mainboard/google/hatch/variants/kindred/include/variant/acpi/dptf.asl b/src/mainboard/google/hatch/variants/kindred/include/variant/acpi/dptf.asl index 43c1b08..aa0ea78 100644 --- a/src/mainboard/google/hatch/variants/kindred/include/variant/acpi/dptf.asl +++ b/src/mainboard/google/hatch/variants/kindred/include/variant/acpi/dptf.asl @@ -125,7 +125,7 @@ Package () { /* Power Limit 2 */ 1, /* PowerLimitIndex, 1 for Power Limit 2 */ 15000, /* PowerLimitMinimum */ - 51000, /* PowerLimitMaximum */ + 29000, /* PowerLimitMaximum */ 28000, /* TimeWindowMinimum */ 32000, /* TimeWindowMaximum */ 1000 /* StepSize */ diff --git a/src/mainboard/google/hatch/variants/kindred/overridetree.cb b/src/mainboard/google/hatch/variants/kindred/overridetree.cb index 1122609..1c99200 100644 --- a/src/mainboard/google/hatch/variants/kindred/overridetree.cb +++ b/src/mainboard/google/hatch/variants/kindred/overridetree.cb @@ -1,6 +1,6 @@ chip soc/intel/cannonlake register "tdp_pl1_override" = "15" - register "tdp_pl2_override" = "51" + register "tdp_pl2_override" = "29"
register "SerialIoDevMode" = "{ [PchSerialIoIndexI2C0] = PchSerialIoPci,
Peter Ou has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Update PL2 power limit value ......................................................................
Patch Set 1:
confirm with customer about PL2 setting will change to 29000mW for Kindred.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Update PL2 power limit value ......................................................................
Patch Set 1: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Update PL2 power limit value ......................................................................
Patch Set 1:
(3 comments)
Why does the value need to be specified in two places?
https://review.coreboot.org/c/coreboot/+/37823/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37823/1//COMMIT_MSG@7 PS1, Line 7: Update Decrease
https://review.coreboot.org/c/coreboot/+/37823/1//COMMIT_MSG@9 PS1, Line 9: Update PL2 power limit value to 29W.
Decrease PL2 power limit value from 51 W to 29 W as recommended by …
https://review.coreboot.org/c/coreboot/+/37823/1//COMMIT_MSG@12 PS1, Line 12: Boot boot
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Update PL2 power limit value ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37823/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kindred/include/variant/acpi/dptf.asl:
https://review.coreboot.org/c/coreboot/+/37823/1/src/mainboard/google/hatch/... PS1, Line 128: 29 This lower value might reduce the performance. Did you check any performance analysis with this low value ? Is this new value based on thermal design ?
I do not have access to bug. Can someone please add me to that ? I would like to go through the bug details and understand the background behind this change. Thanks.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Update PL2 power limit value ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37823/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kindred/include/variant/acpi/dptf.asl:
https://review.coreboot.org/c/coreboot/+/37823/1/src/mainboard/google/hatch/... PS1, Line 128: 29
This lower value might reduce the performance. […]
I'll add you Sumeet. This is a large drop in the PL2 limit. I don't see any rationale for this in the bug; this will almost certainly cause a drop in performance.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Update PL2 power limit value ......................................................................
Patch Set 1:
Patch Set 1:
(3 comments)
Why does the value need to be specified in two places?
One is the initial value to be programmed by FSP (devicetree), the other is exposed to DPTF.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Update PL2 power limit value ......................................................................
Patch Set 1:
(1 comment)
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37823/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kindred/include/variant/acpi/dptf.asl:
https://review.coreboot.org/c/coreboot/+/37823/1/src/mainboard/google/hatch/... PS1, Line 128: 29
I'll add you Sumeet. This is a large drop in the PL2 limit. […]
Thanks Tim for adding me. I have requested Quanta to share details and thermal results with this new value on bug. Is it possible to hold this until we get clear information on this change if we are not blocking any milestone/release ?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Update PL2 power limit value ......................................................................
Patch Set 1: Code-Review-1
-1 just to indicate we need to wait until we get more information from the ODM
Paul Fagerburg has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Update PL2 power limit value ......................................................................
Removed Code-Review+2 by Paul Fagerburg pfagerburg@chromium.org
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Update PL2 power limit value ......................................................................
Patch Set 1:
Tim has more expertise in this area than me, so I'm removing my vote until we get it sorted out per Tim's comment.
Hello Paul Fagerburg, Sumeet R Pawnikar, Philip Chen, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37823
to look at the new patch set (#2).
Change subject: mb/google/hatch/var/kindred: Decrease PL2 power limit value ......................................................................
mb/google/hatch/var/kindred: Decrease PL2 power limit value
Decrease PL2 power limit value from 51W to 29W as recommended by thermal team.
BUG=b:140127035 TEST=Build and boot kindred DVT
Change-Id: Iaefab3011224d6dcf561b437bd34f54a62cf1287 Signed-off-by: David Wu david_wu@quanta.corp-partner.google.com --- M src/mainboard/google/hatch/variants/kindred/include/variant/acpi/dptf.asl M src/mainboard/google/hatch/variants/kindred/overridetree.cb 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/37823/2
David Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Decrease PL2 power limit value ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37823/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37823/1//COMMIT_MSG@7 PS1, Line 7: Update
Decrease
Done
https://review.coreboot.org/c/coreboot/+/37823/1//COMMIT_MSG@9 PS1, Line 9: Update PL2 power limit value to 29W.
Decrease PL2 power limit value from 51 W to 29 W as recommended by …
Done
https://review.coreboot.org/c/coreboot/+/37823/1//COMMIT_MSG@12 PS1, Line 12: Boot
boot
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Decrease PL2 power limit value ......................................................................
Patch Set 2: Code-Review+2
Hmm, the test results definitely surprised me; the performance impact looks very small.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Decrease PL2 power limit value ......................................................................
Patch Set 2:
I have added few comments c#24 and c#25 on bug requesting more details and information on the shared thermal test results.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Decrease PL2 power limit value ......................................................................
Patch Set 2: -Code-Review
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Decrease PL2 power limit value ......................................................................
Patch Set 2:
Hi David, Are you planning to submit new set for this CL as per comment #25 on bug? Thanks, Sumeet.
Peter Ou has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Decrease PL2 power limit value ......................................................................
Patch Set 2:
confirm with customer about PL2 setting will keep 51W for Kindred.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Decrease PL2 power limit value ......................................................................
Patch Set 2:
Patch Set 2:
confirm with customer about PL2 setting will keep 51W for Kindred.
Ok so should this change be abandoned?
David Wu has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37823 )
Change subject: mb/google/hatch/var/kindred: Decrease PL2 power limit value ......................................................................
Abandoned
confirm with customer about PL2 setting will keep 51W for Kindred.
-
David Wu (Code Review) -
Paul Fagerburg (Code Review)
-
Paul Menzel (Code Review)
-
Peter Ou (Code Review)
-
Sumeet R Pawnikar (Code Review)
-
Tim Wawrzynczak (Code Review)