Hello Chris Wang,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/44265
to review the following change.
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
ec/google/chromeec: Add dptc interface support
add the dptc interface support when system in tablet mode
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: I2be7942132cea474237f531021ad4fd9856b5050 --- M src/ec/google/chromeec/acpi/ec.asl M src/mainboard/google/zork/dsdt.asl M src/soc/amd/picasso/acpi/soc.asl 3 files changed, 10 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/44265/1
diff --git a/src/ec/google/chromeec/acpi/ec.asl b/src/ec/google/chromeec/acpi/ec.asl index f8d4bdf..4038a9a 100644 --- a/src/ec/google/chromeec/acpi/ec.asl +++ b/src/ec/google/chromeec/acpi/ec.asl @@ -10,7 +10,9 @@ #ifdef DPTF_ENABLE_CHARGER External (_SB.DPTF.TCHG, DeviceObj) #endif - +#ifdef DPTC_ENABLE +External(_SB.DPTC, MethodObj) +#endif
Device (EC0) { @@ -380,6 +382,9 @@ #ifdef EC_ENABLE_TBMC_DEVICE Notify (TBMC, 0x80) #endif +#ifdef DPTC_ENABLE + _SB.DPTC() +#endif }
/* diff --git a/src/mainboard/google/zork/dsdt.asl b/src/mainboard/google/zork/dsdt.asl index 2dc456d..b3e5f1b 100644 --- a/src/mainboard/google/zork/dsdt.asl +++ b/src/mainboard/google/zork/dsdt.asl @@ -34,9 +34,9 @@ Scope(_SB) { /* Start _SB scope */ /* global utility methods expected within the _SB scope */ #include <arch/x86/acpi/globutil.asl> - +#ifdef DPTC_ENABLE #include <variant/acpi/dptc.asl> - +#endif /* Describe the SOC */ #include <soc.asl>
diff --git a/src/soc/amd/picasso/acpi/soc.asl b/src/soc/amd/picasso/acpi/soc.asl index 909b985..a0a05fa 100644 --- a/src/soc/amd/picasso/acpi/soc.asl +++ b/src/soc/amd/picasso/acpi/soc.asl @@ -17,7 +17,9 @@ /* Describe the devices in the Southbridge */ #include "sb_fch.asl"
+#ifdef DPTC_ENABLE #include "dptc.asl" +#endif
/* Add GPIO library */ #include <soc/amd/common/acpi/gpio_bank_lib.asl>
Keith Tzeng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44265 )
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
Patch Set 5:
please help review, thanks.
Hello build bot (Jenkins), Furquan Shaikh, Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44265
to look at the new patch set (#11).
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
ec/google/chromeec: Add dptc interface support
add the dptc interface support when system in tablet mode
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: I2be7942132cea474237f531021ad4fd9856b5050 --- M src/ec/google/chromeec/acpi/ec.asl 1 file changed, 6 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/44265/11
Keith Tzeng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44265 )
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
Patch Set 11:
Hi Furquan, could you please help review the new patch? thanks.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44265 )
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
Patch Set 11: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/44265/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44265/11//COMMIT_MSG@9 PS11, Line 9: add the dptc interface support when system in tablet mode Can you please add some more context as to what platforms this interface is supposed to be used.
https://review.coreboot.org/c/coreboot/+/44265/11//COMMIT_MSG@12 PS11, Line 12: none zork
https://review.coreboot.org/c/coreboot/+/44265/11/src/ec/google/chromeec/acp... File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/44265/11/src/ec/google/chromeec/acp... PS11, Line 13: #ifdef DPTC_SUPPORT_ENABLE Can you please add a comment here indicating what platforms this method will be present/used on?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44265 )
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44265/11/src/ec/google/chromeec/acp... File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/44265/11/src/ec/google/chromeec/acp... PS11, Line 380: _SB.DPTF.TPET() Not for this change, but I think we should consolidate these calls to DPTF and DPTC into a single interface going out of the EC. Probably when we move the EC code to SSDT generation, this would be simpler to handle.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44265 )
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44265/11/src/ec/google/chromeec/acp... File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/44265/11/src/ec/google/chromeec/acp... PS11, Line 386: _SB.DPTC() I think it would be safer to add a check to ensure that DPTC is really provided:
If (CondRefOf (_SB.DPTC)) { _SB.DPTC() }
Thus, if some board accidentally selects DPTC_SUPPORT_ENABLE, but doesn't set dptc_enable = 1, then this would not result in ACPI errors.
Hello build bot (Jenkins), Furquan Shaikh, Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44265
to look at the new patch set (#13).
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
ec/google/chromeec: Add dptc interface support
add the dptc interface support when system in tablet mode. In some FP5/FT5 platform, which will have different power or thermal parameters depends on different form factor.
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: I2be7942132cea474237f531021ad4fd9856b5050 --- M src/ec/google/chromeec/acpi/ec.asl 1 file changed, 9 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/44265/13
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44265 )
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
Patch Set 15: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44265 )
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44265/16/src/ec/google/chromeec/acp... File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/44265/16/src/ec/google/chromeec/acp... PS16, Line 386: DPTC_SUPPORT_ENABLE nit: I think we should name this something like EC_ENABLE_AMD_DPTC_SUPPORT just to make it clear what the intent of this config is. Sorry, I know you kept it similar to DPTF support, but from the name it is not very clear who the consumer of the config is.
Hello build bot (Jenkins), Furquan Shaikh, Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44265
to look at the new patch set (#17).
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
ec/google/chromeec: Add dptc interface support
add the dptc interface support when system in tablet mode. In some FP5/FT5 platform, which will have different power or thermal parameters depends on different form factor.
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: I2be7942132cea474237f531021ad4fd9856b5050 --- M src/ec/google/chromeec/acpi/ec.asl 1 file changed, 9 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/44265/17
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44265 )
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
Patch Set 17: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44265/16/src/ec/google/chromeec/acp... File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/44265/16/src/ec/google/chromeec/acp... PS16, Line 386: DPTC_SUPPORT_ENABLE
nit: I think we should name this something like EC_ENABLE_AMD_DPTC_SUPPORT just to make it clear wha […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44265 )
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
Patch Set 17:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44265/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44265/11//COMMIT_MSG@9 PS11, Line 9: add the dptc interface support when system in tablet mode
Can you please add some more context as to what platforms this interface is supposed to be used.
Done
https://review.coreboot.org/c/coreboot/+/44265/11//COMMIT_MSG@12 PS11, Line 12: none
zork
Done
https://review.coreboot.org/c/coreboot/+/44265/11/src/ec/google/chromeec/acp... File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/44265/11/src/ec/google/chromeec/acp... PS11, Line 13: #ifdef DPTC_SUPPORT_ENABLE
Can you please add a comment here indicating what platforms this method will be present/used on?
Done
https://review.coreboot.org/c/coreboot/+/44265/11/src/ec/google/chromeec/acp... PS11, Line 386: _SB.DPTC()
I think it would be safer to add a check to ensure that DPTC is really provided: […]
Done
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44265 )
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
ec/google/chromeec: Add dptc interface support
add the dptc interface support when system in tablet mode. In some FP5/FT5 platform, which will have different power or thermal parameters depends on different form factor.
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: I2be7942132cea474237f531021ad4fd9856b5050 Reviewed-on: https://review.coreboot.org/c/coreboot/+/44265 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/ec/google/chromeec/acpi/ec.asl 1 file changed, 9 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/ec/google/chromeec/acpi/ec.asl b/src/ec/google/chromeec/acpi/ec.asl index f8d4bdf..e2fa2de 100644 --- a/src/ec/google/chromeec/acpi/ec.asl +++ b/src/ec/google/chromeec/acpi/ec.asl @@ -10,7 +10,10 @@ #ifdef DPTF_ENABLE_CHARGER External (_SB.DPTF.TCHG, DeviceObj) #endif - +/* Enable DPTC interface with AMD ALIB */ +#ifdef EC_ENABLE_AMD_DPTC_SUPPORT +External(_SB.DPTC, MethodObj) +#endif
Device (EC0) { @@ -380,6 +383,11 @@ #ifdef EC_ENABLE_TBMC_DEVICE Notify (TBMC, 0x80) #endif +#ifdef EC_ENABLE_AMD_DPTC_SUPPORT + If (CondRefOf (_SB.DPTC)) { + _SB.DPTC() + } +#endif }
/*
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44265 )
Change subject: ec/google/chromeec: Add dptc interface support ......................................................................
Patch Set 18:
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/19647 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19646 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19645 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19644 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19643 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19651 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19650 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19649 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19648
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)