Skip to contentSkip to conversation

powernow-k8 max speed sanity check

PATCH In review Tony Lindgren 4 participants 13 messages Jan 31, 2004 – Feb 6, 2004

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 powernow-k8 max speed sanity check

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

...

# 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 [email protected] 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 # --------------------------------------------
#

arch/i386/kernel/cpu/cpufreq/powernow-k8.c +39 -7
diff -Nru a/arch/i386/kernel/cpu/cpufreq/powernow-k8.c b/arch/i386/kernel/cpu/cpufreq/powernow-k8.c
@@ -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",
#1 lore raw
Quoted 5 lines · Tony Lindgren

Hi all,

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

Quoted 7 lines

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

#2 lore raw
In reply to Tony Lindgren

Hi!

Quoted 11 lines

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

Signature · 1 line
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         
#3 lore raw
Quoted 10 lines · Pavel Machek

Hi!

Quoted 8 lines

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.

Quoted 6 lines
Quoted 3 lines

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.

Quoted 2 lines

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

#4 lore raw

Hi!

Quoted 5 lines
Quoted 2 lines

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.

Quoted 4 lines

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.]

include/asm-i386/pci.h +1 -1
Index: linux/include/asm-i386/pci.h
===================================================================
@@ -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);
include/asm-x86_64/pci.h +1 -1
Index: linux/include/asm-x86_64/pci.h
===================================================================
@@ -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);
Quoted 7 lines
Quoted 2 lines

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

arch/i386/kernel/cpu/cpufreq/powernow-k8.c +75 -65

291 lines not on this page · raw message

Signature · 2 lines
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
#5 lore raw
Quoted 3 lines · Pavel Machek
Quoted 3 lines
Quoted 3 lines

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/

Quoted 2 lines

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.

Quoted 8 lines
Quoted 4 lines

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

dsdt.dsl renamed from dsdt.dsl.orig +1 -1
@@ -2922,7 +2922,7 @@
                     }
                 }
 
-                OperationRegion (CCRD, PCI_Config, 0x00, 0xA7)
+                OperationRegion (CCRD, PCI_Config, 0x00, 0xA8)
                 Field (CCRD, DWordAcc, Lock, Preserve)
                 {
                     Offset (0x04), 

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

#6 lore raw
In reply to Pavel Machek

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

#7 lore raw

Hi!

Quoted 6 lines

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.

Quoted 2 lines

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...

Quoted 14 lines

Maybe replace this

arch/i386/kernel/cpu/cpufreq/powernow-k8.c +5 -1
 		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:

arch/i386/kernel/cpu/cpufreq/powernow-k8.c line 607

Quoted 7 lines
arch/i386/kernel/cpu/cpufreq/powernow-k8.c +2 -2
 		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.

Quoted 11 lines

I see a little problem with hardcoding the values:

arch/i386/kernel/cpu/cpufreq/powernow-k8.c +7 -0
+		if (arima) {
+			ppst[1].fid = 0x8;
+			ppst[1].vid = 0x6;
+#ifdef THREE
+			ppst[2].fid = 0xa;
+			ppst[2].vid = 0x2;
+#endif
 		}
Quoted 1 line

This would fail if I upgraded my CPU, right?

Yes.

Quoted 1 line

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

Signature · 2 lines
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
#8 lore raw
Quoted 3 lines · Pavel Machek

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?

Quoted 5 lines
Quoted 2 lines

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?

Quoted 4 lines

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

#9 lore raw

Hi!

Quoted 6 lines
Quoted 2 lines

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.

Quoted 10 lines
Quoted 4 lines
Quoted 1 line

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

Signature · 2 lines
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
#10 lore raw
Quoted 5 lines · Pavel Machek

Hi!

Quoted 3 lines
Quoted 2 lines

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.

Quoted 8 lines
Quoted 3 lines

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 :)

Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line

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...

Quoted 7 lines
Quoted 3 lines

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

#11 lore raw

Hi!

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines

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?

Quoted 8 lines
Quoted 3 lines

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..

Quoted 2 lines

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

Signature · 2 lines
Horseback riding is like software...
...vgf orggre jura vgf serr.
#12 lore raw
Quoted 7 lines · Pavel Machek
Quoted 3 lines

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..

Quoted 6 lines
Quoted 2 lines

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