my dothan didn't work with cpufreq...

my dothan didn't work with cpufreq...

[Damien Marchal]

Hi,

I have a problem to use cpufreq with my Dothan based computer. During 
initialisation i get the following message:

speedstep-centrino: found unsupported CPU with Enhanced SpeedStep: send 
/proc/cpuinfo to Jeremy Fitzhardinge <jeremy@goop.org>

What I did. As i received no feedback I look inside the cpufreq 
sourcecode that throw the error and I tried to fixed it myself. I 
compared it with the content of my /proc/cpuinfo:

cpu family      : 6
model             : 13
model name   : Intel(R) Pentium(R) M processor 1.50GHz
stepping        : 6
cpu MHz         : 1500.834
cache size      : 64 KB

I located a correct cpu_id cpu_id_dothan_b0 but I wasn't able to locate 
were is it used (it  seems to differ from the banias  code).

In addition in the centrino_cpu_init_table(struct cpufreq_policy 
*policy), the loop that detect the processor failed as my processor 
string contains 1.50Ghz and not 1500Mhz (maybe an issue related to my 
Asus BIOS).

In fact, I could try to make a patch for making my processor accepted by 
the centrino checking code, but after this I ignore what will happend as 
no predefined tables frequency/voltage exists like with the Banias. Will 
it burn my computer :) or will it nicely use acpi/ich4 in some way ?

Cheers,
Damien Marchal.

Re: my dothan didn't work with cpufreq...

[Dominik Brodowski]

Hi,

On Mon, Jul 12, 2004 at 10:54:35PM +0200, Damien Marchal wrote:
> speedstep-centrino: found unsupported CPU with Enhanced SpeedStep: send 
> /proc/cpuinfo to Jeremy Fitzhardinge <jeremy@goop.org>
> 
> What I did. As i received no feedback I look inside the cpufreq 
> sourcecode that throw the error and I tried to fixed it myself. I 
> compared it with the content of my /proc/cpuinfo:
> 
> cpu family      : 6
> model             : 13
> model name   : Intel(R) Pentium(R) M processor 1.50GHz
> stepping        : 6
> cpu MHz         : 1500.834
> cache size      : 64 KB
> 
> I located a correct cpu_id cpu_id_dothan_b0 but I wasn't able to locate 
> were is it used (it  seems to differ from the banias  code).

Dothan's only work if X86_SPEEDSTEP_CENTRINO_ACPI is enabled in your kernel
configuartion. Did you enable it? You need to set ACPI_PROCESSOR to "y" if
you want to use it with SPEEDSTEP_CENTRINO="y", or ACPI_PROCESSOR="y" or
ACPI_PROCESSOR="m" if you use it with SPEEDSTEP_CENTRINO="m".

> In addition in the centrino_cpu_init_table(struct cpufreq_policy 
> *policy), the loop that detect the processor failed as my processor 
> string contains 1.50Ghz and not 1500Mhz (maybe an issue related to my 
> Asus BIOS).

No, that changed from Banias to Dothan CPUs in general. However, the "table"
method isn't available for Dothans.

> In fact, I could try to make a patch for making my processor accepted by 
> the centrino checking code, but after this I ignore what will happend as 
> no predefined tables frequency/voltage exists like with the Banias. Will 
> it burn my computer :)

Maybe. The frequency/voltage pairs for Dothans are different to those of
Banias. A built-in table isn't possible for Dothans, IIRC, as even if you 
know the CPUID you can't know for 100% which pairs are valid -- it depends
on some other info not in the CPUID.

If SPEEDSTEP_CENTRINO doesn't work, you can also try out the ACPI P-States
cpufreq driver -- it tends to work on most Centrino (Banias or Dothan)
notebooks, offers the same frequency / voltage scalings but a slower
frequency & voltage scaling path (i.e. transitions take ~100us compared to
~10us, IIRC).

	Dominik

Re: my dothan didn't work with cpufreq...

[Damien Marchal]

Hi,

i check my .config and saw that the X86_SPEEDSTEP_CENTRINO_TABLE was 
always set to Y and it can be changed by make menuconfig.

By manually editing it to N give me a fully functional cpufreq module.

Thank for your help.

I quickly add an entry for this value in cpufreq/Kconfig:
----------------------------------------------------------------------------------------------------------------
config X86_SPEEDSTEP_CENTRINO_TABLE
        bool "Use predefined frequency/voltage pairs (Banias only)"
        depends on X86_SPEEDSTEP_CENTRINO
        default y
        help
                This only work for Banias processor. Dothan users must
                set this to N.

-------------------------------------------------------------------------------------

Cheers,
Damien.
Dominik Brodowski wrote:

>Hi,
>
>On Mon, Jul 12, 2004 at 10:54:35PM +0200, Damien Marchal wrote:
>  
>
>>speedstep-centrino: found unsupported CPU with Enhanced SpeedStep: send 
>>/proc/cpuinfo to Jeremy Fitzhardinge <jeremy@goop.org>
>>
>>What I did. As i received no feedback I look inside the cpufreq 
>>sourcecode that throw the error and I tried to fixed it myself. I 
>>compared it with the content of my /proc/cpuinfo:
>>
>>cpu family      : 6
>>model             : 13
>>model name   : Intel(R) Pentium(R) M processor 1.50GHz
>>stepping        : 6
>>cpu MHz         : 1500.834
>>cache size      : 64 KB
>>
>>I located a correct cpu_id cpu_id_dothan_b0 but I wasn't able to locate 
>>were is it used (it  seems to differ from the banias  code).
>>    
>>
>
>Dothan's only work if X86_SPEEDSTEP_CENTRINO_ACPI is enabled in your kernel
>configuartion. Did you enable it? You need to set ACPI_PROCESSOR to "y" if
>you want to use it with SPEEDSTEP_CENTRINO="y", or ACPI_PROCESSOR="y" or
>ACPI_PROCESSOR="m" if you use it with SPEEDSTEP_CENTRINO="m".
>
>  
>
>>In addition in the centrino_cpu_init_table(struct cpufreq_policy 
>>*policy), the loop that detect the processor failed as my processor 
>>string contains 1.50Ghz and not 1500Mhz (maybe an issue related to my 
>>Asus BIOS).
>>    
>>
>
>No, that changed from Banias to Dothan CPUs in general. However, the "table"
>method isn't available for Dothans.
>
>  
>
>>In fact, I could try to make a patch for making my processor accepted by 
>>the centrino checking code, but after this I ignore what will happend as 
>>no predefined tables frequency/voltage exists like with the Banias. Will 
>>it burn my computer :)
>>    
>>
>
>Maybe. The frequency/voltage pairs for Dothans are different to those of
>Banias. A built-in table isn't possible for Dothans, IIRC, as even if you 
>know the CPUID you can't know for 100% which pairs are valid -- it depends
>on some other info not in the CPUID.
>
>If SPEEDSTEP_CENTRINO doesn't work, you can also try out the ACPI P-States
>cpufreq driver -- it tends to work on most Centrino (Banias or Dothan)
>notebooks, offers the same frequency / voltage scalings but a slower
>frequency & voltage scaling path (i.e. transitions take ~100us compared to
>~10us, IIRC).
>
>	Dominik
>  
>

Re: Re: my dothan didn't work with cpufreq...

[Dominik Brodowski]

On Wed, Jul 14, 2004 at 12:50:09PM +0200, Damien Marchal wrote:
> Hi,
> 
> i check my .config and saw that the X86_SPEEDSTEP_CENTRINO_TABLE was 
> always set to Y and it can be changed by make menuconfig.
> 
> By manually editing it to N give me a fully functional cpufreq module.

Hm, that's strange. the tables are only used if ACPI fails... so saying Y to
ACPI seems to be more important (for Dothan users) than setting TABLE to N
to me...

	Dominik

Re: my dothan didn't work with cpufreq...

[Jeremy Fitzhardinge]

On Tue, 2004-07-13 at 11:49 +0200, Dominik Brodowski wrote:
> Maybe. The frequency/voltage pairs for Dothans are different to those of
> Banias. A built-in table isn't possible for Dothans, IIRC, as even if you 
> know the CPUID you can't know for 100% which pairs are valid -- it depends
> on some other info not in the CPUID.

I've been meaning to look into this to see if there's some way to add a
table method.  It's moderately tricky because there's two steppings of
Dothan, and the specs are not obvious to me (they seem to list 4
voltages per frequency point, which is confusing).

Anyway, I have a good collection of /proc/cpuinfos now, so I can pick
through them to see what common elements there are.

I guess it would also be good to get dumps from the ACPI tables.  What's
the best way to do that?

[ And for those who've mailed me and got nothing back - I'm sorry about
that.  I have been getting a lot of reports - people seem to be buying a
lot of laptops... ]

	J

Re: my dothan didn't work with cpufreq...

[Dominik Brodowski]

On Wed, Jul 21, 2004 at 06:55:20PM -0700, Jeremy Fitzhardinge wrote:
> I've been meaning to look into this to see if there's some way to add a
> table method.  It's moderately tricky because there's two steppings of
> Dothan, and the specs are not obvious to me (they seem to list 4
> voltages per frequency point, which is confusing).

AFAICS in some specs, there are identical-looking (CPUID, string) Dothans
with different voltage requirements, at least for the B1 stepping. Also, as
the usage of the ACPI P-State library in speedstep-centrino seems to work 
flawlessly on Dothans [at least I haven't heard otherwise], I'm tempted to 
remove the "(EXPERIMENTAL)" mark on it and possibly make it even the default. 
What do you think?

> I guess it would also be good to get dumps from the ACPI tables.  What's
> the best way to do that?

acpi_pdump with pdc=1 as parameter. This test module can be grabbed here:

http://www.brodo.de/patches/2004-04-06/cpufreq-acpi_pdump.c

	Dominik

Re: my dothan didn't work with cpufreq...

[Jeremy Fitzhardinge]

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

On Thu, 2004-07-22 at 08:04 +0200, Dominik Brodowski wrote:
> AFAICS in some specs, there are identical-looking (CPUID, string) Dothans
> with different voltage requirements, at least for the B1 stepping.

Yeah, I worked that out from a closer reading of the docs.  There really
doesn't seem to be any non-hacky way to work out which CPU you have, and
they seem to have non-overlapping specs (ie, you can't run a VID#D part
with VID#A voltages).  The hacky way to work it out would be to read
back the MSR and see what freq/voltage pair you have (which would only
work at max speed).

>  Also, as
> the usage of the ACPI P-State library in speedstep-centrino seems to work 
> flawlessly on Dothans [at least I haven't heard otherwise], I'm tempted to 
> remove the "(EXPERIMENTAL)" mark on it and possibly make it even the default. 
> What do you think?

Seems like a reasonable idea, though it would be nice for Dothan users
to not have an ACPI dependency.  I put together this patch to clean
things up a bit for Dothan users and to reduce the flow of mail I'm
getting.  I've tested it as much as I can without a Dothan on hand.

	J

[-- Attachment #2: no-cyrix-est.patch --]
[-- Type: text/x-patch, Size: 5853 bytes --]


A reduce-Jeremy's-mail patch:

 - Only Intel makes EST CPUs.  (Some Cyrix M IIs have the EST bit set - 
	I don't know what it means, but it isn't Enhanced Speedstep.)

 - If it's a known Dothan, but we're looking in the tables, give a
   useful message about using ACPI rather than mailing me.

 - Code cleanups:
  - Make the CPU ID stuff table driven
  - Turn centrino_verify_cpu_id into a proper boolean predicate

 - Diddle some whitespace


 arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c |   84 ++++++++++++----------
 1 files changed, 48 insertions(+), 36 deletions(-)

diff -puN arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c~no-cyrix-est arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c
--- local-2.6/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c~no-cyrix-est	2004-07-21 20:22:16.000000000 -0700
+++ local-2.6-jeremy/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c	2004-07-21 23:52:32.066762443 -0700
@@ -46,27 +46,19 @@ struct cpu_id
 	__u8	x86_mask;	/* stepping */
 };
 
-static const struct cpu_id cpu_id_banias = {
-	.x86_vendor = X86_VENDOR_INTEL,
-	.x86 = 6,
-	.x86_model = 9,
-	.x86_mask = 5,
-};
-
-static const struct cpu_id cpu_id_dothan_a1 = {
-	.x86_vendor = X86_VENDOR_INTEL,
-	.x86 = 6,
-	.x86_model = 13,
-	.x86_mask = 1,
-};
-
-static const struct cpu_id cpu_id_dothan_b0 = {
-	.x86_vendor = X86_VENDOR_INTEL,
-	.x86 = 6,
-	.x86_model = 13,
-	.x86_mask = 6,
+enum {
+	CPU_BANIAS,
+	CPU_DOTHAN_A1,
+	CPU_DOTHAN_B0,
 };
 
+static const struct cpu_id cpu_ids[] = {
+	[CPU_BANIAS]	= { X86_VENDOR_INTEL,	6,  9, 5 },
+	[CPU_DOTHAN_A1]	= { X86_VENDOR_INTEL,	6, 13, 1 },
+	[CPU_DOTHAN_B0]	= { X86_VENDOR_INTEL,	6, 13, 6 },
+};
+#define N_IDS	(sizeof(cpu_ids)/sizeof(cpu_ids[0]))
+
 struct cpu_model
 {
 	const struct cpu_id *cpu_id;
@@ -75,7 +67,7 @@ struct cpu_model
 
 	struct cpufreq_frequency_table *op_points; /* clock/voltage pairs */
 };
-static int centrino_verify_cpu_id(struct cpuinfo_x86 *c, const struct cpu_id *x);
+static int centrino_verify_cpu_id(const struct cpuinfo_x86 *c, const struct cpu_id *x);
 
 /* Operating points for current CPU */
 static struct cpu_model *centrino_model;
@@ -110,9 +102,9 @@ static struct cpufreq_frequency_table ba
 /* Ultra Low Voltage Intel Pentium M processor 1000MHz (Banias) */
 static struct cpufreq_frequency_table banias_1000[] =
 {
-	OP(600,  844),
-	OP(800,  972),
-	OP(900,  988),
+	OP(600,   844),
+	OP(800,   972),
+	OP(900,   988),
 	OP(1000, 1004),
 	{ .frequency = CPUFREQ_TABLE_END }
 };
@@ -206,13 +198,13 @@ static struct cpufreq_frequency_table ba
 	.max_freq	= (max)*1000,	\
 	.op_points	= banias_##max,	\
 }
-#define BANIAS(max)	_BANIAS(&cpu_id_banias, max, #max)
+#define BANIAS(max)	_BANIAS(&cpu_ids[CPU_BANIAS], max, #max)
 
 /* CPU models, their operating frequency range, and freq/voltage
    operating points */
 static struct cpu_model models[] = 
 {
-	_BANIAS(&cpu_id_banias, 900, " 900"),
+	_BANIAS(&cpu_ids[CPU_BANIAS], 900, " 900"),
 	BANIAS(1000),
 	BANIAS(1100),
 	BANIAS(1200),
@@ -221,6 +213,11 @@ static struct cpu_model models[] = 
 	BANIAS(1500),
 	BANIAS(1600),
 	BANIAS(1700),
+
+	/* NULL model_name is a wildcard */
+	{ &cpu_ids[CPU_DOTHAN_A1], NULL, 0, NULL },
+	{ &cpu_ids[CPU_DOTHAN_B0], NULL, 0, NULL },
+
 	{ NULL, }
 };
 #undef _BANIAS
@@ -231,17 +228,28 @@ static int centrino_cpu_init_table(struc
 	struct cpuinfo_x86 *cpu = &cpu_data[policy->cpu];
 	struct cpu_model *model;
 
-	for(model = models; model->model_name != NULL; model++)
-		if ((strcmp(cpu->x86_model_id, model->model_name) == 0) &&
-		    (!centrino_verify_cpu_id(cpu, model->cpu_id)))
+	for(model = models; model->cpu_id != NULL; model++)
+		if (centrino_verify_cpu_id(cpu, model->cpu_id) &&
+		    (model->model_name == NULL ||
+		     strcmp(cpu->x86_model_id, model->model_name) == 0))
 			break;
-	if (model->model_name == NULL) {
+	
+	if (model->cpu_id == NULL) {
+		/* No match at all */
 		printk(KERN_INFO PFX "no support for CPU model \"%s\": "
 		       "send /proc/cpuinfo to " MAINTAINER "\n",
 		       cpu->x86_model_id);
 		return -ENOENT;
 	}
 
+	if (model->op_points == NULL) {
+		/* Matched a non-match */
+		printk(KERN_INFO PFX "no table support for CPU model \"%s\": \n",
+		       cpu->x86_model_id);
+		printk(KERN_INFO PFX "try compiling with CONFIG_X86_SPEEDSTEP_CENTRINO_ACPI enabled\n");
+		return -ENOENT;
+	}
+		
 	centrino_model = model;
 		
 	printk(KERN_INFO PFX "found \"%s\": max frequency: %dkHz\n",
@@ -254,14 +262,14 @@ static int centrino_cpu_init_table(struc
 static inline int centrino_cpu_init_table(struct cpufreq_policy *policy) { return -ENODEV; }
 #endif /* CONFIG_X86_SPEEDSTEP_CENTRINO_TABLE */
 
-static int centrino_verify_cpu_id(struct cpuinfo_x86 *c, const struct cpu_id *x)
+static int centrino_verify_cpu_id(const struct cpuinfo_x86 *c, const struct cpu_id *x)
 {
 	if ((c->x86 == x->x86) &&
 	    (c->x86_vendor == x->x86_vendor) &&
 	    (c->x86_model == x->x86_model) &&
 	    (c->x86_mask == x->x86_mask))
-		return 0;
-	return -ENODEV;
+		return 1;
+	return 0;
 }
 
 /* Extract clock in kHz from PERF_CTL value */
@@ -399,16 +407,20 @@ static int centrino_cpu_init(struct cpuf
 	unsigned freq;
 	unsigned l, h;
 	int ret;
+	int i;
 
 	if (policy->cpu != 0)
 		return -ENODEV;
 
-	if (!cpu_has(cpu, X86_FEATURE_EST))
+	/* Only Intel makes Enhanced Speedstep-capable CPUs */
+	if (cpu->x86_vendor != X86_VENDOR_INTEL || !cpu_has(cpu, X86_FEATURE_EST))
 		return -ENODEV;
 
-	if ((centrino_verify_cpu_id(cpu, &cpu_id_banias)) &&
-	    (centrino_verify_cpu_id(cpu, &cpu_id_dothan_a1)) &&
-		(centrino_verify_cpu_id(cpu, &cpu_id_dothan_b0))) {
+	for (i = 0; i < N_IDS; i++)
+		if (centrino_verify_cpu_id(cpu, &cpu_ids[i]))
+			break;
+
+	if (i == N_IDS) {
 		printk(KERN_INFO PFX "found unsupported CPU with Enhanced SpeedStep: "
 		       "send /proc/cpuinfo to " MAINTAINER "\n");
 		return -ENODEV;

_

[-- 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: my dothan didn't work with cpufreq...

[Dominik Brodowski]

> Yeah, I worked that out from a closer reading of the docs.  There really
> doesn't seem to be any non-hacky way to work out which CPU you have, and
> they seem to have non-overlapping specs (ie, you can't run a VID#D part
> with VID#A voltages).  The hacky way to work it out would be to read
> back the MSR and see what freq/voltage pair you have (which would only
> work at max speed).
... but as many systems boot at a lower speed on battery power, I wouldn't
want to do this.

> Seems like a reasonable idea, though it would be nice for Dothan users
> to not have an ACPI dependency. 

However, as "enhanced SpeedStep" is going to be introduced on
desktop CPUs, the problem will increase: the MSR will likely have a
different encoding. See Venkatesh's patches for details. Also, I fear that
as more CPUs support enhanced SpeedStep, the larger
speedstep-centrino.{o,ko} will become.

> +static const struct cpu_id cpu_ids[] = {
> +	[CPU_BANIAS]	= { X86_VENDOR_INTEL,	6,  9, 5 },
> +	[CPU_DOTHAN_A1]	= { X86_VENDOR_INTEL,	6, 13, 1 },
> +	[CPU_DOTHAN_B0]	= { X86_VENDOR_INTEL,	6, 13, 6 },

Hm, I'm unsure whether this is proper CodingStyle... IIRC, much effort was
spent in converting such { }s to include the respective "fields", like
	{ .x86_vendor = X86_VENDOR_INTEL, x86_family = 6 ... 
and so on.

	Dominik

Re: my dothan didn't work with cpufreq...

[Jeremy Fitzhardinge]

On Thu, 2004-07-22 at 11:31 +0200, Dominik Brodowski wrote:
> > The hacky way to work it out would be to read
> > back the MSR and see what freq/voltage pair you have (which would only
> > work at max speed).
> ... but as many systems boot at a lower speed on battery power, I wouldn't
> want to do this.

Oh, no, I wasn't even considering suggesting it.  It's too nasty.

> However, as "enhanced SpeedStep" is going to be introduced on
> desktop CPUs, the problem will increase: the MSR will likely have a
> different encoding. See Venkatesh's patches for details. Also, I fear that
> as more CPUs support enhanced SpeedStep, the larger
> speedstep-centrino.{o,ko} will become.

The tables are not very big, and I was thinking about generating the
model names from a template rather than explicitly enumerating them all
as we do now.

But you're right, it looks like the simple table approach will be hard
or impossible to maintain.

> > +static const struct cpu_id cpu_ids[] = {
> > +	[CPU_BANIAS]	= { X86_VENDOR_INTEL,	6,  9, 5 },
> > +	[CPU_DOTHAN_A1]	= { X86_VENDOR_INTEL,	6, 13, 1 },
> > +	[CPU_DOTHAN_B0]	= { X86_VENDOR_INTEL,	6, 13, 6 },
> 
> Hm, I'm unsure whether this is proper CodingStyle... IIRC, much effort was
> spent in converting such { }s to include the respective "fields", like
> 	{ .x86_vendor = X86_VENDOR_INTEL, x86_family = 6 ... 
> and so on.

For a tiny little structure like this, which is defined immediately
above, this is fine. (CodingStyle makes no mention of structure
initialization.)

	J

Re: my dothan didn't work with cpufreq...

[Dominik Brodowski]

On Thu, Jul 22, 2004 at 10:34:25AM -0700, Jeremy Fitzhardinge wrote:
> > > +static const struct cpu_id cpu_ids[] = {
> > > +	[CPU_BANIAS]	= { X86_VENDOR_INTEL,	6,  9, 5 },
> > > +	[CPU_DOTHAN_A1]	= { X86_VENDOR_INTEL,	6, 13, 1 },
> > > +	[CPU_DOTHAN_B0]	= { X86_VENDOR_INTEL,	6, 13, 6 },
> > 
> > Hm, I'm unsure whether this is proper CodingStyle... IIRC, much effort was
> > spent in converting such { }s to include the respective "fields", like
> > 	{ .x86_vendor = X86_VENDOR_INTEL, x86_family = 6 ... 
> > and so on.
> 
> For a tiny little structure like this, which is defined immediately
> above, this is fine. (CodingStyle makes no mention of structure
> initialization.)

Again IIRC, it's less about CodingStyle and more about some GCC 3.?
warnings? Though, gcc 3.3.4 doesn't complain...

	Dominik

Re: my dothan didn't work with cpufreq...

[Jeremy Fitzhardinge]

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

On Fri, 2004-07-23 at 21:38 +0200, Dominik Brodowski wrote: 
> Again IIRC, it's less about CodingStyle and more about some GCC 3.?
> warnings? Though, gcc 3.3.4 doesn't complain...

You mean the "[idx] = " syntax?

Anyway, having said that, I got it wrong (I had vendor and family
switched).  Fixed patch attached.

	J

[-- Attachment #2: no-cyrix-est.patch --]
[-- Type: text/x-patch, Size: 6009 bytes --]


A reduce-Jeremy's-mail patch:

 - Only Intel makes EST CPUs.  (Some Cyrix M IIs have the EST bit set - 
	I don't know what it means, but it isn't Enhanced Speedstep.)

 - If it's a known Dothan, but we're looking in the tables, give a
   useful message about using ACPI rather than mailing me.

 - Code cleanups:
  - Make the CPU ID stuff table driven
  - Turn centrino_verify_cpu_id into a proper boolean predicate

 - Diddle some whitespace


 arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c |   86 ++++++++++++----------
 1 files changed, 49 insertions(+), 37 deletions(-)

diff -puN arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c~no-cyrix-est arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c
--- local-2.6/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c~no-cyrix-est	2004-07-21 20:22:16.000000000 -0700
+++ local-2.6-jeremy/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c	2004-07-23 16:15:31.726790060 -0700
@@ -40,33 +40,25 @@
 
 struct cpu_id
 {
-	__u8	x86;            /* CPU family */
 	__u8	x86_vendor;     /* CPU vendor */
+	__u8	x86;            /* CPU family */
 	__u8	x86_model;	/* model */
 	__u8	x86_mask;	/* stepping */
 };
 
-static const struct cpu_id cpu_id_banias = {
-	.x86_vendor = X86_VENDOR_INTEL,
-	.x86 = 6,
-	.x86_model = 9,
-	.x86_mask = 5,
-};
-
-static const struct cpu_id cpu_id_dothan_a1 = {
-	.x86_vendor = X86_VENDOR_INTEL,
-	.x86 = 6,
-	.x86_model = 13,
-	.x86_mask = 1,
-};
-
-static const struct cpu_id cpu_id_dothan_b0 = {
-	.x86_vendor = X86_VENDOR_INTEL,
-	.x86 = 6,
-	.x86_model = 13,
-	.x86_mask = 6,
+enum {
+	CPU_BANIAS,
+	CPU_DOTHAN_A1,
+	CPU_DOTHAN_B0,
 };
 
+static const struct cpu_id cpu_ids[] = {
+	[CPU_BANIAS]	= { X86_VENDOR_INTEL,	6,  9, 5 },
+	[CPU_DOTHAN_A1]	= { X86_VENDOR_INTEL,	6, 13, 1 },
+	[CPU_DOTHAN_B0]	= { X86_VENDOR_INTEL,	6, 13, 6 },
+};
+#define N_IDS	(sizeof(cpu_ids)/sizeof(cpu_ids[0]))
+
 struct cpu_model
 {
 	const struct cpu_id *cpu_id;
@@ -75,7 +67,7 @@ struct cpu_model
 
 	struct cpufreq_frequency_table *op_points; /* clock/voltage pairs */
 };
-static int centrino_verify_cpu_id(struct cpuinfo_x86 *c, const struct cpu_id *x);
+static int centrino_verify_cpu_id(const struct cpuinfo_x86 *c, const struct cpu_id *x);
 
 /* Operating points for current CPU */
 static struct cpu_model *centrino_model;
@@ -110,9 +102,9 @@ static struct cpufreq_frequency_table ba
 /* Ultra Low Voltage Intel Pentium M processor 1000MHz (Banias) */
 static struct cpufreq_frequency_table banias_1000[] =
 {
-	OP(600,  844),
-	OP(800,  972),
-	OP(900,  988),
+	OP(600,   844),
+	OP(800,   972),
+	OP(900,   988),
 	OP(1000, 1004),
 	{ .frequency = CPUFREQ_TABLE_END }
 };
@@ -206,13 +198,13 @@ static struct cpufreq_frequency_table ba
 	.max_freq	= (max)*1000,	\
 	.op_points	= banias_##max,	\
 }
-#define BANIAS(max)	_BANIAS(&cpu_id_banias, max, #max)
+#define BANIAS(max)	_BANIAS(&cpu_ids[CPU_BANIAS], max, #max)
 
 /* CPU models, their operating frequency range, and freq/voltage
    operating points */
 static struct cpu_model models[] = 
 {
-	_BANIAS(&cpu_id_banias, 900, " 900"),
+	_BANIAS(&cpu_ids[CPU_BANIAS], 900, " 900"),
 	BANIAS(1000),
 	BANIAS(1100),
 	BANIAS(1200),
@@ -221,6 +213,11 @@ static struct cpu_model models[] = 
 	BANIAS(1500),
 	BANIAS(1600),
 	BANIAS(1700),
+
+	/* NULL model_name is a wildcard */
+	{ &cpu_ids[CPU_DOTHAN_A1], NULL, 0, NULL },
+	{ &cpu_ids[CPU_DOTHAN_B0], NULL, 0, NULL },
+
 	{ NULL, }
 };
 #undef _BANIAS
@@ -231,17 +228,28 @@ static int centrino_cpu_init_table(struc
 	struct cpuinfo_x86 *cpu = &cpu_data[policy->cpu];
 	struct cpu_model *model;
 
-	for(model = models; model->model_name != NULL; model++)
-		if ((strcmp(cpu->x86_model_id, model->model_name) == 0) &&
-		    (!centrino_verify_cpu_id(cpu, model->cpu_id)))
+	for(model = models; model->cpu_id != NULL; model++)
+		if (centrino_verify_cpu_id(cpu, model->cpu_id) &&
+		    (model->model_name == NULL ||
+		     strcmp(cpu->x86_model_id, model->model_name) == 0))
 			break;
-	if (model->model_name == NULL) {
+	
+	if (model->cpu_id == NULL) {
+		/* No match at all */
 		printk(KERN_INFO PFX "no support for CPU model \"%s\": "
 		       "send /proc/cpuinfo to " MAINTAINER "\n",
 		       cpu->x86_model_id);
 		return -ENOENT;
 	}
 
+	if (model->op_points == NULL) {
+		/* Matched a non-match */
+		printk(KERN_INFO PFX "no table support for CPU model \"%s\": \n",
+		       cpu->x86_model_id);
+		printk(KERN_INFO PFX "try compiling with CONFIG_X86_SPEEDSTEP_CENTRINO_ACPI enabled\n");
+		return -ENOENT;
+	}
+		
 	centrino_model = model;
 		
 	printk(KERN_INFO PFX "found \"%s\": max frequency: %dkHz\n",
@@ -254,14 +262,14 @@ static int centrino_cpu_init_table(struc
 static inline int centrino_cpu_init_table(struct cpufreq_policy *policy) { return -ENODEV; }
 #endif /* CONFIG_X86_SPEEDSTEP_CENTRINO_TABLE */
 
-static int centrino_verify_cpu_id(struct cpuinfo_x86 *c, const struct cpu_id *x)
+static int centrino_verify_cpu_id(const struct cpuinfo_x86 *c, const struct cpu_id *x)
 {
 	if ((c->x86 == x->x86) &&
 	    (c->x86_vendor == x->x86_vendor) &&
 	    (c->x86_model == x->x86_model) &&
 	    (c->x86_mask == x->x86_mask))
-		return 0;
-	return -ENODEV;
+		return 1;
+	return 0;
 }
 
 /* Extract clock in kHz from PERF_CTL value */
@@ -399,16 +407,20 @@ static int centrino_cpu_init(struct cpuf
 	unsigned freq;
 	unsigned l, h;
 	int ret;
+	int i;
 
 	if (policy->cpu != 0)
 		return -ENODEV;
 
-	if (!cpu_has(cpu, X86_FEATURE_EST))
+	/* Only Intel makes Enhanced Speedstep-capable CPUs */
+	if (cpu->x86_vendor != X86_VENDOR_INTEL || !cpu_has(cpu, X86_FEATURE_EST))
 		return -ENODEV;
 
-	if ((centrino_verify_cpu_id(cpu, &cpu_id_banias)) &&
-	    (centrino_verify_cpu_id(cpu, &cpu_id_dothan_a1)) &&
-		(centrino_verify_cpu_id(cpu, &cpu_id_dothan_b0))) {
+	for (i = 0; i < N_IDS; i++)
+		if (centrino_verify_cpu_id(cpu, &cpu_ids[i]))
+			break;
+
+	if (i == N_IDS) {
 		printk(KERN_INFO PFX "found unsupported CPU with Enhanced SpeedStep: "
 		       "send /proc/cpuinfo to " MAINTAINER "\n");
 		return -ENODEV;

_

[-- 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: my dothan didn't work with cpufreq...

[Dominik Brodowski]

On Fri, Jul 23, 2004 at 06:27:24PM -0700, Jeremy Fitzhardinge wrote:
> On Fri, 2004-07-23 at 21:38 +0200, Dominik Brodowski wrote: 
> > Again IIRC, it's less about CodingStyle and more about some GCC 3.?
> > warnings? Though, gcc 3.3.4 doesn't complain...
> 
> You mean the "[idx] = " syntax?

No, the
	{ value, value, ... }
instead of
	{ .field = value, .field = value, ... }
syntax. 

	Dominik

Re: my dothan didn't work with cpufreq...

[Dave Jones]

On Fri, Jul 23, 2004 at 06:27:24PM -0700, Jeremy Fitzhardinge wrote:
 > On Fri, 2004-07-23 at 21:38 +0200, Dominik Brodowski wrote: 
 > > Again IIRC, it's less about CodingStyle and more about some GCC 3.?
 > > warnings? Though, gcc 3.3.4 doesn't complain...
 > 
 > You mean the "[idx] = " syntax?
 > 
 > Anyway, having said that, I got it wrong (I had vendor and family
 > switched).  Fixed patch attached.

I merged this, but something occurred to me whilst eyeballing
the diffs.  Rather than duplicating X86_VENDOR_INTEL in every
entry of the struct, how about we check it in one place?

I'm unaware of any vendor planning to duplicate speedstep,
but even if AMD/VIA threw away their current implementations
in favour of a bit-for-bit compatible implementation, it
wouldn't be hard to add an extra if()

comments?

		Dave

Re: my dothan didn't work with cpufreq...

[Jeremy Fitzhardinge]

On Mon, 2004-08-02 at 20:26 +0100, Dave Jones wrote:
> I merged this, but something occurred to me whilst eyeballing
> the diffs.  Rather than duplicating X86_VENDOR_INTEL in every
> entry of the struct, how about we check it in one place?
> 
> I'm unaware of any vendor planning to duplicate speedstep,
> but even if AMD/VIA threw away their current implementations
> in favour of a bit-for-bit compatible implementation, it
> wouldn't be hard to add an extra if()

I had the same thought.  I already added the vendor check at the top of
the init function, so in theory nothing else should need to look at it.

So, we can just drop the vendor entries in those tables without having
to do anything else.

	J

Re: my dothan didn't work with cpufreq...

[Dave Jones]

On Mon, Aug 02, 2004 at 01:17:53PM -0700, Jeremy Fitzhardinge wrote:
 > On Mon, 2004-08-02 at 20:26 +0100, Dave Jones wrote:
 > > I merged this, but something occurred to me whilst eyeballing
 > > the diffs.  Rather than duplicating X86_VENDOR_INTEL in every
 > > entry of the struct, how about we check it in one place?
 > > 
 > > I'm unaware of any vendor planning to duplicate speedstep,
 > > but even if AMD/VIA threw away their current implementations
 > > in favour of a bit-for-bit compatible implementation, it
 > > wouldn't be hard to add an extra if()
 > 
 > I had the same thought.  I already added the vendor check at the top of
 > the init function, so in theory nothing else should need to look at it.
 > 
 > So, we can just drop the vendor entries in those tables without having
 > to do anything else.

Groovy. I just nuked it in my tree with this patch.
It'll go to Linus with the next batch

If some vendor does clone speedstep, I think it'd be better
done without having to dupe the vendor ID in every entry,
by having per-vendor tables.

		Dave

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/08/02 21:27:47+01:00 davej@redhat.com 
#   [CPUFREQ] speedstep-centrino: Remove unnecessary vendor checks.
#   
#   This is only used on Intel, and if some other vendor ever clones speedstep,
#   we can add an additional check in the init routine.
#   
#   Signed-off-by: Dave Jones <davej@redhat.com>
# 
# arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c
#   2004/08/02 21:27:39+01:00 davej@redhat.com +3 -5
#   [CPUFREQ] speedstep-centrino: Remove unnecessary vendor checks.
#   
#   This is only used on Intel, and if some other vendor ever clones speedstep,
#   we can add an additional check in the init routine.
#   
#   Signed-off-by: Dave Jones <davej@redhat.com>
# 
diff -Nru a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c	2004-08-02 21:28:42 +01:00
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c	2004-08-02 21:28:42 +01:00
@@ -40,7 +40,6 @@
 
 struct cpu_id
 {
-	__u8	x86_vendor;     /* CPU vendor */
 	__u8	x86;            /* CPU family */
 	__u8	x86_model;	/* model */
 	__u8	x86_mask;	/* stepping */
@@ -53,9 +52,9 @@
 };
 
 static const struct cpu_id cpu_ids[] = {
-	[CPU_BANIAS]	= { X86_VENDOR_INTEL,	6,  9, 5 },
-	[CPU_DOTHAN_A1]	= { X86_VENDOR_INTEL,	6, 13, 1 },
-	[CPU_DOTHAN_B0]	= { X86_VENDOR_INTEL,	6, 13, 6 },
+	[CPU_BANIAS]	= { 6,  9, 5 },
+	[CPU_DOTHAN_A1]	= { 6, 13, 1 },
+	[CPU_DOTHAN_B0]	= { 6, 13, 6 },
 };
 #define N_IDS	(sizeof(cpu_ids)/sizeof(cpu_ids[0]))
 
@@ -265,7 +264,6 @@
 static int centrino_verify_cpu_id(const struct cpuinfo_x86 *c, const struct cpu_id *x)
 {
 	if ((c->x86 == x->x86) &&
-	    (c->x86_vendor == x->x86_vendor) &&
 	    (c->x86_model == x->x86_model) &&
 	    (c->x86_mask == x->x86_mask))
 		return 1;