Peichao Li has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
mb/google/kahlee/treeya: Update STAPM parameters for Treeya
Change stapm percentage to 80 and time to 200 second make it meet Lenovo spec and pass CTS respectively.
BUG=b:147762619 TEST=build firmware and install it to DUT and run fishbowl 1000, check temperature whether meets spec.
Signed-off-by: Peichao Wang peichao.wang@bitland.corp-partner.google.com Change-Id: I6a2f059fbd5c89f897cfb46d1f7a82b0923edb17 --- M src/mainboard/google/kahlee/variants/treeya/devicetree.cb 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/38443/1
diff --git a/src/mainboard/google/kahlee/variants/treeya/devicetree.cb b/src/mainboard/google/kahlee/variants/treeya/devicetree.cb index e8477ee..9feee29 100644 --- a/src/mainboard/google/kahlee/variants/treeya/devicetree.cb +++ b/src/mainboard/google/kahlee/variants/treeya/devicetree.cb @@ -20,8 +20,8 @@ register "dram_clear_on_reset" = "DRAM_CONTENTS_KEEP" register "uma_mode" = "UMAMODE_SPECIFIED_SIZE" register "uma_size" = "16 * MiB" - register "stapm_percent" = "68" - register "stapm_time_ms" = "900000" + register "stapm_percent" = "80" + register "stapm_time_ms" = "2000000" register "stapm_power_mw" = "7800"
# Enable I2C0 for audio, USB3 hub at 400kHz
Hello build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38443
to look at the new patch set (#2).
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
mb/google/kahlee/treeya: Update STAPM parameters for Treeya
Change stapm percentage to 80 and time to 200 second make it meet Lenovo spec and pass CTS respectively.
BUG=b:147333429 TEST=build firmware and install it to DUT and run fishbowl 1000, check temperature whether meets spec.
Signed-off-by: Peichao Wang peichao.wang@bitland.corp-partner.google.com Change-Id: I6a2f059fbd5c89f897cfb46d1f7a82b0923edb17 --- M src/mainboard/google/kahlee/variants/treeya/devicetree.cb 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/38443/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@9 PS2, Line 9: second seconds
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@9 PS2, Line 9: 200 second 3 + x minutes? The value is in ms, and you change it to 2000 seconds.
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@10 PS2, Line 10: it meet Fits on line above.
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@11 PS2, Line 11: What is the current error with fishbowl 1000? How much does the temperature rise?
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@14 PS2, Line 14: check Fits on line above.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@10 PS2, Line 10: it meet
Fits on line above.
We need 2 lines for this comment anyway, Let's not quibble about the placement of a single word.
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@11 PS2, Line 11:
What is the current error with fishbowl 1000? How much does the temperature rise?
Fishbowl is just being used to raise the temperature - Hopefully there's no error.
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@14 PS2, Line 14: check
Fits on line above.
Same as above, this is a quibble. Let's not make contributors jump through arbitrary hoops to submit changes.
https://review.coreboot.org/c/coreboot/+/38443/2/src/mainboard/google/kahlee... File src/mainboard/google/kahlee/variants/treeya/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38443/2/src/mainboard/google/kahlee... PS2, Line 25: register "stapm_power_mw" = "7800" According to the comment, you wanted to set it to 200 seconds, but this looks like 2000 seconds or just over half an hour.
Hello build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38443
to look at the new patch set (#3).
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
mb/google/kahlee/treeya: Update STAPM parameters for Treeya
Change stapm percentage to 80 and time to 2000 seconds make DUT meets Lenovo spec and pass CTS respectively.
BUG=b:147762619 TEST=build firmware and install it to DUT and run CTS relevant test, check temperature whether meets spec.
Signed-off-by: Peichao Wang peichao.wang@bitland.corp-partner.google.com Change-Id: I6a2f059fbd5c89f897cfb46d1f7a82b0923edb17 --- M src/mainboard/google/kahlee/variants/treeya/devicetree.cb 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/38443/3
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@9 PS2, Line 9: second
seconds
Done
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@9 PS2, Line 9: 200 second
3 + x minutes? The value is in ms, and you change it to 2000 seconds.
Done
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@10 PS2, Line 10: it meet
We need 2 lines for this comment anyway, Let's not quibble about the placement of a single word.
Done
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@11 PS2, Line 11:
Fishbowl is just being used to raise the temperature - Hopefully there's no error.
Dear Paul, Martin, origin value impact DUT pass CTS youtube test like ticket 147333429. We need modify this value pass the CTS and meet Lenovo '62368 thermal spec'. So modify them. At this moment, we make sure everything is ok before uprev this cl. Many thanks!
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@14 PS2, Line 14: check
Same as above, this is a quibble. […]
Done
Hello build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38443
to look at the new patch set (#4).
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
mb/google/kahlee/treeya: Update STAPM parameters for Treeya
Change stapm percentage to 80 and time to 2000 seconds make DUT meets Lenovo spec and pass CTS respectively.
BUG=b:147333429 TEST=build firmware and install it to DUT and run CTS relevant test, check temperature whether meets spec.
Signed-off-by: Peichao Wang peichao.wang@bitland.corp-partner.google.com Change-Id: I6a2f059fbd5c89f897cfb46d1f7a82b0923edb17 --- M src/mainboard/google/kahlee/variants/treeya/devicetree.cb 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/38443/4
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
Patch Set 4: Code-Review+1
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38443/2/src/mainboard/google/kahlee... File src/mainboard/google/kahlee/variants/treeya/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38443/2/src/mainboard/google/kahlee... PS2, Line 25: register "stapm_power_mw" = "7800"
According to the comment, you wanted to set it to 200 seconds, but this looks like 2000 seconds or j […]
Sorry Martin, make you confuse, I mean need set 2000 seconds, not 200 seconds. Obviously,at this moment, 2000 seconds could meet Lenovo 62368 thermal spec and pass the CTS test.
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38443/2/src/mainboard/google/kahlee... File src/mainboard/google/kahlee/variants/treeya/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38443/2/src/mainboard/google/kahlee... PS2, Line 25: register "stapm_power_mw" = "7800"
Sorry Martin, make you confuse, I mean need set 2000 seconds, not 200 seconds. […]
Done
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@10 PS2, Line 10: it meet
Done
I think if coreboot.org wants to enforce line width in commit messages, it should be done via a script rather than manual review.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@10 PS2, Line 10: it meet
I think if coreboot. […]
I am not really sure, how such a reliable check would look like. Do you know of such checkers?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
Patch Set 4: Verified+1 Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@10 PS2, Line 10: it meet
I am not really sure, how such a reliable check would look like. […]
This is really trivial. The information is there, it's readable, it's understandable. So what if the line isn't QUITE as long as it could be. It all needs to go onto 2 lines anyway.
I could understand a complaint if text that would fit on 2 lines were wrapped to fit on 3.
This though, seems like a complaint in search of a problem. How nitpicky can we get?
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@10 PS2, Line 10: it meet
I am not really sure, how such a reliable check would look like. […]
This is really trivial. The information is there, it's readable, it's understandable. So what if the line isn't QUITE as long as it could be. It all needs to go onto 2 lines anyway.
I could understand a complaint if text that would fit on 2 lines were wrapped to fit on 3.
This though, seems like a complaint in search of a problem. How nitpicky can we get?
Martin Roth has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
Removed Verified+1 by Martin Roth martinroth@google.com
Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
mb/google/kahlee/treeya: Update STAPM parameters for Treeya
Change stapm percentage to 80 and time to 2000 seconds make DUT meets Lenovo spec and pass CTS respectively.
BUG=b:147333429 TEST=build firmware and install it to DUT and run CTS relevant test, check temperature whether meets spec.
Signed-off-by: Peichao Wang peichao.wang@bitland.corp-partner.google.com Change-Id: I6a2f059fbd5c89f897cfb46d1f7a82b0923edb17 Reviewed-on: https://review.coreboot.org/c/coreboot/+/38443 Reviewed-by: Daniel Kurtz djkurtz@google.com Reviewed-by: Martin Roth martinroth@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/kahlee/variants/treeya/devicetree.cb 1 file changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Daniel Kurtz: Looks good to me, approved Peichao Li: Looks good to me, but someone else must approve
diff --git a/src/mainboard/google/kahlee/variants/treeya/devicetree.cb b/src/mainboard/google/kahlee/variants/treeya/devicetree.cb index 019dcf6..6c953b1 100644 --- a/src/mainboard/google/kahlee/variants/treeya/devicetree.cb +++ b/src/mainboard/google/kahlee/variants/treeya/devicetree.cb @@ -20,8 +20,8 @@ register "dram_clear_on_reset" = "DRAM_CONTENTS_KEEP" register "uma_mode" = "UMAMODE_SPECIFIED_SIZE" register "uma_size" = "16 * MiB" - register "stapm_percent" = "68" - register "stapm_time_ms" = "900000" + register "stapm_percent" = "80" + register "stapm_time_ms" = "2000000" register "stapm_power_mw" = "7800" register "lvds_poseq_varybl_to_blon" = "0x5" register "lvds_poseq_blon_to_varybl" = "0x5"
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38443/2//COMMIT_MSG@10 PS2, Line 10: it meet
This is really trivial. The information is there, it's readable, it's understandable. […]
Sorry, not sure how this got added twice - I certainly didn't mean to be that emphatic. I do appreciate the work to make commit messages better. Spelling, grammar, asking for clarification.
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38443 )
Change subject: mb/google/kahlee/treeya: Update STAPM parameters for Treeya ......................................................................
Patch Set 5:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/175 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/174 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/173
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Daniel Kurtz (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)
-
Peichao Li (Code Review)