[PATCH] 2.6.5 speedstep on P4Ms

[PATCH] 2.6.5 speedstep on P4Ms

[Christian Hoelbling]

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

  here is a small patch that should address the following issues for 
speedstep on P4M's

1.) detect all P4M's via the model_id string
2.) correctly register drivers on hyperthreading CPU's
3.) do P4-clockmod on top of speedstep on P4-Ms

 since this is my first attempt at kernel programming, it's probably an 
uglu hack. also there is a problem, that the powersave governor does not 
get the lowest frequency correctly. any suggestions/tips are very welcome.

chris

[-- Attachment #2: patch-2.6.5 --]
[-- Type: text/x-troff-man, Size: 16694 bytes --]

diff --unified --recursive --new-file linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
--- linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-07 20:49:36.000000000 +0000
+++ linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-07 20:56:00.732209368 +0000
@@ -11,6 +11,9 @@
  *  for extensive testing.
  *
  *  BIG FAT DISCLAIMER: Work in progress code. Possibly *dangerous*
+ *
+ *  Added SMT and p4-clockmod support on P4-M's
+ *  Christian Hoelbling, 2004
  */
 
 
@@ -27,7 +30,6 @@
 
 #include "speedstep-lib.h"
 
-
 /* speedstep_chipset:
  *   It is necessary to know which chipset is used. As accesses to 
  * this device occur at various places in this module, we need a 
@@ -51,6 +53,44 @@
 	{0,			CPUFREQ_TABLE_END},
 };
 
+/*
+ * Duty Cycle (3bits) from p4-clockmod
+ * for each of the voltages
+ */
+enum {
+        DC_RESV, LO_DC_DFLT, LO_DC_25PT, LO_DC_38PT, LO_DC_50PT,
+	LO_DC_64PT, LO_DC_75PT, LO_DC_88PT, LO_DC_DISABLE,
+        HI_DC_DFLT, HI_DC_25PT, HI_DC_38PT, HI_DC_50PT,
+	HI_DC_64PT, HI_DC_75PT, HI_DC_88PT, HI_DC_DISABLE
+};
+
+/* 
+ * Extended speedstep + p4-clockmod table
+ * for P4-M's
+ * Values are in kHz for the time being.
+ */
+static struct cpufreq_frequency_table speedstep_p4_freqs[] = {
+	{DC_RESV, CPUFREQ_ENTRY_INVALID},
+	{LO_DC_DFLT, 0},
+	{LO_DC_25PT, 0},
+	{LO_DC_38PT, 0},
+	{LO_DC_50PT, 0},
+	{LO_DC_64PT, 0},
+	{LO_DC_75PT, 0},
+	{LO_DC_88PT, 0},
+	{LO_DC_DISABLE, 0},
+	{HI_DC_DFLT, 0},
+	{HI_DC_25PT, 0},
+	{HI_DC_38PT, 0},
+	{HI_DC_50PT, 0},
+	{HI_DC_64PT, 0},
+	{HI_DC_75PT, 0},
+	{HI_DC_88PT, 0},
+	{HI_DC_DISABLE, 0},
+	{DC_RESV, CPUFREQ_TABLE_END},
+};
+
+static unsigned int stock_freq[2];
 
 /* DEBUG
  *   Define it if you want verbose debug output, e.g. for bug reporting
@@ -63,7 +103,6 @@
 #define dprintk(msg...) do { } while(0)
 #endif
 
-
 /**
  * speedstep_set_state - set the SpeedStep state
  * @state: new processor frequency state (SPEEDSTEP_LOW or SPEEDSTEP_HIGH)
@@ -148,6 +187,251 @@
 	return;
 }
 
+/* This sets P4M's clockmod and speedstep settings.
+ * Note: clockmod only works reliably in the high speedstep
+ *       state. Therefore, the routine always switches to the
+ *       high speedstep state to do the clockmod, even if going
+ *       from one low voltage frequency to another.
+ */
+static int speedstep_p4_set_state(unsigned int cpu, unsigned int newstate, unsigned int notify)
+{
+	u32 l, h, pmbase;
+	u8 pm2_blk, value;
+	cpumask_t cpus_allowed, affected_cpu_map;
+	unsigned long flags;
+	struct cpufreq_freqs freqs;
+	int hyperthreading = 0;
+	int sibling = 0;
+	unsigned int dc_newstate,speedstep_newstate,oldstate,dc_oldstate,speedstep_oldstate;
+	unsigned int speedstep_trans,dc_trans;
+
+	if (!cpu_online(cpu) || (newstate > HI_DC_DISABLE) || 
+		(newstate == DC_RESV))
+		return -EINVAL;
+
+	/* disentangle speedstep and clockmod */
+	dc_newstate=((newstate-1) & 0x07)+1;
+	speedstep_newstate=(newstate>LO_DC_DISABLE);
+
+	/* switch to physical CPU where state is to be changed*/
+	cpus_allowed = current->cpus_allowed;
+
+	/* only run on CPU to be set, or on its sibling */
+       affected_cpu_map = cpumask_of_cpu(cpu);
+#ifdef CONFIG_X86_HT
+	hyperthreading = ((cpu_has_ht) && (smp_num_siblings == 2));
+	if (hyperthreading) {
+		sibling = cpu_sibling_map[cpu];
+                cpu_set(sibling, affected_cpu_map);
+	}
+#endif
+	set_cpus_allowed(current, affected_cpu_map);
+        BUG_ON(!cpu_isset(smp_processor_id(), affected_cpu_map));
+
+	/* get current clockmod state */
+	rdmsr(MSR_IA32_THERM_CONTROL, l, h);
+	if (l & 0x10) {
+		dc_oldstate = l >> 1;
+		dc_oldstate &= 0x7;
+	} else
+	        dc_oldstate = 0x08; /* dc disabled */
+
+	if (dc_oldstate == DC_RESV) {
+	        printk(KERN_ERR "cpufreq: BIG FAT WARNING: currently in invalid setting\n");
+	}
+	dc_trans=(dc_oldstate != dc_newstate);
+
+	/* get currect speedstep state */
+	speedstep_oldstate=(speedstep_get_processor_frequency(speedstep_processor)==speedstep_p4_freqs[HI_DC_DISABLE].frequency);
+	speedstep_trans=(speedstep_oldstate != speedstep_newstate);
+	oldstate  = ((dc_oldstate-1) + (speedstep_oldstate << 3)) + 1;
+
+	if (notify) {
+	        freqs.old = speedstep_p4_freqs[oldstate].frequency; /* relies on indices for all allowed states in frequency table being in sequential order starting from 1 */
+		freqs.cpu = cpu;
+		dprintk(KERN_DEBUG "cpufreq: os: 0x%x ns: 0x%x ssos: 0x%x ssns: 0x%x dcos: 0x%x dcns: 0x%x\n", oldstate,newstate,speedstep_oldstate,speedstep_newstate,dc_oldstate,dc_newstate);
+
+		/* notifiers */
+		freqs.new = speedstep_p4_freqs[newstate].frequency; /* relies on indices for all allowed states in frequency table being in sequential order starting from 1 */
+		dprintk(KERN_DEBUG "cpufreq: preparing transition on cpu %i from %i kHz to %ikHz \n",cpu ,freqs.old ,freqs.new);
+		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+		if (hyperthreading) {
+		        freqs.cpu = sibling;
+			cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+		}
+	}
+
+	/* set speedstep to upper state if needed */
+	if ((speedstep_trans && (speedstep_newstate==0x1)) || 
+	    (dc_trans && (speedstep_oldstate==0x0))){
+
+	        /* get PMBASE */
+	        pci_read_config_dword(speedstep_chipset_dev, 0x40, &pmbase);
+		if (!(pmbase & 0x01)) {
+		        printk(KERN_ERR "cpufreq: could not find speedstep register\n");
+			return 0;
+		}
+		
+		pmbase &= 0xFFFFFFFE;
+		if (!pmbase) {
+		       printk(KERN_ERR "cpufreq: could not find speedstep register\n");
+		       return 0;
+		}
+		
+		/* Disable IRQs */
+		local_irq_save(flags);
+
+		/* read state */
+		value = inb(pmbase + 0x50);
+
+		dprintk(KERN_DEBUG "cpufreq: read at pmbase 0x%x + 0x50 returned 0x%x\n", pmbase, value);
+
+		/* write new state */
+		value &= 0xFE;
+		
+		dprintk(KERN_DEBUG "cpufreq: writing 0x%x to pmbase 0x%x + 0x50\n", value, pmbase);
+
+		/* Disable bus master arbitration */
+		pm2_blk = inb(pmbase + 0x20);
+		pm2_blk |= 0x01;
+		outb(pm2_blk, (pmbase + 0x20));
+
+		/* Actual transition */
+		outb(value, (pmbase + 0x50));
+		
+		/* Restore bus master arbitration */
+		pm2_blk &= 0xfe;
+		outb(pm2_blk, (pmbase + 0x20));
+
+		/* check if transition was successful */
+		value = inb(pmbase + 0x50);
+
+		/* Enable IRQs */
+		local_irq_restore(flags);
+
+		dprintk(KERN_DEBUG "cpufreq: read at pmbase 0x%x + 0x50 returned 0x%x\n", pmbase, value);
+
+		if (~(value & 0x1)) {
+		        dprintk (KERN_INFO "cpufreq: change to %u kHz speedstep state succeeded\n", (speedstep_get_processor_frequency(speedstep_processor)));
+		} else {
+		        printk (KERN_ERR "cpufreq: change failed - I/O error\n");
+		}
+	}
+	
+	
+	/* clockmod transition */
+	if (dc_trans) {
+	        rdmsr(MSR_IA32_THERM_STATUS, l, h);
+#if 0
+		if (l & 0x01)
+		        dprintk(KERN_DEBUG "cpufreq: CPU#%d currently thermal throttled\n", cpu);
+#endif
+		rdmsr(MSR_IA32_THERM_CONTROL, l, h);
+		if (dc_newstate == 0x08) {
+		        dprintk(KERN_INFO "cpufreq: CPU#%d disabling modulation\n", cpu); 
+		        dprintk(KERN_INFO "cpufreq: CPU#%d writing 0x%x to control reg.\n", cpu, (l | 0x0e)& ~(1<<4)); 
+			wrmsr(MSR_IA32_THERM_CONTROL, (l | 0x0e) & ~(1<<4), h);
+		} else {
+      		        dprintk(KERN_INFO "cpufreq: CPU#%d setting duty cycle to %d%%\n",
+		        cpu, ((125 * dc_newstate) / 10)); 
+	              /* bits 63 - 5	: reserved 
+		       * bit  4	: enable/disable
+		       * bits 3-1	: duty cycle
+		       * bit  0	: reserved
+		       */
+			l = (l & ~14);
+			l = l | (1<<4) | ((dc_newstate & 0x7)<<1);
+		        dprintk(KERN_INFO "cpufreq: CPU#%d writing 0x%x to control reg.\n", cpu, l); 
+			wrmsr(MSR_IA32_THERM_CONTROL, l, h);
+		}
+
+        /* get current clockmod state */
+	rdmsr(MSR_IA32_THERM_CONTROL, l, h);
+	dprintk(KERN_INFO "cpufreq: CPU#%d read 0x%x from control reg.\n", cpu, l); 
+	if (l & 0x10) {
+		l = l >> 1;
+		l &= 0x7;
+	} else
+	        l = 0x08; /* dc disabled */
+	dprintk(KERN_DEBUG "cpufreq: new dc state: 0x%x\n", l);
+
+
+
+	}
+
+	/* do speedstep to lower state transition if necessary*/
+	if ((speedstep_newstate==0x0) && (speedstep_trans || dc_trans)) {
+
+	        /* get PMBASE */
+	        pci_read_config_dword(speedstep_chipset_dev, 0x40, &pmbase);
+		if (!(pmbase & 0x01)) {
+		        printk(KERN_ERR "cpufreq: could not find speedstep register\n");
+			return 0;
+		}
+		
+		pmbase &= 0xFFFFFFFE;
+		if (!pmbase) {
+		       printk(KERN_ERR "cpufreq: could not find speedstep register\n");
+		       return 0;
+		}
+		
+		/* Disable IRQs */
+		local_irq_save(flags);
+
+		/* read state */
+		value = inb(pmbase + 0x50);
+
+		dprintk(KERN_DEBUG "cpufreq: read at pmbase 0x%x + 0x50 returned 0x%x\n", pmbase, value);
+
+		/* write new state */
+		value &= 0xFE;
+		value |= 0x1;
+		
+		dprintk(KERN_DEBUG "cpufreq: writing 0x%x to pmbase 0x%x + 0x50\n", value, pmbase);
+
+		/* Disable bus master arbitration */
+		pm2_blk = inb(pmbase + 0x20);
+		pm2_blk |= 0x01;
+		outb(pm2_blk, (pmbase + 0x20));
+
+		/* Actual transition */
+		outb(value, (pmbase + 0x50));
+		
+		/* Restore bus master arbitration */
+		pm2_blk &= 0xfe;
+		outb(pm2_blk, (pmbase + 0x20));
+
+		/* check if transition was successful */
+		value = inb(pmbase + 0x50);
+
+		/* Enable IRQs */
+		local_irq_restore(flags);
+
+		dprintk(KERN_DEBUG "cpufreq: read at pmbase 0x%x + 0x50 returned 0x%x\n", pmbase, value);
+
+		if (value & 0x1) {
+		        dprintk (KERN_INFO "cpufreq: change to %u kHz speedstep state succeeded\n", (speedstep_get_processor_frequency(speedstep_processor)));
+		} else {
+		        printk (KERN_ERR "cpufreq: change failed - I/O error\n");
+		}
+	}
+
+	set_cpus_allowed(current, cpus_allowed);
+
+	if (notify) {
+	        /* notifiers */
+	        cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+	        if (hyperthreading) {
+		        freqs.cpu = cpu;
+			cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+		}
+		dprintk(KERN_DEBUG "cpufreq: transition on cpu %i from %i kHz to %ikHz completed\n",cpu ,freqs.old ,freqs.new);
+		
+	}
+
+	return 0;
+}
+
 
 /**
  * speedstep_activate - activate SpeedStep control in the chipset
@@ -164,6 +448,7 @@
 
 	pci_read_config_word(speedstep_chipset_dev, 
 			     0x00A0, &value);
+	dprintk(KERN_DEBUG "cpufreq: speedstep registers: 0x%x \n",value);
 	if (!(value & 0x08)) {
 		value |= 0x08;
 		dprintk(KERN_DEBUG "cpufreq: activating SpeedStep (TM) registers\n");
@@ -258,6 +543,20 @@
 	return 0;
 }
 
+static int speedstep_p4_target (struct cpufreq_policy *policy,
+			     unsigned int target_freq,
+			     unsigned int relation)
+{
+	unsigned int	newstate = DC_RESV;
+
+	if (cpufreq_frequency_table_target(policy, &speedstep_p4_freqs[0], target_freq, relation, &newstate))
+		return -EINVAL;
+
+	speedstep_p4_set_state(policy->cpu, speedstep_p4_freqs[newstate].index, 1);
+
+	return 0;
+}
+
 
 /**
  * speedstep_verify - verifies a new CPUFreq policy
@@ -271,6 +570,11 @@
 	return cpufreq_frequency_table_verify(policy, &speedstep_freqs[0]);
 }
 
+static int speedstep_p4_verify (struct cpufreq_policy *policy)
+{
+	return cpufreq_frequency_table_verify(policy, &speedstep_p4_freqs[0]);
+}
+
 
 static int speedstep_cpu_init(struct cpufreq_policy *policy)
 {
@@ -281,10 +585,10 @@
 	if (policy->cpu != 0)
 		return -ENODEV;
 
-	/* detect low and high frequency */
+	/* detect speedstep base frequencies */
 	result = speedstep_get_freqs(speedstep_processor,
-				     &speedstep_freqs[SPEEDSTEP_LOW].frequency,
-				     &speedstep_freqs[SPEEDSTEP_HIGH].frequency,
+				     &speedstep_p4_freqs[LO_DC_DISABLE].frequency,
+				     &speedstep_p4_freqs[HI_DC_DISABLE].frequency,
 				     &speedstep_set_state);
 	if (result)
 		return result;
@@ -312,6 +616,57 @@
 	return 0;
 }
 
+static int speedstep_P4_cpu_init(struct cpufreq_policy *policy)
+{
+	int		result = 0;
+	unsigned int    i,max,min,max_to_min_ratio,idx_lo;
+
+	/* detect low and high frequency */
+	result=speedstep_p4_set_state(policy->cpu, speedstep_p4_freqs[LO_DC_DISABLE].index, 0);
+	stock_freq[0]=speedstep_get_processor_frequency(speedstep_processor);
+	if (result)
+		return result;
+	result=speedstep_p4_set_state(policy->cpu, speedstep_p4_freqs[HI_DC_DISABLE].index, 0);
+	stock_freq[1]=speedstep_get_processor_frequency(speedstep_processor);
+	if (result)
+	        return result;
+
+	speedstep_p4_freqs[LO_DC_DISABLE].frequency=stock_freq[0];
+	speedstep_p4_freqs[HI_DC_DISABLE].frequency=stock_freq[1];
+
+	/* init low and high clockmod tables */
+	for (i=1; i<8; i++) {
+	        speedstep_p4_freqs[i].frequency = (speedstep_p4_freqs[LO_DC_DISABLE].frequency * i)/8;
+	        speedstep_p4_freqs[8+i].frequency = (speedstep_p4_freqs[HI_DC_DISABLE].frequency * i)/8;
+	}
+
+	/* check, if there are frequencies which are possible in both voltage states */
+	max=stock_freq[1];
+	min=stock_freq[0]/8;
+	max_to_min_ratio=max/min;
+	if ((min*max_to_min_ratio)==max) {
+	       dprintk(KERN_INFO "cpufreq: non-unique frequencies may appear\n");
+	       /* if yes, add 1 to higher voltage frequency - ugly but effective */
+	       for (i=1; i<9; i++) {
+		       idx_lo=(max*i)/(8*min);
+		       if ((idx_lo<=8) && (speedstep_p4_freqs[idx_lo].frequency == speedstep_p4_freqs[8+i].frequency)) 
+		               speedstep_p4_freqs[8+i].frequency++;
+	       }
+	}
+
+
+	dprintk(KERN_INFO "cpufreq: detected mobile P4 currently at %i MHz\n", 
+		(stock_freq[1] / 1000));
+
+        cpufreq_frequency_table_get_attr(speedstep_p4_freqs, policy->cpu);
+
+	/* cpuinfo and default policy values */
+	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
+	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL*2+1000000; /* assumed */
+	policy->cur = stock_freq[1];
+
+	return cpufreq_frequency_table_cpuinfo(policy, &speedstep_p4_freqs[0]);
+}
 
 static int speedstep_cpu_exit(struct cpufreq_policy *policy)
 {
@@ -336,6 +691,16 @@
 	.attr		= speedstep_attr,
 };
 
+static struct cpufreq_driver speedstep_P4_driver = {
+	.name		= "speedstep-ich",
+	.verify 	= speedstep_p4_verify,
+	.target 	= speedstep_p4_target,
+	.init		= speedstep_P4_cpu_init,
+	.exit		= speedstep_cpu_exit,    /* same as speedstep only */
+	.owner		= THIS_MODULE,
+	.attr		= speedstep_attr,        /* dummy - use same as speedstep only */
+};
+
 
 /**
  * speedstep_init - initializes the SpeedStep CPUFreq driver
@@ -361,7 +726,10 @@
 	if (speedstep_activate())
 		return -EINVAL;
 
-	return cpufreq_register_driver(&speedstep_driver);
+	if (speedstep_processor==SPEEDSTEP_PROCESSOR_P4M)
+	        return cpufreq_register_driver(&speedstep_P4_driver);
+	else
+	        return cpufreq_register_driver(&speedstep_driver);
 }
 
 
@@ -372,11 +740,14 @@
  */
 static void __exit speedstep_exit(void)
 {
-	cpufreq_unregister_driver(&speedstep_driver);
+	if (speedstep_processor==SPEEDSTEP_PROCESSOR_P4M)
+	        cpufreq_unregister_driver(&speedstep_P4_driver);
+	else
+	        cpufreq_unregister_driver(&speedstep_driver);
 }
 
 
-MODULE_AUTHOR ("Dave Jones <davej@codemonkey.org.uk>, Dominik Brodowski <linux@brodo.de>");
+MODULE_AUTHOR ("Dave Jones <davej@codemonkey.org.uk>, Dominik Brodowski <linux@brodo.de>, Christian Hoelbling <christian.holbling@cern.ch>");
 MODULE_DESCRIPTION ("Speedstep driver for Intel mobile processors on chipsets with ICH-M southbridges.");
 MODULE_LICENSE ("GPL");
 
diff --unified --recursive --new-file linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
--- linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c	2004-06-07 20:49:36.000000000 +0000
+++ linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c	2004-06-07 20:56:00.732209368 +0000
@@ -210,8 +210,17 @@
 		ebx = cpuid_ebx(0x00000001);
 		ebx &= 0x000000FF;
 
-		dprintk(KERN_INFO "ebx value is %x, x86_mask is %x\n", ebx, c->86_mask);
+		dprintk(KERN_INFO "ebx value is %x\n", ebx);
 
+		dprintk(KERN_INFO "model_id is %s\n", c->x86_model_id);
+		
+		/*
+		 * If the x86_model_id string contais "Mobile Intel(R) Pentium(R) 4"
+                 * omit all other checks and treat the CPU as a M-P4-M
+		 */
+		if (strstr(c->x86_model_id,"Mobile Intel(R) Pentium(R) 4") != NULL)
+		       return SPEEDSTEP_PROCESSOR_P4M;
+		
 		switch (c->x86_mask) {
 		case 4: 
 			/*
@@ -248,10 +257,11 @@
 			 * So, how to distinguish all those processors with
 			 * ebx=0xf? I don't know. Sort them out, and wait
 			 * for someone to complain.
+			 * also, M-P4M HTs actually have ebx=0x8, too
 			 */
-			if (ebx == 0x0e)
-				return SPEEDSTEP_PROCESSOR_P4M;
-			break;
+		         if (ebx == 0x0e)
+		                return SPEEDSTEP_PROCESSOR_P4M;
+			 break;
 		default:
 			break;
 		}

[PATCH] 2.6.5 speedstep on P4Ms

[Christian Hoelbling]

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

   hi list,

 i am trying to do a small rewrite of the speedstep-ich driver to 
address the following issues:

1.) detect all P4M's via the model_id string
2.) correctly register drivers on hyperthreading CPU's (and stop 
freeze-ups i occasionally had)
3.) do P4-clockmod on top of speedstep on P4-Ms

the patched driver works on my machine, but there seem to be 2 problems: 
sometimes the clockmod state is not set correctly and the powersave 
governor seems to miss the lowest frequency state (it instead switches 
to the second lowest).

it would be great if someone could test it on there machines and send me 
some reports. also any suggestions/tips are very welcome (since this is 
my first attempt at kernel coding).

best,

Christian Hoelbling

[-- Attachment #2: speedstep_patch.2.6.5 --]
[-- Type: text/x-troff-man, Size: 14754 bytes --]

diff --unified --recursive --new-file linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
--- linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-07 20:49:36.000000000 +0000
+++ linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-08 17:04:39.479565680 +0000
@@ -11,6 +11,9 @@
  *  for extensive testing.
  *
  *  BIG FAT DISCLAIMER: Work in progress code. Possibly *dangerous*
+ *
+ *  Added SMT and p4-clockmod support on P4-M's
+ *  Christian Hoelbling, 2004
  */
 
 
@@ -27,6 +30,10 @@
 
 #include "speedstep-lib.h"
 
+/* invalidate high voltage states slower than fastest
+ * low voltage state
+ */
+#define INVALIDATE_SLOW_HI_STATES
 
 /* speedstep_chipset:
  *   It is necessary to know which chipset is used. As accesses to 
@@ -51,6 +58,44 @@
 	{0,			CPUFREQ_TABLE_END},
 };
 
+/*
+ * Duty Cycle (3bits) from p4-clockmod
+ * for each of the voltages
+ */
+enum {
+        DC_RESV, LO_DC_DFLT, LO_DC_25PT, LO_DC_38PT, LO_DC_50PT,
+	LO_DC_64PT, LO_DC_75PT, LO_DC_88PT, LO_DC_DISABLE,
+        HI_DC_DFLT, HI_DC_25PT, HI_DC_38PT, HI_DC_50PT,
+	HI_DC_64PT, HI_DC_75PT, HI_DC_88PT, HI_DC_DISABLE
+};
+
+/* 
+ * Extended speedstep + p4-clockmod table
+ * for P4-M's
+ * Values are in kHz for the time being.
+ */
+static struct cpufreq_frequency_table speedstep_p4_freqs[] = {
+	{DC_RESV, CPUFREQ_ENTRY_INVALID},
+	{LO_DC_DFLT, 0},
+	{LO_DC_25PT, 0},
+	{LO_DC_38PT, 0},
+	{LO_DC_50PT, 0},
+	{LO_DC_64PT, 0},
+	{LO_DC_75PT, 0},
+	{LO_DC_88PT, 0},
+	{LO_DC_DISABLE, 0},
+	{HI_DC_DFLT, 0},
+	{HI_DC_25PT, 0},
+	{HI_DC_38PT, 0},
+	{HI_DC_50PT, 0},
+	{HI_DC_64PT, 0},
+	{HI_DC_75PT, 0},
+	{HI_DC_88PT, 0},
+	{HI_DC_DISABLE, 0},
+	{DC_RESV, CPUFREQ_TABLE_END},
+};
+
+static unsigned int stock_freq[2];
 
 /* DEBUG
  *   Define it if you want verbose debug output, e.g. for bug reporting
@@ -149,6 +194,190 @@
 }
 
 
+static int speedstep_p4_set_state(unsigned int cpu, unsigned int newstate, unsigned int notify)
+{
+	u32 l, h, pmbase;
+	u8 pm2_blk, value;
+	cpumask_t cpus_allowed, affected_cpu_map;
+	unsigned long flags;
+	struct cpufreq_freqs freqs;
+	int hyperthreading = 0;
+	int sibling = 0;
+	unsigned int dc_newstate,speedstep_newstate,oldstate,dc_oldstate,speedstep_oldstate;
+	unsigned int speedstep_trans,dc_trans;
+
+	if (!cpu_online(cpu) || (newstate > HI_DC_DISABLE) || 
+		(newstate == DC_RESV))
+		return -EINVAL;
+
+	/* disentangle speedstep and clockmod */
+	dc_newstate=((newstate-1) & 0x07)+1;
+	speedstep_newstate=(newstate>LO_DC_DISABLE);
+
+	/* switch to physical CPU where state is to be changed*/
+	cpus_allowed = current->cpus_allowed;
+
+	/* only run on CPU to be set, or on its sibling */
+       affected_cpu_map = cpumask_of_cpu(cpu);
+#ifdef CONFIG_X86_HT
+	hyperthreading = ((cpu_has_ht) && (smp_num_siblings == 2));
+	if (hyperthreading) {
+		sibling = cpu_sibling_map[cpu];
+                cpu_set(sibling, affected_cpu_map);
+	}
+#endif
+	set_cpus_allowed(current, affected_cpu_map);
+        BUG_ON(!cpu_isset(smp_processor_id(), affected_cpu_map));
+
+	/* get current clockmod state */
+	rdmsr(MSR_IA32_THERM_CONTROL, l, h);
+	dprintk(KERN_INFO "cpufreq: CPU#%d read 0x%x from control reg.0x%x\n", cpu, l, h); 
+	if (l & 0x10) {
+		dc_oldstate = l >> 1;
+		dc_oldstate &= 0x7;
+	} else
+	        dc_oldstate = 0x08; /* dc disabled */
+
+	if (dc_oldstate == DC_RESV) {
+	        printk(KERN_ERR "cpufreq: BIG FAT WARNING: currently in invalid setting\n");
+	}
+	dc_trans=(dc_oldstate != dc_newstate);
+
+	/* get currect speedstep state */
+	speedstep_oldstate=(speedstep_get_processor_frequency(speedstep_processor)==speedstep_p4_freqs[HI_DC_DISABLE].frequency);
+	speedstep_trans=(speedstep_oldstate != speedstep_newstate);
+	oldstate  = ((dc_oldstate-1) + (speedstep_oldstate << 3)) + 1;
+
+	if (notify) {
+	        freqs.old = speedstep_p4_freqs[oldstate].frequency; /* relies on indices for all allowed states in frequency table being in sequential order starting from 1 */
+		freqs.cpu = cpu;
+		dprintk(KERN_DEBUG "cpufreq: os: 0x%x ns: 0x%x ssos: 0x%x ssns: 0x%x dcos: 0x%x dcns: 0x%x\n", oldstate,newstate,speedstep_oldstate,speedstep_newstate,dc_oldstate,dc_newstate);
+
+		/* notifiers */
+		freqs.new = speedstep_p4_freqs[newstate].frequency; /* relies on indices for all allowed states in frequency table being in sequential order starting from 1 */
+		dprintk(KERN_DEBUG "cpufreq: preparing transition on cpu %i from %i kHz to %ikHz \n",cpu ,freqs.old ,freqs.new);
+		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+		if (hyperthreading) {
+		        freqs.cpu = sibling;
+			cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+		}
+	}
+
+	/* speedstep state transition */
+	if (speedstep_trans) {
+
+	        /* get PMBASE */
+	        pci_read_config_dword(speedstep_chipset_dev, 0x40, &pmbase);
+		if (!(pmbase & 0x01)) {
+		        printk(KERN_ERR "cpufreq: could not find speedstep register\n");
+			return 0;
+		}
+		
+		pmbase &= 0xFFFFFFFE;
+		if (!pmbase) {
+		       printk(KERN_ERR "cpufreq: could not find speedstep register\n");
+		       return 0;
+		}
+		
+		/* Disable IRQs */
+		local_irq_save(flags);
+
+		/* read state */
+		value = inb(pmbase + 0x50);
+
+		dprintk(KERN_DEBUG "cpufreq: read at pmbase 0x%x + 0x50 returned 0x%x\n", pmbase, value);
+
+		/* write new state */
+		value &= 0xFE;
+		value |= 1-speedstep_newstate;
+		
+		dprintk(KERN_DEBUG "cpufreq: writing 0x%x to pmbase 0x%x + 0x50\n", value, pmbase);
+
+		/* Disable bus master arbitration */
+		pm2_blk = inb(pmbase + 0x20);
+		pm2_blk |= 0x01;
+		outb(pm2_blk, (pmbase + 0x20));
+
+		/* Actual transition */
+		outb(value, (pmbase + 0x50));
+		
+		/* Restore bus master arbitration */
+		pm2_blk &= 0xfe;
+		outb(pm2_blk, (pmbase + 0x20));
+
+		/* check if transition was successful */
+		value = inb(pmbase + 0x50);
+
+		/* Enable IRQs */
+		local_irq_restore(flags);
+
+		dprintk(KERN_DEBUG "cpufreq: read at pmbase 0x%x + 0x50 returned 0x%x\n", pmbase, value);
+
+		if (speedstep_newstate != (value & 0x1)) {
+		        dprintk (KERN_INFO "cpufreq: change to %u kHz speedstep state succeeded\n", (speedstep_get_processor_frequency(speedstep_processor)));
+		} else {
+		        printk (KERN_ERR "cpufreq: change failed - I/O error\n");
+		}
+	}
+
+	/* clockmod transition */
+	if (dc_trans) {
+
+	        rdmsr(MSR_IA32_THERM_STATUS, l, h);
+#if 0
+		if (l & 0x01)
+		        dprintk(KERN_DEBUG "cpufreq: CPU#%d currently thermal throttled\n", cpu);
+#endif
+		rdmsr(MSR_IA32_THERM_CONTROL, l, h);
+		if (dc_newstate == 0x08) {
+		        dprintk(KERN_INFO "cpufreq: CPU#%d disabling modulation\n", cpu); 
+		        dprintk(KERN_INFO "cpufreq: CPU#%d writing 0x%x to control reg.\n", cpu, (l | 0x0e)& ~(1<<4)); 
+			wrmsr(MSR_IA32_THERM_CONTROL, (l | 0x0e) & ~(1<<4), h);
+		} else {
+      		        dprintk(KERN_INFO "cpufreq: CPU#%d setting duty cycle to %d%%\n",
+		        cpu, ((125 * dc_newstate) / 10)); 
+	              /* bits 63 - 5	: reserved 
+		       * bit  4	: enable/disable
+		       * bits 3-1	: duty cycle
+		       * bit  0	: reserved
+		       */
+			l = (l & ~14);
+			l = l | (1<<4) | ((dc_newstate & 0x7)<<1);
+		        dprintk(KERN_INFO "cpufreq: CPU#%d writing 0x%x to control reg.0x%x\n", cpu, l, h); 
+			wrmsr(MSR_IA32_THERM_CONTROL, l, h);
+		}
+
+#ifdef SPEEDSTEP_DEBUG
+		/* get current clockmod state */
+		rdmsr(MSR_IA32_THERM_CONTROL, l, h);
+		dprintk(KERN_INFO "cpufreq: CPU#%d read 0x%x from control reg.0x%x\n", cpu, l, h); 
+		if (l & 0x10) {
+		        l = l >> 1;
+			l &= 0x7;
+		} else
+		        l = 0x08; /* dc disabled */
+		dprintk(KERN_DEBUG "cpufreq: new dc state: 0x%x\n", l);
+#endif
+
+	}
+
+	set_cpus_allowed(current, cpus_allowed);
+
+	if (notify) {
+	        /* notifiers */
+	        cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+	        if (hyperthreading) {
+		        freqs.cpu = cpu;
+			cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+		}
+		dprintk(KERN_DEBUG "cpufreq: transition on cpu %i from %i kHz to %ikHz completed\n",cpu ,freqs.old ,freqs.new);
+		
+	}
+
+	return 0;
+}
+
+
 /**
  * speedstep_activate - activate SpeedStep control in the chipset
  *
@@ -164,6 +393,7 @@
 
 	pci_read_config_word(speedstep_chipset_dev, 
 			     0x00A0, &value);
+	dprintk(KERN_DEBUG "cpufreq: speedstep registers: 0x%x \n",value);
 	if (!(value & 0x08)) {
 		value |= 0x08;
 		dprintk(KERN_DEBUG "cpufreq: activating SpeedStep (TM) registers\n");
@@ -259,6 +489,21 @@
 }
 
 
+static int speedstep_p4_target (struct cpufreq_policy *policy,
+			     unsigned int target_freq,
+			     unsigned int relation)
+{
+	unsigned int	newstate = DC_RESV;
+
+	if (cpufreq_frequency_table_target(policy, &speedstep_p4_freqs[0], target_freq, relation, &newstate))
+		return -EINVAL;
+
+	speedstep_p4_set_state(policy->cpu, speedstep_p4_freqs[newstate].index, 1);
+
+	return 0;
+}
+
+
 /**
  * speedstep_verify - verifies a new CPUFreq policy
  * @freq: new policy
@@ -272,6 +517,12 @@
 }
 
 
+static int speedstep_p4_verify (struct cpufreq_policy *policy)
+{
+	return cpufreq_frequency_table_verify(policy, &speedstep_p4_freqs[0]);
+}
+
+
 static int speedstep_cpu_init(struct cpufreq_policy *policy)
 {
 	int		result = 0;
@@ -312,6 +563,67 @@
 	return 0;
 }
 
+static int speedstep_P4_cpu_init(struct cpufreq_policy *policy)
+{
+	int		result = 0;
+	unsigned int    i,max,min,max_to_min_ratio,idx_lo;
+
+	/* detect low and high frequency */
+	result=speedstep_p4_set_state(policy->cpu, speedstep_p4_freqs[LO_DC_DISABLE].index, 0);
+	stock_freq[0]=speedstep_get_processor_frequency(speedstep_processor);
+	if (result)
+		return result;
+	result=speedstep_p4_set_state(policy->cpu, speedstep_p4_freqs[HI_DC_DISABLE].index, 0);
+	stock_freq[1]=speedstep_get_processor_frequency(speedstep_processor);
+	if (result)
+	        return result;
+
+	speedstep_p4_freqs[LO_DC_DISABLE].frequency=stock_freq[0];
+	speedstep_p4_freqs[HI_DC_DISABLE].frequency=stock_freq[1];
+
+	/* init low and high clockmod tables */
+	for (i=1; i<8; i++) {
+	        speedstep_p4_freqs[i].frequency = (speedstep_p4_freqs[LO_DC_DISABLE].frequency * i)/8;
+	        speedstep_p4_freqs[8+i].frequency = (speedstep_p4_freqs[HI_DC_DISABLE].frequency * i)/8;
+	}
+
+	/* check, if there are frequencies which are possible in both voltage states */
+	max=stock_freq[1];
+	min=stock_freq[0]/8;
+	max_to_min_ratio=max/min;
+	if ((min*max_to_min_ratio)==max) {
+	       dprintk(KERN_INFO "cpufreq: non-unique frequencies may appear\n");
+	       /* if yes, subtract 1 from higher voltage frequency - ugly but effective */
+	       for (i=1; i<9; i++) {
+		       idx_lo=(max*i)/(8*min);
+		       if ((idx_lo<=8) && (speedstep_p4_freqs[idx_lo].frequency == speedstep_p4_freqs[8+i].frequency)) 
+		               speedstep_p4_freqs[8+i].frequency--;
+	       }
+	}
+
+#ifdef INVALIDATE_SLOW_HI_STATES
+
+	/*  invalidate high voltage states slower than fastest low voltage state */
+	for (i=9; (speedstep_p4_freqs[i].frequency<stock_freq[0]) && (speedstep_p4_freqs[i].frequency!=CPUFREQ_TABLE_END) ; i++) {
+	        dprintk(KERN_INFO "cpufreq: invalidating mode %i with %i kHz\n", 
+			i,speedstep_p4_freqs[i].frequency);	        
+		speedstep_p4_freqs[i].frequency=CPUFREQ_ENTRY_INVALID;
+	}
+
+#endif
+
+	dprintk(KERN_INFO "cpufreq: detected mobile P4 currently at %i MHz\n", 
+		(stock_freq[1] / 1000));
+
+        cpufreq_frequency_table_get_attr(speedstep_p4_freqs, policy->cpu);
+
+	/* cpuinfo and default policy values */
+	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
+	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL+1000000; /* assumed */
+	policy->cur = stock_freq[1];
+
+	return cpufreq_frequency_table_cpuinfo(policy, &speedstep_p4_freqs[0]);
+}
 
 static int speedstep_cpu_exit(struct cpufreq_policy *policy)
 {
@@ -336,6 +648,16 @@
 	.attr		= speedstep_attr,
 };
 
+static struct cpufreq_driver speedstep_P4_driver = {
+	.name		= "speedstep-ich",
+	.verify 	= speedstep_p4_verify,
+	.target 	= speedstep_p4_target,
+	.init		= speedstep_P4_cpu_init,
+	.exit		= speedstep_cpu_exit,    /* same as speedstep only */
+	.owner		= THIS_MODULE,
+	.attr		= speedstep_attr,        /* dummy - use same as speedstep only */
+};
+
 
 /**
  * speedstep_init - initializes the SpeedStep CPUFreq driver
@@ -359,11 +681,14 @@
 
 	/* activate speedstep support */
 	if (speedstep_activate())
-		return -EINVAL;
+	        return -EINVAL;
 
-	return cpufreq_register_driver(&speedstep_driver);
-}
+	if (speedstep_processor==SPEEDSTEP_PROCESSOR_P4M)
+	        return cpufreq_register_driver(&speedstep_P4_driver);
+	else
+	        return cpufreq_register_driver(&speedstep_driver);
 
+}
 
 /**
  * speedstep_exit - unregisters SpeedStep support
@@ -372,11 +697,14 @@
  */
 static void __exit speedstep_exit(void)
 {
-	cpufreq_unregister_driver(&speedstep_driver);
+	if (speedstep_processor==SPEEDSTEP_PROCESSOR_P4M)
+	        cpufreq_unregister_driver(&speedstep_P4_driver);
+	else
+	        cpufreq_unregister_driver(&speedstep_driver);
 }
 
 
-MODULE_AUTHOR ("Dave Jones <davej@codemonkey.org.uk>, Dominik Brodowski <linux@brodo.de>");
+MODULE_AUTHOR ("Dave Jones <davej@codemonkey.org.uk>, Dominik Brodowski <linux@brodo.de>, Christian Hoelbling <christian.holbling@cern.ch>");
 MODULE_DESCRIPTION ("Speedstep driver for Intel mobile processors on chipsets with ICH-M southbridges.");
 MODULE_LICENSE ("GPL");
 
diff --unified --recursive --new-file linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
--- linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c	2004-06-07 20:49:36.000000000 +0000
+++ linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c	2004-06-07 20:56:00.000000000 +0000
@@ -210,8 +210,17 @@
 		ebx = cpuid_ebx(0x00000001);
 		ebx &= 0x000000FF;
 
-		dprintk(KERN_INFO "ebx value is %x, x86_mask is %x\n", ebx, c->86_mask);
+		dprintk(KERN_INFO "ebx value is %x\n", ebx);
 
+		dprintk(KERN_INFO "model_id is %s\n", c->x86_model_id);
+		
+		/*
+		 * If the x86_model_id string contais "Mobile Intel(R) Pentium(R) 4"
+                 * omit all other checks and treat the CPU as a M-P4-M
+		 */
+		if (strstr(c->x86_model_id,"Mobile Intel(R) Pentium(R) 4") != NULL)
+		       return SPEEDSTEP_PROCESSOR_P4M;
+		
 		switch (c->x86_mask) {
 		case 4: 
 			/*
@@ -248,10 +257,11 @@
 			 * So, how to distinguish all those processors with
 			 * ebx=0xf? I don't know. Sort them out, and wait
 			 * for someone to complain.
+			 * also, M-P4M HTs actually have ebx=0x8, too
 			 */
-			if (ebx == 0x0e)
-				return SPEEDSTEP_PROCESSOR_P4M;
-			break;
+		         if (ebx == 0x0e)
+		                return SPEEDSTEP_PROCESSOR_P4M;
+			 break;
 		default:
 			break;
 		}

[-- Attachment #3: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@www.linux.org.uk
http://www.linux.org.uk/mailman/listinfo/cpufreq

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Bruno Ducrot]

On Tue, Jun 08, 2004 at 05:15:25PM +0000, Christian Hoelbling wrote:
>   hi list,
> 
> i am trying to do a small rewrite of the speedstep-ich driver to 
> address the following issues:
> 
> 1.) detect all P4M's via the model_id string
> 2.) correctly register drivers on hyperthreading CPU's (and stop 
> freeze-ups i occasionally had)
> 3.) do P4-clockmod on top of speedstep on P4-Ms

It would be great if you send 3 differents patches...

Also, I don't think 3) is how thinks should be done.
It would be much more usefull to allow 2 cpufreq drivers to be loaded.
I think it's planed for 2.7 btw.

Cheers,

-- 
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Christian Hoelbling]

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

Bruno Ducrot wrote:

>On Tue, Jun 08, 2004 at 05:15:25PM +0000, Christian Hoelbling wrote:
>  
>
>>  hi list,
>>
>>i am trying to do a small rewrite of the speedstep-ich driver to 
>>address the following issues:
>>
>>1.) detect all P4M's via the model_id string
>>2.) correctly register drivers on hyperthreading CPU's (and stop 
>>freeze-ups i occasionally had)
>>3.) do P4-clockmod on top of speedstep on P4-Ms
>>    
>>
>
>It would be great if you send 3 differents patches...
>
>Also, I don't think 3) is how thinks should be done.
>It would be much more usefull to allow 2 cpufreq drivers to be loaded.
>I think it's planed for 2.7 btw.
>
>Cheers,
>
>  
>
thanks alot for your help. here are the patches for 1) and 2) seperately.

for 3): what you say sounds much better than my brutal hack, so i won't 
post it again. i'd definitely want to try to implement 2 cpufreq 
drivers, but i fear this is way beyond me at the moment and probably 
involves design choices i can't make.

cheers,

chris

[-- Attachment #2: speedstep_patch.2.6.5_detectP4M.diff --]
[-- Type: text/x-patch, Size: 1229 bytes --]

diff --unified --recursive --new-file linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
--- linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c	2004-06-07 20:49:36.000000000 +0000
+++ linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c	2004-06-08 22:56:06.911397232 +0000
@@ -210,8 +210,17 @@
 		ebx = cpuid_ebx(0x00000001);
 		ebx &= 0x000000FF;
 
-		dprintk(KERN_INFO "ebx value is %x, x86_mask is %x\n", ebx, c->86_mask);
+		dprintk(KERN_INFO "ebx value is %x\n", ebx);
 
+		dprintk(KERN_INFO "model_id is %s\n", c->x86_model_id);
+		
+		/*
+		 * If the x86_model_id string contais "Mobile Intel(R) Pentium(R) 4"
+                 * omit all other checks and treat the CPU as a M-P4-M
+		 */
+		if (strstr(c->x86_model_id,"Mobile Intel(R) Pentium(R) 4") != NULL)
+		       return SPEEDSTEP_PROCESSOR_P4M;
+		
 		switch (c->x86_mask) {
 		case 4: 
 			/*
@@ -248,6 +257,7 @@
 			 * So, how to distinguish all those processors with
 			 * ebx=0xf? I don't know. Sort them out, and wait
 			 * for someone to complain.
+			 * also, M-P4M HTs actually have ebx=0x8, too
 			 */
 			if (ebx == 0x0e)
 				return SPEEDSTEP_PROCESSOR_P4M;

[-- Attachment #3: speedstep_patch.2.6.5_SMT.diff --]
[-- Type: text/x-patch, Size: 6766 bytes --]

diff --unified --recursive --new-file linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
--- linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-07 20:49:36.000000000 +0000
+++ linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-08 23:34:00.871702720 +0000
@@ -70,23 +70,52 @@
  *
  *   Tries to change the SpeedStep state. 
  */
-static void speedstep_set_state (unsigned int state, unsigned int notify)
+static void speedstep_set_state (unsigned int cpu, unsigned int state, unsigned int notify)
 {
 	u32			pmbase;
 	u8			pm2_blk;
 	u8			value;
 	unsigned long		flags;
 	struct cpufreq_freqs	freqs;
+	cpumask_t               cpus_allowed, affected_cpu_map;
+	int                     hyperthreading = 0;
+	int                     sibling = 0;
 
 	if (!speedstep_chipset_dev || (state > 0x1))
 		return;
 
+	/* switch to physical CPU where state is to be changed*/
+	cpus_allowed = current->cpus_allowed;
+
+	/* only run on CPU to be set, or on its sibling */
+       affected_cpu_map = cpumask_of_cpu(cpu);
+#ifdef CONFIG_X86_HT
+	hyperthreading = ((cpu_has_ht) && (smp_num_siblings == 2));
+	if (hyperthreading) {
+		sibling = cpu_sibling_map[cpu];
+                cpu_set(sibling, affected_cpu_map);
+	}
+#endif
+	set_cpus_allowed(current, affected_cpu_map);
+        BUG_ON(!cpu_isset(smp_processor_id(), affected_cpu_map));
+
 	freqs.old = speedstep_get_processor_frequency(speedstep_processor);
 	freqs.new = speedstep_freqs[state].frequency;
-	freqs.cpu = 0; /* speedstep.c is UP only driver */
-	
-	if (notify)
+	freqs.cpu = cpu;
+
+	/* no transition necessary */
+	if (freqs.old == freqs.new) {
+	        set_cpus_allowed(current, cpus_allowed);
+		return;
+	}
+
+	if (notify) {
 		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+		if (hyperthreading) {
+		        freqs.cpu = sibling;
+			cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+		}
+	}
 
 	/* get PMBASE */
 	pci_read_config_dword(speedstep_chipset_dev, 0x40, &pmbase);
@@ -142,9 +171,18 @@
 		printk (KERN_ERR "cpufreq: change failed - I/O error\n");
 	}
 
-	if (notify)
-		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+	/* allow to be run on all CPUs */
+	set_cpus_allowed(current, cpus_allowed);
 
+	if (notify) {
+	        /* notifiers */
+	        cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+	        if (hyperthreading) {
+		        freqs.cpu = cpu;
+			cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+		}
+		dprintk(KERN_DEBUG "cpufreq: transition on cpu %i from %i kHz to %ikHz completed\n",cpu ,freqs.old ,freqs.new);
+	}
 	return;
 }
 
@@ -253,7 +291,7 @@
 	if (cpufreq_frequency_table_target(policy, &speedstep_freqs[0], target_freq, relation, &newstate))
 		return -EINVAL;
 
-	speedstep_set_state(newstate, 1);
+	speedstep_set_state(policy->cpu, newstate, 1);
 
 	return 0;
 }
@@ -277,12 +315,8 @@
 	int		result = 0;
 	unsigned int	speed;
 
-	/* capability check */
-	if (policy->cpu != 0)
-		return -ENODEV;
-
 	/* detect low and high frequency */
-	result = speedstep_get_freqs(speedstep_processor,
+	result = speedstep_get_freqs(policy->cpu,speedstep_processor,
 				     &speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				     &speedstep_freqs[SPEEDSTEP_HIGH].frequency,
 				     &speedstep_set_state);
diff --unified --recursive --new-file linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
--- linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c	2004-06-07 20:49:36.000000000 +0000
+++ linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c	2004-06-08 23:34:50.120215808 +0000
@@ -310,20 +310,40 @@
  *                     DETECT SPEEDSTEP SPEEDS                       *
  *********************************************************************/
 
-unsigned int speedstep_get_freqs(unsigned int processor,
+unsigned int speedstep_get_freqs(unsigned int cpu,
+				  unsigned int processor,
 				  unsigned int *low_speed,
 				  unsigned int *high_speed,
-				  void (*set_state) (unsigned int state,
+				  void (*set_state) (unsigned int cpu,
+						     unsigned int state,
 						     unsigned int notify)
 				 )
 {
 	unsigned int prev_speed;
 	unsigned int ret = 0;
 	unsigned long flags;
+	cpumask_t cpus_allowed, affected_cpu_map;
+	int hyperthreading = 0;
+	int sibling = 0;
 
 	if ((!processor) || (!low_speed) || (!high_speed) || (!set_state))
 		return -EINVAL;
 
+	/* switch to physical CPU where state is to be changed*/
+	cpus_allowed = current->cpus_allowed;
+
+	/* only run on CPU to be set, or on its sibling */
+       affected_cpu_map = cpumask_of_cpu(cpu);
+#ifdef CONFIG_X86_HT
+	hyperthreading = ((cpu_has_ht) && (smp_num_siblings == 2));
+	if (hyperthreading) {
+		sibling = cpu_sibling_map[cpu];
+                cpu_set(sibling, affected_cpu_map);
+	}
+#endif
+	set_cpus_allowed(current, affected_cpu_map);
+        BUG_ON(!cpu_isset(smp_processor_id(), affected_cpu_map));
+
 	/* get current speed */
 	prev_speed = speedstep_get_processor_frequency(processor);
 	if (!prev_speed)
@@ -332,7 +352,7 @@
 	local_irq_save(flags);
 
 	/* switch to low state */
-	set_state(SPEEDSTEP_LOW, 0);
+	set_state(cpu, SPEEDSTEP_LOW, 0);
 	*low_speed = speedstep_get_processor_frequency(processor);
 	if (!*low_speed) {
 		ret = -EIO;
@@ -340,7 +360,7 @@
 	}
 
 	/* switch to high state */
-	set_state(SPEEDSTEP_HIGH, 0);
+	set_state(cpu, SPEEDSTEP_HIGH, 0);
 	*high_speed = speedstep_get_processor_frequency(processor);
 	if (!*high_speed) {
 		ret = -EIO;
@@ -354,10 +374,11 @@
 
 	/* switch to previous state, if necessary */
 	if (*high_speed != prev_speed)
-		set_state(SPEEDSTEP_LOW, 0);
+		set_state(cpu, SPEEDSTEP_LOW, 0);
 
  out:
 	local_irq_restore(flags);
+	set_cpus_allowed(current, cpus_allowed);
 	return (ret);
 }
 EXPORT_SYMBOL_GPL(speedstep_get_freqs);
diff --unified --recursive --new-file linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h
--- linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h	2004-06-07 20:49:36.000000000 +0000
+++ linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h	2004-06-08 23:34:06.104907152 +0000
@@ -41,7 +41,7 @@
  * SPEEDSTEP_LOW; the second argument is zero so that no 
  * cpufreq_notify_transition calls are initiated.
  */
-extern unsigned int speedstep_get_freqs(unsigned int processor,
+extern unsigned int speedstep_get_freqs(unsigned int cpu, unsigned int processor,
 	  unsigned int *low_speed,
 	  unsigned int *high_speed,
-	  void (*set_state) (unsigned int state, unsigned int notify));
+	  void (*set_state) (unsigned int cpu, unsigned int state, unsigned int notify));

[-- Attachment #4: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@www.linux.org.uk
http://www.linux.org.uk/mailman/listinfo/cpufreq

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Dominik Brodowski]

[-- Attachment #1.1: Type: text/plain, Size: 5238 bytes --]

Hi Chris,

> for 3): what you say sounds much better than my brutal hack, so i won't 
> post it again. i'd definitely want to try to implement 2 cpufreq 
> drivers, but i fear this is way beyond me at the moment and probably 
> involves design choices i can't make.

well, somebody _does_ have to do design choices... However, I'm not yet
convinced of the necessity of supporting throttling and frequency scaling on
the same system. Even more, under normal circumstances [i.e. idling works,
which it does on most systems], I see no _technical_ reason to do
throttling, or even non-temperature related _dynamic throttling_ like other
OSes do it. For those new to this list, there's some maths in the archives
which proves that you need more energy == battery power if you do
throttling for any given task... So it's something we need to discuss 
during 2.7.



Now, about your patch 1):

> diff --unified --recursive --new-file linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
> --- linux-2.6.5/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c	2004-06-07 20:49:36.000000000 +0000
> +++ linux-2.6.5.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c	2004-06-08 22:56:06.911397232 +0000
> @@ -210,8 +210,17 @@
>  		ebx = cpuid_ebx(0x00000001);
>  		ebx &= 0x000000FF;
>  
> -		dprintk(KERN_INFO "ebx value is %x, x86_mask is %x\n", ebx, c->86_mask);
> +		dprintk(KERN_INFO "ebx value is %x\n", ebx);
>  
> +		dprintk(KERN_INFO "model_id is %s\n", c->x86_model_id);
> +		
> +		/*
> +		 * If the x86_model_id string contais "Mobile Intel(R) Pentium(R) 4"
> +                 * omit all other checks and treat the CPU as a M-P4-M
> +		 */
> +		if (strstr(c->x86_model_id,"Mobile Intel(R) Pentium(R) 4") != NULL)
> +		       return SPEEDSTEP_PROCESSOR_P4M;
> +		

I'm not sure I like this approach: Intel is too relaxed with regard to their
branding of mobile intel pentiums. There are pentium 3-m, mobile pentium
3-m, pentium 4-m, mobile pentium 4, and  so on. And some -- which even have
"mobile" in their name" -- do not support SpeedStep. THe CPUID [and some
msr's for p3s] have proven to be more accurate in detecting speedstep
capabilities. So I'd like to keep it that way, especially as the CPUID tends
to change if features are changed; but the model_id string may stay the same
for (future) processors which do no longer do speedstep in the way it is
currently implemented in speedstep-ich.


>  		switch (c->x86_mask) {
>  		case 4: 
>  			/*
> @@ -248,6 +257,7 @@
>  			 * So, how to distinguish all those processors with
>  			 * ebx=0xf? I don't know. Sort them out, and wait
>  			 * for someone to complain.
> +			 * also, M-P4M HTs actually have ebx=0x8, too
>  			 */
>  			if (ebx == 0x0e)
>  				return SPEEDSTEP_PROCESSOR_P4M;

So I'd prefer, due to the nastiness of the CPUID in the specific 0x0F29 case,
to move the strstr check to this specific case -- e.g. have

			if (ebx == 0x0e)
				return SPEEDSTEP_PROCESSOR_P4M;
			if (strstr(c->x86_model_id,"Mobile Intel(R) Pentium(R) 4") != NULL)
				return SPEEDSTEP_PROCESSOR_P4M;


Could you update your patch accordingly, please?


Now, about your patch 2) [slightly re-ordered]:

> -unsigned int speedstep_get_freqs(unsigned int processor,
> +unsigned int speedstep_get_freqs(unsigned int cpu,
> +				  unsigned int processor,
>  				  unsigned int *low_speed,
>  				  unsigned int *high_speed,
> -				  void (*set_state) (unsigned int state,
> +				  void (*set_state) (unsigned int cpu,
> +						     unsigned int state,
>  						     unsigned int notify)

This will break speedstep-smi, as it uses speedstep_get_freqs too...

> +	/* switch to physical CPU where state is to be changed*/
> +	cpus_allowed = current->cpus_allowed;
> +
> +	/* only run on CPU to be set, or on its sibling */
> +       affected_cpu_map = cpumask_of_cpu(cpu);
> +#ifdef CONFIG_X86_HT
> +	hyperthreading = ((cpu_has_ht) && (smp_num_siblings == 2));
> +	if (hyperthreading) {
> +		sibling = cpu_sibling_map[cpu];
> +                cpu_set(sibling, affected_cpu_map);
> +	}
> +#endif
> +	set_cpus_allowed(current, affected_cpu_map);
> +        BUG_ON(!cpu_isset(smp_processor_id(), affected_cpu_map));

Due to changes to cpu_sibling_map [it's a cpumask_t now], this won't
generate appropriate code [if it even compiles] on 2.6.7-rc3. See in
p4-clockmod.c how "easy" it becomes now.

> +       affected_cpu_map = cpumask_of_cpu(cpu);
> +#ifdef CONFIG_X86_HT
> +	hyperthreading = ((cpu_has_ht) && (smp_num_siblings == 2));
> +	if (hyperthreading) {
> +		sibling = cpu_sibling_map[cpu];
> +                cpu_set(sibling, affected_cpu_map);
> +	}
> +#endif
> +	set_cpus_allowed(current, affected_cpu_map);
> +        BUG_ON(!cpu_isset(smp_processor_id(), affected_cpu_map));

Same here.

> -	/* capability check */
> -	if (policy->cpu != 0)
> -		return -ENODEV;

For the time being, this should be kept. Else you can load two different
governors, or set two different frequencies, on both siblings. Obviously,
it'll break. I'll try to fix it in the 2.6.8 timeframe...


Thanks,
	Dominik

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@www.linux.org.uk
http://www.linux.org.uk/mailman/listinfo/cpufreq

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Bruno Ducrot]

On Wed, Jun 09, 2004 at 05:47:10PM +0200, Dominik Brodowski wrote:
> Hi Chris,
> 
> > for 3): what you say sounds much better than my brutal hack, so i won't 
> > post it again. i'd definitely want to try to implement 2 cpufreq 
> > drivers, but i fear this is way beyond me at the moment and probably 
> > involves design choices i can't make.
> 
> well, somebody _does_ have to do design choices... However, I'm not yet
> convinced of the necessity of supporting throttling and frequency scaling on
> the same system. Even more, under normal circumstances [i.e. idling works,
> which it does on most systems], I see no _technical_ reason to do
> throttling, or even non-temperature related _dynamic throttling_ like other
> OSes do it. For those new to this list, there's some maths in the archives
> which proves that you need more energy == battery power if you do
> throttling for any given task... So it's something we need to discuss 
> during 2.7.

In fact, I was more thinking about the FSB stuff which generate some noise
recently I believe (and that have to be done during 2.7), though
I'm pretty sure changing FSB is more helpfull for overclocker.

-- 
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Dave Jones]

On Wed, 2004-06-09 at 16:47, Dominik Brodowski wrote:
> Hi Chris,
> 
> > for 3): what you say sounds much better than my brutal hack, so i won't 
> > post it again. i'd definitely want to try to implement 2 cpufreq 
> > drivers, but i fear this is way beyond me at the moment and probably 
> > involves design choices i can't make.
> 
> well, somebody _does_ have to do design choices... However, I'm not yet
> convinced of the necessity of supporting throttling and frequency scaling on
> the same system. Even more, under normal circumstances [i.e. idling works,
> which it does on most systems], I see no _technical_ reason to do
> throttling, or even non-temperature related _dynamic throttling_ like other
> OSes do it. For those new to this list, there's some maths in the archives
> which proves that you need more energy == battery power if you do
> throttling for any given task... So it's something we need to discuss 
> during 2.7.

I'm actually inclined to kill off p4-clockmod one day.
It doesn't seem to do anything useful whatsoever.
It certainly doesn't lower the temperature on my xeon.

	Dave

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Dominik Brodowski]

On Wed, Jun 09, 2004 at 05:29:00PM +0100, Dave Jones wrote:
> On Wed, 2004-06-09 at 16:47, Dominik Brodowski wrote:
> > Hi Chris,
> > 
> > > for 3): what you say sounds much better than my brutal hack, so i won't 
> > > post it again. i'd definitely want to try to implement 2 cpufreq 
> > > drivers, but i fear this is way beyond me at the moment and probably 
> > > involves design choices i can't make.
> > 
> > well, somebody _does_ have to do design choices... However, I'm not yet
> > convinced of the necessity of supporting throttling and frequency scaling on
> > the same system. Even more, under normal circumstances [i.e. idling works,
> > which it does on most systems], I see no _technical_ reason to do
> > throttling, or even non-temperature related _dynamic throttling_ like other
> > OSes do it. For those new to this list, there's some maths in the archives
> > which proves that you need more energy == battery power if you do
> > throttling for any given task... So it's something we need to discuss 
> > during 2.7.
> 
> I'm actually inclined to kill off p4-clockmod one day.
> It doesn't seem to do anything useful whatsoever.
> It certainly doesn't lower the temperature on my xeon.

No surprise on that. It only helps if a) idling is broken, or b) you want to
limit the CPU power to avoid overheating. So, I won't oppose removing
p4-clockmod, even though I see some _very_ _limitied_ usefulness of it...

	Dominik

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Mattia Dongili]

On Wed, Jun 09, 2004 at 06:53:04PM +0200, Dominik Brodowski wrote:
> On Wed, Jun 09, 2004 at 05:29:00PM +0100, Dave Jones wrote:
> > On Wed, 2004-06-09 at 16:47, Dominik Brodowski wrote:
[...]
> > I'm actually inclined to kill off p4-clockmod one day.
> > It doesn't seem to do anything useful whatsoever.
> > It certainly doesn't lower the temperature on my xeon.
> 
> No surprise on that. It only helps if a) idling is broken, or b) you want to
> limit the CPU power to avoid overheating. So, I won't oppose removing
> p4-clockmod, even though I see some _very_ _limitied_ usefulness of it...

Please don't remove it!
I use it for (b). It seems my BIOS (or me?) is not too smart and
sometimes fans run too slow too cool down the processor:

Jun  5 16:53:16 inferi-2 kernel: CPU#0: Temperature above threshold
Jun  5 16:53:16 inferi-2 kernel: CPU#0: Running in modulated clock mode
Jun  5 16:53:16 inferi-2 kernel: CPU#0: Temperature/speed normal
Jun  5 16:53:18 inferi-2 kernel: CPU#0: Temperature above threshold
Jun  5 16:53:18 inferi-2 kernel: CPU#0: Running in modulated clock mode
...

the only solution is to be to decrease processor speed through cpufreq.
Maybe my workaround hides some misconfiguration but it works for now.

-- 
mattia
:wq!

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Christian Hoelbling]

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

hi dominik,

thanks alot for your detailed reply and patience with a newbie. i have 
attached 2 patches w.r.t. 2.6.7-rc3 that should adress your suggestions.

i also wonder about one thing, maybe you can help me on that. with the 
old speedstep driver, i would have occational hard freezups of my 
machine. this seems to be gone (just experimental evidence, since i 
didn't have one in a few days now where before they occured about 
hourly). so my guess is that something in this patch must have corrected 
it. since my machine just has one physical cpu, i really don't 
understand what went wrong before.  do you have any idea on that?

Dominik Brodowski wrote:

>Hi Chris,
>
>  
>
<snip>

> Even more, under normal circumstances [i.e. idling works,
>which it does on most systems], I see no _technical_ reason to do
>throttling, or even non-temperature related _dynamic throttling_ like other
>OSes do it. For those new to this list, there's some maths in the archives
>which proves that you need more energy == battery power if you do
>throttling for any given task... So it's something we need to discuss 
>during 2.7.
>  
>
 thanks, i didn't know that

>Now, about your patch 1):
>  
>
<snip>

>So I'd prefer, due to the nastiness of the CPUID in the specific 0x0F29 case,
>to move the strstr check to this specific case -- e.g. have
>
>			if (ebx == 0x0e)
>				return SPEEDSTEP_PROCESSOR_P4M;
>			if (strstr(c->x86_model_id,"Mobile Intel(R) Pentium(R) 4") != NULL)
>				return SPEEDSTEP_PROCESSOR_P4M;
>  
>
 > Could you update your patch accordingly, please?
 
thanks, done it. i was somehow anticipating that since reading the 
version string feels like a cheat.

>Now, about your patch 2) [slightly re-ordered]:
>
>  
>
>>-unsigned int speedstep_get_freqs(unsigned int processor,
>>+unsigned int speedstep_get_freqs(unsigned int cpu,
>>+				  unsigned int processor,
>> 				  unsigned int *low_speed,
>> 				  unsigned int *high_speed,
>>-				  void (*set_state) (unsigned int state,
>>+				  void (*set_state) (unsigned int cpu,
>>+						     unsigned int state,
>> 						     unsigned int notify)
>>    
>>
>
>This will break speedstep-smi, as it uses speedstep_get_freqs too...
>
>  
>
ok, i chopped out the notify and cpu switching part from speedstep_set_state

>>+	/* switch to physical CPU where state is to be changed*/
>>+	cpus_allowed = current->cpus_allowed;
>>+
>>+	/* only run on CPU to be set, or on its sibling */
>>+       affected_cpu_map = cpumask_of_cpu(cpu);
>>+#ifdef CONFIG_X86_HT
>>+	hyperthreading = ((cpu_has_ht) && (smp_num_siblings == 2));
>>+	if (hyperthreading) {
>>+		sibling = cpu_sibling_map[cpu];
>>+                cpu_set(sibling, affected_cpu_map);
>>+	}
>>+#endif
>>+	set_cpus_allowed(current, affected_cpu_map);
>>+        BUG_ON(!cpu_isset(smp_processor_id(), affected_cpu_map));
>>    
>>
>
>Due to changes to cpu_sibling_map [it's a cpumask_t now], this won't
>generate appropriate code [if it even compiles] on 2.6.7-rc3. See in
>p4-clockmod.c how "easy" it becomes now.
>  
>
 thanks, done it

<snip>

>>-	/* capability check */
>>-	if (policy->cpu != 0)
>>-		return -ENODEV;
>>    
>>
>
>For the time being, this should be kept. Else you can load two different
>governors, or set two different frequencies, on both siblings. Obviously,
>it'll break. 
>
as far as i understand, the frequencies are always the same, because the 
siblings are explicitly notified. about the 2 different governors, the 
same problem exists in the current p4-clockmod.

>I'll try to fix it in the 2.6.8 timeframe...
>  
>
you mean that a change of governors gets communicated to the siblings?


thanks again for your help!

chris

[-- Attachment #2: speedstep_patch.2.6.7-rc3_detectP4M.diff --]
[-- Type: text/x-patch, Size: 971 bytes --]

diff --unified --recursive --new-file linux-2.6.7-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c linux-2.6.7-rc3.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
--- linux-2.6.7-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c	2004-06-09 22:27:18.000000000 +0000
+++ linux-2.6.7-rc3.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c	2004-06-09 22:36:31.227318536 +0000
@@ -252,11 +252,10 @@
 			 * specific.
 			 * M-P4-Ms may have either ebx=0xe or 0xf [see above]
 			 * M-P4/533 have either ebx=0xe or 0xf. [25317607.pdf]
-			 * So, how to distinguish all those processors with
-			 * ebx=0xf? I don't know. Sort them out, and wait
-			 * for someone to complain.
+			 * also, M-P4M HTs have ebx=0x8, too
+			 * For now, they are distinguished by the model_id string
 			 */
-			if (ebx == 0x0e)
+		        if ((ebx == 0x0e) || (strstr(c->x86_model_id,"Mobile Intel(R) Pentium(R) 4") != NULL)) 
 				return SPEEDSTEP_PROCESSOR_P4M;
 			break;
 		default:

[-- Attachment #3: speedstep_patch.2.6.7-rc3_SMT.diff --]
[-- Type: text/x-patch, Size: 3747 bytes --]

diff --unified --recursive --new-file linux-2.6.7-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c linux-2.6.7-rc3.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
--- linux-2.6.7-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-09 22:27:18.000000000 +0000
+++ linux-2.6.7-rc3.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-10 00:29:54.388186856 +0000
@@ -70,6 +70,7 @@
  * @notify: whether to call cpufreq_notify_transition for CPU speed changes
  *
  *   Tries to change the SpeedStep state. 
+ * Note: notify is a dummy argument. The routine never notifies.
  */
 static void speedstep_set_state (unsigned int state, unsigned int notify)
 {
@@ -77,18 +78,10 @@
 	u8			pm2_blk;
 	u8			value;
 	unsigned long		flags;
-	struct cpufreq_freqs	freqs;
 
 	if (!speedstep_chipset_dev || (state > 0x1))
 		return;
 
-	freqs.old = speedstep_get_processor_frequency(speedstep_processor);
-	freqs.new = speedstep_freqs[state].frequency;
-	freqs.cpu = 0; /* speedstep.c is UP only driver */
-	
-	if (notify)
-		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
-
 	/* get PMBASE */
 	pci_read_config_dword(speedstep_chipset_dev, 0x40, &pmbase);
 	if (!(pmbase & 0x01))
@@ -143,9 +136,6 @@
 		printk (KERN_ERR "cpufreq: change failed - I/O error\n");
 	}
 
-	if (notify)
-		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
-
 	return;
 }
 
@@ -251,12 +241,56 @@
 			     unsigned int target_freq,
 			     unsigned int relation)
 {
-	unsigned int	newstate = 0;
+	unsigned int newstate = 0;
+	struct cpufreq_freqs freqs;
+	cpumask_t cpus_allowed, affected_cpu_map;
+	int i;
+
 
 	if (cpufreq_frequency_table_target(policy, &speedstep_freqs[0], target_freq, relation, &newstate))
 		return -EINVAL;
 
-	speedstep_set_state(newstate, 1);
+	/* switch to physical CPU where state is to be changed*/
+	cpus_allowed = current->cpus_allowed;
+
+	/* only run on CPU to be set, or on its sibling */
+#ifdef CONFIG_SMP
+	affected_cpu_map = cpu_sibling_map[policy->cpu];
+#else
+	affected_cpu_map = cpumask_of_cpu(policy->cpu);
+#endif
+	set_cpus_allowed(current, affected_cpu_map);
+ 
+	freqs.old = speedstep_get_processor_frequency(speedstep_processor);
+	freqs.new = speedstep_freqs[newstate].frequency;
+	freqs.cpu = policy->cpu;
+
+	/* no transition necessary */
+	if (freqs.old == freqs.new) {
+	        set_cpus_allowed(current, cpus_allowed);
+		return 0;
+	}
+
+	for_each_cpu(i) {
+	        if (cpu_isset(i, affected_cpu_map)) {
+	                freqs.cpu = i;
+			cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+		}
+	}
+
+	/* do the transition */
+	speedstep_set_state(newstate, 0);
+
+	/* allow to be run on all CPUs */
+	set_cpus_allowed(current, cpus_allowed);
+
+	/* notifiers */
+	for_each_cpu(i) {
+	        if (cpu_isset(i, affected_cpu_map)) {
+		        freqs.cpu = i;
+			cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+		}
+	}
 
 	return 0;
 }
@@ -279,10 +313,16 @@
 {
 	int		result = 0;
 	unsigned int	speed;
+	cpumask_t       cpus_allowed,affected_cpu_map;
 
-	/* capability check */
-	if (policy->cpu != 0)
-		return -ENODEV;
+	/* only run on CPU to be set, or on its sibling */
+	cpus_allowed = current->cpus_allowed;
+#ifdef CONFIG_SMP
+	affected_cpu_map = cpu_sibling_map[policy->cpu];
+#else
+	affected_cpu_map = cpumask_of_cpu(policy->cpu);
+#endif
+	set_cpus_allowed(current, affected_cpu_map);
 
 	/* detect low and high frequency */
 	result = speedstep_get_freqs(speedstep_processor,
@@ -297,6 +337,9 @@
 	if (!speed)
 		return -EIO;
 
+	/* allow to run on any CPU */
+	set_cpus_allowed(current, cpus_allowed);
+
 	dprintk(KERN_INFO "cpufreq: currently at %s speed setting - %i MHz\n", 
 		(speed == speedstep_freqs[SPEEDSTEP_LOW].frequency) ? "low" : "high",
 		(speed / 1000));

[-- Attachment #4: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@www.linux.org.uk
http://www.linux.org.uk/mailman/listinfo/cpufreq

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Dominik Brodowski]

[-- Attachment #1.1: Type: text/plain, Size: 1260 bytes --]

> i also wonder about one thing, maybe you can help me on that. with the 
> old speedstep driver,
Do you mean speedstep-smi? Or what driver did you use?

> i would have occational hard freezups of my 
> machine. this seems to be gone (just experimental evidence, since i 
> didn't have one in a few days now where before they occured about 
> hourly). so my guess is that something in this patch must have corrected 
> it. since my machine just has one physical cpu, i really don't 
> understand what went wrong before.  do you have any idea on that?

Depends on the driver you were using :-)

> as far as i understand, the frequencies are always the same, because the 
> siblings are explicitly notified. about the 2 different governors, the 
> same problem exists in the current p4-clockmod.
> 
> >I'll try to fix it in the 2.6.8 timeframe...
> > 
> >
> you mean that a change of governors gets communicated to the siblings?

I mean that only one instance of a governor can be loaded on one physical
CPU. The same problem exists in current p4-clockmod and was a mis-design of
the cpufreq core [by me...], so it needs to be fixed quite soon.

Both patches look good to me -- Dave, could you apply them, please?

Thanks,
	Dominik

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@www.linux.org.uk
http://www.linux.org.uk/mailman/listinfo/cpufreq

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Dave Jones]

On Thu, Jun 10, 2004 at 10:30:38AM +0200, Dominik Brodowski wrote:

 > I mean that only one instance of a governor can be loaded on one physical
 > CPU. The same problem exists in current p4-clockmod and was a mis-design of
 > the cpufreq core [by me...], so it needs to be fixed quite soon.
 > 
 > Both patches look good to me -- Dave, could you apply them, please?

I fell behind again on the cpufreq backlog. I'll try to get
around to clearing it today/tomorrow.

		Dave

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Bruno Ducrot]

On Thu, Jun 10, 2004 at 12:46:36AM +0000, Christian Hoelbling wrote:
> hi dominik,
> 
> thanks alot for your detailed reply and patience with a newbie. i have 
> attached 2 patches w.r.t. 2.6.7-rc3 that should adress your suggestions.
> 
> i also wonder about one thing, maybe you can help me on that. with the 
> old speedstep driver, i would have occational hard freezups of my 
> machine. this seems to be gone (just experimental evidence, since i 
> didn't have one in a few days now where before they occured about 
> hourly). so my guess is that something in this patch must have corrected 
> it. since my machine just has one physical cpu, i really don't 
> understand what went wrong before.  do you have any idea on that?
> 

IMHO the lockup is due to the fact we use local_irq_disable() but it
should be done more globally.  I believe you may still have some lockup
still.

Cheers,

-- 
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Bruno Ducrot]

Hi,

On Thu, Jun 10, 2004 at 12:46:36AM +0000, Christian Hoelbling wrote:


> diff --unified --recursive --new-file linux-2.6.7-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c linux-2.6.7-rc3.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
> --- linux-2.6.7-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-09 22:27:18.000000000 +0000
> +++ linux-2.6.7-rc3.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-10 00:29:54.388186856 +0000
> @@ -70,6 +70,7 @@
>   * @notify: whether to call cpufreq_notify_transition for CPU speed changes
>   *
>   *   Tries to change the SpeedStep state. 
> + * Note: notify is a dummy argument. The routine never notifies.

Maybe it's better to kill notify, and modify a little speedstep-smi and speedstep-lib

Dominik, your though, please?

...

> +
> +	for_each_cpu(i) {
> +	        if (cpu_isset(i, affected_cpu_map)) {
> +	                freqs.cpu = i;
> +			cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +		}
> +	}

What about 
	for_each_cpu_mask(i, affected_cpu_map) {
		...
	}
instead?
>  
>  	/* detect low and high frequency */
>  	result = speedstep_get_freqs(speedstep_processor,
> @@ -297,6 +337,9 @@
>  	if (!speed)
>  		return -EIO;
>  
> +	/* allow to run on any CPU */
> +	set_cpus_allowed(current, cpus_allowed);
> +

Something wrong.  I mean, there is a return but the set_cpu_allowed()
is done after...

Cheers,

-- 
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Dominik Brodowski]

On Thu, Jun 10, 2004 at 05:37:11PM +0200, Bruno Ducrot wrote:
> Dominik, your though, please?

I'd agree with this change.

> What about 
> 	for_each_cpu_mask(i, affected_cpu_map) {
> 		...
> 	}
> instead?

That's indeed shorter -- don't know why whoever converted p4-clockmod to the
new sibling mask did use for_each_cpu_mask...

> > @@ -297,6 +337,9 @@
> >  	if (!speed)
> >  		return -EIO;
> >  
> > +	/* allow to run on any CPU */
> > +	set_cpus_allowed(current, cpus_allowed);
> > +
> 
> Something wrong.  I mean, there is a return but the set_cpu_allowed()
> is done after...

Thanks for catching this -- it definitely needs to be fixed.

	Dominik

Re: [PATCH] 2.6.5 speedstep on P4Ms

[Bruno Ducrot]

On Thu, Jun 10, 2004 at 06:44:34PM +0200, Dominik Brodowski wrote:
> On Thu, Jun 10, 2004 at 05:37:11PM +0200, Bruno Ducrot wrote:
> > Dominik, your though, please?
> 
> I'd agree with this change.

I will do the change for the speedstep-ich tomorrow unless someone else
(Christian ?) beat me.

Cheers,

-- 
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

[PATCH] 2.6.5 speedstep on P4Ms

[Christian Hoelbling]

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

  hi and thanks! here is a batch that should do what you suggested.


>>/ diff --unified --recursive --new-file linux-2.6.7-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c linux-2.6.7-rc3.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
>/>/ --- linux-2.6.7-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-09 22:27:18.000000000 +0000
>/>/ +++ linux-2.6.7-rc3.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-10 00:29:54.388186856 +0000
>/>/ @@ -70,6 +70,7 @@
>/>/   * @notify: whether to call cpufreq_notify_transition for CPU speed changes
>/>/   *
>/>/   *   Tries to change the SpeedStep state. 
>/>/ + * Note: notify is a dummy argument. The routine never notifies.
>/
>Maybe it's better to kill notify, and modify a little speedstep-smi and speedstep-lib

done

>Dominik, your though, please?
>
>...
>
>>/ +
>/>/ +	for_each_cpu(i) {
>/>/ +	        if (cpu_isset(i, affected_cpu_map)) {
>/>/ +	                freqs.cpu = i;
>/>/ +			cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
>/>/ +		}
>/>/ +	}
>/
>What about 
>	for_each_cpu_mask(i, affected_cpu_map) {
>		...
>	}
>instead?

thanks, done

>>/  
>/>/  	/* detect low and high frequency */
>/>/  	result = speedstep_get_freqs(speedstep_processor,
>/>/ @@ -297,6 +337,9 @@
>/>/  	if (!speed)
>/>/  		return -EIO;
>/>/  
>/>/ +	/* allow to run on any CPU */
>/>/ +	set_cpus_allowed(current, cpus_allowed);
>/>/ +
>/
>Something wrong.  I mean, there is a return but the set_cpu_allowed()
>is done after...

 this was a nasty one and i screwed up in 2 other places before. thanks alot!

chris


[-- Attachment #2: speedstep_patch.2.6.7-rc3_SMT.diff --]
[-- Type: text/x-patch, Size: 4434 bytes --]

diff --unified --recursive --new-file linux-2.6.7-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c linux-2.6.7-rc3.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
--- linux-2.6.7-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-09 22:27:18.000000000 +0000
+++ linux-2.6.7-rc3.speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c	2004-06-10 23:57:40.779127072 +0000
@@ -71,24 +71,16 @@
  *
  *   Tries to change the SpeedStep state. 
  */
-static void speedstep_set_state (unsigned int state, unsigned int notify)
+static void speedstep_set_state (unsigned int state)
 {
 	u32			pmbase;
 	u8			pm2_blk;
 	u8			value;
 	unsigned long		flags;
-	struct cpufreq_freqs	freqs;
 
 	if (!speedstep_chipset_dev || (state > 0x1))
 		return;
 
-	freqs.old = speedstep_get_processor_frequency(speedstep_processor);
-	freqs.new = speedstep_freqs[state].frequency;
-	freqs.cpu = 0; /* speedstep.c is UP only driver */
-	
-	if (notify)
-		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
-
 	/* get PMBASE */
 	pci_read_config_dword(speedstep_chipset_dev, 0x40, &pmbase);
 	if (!(pmbase & 0x01))
@@ -143,9 +135,6 @@
 		printk (KERN_ERR "cpufreq: change failed - I/O error\n");
 	}
 
-	if (notify)
-		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
-
 	return;
 }
 
@@ -251,12 +240,53 @@
 			     unsigned int target_freq,
 			     unsigned int relation)
 {
-	unsigned int	newstate = 0;
+	unsigned int newstate = 0;
+	struct cpufreq_freqs freqs;
+	cpumask_t cpus_allowed, affected_cpu_map;
+	int i;
+
 
 	if (cpufreq_frequency_table_target(policy, &speedstep_freqs[0], target_freq, relation, &newstate))
 		return -EINVAL;
 
-	speedstep_set_state(newstate, 1);
+	/* switch to physical CPU where state is to be changed*/
+	cpus_allowed = current->cpus_allowed;
+
+	/* only run on CPU to be set, or on its sibling */
+#ifdef CONFIG_SMP
+	affected_cpu_map = cpu_sibling_map[policy->cpu];
+#else
+	affected_cpu_map = cpumask_of_cpu(policy->cpu);
+#endif
+ 
+	set_cpus_allowed(current, affected_cpu_map);
+	freqs.old = speedstep_get_processor_frequency(speedstep_processor);
+	set_cpus_allowed(current, cpus_allowed);
+
+	freqs.new = speedstep_freqs[newstate].frequency;
+	freqs.cpu = policy->cpu;
+
+	/* no transition necessary */
+	if (freqs.old == freqs.new) {
+	        set_cpus_allowed(current, cpus_allowed);
+		return 0;
+	}
+
+	for_each_cpu_mask(i, affected_cpu_map) {
+	        freqs.cpu = i;
+		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+	}
+
+	/* do the transition */
+	set_cpus_allowed(current, affected_cpu_map);
+	speedstep_set_state(newstate);
+	set_cpus_allowed(current, cpus_allowed);
+
+	/* notifiers */
+	for_each_cpu_mask(i, affected_cpu_map) {
+	        freqs.cpu = i;
+		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+	}
 
 	return 0;
 }
@@ -279,21 +309,31 @@
 {
 	int		result = 0;
 	unsigned int	speed;
+	cpumask_t       cpus_allowed,affected_cpu_map;
 
-	/* capability check */
-	if (policy->cpu != 0)
-		return -ENODEV;
+	/* only run on CPU to be set, or on its sibling */
+	cpus_allowed = current->cpus_allowed;
+#ifdef CONFIG_SMP
+	affected_cpu_map = cpu_sibling_map[policy->cpu];
+#else
+	affected_cpu_map = cpumask_of_cpu(policy->cpu);
+#endif
 
 	/* detect low and high frequency */
+	set_cpus_allowed(current, affected_cpu_map);
 	result = speedstep_get_freqs(speedstep_processor,
 				     &speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				     &speedstep_freqs[SPEEDSTEP_HIGH].frequency,
 				     &speedstep_set_state);
+	set_cpus_allowed(current, cpus_allowed);
 	if (result)
 		return result;
 
 	/* get current speed setting */
+	set_cpus_allowed(current, affected_cpu_map);
 	speed = speedstep_get_processor_frequency(speedstep_processor);
+	set_cpus_allowed(current, cpus_allowed);
+
 	if (!speed)
 		return -EIO;
 
@@ -324,7 +364,20 @@
 
 static unsigned int speedstep_get(unsigned int cpu)
 {
-	return speedstep_get_processor_frequency(speedstep_processor);
+ 	unsigned int	speed;
+	cpumask_t       cpus_allowed,affected_cpu_map;
+ 
+	/* only run on CPU to be set, or on its sibling */
+	cpus_allowed = current->cpus_allowed;
+#ifdef CONFIG_SMP
+	affected_cpu_map = cpu_sibling_map[cpu];
+#else
+	affected_cpu_map = cpumask_of_cpu(cpu);
+#endif
+	set_cpus_allowed(current, affected_cpu_map);
+	speed=speedstep_get_processor_frequency(speedstep_processor);
+	set_cpus_allowed(current, cpus_allowed);
+	return speed;
 }
 
 static struct freq_attr* speedstep_attr[] = {

[-- Attachment #3: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@www.linux.org.uk
http://www.linux.org.uk/mailman/listinfo/cpufreq

[PATCH] 2.6.5 speedstep on P4Ms

[Christian Hoelbling]

>
>
>>/ i also wonder about one thing, maybe you can help me on that. with the 
>/>/ old speedstep driver, i would have occational hard freezups of my 
>/>/ machine. this seems to be gone (just experimental evidence, since i 
>/>/ didn't have one in a few days now where before they occured about 
>/>/ hourly). so my guess is that something in this patch must have corrected 
>/>/ it. since my machine just has one physical cpu, i really don't 
>/>/ understand what went wrong before.  do you have any idea on that?
>/>/ 
>/
>IMHO the lockup is due to the fact we use local_irq_disable() but it
>should be done more globally.  I believe you may still have some lockup
>still.
>  
>
 i didn't, but that might be pure chance. :-)  so does one have to 
disable the interrupts on both siblings of a HT CPU seperately? i just 
don't know how this works.

thanks,

chris