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.