my dothan didn't work with cpufreq...
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 <[email protected]>
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.
Hi,
Quoted 14 lines · Damien Marchal
speedstep-centrino: found unsupported CPU with Enhanced SpeedStep: send
/proc/cpuinfoto Jeremy Fitzhardinge <[email protected]>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
Quoted 2 lines
I located a correct
cpu_idcpu_id_dothan_b0but 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".
Quoted 4 lines
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.
Quoted 4 lines
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
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.
Quoted 53 lines · Dominik Brodowski
Hi,
Quoted 4 lines · Damien Marchal
speedstep-centrino: found unsupported CPU with Enhanced SpeedStep: send
/proc/cpuinfoto Jeremy Fitzhardinge <[email protected]>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:Quoted 10 lines
cpu family : 6
model : 13
model name : Intel(R) Pentium(R) M processor 1.50GHz
stepping : 6
cpu MHz : 1500.834
cache size : 64 KBI located a correct
cpu_idcpu_id_dothan_b0but 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_ACPIis enabled in your kernel configuartion. Did you enable it? You need to setACPI_PROCESSORto "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".Quoted 3 lines
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.
Quoted 3 lines
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_CENTRINOdoesn'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
Quoted 6 lines · Damien Marchal
Hi,
i check my .config and saw that the
X86_SPEEDSTEP_CENTRINO_TABLEwas 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
Quoted 4 lines · Dominik Brodowski
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
Quoted 4 lines · Jeremy Fitzhardinge
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?
Quoted 2 lines
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
Quoted 2 lines · Dominik Brodowski
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).
Quoted 5 lines
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
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(-)
arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c renamed from arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c~no-cyrix-est +48 -36
@@ -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_modelstruct 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(strucstruct 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(strucstatic 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 cpufunsigned 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 ;
_
_______________________________________________
Cpufreq mailing list
[email protected]
http://www.linux.org.uk/mailman/listinfo/cpufreq
Quoted 6 lines
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.
Quoted 2 lines
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.
arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c struct cpu_id · line 58
Quoted 4 lines
arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c renamed from arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c~no-cyrix-est +4
-0+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
Quoted 5 lines · Dominik Brodowski
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 9 lines
arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c struct cpu_id · line 58
Quoted 4 lines
arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c renamed from arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c~no-cyrix-est +4
-0+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
Quoted 10 lines · Jeremy Fitzhardinge
Quoted 9 lines
arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c struct cpu_id · line 58
Quoted 4 lines
arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c renamed from arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c~no-cyrix-est +4
-0+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.
Quoted 3 lines
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
Quoted 2 lines · Dominik Brodowski
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
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(-)
arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c renamed from arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c~no-cyrix-est +49 -37
@@ -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_modelstruct 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(strucstruct 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(strucstatic 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 cpufunsigned 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 ;
_
_______________________________________________
Cpufreq mailing list
[email protected]
http://www.linux.org.uk/mailman/listinfo/cpufreq
Quoted 5 lines · Jeremy Fitzhardinge
Quoted 2 lines · Dominik Brodowski
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
Quoted 8 lines · Jeremy Fitzhardinge
Quoted 2 lines · Dominik Brodowski
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
Quoted 8 lines · Dave Jones
I merged this, but something occurred to me whilst eyeballing the diffs. Rather than duplicating
X86_VENDOR_INTELin 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
Quoted 10 lines · Jeremy Fitzhardinge
Quoted 8 lines · Dave Jones
I merged this, but something occurred to me whilst eyeballing the diffs. Rather than duplicating
X86_VENDOR_INTELin 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()
Quoted 5 lines
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 [email protected]
# [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 <[email protected]>
#
# arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c # 2004/08/02 21:27:39+01:00 [email protected] +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 <[email protected]> # 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;