[PATCH] speedstep-centrino should ignore upper performance control bits

[PATCH] speedstep-centrino should ignore upper performance control bits

[Gary Hade]

On some systems such as the IBM x3650 there are bits set in the 
upper half of the control values provided by the _PSS object.  
These bits are only relevant for cpufreq drivers that use IO ports 
which are not currently supported by the speedstep-centrino driver.  
The current MSR oriented code assumes that upper bits are not set 
and thus fails to work correctly when they are.  e.g. the control 
and status value equality check fails even though the ACPI spec 
allows the inequality.

Signed-off-by: Gary Hade <garyh@us.ibm.com>
---

--- linux-2.6.19-rc4/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c.orig	2006-11-06 10:40:03.000000000 -0800
+++ linux-2.6.19-rc4/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c	2006-11-06 14:23:23.849627864 -0800
@@ -463,6 +463,10 @@
 	}
 
 	for (i=0; i<p->state_count; i++) {
+		/* clear high bits (set by some BIOSes) that are non-relevant and 
+		   problematic for this driver's MSR only frequency transition code */
+		p->states[i].control &= 0xffff; 
+
 		if (p->states[i].control != p->states[i].status) {
 			dprintk("Different control (%llu) and status values (%llu)\n",
 				p->states[i].control, p->states[i].status);

Re: [PATCH] speedstep-centrino should ignore upper performance control bits

[Dominik Brodowski]

On Mon, Nov 06, 2006 at 03:39:23PM -0800, Gary Hade wrote:
> 
> On some systems such as the IBM x3650 there are bits set in the 
> upper half of the control values provided by the _PSS object.  
> These bits are only relevant for cpufreq drivers that use IO ports 
> which are not currently supported by the speedstep-centrino driver.  
... and probably never will.

> The current MSR oriented code assumes that upper bits are not set 
> and thus fails to work correctly when they are.  e.g. the control 
> and status value equality check fails even though the ACPI spec 
> allows the inequality.
How does the acpi-cpufreq driver work with it?

Thanks,
	Dominik

Re: [PATCH] speedstep-centrino should ignore upper performance control bits

[Len Brown]

On Monday 06 November 2006 21:14, Dominik Brodowski wrote:
> On Mon, Nov 06, 2006 at 03:39:23PM -0800, Gary Hade wrote:
> > 
> > On some systems such as the IBM x3650 there are bits set in the 
> > upper half of the control values provided by the _PSS object.  
> > These bits are only relevant for cpufreq drivers that use IO ports 
> > which are not currently supported by the speedstep-centrino driver.  
> ... and probably never will.
> 
> > The current MSR oriented code assumes that upper bits are not set 
> > and thus fails to work correctly when they are.  e.g. the control 
> > and status value equality check fails even though the ACPI spec 
> > allows the inequality.

> How does the acpi-cpufreq driver work with it?

acpu-cpufreq.c in Linus' tree is still IO only.
acpi-cpufreq. in Dave's tree changed to do this:
where  #define INTEL_MSR_RANGE                (0xffff)

-        cmd.addr.port = perf->control_register.address;
-        cmd.addr.bit_width = perf->control_register.bit_width;
-        cmd.val = (u32) perf->states[next_perf_state].control;
+        switch (data->cpu_feature) {
+            case SYSTEM_INTEL_MSR_CAPABLE:
+                cmd.type = SYSTEM_INTEL_MSR_CAPABLE;
+                cmd.addr.msr.reg = MSR_IA32_PERF_CTL;
+                msr = (u32) perf->states[next_perf_state].control & INTEL_MSR_RANGE;
+                cmd.val = (cmd.val & ~INTEL_MSR_RANGE) | msr;
+                break;
+            case SYSTEM_IO_CAPABLE:
+                cmd.type = SYSTEM_IO_CAPABLE;
+                cmd.addr.io.port = perf->control_register.address;
+                cmd.addr.io.bit_width = perf->control_register.bit_width;
+                cmd.val = (u32) perf->states[next_perf_state].control;
+                break;

Re: [PATCH] speedstep-centrino should ignore upper performance control bits

[Gary Hade]

On Tue, Nov 07, 2006 at 02:09:39AM -0500, Len Brown wrote:
> On Monday 06 November 2006 21:14, Dominik Brodowski wrote:
> > On Mon, Nov 06, 2006 at 03:39:23PM -0800, Gary Hade wrote:
> > > 
> > > On some systems such as the IBM x3650 there are bits set in the 
> > > upper half of the control values provided by the _PSS object.  
> > > These bits are only relevant for cpufreq drivers that use IO ports 
> > > which are not currently supported by the speedstep-centrino driver.  
> > ... and probably never will.
> > 
> > > The current MSR oriented code assumes that upper bits are not set 
> > > and thus fails to work correctly when they are.  e.g. the control 
> > > and status value equality check fails even though the ACPI spec 
> > > allows the inequality.
> 
> > How does the acpi-cpufreq driver work with it?
> 
> acpu-cpufreq.c in Linus' tree is still IO only.

True.  On the x3650 this version of acpi-cpufreq fails to load 
with a "No such device" error after debug message complaints
about lack of ACPI_ADR_SPACE_FIXED_HARDWARE address space support.
               ...
  cpufreq-core: adding CPU 0
  acpi-cpufreq: acpi_cpufreq_cpu_init
  acpi-cpufreq: Unsupported address space [127, 127]
  cpufreq-core: initialization failed
               ...

This problem is resolved in the later version mentioned below.

> acpi-cpufreq. in Dave's tree changed to do this:
> where  #define INTEL_MSR_RANGE                (0xffff)
> 
> -        cmd.addr.port = perf->control_register.address;
> -        cmd.addr.bit_width = perf->control_register.bit_width;
> -        cmd.val = (u32) perf->states[next_perf_state].control;
> +        switch (data->cpu_feature) {
> +            case SYSTEM_INTEL_MSR_CAPABLE:
> +                cmd.type = SYSTEM_INTEL_MSR_CAPABLE;
> +                cmd.addr.msr.reg = MSR_IA32_PERF_CTL;
> +                msr = (u32) perf->states[next_perf_state].control & INTEL_MSR_RANGE;
> +                cmd.val = (cmd.val & ~INTEL_MSR_RANGE) | msr;
> +                break;
> +            case SYSTEM_IO_CAPABLE:
> +                cmd.type = SYSTEM_IO_CAPABLE;
> +                cmd.addr.io.port = perf->control_register.address;
> +                cmd.addr.io.bit_width = perf->control_register.bit_width;
> +                cmd.val = (u32) perf->states[next_perf_state].control;
> +                break;

The acpi-cpufreq in 2.6.19-rc4-mm2 which contains these changes 
appears to work fine on the x3650. 

Gary

-- 
Gary Hade
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
garyhade@us.ibm.com
http://www.ibm.com/linux/ltc

Re: [PATCH] speedstep-centrino should ignore upper performance control bits

[Gary Hade]

On Tue, Nov 07, 2006 at 10:49:02AM -0800, Gary Hade wrote:
> On Tue, Nov 07, 2006 at 02:09:39AM -0500, Len Brown wrote:
> > On Monday 06 November 2006 21:14, Dominik Brodowski wrote:
> > > On Mon, Nov 06, 2006 at 03:39:23PM -0800, Gary Hade wrote:
> > > > 
> > > > On some systems such as the IBM x3650 there are bits set in the 
> > > > upper half of the control values provided by the _PSS object.  
> > > > These bits are only relevant for cpufreq drivers that use IO ports 
> > > > which are not currently supported by the speedstep-centrino driver.  
> > > ... and probably never will.
> > > 
> > > > The current MSR oriented code assumes that upper bits are not set 
> > > > and thus fails to work correctly when they are.  e.g. the control 
> > > > and status value equality check fails even though the ACPI spec 
> > > > allows the inequality.
> > 
> > > How does the acpi-cpufreq driver work with it?
> > 
> > acpu-cpufreq.c in Linus' tree is still IO only.
> 
> True.  On the x3650 this version of acpi-cpufreq fails to load 
> with a "No such device" error after debug message complaints
> about lack of ACPI_ADR_SPACE_FIXED_HARDWARE address space support.
>                ...
>   cpufreq-core: adding CPU 0
>   acpi-cpufreq: acpi_cpufreq_cpu_init
>   acpi-cpufreq: Unsupported address space [127, 127]
>   cpufreq-core: initialization failed
>                ...

The above result was obtained with Woodcrest processors 
installed in the system.  With Dempsey processors the 
address space is ACPI_ADR_SPACE_SYSTEM_IO and the 2.6.19-rc4 
version of acpi-cpufreq loads and functions correctly.

The patch I provided modifies the in-kernel copy of the ACPI 
data which is the wrong thing to do.  I will post an improved 
patch as soon as the Woodcrests are reinstalled and I am able 
to test it.

Thanks,
Gary

-- 
Gary Hade
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
garyhade@us.ibm.com
http://www.ibm.com/linux/ltc

Re: [PATCH] speedstep-centrino should ignore upper performance control bits

[Dave Jones]

On Wed, Nov 08, 2006 at 03:21:19PM -0800, Gary Hade wrote:

 > The above result was obtained with Woodcrest processors 
 > installed in the system.  With Dempsey processors the 
 > address space is ACPI_ADR_SPACE_SYSTEM_IO and the 2.6.19-rc4 
 > version of acpi-cpufreq loads and functions correctly.
 > 
 > The patch I provided modifies the in-kernel copy of the ACPI 
 > data which is the wrong thing to do.  I will post an improved 
 > patch as soon as the Woodcrests are reinstalled and I am able 
 > to test it.

Argh. I sat on the fence on this one until the dest settled,
and now, as soon as I merge it, this happens. Bah!

Send an incremental against cpufreq.git so I don't have to
reconstruct the whole git tree?  Or is the right change completely
different, causing a complete revert of this?

		Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH] speedstep-centrino should ignore upper performance control bits

[Gary Hade]

On Wed, Nov 08, 2006 at 06:29:55PM -0500, Dave Jones wrote:
> On Wed, Nov 08, 2006 at 03:21:19PM -0800, Gary Hade wrote:
> 
>  > The above result was obtained with Woodcrest processors 
>  > installed in the system.  With Dempsey processors the 
>  > address space is ACPI_ADR_SPACE_SYSTEM_IO and the 2.6.19-rc4 
>  > version of acpi-cpufreq loads and functions correctly.
>  > 
>  > The patch I provided modifies the in-kernel copy of the ACPI 
>  > data which is the wrong thing to do.  I will post an improved 
>  > patch as soon as the Woodcrests are reinstalled and I am able 
>  > to test it.
> 
> Argh. I sat on the fence on this one until the dest settled,
> and now, as soon as I merge it, this happens. Bah!

Dave, I'm sorry.  I just discovered the problem. :(

> 
> Send an incremental against cpufreq.git so I don't have to
> reconstruct the whole git tree?  

Okay, I'll try to figure out how to do that.

> Or is the right change completely
> different, causing a complete revert of this?

The change will completely remove the lines of codes added 
by the first patch and revise other existing lines of code.

Gary

-- 
Gary Hade
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
garyhade@us.ibm.com
http://www.ibm.com/linux/ltc

Re: [PATCH] speedstep-centrino should ignore upper performance control bits

[Dave Jones]

On Wed, Nov 08, 2006 at 04:00:31PM -0800, Gary Hade wrote:

 > > Argh. I sat on the fence on this one until the dest settled,
 > > and now, as soon as I merge it, this happens. Bah!
 > Dave, I'm sorry.  I just discovered the problem. :(

Ok, I just tried to reconstruct my git tree without that diff
and hit problems trying to get the older patches applying against
a current tree, so I don't want to go down the route of making
everyone rediff and resend the 20 or so patches already queued.
However..

 > > Send an incremental against cpufreq.git so I don't have to
 > > reconstruct the whole git tree?  
 > Okay, I'll try to figure out how to do that.

git revert has nuked the change, but leaves a mess in the commits,
but I think we can live with that.  So just send the right fix,
and we'll make like this never happened :-)

		Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH] speedstep-centrino should ignore upper performance control bits

[Gary Hade]

On Wed, Nov 08, 2006 at 07:22:08PM -0500, Dave Jones wrote:
> On Wed, Nov 08, 2006 at 04:00:31PM -0800, Gary Hade wrote:
> 
>  > > Argh. I sat on the fence on this one until the dest settled,
>  > > and now, as soon as I merge it, this happens. Bah!
>  > Dave, I'm sorry.  I just discovered the problem. :(
> 
> Ok, I just tried to reconstruct my git tree without that diff
> and hit problems trying to get the older patches applying against
> a current tree, so I don't want to go down the route of making
> everyone rediff and resend the 20 or so patches already queued.
> However..
> 
>  > > Send an incremental against cpufreq.git so I don't have to
>  > > reconstruct the whole git tree?  
>  > Okay, I'll try to figure out how to do that.
> 
> git revert has nuked the change, but leaves a mess in the commits,
> but I think we can live with that.  So just send the right fix,
> and we'll make like this never happened :-)

Dave, I apologize again for the trouble.  Improved patch
which does essentially the same thing but avoids modifying
the in-kernel ACPI data is included below.

Thanks!
Gary

-- 
Gary Hade
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
garyhade@us.ibm.com
http://www.ibm.com/linux/ltc


On some systems there could be bits set in the upper half of 
the control value provided by the _PSS object.  These bits are 
only relevant for cpufreq drivers that use IO ports which are not 
currently supported by the speedstep-centrino driver.  The current 
MSR oriented code assumes that upper bits are not set and thus 
fails to work correctly when they are.  e.g. the control and status 
value equality check failed on the IBM x3650 even though the ACPI 
spec allows inequality. 

Signed-off-by: Gary Hade <garyhade@us.ibm.com>

--- linux-2.6.19-rc5/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c.orig	2006-11-07 18:24:20.000000000 -0800
+++ linux-2.6.19-rc5/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c	2006-11-10 09:02:29.000000000 -0800
@@ -36,6 +36,7 @@
 
 #define dprintk(msg...) cpufreq_debug_printk(CPUFREQ_DEBUG_DRIVER, "speedstep-centrino", msg)
 
+#define INTEL_MSR_RANGE	(0xffff)
 
 struct cpu_id
 {
@@ -463,8 +464,9 @@ static int centrino_cpu_init_acpi(struct
 	}
 
 	for (i=0; i<p->state_count; i++) {
-		if (p->states[i].control != p->states[i].status) {
-			dprintk("Different control (%llu) and status values (%llu)\n",
+		if ((p->states[i].control & INTEL_MSR_RANGE) != 
+		    (p->states[i].status & INTEL_MSR_RANGE)) {
+			dprintk("Different MSR bits in control (%llu) and status (%llu)\n",
 				p->states[i].control, p->states[i].status);
 			result = -EINVAL;
 			goto err_unreg;
@@ -500,7 +502,7 @@ static int centrino_cpu_init_acpi(struct
         }
 
         for (i=0; i<p->state_count; i++) {
-		centrino_model[cpu]->op_points[i].index = p->states[i].control;
+		centrino_model[cpu]->op_points[i].index = p->states[i].control & INTEL_MSR_RANGE;
 		centrino_model[cpu]->op_points[i].frequency = p->states[i].core_frequency * 1000;
 		dprintk("adding state %i with frequency %u and control value %04x\n", 
 			i, centrino_model[cpu]->op_points[i].frequency, centrino_model[cpu]->op_points[i].index);

Re: [PATCH] speedstep-centrino should ignore upper performance control bits

[Gary Hade]

On Mon, Nov 06, 2006 at 03:39:23PM -0800, Gary Hade wrote:
> 
> On some systems such as the IBM x3650 there are bits set in the 
> upper half of the control values provided by the _PSS object.  
> These bits are only relevant for cpufreq drivers that use IO ports 
> which are not currently supported by the speedstep-centrino driver.  
> The current MSR oriented code assumes that upper bits are not set 
> and thus fails to work correctly when they are.  e.g. the control 
> and status value equality check fails even though the ACPI spec 
> allows the inequality.
> 
> Signed-off-by: Gary Hade <garyh@us.ibm.com>

Sorry, incorrect email address.  This should be
  Signed-off-by: Gary Hade <garyhade@us.ibm.com>

Gary

-- 
Gary Hade
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
garyhade@us.ibm.com
http://www.ibm.com/linux/ltc