Re: [PATCH 2/2] x86/cpu/intel: Add EIST workaround for Lightning Mountain.

[Martin Schiller]

On 2026-03-06 17:54, Dave Hansen wrote:
> So what's weird about these systems? Do they not have a "normal" BIOS
> based on the Intel reference one?

That's right, the Lightning Mountain SoC is an x86 (Atom) system, but
it doesn't have a classic BIOS.


> I don't know much about this specific feature, but this patch is doing
> some unusual things. I'll elaborate below:
>
> > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > index 
> > 98ae4c37c93eccf775d5632acf122603a19918a8..e49df04e8d491158cc48f8d8bef824c434256d09 
> > 100644
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -466,6 +466,29 @@ static void intel_workarounds(struct cpuinfo_x86 
> > *c)
> >  #else
> >  static void intel_workarounds(struct cpuinfo_x86 *c)
> >  {
> > +	u64 misc_enable;
> > +
> > +	/*
> > +	 * Intel / MaxLinear Lightning Mountain workaround to enable 
> > Enhanced
> > +	 * Intel SpeedStep Technology (EIST) for each cpu. Otherwise, the
> > +	 * frequency on some cpus is locked to the minimum value of 624 MHz.
> > +	 * This usually would be the job of the BIOS / bootloader, but 
> > U-Boot
> > +	 * only enables it on the cpu on which it is running.
> > +	 */
> > +	if (c->x86_vfm == INTEL_ATOM_AIRMONT_NP) {
>
> Model checks area kinda a last resort. A quick search in the SDM found:
>
> CPUID.01H:ECX[7]: If 1, supports Enhanced Intel SpeedStep® technology.
>
> But there's other chit chat in the "Runtime Mutable CPUID Fields"
> section that makes it seem that it's not a really feature enumeration
> bit, but a flag to tell if the BIOS enabled it:
>
> 	CPUID.01H:ECX[7] -- This feature flag reflects the setting in
> 			    IA32_MISC_ENABLE[16]
>
> But the plot thickens because the *existing* code does this:
>
> static int centrino_cpu_init(struct cpufreq_policy *policy)
> {
> ...
>         /* Only Intel makes Enhanced Speedstep-capable CPUs */
>         if (cpu->x86_vendor != X86_VENDOR_INTEL ||
>             !cpu_has(cpu, X86_FEATURE_EST))
>                 return -ENODEV;
> ...
>         if (!(l & MSR_IA32_MISC_ENABLE_ENHANCED_SPEEDSTEP)) {
>
> Which, again, makes it seem like X86_FEATURE_EST (aka. 
> CPUID.01H:ECX[7])
> tells you if the MSR bit is supported, not whether it is enabled.
>
> I'd tend to trust the existing kernel code over quibbling with the SDM
> wording in general. It's also possible the old code was just confused 
> or
> something was buggy.
>
> > +		rdmsrq(MSR_IA32_MISC_ENABLE, misc_enable);
> > +		if (!(misc_enable & MSR_IA32_MISC_ENABLE_ENHANCED_SPEEDSTEP)) {
> > +			misc_enable |= MSR_IA32_MISC_ENABLE_ENHANCED_SPEEDSTEP;
> > +			wrmsrq(MSR_IA32_MISC_ENABLE, misc_enable);
> > +
> > +			/* check to see if it was enabled successfully */
> > +			rdmsrq(MSR_IA32_MISC_ENABLE, misc_enable);
> > +			if (!(misc_enable & MSR_IA32_MISC_ENABLE_ENHANCED_SPEEDSTEP)) {
> > +				pr_info("CPU%d: Can't enable Enhanced SpeedStep\n",
> > +					c->cpu_index);
> > +			}
> > +		}
> > +	}
> >  }
>
>
> This is also not written in the normal kernel style which minimizes
> indentation. For instance, the function should have opened with:
>
> 	if (c->x86_vfm != INTEL_ATOM_AIRMONT_NP)
> 		return;
>
> It also needs to be reconciled with centrino_cpu_init() (at least).
> Having *a* single place to go in and say "If this CPU supports 'EST',
> turn it on" would be a minimal refactoring that could be shared by your
> new workaround and the old centrino code.
>
> centrino_cpu_init() does look gated on X86_FEATURE_EST already, though
> because of the centrino_ids[]. So, you still need to figure out the
> interaction with X86_FEATURE_EST for when you call the workaround.

As you already noted, EIST is also enabled in the Intel Enhanced
SpeedStep (deprecated) "speedstep-centrino.c" driver. It is also
enabled in the VIA C7 Enhanced PowerSaver driver "e_powersaver.c".

The question now is whether and where this is best handled centrally.