Change in coreboot[master]: mb/google/zork: update telemetry settings for morphius

[Kevin Chiu (Code Review)]

Kevin Chiu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: update telemetry settings for morphius
......................................................................

mb/google/zork: update telemetry settings for morphius

update telemetry to improve the performance.

BUG=b:168265881
BRANCH=zork
TEST=emerge-zork coreboot

Change-Id: Id6c4f1a92d7f2ad293df7b63694e9665b85f8018
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/zork/variants/morphius/overridetree.cb
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/46472/1

diff --git a/src/mainboard/google/zork/variants/morphius/overridetree.cb b/src/mainboard/google/zork/variants/morphius/overridetree.cb
index 43a9439..ecadb07 100644
--- a/src/mainboard/google/zork/variants/morphius/overridetree.cb
+++ b/src/mainboard/google/zork/variants/morphius/overridetree.cb
@@ -16,9 +16,9 @@
    register "sustained_power_limit" = "12000" #mw
    register "thermctl_limit" = "100" #degrees C

-	register "telemetry_vddcr_vdd_slope" = "78709" #mA
+	register "telemetry_vddcr_vdd_slope" = "62852" #mA
    register "telemetry_vddcr_vdd_offset" = "0"
-	register "telemetry_vddcr_soc_slope" = "29035" #mA
+	register "telemetry_vddcr_soc_slope" = "28022" #mA
    register "telemetry_vddcr_soc_offset" = "0"

# Set STAPM confiuration for tablet mode

[Keith Tzeng (Code Review)]

Keith Tzeng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: update telemetry settings for morphius
......................................................................

Patch Set 1:

Hi, please help review the change,
We'll need to land to fw branch asap. thanks.

[Keith Tzeng (Code Review)]

Keith Tzeng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: update telemetry settings for morphius
......................................................................

Patch Set 1:

Please help review this, thanks.

[Chris Wang (Code Review)]

Chris Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: update telemetry settings for morphius
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/46472/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46472/1//COMMIT_MSG@11 
PS1, Line 11: b:168265881
Would you also add the issue number b:170271680 on this row.
https://partnerissuetracker.corp.google.com/issues/170271680
It should also related to the performance for Morphius.

[Kevin Chiu (Code Review)]

Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Marshall Dawson, Chris Wang, Keith Tzeng, Edward Hill, Rob Barnes, Matt Papageorge, Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/46472

to look at the new patch set (#2).

Change subject: mb/google/zork: update telemetry settings for morphius
......................................................................

mb/google/zork: update telemetry settings for morphius

update telemetry to improve the performance.

BUG=b:168265881,b:170271680
BRANCH=zork
TEST=emerge-zork coreboot

Change-Id: Id6c4f1a92d7f2ad293df7b63694e9665b85f8018
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/zork/variants/morphius/overridetree.cb
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/46472/2

[Kevin Chiu (Code Review)]

Kevin Chiu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: update telemetry settings for morphius
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46472/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46472/1//COMMIT_MSG@11 
PS1, Line 11: b:168265881
> Would you also add the issue number b:170271680 on this row. […]

Done

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: update telemetry settings for morphius
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/46472/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46472/2//COMMIT_MSG@9 
PS2, Line 9: update telemetry to improve the performance.
What performance was improved, and how did you measure it?

https://review.coreboot.org/c/coreboot/+/46472/2//COMMIT_MSG@11 
PS2, Line 11: BUG=b:168265881,b:170271680
Please always summarize problems in the commit message.

https://review.coreboot.org/c/coreboot/+/46472/2//COMMIT_MSG@13 
PS2, Line 13: TEST=emerge-zork coreboot
How did you measure the performance improvement?

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: update telemetry settings for morphius
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/46472/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46472/2//COMMIT_MSG@9 
PS2, Line 9: update telemetry to improve the performance.
Maybe more specific:
> Decrease telemetry voltage slope values …

https://review.coreboot.org/c/coreboot/+/46472/2/src/mainboard/google/zork/v... 
File src/mainboard/google/zork/variants/morphius/overridetree.cb:

https://review.coreboot.org/c/coreboot/+/46472/2/src/mainboard/google/zork/v... 
PS2, Line 19: telemetry_vddcr_vdd_slope
The unit should be part of the variable name.

[Kevin Chiu (Code Review)]

Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Marshall Dawson, Chris Wang, Keith Tzeng, Edward Hill, Rob Barnes, Matt Papageorge, Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/46472

to look at the new patch set (#3).

Change subject: mb/google/zork: update telemetry settings for morphius
......................................................................

mb/google/zork: update telemetry settings for morphius

update telemetry to have correct setting based on the Power Stardust test
result.

BUG=b:168265881,b:170271680
BRANCH=zork
TEST=emerge-zork coreboot

Change-Id: Id6c4f1a92d7f2ad293df7b63694e9665b85f8018
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/zork/variants/morphius/overridetree.cb
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/46472/3

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: update telemetry settings for morphius
......................................................................

Patch Set 3:

(7 comments)

https://review.coreboot.org/c/coreboot/+/46472/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46472/2//COMMIT_MSG@9 
PS2, Line 9: update telemetry to improve the performance.
> What performance was improved, and how did you measure it?

Done

https://review.coreboot.org/c/coreboot/+/46472/2//COMMIT_MSG@9 
PS2, Line 9: update telemetry to improve the performance.
> Maybe more specific: […]

Kevin, can you give any more information on what the change does or not?

https://review.coreboot.org/c/coreboot/+/46472/2//COMMIT_MSG@11 
PS2, Line 11: BUG=b:168265881,b:170271680
> Please always summarize problems in the commit message.

That's not really something that anyone's been doing.  If we want to change the protocol to include a title, we can probably do that, but that's a much bigger change.  If you'd like to update the gerrit guidelines to reflect that, I could get onboard with that.

The title of 168265881 is "Morphius Stardust test tracking"

This change doesn't help with the other issue, so it should be removed.

https://review.coreboot.org/c/coreboot/+/46472/2//COMMIT_MSG@13 
PS2, Line 13: TEST=emerge-zork coreboot
> How did you measure the performance improvement?

This isn't really a performance improvement.  It's a change to meet the spec from a manufacturing test.

https://review.coreboot.org/c/coreboot/+/46472/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46472/3//COMMIT_MSG@9 
PS3, Line 9: test
Could you please wrap this to the next line.

https://review.coreboot.org/c/coreboot/+/46472/3//COMMIT_MSG@12 
PS3, Line 12: ,b:170271680
Please remove - as noted in the bug, this change doesn't have any effect on the bug.

https://review.coreboot.org/c/coreboot/+/46472/2/src/mainboard/google/zork/v... 
File src/mainboard/google/zork/variants/morphius/overridetree.cb:

https://review.coreboot.org/c/coreboot/+/46472/2/src/mainboard/google/zork/v... 
PS2, Line 19: telemetry_vddcr_vdd_slope
> The unit should be part of the variable name.

I think that will need to be a follow-on patch.  The variables already exist, this is just updating the value.

Change in coreboot[master]: mb/google/zork: Update telemetry settings for morphius

[Marshall Dawson (Code Review)]

Marshall Dawson has uploaded a new patch set (#4) to the change originally created by Kevin Chiu. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: Update telemetry settings for morphius
......................................................................

mb/google/zork: Update telemetry settings for morphius

Correct the two load line slope settings for the SVID3 telemetry.
AGESA sends these values to the SMU, which accepts values as units
of current.  Proper calibration is determined by the AMD SDLE tool
and the Stardust test.

BUG=b:168265881
BRANCH=zork
TEST=emerge-zork coreboot

Change-Id: Id6c4f1a92d7f2ad293df7b63694e9665b85f8018
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/zork/variants/morphius/overridetree.cb
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/46472/4

Change in coreboot[master]: mb/google/zork: Update telemetry settings for morphius

[Marshall Dawson (Code Review)]

Marshall Dawson has uploaded a new patch set (#5) to the change originally created by Kevin Chiu. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: Update telemetry settings for morphius
......................................................................

mb/google/zork: Update telemetry settings for morphius

Correct the two load line slope settings for the SVID3 telemetry.
AGESA sends these values to the SMU, which accepts them as units
of current.  Proper calibration is determined by the AMD SDLE tool
and the Stardust test.

BUG=b:168265881
BRANCH=zork
TEST=emerge-zork coreboot

Change-Id: Id6c4f1a92d7f2ad293df7b63694e9665b85f8018
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/zork/variants/morphius/overridetree.cb
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/46472/5

Change in coreboot[master]: mb/google/zork: Update telemetry settings for morphius

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: Update telemetry settings for morphius
......................................................................

Patch Set 5:

(3 comments)

https://review.coreboot.org/c/coreboot/+/46472/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46472/2//COMMIT_MSG@9 
PS2, Line 9: update telemetry to improve the performance.
> Kevin, can you give any more information on what the change does or not?

Done

https://review.coreboot.org/c/coreboot/+/46472/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46472/3//COMMIT_MSG@9 
PS3, Line 9: test
> Could you please wrap this to the next line.

Done

https://review.coreboot.org/c/coreboot/+/46472/3//COMMIT_MSG@12 
PS3, Line 12: ,b:170271680
> Please remove - as noted in the bug, this change doesn't have any effect on the bug.

Done

Change in coreboot[master]: mb/google/zork: Update telemetry settings for morphius

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: Update telemetry settings for morphius
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: mb/google/zork: Update telemetry settings for morphius

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: Update telemetry settings for morphius
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46472/2/src/mainboard/google/zork/v... 
File src/mainboard/google/zork/variants/morphius/overridetree.cb:

https://review.coreboot.org/c/coreboot/+/46472/2/src/mainboard/google/zork/v... 
PS2, Line 19: telemetry_vddcr_vdd_slope
> I think that will need to be a follow-on patch. […]

Filed a bug to address this issue:
b/171334623
Zork: Update coreboot UPD variable names to include units

Change in coreboot[master]: mb/google/zork: Update telemetry settings for morphius

[Keith Tzeng (Code Review)]

Keith Tzeng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: Update telemetry settings for morphius
......................................................................

Patch Set 5:

Could we land this and CP to chromium tree?
We need get this into RO FW. thanks.

Change in coreboot[master]: mb/google/zork: Update telemetry settings for morphius

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: Update telemetry settings for morphius
......................................................................

mb/google/zork: Update telemetry settings for morphius

Correct the two load line slope settings for the SVID3 telemetry.
AGESA sends these values to the SMU, which accepts them as units
of current.  Proper calibration is determined by the AMD SDLE tool
and the Stardust test.

BUG=b:168265881
BRANCH=zork
TEST=emerge-zork coreboot

Change-Id: Id6c4f1a92d7f2ad293df7b63694e9665b85f8018
Signed-off-by: Kevin Chiu 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46472
Reviewed-by: Martin Roth 
Reviewed-by: Chris Wang 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/google/zork/variants/morphius/overridetree.cb
1 file changed, 2 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Martin Roth: Looks good to me, approved
  Chris Wang: Looks good to me, but someone else must approve

diff --git a/src/mainboard/google/zork/variants/morphius/overridetree.cb b/src/mainboard/google/zork/variants/morphius/overridetree.cb
index 43a9439..ecadb07 100644
--- a/src/mainboard/google/zork/variants/morphius/overridetree.cb
+++ b/src/mainboard/google/zork/variants/morphius/overridetree.cb
@@ -16,9 +16,9 @@
    register "sustained_power_limit" = "12000" #mw
    register "thermctl_limit" = "100" #degrees C

-	register "telemetry_vddcr_vdd_slope" = "78709" #mA
+	register "telemetry_vddcr_vdd_slope" = "62852" #mA
    register "telemetry_vddcr_vdd_offset" = "0"
-	register "telemetry_vddcr_soc_slope" = "29035" #mA
+	register "telemetry_vddcr_soc_slope" = "28022" #mA
    register "telemetry_vddcr_soc_offset" = "0"

# Set STAPM confiuration for tablet mode

Change in coreboot[master]: mb/google/zork: Update telemetry settings for morphius

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46472 )

Change subject: mb/google/zork: Update telemetry settings for morphius
......................................................................

Patch Set 6:

Automatic boot test returned (PASS/FAIL/TOTAL): 5/1/6
"HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/24001
"HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/24000
"QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/23999
"QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/23998
"QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/23996
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/23995

Please note: This test is under development and might not be accurate at all!