Hello Chris Wang,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to review the following change.
Change subject: [WIP] mb/google/zork: add dptc support ......................................................................
[WIP] mb/google/zork: add dptc support
add dptc support for different factory.
Profile 0 : claimshell mode Profile 1 : Tablet mode
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/ec/google/chromeec/acpi/ec.asl M src/mainboard/google/zork/dsdt.asl A src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dtpc.asl A src/mainboard/google/zork/variants/trembyle/include/variant/acpi/dptc.asl A src/soc/amd/picasso/acpi/dptc.asl M src/soc/amd/picasso/acpi/soc.asl A src/soc/amd/picasso/include/dptc.h 7 files changed, 145 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/1
diff --git a/src/ec/google/chromeec/acpi/ec.asl b/src/ec/google/chromeec/acpi/ec.asl index f8d4bdf..993932a 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) { @@ -358,6 +360,9 @@ { Store ("EC: MKBP", Debug) Notify (CREC, 0x80) +#ifdef DPTC_ENABLE + _SB.DPTC(0x0) +#endif }
#ifdef EC_ENABLE_PD_MCU_DEVICE @@ -377,6 +382,9 @@ #ifdef EC_ENABLE_MULTIPLE_DPTF_PROFILES _SB.DPTF.TPET() #endif +#ifdef DPTC_ENABLE + _SB.DPTC(0x1) +#endif #ifdef EC_ENABLE_TBMC_DEVICE Notify (TBMC, 0x80) #endif diff --git a/src/mainboard/google/zork/dsdt.asl b/src/mainboard/google/zork/dsdt.asl index d256f66..e853c25 100644 --- a/src/mainboard/google/zork/dsdt.asl +++ b/src/mainboard/google/zork/dsdt.asl @@ -41,6 +41,8 @@ /* global utility methods expected within the _SB scope */ #include <arch/x86/acpi/globutil.asl>
+ #include <variant/acpi/dptc.asl> + /* Describe the SOC */ #include <soc.asl>
diff --git a/src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dtpc.asl b/src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dtpc.asl new file mode 100644 index 0000000..b421ae3 --- /dev/null +++ b/src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dtpc.asl @@ -0,0 +1,19 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#define SUSTAINED_POWER_LIMIT_MW_0 25000 +#define SLOW_PPT_LIMIT_MW_0 30000 + +#define SUSTAINED_POWER_LIMIT_MW_1 16000 +#define SLOW_PPT_LIMIT_MW_1 20000 + diff --git a/src/mainboard/google/zork/variants/trembyle/include/variant/acpi/dptc.asl b/src/mainboard/google/zork/variants/trembyle/include/variant/acpi/dptc.asl new file mode 100644 index 0000000..d43a9c6 --- /dev/null +++ b/src/mainboard/google/zork/variants/trembyle/include/variant/acpi/dptc.asl @@ -0,0 +1,14 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <baseboard/acpi/dptc.asl> diff --git a/src/soc/amd/picasso/acpi/dptc.asl b/src/soc/amd/picasso/acpi/dptc.asl new file mode 100644 index 0000000..9bd9779 --- /dev/null +++ b/src/soc/amd/picasso/acpi/dptc.asl @@ -0,0 +1,71 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2020 Advanced Micro Devices, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + #define DPTC_CTDP 0x0 + #define DPTC_STAPM_TIME_CONSTANT 0x1 + #define DPTC_SKIN_CONTROL_SCALAR 0x2 + #define DPTC_THERMAL_CONTROL_LIMIT 0x3 + #define DPTC_PACKAGE_POWER_LIMIT 0x4 + #define DPTC_SUSTAINED_POWER_LIMIT 0x5 + #define DPTC_FAST_PPT_LIMIT 0x6 + #define DPTC_SLOW_PPT_LIMIT 0x7 + #define DPTC_SLOW_PPT_TIME_CONSTANT 0x8 + #define DPTC_PROCHOT_L 0x9 + #define DPTC_SYS_TEMP_TRACKING 0xa + #define DPTC_VRM_CURRENT_LIMIT 0xb + #define DPTC_VRM_MAXIMUM_CURRENT_LIMIT 0xc + #define DPTC_VRM_LOW_POWER_THRESHOLD 0xd + #define DPTC_VRM_SOC_CURRENT_LIMIT 0xe + #define DPTC_VRM_SOC_LOW_POWER_THRESHOLD 0xf + #define DPTC_DGPU_CONTROL 0x10 +#define DPTC_PROFILE_0 0x0 +#define DPTC_PROFILE_1 0x1 +External(_SB.ALIB, MethodObj) +Name(DPTI, Buffer(0x07){}) +CreateWordField(DPTI, Zero, SSZE) +CreateByteField(DPTI, 0x02,MSID) +CreateDwordField(DPTI,0x03,DECI) +/* System Bus */ +Method(DPTC, 1, Serialized) +{ + Method(POF0, 0, Serialized) { + Store(0x07, SSZE) + Store(DPTC_SUSTAINED_POWER_LIMIT, MSID) + Store(SUSTAINED_POWER_LIMIT_MW_0, DECI) + ALIB(0xC, DPTI) //for PL1:Sustained power limit + Store(0x07, SSZE) + Store(DPTC_SLOW_PPT_LIMIT, MSID) + Store(SLOW_PPT_LIMIT_MW_0, DECI) + ALIB(0xC, DPTI) //for PL2:Slow PPT limit + } + Method(POF1, 0, Serialized) { + Store(0x07, SSZE) + Store(DPTC_SUSTAINED_POWER_LIMIT, MSID) + Store(SUSTAINED_POWER_LIMIT_MW_1, DECI) + ALIB(0xC, DPTI) //for PL1:Sustained power limit + Store(0x07, SSZE) + Store(DPTC_SLOW_PPT_LIMIT, MSID) + Store(SLOW_PPT_LIMIT_MW_1, DECI) + ALIB(0xC, DPTI) //for PL2:Slow PPT limit + } + Switch (ToInteger(Arg0)) + { + Case (DPTC_PROFILE_0) { + POF0() + } + Case (DPTC_PROFILE_1) { + POF1() + } + } +} diff --git a/src/soc/amd/picasso/acpi/soc.asl b/src/soc/amd/picasso/acpi/soc.asl index b3b036e..909b985 100644 --- a/src/soc/amd/picasso/acpi/soc.asl +++ b/src/soc/amd/picasso/acpi/soc.asl @@ -17,5 +17,7 @@ /* Describe the devices in the Southbridge */ #include "sb_fch.asl"
+#include "dptc.asl" + /* Add GPIO library */ #include <soc/amd/common/acpi/gpio_bank_lib.asl> diff --git a/src/soc/amd/picasso/include/dptc.h b/src/soc/amd/picasso/include/dptc.h new file mode 100644 index 0000000..c4f723b --- /dev/null +++ b/src/soc/amd/picasso/include/dptc.h @@ -0,0 +1,28 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + #define DPTC_CTDP 0x0 + #define DPTC_STAPM_TIME_CONSTANT 0x1 + #define DPTC_SKIN_CONTROL_SCALAR 0x2 + #define DPTC_THERMAL_CONTROL_LIMIT 0x3 + #define DPTC_PACKAGE_POWER_LIMIT 0x4 + #define DPTC_SUSTAINED_POWER_LIMIT 0x5 + #define DPTC_FAST_PPT_LIMIT 0x6 + #define DPTC_SLOW_PPT_LIMIT 0x7 + #define DPTC_SLOW_PPT_TIME_CONSTANT 0x8 + #define DPTC_SYS_TEMP_TRACKING 0xA + #define DPTC_VRM_CURRENT_LIMIT 0xb + #define DPTC_VRM_MAXIMUM_CURRENT_LIMIT 0xc + #define DPTC_VRM_LOW_POWER_THRESHOLD 0xd + #define DPTC_VRM_SOC_CURRENT_LIMIT 0xe + #define DPTC_VRM_SOC_LOW_POWER_THRESHOLD 0xf + #define DPTC_DGPU_CONTROL 0x10
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: [WIP] mb/google/zork: add dptc support ......................................................................
Patch Set 2:
This change is ready for review.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: [WIP] mb/google/zork: add dptc support ......................................................................
Patch Set 2:
Chris, is there a bug associated w/ this work?
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#3).
Change subject: [WIP] mb/google/zork: add dptc support ......................................................................
[WIP] mb/google/zork: add dptc support
add dptc support for tablet/clamshell mode
Profile 0 : clamshell mode Profile 1 : 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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/ec/google/chromeec/acpi/ec.asl M src/mainboard/google/zork/dsdt.asl A src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dtpc.asl A src/mainboard/google/zork/variants/trembyle/include/variant/acpi/dptc.asl A src/soc/amd/picasso/acpi/dptc.asl M src/soc/amd/picasso/acpi/soc.asl A src/soc/amd/picasso/include/dptc.h 7 files changed, 153 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/3
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: [WIP] mb/google/zork: add dptc support ......................................................................
Patch Set 3:
Patch Set 2:
Chris, is there a bug associated w/ this work?
yes,b/157943445 for morphius. this change is still under working.but had been verified by ODM.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: [WIP] mb/google/zork: add dptc support ......................................................................
Patch Set 3:
(1 comment)
Please fix the license headers.
https://review.coreboot.org/c/coreboot/+/43408/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dtpc.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/mainboard/google/zork/v... PS3, Line 3: /* : * This file is part of the coreboot project. : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ Please remove.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: [WIP] mb/google/zork: add dptc support ......................................................................
Patch Set 3:
(8 comments)
https://review.coreboot.org/c/coreboot/+/43408/3/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/ec/google/chromeec/acpi... PS3, Line 13: #ifdef DPTC_ENABLE Please split this file's change into its own patch.
https://review.coreboot.org/c/coreboot/+/43408/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dtpc.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/mainboard/google/zork/v... PS3, Line 17: #define SLOW_PPT_LIMIT_MW_0 30000 Are these constants design specific? If so, we shouldn't put them in baseboard.
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... File src/soc/amd/picasso/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... PS3, Line 2: : /* : * This file is part of the coreboot project. : * : * Copyright (C) 2020 Advanced Micro Devices, Inc. : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ remove
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... PS3, Line 33: #define DPTC_DGPU_CONTROL 0x10 All these #defines are not indented the same.
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... PS3, Line 35: #define DPTC_PROFILE_1 0x1 These seem to be duplicated in the header file. Remove that header file.
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... PS3, Line 37: Name(DPTI, Buffer(0x07){}) Should we scope this named object?
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/so... File src/soc/amd/picasso/acpi/soc.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/so... PS3, Line 20: #include "dptc.asl" Please split soc/amd/picasso changes into their own patch.
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/dptc.h:
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/include... PS3, Line 3: /* : * This file is part of the coreboot project. : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ remove
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: [WIP] mb/google/zork: add dptc support ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43408/3/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/ec/google/chromeec/acpi... PS3, Line 364: _SB.DPTC(0x0) Why is this done here?
https://review.coreboot.org/c/coreboot/+/43408/3/src/ec/google/chromeec/acpi... PS3, Line 386: 0x1 Is the argument 0x1 meant to indicate tablet mode is enabled? If so, then this is not correct. Host event 29 indicates mode change - it could be either tablet mode entry or tablet mode exit(or some other mode change). You will have to check _SB.PCI0.LPCB.EC0.RCDP() in _SB.DPTC() to decide whether tablet mode is entered or exited.
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#4).
Change subject: mb/google/zork: add dptc support ......................................................................
mb/google/zork: add dptc support
add dptc support for tablet/clamshell mode
Profile 0 : clamshell mode Profile 1 : 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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/ec/google/chromeec/acpi/ec.asl A src/soc/amd/picasso/acpi/dptc.asl M src/soc/amd/picasso/acpi/soc.asl 3 files changed, 19 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/4
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#5).
Change subject: mb/google/zork: add dptc support ......................................................................
mb/google/zork: add dptc support
add dptc support for tablet/clamshell mode
Profile 0 : clamshell mode Profile 1 : 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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- A src/soc/amd/picasso/acpi/dptc.asl M src/soc/amd/picasso/acpi/soc.asl 2 files changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/5
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#6).
Change subject: mb/google/zork: add dptc support ......................................................................
mb/google/zork: add dptc support
add dptc support for tablet/clamshell mode
Profile 0 : clamshell mode Profile 1 : 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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- A src/soc/amd/picasso/acpi/dptc.asl M src/soc/amd/picasso/acpi/soc.asl 2 files changed, 18 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/6
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#7).
Change subject: soc/amd/picasso/acpi: add dptc support ......................................................................
soc/amd/picasso/acpi: add dptc support
add dptc support for tablet/clamshell mode
Profile 0 : clamshell mode Profile 1 : 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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- A src/soc/amd/picasso/acpi/dptc.asl M src/soc/amd/picasso/acpi/soc.asl 2 files changed, 18 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/7
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#8).
Change subject: soc/amd/picasso/acpi: add dptc support ......................................................................
soc/amd/picasso/acpi: add dptc support
add dptc support for tablet/clamshell mode
Profile 0 : clamshell mode Profile 1 : 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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- A src/soc/amd/picasso/acpi/dptc.asl M src/soc/amd/picasso/acpi/soc.asl 2 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/8
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso/acpi: add dptc support ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43408/8/src/soc/amd/picasso/acpi/dp... File src/soc/amd/picasso/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/8/src/soc/amd/picasso/acpi/dp... PS8, Line 10: If (LEqual (_SB.PCI0.LPCB.EC0.RCTM , One)) { Please add comments indicating what each method is for.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso/acpi: add dptc support ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43408/8/src/soc/amd/picasso/acpi/dp... File src/soc/amd/picasso/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/8/src/soc/amd/picasso/acpi/dp... PS8, Line 10: LEqual Please use ASL2.0 syntax
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#9).
Change subject: mb/google/zork: add dptc support ......................................................................
mb/google/zork: add dptc support
add dptc support for tablet/clamshell mode
Profile 0 : clamshell mode Profile 1 : 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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/mainboard/google/zork/dsdt.asl A src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl A src/mainboard/google/zork/variants/berknip/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/dalboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/dirinboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/ezkinil/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/morphius/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/trembyle/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/vilboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/woomax/include/variant/acpi/dptc.asl 10 files changed, 77 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/9
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: mb/google/zork: add dptc support ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43408/9/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/9/src/mainboard/google/zork/v... PS9, Line 29: If (_SB.PCI0.LPCB.EC0.RCTM == One) { Please add comments indicating what each method is for.
https://review.coreboot.org/c/coreboot/+/43408/9/src/mainboard/google/zork/v... PS9, Line 37: POF0 These methods don't do anything? What is the purpose of these methods? And why is this file in mainboard? Do we expect the same logic across designs? If so, we should move it to soc?
https://review.coreboot.org/c/coreboot/+/43408/8/src/soc/amd/picasso/acpi/dp... File src/soc/amd/picasso/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/8/src/soc/amd/picasso/acpi/dp... PS8, Line 10: LEqual
Please use ASL2. […]
move this method to src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl and also been modified for ASL2.0.
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#10).
Change subject: mb/google/zork: add dptc support ......................................................................
mb/google/zork: add dptc support
add dptc support for different power parameter on tablet/clamshell mode
Profile 0 : clamshell mode Profile 1 : Tablet mode
The BIOS may choose to adjust power and/or thermal parameters at its own discretion. DPTCi ALIB Function adds flexibility by allowing the BIOS to request power state changes independently of specific events.
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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/mainboard/google/zork/dsdt.asl A src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl A src/mainboard/google/zork/variants/berknip/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/dalboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/dirinboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/ezkinil/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/morphius/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/trembyle/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/vilboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/woomax/include/variant/acpi/dptc.asl 10 files changed, 87 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/10
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#11).
Change subject: mb/google/zork: add dptc support ......................................................................
mb/google/zork: add dptc support
add dptc support for different power parameter on tablet/clamshell mode
Profile 0 : clamshell mode Profile 1 : Tablet mode
The BIOS may choose to adjust power and/or thermal parameters at its own discretion. DPTCi ALIB Function adds flexibility by allowing the BIOS to request power state changes independently of specific events.
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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/mainboard/google/zork/dsdt.asl A src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl A src/mainboard/google/zork/variants/berknip/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/dalboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/dirinboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/ezkinil/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/morphius/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/trembyle/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/vilboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/woomax/include/variant/acpi/dptc.asl 10 files changed, 79 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/11
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: mb/google/zork: add dptc support ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43408/8/src/soc/amd/picasso/acpi/dp... File src/soc/amd/picasso/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/8/src/soc/amd/picasso/acpi/dp... PS8, Line 10: If (LEqual (_SB.PCI0.LPCB.EC0.RCTM , One)) {
Please add comments indicating what each method is for.
move this method to src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl and also add comment for indicate which mode used for.
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: mb/google/zork: add dptc support ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43408/9/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/9/src/mainboard/google/zork/v... PS9, Line 29: If (_SB.PCI0.LPCB.EC0.RCTM == One) {
Please add comments indicating what each method is for.
Done
https://review.coreboot.org/c/coreboot/+/43408/9/src/mainboard/google/zork/v... PS9, Line 37: POF0
These methods don't do anything? What is the purpose of these methods? And why is this file in mainb […]
The dptc should used for specific project design, so move it on the board layer. the project which need specific power parameter via dptc, it need implement in variant/dptc.asl
Keith Tzeng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: mb/google/zork: add dptc support ......................................................................
Patch Set 11:
please help review this, thanks.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: mb/google/zork: add dptc support ......................................................................
Patch Set 11:
Patch Set 11:
please help review this, thanks.
Hello Chris/Keith - I will review and post comments on these CLs tomorrow. Thanks!
Keith Tzeng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: mb/google/zork: add dptc support ......................................................................
Patch Set 11:
Please help review, thanks.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: mb/google/zork: add dptc support ......................................................................
Patch Set 11:
(7 comments)
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 5: #define DPTC_THERMAL_CONTROL_LIMIT 0x3 : #define DPTC_SUSTAINED_POWER_LIMIT 0x5 : #define DPTC_FAST_PPT_LIMIT 0x6 : #define DPTC_SLOW_PPT_LIMIT 0x7 : #define DPTC_SLOW_PPT_TIME_CONSTANT 0x8 : #define DPTC_PROCHOT_L 0x9 : #define DPTC_VRM_CURRENT_LIMIT 0xb : #define DPTC_VRM_MAXIMUM_CURRENT_LIMIT 0xc : #define DPTC_VRM_SOC_CURRENT_LIMIT 0xe : Are these register offsets? Or actual limit values?
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 26: POF1 These are just empty functions. Do you intend to add more changes later?
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 33: Sample call What does this mean?
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 34: POFX Where does this get called from?
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 40: Store(0x07, SSZE) Having comments about what each of these statements is doing would be helpful.
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 42: 4.8w Does this apply to all variants using all types of processors? Picasso, Dali, Pollock?
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 43: ALIB Shouldn't the call to ALIB be done from SoC code? Ideally, I think we should generate the code using runtime SSDT generator and using mainboard devicetree configs to generate the code.
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#12).
Change subject: mb/google/zork: add dptc support ......................................................................
mb/google/zork: add dptc support
add dptc support for different power parameter on tablet/clamshell mode
Profile 0 : clamshell mode Profile 1 : Tablet mode
The BIOS may choose to adjust power and/or thermal parameters at its own discretion. DPTCi ALIB Function adds flexibility by allowing the BIOS to request power state changes independently of specific events.
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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/mainboard/google/zork/dsdt.asl A src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl A src/mainboard/google/zork/variants/berknip/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/dalboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/dirinboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/ezkinil/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/morphius/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/trembyle/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/vilboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/woomax/include/variant/acpi/dptc.asl 10 files changed, 88 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/12
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#13).
Change subject: mb/google/zork: add dptc support ......................................................................
mb/google/zork: add dptc support
add dptc support for different power parameter on tablet/clamshell mode
Profile 0 : clamshell mode Profile 1 : Tablet mode
The BIOS may choose to adjust power and/or thermal parameters at its own discretion. DPTCi ALIB Function adds flexibility by allowing the BIOS to request power state changes independently of specific events.
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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/mainboard/google/zork/dsdt.asl A src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl A src/mainboard/google/zork/variants/berknip/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/dalboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/dirinboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/ezkinil/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/morphius/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/trembyle/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/vilboz/include/variant/acpi/dptc.asl A src/mainboard/google/zork/variants/woomax/include/variant/acpi/dptc.asl 10 files changed, 88 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/13
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: mb/google/zork: add dptc support ......................................................................
Patch Set 13:
(7 comments)
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 5: #define DPTC_THERMAL_CONTROL_LIMIT 0x3 : #define DPTC_SUSTAINED_POWER_LIMIT 0x5 : #define DPTC_FAST_PPT_LIMIT 0x6 : #define DPTC_SLOW_PPT_LIMIT 0x7 : #define DPTC_SLOW_PPT_TIME_CONSTANT 0x8 : #define DPTC_PROCHOT_L 0x9 : #define DPTC_VRM_CURRENT_LIMIT 0xb : #define DPTC_VRM_MAXIMUM_CURRENT_LIMIT 0xc : #define DPTC_VRM_SOC_CURRENT_LIMIT 0xe :
Are these register offsets? Or actual limit values?
These are the parameter ID for correspond power or thermal parameters.
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 26: POF1
These are just empty functions. […]
No, It should be a dummy function and if any board needs to adjust their setting for the tablet mode. Then they should have their own POF1 in variant.
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 33: Sample call
What does this mean?
A sample for the call to ALIB with DPTCi
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 34: POFX
Where does this get called from?
will not call this method. it's a sample name.
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 40: Store(0x07, SSZE)
Having comments about what each of these statements is doing would be helpful.
yes, the comment added.
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 42: 4.8w
Does this apply to all variants using all types of processors? Picasso, Dali, Pollock?
yes, the they support and can be apply by DPTC.
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 43: ALIB
Shouldn't the call to ALIB be done from SoC code? Ideally, I think we should generate the code using […]
Agree with that. But I haven't studied how to do that. maybe will have a second version in further.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: mb/google/zork: add dptc support ......................................................................
Patch Set 13:
(2 comments)
I have posted some comments on the bug. I don't think adding sample code is the right thing to do here. It doesn't really help any partner devices. Please see my proposal on how we can make use of SSDT and devicetree configs to generate the required code at runtime.
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 26: POF1
No, It should be a dummy function and if any board needs to adjust their setting for the tablet mode […]
I don't understand what you mean by having a dummy function. How would a variant use this? What is it expected to do in these calls?
Also, since you have already provided implementations of POF1 and POF0 below, if any variant adds these methods, it is going to result in ACPI errors complaining about duplicate methods. How do you plan to handle that?
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 34: POFX
will not call this method. it's a sample name.
We don't really want to keep dead code which isn't really called/used. What is the intent of this function? Are variants expected to make a copy of this?
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#14).
Change subject: soc/amd/picasso: add dptc support ......................................................................
soc/amd/picasso: add dptc support
add dptc support for different power parameter on tablet/clamshell mode
Profile 0 : clamshell mode Profile 1 : Tablet mode
The BIOS may choose to adjust power and/or thermal parameters at its own discretion. DPTCi ALIB Function adds flexibility by allowing the BIOS to request power state changes independently of specific events.
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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/soc/amd/picasso/chip.h A src/soc/amd/picasso/include/soc/dptc.h M src/soc/amd/picasso/root_complex.c 3 files changed, 110 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/14
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 14:
(4 comments)
https://review.coreboot.org/c/coreboot/+/43408/14/src/soc/amd/picasso/includ... File src/soc/amd/picasso/include/soc/dptc.h:
https://review.coreboot.org/c/coreboot/+/43408/14/src/soc/amd/picasso/includ... PS14, Line 17: #endif /* __PICASSO_DPTC_H__ */ adding a line without newline at end of file
https://review.coreboot.org/c/coreboot/+/43408/14/src/soc/amd/picasso/root_c... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/43408/14/src/soc/amd/picasso/root_c... PS14, Line 153: acpigen_emit_namestring("SSZE");; Statements terminations use 1 semicolon
https://review.coreboot.org/c/coreboot/+/43408/14/src/soc/amd/picasso/root_c... PS14, Line 207: dptci_update_parameter(DPTC_SUSTAINED_POWER_LIMIT, config->dptc_sustained_power_limit); line over 96 characters
https://review.coreboot.org/c/coreboot/+/43408/14/src/soc/amd/picasso/root_c... PS14, Line 214: dptci_update_parameter(DPTC_SUSTAINED_POWER_LIMIT, config->sustained_power_limit); line over 96 characters
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#15).
Change subject: soc/amd/picasso: add dptc support ......................................................................
soc/amd/picasso: add dptc support
add dptc support for different power parameter on tablet/clamshell mode
Profile 0 : clamshell mode Profile 1 : Tablet mode
The BIOS may choose to adjust power and/or thermal parameters at its own discretion. DPTCi ALIB Function adds flexibility by allowing the BIOS to request power state changes independently of specific events.
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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/soc/amd/picasso/chip.h A src/soc/amd/picasso/include/soc/dptc.h M src/soc/amd/picasso/root_complex.c 3 files changed, 110 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/15
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 15:
(4 comments)
https://review.coreboot.org/c/coreboot/+/43408/15/src/soc/amd/picasso/includ... File src/soc/amd/picasso/include/soc/dptc.h:
https://review.coreboot.org/c/coreboot/+/43408/15/src/soc/amd/picasso/includ... PS15, Line 17: #endif /* __PICASSO_DPTC_H__ */ adding a line without newline at end of file
https://review.coreboot.org/c/coreboot/+/43408/15/src/soc/amd/picasso/root_c... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/43408/15/src/soc/amd/picasso/root_c... PS15, Line 153: acpigen_emit_namestring("SSZE");; Statements terminations use 1 semicolon
https://review.coreboot.org/c/coreboot/+/43408/15/src/soc/amd/picasso/root_c... PS15, Line 207: dptci_update_parameter(DPTC_SUSTAINED_POWER_LIMIT, config->dptc_sustained_power_limit); line over 96 characters
https://review.coreboot.org/c/coreboot/+/43408/15/src/soc/amd/picasso/root_c... PS15, Line 214: dptci_update_parameter(DPTC_SUSTAINED_POWER_LIMIT, config->sustained_power_limit); line over 96 characters
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#16).
Change subject: soc/amd/picasso: add dptc support ......................................................................
soc/amd/picasso: add dptc support
add dptc support for different power parameter on tablet/clamshell mode
The BIOS may choose to adjust power and/or thermal parameters at its own discretion. DPTCi ALIB Function adds flexibility by allowing the BIOS to request power state changes independently of specific events.
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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/soc/amd/picasso/chip.h A src/soc/amd/picasso/include/soc/dptc.h M src/soc/amd/picasso/root_complex.c 3 files changed, 112 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/16
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 16:
(8 comments)
https://review.coreboot.org/c/coreboot/+/43408/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43408/16//COMMIT_MSG@12 PS16, Line 12: DPTCi I think the i here means interface? It would be good to mention that just for context. DPTC interface(DPTCi) or something on those lines.
https://review.coreboot.org/c/coreboot/+/43408/16//COMMIT_MSG@17 PS16, Line 17: TEST=Build. check the setting changed. Can you please add the generated ACPI code from SSDT to the commit message for reference?
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/chip.h... PS16, Line 160: uint8_t dptc_enable; : uint32_t dptc_fast_ppt_limit; : uint32_t dptc_slow_ppt_limit; : uint32_t dptc_sustained_power_limit; Can you please move this up along with the other ppt limit params on line 87?
Also, I think we should name this something like
uint32_t fast_ppt_limit_tablet_mode; uint32_t slow_ppt_limit_tablet_mode; uint32_t sustained_power_limit_tablet_mode;
This makes it clear that the limits are associated with tablet mode. We could ideally change the other limits on line 87 to add _default or just leave the fields as is.
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/includ... File src/soc/amd/picasso/include/soc/dptc.h:
PS16: You don't need a .h file for this. These are used only within a single .c file.
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... PS16, Line 136: static void dptci_setup(void) I think these functions can actually be simplified so that we don't have to create and access fields within buffer and do more within C code.
``` enum { ALIB_DPTCI_FUNCTION_ID = 0xc,
SUSTAINED_POWER_LIMIT_PARAM_ID = 0x5, FAST_PPT_LIMIT_PARAM_ID = 0x6, SLOW_PPT_LIMIT_PARAM_ID = 0x7,
DPTC_TOTAL_UPDATE_PARAMS = 3, };
struct dptc_param { uint8_t id; uint32_t value; } __packed;
struct dptc_input { uint16_t size; struct dptc_param params[DPTC_TOTAL_UPDATE_PARAMS]; } __packed;
#define DPTC_INPUTS(_sustained, _fast, _slow) \ { \ .size = sizeof(struct dptc_input), \ .params = { \ { \ .id = SUSTAINED_POWER_LIMIT_PARAM_ID, \ .value = _sustained, \ }, \ { \ .id = FAST_PPT_LIMIT_PARAM_ID, \ .value = _fast, \ }, \ { \ .id = SLOW_PPT_LIMIT_PARAM_ID, \ .value = _slow, \ }, \ }
static void dptc_call_alib(const char *buf_name, uint8_t *buffer, size_t size) { /* Name (buf_name, Buffer(size) {...} */ acpigen_write_name(buf_name); acpigen_write_byte_buffer(buffer, size);
/* _SB.ALIB(0xc, buf_name) */ acpigen_emit_namestring("\_SB.ALIB"); acpigen_write_integer(ALIB_DPTCI_FUNCTION_ID); acpigen_emit_namestring(buf_name); }
static void acipgen_dptci(void) { const config_t *config = config_of_soc();
if (!config->dptc_enable) return;
struct dptc_input default_input = DPTC_INPUTS(config->sustained_power_limit, config->fast_ppt_limit, config->slow_ppt_limit); struct dptc_input tablet_mode_input = DPTC_INPUTS(config->sustained_power_limit_tablet_mode, config->fast_ppt_limit_tablet_mode, config->slow_ppt_limit_tablet_mode);
/* Scope (_SB) */ acpigen_write_scope("\_SB");
/* Method(DPTC, 0, Serialized) */ acpigen_write_method_serialized("DPTC", 0);
/* If (LEqual ("_SB.PCI0.LPCB.EC0.TBMD", 1)) */ acpigen_write_if_lequal_namestr_int("\_SB.PCI0.LPCB.EC0.TBMD", 1);
dptc_call_alib("TABB", (uint8_t *)(void *)&tablet_mode_input, sizeof(tablet_mode_input));
acpigen_pop_len(); /* If */
/* Else */ acpigen_write_else();
dptc_call_alib("DEFB", (uint8_t *)(void *)&default_input, sizeof(default_input));
acpigen_pop_len(); /* Else */
acpigen_pop_len(); /* Method DPTC */ acpigen_pop_len(); /* Scope _SB */ } ```
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... PS16, Line 140: acpigen_write_method If you want serialized, then this should be acpigen_write_method_serialized.
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... PS16, Line 195: dptci_setup(); I think we don't really need this if dptc_enable is not true? Just bail out early:
``` if (!config->dptc_enable) return; ```
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... PS16, Line 205: dptci_update_parameter(DPTC_FAST_PPT_LIMIT, config->dptc_fast_ppt_limit); : dptci_update_parameter(DPTC_SLOW_PPT_LIMIT, config->dptc_slow_ppt_limit); : dptci_update_parameter(DPTC_SUSTAINED_POWER_LIMIT, : config->dptc_sustained_power_limit); Why are we making 3 separate calls to ALIB? As per the spec it looks like ALIB allows multiple parameters to be passed in at the same time?
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#17).
Change subject: soc/amd/picasso: add dptc support ......................................................................
soc/amd/picasso: add dptc support
add dptc support for different power parameter on tablet/clamshell mode
The BIOS may choose to adjust power and/or thermal parameters at its own discretion. DPTCi ALIB Function adds flexibility by allowing the BIOS to request power state changes independently of specific events.
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: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/soc/amd/picasso/chip.h A src/soc/amd/picasso/include/soc/dptc.h M src/soc/amd/picasso/root_complex.c 3 files changed, 112 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/17
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#18).
Change subject: soc/amd/picasso: add dptc support ......................................................................
soc/amd/picasso: add dptc support
add dptc support for different power parameter on tablet/clamshell mode
The BIOS may choose to adjust power and/or thermal parameters at its own discretion. The DPTC interface(DPTCi) ALIB Function adds flexibility by allowing the BIOS to request power state changes independently of specific events.
BUG=b:157943445 BRANCH=none TEST=Build.Generated ASL code from SSDT.check the setting changed.
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/root_complex.c 2 files changed, 94 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/18
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 18:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43408/18/src/soc/amd/picasso/root_c... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/43408/18/src/soc/amd/picasso/root_c... PS18, Line 36: #define DPTC_INPUTS(_sustained, _fast, _slow) \ please, no space before tabs
https://review.coreboot.org/c/coreboot/+/43408/18/src/soc/amd/picasso/root_c... PS18, Line 202: config->sustained_power_limit_tablet_mode, line over 96 characters
https://review.coreboot.org/c/coreboot/+/43408/18/src/soc/amd/picasso/root_c... PS18, Line 214: dptc_call_alib("TABB", (uint8_t *)(void *)&tablet_mode_input, sizeof(tablet_mode_input)); line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 19:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43408/19/src/soc/amd/picasso/root_c... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/43408/19/src/soc/amd/picasso/root_c... PS19, Line 36: #define DPTC_INPUTS(_sustained, _fast, _slow) \ please, no space before tabs
https://review.coreboot.org/c/coreboot/+/43408/19/src/soc/amd/picasso/root_c... PS19, Line 202: config->sustained_power_limit_tablet_mode, line over 96 characters
https://review.coreboot.org/c/coreboot/+/43408/19/src/soc/amd/picasso/root_c... PS19, Line 214: dptc_call_alib("TABB", (uint8_t *)(void *)&tablet_mode_input, sizeof(tablet_mode_input)); line over 96 characters
Hello build bot (Jenkins), Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#20).
Change subject: soc/amd/picasso: add dptc support ......................................................................
soc/amd/picasso: add dptc support
add dptc support for different power parameter on tablet/clamshell mode
The BIOS may choose to adjust power and/or thermal parameters at its own discretion. The DPTC interface(DPTCi) ALIB Function adds flexibility by allowing the BIOS to request power state changes independently of specific events.
BUG=b:157943445 BRANCH=none TEST=Build.Generated ASL code from SSDT.check the setting changed.
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/root_complex.c 2 files changed, 95 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/20
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 20: Code-Review+1
(5 comments)
Minor nits. But LGTM overall.
https://review.coreboot.org/c/coreboot/+/43408/20//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43408/20//COMMIT_MSG@18 PS20, Line 18: Generated ASL code from SSDT Can you please add the generated SSDT code for the DPTC routine here in the commit message?
https://review.coreboot.org/c/coreboot/+/43408/20/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/43408/20/src/soc/amd/picasso/chip.h... PS20, Line 93: * nit: space before *
https://review.coreboot.org/c/coreboot/+/43408/20/src/soc/amd/picasso/chip.h... PS20, Line 94: nit: One space is sufficient.
https://review.coreboot.org/c/coreboot/+/43408/20/src/soc/amd/picasso/chip.h... PS20, Line 95: fast_ppt_limit_tablet_mode I think it would be good to add a comment indicating that these limits are used only when dptc_enable is set to 1.
https://review.coreboot.org/c/coreboot/+/43408/20/src/soc/amd/picasso/root_c... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/43408/20/src/soc/amd/picasso/root_c... PS20, Line 38: .size = sizeof(struct dptc_input), \ : .params = { \ : { \ : .id = SUSTAINED_POWER_LIMIT_PARAM_ID, \ : .value = _sustained, \ : }, \ : { \ : .id = FAST_PPT_LIMIT_PARAM_ID, \ : .value = _fast, \ : }, \ : { \ : .id = SLOW_PPT_LIMIT_PARAM_ID, \ : .value = _slow, \ : }, nit: this whole block can be shifted left by one tab.
Hello build bot (Jenkins), Furquan Shaikh, Chris Wang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43408
to look at the new patch set (#21).
Change subject: soc/amd/picasso: add dptc support ......................................................................
soc/amd/picasso: add dptc support
add dptc support for different power parameter on tablet/clamshell mode
The BIOS may choose to adjust power and/or thermal parameters at its own discretion. The DPTC interface(DPTCi) ALIB Function adds flexibility by allowing the BIOS to request power state changes independently of specific events.
BUG=b:157943445 BRANCH=none TEST=Build.Generated ASL code from SSDT by acipgen_dptci().check the setting changed.
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d --- M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/root_complex.c 2 files changed, 97 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43408/21
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 21: Code-Review+2
(4 comments)
https://review.coreboot.org/c/coreboot/+/43408/20/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/43408/20/src/soc/amd/picasso/chip.h... PS20, Line 93: *
nit: space before *
Done
https://review.coreboot.org/c/coreboot/+/43408/20/src/soc/amd/picasso/chip.h... PS20, Line 94:
nit: One space is sufficient.
Done
https://review.coreboot.org/c/coreboot/+/43408/20/src/soc/amd/picasso/chip.h... PS20, Line 95: fast_ppt_limit_tablet_mode
I think it would be good to add a comment indicating that these limits are used only when dptc_enabl […]
Done
https://review.coreboot.org/c/coreboot/+/43408/20/src/soc/amd/picasso/root_c... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/43408/20/src/soc/amd/picasso/root_c... PS20, Line 38: .size = sizeof(struct dptc_input), \ : .params = { \ : { \ : .id = SUSTAINED_POWER_LIMIT_PARAM_ID, \ : .value = _sustained, \ : }, \ : { \ : .id = FAST_PPT_LIMIT_PARAM_ID, \ : .value = _fast, \ : }, \ : { \ : .id = SLOW_PPT_LIMIT_PARAM_ID, \ : .value = _slow, \ : },
nit: this whole block can be shifted left by one tab.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 21:
(21 comments)
Chris - just for future reference - all pending comments have to be marked as resolved in order for gerrit to provide the submit option.
https://review.coreboot.org/c/coreboot/+/43408/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43408/16//COMMIT_MSG@12 PS16, Line 12: DPTCi
I think the i here means interface? It would be good to mention that just for context. […]
Done
https://review.coreboot.org/c/coreboot/+/43408/16//COMMIT_MSG@17 PS16, Line 17: TEST=Build. check the setting changed.
Can you please add the generated ACPI code from SSDT to the commit message for reference?
Done
https://review.coreboot.org/c/coreboot/+/43408/20//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43408/20//COMMIT_MSG@18 PS20, Line 18: Generated ASL code from SSDT
Can you please add the generated SSDT code for the DPTC routine here in the commit message?
Done
https://review.coreboot.org/c/coreboot/+/43408/3/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/ec/google/chromeec/acpi... PS3, Line 13: #ifdef DPTC_ENABLE
Please split this file's change into its own patch.
Done
https://review.coreboot.org/c/coreboot/+/43408/3/src/ec/google/chromeec/acpi... PS3, Line 364: _SB.DPTC(0x0)
Why is this done here?
Done
https://review.coreboot.org/c/coreboot/+/43408/3/src/ec/google/chromeec/acpi... PS3, Line 386: 0x1
Is the argument 0x1 meant to indicate tablet mode is enabled? If so, then this is not correct. […]
Done
https://review.coreboot.org/c/coreboot/+/43408/9/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/9/src/mainboard/google/zork/v... PS9, Line 37: POF0
The dptc should used for specific project design, so move it on the board layer. […]
Done
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 26: POF1
I don't understand what you mean by having a dummy function. […]
Done
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 34: POFX
We don't really want to keep dead code which isn't really called/used. […]
Done
https://review.coreboot.org/c/coreboot/+/43408/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dtpc.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/mainboard/google/zork/v... PS3, Line 3: /* : * This file is part of the coreboot project. : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */
Please remove.
Done
https://review.coreboot.org/c/coreboot/+/43408/3/src/mainboard/google/zork/v... PS3, Line 17: #define SLOW_PPT_LIMIT_MW_0 30000
Are these constants design specific? If so, we shouldn't put them in baseboard.
Done
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... File src/soc/amd/picasso/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... PS3, Line 2: : /* : * This file is part of the coreboot project. : * : * Copyright (C) 2020 Advanced Micro Devices, Inc. : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */
remove
Done
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... PS3, Line 33: #define DPTC_DGPU_CONTROL 0x10
All these #defines are not indented the same.
Done
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... PS3, Line 35: #define DPTC_PROFILE_1 0x1
These seem to be duplicated in the header file. Remove that header file.
Done
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... PS3, Line 37: Name(DPTI, Buffer(0x07){})
Should we scope this named object?
Done
https://review.coreboot.org/c/coreboot/+/43408/8/src/soc/amd/picasso/acpi/dp... File src/soc/amd/picasso/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/8/src/soc/amd/picasso/acpi/dp... PS8, Line 10: LEqual
move this method to src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc. […]
Done
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/so... File src/soc/amd/picasso/acpi/soc.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/so... PS3, Line 20: #include "dptc.asl"
Please split soc/amd/picasso changes into their own patch.
Done
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/chip.h... PS16, Line 160: uint8_t dptc_enable; : uint32_t dptc_fast_ppt_limit; : uint32_t dptc_slow_ppt_limit; : uint32_t dptc_sustained_power_limit;
Can you please move this up along with the other ppt limit params on line 87? […]
Done
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/includ... File src/soc/amd/picasso/include/soc/dptc.h:
PS16:
You don't need a .h file for this. These are used only within a single .c file.
Done
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... PS16, Line 136: static void dptci_setup(void)
I think these functions can actually be simplified so that we don't have to create and access fields […]
Done
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... PS16, Line 140: acpigen_write_method
If you want serialized, then this should be acpigen_write_method_serialized.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/dptc.h:
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/include... PS3, Line 3: /* : * This file is part of the coreboot project. : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */
remove
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 21:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... PS16, Line 195: dptci_setup();
I think we don't really need this if dptc_enable is not true? Just bail out early: […]
Done
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... PS16, Line 205: dptci_update_parameter(DPTC_FAST_PPT_LIMIT, config->dptc_fast_ppt_limit); : dptci_update_parameter(DPTC_SLOW_PPT_LIMIT, config->dptc_slow_ppt_limit); : dptci_update_parameter(DPTC_SUSTAINED_POWER_LIMIT, : config->dptc_sustained_power_limit);
Why are we making 3 separate calls to ALIB? As per the spec it looks like ALIB allows multiple param […]
Done
chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 21:
Patch Set 21:
(21 comments)
Chris - just for future reference - all pending comments have to be marked as resolved in order for gerrit to provide the submit option.
Yeah, Thanks for your remind. I forgot to reply to those comments after making the change.
Keith Tzeng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 21:
could we land this?
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
soc/amd/picasso: add dptc support
add dptc support for different power parameter on tablet/clamshell mode
The BIOS may choose to adjust power and/or thermal parameters at its own discretion. The DPTC interface(DPTCi) ALIB Function adds flexibility by allowing the BIOS to request power state changes independently of specific events.
BUG=b:157943445 BRANCH=none TEST=Build.Generated ASL code from SSDT by acipgen_dptci().check the setting changed.
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: Icae94103f254f8fdb84e6ee0f5404fb09fa97b2d Reviewed-on: https://review.coreboot.org/c/coreboot/+/43408 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/root_complex.c 2 files changed, 97 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h index e3da255..1167509 100644 --- a/src/soc/amd/picasso/chip.h +++ b/src/soc/amd/picasso/chip.h @@ -90,6 +90,14 @@ uint32_t stapm_time_constant; uint32_t sustained_power_limit;
+ /* Enable dptc for tablet mode (0 = disable, 1 = enable) */ + uint8_t dptc_enable; + + /* STAPM Configuration for tablet mode (need enable dptc_enable first) */ + uint32_t fast_ppt_limit_tablet_mode; + uint32_t slow_ppt_limit_tablet_mode; + uint32_t sustained_power_limit_tablet_mode; + /* PROCHOT_L de-assertion Ramp Time */ uint32_t prochot_l_deassertion_ramp_time;
diff --git a/src/soc/amd/picasso/root_complex.c b/src/soc/amd/picasso/root_complex.c index 21af481..6c721e1 100644 --- a/src/soc/amd/picasso/root_complex.c +++ b/src/soc/amd/picasso/root_complex.c @@ -13,7 +13,44 @@ #include <stdint.h> #include <soc/memmap.h> #include <soc/iomap.h> +#include "chip.h"
+enum { + ALIB_DPTCI_FUNCTION_ID = 0xc, + SUSTAINED_POWER_LIMIT_PARAM_ID = 0x5, + FAST_PPT_LIMIT_PARAM_ID = 0x6, + SLOW_PPT_LIMIT_PARAM_ID = 0x7, + DPTC_TOTAL_UPDATE_PARAMS = 3, +}; + +struct dptc_param { + uint8_t id; + uint32_t value; +} __packed; + +struct dptc_input { + uint16_t size; + struct dptc_param params[DPTC_TOTAL_UPDATE_PARAMS]; +} __packed; + +#define DPTC_INPUTS(_sustained, _fast, _slow) \ + { \ + .size = sizeof(struct dptc_input), \ + .params = { \ + { \ + .id = SUSTAINED_POWER_LIMIT_PARAM_ID, \ + .value = _sustained, \ + }, \ + { \ + .id = FAST_PPT_LIMIT_PARAM_ID, \ + .value = _fast, \ + }, \ + { \ + .id = SLOW_PPT_LIMIT_PARAM_ID, \ + .value = _slow, \ + }, \ + }, \ + } /* * * +--------------------------------+ @@ -139,6 +176,57 @@ gnb_apic->flags = IORESOURCE_MEM | IORESOURCE_ASSIGNED | IORESOURCE_FIXED; }
+static void dptc_call_alib(const char *buf_name, uint8_t *buffer, size_t size) +{ + /* Name (buf_name, Buffer(size) {...} */ + acpigen_write_name(buf_name); + acpigen_write_byte_buffer(buffer, size); + + /* _SB.ALIB(0xc, buf_name) */ + acpigen_emit_namestring("\_SB.ALIB"); + acpigen_write_integer(ALIB_DPTCI_FUNCTION_ID); + acpigen_emit_namestring(buf_name); +} + +static void acipgen_dptci(void) +{ + const config_t *config = config_of_soc(); + + if (!config->dptc_enable) + return; + + struct dptc_input default_input = DPTC_INPUTS(config->sustained_power_limit, + config->fast_ppt_limit, + config->slow_ppt_limit); + struct dptc_input tablet_mode_input = DPTC_INPUTS( + config->sustained_power_limit_tablet_mode, + config->fast_ppt_limit_tablet_mode, + config->slow_ppt_limit_tablet_mode); + /* Scope (_SB) */ + acpigen_write_scope("\_SB"); + + /* Method(DPTC, 0, Serialized) */ + acpigen_write_method_serialized("DPTC", 0); + + /* If (LEqual ("_SB.PCI0.LPCB.EC0.TBMD", 1)) */ + acpigen_write_if_lequal_namestr_int("\_SB.PCI0.LPCB.EC0.TBMD", 1); + + dptc_call_alib("TABB", (uint8_t *)(void *)&tablet_mode_input, + sizeof(tablet_mode_input)); + + acpigen_pop_len(); /* If */ + + /* Else */ + acpigen_write_else(); + + dptc_call_alib("DEFB", (uint8_t *)(void *)&default_input, sizeof(default_input)); + + acpigen_pop_len(); /* Else */ + + acpigen_pop_len(); /* Method DPTC */ + acpigen_pop_len(); /* Scope _SB */ +} + /* Used by _SB.PCI0._CRS */ static void root_complex_fill_ssdt(const struct device *device) { @@ -164,6 +252,7 @@ */ acpigen_write_name_dword("TOM2", (msr.hi << 12) | msr.lo >> 20); acpigen_pop_len(); + acipgen_dptci(); }
static struct device_operations root_complex_operations = {
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 22:
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/19638 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19637 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19636 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19635 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19634 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19642 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19641 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19640 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19639
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
chris wang (Code Review)
-
Furquan Shaikh (Code Review)
-
Keith Tzeng (Code Review)
-
Paul Menzel (Code Review)