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

From: Borislav Petkov
Date: Tue May 17 2011 - 13:36:17 EST

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...?

Hmm.

Some nitpicks below.

> Signed-off-by: Matthew Garrett <mjg@xxxxxxxxxx>
> ---
> arch/x86/include/asm/cpufeature.h | 1 +
> arch/x86/include/asm/msr-index.h | 2 +
> arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c | 43 ++++++++++++++++++++++++----
> arch/x86/kernel/cpu/scattered.c | 1 +
> 4 files changed, 41 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> index 91f3e087..fee7089 100644
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -174,6 +174,7 @@
> #define X86_FEATURE_PLN (7*32+ 5) /* Intel Power Limit Notification */
> #define X86_FEATURE_PTS (7*32+ 6) /* Intel Package Thermal Status */
> #define X86_FEATURE_DTS (7*32+ 7) /* Digital Thermal Sensor */
> +#define X86_FEATURE_POWERNOW (7*32+ 8) /* AMD frequency scaling */
>
> /* Virtualization flags: Linux defined, word 8 */
> #define X86_FEATURE_TPR_SHADOW (8*32+ 0) /* Intel TPR Shadow */
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 3cce714..d6f3e1f 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -233,6 +233,8 @@
>
> #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.

>
> #define MSR_IA32_MPERF 0x000000e7
> #define MSR_IA32_APERF 0x000000e8
> diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> index a2baafb..f3dd3b1 100644
> --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> @@ -57,10 +57,12 @@ MODULE_LICENSE("GPL");
> enum {
> UNDEFINED_CAPABLE = 0,
> SYSTEM_INTEL_MSR_CAPABLE,
> + SYSTEM_AMD_MSR_CAPABLE,
> SYSTEM_IO_CAPABLE,
> };
>
> #define INTEL_MSR_RANGE (0xffff)
> +#define AMD_MSR_RANGE (0x7)
>
> struct acpi_cpufreq_data {
> struct acpi_processor_performance *acpi_data;
> @@ -85,6 +87,13 @@ static int check_est_cpu(unsigned int cpuid)
> return cpu_has(cpu, X86_FEATURE_EST);
> }
>
> +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.

> +
> static unsigned extract_io(u32 value, struct acpi_cpufreq_data *data)
> {
> struct acpi_processor_performance *perf;
> @@ -104,7 +113,11 @@ static unsigned extract_msr(u32 msr, struct acpi_cpufreq_data *data)
> int i;
> struct acpi_processor_performance *perf;
>
> - msr &= INTEL_MSR_RANGE;
> + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> + msr &= AMD_MSR_RANGE;
> + else
> + msr &= INTEL_MSR_RANGE;
> +
> perf = data->acpi_data;
>
> for (i = 0; data->freq_table[i].frequency != CPUFREQ_TABLE_END; i++) {
> @@ -118,6 +131,7 @@ static unsigned extract_freq(u32 val, struct acpi_cpufreq_data *data)
> {
> switch (data->cpu_feature) {
> case SYSTEM_INTEL_MSR_CAPABLE:
> + case SYSTEM_AMD_MSR_CAPABLE:
> return extract_msr(val, data);
> case SYSTEM_IO_CAPABLE:
> return extract_io(val, data);
> @@ -153,6 +167,7 @@ static void do_drv_read(void *_cmd)
>
> switch (cmd->type) {
> case SYSTEM_INTEL_MSR_CAPABLE:
> + case SYSTEM_AMD_MSR_CAPABLE:
> rdmsr(cmd->addr.msr.reg, cmd->val, h);
> break;
> case SYSTEM_IO_CAPABLE:
> @@ -177,6 +192,9 @@ static void do_drv_write(void *_cmd)
> lo = (lo & ~INTEL_MSR_RANGE) | (cmd->val & INTEL_MSR_RANGE);
> wrmsr(cmd->addr.msr.reg, lo, hi);
> break;
> + case SYSTEM_AMD_MSR_CAPABLE:
> + wrmsr(cmd->addr.msr.reg, cmd->val, 0);
> + break;
> case SYSTEM_IO_CAPABLE:
> acpi_os_write_port((acpi_io_address)cmd->addr.io.port,
> cmd->val,
> @@ -220,6 +238,10 @@ static u32 get_cur_val(const struct cpumask *mask)
> cmd.type = SYSTEM_INTEL_MSR_CAPABLE;
> cmd.addr.msr.reg = MSR_IA32_PERF_STATUS;
> break;
> + case SYSTEM_AMD_MSR_CAPABLE:
> + cmd.type = SYSTEM_AMD_MSR_CAPABLE;
> + cmd.addr.msr.reg = MSR_AMD_PERF_STATUS;
> + break;
> case SYSTEM_IO_CAPABLE:
> cmd.type = SYSTEM_IO_CAPABLE;
> perf = per_cpu(acfreq_data, cpumask_first(mask))->acpi_data;
> @@ -329,6 +351,11 @@ static int acpi_cpufreq_target(struct cpufreq_policy *policy,
> cmd.addr.msr.reg = MSR_IA32_PERF_CTL;
> cmd.val = (u32) perf->states[next_perf_state].control;
> break;
> + case SYSTEM_AMD_MSR_CAPABLE:
> + cmd.type = SYSTEM_AMD_MSR_CAPABLE;
> + cmd.addr.msr.reg = MSR_AMD_PERF_CTL;
> + cmd.val = (u32) perf->states[next_perf_state].control;
> + break;
> case SYSTEM_IO_CAPABLE:
> cmd.type = SYSTEM_IO_CAPABLE;
> cmd.addr.io.port = perf->control_register.address;
> @@ -583,12 +610,16 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
> break;
> case ACPI_ADR_SPACE_FIXED_HARDWARE:
> dprintk("HARDWARE addr space\n");
> - if (!check_est_cpu(cpu)) {
> - result = -ENODEV;
> - goto err_unreg;
> + if (check_est_cpu(cpu)) {
> + data->cpu_feature = SYSTEM_INTEL_MSR_CAPABLE;
> + break;
> }
> - data->cpu_feature = SYSTEM_INTEL_MSR_CAPABLE;
> - break;
> + if (check_powernow_cpu(cpu)) {
> + data->cpu_feature = SYSTEM_AMD_MSR_CAPABLE;
> + break;
> + }
> + result = -ENODEV;
> + goto err_unreg;
> default:
> dprintk("Unknown addr space %d\n",
> (u32) (perf->control_register.space_id));
> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
> index c7f64e6..d5086af 100644
> --- 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.

Thanks.

--
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/