[2.5. PATCH] cpufreq: correct initialization on Intel Coppermines

[2.5. PATCH] cpufreq: correct initialization on Intel Coppermines

[Dominik Brodowski]

[-- Attachment #1: Type: text/plain, Size: 4881 bytes --]

[2.5. PATCH] cpufreq: Intel Coppermines -- the saga continues.

The detection process for speedstep-enabled Pentium III Coppermines is
considered proprietary by Intel. The attempt to detect this
capability using MSRs failed. So, users need to pass the option
"speedstep_coppermine=1" to the kernel (boot option or parameter) if
they own a SpeedStep capable PIII Coppermine processor. Tualatins work
as before.

       Dominik


--- linux-2546original/arch/i386/kernel/cpu/cpufreq/speedstep.c	Tue Nov  5 12:27:14 2002
+++ linux/arch/i386/kernel/cpu/cpufreq/speedstep.c	Tue Nov  5 13:01:39 2002
@@ -1,5 +1,5 @@
 /*
- *  $Id: speedstep.c,v 1.53 2002/09/29 23:43:11 db Exp $
+ *  $Id: speedstep.c,v 1.57 2002/11/05 12:01:12 db Exp $
  *
  * (C) 2001  Dave Jones, Arjan van de ven.
  * (C) 2002  Dominik Brodowski <linux@brodo.de>
@@ -46,7 +46,8 @@
 
 /* speedstep_processor
  */
-static unsigned int                     speedstep_processor;
+static unsigned int                     speedstep_processor = 0;
+static int                              speedstep_coppermine = 0;
 
 #define SPEEDSTEP_PROCESSOR_PIII_C      0x00000001  /* Coppermine core */
 #define SPEEDSTEP_PROCESSOR_PIII_T      0x00000002  /* Tualatin core */
@@ -72,7 +73,7 @@
 #ifdef SPEEDSTEP_DEBUG
 #define dprintk(msg...) printk(msg)
 #else
-#define dprintk(msg...) do { } while(0);
+#define dprintk(msg...) do { } while(0)
 #endif
 
 
@@ -429,7 +430,7 @@
 /** 
  * speedstep_detect_processor - detect Intel SpeedStep-capable processors.
  *
- *   Returns the SPEEDSTEP_PROCESSOR_-number for the detected chipset, 
+ *   Returns the SPEEDSTEP_PROCESSOR_-number for the detected processor, 
  * or zero on failure.
  */
 static unsigned int speedstep_detect_processor (void)
@@ -474,8 +475,6 @@
 		return SPEEDSTEP_PROCESSOR_PIII_T;
 
 	case 0x08: /* Intel PIII [Coppermine] */
-		/* based on reverse-engineering information and
-		 * some guessing. HANDLE WITH CARE! */
  	        {
 			u32     msr_lo, msr_hi;
 
@@ -487,21 +486,13 @@
 			if (msr_lo != 0x0080000)
 				return 0;
 
-			/* platform ID seems to be 0x00140000 */
-			rdmsr(MSR_IA32_PLATFORM_ID, msr_lo, msr_hi);
-			dprintk(KERN_DEBUG "cpufreq: Coppermine: MSR_IA32_PLATFORM ID is 0x%x, 0x%x\n", msr_lo, msr_hi);
-			msr_hi = msr_lo & 0x001c0000;
-			if (msr_hi != 0x00140000)
-				return 0;
-
-			/* and these bits seem to be either 00_b, 01_b or
-			 * 10_b but never 11_b */
-			msr_lo &= 0x00030000;
-			if (msr_lo == 0x0030000)
-				return 0;
+			if (speedstep_coppermine)
+				return SPEEDSTEP_PROCESSOR_PIII_C;
 
-			/* let's hope this is correct... */
-			return SPEEDSTEP_PROCESSOR_PIII_C;
+			printk(KERN_INFO "cpufreq: in case this is a SpeedStep-capable Intel Pentium III Coppermine\n");
+			printk(KERN_INFO "cpufreq: processor, please pass the boot option or module parameter\n");
+			printk(KERN_INFO "cpufreq: `speedstep_coppermine=1` to the kernel. Thanks!\n");
+			return 0;
 		}
 
 	default:
@@ -622,6 +613,25 @@
 }
 
 
+#ifndef MODULE
+/**
+ * speedstep_setup  speedstep command line parameter parsing
+ *
+ * speedstep command line parameter.  Use:
+ *  speedstep_coppermine=1
+ * if the CPU in your notebook is a SpeedStep-capable Intel
+ * Pentium III Coppermine. These processors cannot be detected
+ * automatically, as Intel continues to consider the detection 
+ * alogrithm as proprietary material.
+ */
+static int __init speedstep_setup(char *str)
+{
+	speedstep_coppermine = simple_strtoul(str, &str, 0);
+	return 1;
+}
+__setup("speedstep_coppermine=", speedstep_setup);
+#endif
+
 /**
  * speedstep_init - initializes the SpeedStep CPUFreq driver
  *
@@ -637,18 +647,19 @@
 
 
 	/* detect chipset */
-	speedstep_chipset = speedstep_detect_chipset();
+	speedstep_chipset = speedstep_detect_chipset(); 
 
 	/* detect chipset */
 	if (speedstep_chipset)
 		speedstep_processor = speedstep_detect_processor();
 
 	if ((!speedstep_chipset) || (!speedstep_processor)) {
-		dprintk(KERN_INFO "cpufreq: Intel(R) SpeedStep(TM) for this %s not (yet) available.\n", speedstep_processor ? "chipset" : "processor");
+		printk(KERN_INFO "a 0x%x b 0x%x\n", speedstep_processor, speedstep_chipset);
+		printk(KERN_INFO "cpufreq: Intel(R) SpeedStep(TM) for this %s not (yet) available.\n", speedstep_chipset ? "processor" : "chipset");
 		return -ENODEV;
 	}
 
-	dprintk(KERN_INFO "cpufreq: Intel(R) SpeedStep(TM) support $Revision: 1.53 $\n");
+	dprintk(KERN_INFO "cpufreq: Intel(R) SpeedStep(TM) support $Revision: 1.57 $\n");
 	dprintk(KERN_DEBUG "cpufreq: chipset 0x%x - processor 0x%x\n", 
 	       speedstep_chipset, speedstep_processor);
 
@@ -726,3 +737,5 @@
 MODULE_LICENSE ("GPL");
 module_init(speedstep_init);
 module_exit(speedstep_exit);
+
+MODULE_PARM (speedstep_coppermine, "i");

[-- Attachment #2: Type: application/pgp-signature, Size: 240 bytes --]

Re: [2.5. PATCH] cpufreq: correct initialization on Intel Coppermines

[Samuel Thibault]

On fri 08 nov 2002 09:30:08 GMT, Dominik Brodowski wrote:
> The detection process for speedstep-enabled Pentium III Coppermines is
> considered proprietary by Intel.

They seem to have changed their mind:

http://www.intel.com/support/processors/sb/cs-003779-prd24.htm

Which looks a bit like what was implemented at first. Here is a patch.
I kept the setup parameter, but it might be removed now?

--- linux-2.5.70-bk12/arch/i386/kernel/cpu/cpufreq/speedstep.c	2003-05-26 21:00:20.000000000 -0400
+++ linux/arch/i386/kernel/cpu/cpufreq/speedstep.c	2003-06-14 04:33:04.000000000 -0400
@@ -503,6 +503,15 @@
 			if (speedstep_coppermine)
 				return SPEEDSTEP_PROCESSOR_PIII_C;
 
+			/* if the processor is a mobile version,
+			 * platform ID has bit 50 set
+			 * it has SpeedStep technology if either
+			 * bit 56 or 57 is set */
+			rdmsr(MSR_IA32_PLATFORM_ID, msr_lo, msr_hi);
+			dprintk(KERN_DEBUG "cpufreq: Coppermine: MSR_IA32_PLATFORM ID is 0x%x, 0x%x\n", msr_lo, msr_hi);
+			if ((msr_hi & (1<<18)) && (msr_hi & (3<<24)))
+				return SPEEDSTEP_PROCESSOR_PIII_C;
+
 			printk(KERN_INFO "cpufreq: in case this is a SpeedStep-capable Intel Pentium III Coppermine\n");
 			printk(KERN_INFO "cpufreq: processor, please pass the boot option or module parameter\n");
 			printk(KERN_INFO "cpufreq: `speedstep_coppermine=1` to the kernel. Thanks!\n");

[2.5 PATCH] bug if cpufreq driver initialization fails

[Samuel Thibault]

Le dim 15 jun 2003 11:50:44 GMT, Dominik Brodowski a tapoté sur son clavier :
> > However, my 440BX chipset is still not supported, and I couldn't find
> > anything in Intel's documentation...
> 
> Do you know of Bruno Ducrot's work? 
> 
> http://www.poupinou.org/cpufreq/

I read the list archive yesterday, and tried this work. I got the results
http://youpibouh.thefreecat.org/speedstep/ac
http://youpibouh.thefreecat.org/speedstep/battery
and hence tried my gpo_hilo=11, and it works... Sometimes.

I seems like the hardware si sometimes slow to get to the low speed. My
PIII is a 800Mhz/650Mhz, but initialization sometimes shows 800 and 800,
as if the speed measure was done before the hardware actually achieved
the slow down.

As a result, speedstep_cpu_init() fails, hence cpufreq_add_dev() fails,
which is ignored by sysdev_driver_register(). In the end, the module is
kept loaded (since speedstep_init() successes, since
cpufreq_register_driver successes()), and I have to unload it by hand,
which entails kobject badness warning of course, since cpufreq then
tries to remove entries which were actually never added to /sys!

I hence modified drivers/base/sys.c to have sysdev_driver_register()
fail as well, and then I also had to modify kernel/cpufreq.c, because
this failure did not imply a setting cpufreq_driver to NULL (preventing
me from reinsmoding speedstep-ich: EBUSY)

I'd also suggest that the speedstep drivers printks something if
everything went ok (including the cpufreq_frequency_table_cpuinfo()
call), the low & high speed for instance, just to be sure everything
went ok

Regards,
Samuel Thibault

--- linux-2.5.71-orig/drivers/base/sys.c	2003-06-14 17:12:52.000000000 -0400
+++ linux-2.5.71-perso/drivers/base/sys.c	2003-06-15 13:47:45.000000000 -0400
@@ -116,6 +116,7 @@
 int sysdev_driver_register(struct sysdev_class * cls, 
 			   struct sysdev_driver * drv)
 {
+	int ret = 0;
 	down_write(&system_subsys.rwsem);
 	if (cls && kset_get(&cls->kset)) {
 		list_add_tail(&drv->entry,&cls->drivers);
@@ -123,13 +124,16 @@
 		/* If devices of this class already exist, tell the driver */
 		if (drv->add) {
 			struct sys_device *dev;
-			list_for_each_entry(dev, &cls->kset.list, kobj.entry)
-				drv->add(dev);
+			list_for_each_entry(dev, &cls->kset.list, kobj.entry) {
+				ret = drv->add(dev);
+				if (ret) goto out;
+			}
 		}
 	} else
 		list_add_tail(&drv->entry,&global_drivers);
+out:
 	up_write(&system_subsys.rwsem);
-	return 0;
+	return ret;
 }
 
 
--- linux-2.5.71-orig/kernel/cpufreq.c	2003-06-14 17:13:39.000000000 -0400
+++ linux-2.5.71-perso/kernel/cpufreq.c	2003-06-15 13:55:36.000000000 -0400
@@ -811,6 +811,8 @@
  */
 int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 {
+	int ret;
+
 	if (!driver_data || !driver_data->verify || !driver_data->init ||
 	    ((!driver_data->setpolicy) && (!driver_data->target)))
 		return -EINVAL;
@@ -825,13 +827,17 @@
 
 	cpufreq_driver->policy = kmalloc(NR_CPUS * sizeof(struct cpufreq_policy), GFP_KERNEL);
 	if (!cpufreq_driver->policy) {
-		cpufreq_driver = NULL;
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out;
 	}
 
 	memset(cpufreq_driver->policy, 0, NR_CPUS * sizeof(struct cpufreq_policy));
 
-	return sysdev_driver_register(&cpu_sysdev_class,&cpufreq_sysdev_driver);
+	ret = sysdev_driver_register(&cpu_sysdev_class,&cpufreq_sysdev_driver);
+out:
+	if (ret)
+		cpufreq_driver = NULL;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(cpufreq_register_driver);
 

Re: [2.5 PATCH] bug if cpufreq driver initialization fails

[Russell King]

On Sun, Jun 15, 2003 at 02:04:36PM -0400, Samuel Thibault wrote:
> I hence modified drivers/base/sys.c to have sysdev_driver_register()
> fail as well, and then I also had to modify kernel/cpufreq.c, because
> this failure did not imply a setting cpufreq_driver to NULL (preventing
> me from reinsmoding speedstep-ich: EBUSY)

Unfortunately, you created a by by doing so.  Eg:

- you have 3 devices on kset.list.
- you successfully register 2 of them with a driver.
- you fail one.
- sysdev_driver_register returns failure.
- module is unloaded while other parts of the kernel have references into
  the driver.
- the kernel oopses.

> I'd also suggest that the speedstep drivers printks something if
> everything went ok (including the cpufreq_frequency_table_cpuinfo()
> call), the low & high speed for instance, just to be sure everything
> went ok

IMO its better to printk something on failure.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [2.5 PATCH] bug if cpufreq driver initialization fails

[Samuel Thibault]

Le dim 15 jun 2003 19:16:50 GMT, Russell King a tapoté sur son clavier :
> On Sun, Jun 15, 2003 at 02:04:36PM -0400, Samuel Thibault wrote:
> > I hence modified drivers/base/sys.c to have sysdev_driver_register()
> > fail as well, and then I also had to modify kernel/cpufreq.c, because
> > this failure did not imply a setting cpufreq_driver to NULL (preventing
> > me from reinsmoding speedstep-ich: EBUSY)
> 
> Unfortunately, you created a by by doing so.  Eg:
> 
> - you have 3 devices on kset.list.
> - you successfully register 2 of them with a driver.
> - you fail one.
> - sysdev_driver_register returns failure.
> - module is unloaded while other parts of the kernel have references into
>   the driver.
> - the kernel oopses.
Ok, that's what I was wondering, might an exit loop removing the already
added driver be done?

(the second half of the patch might still be applied as soon as now)

Regards,
Samuel Thibault

Re: [2.5 PATCH] bug if cpufreq driver initialization fails

[Russell King]

On Sun, Jun 15, 2003 at 02:25:07PM -0400, Samuel Thibault wrote:
> Le dim 15 jun 2003 19:16:50 GMT, Russell King a tapoté sur son clavier :
> > On Sun, Jun 15, 2003 at 02:04:36PM -0400, Samuel Thibault wrote:
> > > I hence modified drivers/base/sys.c to have sysdev_driver_register()
> > > fail as well, and then I also had to modify kernel/cpufreq.c, because
> > > this failure did not imply a setting cpufreq_driver to NULL (preventing
> > > me from reinsmoding speedstep-ich: EBUSY)
> > 
> > Unfortunately, you created a by by doing so.  Eg:
> > 
> > - you have 3 devices on kset.list.
> > - you successfully register 2 of them with a driver.
> > - you fail one.
> > - sysdev_driver_register returns failure.
> > - module is unloaded while other parts of the kernel have references into
> >   the driver.
> > - the kernel oopses.
> Ok, that's what I was wondering, might an exit loop removing the already
> added driver be done?
> 
> (the second half of the patch might still be applied as soon as now)

Also, I think people need to consider whether "all devices failed"
or "any device failed" counts as overall failure.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html