Change in coreboot[master]: mb/google/dedede/variants/drawcia: Update Power Limit2 minimum value
Sumeet R Pawnikar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47154 )
Change subject: mb/google/dedede/variants/drawcia: Update Power Limit2 minimum value ......................................................................
mb/google/dedede/variants/drawcia: Update Power Limit2 minimum value
Update Power Limit2 minimum value to the same as maximum value.
BUG=None BRANCH=None TEST=Build and test on drawcia system
Change-Id: I7ecf1ffcc7871192ebe18eb8c3c3fd3e1193721e Signed-off-by: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com --- M src/mainboard/google/dedede/variants/baseboard/devicetree.cb M src/mainboard/google/dedede/variants/drawcia/overridetree.cb 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/47154/1
diff --git a/src/mainboard/google/dedede/variants/baseboard/devicetree.cb b/src/mainboard/google/dedede/variants/baseboard/devicetree.cb index a5ff128..ce28123 100644 --- a/src/mainboard/google/dedede/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/dedede/variants/baseboard/devicetree.cb @@ -212,7 +212,7 @@ .granularity = 200, }, .pl2 = { - .min_power = 6000, + .min_power = 20000, .max_power = 20000, .time_window_min = 1 * MSECS_PER_SEC, .time_window_max = 1 * MSECS_PER_SEC, diff --git a/src/mainboard/google/dedede/variants/drawcia/overridetree.cb b/src/mainboard/google/dedede/variants/drawcia/overridetree.cb index 5942121..202e0f4 100644 --- a/src/mainboard/google/dedede/variants/drawcia/overridetree.cb +++ b/src/mainboard/google/dedede/variants/drawcia/overridetree.cb @@ -115,7 +115,7 @@ .granularity = 200, }, .pl2 = { - .min_power = 6000, + .min_power = 20000, .max_power = 20000, .time_window_min = 1 * MSECS_PER_SEC, .time_window_max = 1 * MSECS_PER_SEC,
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47154 )
Change subject: mb/google/dedede/variants/drawcia: Update Power Limit2 minimum value ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG@7 PS1, Line 7: drawcia It is not just drawcia that is being changed.
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG@9 PS1, Line 9: Update Power Limit2 minimum value to the same as maximum value. Why? It would be good to capture the reason/motivation here.
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG@13 PS1, Line 13: test How? And what was the difference in behavior?
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47154 )
Change subject: mb/google/dedede/variants/drawcia: Update Power Limit2 minimum value ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG@7 PS1, Line 7: drawcia
It is not just drawcia that is being changed.
In this patch, modified for drawcia and baseboard. Please, suggest the appropriate one here. I checked all other JSL variants for this PL2 values. For madoo variant looks fine it's already 20W.
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG@9 PS1, Line 9: Update Power Limit2 minimum value to the same as maximum value.
Why? It would be good to capture the reason/motivation here.
The reason is that DTT (DPTF) does not throttle PL2, so need to set both max and min the same value. I see other variant Madoo sets it the same.
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG@13 PS1, Line 13: test
How? And what was the difference in behavior?
Using DTT GUI Tool verified these values. There should not be any difference in behavior.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47154 )
Change subject: mb/google/dedede/variants/drawcia: Update Power Limit2 minimum value ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG@7 PS1, Line 7: drawcia
In this patch, modified for drawcia and baseboard. Please, suggest the appropriate one here. […]
I just meant, it would be good to outline in the commit message all the boards that are impacted because of the change.
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG@9 PS1, Line 9: Update Power Limit2 minimum value to the same as maximum value.
The reason is that DTT (DPTF) does not throttle PL2, so need to set both max and min the same value. […]
Can you please add that to commit message?
Just curious: DTT does not throttle or do you want DTT to not throttle? Because if you set min different than max, then DTT would adjust PL2 within that range, right?
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG@13 PS1, Line 13: test
Using DTT GUI Tool verified these values. There should not be any difference in behavior.
Ack
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47154 )
Change subject: mb/google/dedede/variants/drawcia: Update Power Limit2 minimum value ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG@7 PS1, Line 7: drawcia
I just meant, it would be good to outline in the commit message all the boards that are impacted bec […]
Sure, I will add details in the commit message for this.
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG@9 PS1, Line 9: Update Power Limit2 minimum value to the same as maximum value.
Can you please add that to commit message? […]
DTT does not throttle PL2 based on temperature thresholds.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Tim Wawrzynczak, Paul Menzel, Aaron Durbin, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47154
to look at the new patch set (#2).
Change subject: mb/google/dedede/variants: Update Power Limit2 minimum value ......................................................................
mb/google/dedede/variants: Update Power Limit2 minimum value
Update Power Limit2 (PL2) minimum value to the same as maximum value for dedede variants (baseboard and drawcia). For madoo variant, PL2 minimum value already set the same as PL2 maximum value. DTT does not throttle PL2, so this minimum value change here does not impact any existing behavior on the system.
BUG=None BRANCH=None TEST=Build and test on drawcia system
Change-Id: I7ecf1ffcc7871192ebe18eb8c3c3fd3e1193721e Signed-off-by: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com --- M src/mainboard/google/dedede/variants/baseboard/devicetree.cb M src/mainboard/google/dedede/variants/drawcia/overridetree.cb 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/47154/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47154 )
Change subject: mb/google/dedede/variants: Update Power Limit2 minimum value ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47154/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47154/2//COMMIT_MSG@10 PS2, Line 10: madoo What about drawcia and magalor?
https://review.coreboot.org/c/coreboot/+/47154/2//COMMIT_MSG@10 PS2, Line 10: baseboard Rather than saying baseboard, it would be better to list all variants that are impacted i.e. boten, waddledee, waddledoo, metaknight, wheelie.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47154 )
Change subject: mb/google/dedede/variants: Update Power Limit2 minimum value ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47154/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47154/2//COMMIT_MSG@10 PS2, Line 10: madoo
What about drawcia and magalor?
I have done changes for drawcia in this CL. For magolor, it has PL2 max as 12W and PL2 min as 7W. Not sure, why PL2 max is 12W here for magolor variant. So, firstly would like to discuss with original submitter of the CL on why PL2 max is 12W to understand it better. Later, will submit CL for magolor to make both PL2 max and min the same value. OR do you suggest to change the PL2 min to 12W here for magolor the same as PL2 max ?
https://review.coreboot.org/c/coreboot/+/47154/2//COMMIT_MSG@10 PS2, Line 10: baseboard
Rather than saying baseboard, it would be better to list all variants that are impacted i.e. […]
Under dedede, I checked all the variants. Only baseboard and drawcia needs this change. Other variants like boten, waddledee, waddledoo, metaknight, wheelie are not having these DTT entries.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47154 )
Change subject: mb/google/dedede/variants: Update Power Limit2 minimum value ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47154/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47154/2//COMMIT_MSG@10 PS2, Line 10: madoo
I have done changes for drawcia in this CL. […]
Ack. I think it is okay to leave magolor in this CL as long as you are following up on it.
https://review.coreboot.org/c/coreboot/+/47154/2//COMMIT_MSG@10 PS2, Line 10: baseboard
Under dedede, I checked all the variants. Only baseboard and drawcia needs this change. […]
I meant that boten, waddledee, waddledoo, metaknight, wheelie all use the DTT entry from baseboard devicetree since there is no override present for it. Hence, the ask to add to commit message that these variants are impacted by the change.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Tim Wawrzynczak, Paul Menzel, Aaron Durbin, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47154
to look at the new patch set (#3).
Change subject: mb/google/dedede/variants: Update Power Limit2 minimum value ......................................................................
mb/google/dedede/variants: Update Power Limit2 minimum value
Update Power Limit2 (PL2) minimum value to the same as maximum value for dedede variants (baseboard and drawcia). Currently, variants like boten, waddledee, waddledoo, metaknight and wheelie uses the DTT entries from baseboard devicetree since there is no override present for these variants. So, these variants will also reflect this change of PL1 minimum value. For madoo variant, PL2 minimum value already set the same as PL2 maximum value. DTT does not throttle PL2, so this minimum value change here does not impact any existing behavior on the system.
BUG=None BRANCH=None TEST=Build and test on drawcia system
Change-Id: I7ecf1ffcc7871192ebe18eb8c3c3fd3e1193721e Signed-off-by: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com --- M src/mainboard/google/dedede/variants/baseboard/devicetree.cb M src/mainboard/google/dedede/variants/drawcia/overridetree.cb 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/47154/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47154 )
Change subject: mb/google/dedede/variants: Update Power Limit2 minimum value ......................................................................
Patch Set 3: Code-Review+2
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47154 )
Change subject: mb/google/dedede/variants: Update Power Limit2 minimum value ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG@7 PS1, Line 7: drawcia
Sure, I will add details in the commit message for this.
Ack
https://review.coreboot.org/c/coreboot/+/47154/1//COMMIT_MSG@9 PS1, Line 9: Update Power Limit2 minimum value to the same as maximum value.
DTT does not throttle PL2 based on temperature thresholds.
Ack
https://review.coreboot.org/c/coreboot/+/47154/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47154/2//COMMIT_MSG@10 PS2, Line 10: baseboard
I meant that boten, waddledee, waddledoo, metaknight, wheelie all use the DTT entry from baseboard […]
Done
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47154 )
Change subject: mb/google/dedede/variants: Update Power Limit2 minimum value ......................................................................
mb/google/dedede/variants: Update Power Limit2 minimum value
Update Power Limit2 (PL2) minimum value to the same as maximum value for dedede variants (baseboard and drawcia). Currently, variants like boten, waddledee, waddledoo, metaknight and wheelie uses the DTT entries from baseboard devicetree since there is no override present for these variants. So, these variants will also reflect this change of PL1 minimum value. For madoo variant, PL2 minimum value already set the same as PL2 maximum value. DTT does not throttle PL2, so this minimum value change here does not impact any existing behavior on the system.
BUG=None BRANCH=None TEST=Build and test on drawcia system
Change-Id: I7ecf1ffcc7871192ebe18eb8c3c3fd3e1193721e Signed-off-by: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47154 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/dedede/variants/baseboard/devicetree.cb M src/mainboard/google/dedede/variants/drawcia/overridetree.cb 2 files changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/mainboard/google/dedede/variants/baseboard/devicetree.cb b/src/mainboard/google/dedede/variants/baseboard/devicetree.cb index a5ff128..ce28123 100644 --- a/src/mainboard/google/dedede/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/dedede/variants/baseboard/devicetree.cb @@ -212,7 +212,7 @@ .granularity = 200, }, .pl2 = { - .min_power = 6000, + .min_power = 20000, .max_power = 20000, .time_window_min = 1 * MSECS_PER_SEC, .time_window_max = 1 * MSECS_PER_SEC, diff --git a/src/mainboard/google/dedede/variants/drawcia/overridetree.cb b/src/mainboard/google/dedede/variants/drawcia/overridetree.cb index 5942121..202e0f4 100644 --- a/src/mainboard/google/dedede/variants/drawcia/overridetree.cb +++ b/src/mainboard/google/dedede/variants/drawcia/overridetree.cb @@ -115,7 +115,7 @@ .granularity = 200, }, .pl2 = { - .min_power = 6000, + .min_power = 20000, .max_power = 20000, .time_window_min = 1 * MSECS_PER_SEC, .time_window_max = 1 * MSECS_PER_SEC,
-
Furquan Shaikh (Code Review) -
Patrick Georgi (Code Review)
-
Sumeet R Pawnikar (Code Review)