Change in coreboot[master]: mb/google/hatch: Support separate fixed PL1 setting for tablet mode
Hello Seunghwan Kim,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/35533
to review the following change.
Change subject: mb/google/hatch: Support separate fixed PL1 setting for tablet mode ......................................................................
mb/google/hatch: Support separate fixed PL1 setting for tablet mode
Some variants need separate PL1 setting since thier thermal tuning, but DPTF passive policy 1.0 cannot support it.
So we would use MMIO PL1 register which is currently unused, it can reserve another PL1 setting for tablet mode.
If the MMIO PL1 setting is lower than DPTF PL1 max setting, it will limit PL1 when DPTF sets higher PL1 in tablet mode. Otherwise, if the MMIO PL1 setting is lower than DPTF PL1 min setting, system will have a fixed PL1 value in tablet mode.
BUG=b:138395625 BRANCH=none TEST=Verified MMIO PL1 value and it's enabled in tablet mode when it set
Change-Id: I81c33f9df3e5431f04a08395141b5dc989474289 Signed-off-by: Seunghwan Kim sh_.kim@samsung.com --- M src/ec/google/chromeec/acpi/ec.asl M src/mainboard/google/hatch/mainboard.asl M src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/cpu.c 5 files changed, 32 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/35533/1
diff --git a/src/ec/google/chromeec/acpi/ec.asl b/src/ec/google/chromeec/acpi/ec.asl index 962988e..0c4cc35 100644 --- a/src/ec/google/chromeec/acpi/ec.asl +++ b/src/ec/google/chromeec/acpi/ec.asl @@ -378,6 +378,9 @@ #ifdef EC_ENABLE_TBMC_DEVICE Notify (TBMC, 0x80) #endif +#ifdef EC_ENABLE_MAINBOERD_TABLET_MODE_EVENT + _SB.MTME(^TBMD) +#endif }
/* diff --git a/src/mainboard/google/hatch/mainboard.asl b/src/mainboard/google/hatch/mainboard.asl index dff1a75..769d429 100644 --- a/src/mainboard/google/hatch/mainboard.asl +++ b/src/mainboard/google/hatch/mainboard.asl @@ -55,3 +55,22 @@ LOCL (0) } } + +/* + * Additional action for tablet mode switch event + * Called from _SB.PCI0.LPCB.EC0._Q1D + */ +Method (MTME, 1, Serialized) +{ + OperationRegion (MCHB, + SystemMemory, Add (_SB.PCI0.GMHB(), 0x5000), 0x1000) + Field (MCHB, DWordAcc, Lock, Preserve) + { + Offset (0x9a0), /* PKG_POWER_LIMIT_LO */ + , 15, /* PKG_POWER_LIMIT_MASK */ + MP1E, 1, /* PKG_POWER_LIMIT_EN */ + } + + /* Enable MMIO PL1 at tablet mode */ + Store(Arg0, MP1E) +} diff --git a/src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h b/src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h index 84e050e..7cc5269 100644 --- a/src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h +++ b/src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h @@ -19,5 +19,6 @@ #include <baseboard/ec.h>
#define EC_ENABLE_MULTIPLE_DPTF_PROFILES +#define EC_ENABLE_MAINBOERD_TABLET_MODE_EVENT
#endif diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index 451c920..df6be35 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -232,6 +232,8 @@
/* PL1 Override value in Watts */ uint32_t tdp_pl1_override; + /* PL1 MMIO Override value in Watts */ + uint32_t mmio_pl1_override; /* PL2 Override value in Watts */ uint32_t tdp_pl2_override; /* SysPL2 Value in Watts */ diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c index 0f4d52e..4ec45e5 100644 --- a/src/soc/intel/cannonlake/cpu.c +++ b/src/soc/intel/cannonlake/cpu.c @@ -160,6 +160,13 @@ limit.hi |= PKG_POWER_LIMIT_CLAMP; limit.hi |= PKG_POWER_LIMIT_EN;
+ /* Set PL1 MMIO override value */ + if (conf->mmio_pl1_override) { + tdp_pl1 = (conf->mmio_pl1_override * power_unit); + limit.lo &= (~(PKG_POWER_LIMIT_MASK)); + limit.lo |= tdp_pl1 & PKG_POWER_LIMIT_MASK; + } + /* Power limit 2 time is only programmable on server SKU */ wrmsr(MSR_PKG_POWER_LIMIT, limit);
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35533 )
Change subject: mb/google/hatch: Support separate fixed PL1 setting for tablet mode ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35533/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35533/1//COMMIT_MSG@9 PS1, Line 9: thier their
https://review.coreboot.org/c/coreboot/+/35533/1/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/35533/1/src/ec/google/chromeec/acpi... PS1, Line 381: EC_ENABLE_MAINBOERD_TABLET_MODE_EVENT EC_ENABLE_MAINBOARD_TABLET_MODE_EVENT?
https://review.coreboot.org/c/coreboot/+/35533/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/include/variant/ec.h:
https://review.coreboot.org/c/coreboot/+/35533/1/src/mainboard/google/hatch/... PS1, Line 22: EC_ENABLE_MAINBOERD_TABLET_MODE_EVENT EC_ENABLE_MAINBOARD_TABLET_MODE_EVENT?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35533 )
Change subject: mb/google/hatch: Support separate fixed PL1 setting for tablet mode ......................................................................
Patch Set 1:
This change is touching multiple different components. Each of those components should be split into CL of its own: 1. EC 2. SoC 3. Mainboard
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35533 )
Change subject: mb/google/hatch: Support separate fixed PL1 setting for tablet mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35533/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35533/1//COMMIT_MSG@10 PS1, Line 10: but DPTF passive policy 1.0 cannot support it. Is it okay to just change the PL1 setting in ASL without DPTF really knowing about it? What is the impact on DPTF?
Do you have a bug# where this was discussed?
SH Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35533 )
Change subject: mb/google/hatch: Support separate fixed PL1 setting for tablet mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35533/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35533/1//COMMIT_MSG@10 PS1, Line 10: but DPTF passive policy 1.0 cannot support it.
Is it okay to just change the PL1 setting in ASL without DPTF really knowing about it? What is the i […]
We don't have bug for this implementation. DPTF uses MSR PL1 only and system takes the lowest value of MSR PL1 and MMIO PL1, so there will be 3 case in tablet mode. @Sumeet, Kane, can you confirm about this implementation?
Case1: mmio_pl1 > DPTF PL1 max - No impact on DPTF operation (MSR PL1 activated)
Case2: DPTF PL1 max > mmio_pl1 > DPTF PL1 min - In lower temperature, DPTF wants to set higher PL1 value than mmio_pl1 => PL1 is limited to mmio_pl1 (MMIO PL1 activated) - In higher temperature, DPTF wants to set lower PL1 value than mmio_pl1 => PL1 is set by DPTF (MSR PL1 activated)
Case3: mmio_pl1 < DPTF PL1 min - DPTF always wants to set higher PL1 value than mmio_pl1 => PL1 is limited to mmio_pl1, so system works like it has fixed PL1 value
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35533 )
Change subject: mb/google/hatch: Support separate fixed PL1 setting for tablet mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35533/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35533/1//COMMIT_MSG@10 PS1, Line 10: but DPTF passive policy 1.0 cannot support it.
We don't have bug for this implementation. […]
Could we go back to 138395625 discuss this? I'm not sure what will happen if you directly touch pl1 without notifying DPTF
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35533 )
Change subject: mb/google/hatch: Support separate fixed PL1 setting for tablet mode ......................................................................
Patch Set 1: Code-Review-1
let's discuss in the issue tracker and decide what to do next. thanks.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35533 )
Change subject: mb/google/hatch: Support separate fixed PL1 setting for tablet mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35533/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/35533/1/src/mainboard/google/hatch/... PS1, Line 70: , do we really need this ? or am I missing something here ?
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35533 )
Change subject: mb/google/hatch: Support separate fixed PL1 setting for tablet mode ......................................................................
Patch Set 1:
can we please move code changes for mainboard, soc and ec in different CL/patchset ?
shkim has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35533 )
Change subject: mb/google/hatch: Support separate fixed PL1 setting for tablet mode ......................................................................
Abandoned
-
Furquan Shaikh (Code Review) -
Kane Chen (Code Review)
-
SH Kim (Code Review)
-
Shelley Chen (Code Review)
-
shkim (Code Review)
-
Sumeet R Pawnikar (Code Review)