Skip to contentSkip to conversation

powernow-k7 acpi support.

PATCH In review Bruno Ducrot 3 participants 11 messages Feb 9, 2004 – Feb 10, 2004

lore
Share & tools
lore

Bruno Ducrot

author
#0 lore raw

PATCH 1/1 powernow-k7 acpi support.

Hi Dave, Dominik, list,

Since now linux-2.6 bk tree do have necessary patches from Dominik for, this is a possible version in order to get the powernow table from ACPI, if the legacy method fail, or if the laptop is blacklisted. The following patch do only include the static function powernow_acpi_init() that have to be called in order to get the config stuff from acpi.

By now, there are different ways to use it. The present patch do not include any of them.

1- Include the powernow_acpi_init() as the first call in powernow_decode_bios(), so that ACPI is used first, then if it failed, the legacy method is used,
or
2- Include the powernow_acpi_init() at powernow_decode_bios() just before the 3 printk's stating to write that you should see
http://www.codemonkey.org.uk/projects/cpufreq/...

I guess 2- is 'better', but I would like to get some thought, though.

arch/i386/kernel/cpu/cpufreq/powernow-k7.c +120 -0
@@ -27,6 +27,11 @@
 #include <asm/io.h>
 #include <asm/system.h>
 
+#ifdef CONFIG_ACPI_PROCESSOR
+#include <linux/acpi.h>
+#include <acpi/processor.h>
+#endif
+
 #include "powernow-k7.h"
 
 #define DEBUG

@@ -57,6 +62,17 @@
 	u8 numpstates;
 };
 
+#ifdef CONFIG_ACPI_PROCESSOR
+union powernow_acpi_control_t {
+	struct {
+		unsigned long fid:5,
+		vid:5,
+		sgtc:20,
+		res1:2;
+	} bits;
+	unsigned long val;
+};
+#endif
 
 /* divide by 1000 to get VID. */
 static int mobile_vid_table[32] = {

@@ -309,6 +325,110 @@
 	return 0;
 }
 
+#ifdef CONFIG_ACPI_PROCESSOR
+
+static int powernow_acpi_init(void)
+{
+	int i;
+	int retval = 0;
+	struct acpi_pct_register *r;
+	struct acpi_processor_performance p;
+
+	memset(&p, 0, sizeof (p));
+
+	if (powernow_table != NULL)
+		return -EINVAL;
+
+	printk("1\n");
+	if (acpi_processor_register_performance(&p, 0))
+		return -EIO;
+	printk("2\n");
+
+	r = &p.control_register;
+	if (r->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		retval = -ENODEV;
+		goto out;
+	}
+	r = &p.status_register;
+	if (r->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		retval = -ENODEV;
+		goto out;
+	}
+
+	number_scales = p.state_count;
+
+	if (number_scales < 2) {
+		retval = -ENODEV;
+		goto out;
+	}
+
+	powernow_table = kmalloc((number_scales + 1) * (sizeof(struct cpufreq_frequency_table)), GFP_KERNEL);
+	if (!powernow_table) {
+		retval = -ENOMEM;
+		goto out;
+	}
+
+	memset(powernow_table, 0, ((number_scales + 1) * sizeof(struct cpufreq_frequency_table)));
+
+	fsb = 100;	/* XXX fix me */
+	latency = 10000;	/* XXX don't trust bios writers.. */
+
+	for (i = 0; i < number_scales; i++) {
+		union powernow_acpi_control_t pc;
+		u8 fid, vid;
+		unsigned int speed;
+
+		pc.val = (unsigned long) p.states[i].control;
+		dprintk (KERN_INFO PFX "acpi:  P%d: %d MHz, %d mW, %d uS, control %08x, status %08x, vid: %02x fid: %02x SGTC: %d\n",
+			 i,
+			 (u32) p.states[i].core_frequency,
+			 (u32) p.states[i].power,
+			 (u32) p.states[i].transition_latency,
+			 (u32) p.states[i].control,
+			 (u32) p.states[i].status,
+			 pc.bits.vid,
+			 pc.bits.fid,
+			 pc.bits.sgtc);
+
+		vid = pc.bits.vid;
+		fid = pc.bits.fid;
+
+		powernow_table[i].frequency = fsb * fid_codes[fid] * 100;
+		powernow_table[i].index = fid; /* lower 8 bits */
+		powernow_table[i].index |= (vid << 8); /* upper 8 bits */
+
+		speed = fsb * (fid_codes[fid]/10);
+		if ((fid_codes[fid] % 10)==5) {
+			speed += fsb/2;
+			if (have_a0 == 1)
+				powernow_table[i].frequency = CPUFREQ_ENTRY_INVALID;
+		}
+
+		dprintk (KERN_INFO PFX "   FID: 0x%x (%d.%dx [%dMHz])\t", fid,
+			fid_codes[fid] / 10, fid_codes[fid] % 10, speed);
+		dprintk ("VID: 0x%x (%d.%03dV)\n", vid,	mobile_vid_table[vid]/1000,
+			mobile_vid_table[vid]%1000);
+
+		if (speed < minimum_speed)
+			minimum_speed = speed;
+		if (speed > maximum_speed)
+			maximum_speed = speed;
+	}
+	powernow_table[i].frequency = CPUFREQ_TABLE_END;
+	powernow_table[i].index = 0;
+
+	/*
+	 * We don't need no more acpi.
+	 * XXX is that true?
+	 */
+
+out:
+	acpi_processor_unregister_performance(&p, 0);
+
+	return retval;
+}
+#endif
+
 static int powernow_decode_bios (int maxfid, int startvid)
 {
 	struct psb_s *psb;
Signature · 4 lines
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.
#1 lore raw
Quoted 9 lines

1- Include the powernow_acpi_init() as the first call in powernow_decode_bios(), so that ACPI is used first, then if it failed, the legacy method is used,
or
2- Include the powernow_acpi_init() at powernow_decode_bios() just before the 3 printk's stating to write that you should see
http://www.codemonkey.org.uk/projects/cpufreq/...

I guess 2- is 'better', but I would like to get some thought, though.

Hm. That's difficult to decide, also for the speedstep-centrino case... I'd like to hear some opinions from ACPI and AMD guys first before we decide...

arch/i386/kernel/cpu/cpufreq/powernow-k7.c line 348

Quoted 7 lines
arch/i386/kernel/cpu/cpufreq/powernow-k7.c +7 -0
+	printk("1\n");
+	if (acpi_processor_register_performance(&p, 0))
+		return -EIO;
+	printk("2\n");
+
+	r = &p.control_register;
+	if (r->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {

How about

	if (p.control_register.space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {

?

arch/i386/kernel/cpu/cpufreq/powernow-k7.c line 353

Quoted 5 lines
arch/i386/kernel/cpu/cpufreq/powernow-k7.c +5 -0
+		retval = -ENODEV;
+		goto out;
+	}
+	r = &p.status_register;
+	if (r->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {

same here...

arch/i386/kernel/cpu/cpufreq/powernow-k7.c line 373

Quoted 1 line
arch/i386/kernel/cpu/cpufreq/powernow-k7.c +1 -0
+	fsb = 100;	/* XXX fix me */

Any idea on how to determine the FSB?

arch/i386/kernel/cpu/cpufreq/powernow-k7.c line 374

Quoted 1 line
arch/i386/kernel/cpu/cpufreq/powernow-k7.c +1 -0
+	latency = 10000;	/* XXX don't trust bios writers.. */

heh...

arch/i386/kernel/cpu/cpufreq/powernow-k7.c line 423

Quoted 4 lines
arch/i386/kernel/cpu/cpufreq/powernow-k7.c +4 -0
+	/*
+	 * We don't need no more acpi.
+	 * XXX is that true?
+	 */

No, it's not true. If we use ACPI, we need to keep it registered until we stop using it. Reasons:
- keep buggy drivers from interfering [e.g. other cpufreq drivers which register with ACPI first and with cpufreq later, or avoid registering with cpufreq because of being proprietary...]
- honor _PPC limits
- allow for ACPI interface (additions):
This includes the /proc/acpi/processor/./performance interface which people still tend to use, and additions to /sys/devices/system/cpu/cpu0/cpufreq/ still to be written.

Dominik

_______________________________________________
Cpufreq mailing list
[email protected]
http://www.linux.org.uk/mailman/listinfo/cpufreq

author
#2 lore raw
Quoted 8 lines · Dominik Brodowski
Quoted 8 lines

1- Include the powernow_acpi_init() as the first call in powernow_decode_bios(), so that ACPI is used first, then if it failed, the legacy method is used,
or
2- Include the powernow_acpi_init() at powernow_decode_bios() just before the 3 printk's stating to write that you should see
http://www.codemonkey.org.uk/projects/cpufreq/...

Quoted 4 lines
Quoted 1 line

I guess 2- is 'better', but I would like to get some thought, though.

Hm. That's difficult to decide, also for the speedstep-centrino case... I'd like to hear some opinions from ACPI and AMD guys first before we decide...

Well, ACPI guys would say you to use ACPI for, but I prefer the legacy method because otherwise I loose 3 power states ;)

Quoted 2 lines

arch/i386/kernel/cpu/cpufreq/powernow-k7.c line 342

Quoted 2 lines
arch/i386/kernel/cpu/cpufreq/powernow-k7.c +1 -0
+	printk("1\n");

Sound like I forgot to remove that, sorry..

Quoted 9 lines

arch/i386/kernel/cpu/cpufreq/powernow-k7.c line 348

Quoted 6 lines
arch/i386/kernel/cpu/cpufreq/powernow-k7.c +6 -0
+	if (acpi_processor_register_performance(&p, 0))
+		return -EIO;
+	printk("2\n");
+
+	r = &p.control_register;
+	if (r->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {

How about

	if (p.control_register.space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {

because I use p.control_register at least 2 times, and when that happens I prefer to introduce another variable, because it's more easier to read IMHO.

Quoted 8 lines

arch/i386/kernel/cpu/cpufreq/powernow-k7.c line 353

Quoted 6 lines
arch/i386/kernel/cpu/cpufreq/powernow-k7.c +5 -0
+		retval = -ENODEV;
+		goto out;
+	}
+	r = &p.status_register;
+	if (r->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {

same here...

and that the second one ;)

Quoted 4 lines

arch/i386/kernel/cpu/cpufreq/powernow-k7.c line 373

Quoted 2 lines
arch/i386/kernel/cpu/cpufreq/powernow-k7.c +1 -0
+	fsb = 100;	/* XXX fix me */

Any idea on how to determine the FSB?

Unfortunately no. I don't have all the AMD processor specification for this.

Quoted 4 lines

arch/i386/kernel/cpu/cpufreq/powernow-k7.c line 374

Quoted 2 lines
arch/i386/kernel/cpu/cpufreq/powernow-k7.c +1 -0
+	latency = 10000;	/* XXX don't trust bios writers.. */

heh...

OK, I'm perhaps a little bit rude here. I will rewrite that.

Quoted 17 lines

arch/i386/kernel/cpu/cpufreq/powernow-k7.c line 423

Quoted 5 lines
arch/i386/kernel/cpu/cpufreq/powernow-k7.c +4 -0
+	/*
+	 * We don't need no more acpi.
+	 * XXX is that true?
+	 */

No, it's not true. If we use ACPI, we need to keep it registered until we stop using it. Reasons:
- keep buggy drivers from interfering [e.g. other cpufreq drivers which register with ACPI first and with cpufreq later, or avoid registering with cpufreq because of being proprietary...]
- honor _PPC limits
- allow for ACPI interface (additions):
This includes the /proc/acpi/processor/./performance interface which people still tend to use, and additions to /sys/devices/system/cpu/cpu0/cpufreq/ still to be written.

Ok (actually, I don't think _PPC is supported on most K7 mobo, since it return 0 in almost all DSDT, but well, I should consider though in order to be fully ACPI compliant).

Thanks,

Signature · 4 lines
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.
author
#3 lore raw
Quoted 5 lines · Bruno Ducrot
Quoted 2 lines

How about

	if (p.control_register.space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {

because I use p.control_register at least 2 times, and when that happens I prefer to introduce another variable, because it's more easier to read IMHO.

Actually, what I wrotte is stupid in that case, and that obfuscate more. I will do what you suggest.

Signature · 4 lines
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.
author
#4 lore raw
In reply to Dominik Brodowski

How about that one? By now, I'm a little puzzled with the fsb stuff, though. A first idea may be to look into what say ACPI about the frequency, and deduce the fsb from, but I don't like it.

arch/i386/kernel/cpu/cpufreq/powernow-k7.c renamed from powernow-k7.c +146 -0
@@ -27,6 +27,11 @@
 #include <asm/io.h>
 #include <asm/system.h>
 
+#ifdef CONFIG_ACPI_PROCESSOR
+#include <linux/acpi.h>
+#include <acpi/processor.h>
+#endif
+
 #include "powernow-k7.h"
 
 #define DEBUG

@@ -57,6 +62,17 @@
 	u8 numpstates;
 };
 
+#ifdef CONFIG_ACPI_PROCESSOR
+union powernow_acpi_control_t {
+	struct {
+		unsigned long fid:5,
+		vid:5,
+		sgtc:20,
+		res1:2;
+	} bits;
+	unsigned long val;
+};
+#endif
 
 /* divide by 1000 to get VID. */
 static int mobile_vid_table[32] = {

@@ -308,6 +325,132 @@
 	return 0;
 }
 
+#ifdef CONFIG_ACPI_PROCESSOR
+
+struct acpi_processor_performance *acpi_processor_perf;
+
+static int powernow_acpi_init(void)
+{
+	int i;
+	int retval = 0;
+
+	if (acpi_processor_perf != NULL)
+		return -EINVAL;
+
+	if (powernow_table != NULL)
+		return -EINVAL;
+
+	acpi_processor_perf = kmalloc(sizeof(struct acpi_processor_performance),
+				      GFP_KERNEL);
+
+	if (!acpi_processor_perf)
+		return -ENOMEM;
+
+	memset(acpi_processor_perf, 0, sizeof(struct acpi_processor_performance));
+
+	if (acpi_processor_register_performance(acpi_processor_perf, 0)) {
+		retval = -EIO;
+		goto err1;
+	}
+
+	if (acpi_processor_perf->control_register.space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		retval = -ENODEV;
+		goto err2;
+	}
+
+	if (acpi_processor_perf->status_register.space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		retval = -ENODEV;
+		goto err2;
+	}
+
+	number_scales = acpi_processor_perf->state_count;
+
+	if (number_scales < 2) {
+		retval = -ENODEV;
+		goto err2;
+	}
+
+	powernow_table = kmalloc((number_scales + 1) * (sizeof(struct cpufreq_frequency_table)), GFP_KERNEL);
+	if (!powernow_table) {
+		retval = -ENOMEM;
+		goto err2;
+	}
+
+	memset(powernow_table, 0, ((number_scales + 1) * sizeof(struct cpufreq_frequency_table)));
+
+	fsb = 100;	/* XXX fix me */
+	latency = 0;
+
+	for (i = 0; i < number_scales; i++) {
+		u8 fid, vid;
+		union powernow_acpi_control_t pc;
+		unsigned int speed;
+
+		pc.val = (unsigned long) acpi_processor_perf->states[i].control;
+		dprintk (KERN_INFO PFX "acpi:  P%d: %d MHz, %d mW, %d uS, control %08x, status %08x, vid: %02x fid: %02x SGTC: %d\n",
+			 i,
+			 (u32) acpi_processor_perf->states[i].core_frequency,
+			 (u32) acpi_processor_perf->states[i].power,
+			 (u32) acpi_processor_perf->states[i].transition_latency,
+			 (u32) acpi_processor_perf->states[i].control,
+			 (u32) acpi_processor_perf->states[i].status,
+			 pc.bits.vid,
+			 pc.bits.fid,
+			 pc.bits.sgtc);
+
+		vid = pc.bits.vid;
+		fid = pc.bits.fid;
+
+		powernow_table[i].frequency = fsb * fid_codes[fid] * 100;
+		powernow_table[i].index = fid; /* lower 8 bits */
+		powernow_table[i].index |= (vid << 8); /* upper 8 bits */
+
+		speed = fsb * (fid_codes[fid]/10);
+		if ((fid_codes[fid] % 10)==5) {
+			speed += fsb/2;
+			if (have_a0 == 1)
+				powernow_table[i].frequency = CPUFREQ_ENTRY_INVALID;
+		}
+
+		dprintk (KERN_INFO PFX "   FID: 0x%x (%d.%dx [%dMHz])\t", fid,
+			fid_codes[fid] / 10, fid_codes[fid] % 10, speed);
+		dprintk ("VID: 0x%x (%d.%03dV)\n", vid,	mobile_vid_table[vid]/1000,
+			mobile_vid_table[vid]%1000);
+
+		if (latency < pc.bits.sgtc)
+			latency = pc.bits.sgtc;
+
+		if (speed < minimum_speed)
+			minimum_speed = speed;
+		if (speed > maximum_speed)
+			maximum_speed = speed;
+	}
+
+	/* Validate latency.  The latency should be at least
+	 * 10000 and must fit in a 20 bit register.
+	 */
+	if ((latency & ((~((~0u) << 20)))) < 10000) {
+		printk(KERN_WARNING PFX "broken latency in ACPI table.\n");
+		printk(KERN_INFO PFX "ACPI set settling time to %d.%02d microseconds.  "
+			"Should be at least 100. Correcting.\n",
+			latency/100,
+			latency%100);
+		latency = 10000;
+	}
+	powernow_table[i].frequency = CPUFREQ_TABLE_END;
+	powernow_table[i].index = 0;
+
+	return 0;
+
+err2:
+	acpi_processor_unregister_performance(acpi_processor_perf, 0);
+err1:
+	kfree(acpi_processor_perf);
+	acpi_processor_perf = NULL;
+	return retval;
+}
+#endif
+
 static int powernow_decode_bios (int maxfid, int startvid)
 {
 	struct psb_s *psb;

@@ -640,6 +804,10 @@
 
 static void __exit powernow_exit (void)
 {
+	if (acpi_processor_perf) {
+		acpi_processor_unregister_performance(acpi_processor_perf, 0);
+		kfree(acpi_processor_perf);
+	}
 	cpufreq_unregister_driver(&powernow_driver);
 	if (powernow_table)
 		kfree(powernow_table);
Signature · 4 lines
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.
#5 lore raw
Quoted 2 lines · Bruno Ducrot

How about that one?

Looks much better, IMO.

Quoted 3 lines

By now, I'm a little puzzled with the fsb stuff,
though. A first idea may be to look into what say ACPI about the frequency, and deduce the fsb from, but I don't like it.

So you wouldn't like

	fsb = (acpi_processor_perf->states[i].core_frequency * 10) / fid_codes[fid]

? Why?

Dominik

_______________________________________________
Cpufreq mailing list
[email protected]
http://www.linux.org.uk/mailman/listinfo/cpufreq

author
#6 lore raw
Quoted 5 lines · Dominik Brodowski
Quoted 2 lines · Bruno Ducrot

How about that one?

Looks much better, IMO.

Thanks.

Quoted 10 lines
Quoted 4 lines

By now, I'm a little puzzled with the fsb stuff,
though. A first idea may be to look into what say ACPI about the frequency, and deduce the fsb from, but I don't like it.

So you wouldn't like

	fsb = (acpi_processor_perf->states[i].core_frequency * 10) / fid_codes[fid]

? Why?

Because I would much prefer a generic function get_amd_fsb(), independant from bios.

Signature · 4 lines
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.
#8 lore raw
Quoted 5 lines · Dominik Brodowski
Quoted 1 line · Bruno Ducrot

Ok, you win. That one is ok ?

Looks good as well. All we have to decide is where to call these functions, though... and Davej has to decide whether he likes it, too :-)

Looks good to me, though I'm missing the part where you wire this up to the rest of the code. I see two possibilities.

- fallback to acpi when we detect bad PST's.
- module parameter to force usage of acpi (override PST)

Dave

author
#9 lore raw
Quoted 9 lines · Dave Jones
Quoted 5 lines · Dominik Brodowski
Quoted 1 line · Bruno Ducrot

Ok, you win. That one is ok ?

Looks good as well. All we have to decide is where to call these functions, though... and Davej has to decide whether he likes it, too :-)

Looks good to me, though I'm missing the part where you wire this up to the rest of the code. I see two possibilities.

Well, that need to be decided at first. I'm acually testing it by calling that function in powernow_decode_bios() with something like that:

arch/i386/kernel/cpu/cpufreq/powernow-k7.c +3 -0
@@ -426,6 +426,9 @@
 	unsigned int etuple;
 	unsigned int ret;
 
+	if (powernow_acpi_init() == 0)
+		return 0;
+
 	etuple = cpuid_eax(0x80000001);
 	etuple &= 0xf00;
 	etuple |= (c->x86_model<<4)|(c->x86_mask);

but I guess this is not what people want probably, especially if there are good PST's.

Quoted 1 line

- fallback to acpi when we detect bad PST's.

That sound good.

Quoted 1 line

- module parameter to force usage of acpi (override PST)

Yes, or check if the laptop is blacklisted.

Signature · 4 lines
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.
author
#10 lore raw
In reply to Dominik Brodowski
Quoted 6 lines · Dominik Brodowski
Quoted 2 lines · Bruno Ducrot

How about that one?

Looks much better, IMO.

Quoted 8 lines
Quoted 3 lines

By now, I'm a little puzzled with the fsb stuff,
though. A first idea may be to look into what say ACPI about the frequency, and deduce the fsb from, but I don't like it.

So you wouldn't like

	fsb = (acpi_processor_perf->states[i].core_frequency * 10) / fid_codes[fid]

Ok, you win. That one is ok ?

arch/i386/kernel/cpu/cpufreq/powernow-k7.c renamed from arch/i386/kernel/cpu/cpufreq/powernow-k7.c.orig +13 -2
@@ -333,6 +333,7 @@
 {
 	int i;
 	int retval = 0;
+	union powernow_acpi_control_t pc;
 
 	if (acpi_processor_perf != NULL)
 		return -EINVAL;

@@ -378,12 +379,22 @@
 
 	memset(powernow_table, 0, ((number_scales + 1) * sizeof(struct cpufreq_frequency_table)));
 
-	fsb = 100;	/* XXX fix me */
+	pc.val = (unsigned long) acpi_processor_perf->states[0].control;
+	fsb = (10 * (u32) acpi_processor_perf->states[0].core_frequency) / (fid_codes[pc.bits.fid]);
+	if (fsb > 133)
+		fsb = 133;
+	if (fsb < 100)
+		fsb = 100;
+
+	if (100 + 133 < 2*fsb)
+		fsb = 133;
+	else
+		fsb = 100;
+
 	latency = 0;
 
 	for (i = 0; i < number_scales; i++) {
 		u8 fid, vid;
-		union powernow_acpi_control_t pc;
 		unsigned int speed;
 
 		pc.val = (unsigned long) acpi_processor_perf->states[i].control;
Signature · 4 lines
Bruno Ducrot

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