Re: [ACPI] [PATCHES] ACPI Processor update [idle, throttling, thermal, cpufreq]

Dominik Brodowski
2003-09-05T06:52:39Z

Dmitry,

Thanks for your input.

On Fri, Sep 05, 2003 at 12:23:06AM -0500, Dmitry Torokhov wrote:
> I have couple of concerns regarding P-states IO library, esp.
> acpi_processor_get_frequency. It seems that ACPI does not allow
> to read current state without setting it first.

Indeed. The specification allows this behaviour. That's really annoying, but
well...

> Also, do you really need to do notify_transition twice
> (acpi_processor_set_performance)?

No, it's a bug.

The attached patch should fix both issues.

diff -ruN linux-original/arch/i386/kernel/cpu/cpufreq/acpi-io-cpufreq.c linux/arch/i386/kernel/cpu/cpufreq/acpi-io-cpufreq.c
--- linux-original/arch/i386/kernel/cpu/cpufreq/acpi-io-cpufreq.c 2003-09-04 21:36:58.000000000 +0200
+++ linux/arch/i386/kernel/cpu/cpufreq/acpi-io-cpufreq.c 2003-09-05 08:47:54.302664624 +0200
@@ -33,6 +33,7 @@
#include <asm/io.h>
#include <asm/delay.h>
#include <asm/uaccess.h>
+#include <asm/timex.h>

#include <linux/acpi.h>
#include <acpi/processor.h>
@@ -54,32 +55,16 @@
u16 control_register;
u16 status_register;

+ unsigned long previous_freq;
struct cpufreq_frequency_table *table;
};

static struct cpufreq_acpi_io_data acpi_io_data[NR_CPUS];

-static unsigned long acpi_processor_get_frequency (
- struct cpufreq_acpi_io_data *data)
-{
- unsigned int i;
- u16 port;
- u8 value;
-
- port = data->status_register;
- value = inb(port);
-
- for (i=0; i<data->data->state_count; i++) {
- if (value == (u8) data->data->states[i].status)
- return (1000 * data->data->states[i].core_frequency);
- }
-
- return 0;
-}
-
static int acpi_processor_set_performance (
struct cpufreq_acpi_io_data *data,
- int state)
+ int state,
+ unsigned int notify)
{
u16 port;
u8 value;
@@ -88,14 +73,12 @@

/* cpufreq frequency struct */
cpufreq_freqs.cpu = data->cpu;
- cpufreq_freqs.old = acpi_processor_get_frequency(data);
+ cpufreq_freqs.old = data->previous_freq;
cpufreq_freqs.new = data->data->states[state].core_frequency;

/* notify cpufreq */
- cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);
-
- /* notify cpufreq */
- cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);
+ if (notify)
+ cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);

/*
* First we write the target state's 'control' value to the
@@ -129,18 +112,21 @@
}

/* notify cpufreq */
+ if (notify)
cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_POSTCHANGE);

if (value != data->data->states[state].status) {
unsigned int tmp = cpufreq_freqs.new;
cpufreq_freqs.new = cpufreq_freqs.old;
cpufreq_freqs.old = tmp;
- cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);
- cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_POSTCHANGE);
+ if (notify) {
+ cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);
+ cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_POSTCHANGE);
+ }
printk(KERN_ERR "Transition failed\n");
return -EINVAL;
}
-
+ data->previous_freq = data->data->states[state].core_frequency * 1000;
dprintk(KERN_INFO "Transition successful after %d microseconds\n", i * 10);
return 0;
}
@@ -162,7 +148,7 @@
if (result)
return (result);

- return acpi_processor_set_performance (data, next_state);
+ return acpi_processor_set_performance (data, next_state, 1);
}

static int acpi_cpufreq_verify (struct cpufreq_policy *policy)
@@ -183,6 +169,7 @@
struct acpi_perflib_data *data;
struct cpufreq_frequency_table *table;
unsigned int i;
+ unsigned long tmp_speed;

data = acpi_processor_perflib_register (&acpi_cpufreq_perflib_driver,
policy->cpu);
@@ -221,7 +208,29 @@
acpi_io_data[policy->cpu].status_register = (u16) data->pct_status.address;
acpi_io_data[policy->cpu].control_register = (u16) data->pct_control.address;

- return 0;
+ /*
+ * The ACPI specificiation is broken in the regard that
+ * it is impossible to detect the _current_ P-State.
+ * The control_register is only valid _after_ a frequency
+ * transition.
+ * So, we try to get the current setting from cpu_khz,
+ * but to make sure, this state we think the CPU is in
+ * is also set.
+ */
+ if (cpu_khz) {
+ tmp_speed = cpu_khz / 100;
+ tmp_speed *= 105;
+ } else {
+ tmp_speed = policy->cpuinfo.max_freq;
+ }
+ result = cpufreq_frequency_table_target(policy, data->table,
+ tmp_speed,
+ CPUFREQ_RELATION_L,
+ &i);
+ if (result)
+ return (result);
+
+ return acpi_processor_set_performance (data, i, 0);
}