Skip to contentSkip to conversation

cpufreq: Add SFI based cpufreq driver support

PATCH Superseded Srinidhi Kasagar 3 participants 5 messages Dec 18, 2014 – Dec 19, 2014

Subsystems (2)
x86 CPU frequency scaling framework

v8 of 9 Superseded Latest is v9 →

lore
Share & tools
lore

Srinidhi Kasagar

author
#0 lore raw

PATCH v8 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.

Signed-off-by Rudramuni, Vishwesh M <[email protected]>
Signed-off-by Srinidhi Kasagar <[email protected]>
Acked-by Viresh Kumar <[email protected]>

---
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

v6:
* release the mutex in case of error in register_performance (comments from Viresh)
* drop local perf pointer in ->target and ->init
* fix the order of freeing memories in the error path of ->init
* remove alloc_percpu, rather allocate it at runtime in ->init

v7:
(comments from Len Brown)
* drop the unnecessary mutex
* move sfi_table_parse to __init as it is global to the system
* drop ->get callback
* consolidate few global data structures

v8:
(Viresh)
* drop per-cpu usage
(Len Brown)
* drop the usage of SFI_FREQ_MAX
* remove the logic of detecting transition_latency
* remove the last redundant pr_debug in ->init

 arch/x86/include/uapi/asm/msr-index.h |    1 +
 drivers/cpufreq/Kconfig.x86           |   10 +++
 drivers/cpufreq/Makefile              |    1 +
 drivers/cpufreq/sfi-cpufreq.c         |  144 +++++++++++++++++++++++++++++++++
 4 files changed, 156 insertions(+)
 create mode 100644 drivers/cpufreq/sfi-cpufreq.c
arch/x86/include/uapi/asm/msr-index.h +1 -0
index e21331ce368f..4f6dae67dd10 100644
@@ -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
index 89ae88f91895..c59bdcb83217 100644
@@ -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 Performance-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
index db6d9a2fea4d..c3b51efd4a85 100644
@@ -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 +144 -0

144 lines not on this page · raw message

Signature · 1 line
1.7.9.5
maintainer Acked-by on v8 1/1
#1 lore raw
Quoted 56 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]>

---
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

v6:
* release the mutex in case of error in register_performance (comments from Viresh)
* drop local perf pointer in ->target and ->init
* fix the order of freeing memories in the error path of ->init
* remove alloc_percpu, rather allocate it at runtime in ->init

v7:
(comments from Len Brown)
* drop the unnecessary mutex
* move sfi_table_parse to __init as it is global to the system
* drop ->get callback
* consolidate few global data structures

Quoted 7 lines

v8:
(Viresh)
* drop per-cpu usage
(Len Brown)
* drop the usage of SFI_FREQ_MAX
* remove the logic of detecting transition_latency
* remove the last redundant pr_debug in ->init

Acked-by Viresh Kumar <[email protected]>
#2 lore raw
In reply to Srinidhi Kasagar
Quoted 1 line

v8:

...

Quoted 8 lines
drivers/cpufreq/sfi-cpufreq.c new file +2 -0
new file mode 100644
index 000000000000..f9a913242537
@@ -0,0 +1,144 @@
+/*
+ *  SFI Performance States Driver

...

drivers/cpufreq/sfi-cpufreq.c line 84

Quoted 13 lines
drivers/cpufreq/sfi-cpufreq.c new file +13 -0
+static int sfi_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+       struct cpufreq_frequency_table *freq_table;
+       unsigned int i, result;
+
+       freq_table = kzalloc(sizeof(*freq_table) *
+                       (num_freq_table_entries + 1), GFP_KERNEL);
+       if (!freq_table)
+               return -ENOMEM;
+
+       policy->shared_type = CPUFREQ_SHARED_TYPE_HW;
+
+       policy->cpuinfo.transition_latency = 100000;    /* 100us */

It would be better to use CPUFREQ_ETERNAL here
than to make up a number (100us) asserting that it is valid for all possible
cores that might run this driver. If we put a real number here, then people
will argue about how real it is when a different core runs this driver. The fact of the matter is that only ondemand *consumes* this number, and it compares it to 10,000,000 to see if can load. Then it compares MIN_LATENCY_MULTIPLIER (20) * latency, as a floor for min_sampling_rate, which would otherwise default to MICRO_FREQUENCY_MIN_SAMPLE_RATE (10000).

ie. all number you give here that are less than MICRO_FREQUENCY_MIN_SAMPLE_RATE/MIN_LATENCY_MULTIPLIER = 500us are equivalent.

So just use -1 and avoid getting into a debate as to what this number really is.

drivers/cpufreq/sfi-cpufreq.c line 86

Quoted 2 lines
drivers/cpufreq/sfi-cpufreq.c new file +2 -0
+       for (i = 0; i < num_freq_table_entries; i++) {
+               /* initialize the freq table */

above comment adds no value to the two lines below, so you can remove the comment.

drivers/cpufreq/sfi-cpufreq.c line 92

Quoted 6 lines
drivers/cpufreq/sfi-cpufreq.c new file +6 -0
+               freq_table[i].driver_data = i;
+               freq_table[i].frequency = sfi_cpufreq_array[i].freq_mhz * 1000;
+       }
+       freq_table[i].frequency = CPUFREQ_TABLE_END;
+
+       result = cpufreq_table_validate_and_show(policy, freq_table);

policy is PER_CPU
and sfi_cpufreq_cpu_init() will be called for every CPU in the system, yes? So here we are kzalloc-ing and initializing an identical freq_table for all the CPUs in the system.

But it does not appears that there is any per-cpu-specific data in that table.

As they are identical, can we allocate just one
and point all cpus to it? We would free that one
after the last CPU has exited.

Quoted 8 lines
drivers/cpufreq/sfi-cpufreq.c new file +8 -0
+       if (result)
+               goto err_free;
+
+       pr_debug("CPU%u - SFI freq driver activated.\n", policy->cpu);
+
+       return result;
+
+err_free:

drivers/cpufreq/sfi-cpufreq.c line 110

Quoted 10 lines
drivers/cpufreq/sfi-cpufreq.c new file +10 -0
+       kfree(freq_table);
+
+       return result;
+}
+
+static int sfi_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+       kfree(policy->freq_table);
+       return 0;
+}

If we have a single system-wide copy of the cpufreq_frequency_table, then we don't have any per-cpu state to clean-up,
and you can delete sfi_cpufreq_cpu_exit() entirely.

Quoted 18 lines
drivers/cpufreq/sfi-cpufreq.c new file +18 -0
+
+static struct cpufreq_driver sfi_cpufreq_driver = {
+       .flags          = CPUFREQ_CONST_LOOPS,
+       .verify         = cpufreq_generic_frequency_table_verify,
+       .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)
+{
+       int ret;
+
+       /* parse the freq table from SFI */
+       ret = sfi_table_parse(SFI_SIG_FREQ, NULL, NULL, sfi_parse_freq);
+       if (ret)

drivers/cpufreq/sfi-cpufreq.c line 138

Quoted 10 lines
drivers/cpufreq/sfi-cpufreq.c new file +10 -0
+               return ret;
+
+       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);
+       kfree(sfi_cpufreq_array);

Here is where you'd also kfree the single copy of the cpufreq_frequency_table

drivers/cpufreq/sfi-cpufreq.c

Quoted 7 lines
drivers/cpufreq/sfi-cpufreq.c new file +6 -1
+}
+module_exit(sfi_cpufreq_exit);
+
+MODULE_AUTHOR("Vishwesh M Rudramuni <[email protected]>");
+MODULE_DESCRIPTION("SFI Performance-States Driver");
+MODULE_LICENSE("GPL");
--

cheers,
Len Brown, Intel Open Source Technology Center

author
#3 lore raw
Quoted 35 lines · Len Brown
Quoted 1 line

v8:

...

Quoted 8 lines
drivers/cpufreq/sfi-cpufreq.c new file +2 -0
new file mode 100644
index 000000000000..f9a913242537
@@ -0,0 +1,144 @@
+/*
+ *  SFI Performance States Driver

...

drivers/cpufreq/sfi-cpufreq.c line 84

Quoted 13 lines
drivers/cpufreq/sfi-cpufreq.c new file +13 -0
+static int sfi_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+       struct cpufreq_frequency_table *freq_table;
+       unsigned int i, result;
+
+       freq_table = kzalloc(sizeof(*freq_table) *
+                       (num_freq_table_entries + 1), GFP_KERNEL);
+       if (!freq_table)
+               return -ENOMEM;
+
+       policy->shared_type = CPUFREQ_SHARED_TYPE_HW;
+
+       policy->cpuinfo.transition_latency = 100000;    /* 100us */

It would be better to use CPUFREQ_ETERNAL here
than to make up a number (100us) asserting that it is valid for all possible
cores that might run this driver. If we put a real number here, then people
will argue about how real it is when a different core runs this driver. The fact of the matter is that only ondemand *consumes* this number, and it compares it to 10,000,000 to see if can load. Then it compares MIN_LATENCY_MULTIPLIER (20) * latency, as a floor for min_sampling_rate, which would otherwise default to MICRO_FREQUENCY_MIN_SAMPLE_RATE (10000).

Quoted 5 lines

ie. all number you give here that are less than MICRO_FREQUENCY_MIN_SAMPLE_RATE/MIN_LATENCY_MULTIPLIER = 500us are equivalent.

So just use -1 and avoid getting into a debate as to what this number really is.

I don't get your point.

* Why should we say the transition_latency as "unknown" or ETERNAL knowing the fact that this system always has the latency of 100 us?
* If you set the transition_latency as -1/ETERNAL, then the interactive and ondemand governors will not load because the following condition will be true in __cpufreq_governor and thus fallback to performance.

if (policy->governor->max_transition_latency &&

            policy->cpuinfo.transition_latency >
            policy->governor->max_transition_latency) {

$cat /sys/devices/system/cpu/cpu1/cpufreq/cpuinfo_transition_latency 4294967295

am i missing something?

Quoted 19 lines

drivers/cpufreq/sfi-cpufreq.c line 86

Quoted 3 lines
drivers/cpufreq/sfi-cpufreq.c new file +2 -0
+       for (i = 0; i < num_freq_table_entries; i++) {
+               /* initialize the freq table */

above comment adds no value to the two lines below, so you can remove the comment.

drivers/cpufreq/sfi-cpufreq.c line 92

Quoted 6 lines
drivers/cpufreq/sfi-cpufreq.c new file +6 -0
+               freq_table[i].driver_data = i;
+               freq_table[i].frequency = sfi_cpufreq_array[i].freq_mhz * 1000;
+       }
+       freq_table[i].frequency = CPUFREQ_TABLE_END;
+
+       result = cpufreq_table_validate_and_show(policy, freq_table);

policy is PER_CPU
and sfi_cpufreq_cpu_init() will be called for every CPU in the system, yes? So here we are kzalloc-ing and initializing an identical freq_table for all the CPUs in the system.

Quoted 5 lines

But it does not appears that there is any per-cpu-specific data in that table.

As they are identical, can we allocate just one
and point all cpus to it? We would free that one
after the last CPU has exited.

Yes, this can be done now as there is no per-cpu specific data.

Srinidhi

#4 lore raw
Quoted 5 lines

* If you set the transition_latency as -1/ETERNAL, then the interactive and ondemand governors will not load because the following condition will be true in __cpufreq_governor and thus fallback to performance.

Quoted 8 lines

if (policy->governor->max_transition_latency &&

            policy->cpuinfo.transition_latency >
            policy->governor->max_transition_latency) {

$cat /sys/devices/system/cpu/cpu1/cpufreq/cpuinfo_transition_latency 4294967295

am i missing something?

Oops, I goofed.
When i saw the definition of
#define CPUFREQ_ETERNAL (-1)

I assumed that this was a signed comparison, but it is UN-signed, and so you are right -- we need to supply a positive number under 10000 to load ondemand, and it must be under 500 to have no effect
on ondemand's sample interval.

100 meets that criteria.

thanks,
Len Brown, Intel Open Source Technology Center