All of lore.kernel.org
 help / color / mirror / Atom feed
* RE: Cleanups for powernow-k8
@ 2004-01-13 22:37 ` paul.devriendt
  0 siblings, 0 replies; 35+ messages in thread
From: paul.devriendt @ 2004-01-13 22:37 UTC (permalink / raw)
  To: davej, pavel; +Cc: cpufreq, linux, linux-kernel

> On Tue, Jan 13, 2004 at 10:51:49PM +0100, Pavel Machek wrote:
> 
>  > powernow-k8 uses strange kind of comments
> 
> comments part I kinda agree with, though its not critical..

Ok, point taken.

>  > and is way too verbose.
> 
> I agree that something like that output belongs more in x86info,
> or a standalone tool, but I think Paul wanted to keep debugging stuff
> there for the time being. Maybe silence it, and have it enabled
> with a 'debug' module param ? Paul ?

In the early days of frequency management and K8, I really wanted the
debug stuff there to help with problems in the field - the debug spew
really did help out with all of the buggy BIOSs that reported bogus
data. Things are more stable now, and it has outlived its usefulness,
it will be majorly cleaned up in the new driver (see below).

Perhaps I will put out a separate debug utility or something like that.

>  >  
>  >  		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;
>  >  		}
>  > the test probably should be numps <= 1 only, but it does 
> not matter as
>  > we force numps = batps]
> 
> 1 state on battery sounds odd. Buggy BIOS ?

No, it can be valid. The capability was put into the spec to allow
a platform manufacturer to have a system where the battery pack
could not power the system at full speed. It is legit (only the
platform manufacturer can say whether it is correct or not) to 
say that operation must be restricted to the lowest frequency when 
mains power is not available. Add up the current draw from all the
devices including the processor, and look at the current that the
battery is capable of supplying ...

I have a totally new driver, that I am hoping to release within about
a month. (I did target the end of the year, but I got distracted on
some other stuff). The new driver :
  - uses ACPI to figure out the available p-states. I have seen a *lot*
    of buggy BIOSs where the PSB/PST info is wrong or missing,
  - uses ACPI to handle battery / mains power transitions,
and some other clean ups.

I would appreciate some advice on a question ... should I leave the old
non-ACPI capability there for those people who do not want to enable ACPI
in the kernel ? If so, is this a big ifdef, or is there a better way to do
it ? Or should I just say that it is dependent on ACPI, got to have ACPI ?

Paul.



^ permalink raw reply	[flat|nested] 35+ messages in thread
* RE: Cleanups for powernow-k8
@ 2004-01-14 14:49 ` paul.devriendt
  0 siblings, 0 replies; 35+ messages in thread
From: paul.devriendt @ 2004-01-14 14:49 UTC (permalink / raw)
  To: linux; +Cc: pavel, davej, mark.langsdorf, cpufreq, linux-kernel

>> hardware. Dominik sent me some great patches to use the cpufreq table support 
>> and remove some redundant code - let me know if you do not have them and
>> want me to forward them. They work great.
> 
> I've rediffed them for 2.6.1 and put them here:
> 
> http://www.brodo.de/patches/2004_01_14/powernow-k8-2.6.1-freq_table_1
> 
> http://www.brodo.de/patches/2004_01_14/powernow-k8-2.6.1-freq_table_2
> 
> http://www.brodo.de/patches/2004_01_14/powernow-k8-2.6.1-freq_table_3
> 
> http://www.brodo.de/patches/2004_01_14/powernow-k8-2.6.1-freq_table_4
> 
> http://www.brodo.de/patches/2004_01_14/powernow-k8-2.6.1-freq_table_5
> 
> Paul: do you agree to them being merged?
> 
> 	Dominik

Absolutely. I am planning on using them also in the new ACPI based driver,
along with your acpi-perflib.

What is your progress on getting acpi-perflib merged into the kernel so that
an additional patch is not needed ?

Paul.



^ permalink raw reply	[flat|nested] 35+ messages in thread
* RE: Cleanups for powernow-k8
@ 2004-01-14  3:39 ` paul.devriendt
  0 siblings, 0 replies; 35+ messages in thread
From: paul.devriendt @ 2004-01-14  3:39 UTC (permalink / raw)
  To: davej; +Cc: pavel, cpufreq, linux, linux-kernel, mark.langsdorf

>> Dave had a good idea of a minimal ACPI parser for trying to retrieve the
>> table of p-states from an ACPI BIOS without needing the full AML interpreter.
>> I will see if I can get that to work in powernow-k8-acpi 
>  
> If done properly, that parsing code could be shared by the K7 
> driver too.
> 
> 	Dave

Agreed. Function in a header file? Don't want the drivers attempting to
call each other at runtime.


 


^ permalink raw reply	[flat|nested] 35+ messages in thread
* RE: Cleanups for powernow-k8
@ 2004-01-14  2:49 ` paul.devriendt
  0 siblings, 0 replies; 35+ messages in thread
From: paul.devriendt @ 2004-01-14  2:49 UTC (permalink / raw)
  To: pavel; +Cc: davej, cpufreq, linux, linux-kernel, mark.langsdorf

> I'd suggest to leave old driver there (possibly let me clean it up
> ;-)))), and create new powernow-k8-acpi, or powernow-acpi or something
> like that.
> 								Pavel

Keeping the old driver around for machines without ACPI support or with
broken ACPI support sounds reasonable to me. Please feel free to clean
it up. Feel free to send it to me if you want me to test it on additional
hardware. Dominik sent me some great patches to use the cpufreq table support 
and remove some redundant code - let me know if you do not have them and
want me to forward them. They work great.

powernow-k8-acpi sounds like a fine plan to me. Keeping the k8 in the name 
is goodness.

Dave had a good idea of a minimal ACPI parser for trying to retrieve the
table of p-states from an ACPI BIOS without needing the full AML interpreter.
I will see if I can get that to work in powernow-k8-acpi ... that way, there
is the best chance of being able to work around a broken BIOS. I hate broken
BIOSs and would rather see the BIOS vendor fix the BIOS, but that does not
always seem to happen.

Paul.


^ permalink raw reply	[flat|nested] 35+ messages in thread
* Cleanups for powernow-k8
@ 2004-01-13 21:51 Pavel Machek
  2004-01-13 21:59   ` Dave Jones
  0 siblings, 1 reply; 35+ messages in thread
From: Pavel Machek @ 2004-01-13 21:51 UTC (permalink / raw)
  To: davej, cpufreq, linux, kernel list, paul.devriendt

Hi!

powernow-k8 uses strange kind of comments, and is way too
verbose. Also powernow-k7 should just shut up when it is monolithic
kernel and cpu is not k7.

[Oh and look at this:
@@ -637,6 +629,7 @@
 		}
 
 		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;
 		}
the test probably should be numps <= 1 only, but it does not matter as
we force numps = batps]

Please apply,
								Pavel

--- tmp/linux/arch/i386/kernel/cpu/cpufreq/powernow-k7.c	2004-01-09 20:24:13.000000000 +0100
+++ linux/arch/i386/kernel/cpu/cpufreq/powernow-k7.c	2004-01-13 21:53:29.000000000 +0100
@@ -91,18 +91,13 @@
 	struct cpuinfo_x86 *c = cpu_data;
 	unsigned int maxei, eax, ebx, ecx, edx;
 
-	if (c->x86_vendor != X86_VENDOR_AMD) {
-		printk (KERN_INFO PFX "AMD processor not detected.\n");
-		return 0;
-	}
-
-	if (c->x86 !=6) {
+	if ((c->x86_vendor != X86_VENDOR_AMD) || (c->x86 !=6)) {
+#ifdef MODULE
 		printk (KERN_INFO PFX "This module only works with AMD K7 CPUs\n");
+#endif
 		return 0;
 	}
 
-	printk (KERN_INFO PFX "AMD K7 CPU detected.\n");
-
 	if ((c->x86_model == 6) && (c->x86_mask == 0)) {
 		printk (KERN_INFO PFX "K7 660[A0] core detected, enabling errata workarounds\n");
 		have_a0 = 1;
--- tmp/linux/arch/i386/kernel/cpu/cpufreq/powernow-k8.c	2004-01-09 20:24:13.000000000 +0100
+++ linux/arch/i386/kernel/cpu/cpufreq/powernow-k8.c	2004-01-13 22:24:08.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
@@ -107,11 +107,13 @@
 	}
 }
 
-/* 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)
 {
@@ -134,29 +136,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 +273,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 +320,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 +506,9 @@
 	}
 
 	if (c->x86_vendor != X86_VENDOR_AMD) {
+#ifdef MODULE
 		printk(KERN_INFO PFX "Not an AMD processor\n");
+#endif
 		return 0;
 	}
 
@@ -533,9 +541,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;
 }
 
@@ -573,33 +579,19 @@
 		}
 
 		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) {
@@ -718,8 +711,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 +980,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?]

^ permalink raw reply	[flat|nested] 35+ messages in thread
end of thread, other threads:[~2004-01-14 15:22 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-01-13 22:37 Cleanups for powernow-k8 paul.devriendt
2004-01-13 22:37 ` paul.devriendt
2004-01-13 23:06 ` Dave Jones
2004-01-13 23:06   ` Dave Jones
2004-01-14  9:33   ` Ducrot Bruno
2004-01-14 10:10   ` Dominik Brodowski
2004-01-14 10:10     ` Dominik Brodowski
2004-01-14 15:21     ` Dave Jones
2004-01-14  0:03 ` Pavel Machek
2004-01-14  0:03   ` Pavel Machek
2004-01-14  9:17 ` Ducrot Bruno
2004-01-14  9:17   ` Ducrot Bruno
  -- strict thread matches above, loose matches on Subject: below --
2004-01-14 14:49 paul.devriendt
2004-01-14 14:49 ` paul.devriendt
2004-01-14 15:08 ` Dominik Brodowski
2004-01-14 15:08   ` Dominik Brodowski
2004-01-14  3:39 paul.devriendt
2004-01-14  3:39 ` paul.devriendt
2004-01-14  3:42 ` Dave Jones
2004-01-14  3:42   ` Dave Jones
2004-01-14  9:01   ` Pavel Machek
2004-01-14  9:25     ` Dominik Brodowski
2004-01-14  9:25       ` Dominik Brodowski
2004-01-14  9:11   ` Dominik Brodowski
2004-01-14  9:11     ` Dominik Brodowski
2004-01-14  2:49 paul.devriendt
2004-01-14  2:49 ` paul.devriendt
2004-01-14  3:33 ` Dave Jones
2004-01-14  3:33   ` Dave Jones
2004-01-14 10:24 ` Dominik Brodowski
2004-01-14 10:24   ` Dominik Brodowski
2004-01-13 21:51 Pavel Machek
2004-01-13 21:59 ` Dave Jones
2004-01-13 21:59   ` Dave Jones
2004-01-13 23:59   ` Pavel Machek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.