Hello Chris Wang,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/44804
to review the following change.
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
mb/google/vilboz: update telemetry settings
Update telemetry setting for second SDLE testing.
BUG=b:160698427 BRANCH=None TEST=emerge-zork coreboot
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: I4cf5b8f090befd6a3c4990f44f2f200bc66aa1f4 --- M src/mainboard/google/zork/variants/vilboz/overridetree.cb 1 file changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/44804/1
diff --git a/src/mainboard/google/zork/variants/vilboz/overridetree.cb b/src/mainboard/google/zork/variants/vilboz/overridetree.cb index 00adb23..b547590 100644 --- a/src/mainboard/google/zork/variants/vilboz/overridetree.cb +++ b/src/mainboard/google/zork/variants/vilboz/overridetree.cb @@ -18,10 +18,10 @@
# End : OPN Performance Configuration
- register "telemetry_vddcr_vdd_slope" = "32453" #mA - register "telemetry_vddcr_vdd_offset" = "168" - register "telemetry_vddcr_soc_slope" = "22644" #mA - register "telemetry_vddcr_soc_offset" = "-70" + register "telemetry_vddcr_vdd_slope" = "32643" #mA + register "telemetry_vddcr_vdd_offset" = "208" + register "telemetry_vddcr_soc_slope" = "22742" #mA + register "telemetry_vddcr_soc_offset" = "-83"
# USB OC pin mapping register "usb_port_overcurrent_pin[1]" = "USB_OC_NONE" # LTE instead of USB C1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44804 )
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@9 PS1, Line 9: SDLE What does that mean?
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@10 PS1, Line 10: What problems does increasing the values fix?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44804 )
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@9 PS1, Line 9: testing Can you please capture the results of the test on the bug?
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@12 PS1, Line 12: None zork
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44804 )
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@9 PS1, Line 9: SDLE
What does that mean?
An electric test for AMD CPU.
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@10 PS1, Line 10:
What problems does increasing the values fix?
those value is used to power calibration for CPU for achieve the best performance.
Hello build bot (Jenkins), Furquan Shaikh, Chris Wang, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44804
to look at the new patch set (#2).
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
mb/google/vilboz: update telemetry settings
Update telemetry setting for second SDLE testing.
BUG=b:160698427 BRANCH=zork TEST=emerge-zork coreboot
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: I4cf5b8f090befd6a3c4990f44f2f200bc66aa1f4 --- M src/mainboard/google/zork/variants/vilboz/overridetree.cb 1 file changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/44804/2
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44804 )
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@9 PS1, Line 9: SDLE
An electric test for AMD CPU.
Done
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@9 PS1, Line 9: testing
Can you please capture the results of the test on the bug?
https://partnerissuetracker.corp.google.com/issues/160698427#comment14 Here's the result and recommend telemetry setting.
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@10 PS1, Line 10:
those value is used to power calibration for CPU for achieve the best performance.
Done
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@12 PS1, Line 12: None
zork
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44804 )
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
Patch Set 2: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44804 )
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44804/2/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/vilboz/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/44804/2/src/mainboard/google/zork/v... PS2, Line 24: telemetry_vddcr_soc_offset This seems to be a uint32_t field(https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/amd/picasso/chip....). Is the negative value actually expected? Does the field type needs to be changed?
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44804 )
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44804/2/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/vilboz/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/44804/2/src/mainboard/google/zork/v... PS2, Line 24: telemetry_vddcr_soc_offset
This seems to be a uint32_t field(https://review.coreboot.org/cgit/coreboot. […]
In original Agesa,it will apply a UINT32 value for PCD,so it will convert the type as below PcdSet32(PcdTelemetry_VddcrSocOffset, (UINT32) (0-83)); And the UPD will set to the PCD directly, and in the end this PCD will pass an int32 value to SMU.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44804 )
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@9 PS1, Line 9: SDLE
Done
Please add that to the commit message.
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@10 PS1, Line 10:
Done
Please extend the commit message accordingly.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44804 )
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44804/2/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/vilboz/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/44804/2/src/mainboard/google/zork/v... PS2, Line 24: telemetry_vddcr_soc_offset
In original Agesa,it will apply a UINT32 value for PCD,so it will convert the type as below […]
I think it would have been better to use the correct data type for this field. Anyways, it will have to be handled as a separate change.
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44804 )
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
Patch Set 2:
Hi Furquan, Do we need more action for this CLs or we could cq this CL now?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44804 )
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
Patch Set 2:
Patch Set 2:
Hi Furquan, Do we need more action for this CLs or we could cq this CL now?
Nothing pending from my side. But, there are still 2 open comments from Paul that i don't think have been addressed yet. Once those are addressed, please mark the comments as resolved.
Hello build bot (Jenkins), Furquan Shaikh, Chris Wang, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44804
to look at the new patch set (#3).
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
mb/google/vilboz: update telemetry settings
update the telemetry setting for second SDLE testing(for APU power adjusting). Those values are used to power calibration the APU power and achieving the best performance.
BUG=b:160698427 BRANCH=zork TEST=emerge-zork coreboot
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: I4cf5b8f090befd6a3c4990f44f2f200bc66aa1f4 --- M src/mainboard/google/zork/variants/vilboz/overridetree.cb 1 file changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/44804/3
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44804 )
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@9 PS1, Line 9: SDLE
Please add that to the commit message.
Done
https://review.coreboot.org/c/coreboot/+/44804/1//COMMIT_MSG@10 PS1, Line 10:
Please extend the commit message accordingly.
Done
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44804 )
Change subject: mb/google/vilboz: update telemetry settings ......................................................................
mb/google/vilboz: update telemetry settings
update the telemetry setting for second SDLE testing(for APU power adjusting). Those values are used to power calibration the APU power and achieving the best performance.
BUG=b:160698427 BRANCH=zork TEST=emerge-zork coreboot
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: I4cf5b8f090befd6a3c4990f44f2f200bc66aa1f4 Reviewed-on: https://review.coreboot.org/c/coreboot/+/44804 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/zork/variants/vilboz/overridetree.cb 1 file changed, 4 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/mainboard/google/zork/variants/vilboz/overridetree.cb b/src/mainboard/google/zork/variants/vilboz/overridetree.cb index d415de5..3d9ff7c 100644 --- a/src/mainboard/google/zork/variants/vilboz/overridetree.cb +++ b/src/mainboard/google/zork/variants/vilboz/overridetree.cb @@ -18,10 +18,10 @@
# End : OPN Performance Configuration
- register "telemetry_vddcr_vdd_slope" = "32453" #mA - register "telemetry_vddcr_vdd_offset" = "168" - register "telemetry_vddcr_soc_slope" = "22644" #mA - register "telemetry_vddcr_soc_offset" = "-70" + register "telemetry_vddcr_vdd_slope" = "32643" #mA + register "telemetry_vddcr_vdd_offset" = "208" + register "telemetry_vddcr_soc_slope" = "22742" #mA + register "telemetry_vddcr_soc_offset" = "-83"
# USB OC pin mapping register "usb_port_overcurrent_pin[1]" = "USB_OC_NONE" # LTE instead of USB C1