Hello Chris Wang,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/45153
to review the following change.
Change subject: mb/google/zork:Add dptc interface support ......................................................................
mb/google/zork:Add dptc interface support
Add dptc interface in devicetree.the variants can set below power parameters in overridetree as below ex: dptc_enable = 1 dptc_fast_ppt_limit = 24000 dptc_slow_ppt_limit = 20000 dptc_sustained_power_limit = 6000
BUG=b:157943445 BRANCH=none TEST=Build. check the setting changed.
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: I4dac4b7e5157ad7ad407f42a6fc6b06eefbf3291 --- M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb M src/mainboard/google/zork/variants/baseboard/include/baseboard/thermal.h 3 files changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/45153/1
diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb b/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb index 42219d7..f240a7c 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb @@ -40,6 +40,8 @@
# End : OPN Performance Configuration
+ register "dptc_enable" = "0" + register "sd_emmc_config" = "SD_EMMC_EMMC_HS400"
register "xhci0_force_gen1" = "0" diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb b/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb index 1620643..50fadae 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb @@ -40,6 +40,8 @@
# End : OPN Performance Configuration
+ register "dptc_enable" = "0" + register "sd_emmc_config" = "SD_EMMC_EMMC_HS400"
register "xhci0_force_gen1" = "0" diff --git a/src/mainboard/google/zork/variants/baseboard/include/baseboard/thermal.h b/src/mainboard/google/zork/variants/baseboard/include/baseboard/thermal.h index b3c951b..fd110cc 100644 --- a/src/mainboard/google/zork/variants/baseboard/include/baseboard/thermal.h +++ b/src/mainboard/google/zork/variants/baseboard/include/baseboard/thermal.h @@ -3,6 +3,8 @@ #ifndef THERMAL_H #define THERMAL_H
+#define DPTC_SUPPORT_ENABLE + /* * Picasso Thermal Requirements * TDP (W) 15
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45153 )
Change subject: mb/google/zork:Add dptc interface support ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45153/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45153/1//COMMIT_MSG@7 PS1, Line 7: mb/google/zork:Add dptc interface support Please add a space after the colon.
https://review.coreboot.org/c/coreboot/+/45153/1//COMMIT_MSG@9 PS1, Line 9: Add dptc interface in devicetree.the variants can set below power Please add a space after the dot/period.
https://review.coreboot.org/c/coreboot/+/45153/1//COMMIT_MSG@12 PS1, Line 12: dptc_enable = 1 : dptc_fast_ppt_limit = 24000 : dptc_slow_ppt_limit = 20000 : dptc_sustained_power_limit = 6000 Please indent with four spaces.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45153 )
Change subject: mb/google/zork:Add dptc interface support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45153/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/thermal.h:
https://review.coreboot.org/c/coreboot/+/45153/1/src/mainboard/google/zork/v... PS1, Line 6: #define DPTC_SUPPORT_ENABLE This is really weird. I think we should set DPTC_SUPPORT_ENABLE only for the variants that set dptc_enable to 1.
Hello build bot (Jenkins), Furquan Shaikh, Chris Wang, Keith Tzeng, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45153
to look at the new patch set (#2).
Change subject: mb/google/zork:Add dptc interface support ......................................................................
mb/google/zork:Add dptc interface support
Add dptc interface in devicetree.the variants can set below power parameters in overridetree as below ex: dptc_enable = 1 dptc_fast_ppt_limit = 24000 dptc_slow_ppt_limit = 20000 dptc_sustained_power_limit = 6000
BUG=b:157943445 BRANCH=none TEST=Build. check the setting changed.
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: I4dac4b7e5157ad7ad407f42a6fc6b06eefbf3291 --- M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb 2 files changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/45153/2
Hello build bot (Jenkins), Furquan Shaikh, Chris Wang, Keith Tzeng, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45153
to look at the new patch set (#4).
Change subject: mb/google/zork:Add dptc interface support ......................................................................
mb/google/zork:Add dptc interface support
Add dptc interface in devicetree.the variants can set below power parameters in overridetree as below ex: dptc_enable = 1 dptc_fast_ppt_limit = 24000 dptc_slow_ppt_limit = 20000 dptc_sustained_power_limit = 6000
BUG=b:157943445 BRANCH=zork TEST=Build. check the setting changed.
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: I4dac4b7e5157ad7ad407f42a6fc6b06eefbf3291 --- M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb 2 files changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/45153/4
Hello build bot (Jenkins), Furquan Shaikh, Chris Wang, Keith Tzeng, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45153
to look at the new patch set (#6).
Change subject: mb/google/zork:Add dptc interface support ......................................................................
mb/google/zork:Add dptc interface support
Add dptc interface in devicetree.the variants can set below power parameters in overridetree as below ex: dptc_enable = 1 dptc_fast_ppt_limit = 24000 dptc_slow_ppt_limit = 20000 dptc_sustained_power_limit = 6000
BUG=b:157943445 BRANCH=zork TEST=Build. check the setting changed.
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: I4dac4b7e5157ad7ad407f42a6fc6b06eefbf3291 --- M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb 2 files changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/45153/6
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45153 )
Change subject: mb/google/zork:Add dptc interface support ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45153/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45153/6//COMMIT_MSG@12 PS6, Line 12: dptc_enable = 1 : dptc_fast_ppt_limit = 24000 : dptc_slow_ppt_limit = 20000 : dptc_sustained_power_limit = 6000 What variant actually needs this? Instead of adding dptc_enable to 0, can you instead set dptc_enable and the corresponding parameters for that variant in the override tree?
Hello build bot (Jenkins), Furquan Shaikh, Chris Wang, Keith Tzeng, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45153
to look at the new patch set (#8).
Change subject: mb/google/zork:Add dptc interface support for morphius ......................................................................
mb/google/zork:Add dptc interface support for morphius
Add dptc interface in devicetree for morphius. Set the STAPM parameters for tablet mode:
dptc_enable = 1 dptc_fast_ppt_limit = 24000 dptc_slow_ppt_limit = 20000 dptc_sustained_power_limit = 6000
BUG=b:157943445 BRANCH=zork TEST=Build. check the setting changed.
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: I4dac4b7e5157ad7ad407f42a6fc6b06eefbf3291 --- M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb M src/mainboard/google/zork/variants/morphius/include/variant/thermal.h M src/mainboard/google/zork/variants/morphius/overridetree.cb 3 files changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/45153/8
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45153 )
Change subject: mb/google/zork:Add dptc interface support for morphius ......................................................................
Patch Set 8:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb:
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... PS8, Line 43: Blank line change not needed.
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/morphius/include/variant/thermal.h:
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... PS8, Line 4: #define DPTC_SUPPORT_ENABLE I think this should go in ec.h since it is used only by the EC.
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/morphius/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... PS8, Line 23: # End : OPN Performance Configuration This should be after the new lines being added?
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... PS8, Line 26: register "slow_ppt_limit_tablet_mode" = "20000" #mw : register "fast_ppt_limit_tablet_mode" = "24000" #mw These values are exactly the same as the default values above. Is that expected?
Hello build bot (Jenkins), Furquan Shaikh, Chris Wang, Keith Tzeng, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45153
to look at the new patch set (#9).
Change subject: mb/google/zork:Add dptc interface support for morphius ......................................................................
mb/google/zork:Add dptc interface support for morphius
Add dptc interface in devicetree for morphius. Set the STAPM parameters for tablet mode:
dptc_enable = 1 dptc_fast_ppt_limit = 24000 dptc_slow_ppt_limit = 20000 dptc_sustained_power_limit = 6000
BUG=b:157943445 BRANCH=zork TEST=Build. check the setting changed.
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: I4dac4b7e5157ad7ad407f42a6fc6b06eefbf3291 --- M src/mainboard/google/zork/variants/morphius/include/variant/ec.h M src/mainboard/google/zork/variants/morphius/include/variant/thermal.h M src/mainboard/google/zork/variants/morphius/overridetree.cb 3 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/45153/9
Hello build bot (Jenkins), Furquan Shaikh, Chris Wang, Keith Tzeng, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45153
to look at the new patch set (#10).
Change subject: mb/google/zork:Add dptc interface support for morphius ......................................................................
mb/google/zork:Add dptc interface support for morphius
Add dptc interface in devicetree for morphius. Set the STAPM parameters for tablet mode:
dptc_enable = 1 dptc_fast_ppt_limit = 24000 dptc_slow_ppt_limit = 20000 dptc_sustained_power_limit = 6000
BUG=b:157943445 BRANCH=zork TEST=Build. check the setting changed.
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: I4dac4b7e5157ad7ad407f42a6fc6b06eefbf3291 --- M src/mainboard/google/zork/variants/morphius/include/variant/ec.h M src/mainboard/google/zork/variants/morphius/overridetree.cb 2 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/45153/10
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45153 )
Change subject: mb/google/zork:Add dptc interface support for morphius ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb:
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... PS8, Line 43:
Blank line change not needed.
Done
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/morphius/include/variant/thermal.h:
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... PS8, Line 4: #define DPTC_SUPPORT_ENABLE
I think this should go in ec.h since it is used only by the EC.
Done
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/morphius/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... PS8, Line 23: # End : OPN Performance Configuration
This should be after the new lines being added?
Done
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... PS8, Line 26: register "slow_ppt_limit_tablet_mode" = "20000" #mw : register "fast_ppt_limit_tablet_mode" = "24000" #mw
These values are exactly the same as the default values above. […]
From issue describe, morphius will use this configure for tablet mode.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45153 )
Change subject: mb/google/zork:Add dptc interface support for morphius ......................................................................
Patch Set 10: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/morphius/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/45153/8/src/mainboard/google/zork/v... PS8, Line 26: register "slow_ppt_limit_tablet_mode" = "20000" #mw : register "fast_ppt_limit_tablet_mode" = "24000" #mw
From issue describe, morphius will use this configure for tablet mode.
SG.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45153 )
Change subject: mb/google/zork:Add dptc interface support for morphius ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45153/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45153/1//COMMIT_MSG@7 PS1, Line 7: mb/google/zork:Add dptc interface support
Please add a space after the colon.
Chris - this still needs to be addressed.
https://review.coreboot.org/c/coreboot/+/45153/1//COMMIT_MSG@9 PS1, Line 9: Add dptc interface in devicetree.the variants can set below power
Please add a space after the dot/period.
Done
https://review.coreboot.org/c/coreboot/+/45153/1//COMMIT_MSG@12 PS1, Line 12: dptc_enable = 1 : dptc_fast_ppt_limit = 24000 : dptc_slow_ppt_limit = 20000 : dptc_sustained_power_limit = 6000
Please indent with four spaces.
Done
https://review.coreboot.org/c/coreboot/+/45153/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45153/6//COMMIT_MSG@12 PS6, Line 12: dptc_enable = 1 : dptc_fast_ppt_limit = 24000 : dptc_slow_ppt_limit = 20000 : dptc_sustained_power_limit = 6000
What variant actually needs this? Instead of adding dptc_enable to 0, can you instead set dptc_enabl […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45153 )
Change subject: mb/google/zork:Add dptc interface support for morphius ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45153/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/thermal.h:
https://review.coreboot.org/c/coreboot/+/45153/1/src/mainboard/google/zork/v... PS1, Line 6: #define DPTC_SUPPORT_ENABLE
This is really weird. […]
Done
Keith Tzeng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45153 )
Change subject: mb/google/zork:Add dptc interface support for morphius ......................................................................
Patch Set 10:
Could we land this?
Hello build bot (Jenkins), Furquan Shaikh, Chris Wang, Keith Tzeng, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45153
to look at the new patch set (#11).
Change subject: mb/google/zork: Add dptc interface support for morphius ......................................................................
mb/google/zork: Add dptc interface support for morphius
Add dptc interface in devicetree for morphius. Set the STAPM parameters for tablet mode:
dptc_enable = 1 dptc_fast_ppt_limit = 24000 dptc_slow_ppt_limit = 20000 dptc_sustained_power_limit = 6000
BUG=b:157943445 BRANCH=zork TEST=Build. check the setting changed.
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: I4dac4b7e5157ad7ad407f42a6fc6b06eefbf3291 --- M src/mainboard/google/zork/variants/morphius/include/variant/ec.h M src/mainboard/google/zork/variants/morphius/overridetree.cb 2 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/45153/11
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45153 )
Change subject: mb/google/zork: Add dptc interface support for morphius ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45153/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45153/1//COMMIT_MSG@7 PS1, Line 7: mb/google/zork:Add dptc interface support
Chris - this still needs to be addressed.
Done
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45153 )
Change subject: mb/google/zork: Add dptc interface support for morphius ......................................................................
mb/google/zork: Add dptc interface support for morphius
Add dptc interface in devicetree for morphius. Set the STAPM parameters for tablet mode:
dptc_enable = 1 dptc_fast_ppt_limit = 24000 dptc_slow_ppt_limit = 20000 dptc_sustained_power_limit = 6000
BUG=b:157943445 BRANCH=zork TEST=Build. check the setting changed.
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: I4dac4b7e5157ad7ad407f42a6fc6b06eefbf3291 Reviewed-on: https://review.coreboot.org/c/coreboot/+/45153 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/zork/variants/morphius/include/variant/ec.h M src/mainboard/google/zork/variants/morphius/overridetree.cb 2 files changed, 9 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/mainboard/google/zork/variants/morphius/include/variant/ec.h b/src/mainboard/google/zork/variants/morphius/include/variant/ec.h index e177507..04dfba4 100644 --- a/src/mainboard/google/zork/variants/morphius/include/variant/ec.h +++ b/src/mainboard/google/zork/variants/morphius/include/variant/ec.h @@ -4,3 +4,6 @@
/* Enable PS/2 Mouse */ #define SIO_EC_ENABLE_PS2M + +/* Enable DPTC support */ +#define EC_ENABLE_AMD_DPTC_SUPPORT diff --git a/src/mainboard/google/zork/variants/morphius/overridetree.cb b/src/mainboard/google/zork/variants/morphius/overridetree.cb index ed26702..0040389 100644 --- a/src/mainboard/google/zork/variants/morphius/overridetree.cb +++ b/src/mainboard/google/zork/variants/morphius/overridetree.cb @@ -20,6 +20,12 @@ register "telemetry_vddcr_soc_slope" = "29035" #mA register "telemetry_vddcr_soc_offset" = "0"
+ # Set STAPM confiuration for tablet mode + register "dptc_enable" = "1" + register "slow_ppt_limit_tablet_mode" = "20000" #mw + register "fast_ppt_limit_tablet_mode" = "24000" #mw + register "sustained_power_limit_tablet_mode" = "6000" #mw + # End : OPN Performance Configuration
# Enable I2C2 for trackpad, touchscreen, pen at 400kHz
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45153 )
Change subject: mb/google/zork: Add dptc interface support for morphius ......................................................................
Patch Set 12:
Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19629 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19628 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19627 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19626 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19625 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19633 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19632 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19631 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19630
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
chris wang (Code Review)
-
Furquan Shaikh (Code Review)
-
Keith Tzeng (Code Review)
-
Paul Menzel (Code Review)