CPUFreq Support for PXA255
Subsystems (5)
PATCH 1/1 CPUFreq Support for PXA255
Hi,
Below is a patch adding cpufreq support for the PXA255 Xscale processor. I'd appreciate review from you guys with a view to getting it merged (after 2.6.13 I guess).
The driver was originally from Intrinsyc Software and spent some time in the old -bkpxa tree. Since then I've tweaked it a little and have been using it successfully for some time.
I'll also submit to the ARM kernel mailing list, since I know at least one of the lists is subscriber only I didn't CC it now. When it comes to merging would via the ARM or cpufreq maintainer be preferable?
arch/arm/Kconfig +7 -1
@@ -516,7 +516,7 @@ endmenu -if (ARCH_SA1100 || ARCH_INTEGRATOR || ARCH_OMAP1) +if (ARCH_SA1100 || ARCH_INTEGRATOR || ARCH_OMAP1 || ARCH_PXA) menu "CPU Frequency scaling"
@@ -543,6 +543,12 @@ If in doubt, say Y. +config CPU_FREQ_PXA + bool + select CPU_FREQ_TABLE + depends on CPU_FREQ && ARCH_PXA + default y + endmenu endif
arch/arm/mach-pxa/Makefile +1 -0
@@ -24,6 +24,7 @@ # Misc features obj-$(CONFIG_PM) += pm.o sleep.o +obj-$(CONFIG_CPU_FREQ) += cpu-pxa.o ifeq ($(CONFIG_PXA27x),y) obj-$(CONFIG_PM) += standby.o
arch/arm/mach-pxa/cpu-pxa.c new file +330 -0
330 lines not on this page · raw message
Documentation/cpu-freq/user-guide.txt +4 -3
@@ -18,7 +18,7 @@ Contents: --------- 1. Supported Architectures and Processors -1.1 ARM +1.1 ARM, PXA 1.2 x86 1.3 sparc64 1.4 ppc
@@ -37,14 +37,15 @@ 1. Supported Architectures and Processors ========================================= -1.1 ARM -------- +1.1 ARM, PXA +------------ The following ARM processors are supported by cpufreq: ARM Integrator ARM-SA1100 ARM-SA1110 +Intel PXA 1.2 x86
drivers/cpufreq/Kconfig +1 -1
@@ -52,7 +52,7 @@ choice prompt "Default CPUFreq governor" - default CPU_FREQ_DEFAULT_GOV_USERSPACE if CPU_FREQ_SA1100 || CPU_FREQ_SA1110 + default CPU_FREQ_DEFAULT_GOV_USERSPACE if CPU_FREQ_SA1100 || CPU_FREQ_SA1110 || CPU_FREQ_PXA default CPU_FREQ_DEFAULT_GOV_PERFORMANCE help This option sets which CPUFreq governor shall be loaded at
Signature · 10 lines
Ian Campbell, Senior Design Engineer
Web: http://www.arcom.com
Arcom, Clifton Road, Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom Phone: +44 (0)1223 411 200
_____________________________________________________________________
The message in this transmission is sent in confidence for the attention of the addressee only and should not be disclosed to any other party. Unauthorised recipients are requested to preserve this confidentiality. Please advise the sender if the addressee is not resident at the receiving end. Email to and from Arcom is automatically monitored for operational and lawful business reasons.
This message has been virus scanned by MessageLabs.14.07.2005 13:40, Ian Campbell wrote/a écrit:
Quoted 1 line
Hi,
Hello,
I don't know so much about cpufreq, so only very few comments:
Quoted 10 lines
===================================================================
arch/arm/mach-pxa/cpu-pxa.c new file +7
-0+ +static void pxa_select_freq_table (struct cpufreq_policy *policy, +pxa_freqs_t ** settings, +struct cpufreq_frequency_table **table) +{ +if (strcmp (policy->governor->name,"performance" ) ==0 ) { +if (settings)
arch/arm/mach-pxa/cpu-pxa.c line 127
Quoted 10 lines
arch/arm/mach-pxa/cpu-pxa.c new file +10
-0+ *settings = pxa255_run_freqs; +if (table) + *table = pxa255_run_freq_table; + }else { +if (settings) + *settings = pxa255_turbo_freqs; +if (table) + *table = pxa255_turbo_freq_table; + } +}
So your driver depends on a hard-coded name of a governor ? It seems suspicious...
arch/arm/mach-pxa/cpu-pxa.c line 149
Quoted 4 lines
arch/arm/mach-pxa/cpu-pxa.c new file +4
-0+ +static int pxa_set_target (struct cpufreq_policy *policy, +unsigned int target_freq,unsigned int relation) +{
:
arch/arm/mach-pxa/cpu-pxa.c line 182
Quoted 11 lines
arch/arm/mach-pxa/cpu-pxa.c new file +11
-0+ +/* Get the current policy */ +pxa_select_freq_table (policy, &pxa_freq_settings, &pxa_freqs_table); + +/* Lookup the next frequency */ +if (cpufreq_frequency_table_target (policy, pxa_freqs_table, + target_freq, relation, &idx)) +return -EINVAL ; + + freqs.old =get_clk_frequency_khz (0 ); + freqs.new = pxa_freq_settings[idx].khz;
It's probably a good idea to include such a code:
if (freqs.new == freqs.old) return 0;
This avoid to do lots of things while it would change nothing.
arch/arm/mach-pxa/cpu-pxa.c
Quoted 2 lines
arch/arm/mach-pxa/cpu-pxa.c new file +2
-0+static int pxa_cpufreq_init (struct cpufreq_policy *policy) +{
:
arch/arm/mach-pxa/cpu-pxa.c line 285
Quoted 2 lines
arch/arm/mach-pxa/cpu-pxa.c new file +2
-0+ policy->cpuinfo.min_freq = PXA25x_MIN_FREQ; + policy->cpuinfo.transition_latency =1000 ;/* FIXME: 1 ms, assumed */
If you want to say 1ms, you have to write 1000000 (it's in nanosecond). This value is quite important for the ondemand governor so if you have better knowledge of the latency, don't hesitate to fix it!
arch/arm/mach-pxa/cpu-pxa.c line 314
Quoted 8 lines
arch/arm/mach-pxa/cpu-pxa.c new file +8
-0+ +static struct cpufreq_driver pxa_cpufreq_driver = { + .verify = pxa_verify_policy, + .target = pxa_set_target, + .init = pxa_cpufreq_init, + .get = pxa_cpufreq_get, + .name ="PXA25x" , +};
Not sure it's a good idea to put the name in uppercase.
Eric
Quoted 19 lines · Eric Piel
Quoted 1 line · Ian Campbell wrote/
Hi,
Hello,
I don't know so much about cpufreq, so only very few comments:
Quoted 14 lines
===================================================================
arch/arm/mach-pxa/cpu-pxa.c new file +11
-0+ +static void pxa_select_freq_table (struct cpufreq_policy *policy, +pxa_freqs_t ** settings, +struct cpufreq_frequency_table **table) +{ +if (strcmp (policy->governor->name,"performance" ) ==0 ) { +if (settings) + *settings = pxa255_run_freqs; +if (table) + *table = pxa255_run_freq_table; + }else {
Quoted 8 lines
arch/arm/mach-pxa/cpu-pxa.c line 127
Quoted 6 lines
arch/arm/mach-pxa/cpu-pxa.c new file +6
-0+if (settings) + *settings = pxa255_turbo_freqs; +if (table) + *table = pxa255_turbo_freq_table; + } +}So your driver depends on a hard-coded name of a governor ? It seems suspicious...
I'm not entirely happy about this either, but let me explain...
The PXA255 has essentially two orthogonal sets of available frequencies, and it is possible to get e.g. 400MHz using either one, but with different power consumption vs performance trades.
One of the sets of frequencies is achieved by modifying the "run mode multiplier" which costs power but gives best performance while the other involves modifying the "turbo mode multiplier" which uses less power for a given speed but at the cost of some performance.
So the code selects the run mode freqs for the performance governor but turbo for the powersave governor + any others. I did it this way because it is an embedded processor which often can run from a battery so I figured it makes sense to select the best power consumption over performance unless explicitly requested. I couldn't see another method in the cpufreq framework to select between the two modes, I guess it could be a command line or module parameter option or something.
Quoted 1 line
[snip]
Thanks for your other comments, I'll make those changes.
Ian.
Signature · 10 lines
Ian Campbell, Senior Design Engineer
Web: http://www.arcom.com
Arcom, Clifton Road, Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom Phone: +44 (0)1223 411 200
_____________________________________________________________________
The message in this transmission is sent in confidence for the attention of the addressee only and should not be disclosed to any other party. Unauthorised recipients are requested to preserve this confidentiality. Please advise the sender if the addressee is not resident at the receiving end. Email to and from Arcom is automatically monitored for operational and lawful business reasons.
This message has been virus scanned by MessageLabs.Hi,
arch/arm/mach-pxa/cpu-pxa.c line 54
Quoted 9 lines · Ian Campbell
arch/arm/mach-pxa/cpu-pxa.c new file +9
-0+#define DEBUG 0 + +#ifdef DEBUG +static unsigned int freq_debug =DEBUG ; +module_param (freq_debug, int,0 ); +MODULE_PARM_DESC (freq_debug,"Set the debug messages to on=1/off=0" ); +#else +#define freq_debug 0 +#endif
Please use the cpufreq debug infrastructure for this:
#define dprintk(msg...) cpufreq_debug_printk(CPUFREQ_DEBUG_DRIVER, "pxa", msg)
arch/arm/mach-pxa/cpu-pxa.c line 61
Quoted 6 lines
arch/arm/mach-pxa/cpu-pxa.c new file +6
-0+typedef struct { +unsigned int khz; +unsigned int membus; +unsigned int cccr; +unsigned int div2; +}pxa_freqs_t ;
Don't use typedefs, please. Just name it "struct pxa_freqs"
Quoted 6 lines
arch/arm/mach-pxa/cpu-pxa.c new file +6
-0+static void pxa_select_freq_table (struct cpufreq_policy *policy, +pxa_freqs_t ** settings, +struct cpufreq_frequency_table **table) +{ +if (strcmp (policy->governor->name,"performance" ) ==0 ) { +if (settings)
arch/arm/mach-pxa/cpu-pxa.c line 127
Quoted 10 lines
arch/arm/mach-pxa/cpu-pxa.c new file +10
-0+ *settings = pxa255_run_freqs; +if (table) + *table = pxa255_run_freq_table; + }else { +if (settings) + *settings = pxa255_turbo_freqs; +if (table) + *table = pxa255_turbo_freq_table; + } +}
As noted by Eric Piel already, please use a module parameter combined with a sysfs file instead.
arch/arm/mach-pxa/cpu-pxa.c line 142
Quoted 4 lines
arch/arm/mach-pxa/cpu-pxa.c new file +4
-0+if (freq_debug) { +printk ("Verified CPU policy: %dKhz min to %dKhz max\n" , + policy->min, policy->max); + }
dprintk("Verified ...);Review
Quoted 1 line
(quoted diff) +1
-0+ cpumask_t cpus_allowed;
Is this a SMP-capable platform?
arch/arm/mach-pxa/cpu-pxa.c line 285
Quoted 1 line
arch/arm/mach-pxa/cpu-pxa.c new file +1
-0+ policy->cpuinfo.transition_latency =1000 ;/* FIXME: 1 ms, assumed */
1000000
Quoted 9 lines
+1.1 ARM, PXA
+------------The following ARM processors are supported by cpufreq:
ARM Integrator
ARM-SA1100
ARM-SA1110
+Intel PXA
Is the PXA just a sub-architecture of ARM?
drivers/cpufreq/Kconfig line 55
Quoted 4 lines
choice
drivers/cpufreq/Kconfig +1
-1prompt "Default CPUFreq governor" - default CPU_FREQ_DEFAULT_GOV_USERSPACE if CPU_FREQ_SA1100 || CPU_FREQ_SA1110 + default CPU_FREQ_DEFAULT_GOV_USERSPACE if CPU_FREQ_SA1100 || CPU_FREQ_SA1110 || CPU_FREQ_PXA
Why is that?
Except the two-tables-issue I'd like to see solved differently, it's good code IMHO.
Dominik
On Fri, 2005-07-15 at 18:41 +0200, Dominik Brodowski wrote:
Thanks for you comments.
I've snipped the bits for changes that I've already done.
Quoted 11 lines
arch/arm/mach-pxa/cpu-pxa.c
Quoted 3 lines
arch/arm/mach-pxa/cpu-pxa.c new file +3
-0+static void pxa_select_freq_table (struct cpufreq_policy *policy, +pxa_freqs_t ** settings, + struct cpufreq_frequency_table**table)
Quoted 1 line
+{
[SNIP]
Quoted 1 line
+}
As noted by Eric Piel already, please use a module parameter combined with a sysfs file instead.
I've got it working with a module parameter so cpu_pxa.performance=1 allows selection of the performance table at boot time.
This also creates /sys/module/cpu_pxa/parameters/performance which can be written to and takes effect on the next frequency change. Is this acceptable or should I look for a way to add a sysfs file to /sys/devices/system/cpu/cpu0/cpufreq? The advantage would be immediate application of the change (I think...)
Quoted 3 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ cpumask_t cpus_allowed;Is this a SMP-capable platform?
I don't really know to be honest -- there are SMP ARM's in the world but I don't know if that will ever extend to the PXA. I doubt it, but I've asked on the ARM kernel list.
Quoted 1 line
Is the PXA just a sub-architecture of ARM?
That's right, it's one of Intel's ARM implementations, it takes over from their StrongARM stuff. The PXA line is quite commonly found in PDAs.
Quoted 6 lines
drivers/cpufreq/Kconfig line 55
Quoted 4 lines
choice
drivers/cpufreq/Kconfig +1
-1prompt "Default CPUFreq governor" - default CPU_FREQ_DEFAULT_GOV_USERSPACE if CPU_FREQ_SA1100 || CPU_FREQ_SA1110 + default CPU_FREQ_DEFAULT_GOV_USERSPACE if CPU_FREQ_SA1100 || CPU_FREQ_SA1110 || CPU_FREQ_PXAWhy is that?
I've no idea -- I just copied the StrongARM stuff. My guess is that someone decided that PERFORMANCE wasn't suitable as the default for an "embedded" platform. I think I'll probably just drop this chunk since you can always choose via config anyway. I've pinged the ARM list just in case there is a more pressing reason.
Ian.
Signature · 10 lines
Ian Campbell, Senior Design Engineer
Web: http://www.arcom.com
Arcom, Clifton Road, Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom Phone: +44 (0)1223 411 200
_____________________________________________________________________
The message in this transmission is sent in confidence for the attention of the addressee only and should not be disclosed to any other party. Unauthorised recipients are requested to preserve this confidentiality. Please advise the sender if the addressee is not resident at the receiving end. Email to and from Arcom is automatically monitored for operational and lawful business reasons.
This message has been virus scanned by MessageLabs.Hi,
Quoted 12 lines · Ian Campbell
Quoted 11 lines · Dominik Brodowski
arch/arm/mach-pxa/cpu-pxa.c
Quoted 3 lines
arch/arm/mach-pxa/cpu-pxa.c new file +3
-0+static void pxa_select_freq_table (struct cpufreq_policy *policy, +pxa_freqs_t ** settings, + struct cpufreq_frequency_table**table)
Quoted 1 line
+{
[SNIP]
Quoted 1 line
+}
As noted by Eric Piel already, please use a module parameter combined with a sysfs file instead.
Quoted 8 lines
I've got it working with a module parameter so cpu_pxa.performance=1 allows selection of the performance table at boot time.
This also creates
/sys/module/cpu_pxa/parameters/performancewhich can be written to and takes effect on the next frequency change. Is this acceptable or should I look for a way to add a sysfs file to/sys/devices/system/cpu/cpu0/cpufreq? The advantage would be immediate application of the change (I think...)
Tough question. As sysfs supports links... could you try adding a link from /sys/devices/system/cpu/cpu0/performance_mode to /sys/module/cpu_pxa/parameters/performance ?
Quoted 7 lines
Quoted 6 lines
drivers/cpufreq/Kconfig line 55
Quoted 4 lines
choice
drivers/cpufreq/Kconfig +1
-1prompt "Default CPUFreq governor" - default CPU_FREQ_DEFAULT_GOV_USERSPACE if CPU_FREQ_SA1100 || CPU_FREQ_SA1110 + default CPU_FREQ_DEFAULT_GOV_USERSPACE if CPU_FREQ_SA1100 || CPU_FREQ_SA1110 || CPU_FREQ_PXAWhy is that?
Quoted 5 lines
I've no idea -- I just copied the StrongARM stuff. My guess is that someone decided that PERFORMANCE wasn't suitable as the default for an "embedded" platform. I think I'll probably just drop this chunk since you can always choose via config anyway. I've pinged the ARM list just in case there is a more pressing reason.
I added it to maintain backwards compatibility. AFAICS, there is no real reason to copy this behaviour to new drivers.
Thanks,
Dominik
Quoted 9 lines · Dominik Brodowski
Quoted 5 lines
This also creates
/sys/module/cpu_pxa/parameters/performancewhich can be written to and takes effect on the next frequency change. Is this acceptable or should I look for a way to add a sysfs file to/sys/devices/system/cpu/cpu0/cpufreq? The advantage would be immediate application of the change (I think...)Tough question. As sysfs supports links... could you try adding a link from
/sys/devices/system/cpu/cpu0/performance_modeto/sys/module/cpu_pxa/parameters/performance?
I can certainly try, although I suspect you'll see me on linux-kernel in about an hour soliciting help from the sysfs guys ;-)
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
drivers/cpufreq/Kconfig line 55
Quoted 4 lines
choice
drivers/cpufreq/Kconfig +1
-1prompt "Default CPUFreq governor" - default CPU_FREQ_DEFAULT_GOV_USERSPACE if CPU_FREQ_SA1100 || CPU_FREQ_SA1110 + default CPU_FREQ_DEFAULT_GOV_USERSPACE if CPU_FREQ_SA1100 || CPU_FREQ_SA1110 || CPU_FREQ_PXAWhy is that?
Quoted 8 lines
Quoted 5 lines
I've no idea -- I just copied the StrongARM stuff. My guess is that someone decided that PERFORMANCE wasn't suitable as the default for an "embedded" platform. I think I'll probably just drop this chunk since you can always choose via config anyway. I've pinged the ARM list just in case there is a more pressing reason.
I added it to maintain backwards compatibility. AFAICS, there is no real reason to copy this behaviour to new drivers.
K, I'll drop that bit then. Thanks.
Ian.
Signature · 10 lines
Ian Campbell, Senior Design Engineer
Web: http://www.arcom.com
Arcom, Clifton Road, Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom Phone: +44 (0)1223 411 200
_____________________________________________________________________
The message in this transmission is sent in confidence for the attention of the addressee only and should not be disclosed to any other party. Unauthorised recipients are requested to preserve this confidentiality. Please advise the sender if the addressee is not resident at the receiving end. Email to and from Arcom is automatically monitored for operational and lawful business reasons.
This message has been virus scanned by MessageLabs.Quoted 7 lines · Ian Campbell
Quoted 6 lines · Dominik Brodowski
Quoted 5 lines
This also creates
/sys/module/cpu_pxa/parameters/performancewhich can be written to and takes effect on the next frequency change. Is this acceptable or should I look for a way to add a sysfs file to/sys/devices/system/cpu/cpu0/cpufreq? The advantage would be immediate application of the change (I think...)
Quoted 6 lines
Quoted 3 lines
Tough question. As sysfs supports links... could you try adding a link from
/sys/devices/system/cpu/cpu0/performance_modeto/sys/module/cpu_pxa/parameters/performance?I can certainly try, although I suspect you'll see me on linux-kernel in about an hour soliciting help from the sysfs guys ;-)
As near as I can tell this isn't going to be possible because when the module is built in there is no kobject that I can get my hands on to use as the target, since mkobj is faked out in kernel/params.c:kernel_param_sysfs_setup() and THIS_MODULE is NULL anyway.
So I've decided just add a freq_attr for the value, keeping the module param (but mode == 0) to set the initial value.
I want to make it transition to the new set of frequencies immediately instead of waiting for the next frequency change. I have:
static ssize_t store_pxa_mode_attr(struct cpufreq_policy * policy, const char *buf, size_t count)
{
unsigned int ret = -EINVAL;
char str[16];
ret = sscanf (buf, "%15s", str);
if (ret != 1)
return -EINVAL;
if (strnicmp(str,"performance",16)==0)
performance = 1;
else
performance = 0;
ret = cpufreq_governor(policy->cpu, CPUFREQ_GOV_START);
return ret ? ret : count;
}I'm not sure about the call to cpufreq_governor to "restart" the current governor (and hence apply the new timings), it looks ok from the current code, and appears to work, but I don't want to rely on undefined behaviour. Is it OK?
I'll probably end up calling the attribute "pxa2xx_freq_model" unless anyone has a better idea.
Cheers,
Ian.
(incidentally -- the init of ret = -EINVAL above came from store_one() in cpufreq.c and seems to be extraneous since ret is immediately assigned to there as well, I'll drop it here -- I just left it as a hook for this aside ;-). The space after sscanf too.).
Signature · 4 lines
Ian Campbell, Senior Design Engineer
Web: http://www.arcom.com
Arcom, Clifton Road, Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom Phone: +44 (0)1223 411 200Quoted 46 lines · Ian Campbell
Quoted 7 lines · Ian Campbell
Quoted 6 lines · Dominik Brodowski
Quoted 5 lines
This also creates
/sys/module/cpu_pxa/parameters/performancewhich can be written to and takes effect on the next frequency change. Is this acceptable or should I look for a way to add a sysfs file to/sys/devices/system/cpu/cpu0/cpufreq? The advantage would be immediate application of the change (I think...)Quoted 6 lines
Quoted 3 lines
Tough question. As sysfs supports links... could you try adding a link from
/sys/devices/system/cpu/cpu0/performance_modeto/sys/module/cpu_pxa/parameters/performance?I can certainly try, although I suspect you'll see me on linux-kernel in about an hour soliciting help from the sysfs guys ;-)
As near as I can tell this isn't going to be possible because when the module is built in there is no kobject that I can get my hands on to use as the target, since mkobj is faked out in kernel/params.c:kernel_param_sysfs_setup() and
THIS_MODULEis NULL anyway.So I've decided just add a
freq_attrfor the value, keeping the module param (but mode == 0) to set the initial value.I want to make it transition to the new set of frequencies immediately instead of waiting for the next frequency change. I have:
static ssize_t store_pxa_mode_attr(struct cpufreq_policy * policy, const char *buf, size_t count) { unsigned int ret = -EINVAL; char str[16]; ret = sscanf (buf, "%15s", str); if (ret != 1) return -EINVAL; if (strnicmp(str,"performance",16)==0) performance = 1; else performance = 0; ret = cpufreq_governor(policy->cpu, CPUFREQ_GOV_START); return ret ? ret : count; }
Quoted 4 lines
I'm not sure about the call to
cpufreq_governorto "restart" the current governor (and hence apply the new timings), it looks ok from the current code, and appears to work, but I don't want to rely on undefined behaviour. Is it OK?
No. Just call
cpufreq_update_policy(policy->cpu);
instead.
Thanks,
Dominik
Quoted 5 lines · Dominik Brodowski
Quoted 1 line
ret = cpufreq_governor(policy->cpu, CPUFREQ_GOV_START);No. Just call
cpufreq_update_policy(policy->cpu);instead.
Thanks -- don't know how I missed that one.
Ian.
Signature · 10 lines
Ian Campbell, Senior Design Engineer
Web: http://www.arcom.com
Arcom, Clifton Road, Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom Phone: +44 (0)1223 411 200
_____________________________________________________________________
The message in this transmission is sent in confidence for the attention of the addressee only and should not be disclosed to any other party. Unauthorised recipients are requested to preserve this confidentiality. Please advise the sender if the addressee is not resident at the receiving end. Email to and from Arcom is automatically monitored for operational and lawful business reasons.
This message has been virus scanned by MessageLabs.Quoted 3 lines · Ian Campbell
Below is a patch adding cpufreq support for the PXA255 Xscale processor. I'd appreciate review from you guys with a view to getting it merged (after 2.6.13 I guess).
Thanks for all your comments. Here is an updated version, changes include:
* use pxa25x as name throughout.
* remove SMP support.
* support selection of performance vs. powersave via a sysfs file/module parameter instead of by looking at the governor (requested/suggested by cpufreq list).
* some other bits and pieces...
If you are happy with it then please apply.
%status
external
%patch
arch/arm/Kconfig +7 -1
@@ -516,7 +516,7 @@ endmenu -if (ARCH_SA1100 || ARCH_INTEGRATOR || ARCH_OMAP1) +if (ARCH_SA1100 || ARCH_INTEGRATOR || ARCH_OMAP1 || ARCH_PXA) menu "CPU Frequency scaling"
@@ -543,6 +543,12 @@ If in doubt, say Y. +config CPU_FREQ_PXA25x + bool + select CPU_FREQ_TABLE + depends on CPU_FREQ && PXA25x + default y + endmenu endif
arch/arm/mach-pxa/Makefile +3 -0
@@ -22,6 +22,9 @@ obj-$(CONFIG_LEDS) += $(led-y) +# CPU freq support +obj-$(CONFIG_CPU_FREQ_PXA25x) += cpu-pxa25x.o + # Misc features obj-$(CONFIG_PM) += pm.o sleep.o
Documentation/cpu-freq/user-guide.txt +1 -0
@@ -45,6 +45,7 @@ ARM Integrator ARM-SA1100 ARM-SA1110 +Intel XScale PXA25x 1.2 x86
arch/arm/mach-pxa/cpu-pxa25x.c new file +351 -0
351 lines not on this page · raw message
Signature · 4 lines
Ian Campbell, Senior Design Engineer
Web: http://www.arcom.com
Arcom, Clifton Road, Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom Phone: +44 (0)1223 411 200Hi,
Quoted 4 lines · Ian Campbell
arch/arm/mach-pxa/cpu-pxa25x.c new file +4
-0+static int pxa_set_target (struct cpufreq_policy *policy, +unsigned int target_freq,unsigned int relation) +{ +int idx;
arch/arm/mach-pxa/cpu-pxa25x.c line 159
Quoted 10 lines
arch/arm/mach-pxa/cpu-pxa25x.c new file +10
-0+struct cpufreq_freqs freqs; +struct pxa_freqs *pxa_freq_settings; +struct cpufreq_frequency_table *pxa_freqs_table; +unsigned long flags; +unsigned int unused; +unsigned int preset_mdrefr, postset_mdrefr; +void *ramstart; + +/* Get the current policy */ +pxa_select_freq_table (policy, &pxa_freq_settings, &pxa_freqs_table);
In my opinion you should only switch between the tables in verify. Only then can we assure that the ->min and ->max values provided are OK at this moment.
arch/arm/mach-pxa/cpu-pxa25x.c line 265
Quoted 7 lines
arch/arm/mach-pxa/cpu-pxa25x.c new file +7
-0+/* set default policy and cpuinfo */ + policy->governor =CPUFREQ_DEFAULT_GOVERNOR ; + policy->cpuinfo.max_freq = max_frequency; + policy->cpuinfo.min_freq = PXA25x_MIN_FREQ; + policy->cpuinfo.transition_latency =1000000 ;/* FIXME: 1 ms, assumed */ + policy->cur =get_clk_frequency_khz (0 );/* current freq */ + policy->min = policy->max = policy->cur;
Better set policy->min to cpuinfo.min_freq and ->max to cpuinfo.max_freq.
arch/arm/mach-pxa/cpu-pxa25x.c line 323
Quoted 2 lines
arch/arm/mach-pxa/cpu-pxa25x.c new file +2
-0+static struct freq_attr * pxa_cpufreq_attr[] = { + &cpufreq_freq_attr_scaling_available_freqs,
Tab, not space
Looks good, thanks!
Dominik
Quoted 3 lines · Dominik Brodowski
In my opinion you should only switch between the tables in verify. Only then can we assure that the ->min and ->max values provided are OK at this moment.
That makes perfect sense, done.
Quoted 3 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ policy->min = policy->max = policy->cur;Better set
policy->minto cpuinfo.min_freq and ->max to cpuinfo.max_freq.
Someone queried this on the ARM list too -- I've made the change you suggest.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+static struct freq_attr* pxa_cpufreq_attr[] = { + &cpufreq_freq_attr_scaling_available_freqs,Tab, not space
Cheers, I missed that one.
Updated patch follows.
arch/arm/Kconfig +7 -1
@@ -516,7 +516,7 @@ endmenu -if (ARCH_SA1100 || ARCH_INTEGRATOR || ARCH_OMAP1) +if (ARCH_SA1100 || ARCH_INTEGRATOR || ARCH_OMAP1 || ARCH_PXA) menu "CPU Frequency scaling"
@@ -543,6 +543,12 @@ If in doubt, say Y. +config CPU_FREQ_PXA25x + bool + select CPU_FREQ_TABLE + depends on CPU_FREQ && PXA25x + default y + endmenu endif
arch/arm/mach-pxa/Makefile +3 -0
@@ -22,6 +22,9 @@ obj-$(CONFIG_LEDS) += $(led-y) +# CPU freq support +obj-$(CONFIG_CPU_FREQ_PXA25x) += cpu-pxa25x.o + # Misc features obj-$(CONFIG_PM) += pm.o sleep.o
Documentation/cpu-freq/user-guide.txt +1 -0
@@ -45,6 +45,7 @@ ARM Integrator ARM-SA1100 ARM-SA1110 +Intel XScale PXA25x 1.2 x86
arch/arm/mach-pxa/cpu-pxa25x.c new file +342 -0
342 lines not on this page · raw message
Ian.
Signature · 4 lines
Ian Campbell, Senior Design Engineer
Web: http://www.arcom.com
Arcom, Clifton Road, Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom Phone: +44 (0)1223 411 200