Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46916 )
Change subject: cpu/intel/haswell: Enable timed MWAIT if supported ......................................................................
cpu/intel/haswell: Enable timed MWAIT if supported
Broadwell unconditionally enables it, but not all Haswell steppings support it. Thus, check the capability bit before attempting to set it.
Change-Id: I1d11d62f1801d65ae4d5623994fd55fd35e8f34a Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/cpu/intel/haswell/haswell.h M src/cpu/intel/haswell/haswell_init.c 2 files changed, 8 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/46916/1
diff --git a/src/cpu/intel/haswell/haswell.h b/src/cpu/intel/haswell/haswell.h index b23fbac..2352489 100644 --- a/src/cpu/intel/haswell/haswell.h +++ b/src/cpu/intel/haswell/haswell.h @@ -36,6 +36,7 @@ #define MSR_CORE_THREAD_COUNT 0x35 #define MSR_PLATFORM_INFO 0xce #define PLATFORM_INFO_SET_TDP (1 << 29) +#define TIMED_MWAIT_SUPPORTED (1 << (37 - 32)) #define MSR_PKG_CST_CONFIG_CONTROL 0xe2 #define MSR_PMG_IO_CAPTURE_BASE 0xe4 #define MSR_FEATURE_CONFIG 0x13c diff --git a/src/cpu/intel/haswell/haswell_init.c b/src/cpu/intel/haswell/haswell_init.c index 438a317..21f3f4a 100644 --- a/src/cpu/intel/haswell/haswell_init.c +++ b/src/cpu/intel/haswell/haswell_init.c @@ -433,7 +433,9 @@
static void configure_c_states(void) { - msr_t msr; + msr_t msr = rdmsr(MSR_PLATFORM_INFO); + + const bool timed_mwait_capable = !!(msr.hi & TIMED_MWAIT_SUPPORTED);
msr = rdmsr(MSR_PKG_CST_CONFIG_CONTROL); msr.lo |= (1 << 30); // Package c-state Undemotion Enable @@ -443,6 +445,10 @@ msr.lo |= (1 << 26); // C1 Auto Demotion Enable msr.lo |= (1 << 25); // C3 Auto Demotion Enable msr.lo &= ~(1 << 10); // Disable IO MWAIT redirection + + if (timed_mwait_capable) + msr.lo |= (1 << 31); // Timed MWAIT Enable + /* The deepest package c-state defaults to factory-configured value. */ wrmsr(MSR_PKG_CST_CONFIG_CONTROL, msr);
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46916
to look at the new patch set (#6).
Change subject: cpu/intel/haswell: Enable timed MWAIT if supported ......................................................................
cpu/intel/haswell: Enable timed MWAIT if supported
Broadwell unconditionally enables it, but not all Haswell steppings support it. Thus, check the capability bit before attempting to set it.
Change-Id: I1d11d62f1801d65ae4d5623994fd55fd35e8f34a Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/cpu/intel/haswell/haswell.h M src/cpu/intel/haswell/haswell_init.c 2 files changed, 8 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/46916/6
Attention is currently required from: Angel Pons. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46916 )
Change subject: cpu/intel/haswell: Enable timed MWAIT if supported ......................................................................
Patch Set 16: Code-Review+2
Attention is currently required from: Angel Pons. Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46916 )
Change subject: cpu/intel/haswell: Enable timed MWAIT if supported ......................................................................
Patch Set 16:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/46916/comment/c4e9a8d7_b2ec3eb9 PS16, Line 9: unconditionally enables it I don't see any unconditional enablement that you remove, just an addition to enable it if it's supported
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46916 )
Change subject: cpu/intel/haswell: Enable timed MWAIT if supported ......................................................................
Patch Set 16:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/46916/comment/0b7f5e5d_9e9742f5 PS16, Line 9: unconditionally enables it
I don't see any unconditional enablement that you remove, just an addition to enable it if it's supp […]
The specific line that I refer to is: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/broadwell/c...
I guess the commit message may not make much sense without considering the context (backporting Broadwell things to Haswell).
Hello build bot (Jenkins), Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46916
to look at the new patch set (#17).
Change subject: cpu/intel/haswell: Enable timed MWAIT if supported ......................................................................
cpu/intel/haswell: Enable timed MWAIT if supported
Broadwell code unconditionally enables timed MWAIT, but not all Haswell steppings support it. In preparation for merging Haswell and Broadwell, also enable timed MWAIT on Haswell code, but only if it is supported.
Change-Id: I1d11d62f1801d65ae4d5623994fd55fd35e8f34a Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/cpu/intel/haswell/haswell.h M src/cpu/intel/haswell/haswell_init.c 2 files changed, 8 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/46916/17
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46916 )
Change subject: cpu/intel/haswell: Enable timed MWAIT if supported ......................................................................
Patch Set 17:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/46916/comment/a6f16b9e_c1d82c00 PS16, Line 9: unconditionally enables it
The specific line that I refer to is: https://review.coreboot.org/cgit/coreboot. […]
Reworded the commit message
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46916 )
Change subject: cpu/intel/haswell: Enable timed MWAIT if supported ......................................................................
cpu/intel/haswell: Enable timed MWAIT if supported
Broadwell code unconditionally enables timed MWAIT, but not all Haswell steppings support it. In preparation for merging Haswell and Broadwell, also enable timed MWAIT on Haswell code, but only if it is supported.
Change-Id: I1d11d62f1801d65ae4d5623994fd55fd35e8f34a Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46916 Reviewed-by: Arthur Heymans arthur@aheymans.xyz Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/cpu/intel/haswell/haswell.h M src/cpu/intel/haswell/haswell_init.c 2 files changed, 8 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/cpu/intel/haswell/haswell.h b/src/cpu/intel/haswell/haswell.h index 87c4e0a..cb602ce 100644 --- a/src/cpu/intel/haswell/haswell.h +++ b/src/cpu/intel/haswell/haswell.h @@ -36,6 +36,7 @@ #define MSR_CORE_THREAD_COUNT 0x35 #define MSR_PLATFORM_INFO 0xce #define PLATFORM_INFO_SET_TDP (1 << 29) +#define TIMED_MWAIT_SUPPORTED (1 << (37 - 32)) #define MSR_PKG_CST_CONFIG_CONTROL 0xe2 #define MSR_PMG_IO_CAPTURE_BASE 0xe4 #define MSR_FEATURE_CONFIG 0x13c diff --git a/src/cpu/intel/haswell/haswell_init.c b/src/cpu/intel/haswell/haswell_init.c index 0e48876..9881bb8 100644 --- a/src/cpu/intel/haswell/haswell_init.c +++ b/src/cpu/intel/haswell/haswell_init.c @@ -431,7 +431,9 @@
static void configure_c_states(void) { - msr_t msr; + msr_t msr = rdmsr(MSR_PLATFORM_INFO); + + const bool timed_mwait_capable = !!(msr.hi & TIMED_MWAIT_SUPPORTED);
msr = rdmsr(MSR_PKG_CST_CONFIG_CONTROL); msr.lo |= (1 << 30); // Package c-state Undemotion Enable @@ -441,6 +443,10 @@ msr.lo |= (1 << 26); // C1 Auto Demotion Enable msr.lo |= (1 << 25); // C3 Auto Demotion Enable msr.lo &= ~(1 << 10); // Disable IO MWAIT redirection + + if (timed_mwait_capable) + msr.lo |= (1 << 31); // Timed MWAIT Enable + /* The deepest package c-state defaults to factory-configured value. */ wrmsr(MSR_PKG_CST_CONFIG_CONTROL, msr);
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Patrick Georgi (Code Review)