Attention is currently required from: Nico Huber. Hello Nico Huber,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/56662
to review the following change.
Change subject: Revert "src/soc/intel/cannonlake: Update C-state latency control limits" ......................................................................
Revert "src/soc/intel/cannonlake: Update C-state latency control limits"
This reverts commit 66dbb0c5d67279722fcbcb547d9c6b61e606d50e.
This was far from Intel's recommendations. It seems very unclear how it can fix C10. Maybe this was some hack to force C10 even if the control logic says that it's not worth the latency?
Change-Id: Iea56c6a29ca4b34c9852393fed2e3be4de128ec6 Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/soc/intel/cannonlake/cpu.c M src/soc/intel/cannonlake/include/soc/cpu.h 2 files changed, 17 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/56662/1
diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c index 69ab1d5..be6f13c 100644 --- a/src/soc/intel/cannonlake/cpu.c +++ b/src/soc/intel/cannonlake/cpu.c @@ -65,31 +65,36 @@ wrmsr(MSR_PKG_CST_CONFIG_CONTROL, msr); }
+ /* C-state Interrupt Response Latency Control 0 - package C3 latency */ + msr.hi = 0; + msr.lo = IRTL_VALID | IRTL_1024_NS | C_STATE_LATENCY_CONTROL_0_LIMIT; + wrmsr(MSR_C_STATE_LATENCY_CONTROL_0, msr); + /* C-state Interrupt Response Latency Control 1 - package C6/C7 short */ msr.hi = 0; - msr.lo = IRTL_VALID | IRTL_32768_NS | C_STATE_LATENCY_CONTROL_1_LIMIT; + msr.lo = IRTL_VALID | IRTL_1024_NS | C_STATE_LATENCY_CONTROL_1_LIMIT; wrmsr(MSR_C_STATE_LATENCY_CONTROL_1, msr);
/* C-state Interrupt Response Latency Control 2 - package C6/C7 long */ msr.hi = 0; - msr.lo = IRTL_VALID | IRTL_32768_NS | C_STATE_LATENCY_CONTROL_2_LIMIT; + msr.lo = IRTL_VALID | IRTL_1024_NS | C_STATE_LATENCY_CONTROL_2_LIMIT; wrmsr(MSR_C_STATE_LATENCY_CONTROL_2, msr);
/* C-state Interrupt Response Latency Control 3 - package C8 */ msr.hi = 0; - msr.lo = IRTL_VALID | IRTL_32768_NS | + msr.lo = IRTL_VALID | IRTL_1024_NS | C_STATE_LATENCY_CONTROL_3_LIMIT; wrmsr(MSR_C_STATE_LATENCY_CONTROL_3, msr);
/* C-state Interrupt Response Latency Control 4 - package C9 */ msr.hi = 0; - msr.lo = IRTL_VALID | IRTL_32768_NS | + msr.lo = IRTL_VALID | IRTL_1024_NS | C_STATE_LATENCY_CONTROL_4_LIMIT; wrmsr(MSR_C_STATE_LATENCY_CONTROL_4, msr);
/* C-state Interrupt Response Latency Control 5 - package C10 */ msr.hi = 0; - msr.lo = IRTL_VALID | IRTL_32768_NS | + msr.lo = IRTL_VALID | IRTL_1024_NS | C_STATE_LATENCY_CONTROL_5_LIMIT; wrmsr(MSR_C_STATE_LATENCY_CONTROL_5, msr); } diff --git a/src/soc/intel/cannonlake/include/soc/cpu.h b/src/soc/intel/cannonlake/include/soc/cpu.h index 3542a2b..3456847 100644 --- a/src/soc/intel/cannonlake/include/soc/cpu.h +++ b/src/soc/intel/cannonlake/include/soc/cpu.h @@ -6,13 +6,13 @@ #include <device/device.h> #include <intelblocks/msr.h>
-/* Latency times in units of 32768ns */ -#define C_STATE_LATENCY_CONTROL_0_LIMIT 0x9d -#define C_STATE_LATENCY_CONTROL_1_LIMIT 0x9d -#define C_STATE_LATENCY_CONTROL_2_LIMIT 0x9d -#define C_STATE_LATENCY_CONTROL_3_LIMIT 0x9d -#define C_STATE_LATENCY_CONTROL_4_LIMIT 0x9d -#define C_STATE_LATENCY_CONTROL_5_LIMIT 0x9d +/* Latency times in units of 1024ns. */ +#define C_STATE_LATENCY_CONTROL_0_LIMIT 0x4e +#define C_STATE_LATENCY_CONTROL_1_LIMIT 0x76 +#define C_STATE_LATENCY_CONTROL_2_LIMIT 0x94 +#define C_STATE_LATENCY_CONTROL_3_LIMIT 0xfa +#define C_STATE_LATENCY_CONTROL_4_LIMIT 0x14c +#define C_STATE_LATENCY_CONTROL_5_LIMIT 0x3f2
/* Power in units of mW */ #define C1_POWER 0x3e8