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

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

[Dominik Brodowski]

Over the last few hours, I've rewritten large portions of ACPI processor
handling. Due to lack of appropriate hardware, I can hardly test these
changes, so expect a few rough edges.

These patches 
- a) replace the broken ACPI CPUfreq driver with a better, flexible variant,
- b) modularize the processor.c code: instead of one large file there are
     many small files.
- c) improve passive cooling support
- d) allow for easy adding of proper locking and ref-counting in processor.c

http://www.brodo.de/cpufreq_tmp/cpufreq-2.6.0-test4-ALL-series1
	Large CPUfreq update -- needed for other changes
	[in CPUfreq BK tree, merge with Linus expected soon]

http://www.brodo.de/cpufreq_tmp/cpufreq-2.6.0-test4-update_policy
http://www.brodo.de/cpufreq_tmp/cpufreq-2.6.0-test4-Kconfig-speedstep
	Additional CPUfreq updates -- needed for other changes
	[Already sent to CPUfreq list]

http://www.brodo.de/cpufreq_tmp/drivermodel-2.6.0-test4-exports
	Missing EXPORTs in sysdevice -- needed for other changes
	[Already sent to Patrick]


http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-remove_previous_pstates_implementation
	The previous arch/i386/kernel/cpu/cpufreq/acpi.c CPUfreq
	driver was broken, of bad design, and needs to replaced 
	by something better. But, as a first step, remove all 
	parts related to P-States from ACPI code.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_submodules
	Add a "submodule interface" to drivers/acpi/processor.c

	It allows to create other "modules" which access the acpi_handle 
	for the processor, and which get notified if the event value 
	maches the value passed in acpi_processor_register_notify.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_perflib
	This patch adds a new "P-States library" to drivers/acpi/

	CPUfreq drivers can now easly access the contents of the 
	_PCT and the _PSS. For example, the speedstep-centrino 
	driver could be appended so that it passes the appropriate 
	value to the P-States library which then evaluates _PDC, 
	and then returns the updated _PCT and _PSS.

	Also, the platform limit is now handled as a cpufreq notifier and
	as a call to cpufreq_policy_update.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-new_acpi_io_driver
	This re-adds the acpi P-States I/O driver. It is much smaller,
	leaner and cleaner.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_idle_submodule
	This moves the idle handler out of drivers/acpi/processor and into
	an own module.

	Even if only C1 is available, it is now used. If the user prefers the 
	default pm_idle, he can unload processor_idle, and still have the other
	functionality available.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_thermal_submodule
	This adds a new mechanism to manage passive cooling. Instead of
	hardcoded -and partly wrong- access to CPUfreq and ACPI 
	throttling-, add a generic mechanism which other modules can 
	register with.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_thermal_cpufreq
	Use _any_ CPUfreq driver for passive cooling. Implemented by an
	cpufreq policy notifier and cpufreq_update_policy.
 
http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_thermal_throttling
	Move throttling into its own submodule, and register it with the new
	passive cooling module. Also, the now-useless "limit" interface is
	removed.


Please review, test (carefully), and -if appropriate- merge,

	Dominik

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

[Dmitry Torokhov]

Dominik,

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, at least with
the following code on my notebook I was always getting garbage 
in the status register:

+static int
+acpi_processor_get_current_state(
+       struct acpi_processor_performance       *perf)
+{
+       int     result = 0;
+       int     i;
+       u8      value;
+
+       ACPI_FUNCTION_TRACE("acpi_processor_get_current_state");
+
+       if (!perf->pr->flags.performance)
+               goto out;
+
+       value = inb(perf->status_register);
+
+       ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+               "Got 0x%02x from port 0x%04x\n", value, perf->status_register));
+
+       for (i = 0; i < perf->state_count; i++) {
+               if (value == (u8) perf->states[i].status) {
+                       perf->state = i;
+                       goto out;
+               }
+       }
+
+       /* couldn't match our state table - garbage in status register */
+       ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+               "Bad data 0x%02x in performance status register 0x%04x\n",
+               value, perf->status_register));
+       result = -EFAULT;
+out:
+       return_VALUE(result);
+}

I am wondering is it just me, my code or general deficiency.

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

Dmitry

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

[Dominik Brodowski]

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);
 }

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

[Pavel Machek]

Hi!

>http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_thermal_cpufreq
> 	Use _any_ CPUfreq driver for passive cooling. Implemented by an
> 	cpufreq policy notifier and cpufreq_update_policy.
>  

Good - it is going to help a lot on k8 notebooks.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_thermal_throttling
> 	Move throttling into its own submodule, and register it with the new
> 	passive cooling module. Also, the now-useless "limit" interface is
> 	removed.
> 

Actually I liked to be able to echo 0:7 > limit and conserve battery/
make sure machine does not overheat  with it...


-- 
				Pavel
Written on sharp zaurus, because my Velo1 broke. If you have Velo you don't need...

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

[linux]

Hi Pavel,

Glad you liked [at least parts] of my patches.

On Mon, Sep 08, 2003 at 03:29:40PM +0200, Pavel Machek wrote:
> http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_thermal_throttling
> > 	Move throttling into its own submodule, and register it with the new
> > 	passive cooling module. Also, the now-useless "limit" interface is
> > 	removed.
> > 
> 
> Actually I liked to be able to echo 0:7 > limit and conserve battery/
> make sure machine does not overheat  with it...

echo 7 > throttling should do the same thing. However, don't expect any
battey conservation if ACPI C2 throttling works, and to set custom levels
against "overheating" there's a write access to
/proc/acpi/thermal/*/trip_points too...

	Dominik

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

[Pavel Machek]

Hi!

> Glad you liked [at least parts] of my patches.

:-). [BTW what is status of powernow-k8? is it pending or does it need
to be fixed some more?]

> On Mon, Sep 08, 2003 at 03:29:40PM +0200, Pavel Machek wrote:
> > http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_thermal_throttling
> > > 	Move throttling into its own submodule, and register it with the new
> > > 	passive cooling module. Also, the now-useless "limit" interface is
> > > 	removed.
> > > 
> > 
> > Actually I liked to be able to echo 0:7 > limit and conserve battery/
> > make sure machine does not overheat  with it...
> 
> echo 7 > throttling should do the same thing. 

Will not first change in thermal system unthrottle it for me, behind
my back?

> However, don't expect any
> battey conservation if ACPI C2 throttling works, and to set custom levels
> against "overheating" there's a write access to
> /proc/acpi/thermal/*/trip_points too...

I'd should not expect battery conservation *when cpu is idle*. If I'm
moving windows in X, 0:7 limit will actually help. And it should allow
battery to be drained further by limiting maximum amps eaten by
CPU. [Ok, that's little evil, but being able to run for 30 minutes+ on
0% battery is nice ;-)].
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

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

[linux]

Hi,

[powernow-k8 see separate e-mail]

On Wed, Sep 10, 2003 at 01:13:03AM +0200, Pavel Machek wrote:
> > On Mon, Sep 08, 2003 at 03:29:40PM +0200, Pavel Machek wrote:
> > > http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_thermal_throttling
> > > > 	Move throttling into its own submodule, and register it with the new
> > > > 	passive cooling module. Also, the now-useless "limit" interface is
> > > > 	removed.
> > > > 
> > > 
> > > Actually I liked to be able to echo 0:7 > limit and conserve battery/
> > > make sure machine does not overheat  with it...
> > 
> > echo 7 > throttling should do the same thing. 
> 
> Will not first change in thermal system unthrottle it for me, behind
> my back?

Well yes, it might, if the temperature had been higher than the passive
cooling trip point. If you want me to, I'll change the behaviour so that the
/proc/acpi/.../throttling input is consistent (minimum limit) until the user 
choses a different throttling level.

> > However, don't expect any
> > battey conservation if ACPI C2 throttling works, and to set custom levels
> > against "overheating" there's a write access to
> > /proc/acpi/thermal/*/trip_points too...
> 
> I'd should not expect battery conservation *when cpu is idle*. If I'm
> moving windows in X, 0:7 limit will actually help.

depends if you see it relative to the time or relative to the energy...

> And it should allow
> battery to be drained further by limiting maximum amps eaten by
> CPU. [Ok, that's little evil, but being able to run for 30 minutes+ on
> 0% battery is nice ;-)].

Well yes, for this case, setting throttling to 0:7 might indeed make
sense...

	Dominik

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

[Pavel Machek]

Hi!

> > > > http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_thermal_throttling
> > > > > 	Move throttling into its own submodule, and register it with the new
> > > > > 	passive cooling module. Also, the now-useless "limit" interface is
> > > > > 	removed.
> > > > > 
> > > > 
> > > > Actually I liked to be able to echo 0:7 > limit and conserve battery/
> > > > make sure machine does not overheat  with it...
> > > 
> > > echo 7 > throttling should do the same thing. 
> > 
> > Will not first change in thermal system unthrottle it for me, behind
> > my back?
> 
> Well yes, it might, if the temperature had been higher than the passive
> cooling trip point. If you want me to, I'll change the behaviour so that the
> /proc/acpi/.../throttling input is consistent (minimum limit) until the user 
> choses a different throttling level.

Yes I guess that would make sense.

								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]