On Thursday, July 26, 2012, Andre Przywara wrote:
> From: Matthew Garrett <[email protected]>
>
> These chips are now supported by acpi-cpufreq, so we can delete all the
> code handling them.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> Signed-off-by: Andre Przywara <[email protected]>
Would it be very wrong/confusing to keep that support in the powernow-k8
driver for the time being, perhaps making it print a message that the ACPI
driver is recommended for those chips?
Rafael
> ---
> drivers/cpufreq/Makefile | 2 +-
> drivers/cpufreq/powernow-k8.c | 385 +++--------------------------------------
> drivers/cpufreq/powernow-k8.h | 32 ----
> 3 files changed, 26 insertions(+), 393 deletions(-)
>
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 9531fc2..b99790f 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -19,7 +19,7 @@ obj-$(CONFIG_CPU_FREQ_TABLE) += freq_table.o
> # K8 systems. ACPI is preferred to all other hardware-specific drivers.
> # speedstep-* is preferred over p4-clockmod.
>
> -obj-$(CONFIG_X86_POWERNOW_K8) += powernow-k8.o mperf.o
> +obj-$(CONFIG_X86_POWERNOW_K8) += powernow-k8.o
> obj-$(CONFIG_X86_ACPI_CPUFREQ) += acpi-cpufreq.o mperf.o
> obj-$(CONFIG_X86_PCC_CPUFREQ) += pcc-cpufreq.o
> obj-$(CONFIG_X86_POWERNOW_K6) += powernow-k6.o
> diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
> index 6e35ed2..ed1cb14 100644
> --- a/drivers/cpufreq/powernow-k8.c
> +++ b/drivers/cpufreq/powernow-k8.c
> @@ -49,22 +49,12 @@
> #define PFX "powernow-k8: "
> #define VERSION "version 2.20.00"
> #include "powernow-k8.h"
> -#include "mperf.h"
>
> /* serialize freq changes */
> static DEFINE_MUTEX(fidvid_mutex);
>
> static DEFINE_PER_CPU(struct powernow_k8_data *, powernow_data);
>
> -static int cpu_family = CPU_OPTERON;
> -
> -/* array to map SW pstate number to acpi state */
> -static u32 ps_to_as[8];
> -
> -/* core performance boost */
> -static bool cpb_capable, cpb_enabled;
> -static struct msr __percpu *msrs;
> -
> static struct cpufreq_driver cpufreq_amd64_driver;
>
> #ifndef CONFIG_SMP
> @@ -86,12 +76,6 @@ static u32 find_khz_freq_from_fid(u32 fid)
> return 1000 * find_freq_from_fid(fid);
> }
>
> -static u32 find_khz_freq_from_pstate(struct cpufreq_frequency_table *data,
> - u32 pstate)
> -{
> - return data[ps_to_as[pstate]].frequency;
> -}
> -
> /* Return the vco fid for an input fid
> *
> * Each "low" fid has corresponding "high" fid, and you can get to "low" fids
> @@ -114,9 +98,6 @@ static int pending_bit_stuck(void)
> {
> u32 lo, hi;
>
> - if (cpu_family == CPU_HW_PSTATE)
> - return 0;
> -
> rdmsr(MSR_FIDVID_STATUS, lo, hi);
> return lo & MSR_S_LO_CHANGE_PENDING ? 1 : 0;
> }
> @@ -130,20 +111,6 @@ static int query_current_values_with_pending_wait(struct powernow_k8_data *data)
> u32 lo, hi;
> u32 i = 0;
>
> - if (cpu_family == CPU_HW_PSTATE) {
> - rdmsr(MSR_PSTATE_STATUS, lo, hi);
> - i = lo & HW_PSTATE_MASK;
> - data->currpstate = i;
> -
> - /*
> - * a workaround for family 11h erratum 311 might cause
> - * an "out-of-range Pstate if the core is in Pstate-0
> - */
> - if ((boot_cpu_data.x86 == 0x11) && (i >= data->numps))
> - data->currpstate = HW_PSTATE_0;
> -
> - return 0;
> - }
> do {
> if (i++ > 10000) {
> pr_debug("detected change pending stuck\n");
> @@ -300,14 +267,6 @@ static int decrease_vid_code_by_step(struct powernow_k8_data *data,
> return 0;
> }
>
> -/* Change hardware pstate by single MSR write */
> -static int transition_pstate(struct powernow_k8_data *data, u32 pstate)
> -{
> - wrmsr(MSR_PSTATE_CTRL, pstate, 0);
> - data->currpstate = pstate;
> - return 0;
> -}
> -
> /* Change Opteron/Athlon64 fid and vid, by the 3 phases. */
> static int transition_fid_vid(struct powernow_k8_data *data,
> u32 reqfid, u32 reqvid)
> @@ -524,8 +483,6 @@ static int core_voltage_post_transition(struct powernow_k8_data *data,
> static const struct x86_cpu_id powernow_k8_ids[] = {
> /* IO based frequency switching */
> { X86_VENDOR_AMD, 0xf },
> - /* MSR based frequency switching supported */
> - X86_FEATURE_MATCH(X86_FEATURE_HW_PSTATE),
> {}
> };
> MODULE_DEVICE_TABLE(x86cpu, powernow_k8_ids);
> @@ -561,15 +518,8 @@ static void check_supported_cpu(void *_rc)
> "Power state transitions not supported\n");
> return;
> }
> - } else { /* must be a HW Pstate capable processor */
> - cpuid(CPUID_FREQ_VOLT_CAPABILITIES, &eax, &ebx, &ecx, &edx);
> - if ((edx & USE_HW_PSTATE) == USE_HW_PSTATE)
> - cpu_family = CPU_HW_PSTATE;
> - else
> - return;
> + *rc = 0;
> }
> -
> - *rc = 0;
> }
>
> static int check_pst_table(struct powernow_k8_data *data, struct pst_s *pst,
> @@ -633,18 +583,11 @@ static void print_basics(struct powernow_k8_data *data)
> for (j = 0; j < data->numps; j++) {
> if (data->powernow_table[j].frequency !=
> CPUFREQ_ENTRY_INVALID) {
> - if (cpu_family == CPU_HW_PSTATE) {
> - printk(KERN_INFO PFX
> - " %d : pstate %d (%d MHz)\n", j,
> - data->powernow_table[j].index,
> - data->powernow_table[j].frequency/1000);
> - } else {
> printk(KERN_INFO PFX
> "fid 0x%x (%d MHz), vid 0x%x\n",
> data->powernow_table[j].index & 0xff,
> data->powernow_table[j].frequency/1000,
> data->powernow_table[j].index >> 8);
> - }
> }
> }
> if (data->batps)
> @@ -652,20 +595,6 @@ static void print_basics(struct powernow_k8_data *data)
> data->batps);
> }
>
> -static u32 freq_from_fid_did(u32 fid, u32 did)
> -{
> - u32 mhz = 0;
> -
> - if (boot_cpu_data.x86 == 0x10)
> - mhz = (100 * (fid + 0x10)) >> did;
> - else if (boot_cpu_data.x86 == 0x11)
> - mhz = (100 * (fid + 8)) >> did;
> - else
> - BUG();
> -
> - return mhz * 1000;
> -}
> -
> static int fill_powernow_table(struct powernow_k8_data *data,
> struct pst_s *pst, u8 maxvid)
> {
> @@ -825,7 +754,7 @@ static void powernow_k8_acpi_pst_values(struct powernow_k8_data *data,
> {
> u64 control;
>
> - if (!data->acpi_data.state_count || (cpu_family == CPU_HW_PSTATE))
> + if (!data->acpi_data.state_count)
> return;
>
> control = data->acpi_data.states[index].control;
> @@ -876,10 +805,7 @@ static int powernow_k8_cpu_init_acpi(struct powernow_k8_data *data)
> data->numps = data->acpi_data.state_count;
> powernow_k8_acpi_pst_values(data, 0);
>
> - if (cpu_family == CPU_HW_PSTATE)
> - ret_val = fill_powernow_table_pstate(data, powernow_table);
> - else
> - ret_val = fill_powernow_table_fidvid(data, powernow_table);
> + ret_val = fill_powernow_table_fidvid(data, powernow_table);
> if (ret_val)
> goto err_out_mem;
>
> @@ -916,51 +842,6 @@ err_out:
> return ret_val;
> }
>
> -static int fill_powernow_table_pstate(struct powernow_k8_data *data,
> - struct cpufreq_frequency_table *powernow_table)
> -{
> - int i;
> - u32 hi = 0, lo = 0;
> - rdmsr(MSR_PSTATE_CUR_LIMIT, lo, hi);
> - data->max_hw_pstate = (lo & HW_PSTATE_MAX_MASK) >> HW_PSTATE_MAX_SHIFT;
> -
> - for (i = 0; i < data->acpi_data.state_count; i++) {
> - u32 index;
> -
> - index = data->acpi_data.states[i].control & HW_PSTATE_MASK;
> - if (index > data->max_hw_pstate) {
> - printk(KERN_ERR PFX "invalid pstate %d - "
> - "bad value %d.\n", i, index);
> - printk(KERN_ERR PFX "Please report to BIOS "
> - "manufacturer\n");
> - invalidate_entry(powernow_table, i);
> - continue;
> - }
> -
> - ps_to_as[index] = i;
> -
> - /* Frequency may be rounded for these */
> - if ((boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10)
> - || boot_cpu_data.x86 == 0x11) {
> -
> - rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi);
> - if (!(hi & HW_PSTATE_VALID_MASK)) {
> - pr_debug("invalid pstate %d, ignoring\n", index);
> - invalidate_entry(powernow_table, i);
> - continue;
> - }
> -
> - powernow_table[i].frequency =
> - freq_from_fid_did(lo & 0x3f, (lo >> 6) & 7);
> - } else
> - powernow_table[i].frequency =
> - data->acpi_data.states[i].core_frequency * 1000;
> -
> - powernow_table[i].index = index;
> - }
> - return 0;
> -}
> -
> static int fill_powernow_table_fidvid(struct powernow_k8_data *data,
> struct cpufreq_frequency_table *powernow_table)
> {
> @@ -1037,15 +918,8 @@ static int get_transition_latency(struct powernow_k8_data *data)
> max_latency = cur_latency;
> }
> if (max_latency == 0) {
> - /*
> - * Fam 11h and later may return 0 as transition latency. This
> - * is intended and means "very fast". While cpufreq core and
> - * governors currently can handle that gracefully, better set it
> - * to 1 to avoid problems in the future.
> - */
> - if (boot_cpu_data.x86 < 0x11)
> - printk(KERN_ERR FW_WARN PFX "Invalid zero transition "
> - "latency\n");
> + printk(KERN_ERR FW_WARN PFX "Invalid zero transition "
> + "latency\n");
> max_latency = 1;
> }
> /* value in usecs, needs to be in nanoseconds */
> @@ -1105,40 +979,6 @@ static int transition_frequency_fidvid(struct powernow_k8_data *data,
> return res;
> }
>
> -/* Take a frequency, and issue the hardware pstate transition command */
> -static int transition_frequency_pstate(struct powernow_k8_data *data,
> - unsigned int index)
> -{
> - u32 pstate = 0;
> - int res, i;
> - struct cpufreq_freqs freqs;
> -
> - pr_debug("cpu %d transition to index %u\n", smp_processor_id(), index);
> -
> - /* get MSR index for hardware pstate transition */
> - pstate = index & HW_PSTATE_MASK;
> - if (pstate > data->max_hw_pstate)
> - return -EINVAL;
> -
> - freqs.old = find_khz_freq_from_pstate(data->powernow_table,
> - data->currpstate);
> - freqs.new = find_khz_freq_from_pstate(data->powernow_table, pstate);
> -
> - for_each_cpu(i, data->available_cores) {
> - freqs.cpu = i;
> - cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> - }
> -
> - res = transition_pstate(data, pstate);
> - freqs.new = find_khz_freq_from_pstate(data->powernow_table, pstate);
> -
> - for_each_cpu(i, data->available_cores) {
> - freqs.cpu = i;
> - cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> - }
> - return res;
> -}
> -
> /* Driver entry point to switch to the target frequency */
> static int powernowk8_target(struct cpufreq_policy *pol,
> unsigned targfreq, unsigned relation)
> @@ -1180,18 +1020,16 @@ static int powernowk8_target(struct cpufreq_policy *pol,
> if (query_current_values_with_pending_wait(data))
> goto err_out;
>
> - if (cpu_family != CPU_HW_PSTATE) {
> - pr_debug("targ: curr fid 0x%x, vid 0x%x\n",
> - data->currfid, data->currvid);
> + pr_debug("targ: curr fid 0x%x, vid 0x%x\n",
> + data->currfid, data->currvid);
>
> - if ((checkvid != data->currvid) ||
> - (checkfid != data->currfid)) {
> - printk(KERN_INFO PFX
> - "error - out of sync, fix 0x%x 0x%x, "
> - "vid 0x%x 0x%x\n",
> - checkfid, data->currfid,
> - checkvid, data->currvid);
> - }
> + if ((checkvid != data->currvid) ||
> + (checkfid != data->currfid)) {
> + printk(KERN_INFO PFX
> + "error - out of sync, fix 0x%x 0x%x, "
> + "vid 0x%x 0x%x\n",
> + checkfid, data->currfid,
> + checkvid, data->currvid);
> }
>
> if (cpufreq_frequency_table_target(pol, data->powernow_table,
> @@ -1202,11 +1040,8 @@ static int powernowk8_target(struct cpufreq_policy *pol,
>
> powernow_k8_acpi_pst_values(data, newstate);
>
> - if (cpu_family == CPU_HW_PSTATE)
> - ret = transition_frequency_pstate(data,
> - data->powernow_table[newstate].index);
> - else
> - ret = transition_frequency_fidvid(data, newstate);
> + ret = transition_frequency_fidvid(data, newstate);
> +
> if (ret) {
> printk(KERN_ERR PFX "transition frequency failed\n");
> ret = 1;
> @@ -1215,11 +1050,7 @@ static int powernowk8_target(struct cpufreq_policy *pol,
> }
> mutex_unlock(&fidvid_mutex);
>
> - if (cpu_family == CPU_HW_PSTATE)
> - pol->cur = find_khz_freq_from_pstate(data->powernow_table,
> - data->powernow_table[newstate].index);
> - else
> - pol->cur = find_khz_freq_from_fid(data->currfid);
> + pol->cur = find_khz_freq_from_fid(data->currfid);
> ret = 0;
>
> err_out:
> @@ -1259,8 +1090,7 @@ static void __cpuinit powernowk8_cpu_init_on_cpu(void *_init_on_cpu)
> return;
> }
>
> - if (cpu_family == CPU_OPTERON)
> - fidvid_msr_init();
> + fidvid_msr_init();
>
> init_on_cpu->rc = 0;
> }
> @@ -1274,7 +1104,6 @@ static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol)
> struct powernow_k8_data *data;
> struct init_on_cpu init_on_cpu;
> int rc;
> - struct cpuinfo_x86 *c = &cpu_data(pol->cpu);
>
> if (!cpu_online(pol->cpu))
> return -ENODEV;
> @@ -1290,7 +1119,6 @@ static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol)
> }
>
> data->cpu = pol->cpu;
> - data->currpstate = HW_PSTATE_INVALID;
>
> if (powernow_k8_cpu_init_acpi(data)) {
> /*
> @@ -1327,17 +1155,10 @@ static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol)
> if (rc != 0)
> goto err_out_exit_acpi;
>
> - if (cpu_family == CPU_HW_PSTATE)
> - cpumask_copy(pol->cpus, cpumask_of(pol->cpu));
> - else
> - cpumask_copy(pol->cpus, cpu_core_mask(pol->cpu));
> + cpumask_copy(pol->cpus, cpu_core_mask(pol->cpu));
> data->available_cores = pol->cpus;
>
> - if (cpu_family == CPU_HW_PSTATE)
> - pol->cur = find_khz_freq_from_pstate(data->powernow_table,
> - data->currpstate);
> - else
> - pol->cur = find_khz_freq_from_fid(data->currfid);
> + pol->cur = find_khz_freq_from_fid(data->currfid);
> pr_debug("policy current frequency %d kHz\n", pol->cur);
>
> /* min/max the cpu is capable of */
> @@ -1349,18 +1170,10 @@ static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol)
> return -EINVAL;
> }
>
> - /* Check for APERF/MPERF support in hardware */
> - if (cpu_has(c, X86_FEATURE_APERFMPERF))
> - cpufreq_amd64_driver.getavg = cpufreq_get_measured_perf;
> -
> cpufreq_frequency_table_get_attr(data->powernow_table, pol->cpu);
>
> - if (cpu_family == CPU_HW_PSTATE)
> - pr_debug("cpu_init done, current pstate 0x%x\n",
> - data->currpstate);
> - else
> - pr_debug("cpu_init done, current fid 0x%x, vid 0x%x\n",
> - data->currfid, data->currvid);
> + pr_debug("cpu_init done, current fid 0x%x, vid 0x%x\n",
> + data->currfid, data->currvid);
>
> per_cpu(powernow_data, pol->cpu) = data;
>
> @@ -1413,88 +1226,15 @@ static unsigned int powernowk8_get(unsigned int cpu)
> if (err)
> goto out;
>
> - if (cpu_family == CPU_HW_PSTATE)
> - khz = find_khz_freq_from_pstate(data->powernow_table,
> - data->currpstate);
> - else
> - khz = find_khz_freq_from_fid(data->currfid);
> + khz = find_khz_freq_from_fid(data->currfid);
>
>
> out:
> return khz;
> }
>
> -static void _cpb_toggle_msrs(bool t)
> -{
> - int cpu;
> -
> - get_online_cpus();
> -
> - rdmsr_on_cpus(cpu_online_mask, MSR_K7_HWCR, msrs);
> -
> - for_each_cpu(cpu, cpu_online_mask) {
> - struct msr *reg = per_cpu_ptr(msrs, cpu);
> - if (t)
> - reg->l &= ~BIT(25);
> - else
> - reg->l |= BIT(25);
> - }
> - wrmsr_on_cpus(cpu_online_mask, MSR_K7_HWCR, msrs);
> -
> - put_online_cpus();
> -}
> -
> -/*
> - * Switch on/off core performance boosting.
> - *
> - * 0=disable
> - * 1=enable.
> - */
> -static void cpb_toggle(bool t)
> -{
> - if (!cpb_capable)
> - return;
> -
> - if (t && !cpb_enabled) {
> - cpb_enabled = true;
> - _cpb_toggle_msrs(t);
> - printk(KERN_INFO PFX "Core Boosting enabled.\n");
> - } else if (!t && cpb_enabled) {
> - cpb_enabled = false;
> - _cpb_toggle_msrs(t);
> - printk(KERN_INFO PFX "Core Boosting disabled.\n");
> - }
> -}
> -
> -static ssize_t store_cpb(struct cpufreq_policy *policy, const char *buf,
> - size_t count)
> -{
> - int ret = -EINVAL;
> - unsigned long val = 0;
> -
> - ret = strict_strtoul(buf, 10, &val);
> - if (!ret && (val == 0 || val == 1) && cpb_capable)
> - cpb_toggle(val);
> - else
> - return -EINVAL;
> -
> - return count;
> -}
> -
> -static ssize_t show_cpb(struct cpufreq_policy *policy, char *buf)
> -{
> - return sprintf(buf, "%u\n", cpb_enabled);
> -}
> -
> -#define define_one_rw(_name) \
> -static struct freq_attr _name = \
> -__ATTR(_name, 0644, show_##_name, store_##_name)
> -
> -define_one_rw(cpb);
> -
> static struct freq_attr *powernow_k8_attr[] = {
> &cpufreq_freq_attr_scaling_available_freqs,
> - &cpb,
> NULL,
> };
>
> @@ -1510,51 +1250,10 @@ static struct cpufreq_driver cpufreq_amd64_driver = {
> .attr = powernow_k8_attr,
> };
>
> -/*
> - * Clear the boost-disable flag on the CPU_DOWN path so that this cpu
> - * cannot block the remaining ones from boosting. On the CPU_UP path we
> - * simply keep the boost-disable flag in sync with the current global
> - * state.
> - */
> -static int cpb_notify(struct notifier_block *nb, unsigned long action,
> - void *hcpu)
> -{
> - unsigned cpu = (long)hcpu;
> - u32 lo, hi;
> -
> - switch (action) {
> - case CPU_UP_PREPARE:
> - case CPU_UP_PREPARE_FROZEN:
> -
> - if (!cpb_enabled) {
> - rdmsr_on_cpu(cpu, MSR_K7_HWCR, &lo, &hi);
> - lo |= BIT(25);
> - wrmsr_on_cpu(cpu, MSR_K7_HWCR, lo, hi);
> - }
> - break;
> -
> - case CPU_DOWN_PREPARE:
> - case CPU_DOWN_PREPARE_FROZEN:
> - rdmsr_on_cpu(cpu, MSR_K7_HWCR, &lo, &hi);
> - lo &= ~BIT(25);
> - wrmsr_on_cpu(cpu, MSR_K7_HWCR, lo, hi);
> - break;
> -
> - default:
> - break;
> - }
> -
> - return NOTIFY_OK;
> -}
> -
> -static struct notifier_block cpb_nb = {
> - .notifier_call = cpb_notify,
> -};
> -
> /* driver entry point for init */
> static int __cpuinit powernowk8_init(void)
> {
> - unsigned int i, supported_cpus = 0, cpu;
> + unsigned int i, supported_cpus = 0;
> int rv;
>
> if (!x86_match_cpu(powernow_k8_ids))
> @@ -1577,35 +1276,8 @@ static int __cpuinit powernowk8_init(void)
> printk(KERN_INFO PFX "Found %d %s (%d cpu cores) (" VERSION ")\n",
> num_online_nodes(), boot_cpu_data.x86_model_id, supported_cpus);
>
> - if (boot_cpu_has(X86_FEATURE_CPB)) {
> -
> - cpb_capable = true;
> -
> - msrs = msrs_alloc();
> - if (!msrs) {
> - printk(KERN_ERR "%s: Error allocating msrs!\n", __func__);
> - return -ENOMEM;
> - }
> -
> - register_cpu_notifier(&cpb_nb);
> -
> - rdmsr_on_cpus(cpu_online_mask, MSR_K7_HWCR, msrs);
> -
> - for_each_cpu(cpu, cpu_online_mask) {
> - struct msr *reg = per_cpu_ptr(msrs, cpu);
> - cpb_enabled |= !(!!(reg->l & BIT(25)));
> - }
> -
> - printk(KERN_INFO PFX "Core Performance Boosting: %s.\n",
> - (cpb_enabled ? "on" : "off"));
> - }
> -
> rv = cpufreq_register_driver(&cpufreq_amd64_driver);
> - if (rv < 0 && boot_cpu_has(X86_FEATURE_CPB)) {
> - unregister_cpu_notifier(&cpb_nb);
> - msrs_free(msrs);
> - msrs = NULL;
> - }
> +
> return rv;
> }
>
> @@ -1614,13 +1286,6 @@ static void __exit powernowk8_exit(void)
> {
> pr_debug("exit\n");
>
> - if (boot_cpu_has(X86_FEATURE_CPB)) {
> - msrs_free(msrs);
> - msrs = NULL;
> -
> - unregister_cpu_notifier(&cpb_nb);
> - }
> -
> cpufreq_unregister_driver(&cpufreq_amd64_driver);
> }
>
> diff --git a/drivers/cpufreq/powernow-k8.h b/drivers/cpufreq/powernow-k8.h
> index 3744d26..79329d4 100644
> --- a/drivers/cpufreq/powernow-k8.h
> +++ b/drivers/cpufreq/powernow-k8.h
> @@ -5,24 +5,11 @@
> * http://www.gnu.org/licenses/gpl.html
> */
>
> -enum pstate {
> - HW_PSTATE_INVALID = 0xff,
> - HW_PSTATE_0 = 0,
> - HW_PSTATE_1 = 1,
> - HW_PSTATE_2 = 2,
> - HW_PSTATE_3 = 3,
> - HW_PSTATE_4 = 4,
> - HW_PSTATE_5 = 5,
> - HW_PSTATE_6 = 6,
> - HW_PSTATE_7 = 7,
> -};
> -
> struct powernow_k8_data {
> unsigned int cpu;
>
> u32 numps; /* number of p-states */
> u32 batps; /* number of p-states supported on battery */
> - u32 max_hw_pstate; /* maximum legal hardware pstate */
>
> /* these values are constant when the PSB is used to determine
> * vid/fid pairings, but are modified during the ->target() call
> @@ -37,7 +24,6 @@ struct powernow_k8_data {
> /* keep track of the current fid / vid or pstate */
> u32 currvid;
> u32 currfid;
> - enum pstate currpstate;
>
> /* the powernow_table includes all frequency and vid/fid pairings:
> * fid are the lower 8 bits of the index, vid are the upper 8 bits.
> @@ -97,23 +83,6 @@ struct powernow_k8_data {
> #define MSR_S_HI_CURRENT_VID 0x0000003f
> #define MSR_C_HI_STP_GNT_BENIGN 0x00000001
>
> -
> -/* Hardware Pstate _PSS and MSR definitions */
> -#define USE_HW_PSTATE 0x00000080
> -#define HW_PSTATE_MASK 0x00000007
> -#define HW_PSTATE_VALID_MASK 0x80000000
> -#define HW_PSTATE_MAX_MASK 0x000000f0
> -#define HW_PSTATE_MAX_SHIFT 4
> -#define MSR_PSTATE_DEF_BASE 0xc0010064 /* base of Pstate MSRs */
> -#define MSR_PSTATE_STATUS 0xc0010063 /* Pstate Status MSR */
> -#define MSR_PSTATE_CTRL 0xc0010062 /* Pstate control MSR */
> -#define MSR_PSTATE_CUR_LIMIT 0xc0010061 /* pstate current limit MSR */
> -
> -/* define the two driver architectures */
> -#define CPU_OPTERON 0
> -#define CPU_HW_PSTATE 1
> -
> -
> /*
> * There are restrictions frequencies have to follow:
> * - only 1 entry in the low fid table ( <=1.4GHz )
> @@ -218,5 +187,4 @@ static int core_frequency_transition(struct powernow_k8_data *data, u32 reqfid);
>
> static void powernow_k8_acpi_pst_values(struct powernow_k8_data *data, unsigned int index);
>
> -static int fill_powernow_table_pstate(struct powernow_k8_data *data, struct cpufreq_frequency_table *powernow_table);
> static int fill_powernow_table_fidvid(struct powernow_k8_data *data, struct cpufreq_frequency_table *powernow_table);
>
On 08/05/2012 11:33 PM, Rafael J. Wysocki wrote:
> On Thursday, July 26, 2012, Andre Przywara wrote:
>> From: Matthew Garrett <[email protected]>
>>
>> These chips are now supported by acpi-cpufreq, so we can delete all the
>> code handling them.
>>
>> Signed-off-by: Matthew Garrett <[email protected]>
>> Signed-off-by: Andre Przywara <[email protected]>
>
> Would it be very wrong/confusing to keep that support in the powernow-k8
> driver for the time being, perhaps making it print a message that the ACPI
> driver is recommended for those chips?
Why would you like to do this? Are you concerned about regressions? Or
do you just want to avoid the introduction of the doomed "cpb" feature
in acpi-cpufreq?
I am not sure if keeping support in powernow-k8 would just make people
use it still in the future. At least if it would just load easily as before.
One idea could be to keep the code around, but only load on family 10h
if a force_fam10h or so command line option is provided. But again this
could just push distributions to provide this option to avoid the
transition.
One of my motivations was to keep only _one_ driver around, the code
removal of the fam10h support from powernow-k8 supports this.
If you insist, I can keep the code in powernow-k8, but it probably
wouldn't receive any support anymore and would increase confusion on the
user side.
Thanks for the review,
Andre.
--
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
On Monday, August 20, 2012, Andre Przywara wrote:
> On 08/05/2012 11:33 PM, Rafael J. Wysocki wrote:
> > On Thursday, July 26, 2012, Andre Przywara wrote:
> >> From: Matthew Garrett <[email protected]>
> >>
> >> These chips are now supported by acpi-cpufreq, so we can delete all the
> >> code handling them.
> >>
> >> Signed-off-by: Matthew Garrett <[email protected]>
> >> Signed-off-by: Andre Przywara <[email protected]>
> >
> > Would it be very wrong/confusing to keep that support in the powernow-k8
> > driver for the time being, perhaps making it print a message that the ACPI
> > driver is recommended for those chips?
>
> Why would you like to do this? Are you concerned about regressions?
Yes. Suppose you have a system configured to use the powernow-k8 driver
right now and you find that it stopped working due to a kernel update.
You would be quite upset I suppose.
> Or do you just want to avoid the introduction of the doomed "cpb" feature
> in acpi-cpufreq?
>
> I am not sure if keeping support in powernow-k8 would just make people
> use it still in the future. At least if it would just load easily as before.
> One idea could be to keep the code around, but only load on family 10h
> if a force_fam10h or so command line option is provided. But again this
> could just push distributions to provide this option to avoid the
> transition.
We don't force transitions like that, mind you.
> One of my motivations was to keep only _one_ driver around, the code
> removal of the fam10h support from powernow-k8 supports this.
>
> If you insist, I can keep the code in powernow-k8, but it probably
> wouldn't receive any support anymore and would increase confusion on the
> user side.
I'm not afraid of that. And as I said, you can just add info messages to
powernow-k8 saying that the feature is deprecated and will be removed in the
future and _then_ you actually _can_ remove it in the future (say, 2-3 major
kernel releasew from now).
Thanks,
Rafael
On Monday 20 August 2012 22:49:16 Rafael J. Wysocki wrote:
> On Monday, August 20, 2012, Andre Przywara wrote:
> > On 08/05/2012 11:33 PM, Rafael J. Wysocki wrote:
> > > On Thursday, July 26, 2012, Andre Przywara wrote:
...
> >
> > If you insist, I can keep the code in powernow-k8, but it probably
> > wouldn't receive any support anymore and would increase confusion on the
> > user side.
>
> I'm not afraid of that. And as I said, you can just add info messages to
> powernow-k8 saying that the feature is deprecated and will be removed in the
> future and _then_ you actually _can_ remove it in the future (say, 2-3 major
> kernel releasew from now).
Full code duplication in powernow-k8 and acpi-cpufreq does not make sense
to me.
You would need extra logic that only the first is successfully loaded etc.
IMO this has more risk of introducing new bugs than any good.
A message like that might be useful though:
if (boot_cpu_has(X86_FEATURE_HW_PSTATE))
{
printk("powernowk8 does not serve MSR based frequency switching anymore, use acpi-cpufreq instead\n");
return -1;
}
This would show people with init scripts that try to load cpufreq drivers
manually that they are not needed anymore. acpi-cpufreq should have been
loaded automatically already and cpufreq should be active.
Thomas
On 08/22/2012 03:00 AM, Thomas Renninger wrote:
> On Monday 20 August 2012 22:49:16 Rafael J. Wysocki wrote:
>> On Monday, August 20, 2012, Andre Przywara wrote:
>>> On 08/05/2012 11:33 PM, Rafael J. Wysocki wrote:
>>>> On Thursday, July 26, 2012, Andre Przywara wrote:
> ...
>>>
>>> If you insist, I can keep the code in powernow-k8, but it probably
>>> wouldn't receive any support anymore and would increase confusion on the
>>> user side.
>>
>> I'm not afraid of that. And as I said, you can just add info messages to
>> powernow-k8 saying that the feature is deprecated and will be removed in the
>> future and _then_ you actually _can_ remove it in the future (say, 2-3 major
>> kernel releasew from now).
>
> Full code duplication in powernow-k8 and acpi-cpufreq does not make sense
> to me.
> You would need extra logic that only the first is successfully loaded etc.
> IMO this has more risk of introducing new bugs than any good.
> A message like that might be useful though:
> if (boot_cpu_has(X86_FEATURE_HW_PSTATE))
> {
> printk("powernowk8 does not serve MSR based frequency switching anymore, use acpi-cpufreq instead\n");
> return -1;
> }
Matthew had something even better, that is patch 3/8:
+ if (static_cpu_has(X86_FEATURE_HW_PSTATE))
+ request_module("acpi_cpufreq");
So if someone tries to load powernow-k8 on a recent CPU, it will
automatically load acpi-cpufreq instead.
I just realized that this doesn't work as expected, because powernow-k8
bails out early due to only family 0xf being in the matching CPUID
family list. Will fix this.
I will do some tests now to check our options:
1. A combination of Matthew's and Thomas' ideas: if powernow-k8 is
loaded on newer CPUs, request acpi-cpufreq to load _plus_ write a
warning that powernow-k8 is obsolete for this hardware. Don't load
powernow-k8 (which has support removed anyway). My favorite.
2. Add code (probably to the generic cpufreq framework) to avoid loading
two drivers. Print a warning if tried anyways. Leave h/w P-state support
in powernow-k8. It seems like that acpi-cpufreq takes precedence over
powernow-k8 in distribution's module load list, so this should work as
expected even with keeping the support in powernow-k8 (as a fallback in
case of trouble).
Regards,
Andre.
> This would show people with init scripts that try to load cpufreq drivers
> manually that they are not needed anymore. acpi-cpufreq should have been
> loaded automatically already and cpufreq should be active.
>
> Thomas
>
--
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
On Wednesday, August 22, 2012 03:39:40 PM Andre Przywara wrote:
> On 08/22/2012 03:00 AM, Thomas Renninger wrote:
> > On Monday 20 August 2012 22:49:16 Rafael J. Wysocki wrote:
> >> On Monday, August 20, 2012, Andre Przywara wrote:
> >>> On 08/05/2012 11:33 PM, Rafael J. Wysocki wrote:
> >>>> On Thursday, July 26, 2012, Andre Przywara wrote:
> > ...
> >>>
> >>> If you insist, I can keep the code in powernow-k8, but it probably
> >>> wouldn't receive any support anymore and would increase confusion on the
> >>> user side.
> >>
> >> I'm not afraid of that. And as I said, you can just add info messages to
> >> powernow-k8 saying that the feature is deprecated and will be removed in the
> >> future and _then_ you actually _can_ remove it in the future (say, 2-3 major
> >> kernel releasew from now).
> >
> > Full code duplication in powernow-k8 and acpi-cpufreq does not make sense
> > to me.
> > You would need extra logic that only the first is successfully loaded etc.
> > IMO this has more risk of introducing new bugs than any good.
> > A message like that might be useful though:
> > if (boot_cpu_has(X86_FEATURE_HW_PSTATE))
> > {
> > printk("powernowk8 does not serve MSR based frequency switching anymore, use acpi-cpufreq instead\n");
> > return -1;
> > }
>
> Matthew had something even better, that is patch 3/8:
> + if (static_cpu_has(X86_FEATURE_HW_PSTATE))
> + request_module("acpi_cpufreq");
>
> So if someone tries to load powernow-k8 on a recent CPU, it will
> automatically load acpi-cpufreq instead.
> I just realized that this doesn't work as expected, because powernow-k8
> bails out early due to only family 0xf being in the matching CPUID
> family list. Will fix this.
No need. I would just cut it out.
> I will do some tests now to check our options:
> 1. A combination of Matthew's and Thomas' ideas: if powernow-k8 is
> loaded on newer CPUs, request acpi-cpufreq to load _plus_ write a
> warning that powernow-k8 is obsolete for this hardware. Don't load
> powernow-k8 (which has support removed anyway). My favorite.
>
> 2. Add code (probably to the generic cpufreq framework) to avoid loading
> two drivers. Print a warning if tried anyways. Leave h/w P-state support
> in powernow-k8. It seems like that acpi-cpufreq takes precedence over
> powernow-k8 in distribution's module load list, so this should work as
> expected even with keeping the support in powernow-k8 (as a fallback in
> case of trouble).
I would just issue above message (see my other mail).
Loading of acpi-cpufreq, powernow-k8 and most other cpufreq drivers
(beside some old obscure ones, for example speedstep-*) just works since:
commit fa8031aefec0cf7ea6c2387c93610d99d9659aa2
Author: Andi Kleen <[email protected]>
cpufreq: Add support for x86 cpuinfo auto loading v4
If someone else (than udev via x86cpu autoloading modalias or
request_module() in processor driver) tries
to load powernow-k8 on a X86_FEATURE_HW_PSTATE capable machine,
powernow-k8 should just bail out with a "deprecated, do not try
to load" message, so that userspace init scripts can get fixed.
On which cpu families did you develop/test this (also an Intel one)?
I cannot promise, but I try to find time to give the one or other
different CPU a cpupower-bench test. With and without the patches.
If performance behavior is identical, one could be rather
sure that everything works the same way.
Thomas