[PATCH] powernow-k8 max speed sanity check

[PATCH] powernow-k8 max speed sanity check

[Tony Lindgren]

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

Hi all,

Following is a little patch to do a sanity check on the max speed and 
voltage values provided by the bios.

Some buggy bioses provide bad values if the cpu changes, for example, in 
my case the bios claims the max cpu speed is 1600MHz, while it's running at
1800MHz. (Cheapo Emachines m6805 you know...) This could also happen on 
machines where the cpu is upgraded.

These checks should be safe, as they only change things if the machine is
already running at a higher speed than the bios claims.

Regards,

Tony

A here's some sample output after applying the patch:
...
powernow-k8: Found AMD Athlon 64 / Opteron processor supporting p-state transitions
powernow-k8: voltage stable time: 5 (units 20us)
powernow-k8: p states on battery: 0 - all available
powernow-k8: ramp voltage offset: 2
powernow-k8: isochronous relief time: 3
powernow-k8: maximum voltage step: 0
powernow-k8: BIOS error: numpst listed as 8 should be 1. Using 1.
powernow-k8: pll lock time: 0x2
powernow-k8: maxfid: 0x8
powernow-k8: maxvid: 0x0
powernow-k8: numpstates: 0x2
powernow-k8:    0 : fid 0x0, vid 0x12
powernow-k8:    1 : fid 0x8, vid 0x0
powernow-k8: BIOS error: max speed fid listed as 0x08, should be at least 0x0a. Using current speed.
powernow-k8: BIOS error: max voltage vid listed as 0x00, should be at least 0x06. Using current speed.
powernow-k8: currfid 0xa, currvid 0x6
powernow-k8: cpu_init done, current fid 0xa, vid 0x6
...


[-- Attachment #2: patch-2.6.2-pre3-powernow-k8-max-speed-check --]
[-- Type: text/plain, Size: 2808 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1550  -> 1.1551 
#	arch/i386/kernel/cpu/cpufreq/powernow-k8.c	1.9     -> 1.10   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/01/31	tony@atomide.com	1.1551
# Sanity check on the BIOS provided max speed vs. current speed
# 
# - Moves the current speed detection a bit earlier
# - Overrides the max speed and voltage if current running values are
#   greater than BIOS provided values
# - Ignores incorrect BIOS numpst value as it is not used anyways
# --------------------------------------------
#
diff -Nru a/arch/i386/kernel/cpu/cpufreq/powernow-k8.c b/arch/i386/kernel/cpu/cpufreq/powernow-k8.c
--- a/arch/i386/kernel/cpu/cpufreq/powernow-k8.c	Sat Jan 31 12:21:04 2004
+++ b/arch/i386/kernel/cpu/cpufreq/powernow-k8.c	Sat Jan 31 12:21:05 2004
@@ -539,6 +539,34 @@
 	return 1;
 }
 
+/* Use current frequency if greater than BIOS value. */
+static inline int
+fid_check_max(int cur, int max)
+{
+	if (max < cur) {
+		printk(KERN_WARNING BFX "max speed fid listed as 0x%02x, "
+		       "should be at least 0x%02x. Using current speed.\n", 
+		       max, cur);
+		return cur;
+	}
+
+	return max;
+}
+
+/* Use current voltage if greater than BIOS value. */
+static inline int
+vid_check_max(int cur, int max)
+{
+	if (max < cur) {
+		printk(KERN_WARNING BFX "max voltage vid listed as 0x%02x, "
+		       "should be at least 0x%02x. Using current speed.\n", 
+		       max, cur);
+		return cur;
+	}
+
+	return max;
+}
+
 /* Find and validate the PSB/PST table in BIOS. */
 static inline int
 find_psb_table(void)
@@ -603,8 +631,8 @@
 
 		dprintk(KERN_DEBUG PFX "numpst: 0x%x\n", psb->numpst);
 		if (psb->numpst != 1) {
-			printk(KERN_ERR BFX "numpst must be 1\n");
-			return -ENODEV;
+			printk(KERN_WARNING BFX "numpst listed as %i "
+			       "should be 1. Using 1.\n", psb->numpst);
 		}
 
 		dprintk(KERN_DEBUG PFX "cpuid: 0x%x\n", psb->cpuid);
@@ -668,6 +696,15 @@
 			return -ENODEV;
 		}
 
+		if (query_current_values_with_pending_wait()) {
+			kfree(ppst);
+			return -EIO;
+		}
+
+		/* Do a sanity check on the max values vs. current values */
+		ppst[numps-1].fid = fid_check_max(currfid, ppst[numps-1].fid);
+		ppst[numps-1].vid = vid_check_max(currvid, ppst[numps-1].vid);
+
 		for (j = 1; j < numps; j++) {
 			if ((lastfid >= ppst[j].fid)
 			    || (ppst[j].fid & 1)
@@ -696,11 +733,6 @@
 				kfree(ppst);
 				return -ENODEV;
 			}
-		}
-
-		if (query_current_values_with_pending_wait()) {
-			kfree(ppst);
-			return -EIO;
 		}
 
 		printk(KERN_INFO PFX "currfid 0x%x, currvid 0x%x\n",

Re: [PATCH] powernow-k8 max speed sanity check

[Dave Jones]

On Sat, Jan 31, 2004 at 12:35:12PM -0800, Tony Lindgren wrote:
 > Hi all,
 > 
 > Following is a little patch to do a sanity check on the max speed and 
 > voltage values provided by the bios.
 > 
 > Some buggy bioses provide bad values if the cpu changes, for example, in 
 > my case the bios claims the max cpu speed is 1600MHz, while it's running at
 > 1800MHz. (Cheapo Emachines m6805 you know...) This could also happen on 
 > machines where the cpu is upgraded.
 > 
 > These checks should be safe, as they only change things if the machine is
 > already running at a higher speed than the bios claims.

ye gads, yet another problem with eMachines PST tables.
Conclusive proof I think that we need the 'read from ACPI P state tables
when PST contains garbage' patch.

		Dave

Re: [PATCH] powernow-k8 max speed sanity check

[Pavel Machek]

Hi!

> Following is a little patch to do a sanity check on the max speed and 
> voltage values provided by the bios.
> 
> Some buggy bioses provide bad values if the cpu changes, for example, in 
> my case the bios claims the max cpu speed is 1600MHz, while it's running at
> 1800MHz. (Cheapo Emachines m6805 you know...) This could also happen on 
> machines where the cpu is upgraded.
> 
> These checks should be safe, as they only change things if the machine is
> already running at a higher speed than the bios claims.
> 

Someone should really bug them to fix their BIOS. (BTW does keyboard work
ok for you?) 
Going though ACPI solves this, and I have perhaps better
patch to hardcode right values...
				Pavel

-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [PATCH] powernow-k8 max speed sanity check

[Tony Lindgren]

* Pavel Machek <pavel@ucw.cz> [040205 06:03]:
> Hi!
> 
> > Following is a little patch to do a sanity check on the max speed and 
> > voltage values provided by the bios.
> > 
> > Some buggy bioses provide bad values if the cpu changes, for example, in 
> > my case the bios claims the max cpu speed is 1600MHz, while it's running at
> > 1800MHz. (Cheapo Emachines m6805 you know...) This could also happen on 
> > machines where the cpu is upgraded.
> > 
> > These checks should be safe, as they only change things if the machine is
> > already running at a higher speed than the bios claims.
> > 
> 
> Someone should really bug them to fix their BIOS. (BTW does keyboard work
> ok for you?) 

No problems with keyboard, and the cpufreq works fine with the patch, but
not at all without the patch.

There are some ACPI related issues though, such as: via-rhine gets wrong 
irq with ACPI on, system hangs with yenta_socket loaded if I 
connect/disconnect the power cord... So for now, I don't use the PCMCIA.

> Going though ACPI solves this, and I have perhaps better
> patch to hardcode right values...

Still, the max speed check should be safe. Maybe pass values as module
options too? I would not trust on ACPI working right on this machine :)

Tony

Re: [PATCH] powernow-k8 max speed sanity check

[Pavel Machek]

Hi!

> > Someone should really bug them to fix their BIOS. (BTW does keyboard work
> > ok for you?) 
> 
> No problems with keyboard, and the cpufreq works fine with the patch, but
> not at all without the patch.

Attached is my cpufreq patch. You may still prefer yours, but this one
has correct tables derived from ACPI.

> There are some ACPI related issues though, such as: via-rhine gets wrong 
> irq with ACPI on, system hangs with yenta_socket loaded if I 
> connect/disconnect the power cord... So for now, I don't use the
> PCMCIA.

They did something very stupid with io-ports at 0x4000. This should
work around it. [Note, I probably have slightly different machine from
you.]

Index: linux/include/asm-i386/pci.h
===================================================================
--- linux.orig/include/asm-i386/pci.h	2004-02-04 23:51:38.000000000 +0100
+++ linux/include/asm-i386/pci.h	2004-02-05 02:26:36.000000000 +0100
@@ -20,7 +20,7 @@
 #define PCIBIOS_MIN_IO		0x1000
 #define PCIBIOS_MIN_MEM		(pci_mem_start)
 
-#define PCIBIOS_MIN_CARDBUS_IO	0x4000
+#define PCIBIOS_MIN_CARDBUS_IO	0x5000
 
 void pcibios_config_init(void);
 struct pci_bus * pcibios_scan_root(int bus);
Index: linux/include/asm-x86_64/pci.h
===================================================================
--- linux.orig/include/asm-x86_64/pci.h	2004-02-04 23:51:38.000000000 +0100
+++ linux/include/asm-x86_64/pci.h	2004-02-05 02:26:36.000000000 +0100
@@ -24,7 +24,7 @@
 #define PCIBIOS_MIN_IO		0x1000
 #define PCIBIOS_MIN_MEM		(pci_mem_start)
 
-#define PCIBIOS_MIN_CARDBUS_IO	0x4000
+#define PCIBIOS_MIN_CARDBUS_IO	0x5000
 
 void pcibios_config_init(void);
 struct pci_bus * pcibios_scan_root(int bus);

 

> > Going though ACPI solves this, and I have perhaps better
> > patch to hardcode right values...
> 
> Still, the max speed check should be safe. Maybe pass values as module
> options too? I would not trust on ACPI working right on this machine :)
> 
> Tony


Index: linux/arch/i386/kernel/cpu/cpufreq/powernow-k8.c
===================================================================
--- linux.orig/arch/i386/kernel/cpu/cpufreq/powernow-k8.c	2004-02-04 23:51:40.000000000 +0100
+++ linux/arch/i386/kernel/cpu/cpufreq/powernow-k8.c	2004-02-05 02:25:37.000000000 +0100
@@ -31,7 +31,7 @@
 
 #define PFX "powernow-k8: "
 #define BFX PFX "BIOS error: "
-#define VERSION "version 1.00.08 - September 26, 2003"
+#define VERSION "version 1.00.08a"
 #include "powernow-k8.h"
 
 #ifdef CONFIG_PREEMPT
@@ -44,7 +44,7 @@
 static u32 rvo;		/* ramp voltage offset, from PSB */
 static u32 irt;		/* isochronous relief time, from PSB */
 static u32 vidmvs;	/* usable value calculated from mvs, from PSB */
-struct pst_s *ppst;	/* array of p states, valid for this part */
+static struct pst_s *ppst; /* array of p states, valid for this part */
 static u32 currvid;	/* keep track of the current fid / vid */
 static u32 currfid;
 
@@ -107,23 +107,27 @@
 	}
 }
 
-/* Sort the fid/vid frequency table into ascending order by fid. The spec */
-/* implies that it will be sorted by BIOS, but, it only implies it, and I */
-/* prefer not to trust when I can check.                                  */
-/* Yes, it is a simple bubble sort, but the PST is really small, so the   */
-/* choice of algorithm is pretty irrelevant.                              */
+/*
+ * Sort the fid/vid frequency table into ascending order by fid. The spec
+ * implies that it will be sorted by BIOS, but, it only implies it, and I
+ * prefer not to trust when I can check.
+ * Yes, it is a simple bubble sort, but the PST is really small, so the
+ * choice of algorithm is pretty irrelevant.
+ */
 static inline void
 sort_pst(struct pst_s *ppst, u32 numpstates)
 {
 	u32 i;
 	u8 tempfid;
 	u8 tempvid;
-	int swaps = 1;
+	int swaps = 2;
 
 	while (swaps) {
 		swaps = 0;
 		for (i = 0; i < (numpstates - 1); i++) {
 			if (ppst[i].fid > ppst[i + 1].fid) {
+				if (swaps == 2)
+					printk(KERN_WARNING BFX "PST table not sorted according to frequency, fixed.");
 				swaps = 1;
 				tempfid = ppst[i].fid;
 				tempvid = ppst[i].vid;
@@ -134,29 +138,29 @@
 			}
 		}
 	}
-
 	return;
 }
 
-/* Return 1 if the pending bit is set. Unless we are actually just told the */
-/* processor to transition a state, seeing this bit set is really bad news. */
+/*
+ * Return 1 if the pending bit is set. Unless we are actually just told the
+ * processor to transition a state, seeing this bit set is really bad news.
+ */
 static inline int
 pending_bit_stuck(void)
 {
-	u32 lo;
-	u32 hi;
-
+	u32 lo, hi;
 	rdmsr(MSR_FIDVID_STATUS, lo, hi);
 	return lo & MSR_S_LO_CHANGE_PENDING ? 1 : 0;
 }
 
-/* Update the global current fid / vid values from the status msr. Returns 1 */
-/* on error.                                                                 */
+/*
+ * Update the global current fid / vid values from the status msr. Returns 1
+ * on error.
+ */
 static int
 query_current_values_with_pending_wait(void)
 {
-	u32 lo;
-	u32 hi;
+	u32 lo, hi;
 	u32 i = 0;
 
 	lo = MSR_S_LO_CHANGE_PENDING;
@@ -271,9 +275,11 @@
 	return 0;
 }
 
-/* Reduce the vid by the max of step or reqvid.                   */
-/* Decreasing vid codes represent increasing voltages :           */
-/* vid of 0 is 1.550V, vid of 0x1e is 0.800V, vid of 0x1f is off. */
+/*
+ * Reduce the vid by the max of step or reqvid.
+ * Decreasing vid codes represent increasing voltages:
+ * vid of 0 is 1.550V, vid of 0x1e is 0.800V, vid of 0x1f is off.
+ */
 static int
 decrease_vid_code_by_step(u32 reqvid, u32 step)
 {
@@ -316,8 +322,10 @@
 	return 0;
 }
 
-/* Phase 1 - core voltage transition ... setup appropriate voltage for the */
-/* fid transition.                                                         */
+/*
+ * Phase 1 - core voltage transition ... setup appropriate voltage for the
+ * fid transition.
+ */
 static inline int
 core_voltage_pre_transition(u32 reqvid)
 {
@@ -500,7 +508,9 @@
 	}
 
 	if (c->x86_vendor != X86_VENDOR_AMD) {
+#ifdef MODULE
 		printk(KERN_INFO PFX "Not an AMD processor\n");
+#endif
 		return 0;
 	}
 
@@ -533,9 +543,7 @@
 		return 0;
 	}
 
-	printk(KERN_INFO PFX "Found AMD Athlon 64 / Opteron processor "
-	       "supporting p-state transitions\n");
-
+	printk(KERN_INFO PFX "Found AMD processor supporting PowerNow (" VERSION ")\n");
 	return 1;
 }
 
@@ -549,6 +557,7 @@
 	u32 lastfid;
 	u32 mvs;
 	u8 maxvid;
+	int arima = 0;
 
 	for (i = 0xc0000; i < 0xffff0; i += 0x10) {
 		/* Scan BIOS looking for the signature. */
@@ -573,51 +582,40 @@
 		}
 
 		vstable = psb->voltagestabilizationtime;
-		printk(KERN_INFO PFX "voltage stable time: %d (units 20us)\n",
-		       vstable);
-
 		dprintk(KERN_DEBUG PFX "flags2: 0x%x\n", psb->flags2);
 		rvo = psb->flags2 & 3;
 		irt = ((psb->flags2) >> 2) & 3;
 		mvs = ((psb->flags2) >> 4) & 3;
 		vidmvs = 1 << mvs;
 		batps = ((psb->flags2) >> 6) & 3;
-		printk(KERN_INFO PFX "p states on battery: %d ", batps);
-		switch (batps) {
-		case 0:
-			printk("- all available\n");
-			break;
-		case 1:
-			printk("- only the minimum\n");
-			break;
-		case 2:
-			printk("- only the 2 lowest\n");
-			break;
-		case 3:
-			printk("- only the 3 lowest\n");
-			break;
-		}
-		printk(KERN_INFO PFX "ramp voltage offset: %d\n", rvo);
-		printk(KERN_INFO PFX "isochronous relief time: %d\n", irt);
-		printk(KERN_INFO PFX "maximum voltage step: %d\n", mvs);
+
+		printk(KERN_INFO PFX "voltage stable in %d usec", vstable * 20);
+		if (batps)
+			printk(", only %d lowest states on battery", batps);
+		printk(", ramp voltage offset: %d", rvo);
+		printk(", isochronous relief time: %d", irt);
+		printk(", maximum voltage step: %d\n", mvs);
 
 		dprintk(KERN_DEBUG PFX "numpst: 0x%x\n", psb->numpst);
 		if (psb->numpst != 1) {
 			printk(KERN_ERR BFX "numpst must be 1\n");
-			return -ENODEV;
+			if (psb->numpst == 3) {
+				printk(KERN_INFO PFX "assuming arima notebug\n");
+				arima = 1;
+			} else
+				return -ENODEV;
 		}
 
 		dprintk(KERN_DEBUG PFX "cpuid: 0x%x\n", psb->cpuid);
 
 		plllock = psb->plllocktime;
-		printk(KERN_INFO PFX "pll lock time: 0x%x\n", plllock);
+		printk(KERN_INFO PFX "pll lock time: 0x%x, ", plllock);
 
 		maxvid = psb->maxvid;
-		printk(KERN_INFO PFX "maxfid: 0x%x\n", psb->maxfid);
-		printk(KERN_INFO PFX "maxvid: 0x%x\n", maxvid);
+		printk("maxfid 0x%x (%d MHz), maxvid 0x%x\n", 
+		       psb->maxfid, find_freq_from_fid(psb->maxfid), maxvid);
 
 		numps = psb->numpstates;
-		printk(KERN_INFO PFX "numpstates: 0x%x\n", numps);
 		if (numps < 2) {
 			printk(KERN_ERR BFX "no p states to transition\n");
 			return -ENODEV;
@@ -637,10 +635,15 @@
 		}
 
 		if ((numps <= 1) || (batps <= 1)) {
+			/* FIXME: Is this right? I can see one state on battery, two states total as an usefull config */
 			printk(KERN_ERR PFX "only 1 p-state to transition\n");
 			return -ENODEV;
 		}
 
+#ifdef THREE
+		if (arima)
+			numps = 3;
+#endif
 		ppst = kmalloc(sizeof (struct pst_s) * numps, GFP_KERNEL);
 		if (!ppst) {
 			printk(KERN_ERR PFX "ppst memory alloc failure\n");
@@ -651,12 +654,23 @@
 		for (j = 0; j < numps; j++) {
 			ppst[j].fid = pst[j].fid;
 			ppst[j].vid = pst[j].vid;
-			printk(KERN_INFO PFX
-			       "   %d : fid 0x%x, vid 0x%x\n", j,
-			       ppst[j].fid, ppst[j].vid);
+		}
+		if (arima) {
+			ppst[1].fid = 0x8;
+			ppst[1].vid = 0x6;
+#ifdef THREE
+			ppst[2].fid = 0xa;
+			ppst[2].vid = 0x2;
+#endif
 		}
 		sort_pst(ppst, numps);
 
+		for (j = 0; j < numps; j++) {
+			printk(KERN_INFO PFX
+			       "   %d : fid 0x%x (%d MHz), vid 0x%x\n", j,
+			       ppst[j].fid, find_freq_from_fid(ppst[j].fid), ppst[j].vid);
+		}
+
 		lastfid = ppst[0].fid;
 		if (lastfid > LO_FID_TABLE_TOP)
 			printk(KERN_INFO BFX "first fid not in lo freq tbl\n");
@@ -687,14 +701,10 @@
 			if (ppst[j].vid < rvo) {	/* vid+rvo >= 0 */
 				printk(KERN_ERR BFX
 				       "0 vid exceeded with pstate %d\n", j);
-				kfree(ppst);
-				return -ENODEV;
 			}
 			if (ppst[j].vid < maxvid+rvo) { /* vid+rvo >= maxvid */
 				printk(KERN_ERR BFX
 				       "maxvid exceeded with pstate %d\n", j);
-				kfree(ppst);
-				return -ENODEV;
 			}
 		}
 
@@ -703,8 +713,8 @@
 			return -EIO;
 		}
 
-		printk(KERN_INFO PFX "currfid 0x%x, currvid 0x%x\n",
-		       currfid, currvid);
+		printk(KERN_INFO PFX "currfid 0x%x (%d MHz), currvid 0x%x\n",
+		       currfid, find_freq_from_fid(currfid), currvid);
 
 		for (j = 0; j < numps; j++)
 			if ((ppst[j].fid==currfid) && (ppst[j].vid==currvid))
@@ -718,8 +728,10 @@
 	return -ENODEV;
 }
 
-/* Converts a frequency (that might not necessarily be a multiple of 200) */
-/* to a fid.                                                              */
+/*
+ * Converts a frequency (that might not necessarily be a multiple of 200)
+ * to a fid.
+ */
 static u32
 find_closest_fid(u32 freq, int searchup)
 {
@@ -985,8 +997,6 @@
 {
 	int rc;
 
-	printk(KERN_INFO PFX VERSION "\n");
-
 	if (check_supported_cpu() == 0)
 		return -ENODEV;
 



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

Re: [PATCH] powernow-k8 max speed sanity check

[Tony Lindgren]

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

* Pavel Machek <pavel@ucw.cz> [040205 10:50]:
> 
> > > Someone should really bug them to fix their BIOS. (BTW does keyboard work
> > > ok for you?) 

Ah, I think what's the keyboard problem. If I don't have usb compiled into
the kernel, my keyboard shows stuck keys continuously. I just posted my .config
at http://www.muru.com/linux/amd64/

> Attached is my cpufreq patch. You may still prefer yours, but this one
> has correct tables derived from ACPI.

Cool, I'll check it out.

> > There are some ACPI related issues though, such as: via-rhine gets wrong 
> > irq with ACPI on, system hangs with yenta_socket loaded if I 
> > connect/disconnect the power cord... So for now, I don't use the
> > PCMCIA.
> 
> They did something very stupid with io-ports at 0x4000. This should
> work around it. [Note, I probably have slightly different machine from
> you.]

Excellent, I'll try it out as well for the PCMCIA.

Here's a bios patch that gets rid of one error and allows the machine to
boot with ioapic option. I've not yet verified that it's this patch though.
With ioapic, the USB and network interrupts stop working, so there's
something else.

Tony						       

[-- Attachment #2: patch-m6805-bios-warning --]
[-- Type: text/plain, Size: 409 bytes --]

--- dsdt.dsl.orig	2004-02-03 23:34:13.000000000 -0800
+++ dsdt.dsl	2004-02-03 23:34:19.000000000 -0800
@@ -2922,7 +2922,7 @@
                     }
                 }
 
-                OperationRegion (CCRD, PCI_Config, 0x00, 0xA7)
+                OperationRegion (CCRD, PCI_Config, 0x00, 0xA8)
                 Field (CCRD, DWordAcc, Lock, Preserve)
                 {
                     Offset (0x04), 

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

dump:
	./acpidmp DSDT > dsdt.orig && \
	cp dsdt.orig dsdt

decompile:
	./iasl -d dsdt

compile:
	./iasl -tc dsdt.dsl

install:
	cp dsdt.hex /usr/src/linux/drivers/acpi/tables/acpi_dsdt.c

Re: [PATCH] powernow-k8 max speed sanity check

[Tony Lindgren]

Pavel,

Here's some comments after trying out your 2 patches. 

First, the PCMCIA patch worked great, I can now plug/unplug power cord with
yenta_socket loaded :) Maybe now I can _haul_ this laptop to a cafe and use 
the WLAN, hehe. In case anybody else needs them, I'll put the patches 
I'm using to:

http://www.muru.com/linux/amd64/

The powernow-k8.c patch did not work, as my numpst is 8, not 3. So why not
just ignore the numpst, as it is not used?

Maybe replace this

 		if (psb->numpst != 1) {
 			printk(KERN_ERR BFX "numpst must be 1\n");
-			return -ENODEV;
+			if (psb->numpst == 3) {
+				printk(KERN_INFO PFX "assuming arima notebug\n");
+				arima = 1;
+			} else
+				return -ENODEV;
 		}

With this instead:
 
 		dprintk(KERN_DEBUG PFX "numpst: 0x%x\n", psb->numpst);
 		if (psb->numpst != 1) {
-			printk(KERN_ERR BFX "numpst must be 1\n");
-			return -ENODEV;
+			printk(KERN_WARNING BFX "numpst listed as %i "
+			       "should be 1. Using 1.\n", psb->numpst);
 		}
 
Hmm, looks like that contains a bug, where it does not change psb->numpst to 1,
but that's not used anyways, so it could all be actually chopped out?

I see a little problem with hardcoding the values:

+		if (arima) {
+			ppst[1].fid = 0x8;
+			ppst[1].vid = 0x6;
+#ifdef THREE
+			ppst[2].fid = 0xa;
+			ppst[2].vid = 0x2;
+#endif
 		}

This would fail if I upgraded my CPU, right? 

What do you think about using module options maxfid and maxvid?

Regards,

Tony

Re: [PATCH] powernow-k8 max speed sanity check

[Pavel Machek]

Hi!

> Here's some comments after trying out your 2 patches. 
> 
> First, the PCMCIA patch worked great, I can now plug/unplug power cord with
> yenta_socket loaded :) Maybe now I can _haul_ this laptop to a cafe and use 
> the WLAN, hehe. In case anybody else needs them, I'll put the
> patches 

Good.

> The powernow-k8.c patch did not work, as my numpst is 8, not 3. So why not
> just ignore the numpst, as it is not used?

What are the BIOS people smoking? I thought they have left some old
tables there, but they keep updating them...


> Maybe replace this
> 
>  		if (psb->numpst != 1) {
>  			printk(KERN_ERR BFX "numpst must be 1\n");
> -			return -ENODEV;
> +			if (psb->numpst == 3) {
> +				printk(KERN_INFO PFX "assuming arima notebug\n");
> +				arima = 1;
> +			} else
> +				return -ENODEV;
>  		}
> 
> With this instead:
>  
>  		dprintk(KERN_DEBUG PFX "numpst: 0x%x\n", psb->numpst);
>  		if (psb->numpst != 1) {
> -			printk(KERN_ERR BFX "numpst must be 1\n");
> -			return -ENODEV;
> +			printk(KERN_WARNING BFX "numpst listed as %i "
> +			       "should be 1. Using 1.\n", psb->numpst);
>  		}

Well, I wanted some way to detect exactly this broken machine. You
might want to simply put == 8 there.. but proper solution is DMI blacklist. 

> I see a little problem with hardcoding the values:
> 
> +		if (arima) {
> +			ppst[1].fid = 0x8;
> +			ppst[1].vid = 0x6;
> +#ifdef THREE
> +			ppst[2].fid = 0xa;
> +			ppst[2].vid = 0x2;
> +#endif
>  		}
> 
> This would fail if I upgraded my CPU, right? 

Yes.

> What do you think about using module options maxfid and maxvid?

Well, the original BIOS has not only maximum values wrong, but also
1600MHz wrong, as far as I can tell...

Something like /proc/frequencies file would be needed where you could

echo "0 0xa; 0x8 0x6; 0xa 0x2" > /proc/frequencies to override. You
need to override all of them, not just top one.
							Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [PATCH] powernow-k8 max speed sanity check

[Tony Lindgren]

* Pavel Machek <pavel@suse.cz> [040205 13:39]:
> 
> Well, I wanted some way to detect exactly this broken machine. You
> might want to simply put == 8 there.. but proper solution is DMI blacklist. 

Or just use the current running values for max speed. That will fail if the
system boots at lower speeds on battery though. And the BIOS checks will
fail on buggy BIOSes and when upgrading the CPU. So maybe use both checks?

> 
> > What do you think about using module options maxfid and maxvid?
> 
> Well, the original BIOS has not only maximum values wrong, but also
> 1600MHz wrong, as far as I can tell...

Outch! I did not know that...

Are the middle values needed? What if you only use the min and max 
fid/vid values, and always recalculate the stepping tables from those 
values?

> Something like /proc/frequencies file would be needed where you could
> 
> echo "0 0xa; 0x8 0x6; 0xa 0x2" > /proc/frequencies to override. You
> need to override all of them, not just top one.

Maybe not, if the stepping table would get recalculated on init?

Tony

Re: [PATCH] powernow-k8 max speed sanity check

[Pavel Machek]

Hi!

> > Well, I wanted some way to detect exactly this broken machine. You
> > might want to simply put == 8 there.. but proper solution is DMI blacklist. 
> 
> Or just use the current running values for max speed. That will fail if the
> system boots at lower speeds on battery though. And the BIOS checks will
> fail on buggy BIOSes and when upgrading the CPU. So maybe use both checks?

Well... for your own kernel hack it any way you want.

For public consumtion... I guess DMI blacklist and assume user did not
exchange cpus... We could cross-check /proc/cpuinfo.

> > > What do you think about using module options maxfid and maxvid?
> > 
> > Well, the original BIOS has not only maximum values wrong, but also
> > 1600MHz wrong, as far as I can tell...
> 
> Outch! I did not know that...
> 
> Are the middle values needed? What if you only use the min and max 
> fid/vid values, and always recalculate the stepping tables from those 
> values?

Well, 1600MHz operation is very nice, as it has significantly less
power consumption but pretty much same performance. It also does not
start CPU fan most of the time :-).  

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

Re: [PATCH] powernow-k8 max speed sanity check

[Tony Lindgren]

* Pavel Machek <pavel@ucw.cz> [040205 16:28]:
> Hi!
> 
> > > Well, I wanted some way to detect exactly this broken machine. You
> > > might want to simply put == 8 there.. but proper solution is DMI blacklist. 
> > 
> > Or just use the current running values for max speed. That will fail if the
> > system boots at lower speeds on battery though. And the BIOS checks will
> > fail on buggy BIOSes and when upgrading the CPU. So maybe use both checks?
> 
> Well... for your own kernel hack it any way you want.
> 
> For public consumtion... I guess DMI blacklist and assume user did not
> exchange cpus... We could cross-check /proc/cpuinfo.

Yeah, I don't care as long as it works good :)

> > > > What do you think about using module options maxfid and maxvid?
> > > 
> > > Well, the original BIOS has not only maximum values wrong, but also
> > > 1600MHz wrong, as far as I can tell...
> > 
> > Outch! I did not know that...
> > 
> > Are the middle values needed? What if you only use the min and max 
> > fid/vid values, and always recalculate the stepping tables from those 
> > values?
> 
> Well, 1600MHz operation is very nice, as it has significantly less
> power consumption but pretty much same performance. It also does not
> start CPU fan most of the time :-).  

Yes, 1600MHz would be nice, I agree.

But I meant calculating all the valid values inbetween min and max without
relying on getting those values from the BIOS.

Then you would only need to find out the min and max values, and not
care about bugs in the BIOS table inbetween min and max.

I guess the code already does that to figure out how many steps are needed
to change between min and max?

Tony

Re: [PATCH] powernow-k8 max speed sanity check

[Pavel Machek]

Hi!

> > > Are the middle values needed? What if you only use the min and max 
> > > fid/vid values, and always recalculate the stepping tables from those 
> > > values?
> > 
> > Well, 1600MHz operation is very nice, as it has significantly less
> > power consumption but pretty much same performance. It also does not
> > start CPU fan most of the time :-).  
> 
> Yes, 1600MHz would be nice, I agree.
> 
> But I meant calculating all the valid values inbetween min and max without
> relying on getting those values from the BIOS.

I do not think values can be calculated by some simple
formula. voltage/frequency relations may be quite complex..

> I guess the code already does that to figure out how many steps are needed
> to change between min and max?

Well, but it steps voltage first, then frequency [going up] or
frequency first, voltage then [going down]; middle values used by the
transitions are not too good if you want to be power-efficient.

								Pavel
-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

Re: [PATCH] powernow-k8 max speed sanity check

[Tony Lindgren]

* Pavel Machek <pavel@ucw.cz> [040206 04:57]:
> 
> > But I meant calculating all the valid values inbetween min and max without
> > relying on getting those values from the BIOS.
> 
> I do not think values can be calculated by some simple
> formula. voltage/frequency relations may be quite complex..
> 
> > I guess the code already does that to figure out how many steps are needed
> > to change between min and max?
> 
> Well, but it steps voltage first, then frequency [going up] or
> frequency first, voltage then [going down]; middle values used by the
> transitions are not too good if you want to be power-efficient.

OK, I see. Then your idea of using a lookup table for blacklisted BIOSes
makes good sense.

Tony