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

[Martin Schiller <ms@dev.tdt.de>]

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.