Michael Büchler has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45184 )
Change subject: cpu/intel/model_1067x: enable PECI ......................................................................
cpu/intel/model_1067x: enable PECI
This is required for Super I/Os to be able to read the CPU temperature through PECI.
On 45nm Core 2 CPUs (Wolfdale, Yorkfield) it is not enabled by default. This is probably related to erratum AW67 "Enabling PECI via the PECI_CTL MSR incorrectly writes CPUID_FEATURE_MASK1 MSR". The suggested workaround is "Do not initialize PECI before processor update is loaded". Since coreboot performs microcode updates before running this code it should not cause any trouble. It was tested on a Core 2 Duo E8400, stepping E0.
PECI is already enabled by default on older (65nm) CPUs. Tested: Pentium Dual-Core E2160.
See commit edac28ce65e48d6b2a0a2421d046a4fe4b2bf589 for the same change on cpu/intel/model_6fx.
Signed-off-by: Michael Büchler michael.buechler@posteo.net Change-Id: I5a3ec033bd816665af4ecc82f7b167857cd7c1b6 --- M src/cpu/intel/model_1067x/model_1067x_init.c 1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/45184/1
diff --git a/src/cpu/intel/model_1067x/model_1067x_init.c b/src/cpu/intel/model_1067x/model_1067x_init.c index 6553f29..c59e1fc 100644 --- a/src/cpu/intel/model_1067x/model_1067x_init.c +++ b/src/cpu/intel/model_1067x/model_1067x_init.c @@ -166,6 +166,8 @@ wrmsr(MSR_EMTTM_CR_TABLE(5), msr); }
+#define IA32_PECI_CTL 0x5a0 + static void configure_misc(const int eist, const int tm2, const int emttm) { msr_t msr; @@ -208,6 +210,11 @@ msr.lo |= (1 << 20); /* Lock Enhanced SpeedStep Enable */ wrmsr(IA32_MISC_ENABLE, msr); } + + /* Enable PECI */ + msr = rdmsr(IA32_PECI_CTL); + msr.lo |= 1; + wrmsr(IA32_PECI_CTL, msr); }
#define PIC_SENS_CFG 0x1aa
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45184 )
Change subject: cpu/intel/model_1067x: enable PECI ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45184/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45184/1//COMMIT_MSG@13 PS1, Line 13: erratum AW67 "Enabling PECI via the PECI_CTL : MSR incorrectly writes CPUID_FEATURE_MASK1 MSR". The suggested : workaround is "Do not initialize PECI before processor update is : loaded". Since coreboot performs microcode updates before running this : code it should not cause any trouble. Maybe add this as a comment in the code to make sure that (although unlikely) future development keeps this after the microcode update?
Hello build bot (Jenkins), Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45184
to look at the new patch set (#2).
Change subject: cpu/intel/model_1067x: enable PECI ......................................................................
cpu/intel/model_1067x: enable PECI
This is required for Super I/Os to be able to read the CPU temperature through PECI.
On 45nm Core 2 CPUs (Wolfdale, Yorkfield) it is not enabled by default. This is probably related to erratum AW67 "Enabling PECI via the PECI_CTL MSR incorrectly writes CPUID_FEATURE_MASK1 MSR". The suggested workaround is "Do not initialize PECI before processor update is loaded". Since coreboot performs microcode updates before running this code it should not cause any trouble. It was tested on a Core 2 Duo E8400, stepping E0.
PECI is already enabled by default on older (65nm) CPUs. Tested: Pentium Dual-Core E2160.
See commit edac28ce65e48d6b2a0a2421d046a4fe4b2bf589 for the same change on cpu/intel/model_6fx.
Signed-off-by: Michael Büchler michael.buechler@posteo.net Change-Id: I5a3ec033bd816665af4ecc82f7b167857cd7c1b6 --- M src/cpu/intel/model_1067x/model_1067x_init.c 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/45184/2
Michael Büchler has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45184 )
Change subject: cpu/intel/model_1067x: enable PECI ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45184/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45184/1//COMMIT_MSG@13 PS1, Line 13: erratum AW67 "Enabling PECI via the PECI_CTL : MSR incorrectly writes CPUID_FEATURE_MASK1 MSR". The suggested : workaround is "Do not initialize PECI before processor update is : loaded". Since coreboot performs microcode updates before running this : code it should not cause any trouble.
Maybe add this as a comment in the code to make sure that (although unlikely) future development kee […]
Sounds like a good idea. I rephrased it a bit - what do you think?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45184 )
Change subject: cpu/intel/model_1067x: enable PECI ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45184/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45184/1//COMMIT_MSG@13 PS1, Line 13: erratum AW67 "Enabling PECI via the PECI_CTL : MSR incorrectly writes CPUID_FEATURE_MASK1 MSR". The suggested : workaround is "Do not initialize PECI before processor update is : loaded". Since coreboot performs microcode updates before running this : code it should not cause any trouble.
Sounds like a good idea. […]
Done
Arthur Heymans has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45184 )
Change subject: cpu/intel/model_1067x: enable PECI ......................................................................
cpu/intel/model_1067x: enable PECI
This is required for Super I/Os to be able to read the CPU temperature through PECI.
On 45nm Core 2 CPUs (Wolfdale, Yorkfield) it is not enabled by default. This is probably related to erratum AW67 "Enabling PECI via the PECI_CTL MSR incorrectly writes CPUID_FEATURE_MASK1 MSR". The suggested workaround is "Do not initialize PECI before processor update is loaded". Since coreboot performs microcode updates before running this code it should not cause any trouble. It was tested on a Core 2 Duo E8400, stepping E0.
PECI is already enabled by default on older (65nm) CPUs. Tested: Pentium Dual-Core E2160.
See commit edac28ce65e48d6b2a0a2421d046a4fe4b2bf589 for the same change on cpu/intel/model_6fx.
Signed-off-by: Michael Büchler michael.buechler@posteo.net Change-Id: I5a3ec033bd816665af4ecc82f7b167857cd7c1b6 Reviewed-on: https://review.coreboot.org/c/coreboot/+/45184 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/model_1067x/model_1067x_init.c 1 file changed, 9 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/cpu/intel/model_1067x/model_1067x_init.c b/src/cpu/intel/model_1067x/model_1067x_init.c index 6553f29..cd722f5 100644 --- a/src/cpu/intel/model_1067x/model_1067x_init.c +++ b/src/cpu/intel/model_1067x/model_1067x_init.c @@ -166,6 +166,8 @@ wrmsr(MSR_EMTTM_CR_TABLE(5), msr); }
+#define IA32_PECI_CTL 0x5a0 + static void configure_misc(const int eist, const int tm2, const int emttm) { msr_t msr; @@ -208,6 +210,13 @@ msr.lo |= (1 << 20); /* Lock Enhanced SpeedStep Enable */ wrmsr(IA32_MISC_ENABLE, msr); } + + /* Enable PECI + WARNING: due to Erratum AW67 described in Intel document #318733 + the microcode must be updated before this MSR is written to. */ + msr = rdmsr(IA32_PECI_CTL); + msr.lo |= 1; + wrmsr(IA32_PECI_CTL, msr); }
#define PIC_SENS_CFG 0x1aa
-
Arthur Heymans (Code Review) -
Michael Büchler (Code Review)