* my dothan didn't work with cpufreq... @ 2004-07-12 20:54 Damien Marchal 2004-07-13 9:49 ` Dominik Brodowski 0 siblings, 1 reply; 15+ messages in thread From: Damien Marchal @ 2004-07-12 20:54 UTC (permalink / raw) To: 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 <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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: my dothan didn't work with cpufreq... 2004-07-12 20:54 my dothan didn't work with cpufreq Damien Marchal @ 2004-07-13 9:49 ` Dominik Brodowski 2004-07-14 10:50 ` Damien Marchal 2004-07-22 1:55 ` Jeremy Fitzhardinge 0 siblings, 2 replies; 15+ messages in thread From: Dominik Brodowski @ 2004-07-13 9:49 UTC (permalink / raw) To: Damien Marchal; +Cc: cpufreq 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: my dothan didn't work with cpufreq... 2004-07-13 9:49 ` Dominik Brodowski @ 2004-07-14 10:50 ` Damien Marchal 2004-07-16 8:19 ` Dominik Brodowski 2004-07-22 1:55 ` Jeremy Fitzhardinge 1 sibling, 1 reply; 15+ messages in thread From: Damien Marchal @ 2004-07-14 10:50 UTC (permalink / raw) To: cpufreq 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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Re: my dothan didn't work with cpufreq... 2004-07-14 10:50 ` Damien Marchal @ 2004-07-16 8:19 ` Dominik Brodowski 0 siblings, 0 replies; 15+ messages in thread From: Dominik Brodowski @ 2004-07-16 8:19 UTC (permalink / raw) To: Damien Marchal; +Cc: cpufreq 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: my dothan didn't work with cpufreq... 2004-07-13 9:49 ` Dominik Brodowski 2004-07-14 10:50 ` Damien Marchal @ 2004-07-22 1:55 ` Jeremy Fitzhardinge 2004-07-22 6:04 ` Dominik Brodowski 1 sibling, 1 reply; 15+ messages in thread From: Jeremy Fitzhardinge @ 2004-07-22 1:55 UTC (permalink / raw) To: Dominik Brodowski; +Cc: cpufreq 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: my dothan didn't work with cpufreq... 2004-07-22 1:55 ` Jeremy Fitzhardinge @ 2004-07-22 6:04 ` Dominik Brodowski 2004-07-22 6:56 ` Jeremy Fitzhardinge 0 siblings, 1 reply; 15+ messages in thread From: Dominik Brodowski @ 2004-07-22 6:04 UTC (permalink / raw) To: Jeremy Fitzhardinge; +Cc: cpufreq 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: my dothan didn't work with cpufreq... 2004-07-22 6:04 ` Dominik Brodowski @ 2004-07-22 6:56 ` Jeremy Fitzhardinge 2004-07-22 9:31 ` Dominik Brodowski 0 siblings, 1 reply; 15+ messages in thread From: Jeremy Fitzhardinge @ 2004-07-22 6:56 UTC (permalink / raw) To: Dominik Brodowski; +Cc: cpufreq list [-- 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: my dothan didn't work with cpufreq... 2004-07-22 6:56 ` Jeremy Fitzhardinge @ 2004-07-22 9:31 ` Dominik Brodowski 2004-07-22 17:34 ` Jeremy Fitzhardinge 0 siblings, 1 reply; 15+ messages in thread From: Dominik Brodowski @ 2004-07-22 9:31 UTC (permalink / raw) To: Jeremy Fitzhardinge; +Cc: cpufreq list > 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: my dothan didn't work with cpufreq... 2004-07-22 9:31 ` Dominik Brodowski @ 2004-07-22 17:34 ` Jeremy Fitzhardinge 2004-07-23 19:38 ` Dominik Brodowski 0 siblings, 1 reply; 15+ messages in thread From: Jeremy Fitzhardinge @ 2004-07-22 17:34 UTC (permalink / raw) To: Dominik Brodowski; +Cc: cpufreq list 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: my dothan didn't work with cpufreq... 2004-07-22 17:34 ` Jeremy Fitzhardinge @ 2004-07-23 19:38 ` Dominik Brodowski 2004-07-24 1:27 ` Jeremy Fitzhardinge 0 siblings, 1 reply; 15+ messages in thread From: Dominik Brodowski @ 2004-07-23 19:38 UTC (permalink / raw) To: Jeremy Fitzhardinge; +Cc: cpufreq list 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: my dothan didn't work with cpufreq... 2004-07-23 19:38 ` Dominik Brodowski @ 2004-07-24 1:27 ` Jeremy Fitzhardinge 2004-07-24 7:06 ` Dominik Brodowski 2004-08-02 19:26 ` Dave Jones 0 siblings, 2 replies; 15+ messages in thread From: Jeremy Fitzhardinge @ 2004-07-24 1:27 UTC (permalink / raw) To: Dominik Brodowski; +Cc: cpufreq list [-- 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: my dothan didn't work with cpufreq... 2004-07-24 1:27 ` Jeremy Fitzhardinge @ 2004-07-24 7:06 ` Dominik Brodowski 2004-08-02 19:26 ` Dave Jones 1 sibling, 0 replies; 15+ messages in thread From: Dominik Brodowski @ 2004-07-24 7:06 UTC (permalink / raw) To: Jeremy Fitzhardinge; +Cc: cpufreq list 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: my dothan didn't work with cpufreq... 2004-07-24 1:27 ` Jeremy Fitzhardinge 2004-07-24 7:06 ` Dominik Brodowski @ 2004-08-02 19:26 ` Dave Jones 2004-08-02 20:17 ` Jeremy Fitzhardinge 1 sibling, 1 reply; 15+ messages in thread From: Dave Jones @ 2004-08-02 19:26 UTC (permalink / raw) To: Jeremy Fitzhardinge; +Cc: Dominik Brodowski, cpufreq list 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: my dothan didn't work with cpufreq... 2004-08-02 19:26 ` Dave Jones @ 2004-08-02 20:17 ` Jeremy Fitzhardinge 2004-08-02 20:29 ` Dave Jones 0 siblings, 1 reply; 15+ messages in thread From: Jeremy Fitzhardinge @ 2004-08-02 20:17 UTC (permalink / raw) To: Dave Jones; +Cc: Dominik Brodowski, cpufreq list 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: my dothan didn't work with cpufreq... 2004-08-02 20:17 ` Jeremy Fitzhardinge @ 2004-08-02 20:29 ` Dave Jones 0 siblings, 0 replies; 15+ messages in thread From: Dave Jones @ 2004-08-02 20:29 UTC (permalink / raw) To: Jeremy Fitzhardinge; +Cc: Dominik Brodowski, cpufreq list 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; ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2004-08-02 20:29 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2004-07-12 20:54 my dothan didn't work with cpufreq Damien Marchal 2004-07-13 9:49 ` Dominik Brodowski 2004-07-14 10:50 ` Damien Marchal 2004-07-16 8:19 ` Dominik Brodowski 2004-07-22 1:55 ` Jeremy Fitzhardinge 2004-07-22 6:04 ` Dominik Brodowski 2004-07-22 6:56 ` Jeremy Fitzhardinge 2004-07-22 9:31 ` Dominik Brodowski 2004-07-22 17:34 ` Jeremy Fitzhardinge 2004-07-23 19:38 ` Dominik Brodowski 2004-07-24 1:27 ` Jeremy Fitzhardinge 2004-07-24 7:06 ` Dominik Brodowski 2004-08-02 19:26 ` Dave Jones 2004-08-02 20:17 ` Jeremy Fitzhardinge 2004-08-02 20:29 ` Dave Jones
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.