Re: [PATCH 1/5] acpi-cpufreq: Add support for modern AMD CPUs

[Matthew Garrett <mjg@redhat.com>]

On Tue, May 17, 2011 at 07:35:36PM +0200, Borislav Petkov wrote:
> On Tue, May 17, 2011 at 01:03:35PM -0400, Matthew Garrett wrote:
> > The programming model for P-states on modern AMD CPUs is very similar to
> > that of Intel and VIA. It makes sense to consolidate this support into one
> > driver rather than duplicating functionality between two of them. This
> > patch adds support for AMDs with hardware P-state control to acpi-cpufreq.
> 
> Ok, I'm a bit confused here but maybe because I don't know the whole
> cpufreq subsystem that well. Is the purpose here to add hw pstates
> support to acpi-cpufreq so that it is used on AMD but leave the old
> Fid/Vid method to powernow-k8, thus phasing it out...?

Yes. The last patch in the set removes the hw pstate code from 
powernow-k8.

> >  #define MSR_IA32_PERF_STATUS		0x00000198
> >  #define MSR_IA32_PERF_CTL		0x00000199
> > +#define MSR_AMD_PERF_STATUS		0xc0010063
> > +#define MSR_AMD_PERF_CTL		0xc0010062
> 
> Yeah, there are defines for those in
> <arch/x86/kernel/cpu/cpufreq/powernow-k8.h>:
> 
> #define MSR_PSTATE_STATUS       0xc0010063 /* Pstate Status MSR */
> #define MSR_PSTATE_CTRL         0xc0010062 /* Pstate control MSR */
> 
> can you remove them from there for consistency so that we can use only
> the msr-index.h definitions.

That happens in the final patch.

> > +static int check_powernow_cpu(unsigned int cpuid)
> > +{
> > +	struct cpuinfo_x86 *cpu = &cpu_data(cpuid);
> > +
> > +	return cpu_has(cpu, X86_FEATURE_POWERNOW);
> > +}
> 
> This could be static_cpu_has() since all the CPUs, including the boot
> CPU, will have the HwPstate thing set. Thus, you can ignore the "cpuid"
> parameter.

Ok, this was just for symmetry with the est version.

> > --- a/arch/x86/kernel/cpu/scattered.c
> > +++ b/arch/x86/kernel/cpu/scattered.c
> > @@ -39,6 +39,7 @@ void __cpuinit init_scattered_cpuid_features(struct cpuinfo_x86 *c)
> >  		{ X86_FEATURE_APERFMPERF,	CR_ECX, 0, 0x00000006, 0 },
> >  		{ X86_FEATURE_EPB,		CR_ECX, 3, 0x00000006, 0 },
> >  		{ X86_FEATURE_XSAVEOPT,		CR_EAX,	0, 0x0000000d, 1 },
> > +		{ X86_FEATURE_POWERNOW,		CR_EDX, 7, 0x80000007, 0 },
> >  		{ X86_FEATURE_CPB,		CR_EDX, 9, 0x80000007, 0 },
> >  		{ X86_FEATURE_NPT,		CR_EDX, 0, 0x8000000a, 0 },
> >  		{ X86_FEATURE_LBRV,		CR_EDX, 1, 0x8000000a, 0 },
> 
> It might make sense to split out the cpuid changes to a different patch,
> IMHO.

I'd have no problem with that.

-- 
Matthew Garrett | mjg59@srcf.ucam.org