cpufreq: Add SFI based cpufreq driver support
Subsystems (2)
PATCH v5 1/1 cpufreq: Add SFI based cpufreq driver support
This adds the SFI based cpu freq driver for some of the Intel's Silvermont based Atom architectures like Z34xx and Z35xx.
---
v2:
(v2-v3: comments from Viresh)
* rephrase the Kconfig text
* merge the header file as its not being shared with anyone
* remove the unnecessary checks in sfi_cpufreq_target
* style changes in kzalloc
* fix the potential memory leak sfi_cpufreq_cpu_init
* move the cpufreq driver flag to the structure definition
* remove the redundant init of policy->cpu and policy->cur
* remove the unnecessary check while init the freq table
* merge the for loops in cpu_init
* use cpufreq_generic_attr rather than the custom one
v3:
* Add this change log
* remove the left over not used cached_freq var
* merge yet another for loop in cpu_init
v4:
* remove unnecessary pr_debug
* rephrase Kconfig text
* remove unused macro SFI_CPU_MAX
* Add Viresh's Acked-by
v5:
(comments from Len Brown)
* rename few private functions with sfi_cpufreq_ prefix
* move the PERF_CTL_MASK to msr-index.h
* make the driver.get current freq to refer PERF_CTRL
* remove sfi_processor_px, rather reuse sfi_freq_table_entry
* remove unnecessary memcpy, rather just point to the freq table
* consolidate get_performance_states with parse_freq
* remove extract_freq and make get_cur_freq to read from MSR directly
* move the state_count check while registering the perf states
arch/x86/include/uapi/asm/msr-index.h | 1 + drivers/cpufreq/Kconfig.x86 | 10 ++ drivers/cpufreq/Makefile | 1 + drivers/cpufreq/sfi-cpufreq.c | 238 +++++++++++++++++++++++++++++++++ 4 files changed, 250 insertions(+) create mode 100644 drivers/cpufreq/sfi-cpufreq.c
arch/x86/include/uapi/asm/msr-index.h +1 -0
@@ -318,6 +318,7 @@#define MSR_IA32_PERF_STATUS 0x00000198#define MSR_IA32_PERF_CTL 0x00000199 +#define INTEL_PERF_CTL_MASK 0xffff#define MSR_AMD_PSTATE_DEF_BASE 0xc0010064#define MSR_AMD_PERF_STATUS 0xc0010063#define MSR_AMD_PERF_CTL 0xc0010062
drivers/cpufreq/Kconfig.x86 +10 -0
@@ -57,6 +57,16 @@ config X86_ACPI_CPUFREQ_CPB By enabling this option the acpi_cpufreq driver provides the old entry in addition to the new boost ones, for compatibility reasons. +config X86_SFI_CPUFREQ + tristate "SFI P-States driver" + depends on X86_INTEL_MID && SFI + help + This adds a CPUFreq driver for some Silvermont based Intel Atom + architectures like Z34xx and Z35xx which enumerate processor + performance states through SFI. + + If in doubt, say N. + config ELAN_CPUFREQ tristate "AMD Elan SC400 and SC410" depends on MELAN
drivers/cpufreq/Makefile +1 -0
@@ -41,6 +41,7 @@ obj-$(CONFIG_X86_P4_CLOCKMOD) += p4-clockmod.o obj-$(CONFIG_X86_CPUFREQ_NFORCE2) += cpufreq-nforce2.o obj-$(CONFIG_X86_INTEL_PSTATE) += intel_pstate.o obj-$(CONFIG_X86_AMD_FREQ_SENSITIVITY) += amd_freq_sensitivity.o +obj-$(CONFIG_X86_SFI_CPUFREQ) += sfi-cpufreq.o ################################################################################## # ARM SoC drivers
drivers/cpufreq/sfi-cpufreq.c new file +238 -0
238 lines not on this page · raw message
Signature · 4 lines
1.7.9.5 --
Quoted 7 lines · Srinidhi Kasagar
This adds the SFI based cpu freq driver for some of the Intel's Silvermont based Atom architectures like Z34xx and Z35xx.
Signed-off-by Rudramuni, Vishwesh M <[email protected]>Signed-off-by Srinidhi Kasagar <[email protected]>Acked-by Viresh Kumar <[email protected]>---
[..]
drivers/cpufreq/sfi-cpufreq.c line 74
Quoted 11 lines
drivers/cpufreq/sfi-cpufreq.c new file +11
-0+static int sfi_cpufreq_register_performance (struct sfi_cpufreq_performance *pr) +{ +mutex_lock (&performance_mutex); + +/* parse the freq table from SFI */ +sfi_table_parse (SFI_SIG_FREQ ,NULL ,NULL , parse_freq); + + pr->state_count = sfi_cpufreq_num; +if (pr->state_count <=1 ) { +pr_err ("No p-states discovered\n" ); +return -ENODEV ;
Was in a hurry, will release the mutex in the next version. Let me knmow if you have further comments.
Srinidhi
Viresh Kumar
maintainerdrivers/cpufreq/sfi-cpufreq.c line 52
Quoted 2 lines · Srinidhi Kasagar
drivers/cpufreq/sfi-cpufreq.c +1
-0+static int parse_freq (struct sfi_table_header *table)
When this can't fail, why should it return a int?
Quoted 60 lines
drivers/cpufreq/sfi-cpufreq.c +60
-0+{ +struct sfi_table_simple *sb; + + sb = (struct sfi_table_simple *)table; + + sfi_cpufreq_num = SFI_GET_NUM_ENTRIES(sb, struct sfi_freq_table_entry); + f_entry = (struct sfi_freq_table_entry *)sb->pentry; + +return 0 ; +} + +static int sfi_cpufreq_register_performance (struct sfi_cpufreq_performance *pr) +{ +mutex_lock (&performance_mutex); + +/* parse the freq table from SFI */ +sfi_table_parse (SFI_SIG_FREQ ,NULL ,NULL , parse_freq); + + pr->state_count = sfi_cpufreq_num; +if (pr->state_count <=1 ) { +pr_err ("No p-states discovered\n" ); +return -ENODEV ; + } + + pr->states = f_entry; + +mutex_unlock (&performance_mutex); + +return 0 ; +} + +static unsigned int get_cur_freq_on_cpu (unsigned int cpu) +{ +u32 lo, hi, sfi_ctrl; +struct sfi_cpufreq_performance *perf; +struct cpufreq_frequency_table *pos; +struct sfi_cpufreq_data *data =per_cpu (drv_data, cpu); + +rdmsr_on_cpu (cpu,MSR_IA32_PERF_CTL , &lo, &hi); + lo &=SFI_FREQ_MASK ; + + perf = data->sfi_data; + +cpufreq_for_each_entry (pos, data->freq_table) { + sfi_ctrl = perf->states[pos->driver_data].ctrl_val + &SFI_FREQ_MASK ; +if (sfi_ctrl == lo) +return pos->frequency; + } +return data->freq_table[0 ].frequency; +} + +static int sfi_cpufreq_target (struct cpufreq_policy *policy, +unsigned int index) +{ +struct sfi_cpufreq_data *data =per_cpu (drv_data, policy->cpu); +struct sfi_cpufreq_performance *perf; +unsigned int next_perf_state =0 ;/* Index into perf table */ +u32 lo, hi; +
drivers/cpufreq/sfi-cpufreq.c line 122
Quoted 10 lines
drivers/cpufreq/sfi-cpufreq.c +10
-0+ perf = data->sfi_data; + next_perf_state = data->freq_table[index].driver_data; + +rdmsr_on_cpu (policy->cpu,MSR_IA32_PERF_CTL , &lo, &hi); + lo = (lo & ~INTEL_PERF_CTL_MASK ) | + ((u32 ) perf->states[next_perf_state].ctrl_val & +INTEL_PERF_CTL_MASK ); +wrmsr_on_cpu (policy->cpu,MSR_IA32_PERF_CTL , lo, hi); + + perf->state = next_perf_state;
Can use data->sfi_data->state = next_perf_state here directly and drop local var 'perf'.
Quoted 6 lines
drivers/cpufreq/sfi-cpufreq.c +6
-0+ +return 0 ; +} + +static int sfi_cpufreq_cpu_init (struct cpufreq_policy *policy) +{
drivers/cpufreq/sfi-cpufreq.c line 138
Quoted 10 lines
drivers/cpufreq/sfi-cpufreq.c +10
-0+unsigned int i, result =0 , valid_states =0 ; +unsigned int cpu = policy->cpu; +struct sfi_cpufreq_data *data; +struct sfi_cpufreq_performance *perf; + + data =kzalloc (sizeof (*data),GFP_KERNEL ); +if (!data) +return -ENOMEM ; + + data->sfi_data =per_cpu_ptr (sfi_perf_data, cpu);
I told you this before and this is still broken:
Quoted 2 lines
Your code is dependent on
policy->cpuat multiple places and it can change on CPU hotplug.
To explain again, in your case a single policy will have two CPUs. Groups are: 0-1 and 2-3.policy->cpu for both policies are 0 and 2.
If we hotplug out cpu 0 or cpu 2, policy->cpu will be updated to 1 and 3.
And then the above per-cpu variable access will return NULL, as you have initialized
them just for 0 and 2, not 1 and 3.
Quoted 42 lines
drivers/cpufreq/sfi-cpufreq.c +42
-0+per_cpu (drv_data, cpu) = data; + + result =sfi_cpufreq_register_performance (data->sfi_data); +if (result) + goto err_free; + + perf = data->sfi_data; + policy->shared_type =CPUFREQ_SHARED_TYPE_HW ; + + data->freq_table =kzalloc (sizeof (*data->freq_table) * + (perf->state_count+1 ),GFP_KERNEL ); +if (!data->freq_table) { + result = -ENOMEM ; + goto err_free; + } + + policy->cpuinfo.transition_latency =0 ; +for (i =0 ; i < perf->state_count; i++) { +/* detect transition latency */ +if ((perf->states[i].latency *1000 ) > + policy->cpuinfo.transition_latency) + policy->cpuinfo.transition_latency = + perf->states[i].latency *1000 ; + +/* initialize the freq table */ + data->freq_table[valid_states].driver_data = i; + data->freq_table[valid_states].frequency = + perf->states[i].freq_mhz *1000 ; + valid_states++; + +pr_debug (" %cP%d: %d MHz, %d uS\n" , + (i == perf->state ?'*' :' ' ), i, + (u32 ) perf->states[i].freq_mhz, + (u32 ) perf->states[i].latency); + } + + data->freq_table[valid_states].frequency =CPUFREQ_TABLE_END ; + perf->state =0 ; + + result =cpufreq_table_validate_and_show (policy, data->freq_table); +if (result) + goto err_freqfree;
drivers/cpufreq/sfi-cpufreq.c line 190
Quoted 10 lines
drivers/cpufreq/sfi-cpufreq.c +10
-0+ +pr_debug ("CPU%u - SFI performance management activated.\n" , cpu); + +return result; + +err_freqfree: +kfree (data->freq_table); +err_free: +kfree (data); +per_cpu (drv_data, cpu) =NULL ;
Order of these two instructions must be reversed..
Quoted 19 lines
drivers/cpufreq/sfi-cpufreq.c +19
-0+ +return result; +} + +static int sfi_cpufreq_cpu_exit (struct cpufreq_policy *policy) +{ +struct sfi_cpufreq_data *data =per_cpu (drv_data, policy->cpu); + +per_cpu (drv_data, policy->cpu) =NULL ; +kfree (data->freq_table); +kfree (data); + +return 0 ; +} + +static struct cpufreq_driver sfi_cpufreq_driver = { + .flags =CPUFREQ_CONST_LOOPS , + .get = get_cur_freq_on_cpu, + .verify = cpufreq_generic_frequency_table_verify,
drivers/cpufreq/sfi-cpufreq.c line 219
Quoted 10 lines
drivers/cpufreq/sfi-cpufreq.c +10
-0+ .target_index = sfi_cpufreq_target, + .init = sfi_cpufreq_cpu_init, + .exit = sfi_cpufreq_cpu_exit, + .name = "sfi-cpufreq", + .attr = cpufreq_generic_attr, +}; + +static int __init sfi_cpufreq_init (void ) +{ + sfi_perf_data = alloc_percpu(struct sfi_cpufreq_performance);
Why do you need a per-cpu struct for this? Its per-policy and so you better allocate it at runtime from ->init() call.
Quoted 24 lines
drivers/cpufreq/sfi-cpufreq.c +19
-1+if (!sfi_perf_data) { +pr_err ("Memory allocation error for sfi_perf_data.\n" ); +return -ENOMEM ; + } + +return cpufreq_register_driver (&sfi_cpufreq_driver); +} +late_initcall (sfi_cpufreq_init); + +static void __exit sfi_cpufreq_exit(void) +{ +cpufreq_unregister_driver (&sfi_cpufreq_driver); +free_percpu (sfi_perf_data); +} +module_exit (sfi_cpufreq_exit); + +MODULE_AUTHOR ("Vishwesh M Rudramuni <[email protected]>" ); +MODULE_DESCRIPTION ("SFI P-States Driver" ); +MODULE_LICENSE ("GPL" ); --1.7.9.5
--
Quoted 4 lines · Viresh Kumar
Quoted 2 lines · Srinidhi Kasagar
drivers/cpufreq/sfi-cpufreq.c +1
-0+static int parse_freq (struct sfi_table_header *table)
[..]
Quoted 16 lines
drivers/cpufreq/sfi-cpufreq.c line 138
Quoted 13 lines
drivers/cpufreq/sfi-cpufreq.c +13
-0+ +static int sfi_cpufreq_cpu_init (struct cpufreq_policy *policy) +{ +unsigned int i, result =0 , valid_states =0 ; +unsigned int cpu = policy->cpu; +struct sfi_cpufreq_data *data; +struct sfi_cpufreq_performance *perf; + + data =kzalloc (sizeof (*data),GFP_KERNEL ); +if (!data) +return -ENOMEM ; + + data->sfi_data =per_cpu_ptr (sfi_perf_data, cpu);I told you this before and this is still broken:
Quoted 6 lines
Quoted 2 lines
Your code is dependent on
policy->cpuat multiple places and it can change on CPU hotplug.To explain again, in your case a single policy will have two CPUs. Groups are: 0-1 and 2-3.
policy->cpufor both policies are 0 and 2.
Not anymore now..I have fixed it in my earlier version.
$ cat /sys/devices/system/cpu/cpu*/cpufreq/affected_cpus 0 1 2 3
$ cat /sys/devices/system/cpu/cpu*/cpufreq/related_cpus 0 1 2 3
Quoted 6 lines
If we hotplug out cpu 0 or cpu 2,
policy->cpuwill be updated to 1 and 3.And then the above per-cpu variable access will return NULL, as you have initialized
them just for 0 and 2, not 1 and 3.
Srinidhi
Viresh Kumar
maintainerQuoted 8 lines · Srinidhi
Not anymore now..I have fixed it in my earlier version.
$ cat /sys/devices/system/cpu/cpu*/cpufreq/affected_cpus 0 1 2 3
Quoted 5 lines
$ cat /sys/devices/system/cpu/cpu*/cpufreq/related_cpus 0 1 2 3
What do you mean by you have fixed it? Weren't the CPUs sharing clock lines? I hope cpu 0 and 1 should be sharing their clock lines, i.e. they change freq together.
Also same for cpu 2 and 3.
Quoted 15 lines · Viresh Kumar
Quoted 8 lines · Srinidhi
Not anymore now..I have fixed it in my earlier version.
$ cat /sys/devices/system/cpu/cpu*/cpufreq/affected_cpus 0 1 2 3Quoted 5 lines
$ cat /sys/devices/system/cpu/cpu*/cpufreq/related_cpus 0 1 2 3
Quoted 4 lines
What do you mean by you have fixed it? Weren't the CPUs sharing clock lines? I hope cpu 0 and 1 should be sharing their clock lines, i.e. they change freq together.
Also same for cpu 2 and 3.
The architecture supports both, and the current code allows me to change the freq independently and that's why the above output lines.
Srinidhi
Viresh Kumar
maintainerQuoted 3 lines · Srinidhi
The architecture supports both, and the current code allows me to change the freq independently and that's why the above output lines.
Which part of the code exactly controls this ?
Quoted 6 lines · Viresh Kumar
Quoted 3 lines · Srinidhi
The architecture supports both, and the current code allows me to change the freq independently and that's why the above output lines.
Which part of the code exactly controls this ?
That's the default behavior and I need not have to control it. What I did wrong was to report the output of out-of-my-tree kernel which leads to this confusion..
Srinidhi