There has been lots of discussion over mailing lists about how to get CPU's clock sharing information for platforms as CPUFreq core depends on that. It doesn't look that there will be any immediate solution to that as there are multiple views over how to get that from DT.
But there are platforms (already upstreamed) which depend on this requirement and are waiting for a solution to sail through.
The common thing about them (Krait and Mvebu) is that all CPUs have independent clock lines and that makes things easier for us.
So, this patch creates another Kconfig option to indicate if all CPUs share same clock line or all have independent. The complex cases of multiple clusters having separate clock lines, but having same for all CPUs within a cluster isn't addressed yet.
This is a *short-term* solution for getting these platforms up an running and a DT based solution should be upstreamed soon.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- Yesterday again I was pinged by Thomas about the status of this work and the thread isn't going anywhere. He is blocked with just this to get his platform running on mainline.
Will it be possible to get this through as a temporary solution?
drivers/cpufreq/Kconfig | 9 +++++++++ drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index 3489f8f..a755227 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -196,6 +196,15 @@ config CPUFREQ_DT
If in doubt, say N.
+config CPUFREQ_DT_INDEPENDENT_CLOCKS + bool "CPUs change clocks independently" + depends on CPUFREQ_DT + help + Selecting this will make cpufreq-dt driver believe that all CPUs have + independent clock lines and so all will have separate 'struct + cpufreq_policy' instances for them. This must be replaced by proper DT + bindings later on. + menu "x86 CPU frequency scaling drivers" depends on X86 source "drivers/cpufreq/Kconfig.x86" diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index e002650..dbb6f22 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -266,9 +266,22 @@ static int cpufreq_init(struct cpufreq_policy *policy) policy->driver_data = priv;
policy->clk = cpu_clk; - ret = cpufreq_generic_init(policy, freq_table, transition_latency); - if (ret) + ret = cpufreq_table_validate_and_show(policy, freq_table); + if (ret) { + dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__, + ret); goto out_cooling_unregister; + } + + policy->cpuinfo.transition_latency = transition_latency; + +#ifndef CONFIG_CPUFREQ_DT_INDEPENDENT_CLOCKS + /* + * The driver only supports the SMP configuartion where all processors + * share the clock and voltage and clock. + */ + cpumask_setall(policy->cpus); +#endif
return 0;
Dear Viresh Kumar,
On Wed, 10 Sep 2014 09:59:07 +0530, Viresh Kumar wrote:
Yesterday again I was pinged by Thomas about the status of this work and the thread isn't going anywhere. He is blocked with just this to get his platform running on mainline.
Will it be possible to get this through as a temporary solution?
A compile time conditional does not work, as it isn't compatible with multiplatform: what if in the same kernel you want to support two platforms that use cpufreq-dt, one having a single clock for all CPUs, and one having one clock per CPU.
I'm still confused as to what is the problem with the DT binding surrounding this driver. Isn't it simply a matter of adding a clocks = <...> property to all CPUs, and that's it?
Viresh told me there was an issue related to comparing the clocks in the driver to find out which CPUs share which clocks. But that is not a DT binding issue, it's an implementation detail. Since the driver is purely for DT platforms (as its name suggest), comparing phandles could work as a temporary solution. And since it's just an implementation detail that doesn't affect the DT binding, we could change the implementation later to use a proper clock API like clk_is_same(clk1, clk2) that would tell us if the clocks are the same.
But I have probably missed some of the complexity of this.
Thanks,
Thomas
On 10 September 2014 11:59, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
A compile time conditional does not work, as it isn't compatible with multiplatform: what if in the same kernel you want to support two platforms that use cpufreq-dt, one having a single clock for all CPUs, and one having one clock per CPU.
Yeah, that's a known problem. Should have mentioned this in logs.
I'm still confused as to what is the problem with the DT binding surrounding this driver. Isn't it simply a matter of adding a clocks = <...> property to all CPUs, and that's it?
Viresh told me there was an issue related to comparing the clocks in the driver to find out which CPUs share which clocks. But that is not a DT binding issue, it's an implementation detail. Since the driver is purely for DT platforms (as its name suggest), comparing phandles could work as a temporary solution. And since it's just an implementation detail that doesn't affect the DT binding, we could change the implementation later to use a proper clock API like clk_is_same(clk1, clk2) that would tell us if the clocks are the same.
But I have probably missed some of the complexity of this.
(Couldn't give you these links yesterday as I was done for the day and was replying from my phone (In our private chat))
https://lkml.org/lkml/2014/6/26/577 https://lkml.org/lkml/2014/7/1/565 https://lkml.org/lkml/2014/7/3/813 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/275520.html
So these are few mails I could pick, specially from Mike, where he pointed out why getting a clk-API for this is a bad idea.
-- viresh
Hello,
Here is a proposal to solve the cpufreq-dt issue.
We don't seem to be anywhere near reaching an agreement on a DT binding to represent the topology of CPU clocks in a system, because it's a complicated matter. Mike doesn't want to add a clock API that would allow the generic cpufreq driver to find out which CPUs share clocks, as he believes this is part of the hardware description and should therefore be described in the DT rather than "guessed" dynamically at boot time by looking at the clocks referenced by each CPU. And with Viresh not accepting any machine specific driver, it results in platforms like Armada XP having no solution to support cpufreq...
So this proposal consists in adding a platform_data flag for the cpufreq-dt driver, which allows platform code to tell whether CPU clocks are shared or are independent.
If you don't like platform_data, we can also register two different platform_driver for the two different cases, simply with different names.
Another approach would be to lift the ban on machine-specific cpufreq drivers, since the generic driver is not capable of handling all situations.
Thomas
Thomas Petazzoni (4): cpufreq: allow driver-specific flags cpufreq: cpufreq-dt: extend with platform_data ARM: mvebu: use the cpufreq-dt platform_data for independent clocks cpufreq: cpufreq-dt: remove warning about regulators
arch/arm/mach-mvebu/pmsu.c | 8 +++++++- drivers/cpufreq/cpufreq-dt.c | 23 ++++++++++++++++++----- drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ include/linux/cpufreq.h | 5 ++++- 5 files changed, 66 insertions(+), 7 deletions(-) create mode 100644 include/linux/cpufreq-dt.h
Exactly like the cpuidle subsystem separates core cpuidle flags from driver-specific cpuidle flags, this commit extends the cpufreq subsystem with the same idea: the cpufreq_driver->flags field contain the existing core cpufreq flags in the low 16 bits, and driver-specific flags in the high 16 bits.
A new function called cpufreq_get_driver_flags() is added to allow a cpufreq driver to retrieve those flags, since they are typically needed from a cpufreq_policy->init() callback, which does not have access to the cpufreq_driver structure. This function call is similar to the existing cpufreq_get_current_driver() function call.
Signed-off-by: Thomas Petazzoni thomas.petazzoni@free-electrons.com --- drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ include/linux/cpufreq.h | 5 ++++- 2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index d9fdedd..e41b971 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1728,6 +1728,21 @@ const char *cpufreq_get_current_driver(void) } EXPORT_SYMBOL_GPL(cpufreq_get_current_driver);
+/** + * cpufreq_get_driver_flags - return current driver's flags + * + * Return the flags of the currently loaded cpufreq driver, or 0 + * if none. + */ +unsigned int cpufreq_get_driver_flags(void) +{ + if (cpufreq_driver) + return cpufreq_driver->flags & CPUFREQ_DRIVER_FLAGS_MASK; + + return 0; +} +EXPORT_SYMBOL_GPL(cpufreq_get_driver_flags); + /********************************************************************* * NOTIFIER LISTS INTERFACE * *********************************************************************/ diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 138336b..fa35601 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -218,7 +218,7 @@ __ATTR(_name, 0644, show_##_name, store_##_name)
struct cpufreq_driver { char name[CPUFREQ_NAME_LEN]; - u8 flags; + unsigned int flags;
/* needed by all drivers */ int (*init) (struct cpufreq_policy *policy); @@ -308,10 +308,13 @@ struct cpufreq_driver { */ #define CPUFREQ_NEED_INITIAL_FREQ_CHECK (1 << 5)
+#define CPUFREQ_DRIVER_FLAGS_MASK (0xFFFF0000) + int cpufreq_register_driver(struct cpufreq_driver *driver_data); int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
const char *cpufreq_get_current_driver(void); +unsigned int cpufreq_get_driver_flags(void);
static inline void cpufreq_verify_within_limits(struct cpufreq_policy *policy, unsigned int min, unsigned int max)
On 10 September 2014 15:11, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 138336b..fa35601 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -218,7 +218,7 @@ __ATTR(_name, 0644, show_##_name, store_##_name)
struct cpufreq_driver { char name[CPUFREQ_NAME_LEN];
u8 flags;
unsigned int flags; /* needed by all drivers */ int (*init) (struct cpufreq_policy *policy);@@ -308,10 +308,13 @@ struct cpufreq_driver { */ #define CPUFREQ_NEED_INITIAL_FREQ_CHECK (1 << 5)
+#define CPUFREQ_DRIVER_FLAGS_MASK (0xFFFF0000)
The flags field is used today to pass on information to cpufreq core and I believe it better stays that way only. Also all these changes might later be reverted and so we better don't change usage of variables..
Instead of this add a "void *driver_data" field in this structure and fill that with your structure. This can be later used for other purposes as well..
Also this will just add few more bytes to the cpufreq-driver structure which wouldn't have many instances in a compiled kernel, and so space isn't a problem..
Otherwise things looked good to me in your complete series..
Quoting Viresh Kumar (2014-09-10 03:49:25)
On 10 September 2014 15:11, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 138336b..fa35601 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -218,7 +218,7 @@ __ATTR(_name, 0644, show_##_name, store_##_name)
struct cpufreq_driver { char name[CPUFREQ_NAME_LEN];
u8 flags;
unsigned int flags; /* needed by all drivers */ int (*init) (struct cpufreq_policy *policy);@@ -308,10 +308,13 @@ struct cpufreq_driver { */ #define CPUFREQ_NEED_INITIAL_FREQ_CHECK (1 << 5)
+#define CPUFREQ_DRIVER_FLAGS_MASK (0xFFFF0000)
The flags field is used today to pass on information to cpufreq core and I believe it better stays that way only. Also all these changes might later be reverted and so we better don't change usage of variables..
I have another use case for exposing driver flags and I have created a similar patch to this locally in my git tree.
Basically some cpufreq drivers may block or sleep during their .set_target callback, and others will not. E.g. the former case is the common pattern to use the clock framework and the regulator framework, both of which hold mutexes and may have slow operations and the latter case is more like the Intel P-state driver with just some register writes.
For the energy-aware scheduling work it is desirable to know if the cpufreq driver can do a "fast" transition of it might sleep, since that affects whether we adjust the cpu frequency from the scheduler context of whether need to put new work on the workqueue (e.g. waking up a kthread dedicated to calling .set_target).
Regards, Mike
Instead of this add a "void *driver_data" field in this structure and fill that with your structure. This can be later used for other purposes as well..
Also this will just add few more bytes to the cpufreq-driver structure which wouldn't have many instances in a compiled kernel, and so space isn't a problem..
Otherwise things looked good to me in your complete series..
On 27 September 2014 15:44, Mike Turquette mike.turquette@linaro.org wrote:
I have another use case for exposing driver flags and I have created a similar patch to this locally in my git tree.
Basically some cpufreq drivers may block or sleep during their .set_target callback, and others will not. E.g. the former case is the common pattern to use the clock framework and the regulator framework, both of which hold mutexes and may have slow operations and the latter case is more like the Intel P-state driver with just some register writes.
For the energy-aware scheduling work it is desirable to know if the cpufreq driver can do a "fast" transition of it might sleep, since that affects whether we adjust the cpu frequency from the scheduler context of whether need to put new work on the workqueue (e.g. waking up a kthread dedicated to calling .set_target).
I wasn't against exposing the flags but using the same variable to store driver specific data.
Quoting Viresh Kumar (2014-09-29 01:54:51)
On 27 September 2014 15:44, Mike Turquette mike.turquette@linaro.org wrote:
I have another use case for exposing driver flags and I have created a similar patch to this locally in my git tree.
Basically some cpufreq drivers may block or sleep during their .set_target callback, and others will not. E.g. the former case is the common pattern to use the clock framework and the regulator framework, both of which hold mutexes and may have slow operations and the latter case is more like the Intel P-state driver with just some register writes.
For the energy-aware scheduling work it is desirable to know if the cpufreq driver can do a "fast" transition of it might sleep, since that affects whether we adjust the cpu frequency from the scheduler context of whether need to put new work on the workqueue (e.g. waking up a kthread dedicated to calling .set_target).
I wasn't against exposing the flags but using the same variable to store driver specific data.
For reference, here is a patch that I've had locally in my tree for a couples months. I'm using it while prototyping some ways to integrate CPUfreq with the scheduler. It is almost identical to Thomas's approach but it doesn't try to mask off some flags. Additionally it does NOT change the u8 return type, which is currently how struct cpufreq_driver->flags is defined today. This is probably not good and should be an unsigned int or unsigned long so that the api is a bit more forward-looking.
Thoughts?
Thanks, Mike
From 0053dfc6b09fa0c5923b7dcba23b67c1c005cb88 Mon Sep 17 00:00:00 2001 From: Mike Turquette mturquette@linaro.org Date: Sun, 3 Aug 2014 21:25:38 -0700 Subject: [PATCH] cpufreq: new function to query driver for flags
Signed-off-by: Mike Turquette mturquette@linaro.org --- drivers/cpufreq/cpufreq.c | 9 +++++++++ include/linux/cpufreq.h | 1 + 2 files changed, 10 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 1abb44d..515e905 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -2003,6 +2003,15 @@ int cpufreq_driver_target(struct cpufreq_policy *policy, } EXPORT_SYMBOL_GPL(cpufreq_driver_target);
+u8 cpufreq_driver_get_flags(void) +{ + if (!cpufreq_driver) + return 0; + + return cpufreq_driver->flags; +} +EXPORT_SYMBOL_GPL(cpufreq_driver_get_flags); + /* * when "event" is CPUFREQ_GOV_LIMITS */ diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 221f0eb..5a97f5f 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -417,6 +417,7 @@ int cpufreq_driver_target(struct cpufreq_policy *policy, int __cpufreq_driver_target(struct cpufreq_policy *policy, unsigned int target_freq, unsigned int relation); +u8 cpufreq_driver_get_flags(void); int cpufreq_register_governor(struct cpufreq_governor *governor); void cpufreq_unregister_governor(struct cpufreq_governor *governor);
On 5 October 2014 06:11, Mike Turquette mike.turquette@linaro.org wrote:
For reference, here is a patch that I've had locally in my tree for a couples months. I'm using it while prototyping some ways to integrate CPUfreq with the scheduler. It is almost identical to Thomas's approach but it doesn't try to mask off some flags. Additionally it does NOT change the u8 return type, which is currently how struct cpufreq_driver->flags is defined today. This is probably not good and should be an unsigned int or unsigned long so that the api is a bit more forward-looking.
Probably yes.
From 0053dfc6b09fa0c5923b7dcba23b67c1c005cb88 Mon Sep 17 00:00:00 2001 From: Mike Turquette mturquette@linaro.org Date: Sun, 3 Aug 2014 21:25:38 -0700 Subject: [PATCH] cpufreq: new function to query driver for flags
Signed-off-by: Mike Turquette mturquette@linaro.org
drivers/cpufreq/cpufreq.c | 9 +++++++++ include/linux/cpufreq.h | 1 + 2 files changed, 10 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 1abb44d..515e905 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -2003,6 +2003,15 @@ int cpufreq_driver_target(struct cpufreq_policy *policy, } EXPORT_SYMBOL_GPL(cpufreq_driver_target);
+u8 cpufreq_driver_get_flags(void) +{
if (!cpufreq_driver)return 0;
Probably you need to return some error here. How would the caller know if there is no cpufreq-driver there..
return cpufreq_driver->flags;+} +EXPORT_SYMBOL_GPL(cpufreq_driver_get_flags);
/*
- when "event" is CPUFREQ_GOV_LIMITS
*/ diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 221f0eb..5a97f5f 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -417,6 +417,7 @@ int cpufreq_driver_target(struct cpufreq_policy *policy, int __cpufreq_driver_target(struct cpufreq_policy *policy, unsigned int target_freq, unsigned int relation); +u8 cpufreq_driver_get_flags(void); int cpufreq_register_governor(struct cpufreq_governor *governor); void cpufreq_unregister_governor(struct cpufreq_governor *governor);
-- 1.8.3.2
Quoting Viresh Kumar (2014-10-05 20:56:26)
On 5 October 2014 06:11, Mike Turquette mike.turquette@linaro.org wrote:
For reference, here is a patch that I've had locally in my tree for a couples months. I'm using it while prototyping some ways to integrate CPUfreq with the scheduler. It is almost identical to Thomas's approach but it doesn't try to mask off some flags. Additionally it does NOT change the u8 return type, which is currently how struct cpufreq_driver->flags is defined today. This is probably not good and should be an unsigned int or unsigned long so that the api is a bit more forward-looking.
Probably yes.
From 0053dfc6b09fa0c5923b7dcba23b67c1c005cb88 Mon Sep 17 00:00:00 2001 From: Mike Turquette mturquette@linaro.org Date: Sun, 3 Aug 2014 21:25:38 -0700 Subject: [PATCH] cpufreq: new function to query driver for flags
Signed-off-by: Mike Turquette mturquette@linaro.org
drivers/cpufreq/cpufreq.c | 9 +++++++++ include/linux/cpufreq.h | 1 + 2 files changed, 10 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 1abb44d..515e905 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -2003,6 +2003,15 @@ int cpufreq_driver_target(struct cpufreq_policy *policy, } EXPORT_SYMBOL_GPL(cpufreq_driver_target);
+u8 cpufreq_driver_get_flags(void) +{
if (!cpufreq_driver)return 0;Probably you need to return some error here. How would the caller know if there is no cpufreq-driver there..
Viresh,
If I change the return type to int and replace 'return 0' with 'return -ENODEV', would you accep this patch?
Thomas,
Does such a solution solve your problem? Do you still need to create a platform_device if you can easily get at the driver flags with this function?
Regards, Mike
return cpufreq_driver->flags;+} +EXPORT_SYMBOL_GPL(cpufreq_driver_get_flags);
/*
- when "event" is CPUFREQ_GOV_LIMITS
*/ diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 221f0eb..5a97f5f 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -417,6 +417,7 @@ int cpufreq_driver_target(struct cpufreq_policy *policy, int __cpufreq_driver_target(struct cpufreq_policy *policy, unsigned int target_freq, unsigned int relation); +u8 cpufreq_driver_get_flags(void); int cpufreq_register_governor(struct cpufreq_governor *governor); void cpufreq_unregister_governor(struct cpufreq_governor *governor);
-- 1.8.3.2
Dear Mike Turquette,
On Mon, 06 Oct 2014 11:36:24 -0700, Mike Turquette wrote:
Thomas,
Does such a solution solve your problem? Do you still need to create a platform_device if you can easily get at the driver flags with this function?
Hum, I'm not sure to get the question: of course to instantiate cpufreq I need to create a platform_device. The only difference between my v1 and v2 is that in v1 I was using those driver flags to pass from the cpufreq ->probe() function to the cpufreq ->init() function that we have independent clocks, while in v2, I'm using a new void *driver_data field. Really, it doesn't change anything and is purely a matter of taste.
Best regards,
Thomas
On Mon, Oct 6, 2014 at 11:55 AM, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
Dear Mike Turquette,
On Mon, 06 Oct 2014 11:36:24 -0700, Mike Turquette wrote:
Thomas,
Does such a solution solve your problem? Do you still need to create a platform_device if you can easily get at the driver flags with this function?
Hum, I'm not sure to get the question: of course to instantiate cpufreq I need to create a platform_device. The only difference between my v1 and v2 is that in v1 I was using those driver flags to pass from the cpufreq ->probe() function to the cpufreq ->init() function that we have independent clocks, while in v2, I'm using a new void *driver_data field. Really, it doesn't change anything and is purely a matter of taste.
Poorly worded question. I was asking if the above patch would work for you and it seems the answer is yes.
The reason I care is that I am working on a governor that needs to know a driver flag. It would be painful for the machine-independent governor to understand a machine-specific private data structure from the driver. The flags are listed in cpufreq.h so it makes life easy.
Regards, Mike
Best regards,
Thomas
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
On 7 October 2014 03:14, Mike Turquette mturquette@linaro.org wrote:
Poorly worded question. I was asking if the above patch would work for you and it seems the answer is yes.
The reason I care is that I am working on a governor that needs to know a driver flag. It would be painful for the machine-independent governor to understand a machine-specific private data structure from the driver. The flags are listed in cpufreq.h so it makes life easy.
This patch isn't required anymore by thomas. All he needs is a flag local to cpufreq-cpu0 driver and we didn't wanted to abuse cpufreq_driver's flag field for that..
So this patch is just for your work..
On 7 October 2014 00:06, Mike Turquette mike.turquette@linaro.org wrote:
If I change the return type to int and replace 'return 0' with 'return -ENODEV', would you accep this patch?
Ofcourse :)
Does such a solution solve your problem? Do you still need to create a platform_device if you can easily get at the driver flags with this function?
Will reply to this on the last mail..
This series is partially in response to a discussion around DT bindings for CPUfreq drivers [0], but it is also needed for on-going work to integrate CPUfreq with the scheduler. In particular a scheduler-driven cpu frequency scaling policy would be well served to know if the underlying CPUfreq driver .target callback might sleep or block for a long time.
[0] http://lkml.kernel.org/r/fcb88cd21f31a467d2d49911c2505082837f72ea.1410323179.git.viresh.kumar@linaro.org
Mike Turquette (2): cpufreq: add driver flag for sleepable transitions cpufreq: new function to query driver for flags
drivers/cpufreq/cpufreq.c | 9 +++++++++ include/linux/cpufreq.h | 13 +++++++++++++ 2 files changed, 22 insertions(+)
The CPUfreq core does not differentiate between .target & .target_index callbacks that may sleep or block and callbacks that are fast and return immediately. To date this has not mattered much since the typical CPUfreq governor calls the .target callback from process context via a workqueue.
When calling the .target callback from a different context, such as a scheduler load balance operation (see Morten's "energy aware scheduler" RFC[0]), this detail matters.
This patch introduces a new CPUfreq driver flag for fast .target callbacks that are guaranteed to neither sleep nor block. Setting this flag allows for .target to be called from schedule() and other call sites that have interrupts disabled or other constraints. Drivers may set CPUFREQ_NO_SLEEP at driver registration-time.
The default is to not have this flag set, resulting in the need to defer calls to .target and .target_index.
[0] http://lkml.kernel.org/r/1381511957-29776-7-git-send-email-morten.rasmussen@arm.com
Signed-off-by: Mike Turquette mturquette@linaro.org --- include/linux/cpufreq.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 138336b..9034573 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -308,6 +308,19 @@ struct cpufreq_driver { */ #define CPUFREQ_NEED_INITIAL_FREQ_CHECK (1 << 5)
+/* + * Set by drivers whose .target or .target_index callback will never sleep or + * block. Setting this flag allows for more optimal code by calling .target + * from a context that otherwise would require deferring the work. + * + * An example use of this flag is when scaling cpu frequency from within a call + * to schedule() with interrupts disabled and runqueue locks held. + * + * This flag is not set by default, causing calls to .target to be done in + * process context. + */ +#define CPUFREQ_NO_SLEEP (1 << 6) + int cpufreq_register_driver(struct cpufreq_driver *driver_data); int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
There are cases for CPUfreq driver flags to be exposed outside of the CPUfreq core code. In particular the CPUFREQ_NO_SLEEP flag can be used by CPUfreq governors to optimize when and how they call that drivers .target callback. In fact this knowledge is a requirement for the on-going work to initiate cpu frequency transitions from the scheduler.
This patch implements a simple function to return the CPUfreq driver flags. While currently the flags are a u8, that could grow at a future date and cpufreq_driver_get_flags returns an int. Additionally the function needs to return an error in case no driver is registered.
Signed-off-by: Mike Turquette mturquette@linaro.org --- drivers/cpufreq/cpufreq.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 9b471b2..f3b9042 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1980,6 +1980,15 @@ int cpufreq_driver_target(struct cpufreq_policy *policy, } EXPORT_SYMBOL_GPL(cpufreq_driver_target);
+int cpufreq_driver_get_flags(void) +{ + if (!cpufreq_driver) + return -ENODEV; + + return cpufreq_driver->flags; +} +EXPORT_SYMBOL_GPL(cpufreq_driver_get_flags); + /* * when "event" is CPUFREQ_GOV_LIMITS */
On 8 October 2014 13:18, Mike Turquette mturquette@linaro.org wrote:
This series is partially in response to a discussion around DT bindings for CPUfreq drivers [0], but it is also needed for on-going work to integrate CPUfreq with the scheduler. In particular a scheduler-driven cpu frequency scaling policy would be well served to know if the underlying CPUfreq driver .target callback might sleep or block for a long time.
[0] http://lkml.kernel.org/r/fcb88cd21f31a467d2d49911c2505082837f72ea.1410323179.git.viresh.kumar@linaro.org
Firstly this link is broken and then the last comment I gave in Thomas's thread was that he doesn't need this routine anymore. And so your scheduler work is the only user of it.. And so probably we should get this included with the scheduler patches only, isn't it? Just including them without any user wouldn't benefit..
Or am I missing something ?
Dear Viresh Kumar,
On Wed, 8 Oct 2014 13:24:30 +0530, Viresh Kumar wrote:
On 8 October 2014 13:18, Mike Turquette mturquette@linaro.org wrote:
This series is partially in response to a discussion around DT bindings for CPUfreq drivers [0], but it is also needed for on-going work to integrate CPUfreq with the scheduler. In particular a scheduler-driven cpu frequency scaling policy would be well served to know if the underlying CPUfreq driver .target callback might sleep or block for a long time.
[0] http://lkml.kernel.org/r/fcb88cd21f31a467d2d49911c2505082837f72ea.1410323179.git.viresh.kumar@linaro.org
Firstly this link is broken and then the last comment I gave in Thomas's thread was that he doesn't need this routine anymore. And so your scheduler work is the only user of it.. And so probably we should get this included with the scheduler patches only, isn't it? Just including them without any user wouldn't benefit..
Or am I missing something ?
Well, when one has to merge a large number of changes, we often recommend to merge them piece by piece, which is what Mike is trying to do here. So we cannot at the same time ask developers to merge things in small pieces that are easy to review and to merge everything together because the users of a given API are not there yet.
Thomas
On 8 October 2014 13:41, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
On Wed, 8 Oct 2014 13:24:30 +0530, Viresh Kumar wrote:
On 8 October 2014 13:18, Mike Turquette mturquette@linaro.org wrote:
This series is partially in response to a discussion around DT bindings for CPUfreq drivers [0], but it is also needed for on-going work to integrate CPUfreq with the scheduler. In particular a scheduler-driven
Well, when one has to merge a large number of changes, we often recommend to merge them piece by piece, which is what Mike is trying to do here. So we cannot at the same time ask developers to merge things in small pieces that are easy to review and to merge everything together because the users of a given API are not there yet.
From the wording of Mike it looks like:
- This is required by cpufreq drivers - today - And this will also be useful for scheduler
The first point doesn't stand true anymore. Lets wait for Mike's reply and see his opinion.
And then the patches are so small and there are no objections against them. I don't think getting them with the scheduler changes is that bad of an idea. In worst case, what if he has to redesign his idea? The new routines will stay without a caller then :)
-- viresh
Quoting Viresh Kumar (2014-10-08 01:19:40)
On 8 October 2014 13:41, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
On Wed, 8 Oct 2014 13:24:30 +0530, Viresh Kumar wrote:
On 8 October 2014 13:18, Mike Turquette mturquette@linaro.org wrote:
This series is partially in response to a discussion around DT bindings for CPUfreq drivers [0], but it is also needed for on-going work to integrate CPUfreq with the scheduler. In particular a scheduler-driven
Well, when one has to merge a large number of changes, we often recommend to merge them piece by piece, which is what Mike is trying to do here. So we cannot at the same time ask developers to merge things in small pieces that are easy to review and to merge everything together because the users of a given API are not there yet.
From the wording of Mike it looks like:
- This is required by cpufreq drivers - today
- And this will also be useful for scheduler
Hi Viresh and Thomas,
Apologies if my wording was confusing. Without getting into a grammar war, I did say that these patches were "in response" to this thread (entirely accurate) and only necessary for the "on-going work" I'm doing with the scheduler. Sorry if any of that came across as me stating that these patches were necessary to solve Thomas' problem.
The first point doesn't stand true anymore. Lets wait for Mike's reply and see his opinion.
And then the patches are so small and there are no objections against them. I don't think getting them with the scheduler changes is that bad of an idea. In worst case, what if he has to redesign his idea? The new routines will stay without a caller then :)
Whether you merge the patches now or later is fine by me. I prefer to get my patches out early and often so I can avoid any surprises later on. If you have a fundamental objection to these patches then please let me know. Otherwise it would be wonderful to have an Ack.
Thanks! Mike
-- viresh
On 9 October 2014 05:31, Mike Turquette mturquette@linaro.org wrote:
Whether you merge the patches now or later is fine by me. I prefer to get my patches out early and often so I can avoid any surprises later on. If you have a fundamental objection to these patches then please let me know. Otherwise it would be wonderful to have an Ack.
Acked-by: Viresh Kumar viresh.kumar@linaro.org
But I would still ask you to get these merged with scheduler changes. :)
This commit extends the cpufreq-dt driver to take a platform_data structure. This structure is for now used to tell the cpufreq-dt driver the layout of the clocks on the platform, i.e whether all CPUs share the same clock or whether each CPU has a separate clock.
Signed-off-by: Thomas Petazzoni thomas.petazzoni@free-electrons.com --- drivers/cpufreq/cpufreq-dt.c | 20 ++++++++++++++++++-- include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 include/linux/cpufreq-dt.h
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index e002650..35d396e 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -18,6 +18,7 @@ #include <linux/cpu.h> #include <linux/cpu_cooling.h> #include <linux/cpufreq.h> +#include <linux/cpufreq-dt.h> #include <linux/cpumask.h> #include <linux/err.h> #include <linux/module.h> @@ -28,6 +29,9 @@ #include <linux/slab.h> #include <linux/thermal.h>
+/* cpufreq_driver->flags values */ +#define CPUFREQ_DT_INDEPENDENT_CLOCKS (1 << 16) + struct private_data { struct device *cpu_dev; struct regulator *cpu_reg; @@ -266,9 +270,17 @@ static int cpufreq_init(struct cpufreq_policy *policy) policy->driver_data = priv;
policy->clk = cpu_clk; - ret = cpufreq_generic_init(policy, freq_table, transition_latency); - if (ret) + ret = cpufreq_table_validate_and_show(policy, freq_table); + if (ret) { + dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__, + ret); goto out_cooling_unregister; + } + + policy->cpuinfo.transition_latency = transition_latency; + + if (!(cpufreq_get_driver_flags() & CPUFREQ_DT_INDEPENDENT_CLOCKS)) + cpumask_setall(policy->cpus);
return 0;
@@ -314,6 +326,7 @@ static struct cpufreq_driver dt_cpufreq_driver = {
static int dt_cpufreq_probe(struct platform_device *pdev) { + struct cpufreq_dt_platform_data *pd = dev_get_platdata(&pdev->dev); struct device *cpu_dev; struct regulator *cpu_reg; struct clk *cpu_clk; @@ -334,6 +347,9 @@ static int dt_cpufreq_probe(struct platform_device *pdev) if (!IS_ERR(cpu_reg)) regulator_put(cpu_reg);
+ if (pd->independent_clocks) + dt_cpufreq_driver.flags |= CPUFREQ_DT_INDEPENDENT_CLOCKS; + ret = cpufreq_register_driver(&dt_cpufreq_driver); if (ret) dev_err(cpu_dev, "failed register driver: %d\n", ret); diff --git a/include/linux/cpufreq-dt.h b/include/linux/cpufreq-dt.h new file mode 100644 index 0000000..0414009 --- /dev/null +++ b/include/linux/cpufreq-dt.h @@ -0,0 +1,22 @@ +/* + * Copyright (C) 2014 Marvell + * Thomas Petazzoni thomas.petazzoni@free-electrons.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __CPUFREQ_DT_H__ +#define __CPUFREQ_DT_H__ + +struct cpufreq_dt_platform_data { + /* + * True when each CPU has its own clock to control its + * frequency, false when all CPUs are controlled by a single + * clock. + */ + bool independent_clocks; +}; + +#endif /* __CPUFREQ_DT_H__ */
This commit adjusts the registration of the cpufreq-dt driver in the mvebu platform to indicate to the cpufreq driver that the platform has independent clocks for each CPU.
Signed-off-by: Thomas Petazzoni thomas.petazzoni@free-electrons.com --- arch/arm/mach-mvebu/pmsu.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c index bbd8664..c81dd5f 100644 --- a/arch/arm/mach-mvebu/pmsu.c +++ b/arch/arm/mach-mvebu/pmsu.c @@ -20,6 +20,7 @@
#include <linux/clk.h> #include <linux/cpu_pm.h> +#include <linux/cpufreq-dt.h> #include <linux/delay.h> #include <linux/init.h> #include <linux/io.h> @@ -572,6 +573,10 @@ int mvebu_pmsu_dfs_request(int cpu) return 0; }
+struct cpufreq_dt_platform_data cpufreq_dt_pd = { + .independent_clocks = true, +}; + static int __init armada_xp_pmsu_cpufreq_init(void) { struct device_node *np; @@ -644,7 +649,8 @@ static int __init armada_xp_pmsu_cpufreq_init(void) } }
- platform_device_register_simple("cpufreq-dt", -1, NULL, 0); + platform_device_register_data(NULL, "cpufreq-dt", -1, + &cpufreq_dt_pd, sizeof(cpufreq_dt_pd)); return 0; }
The cpufreq-dt driver tries to get a regulator for each CPU. This regulator is optional, but when not present, a scary message "failed to get cpuX regulator" is displayed. Since the regulator is really optional, we cannot sanely display any useful message, so this commit simply gets rid of this problematic message.
Signed-off-by: Thomas Petazzoni thomas.petazzoni@free-electrons.com --- drivers/cpufreq/cpufreq-dt.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 35d396e..8060022 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -149,9 +149,6 @@ try_again: reg = reg_cpu; goto try_again; } - - dev_warn(cpu_dev, "failed to get cpu%d regulator: %ld\n", - cpu, PTR_ERR(cpu_reg)); }
cpu_clk = clk_get(cpu_dev, NULL);
On 10 September 2014 15:11, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
The cpufreq-dt driver tries to get a regulator for each CPU. This regulator is optional, but when not present, a scary message "failed to get cpuX regulator" is displayed. Since the regulator is really optional, we cannot sanely display any useful message, so this commit simply gets rid of this problematic message.
Signed-off-by: Thomas Petazzoni thomas.petazzoni@free-electrons.com
drivers/cpufreq/cpufreq-dt.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 35d396e..8060022 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -149,9 +149,6 @@ try_again: reg = reg_cpu; goto try_again; }
dev_warn(cpu_dev, "failed to get cpu%d regulator: %ld\n",cpu, PTR_ERR(cpu_reg));
What about converting it to dev_dbg() instead?
On Wednesday 10 September 2014 11:41:48 Thomas Petazzoni wrote:
We don't seem to be anywhere near reaching an agreement on a DT binding to represent the topology of CPU clocks in a system, because it's a complicated matter. Mike doesn't want to add a clock API that would allow the generic cpufreq driver to find out which CPUs share clocks, as he believes this is part of the hardware description and should therefore be described in the DT rather than "guessed" dynamically at boot time by looking at the clocks referenced by each CPU. And with Viresh not accepting any machine specific driver, it results in platforms like Armada XP having no solution to support cpufreq...
So this proposal consists in adding a platform_data flag for the cpufreq-dt driver, which allows platform code to tell whether CPU clocks are shared or are independent.
If you don't like platform_data, we can also register two different platform_driver for the two different cases, simply with different names.
Another approach would be to lift the ban on machine-specific cpufreq drivers, since the generic driver is not capable of handling all situations.
Using platform_data works nicely if you register the cpufreq device from the platform code, but that is a particular thing that has been bugging me for a long time and that I really want to get rid of: We don't want to introduce platform-specific files for ARM64, so we have to solve this anyway by just looking at DT, and whatever solution we end up with should work for both ARM32 and ARM64.
Most of us will be at LCU next week, so I'd suggest we solve this problem using the 'lock everyone into one room without beer until we come up with a working approach' method.
Arnd
Arnd,
On Wed, 10 Sep 2014 11:53:27 +0200, Arnd Bergmann wrote:
Another approach would be to lift the ban on machine-specific cpufreq drivers, since the generic driver is not capable of handling all situations.
Using platform_data works nicely if you register the cpufreq device from the platform code, but that is a particular thing that has been bugging me for a long time and that I really want to get rid of: We don't want to introduce platform-specific files for ARM64, so we have to solve this anyway by just looking at DT, and whatever solution we end up with should work for both ARM32 and ARM64.
Right. Note that there is a similar issue with cpuidle.
Maybe each cpufreq/cpuidle driver should simply list of top-level compatible strings it is compatible with, so that at boot time, each cpufreq/cpuidle driver gets a chance to init itself and see whether it is matching the currently running platform.
Most of us will be at LCU next week, so I'd suggest we solve this problem using the 'lock everyone into one room without beer until we come up with a working approach' method.
That does seem like an interesting idea. However, I initially wanted to push cpufreq support for Armada XP in 3.17, and everything was pushed *except* the cpufreq driver bits. So I was hoping to at least get things working for 3.18. If the discussion takes place next week, and then the time to finally implement something, we'll also miss 3.18.
In the mean time, would it be possible to realize that the existing generic cpufreq driver simply isn't generic enough, and that we should accept machine-specific cpufreq drivers for the time being?
Thanks,
Thomas
On Wednesday 10 September 2014 12:10:02 Thomas Petazzoni wrote:
On Wed, 10 Sep 2014 11:53:27 +0200, Arnd Bergmann wrote:
Another approach would be to lift the ban on machine-specific cpufreq drivers, since the generic driver is not capable of handling all situations.
Using platform_data works nicely if you register the cpufreq device from the platform code, but that is a particular thing that has been bugging me for a long time and that I really want to get rid of: We don't want to introduce platform-specific files for ARM64, so we have to solve this anyway by just looking at DT, and whatever solution we end up with should work for both ARM32 and ARM64.
Right. Note that there is a similar issue with cpuidle.
Maybe each cpufreq/cpuidle driver should simply list of top-level compatible strings it is compatible with, so that at boot time, each cpufreq/cpuidle driver gets a chance to init itself and see whether it is matching the currently running platform.
The main problem I see with that is that it requires updating the driver for every new platform we want to support. It would be better to find a way that lets you add new platforms using just new dts files that work with the existing driver.
This is the main reason why we try to never match the root compatible property in any driver, and I don't think cpufreq should be an exception to this just because it can't easily be mapped to a particular register set.
Most of us will be at LCU next week, so I'd suggest we solve this problem using the 'lock everyone into one room without beer until we come up with a working approach' method.
That does seem like an interesting idea. However, I initially wanted to push cpufreq support for Armada XP in 3.17, and everything was pushed *except* the cpufreq driver bits. So I was hoping to at least get things working for 3.18. If the discussion takes place next week, and then the time to finally implement something, we'll also miss 3.18.
I see.
In the mean time, would it be possible to realize that the existing generic cpufreq driver simply isn't generic enough, and that we should accept machine-specific cpufreq drivers for the time being?
That would work for me, to get things going for 3.18, but it's really up to the cpufreq maintainers. Your platform_data based patch set seems like a better solution to me than separate drivers, but I haven't looked at the details.
Arnd
Arnd,
On Wed, 10 Sep 2014 12:19:41 +0200, Arnd Bergmann wrote:
the driver for every new platform we want to support. It would be better to find a way that lets you add new platforms using just new dts files that work with the existing driver.
What do you call a "platform" ? A board or a SoC ? The top-level compatible string contain both references to the board and the SoC, and my idea was obviously that the cpufreq driver would match against the SoC compatible string, not the board one. This definitely allows to support additional boards by adding more dts without touching the code.
Of course, it however means that when a new SoC shows up, you'll need to adjust the cpufreq driver. But you generally anyway need some kernel changes to support a new SoC: .dtsi changes are rarely sufficient to completely support a new chip.
In the mean time, would it be possible to realize that the existing generic cpufreq driver simply isn't generic enough, and that we should accept machine-specific cpufreq drivers for the time being?
That would work for me, to get things going for 3.18, but it's really up to the cpufreq maintainers. Your platform_data based patch set seems like a better solution to me than separate drivers, but I haven't looked at the details.
I personally don't really care: using platform_data, adding a machine-specific driver, having two platform_driver for cpufreq-dt with different names.
Viresh ?
Thomas
On 09/10/14 02:53, Arnd Bergmann wrote:
Most of us will be at LCU next week, so I'd suggest we solve this problem using the 'lock everyone into one room without beer until we come up with a working approach' method.
I'll be at connect Tuesday to mid-Thursday. I'd like to be included if such a discussion happens.
On 10 September 2014 15:11, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
And with Viresh not accepting any machine specific driver, it
Its not that we aren't accepting any drivers at all, but if we can reuse existing infrastructure then we better use it for long term maintainability.
But there is another view here which Mike objected to, sometime back. Its also not a good idea to push everything onto a virtual-cpu-clock driver..
I had something in mind to get that fixed, but this thread hasn't allowed me to work on that as it should be finished first.
Probably we can add some callbacks to the cpufreq-cpu0 driver. For example if somebody wants to do something crazy enough in their ->target() routine then they can just register their callback and we will call it instead of cpufreq-dt's target() routine..
That way we can still reuse the common part..
results in platforms like Armada XP having no solution to support cpufreq...
So this proposal consists in adding a platform_data flag for the cpufreq-dt driver, which allows platform code to tell whether CPU clocks are shared or are independent.
If you don't like platform_data, we can also register two different platform_driver for the two different cases, simply with different names.
Another approach would be to lift the ban on machine-specific cpufreq drivers, since the generic driver is not capable of handling all situations.
The platform-data solution is better than any other here :), and that will get fixed once we get all this including which driver to probe from DT..
Dear Viresh Kumar,
On Wed, 10 Sep 2014 16:08:51 +0530, Viresh Kumar wrote:
Its not that we aren't accepting any drivers at all, but if we can reuse existing infrastructure then we better use it for long term maintainability.
Sure, but when "existing infrastructure" isn't ready to support a particular use case, and there seems to be no solution on which people are agreeing?
But there is another view here which Mike objected to, sometime back. Its also not a good idea to push everything onto a virtual-cpu-clock driver..
I had something in mind to get that fixed, but this thread hasn't allowed me to work on that as it should be finished first.
Probably we can add some callbacks to the cpufreq-cpu0 driver. For example if somebody wants to do something crazy enough in their ->target() routine then they can just register their callback and we will call it instead of cpufreq-dt's target() routine..
That way we can still reuse the common part..
I personally don't think it would be a very good idea: in this case, you'd better do a complete cpufreq driver on its own. Doing subsystems in subsystems just for the sake of factorizing 10 lines of initialization seems a bit silly to me.
The platform-data solution is better than any other here :), and that will get fixed once we get all this including which driver to probe from DT..
Ok, I'll respin with a different solution than the flags.
Thanks,
Thomas
Hello,
Here is the v2 of the platform_data based proposal to make cpufreq-dt understand when it has a shared clock by all CPUs vs. independent clock for each CPU.
Changes since v1:
- Use a 'void *driver_data' in cpufreq_driver instead of extending the existing flags field. Suggested by Viresh.
- Keep the regulator related warning in cpufreq-dt, but reword it, and move it to the dev_dbg() level instead of dev_warn().
Thanks,
Thomas
Thomas Petazzoni (4): cpufreq: allow driver-specific data cpufreq: cpufreq-dt: extend with platform_data ARM: mvebu: use the cpufreq-dt platform_data for independent clocks cpufreq: cpufreq-dt: adjust message related to regulators
arch/arm/mach-mvebu/pmsu.c | 8 +++++++- drivers/cpufreq/cpufreq-dt.c | 21 +++++++++++++++++---- drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ include/linux/cpufreq.h | 2 ++ 5 files changed, 63 insertions(+), 5 deletions(-) create mode 100644 include/linux/cpufreq-dt.h
This commit extends the cpufreq_driver structure with an additional 'void *driver_data' field that can be filled by the ->probe() function of a cpufreq driver to pass additional custom information to the driver itself.
A new function called cpufreq_get_driver_data() is added to allow a cpufreq driver to retrieve those driver data, since they are typically needed from a cpufreq_policy->init() callback, which does not have access to the cpufreq_driver structure. This function call is similar to the existing cpufreq_get_current_driver() function call.
Signed-off-by: Thomas Petazzoni thomas.petazzoni@free-electrons.com --- drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ include/linux/cpufreq.h | 2 ++ 2 files changed, 17 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index d9fdedd..3b01d59 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1728,6 +1728,21 @@ const char *cpufreq_get_current_driver(void) } EXPORT_SYMBOL_GPL(cpufreq_get_current_driver);
+/** + * cpufreq_get_driver_data - return current driver data + * + * Return the private data of the currently loaded cpufreq + * driver, or NULL if no cpufreq driver is loaded. + */ +void *cpufreq_get_driver_data(void) +{ + if (cpufreq_driver) + return cpufreq_driver->driver_data; + + return NULL; +} +EXPORT_SYMBOL_GPL(cpufreq_get_driver_data); + /********************************************************************* * NOTIFIER LISTS INTERFACE * *********************************************************************/ diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 138336b..503b085 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -219,6 +219,7 @@ __ATTR(_name, 0644, show_##_name, store_##_name) struct cpufreq_driver { char name[CPUFREQ_NAME_LEN]; u8 flags; + void *driver_data;
/* needed by all drivers */ int (*init) (struct cpufreq_policy *policy); @@ -312,6 +313,7 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data); int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
const char *cpufreq_get_current_driver(void); +void *cpufreq_get_driver_data(void);
static inline void cpufreq_verify_within_limits(struct cpufreq_policy *policy, unsigned int min, unsigned int max)
This commit extends the cpufreq-dt driver to take a platform_data structure. This structure is for now used to tell the cpufreq-dt driver the layout of the clocks on the platform, i.e whether all CPUs share the same clock or whether each CPU has a separate clock.
Signed-off-by: Thomas Petazzoni thomas.petazzoni@free-electrons.com --- drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++++++-- include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 include/linux/cpufreq-dt.h
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index e002650..987bbfa 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -18,6 +18,7 @@ #include <linux/cpu.h> #include <linux/cpu_cooling.h> #include <linux/cpufreq.h> +#include <linux/cpufreq-dt.h> #include <linux/cpumask.h> #include <linux/err.h> #include <linux/module.h> @@ -178,6 +179,7 @@ try_again:
static int cpufreq_init(struct cpufreq_policy *policy) { + struct cpufreq_dt_platform_data *pd; struct cpufreq_frequency_table *freq_table; struct thermal_cooling_device *cdev; struct device_node *np; @@ -266,9 +268,18 @@ static int cpufreq_init(struct cpufreq_policy *policy) policy->driver_data = priv;
policy->clk = cpu_clk; - ret = cpufreq_generic_init(policy, freq_table, transition_latency); - if (ret) + ret = cpufreq_table_validate_and_show(policy, freq_table); + if (ret) { + dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__, + ret); goto out_cooling_unregister; + } + + policy->cpuinfo.transition_latency = transition_latency; + + pd = cpufreq_get_driver_data(); + if (pd && !pd->independent_clocks) + cpumask_setall(policy->cpus);
return 0;
@@ -334,6 +345,8 @@ static int dt_cpufreq_probe(struct platform_device *pdev) if (!IS_ERR(cpu_reg)) regulator_put(cpu_reg);
+ dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev); + ret = cpufreq_register_driver(&dt_cpufreq_driver); if (ret) dev_err(cpu_dev, "failed register driver: %d\n", ret); diff --git a/include/linux/cpufreq-dt.h b/include/linux/cpufreq-dt.h new file mode 100644 index 0000000..0414009 --- /dev/null +++ b/include/linux/cpufreq-dt.h @@ -0,0 +1,22 @@ +/* + * Copyright (C) 2014 Marvell + * Thomas Petazzoni thomas.petazzoni@free-electrons.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __CPUFREQ_DT_H__ +#define __CPUFREQ_DT_H__ + +struct cpufreq_dt_platform_data { + /* + * True when each CPU has its own clock to control its + * frequency, false when all CPUs are controlled by a single + * clock. + */ + bool independent_clocks; +}; + +#endif /* __CPUFREQ_DT_H__ */
On 10 September 2014 17:38, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev);
I don't think this is right. What if platform device's platform data is freed later? That's why its always better to duplicate that structure instead of playing with pointers.
Dear Viresh Kumar,
On Wed, 10 Sep 2014 17:52:59 +0530, Viresh Kumar wrote:
On 10 September 2014 17:38, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev);I don't think this is right. What if platform device's platform data is freed later? That's why its always better to duplicate that structure instead of playing with pointers.
Isn't the piece of code registering the platform_device supposed to make sure that platform_data doesn't disappear? At least, in PATCH 3/4, I'm using platform_device_register_data(), which does a kmemdup() of the custom data being passed before assigning the struct device->platform_data field.
But if you like, I can add one more memory copy :)
Thomas
On 10 September 2014 17:58, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
Dear Viresh Kumar,
On Wed, 10 Sep 2014 17:52:59 +0530, Viresh Kumar wrote:
On 10 September 2014 17:38, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev);I don't think this is right. What if platform device's platform data is freed later? That's why its always better to duplicate that structure instead of playing with pointers.
Isn't the piece of code registering the platform_device supposed to make sure that platform_data doesn't disappear? At least, in PATCH 3/4,
I don't know. I remember this from the days when I used to write individual drivers for SPEAr platform... Its been some time now that I have seen this :)
I'm using platform_device_register_data(), which does a kmemdup() of the custom data being passed before assigning the struct device->platform_data field.
Atleast in your case it isn't required to copy anymore but this driver can be used by others which may not guarantee that..
But if you like, I can add one more memory copy :)
Its not what I like, as wasting memory isn't sensible at all.. But about what's the right thing to do to make this code un-breakable..
@Arnd: Any inputs?
On Wednesday 10 September 2014 18:02:59 Viresh Kumar wrote:
I'm using platform_device_register_data(), which does a kmemdup() of the custom data being passed before assigning the struct device->platform_data field.
Atleast in your case it isn't required to copy anymore but this driver can be used by others which may not guarantee that..
But if you like, I can add one more memory copy
Its not what I like, as wasting memory isn't sensible at all.. But about what's the right thing to do to make this code un-breakable..
@Arnd: Any inputs?
platform_device_register_data() is the right way to register a platform device if one needs to do that, and it will always copy the data, so that should be used, and the driver should rely on the data not getting freed under it.
Statically defining platform_device instances is generally a bad idea, and we should not do that, but if someone does it is up to them to make sure the platform_data is not marked as __init.
Arnd
This commit adjusts the registration of the cpufreq-dt driver in the mvebu platform to indicate to the cpufreq driver that the platform has independent clocks for each CPU.
Signed-off-by: Thomas Petazzoni thomas.petazzoni@free-electrons.com --- arch/arm/mach-mvebu/pmsu.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c index bbd8664..c81dd5f 100644 --- a/arch/arm/mach-mvebu/pmsu.c +++ b/arch/arm/mach-mvebu/pmsu.c @@ -20,6 +20,7 @@
#include <linux/clk.h> #include <linux/cpu_pm.h> +#include <linux/cpufreq-dt.h> #include <linux/delay.h> #include <linux/init.h> #include <linux/io.h> @@ -572,6 +573,10 @@ int mvebu_pmsu_dfs_request(int cpu) return 0; }
+struct cpufreq_dt_platform_data cpufreq_dt_pd = { + .independent_clocks = true, +}; + static int __init armada_xp_pmsu_cpufreq_init(void) { struct device_node *np; @@ -644,7 +649,8 @@ static int __init armada_xp_pmsu_cpufreq_init(void) } }
- platform_device_register_simple("cpufreq-dt", -1, NULL, 0); + platform_device_register_data(NULL, "cpufreq-dt", -1, + &cpufreq_dt_pd, sizeof(cpufreq_dt_pd)); return 0; }
The cpufreq-dt driver tries to get a regulator for each CPU. This regulator is optional, but when not present, a scary message "failed to get cpuX regulator" is displayed. To solve this, we reduce the severity of the message from dev_warn() to dev_dbg() and we reword the message to not be as scary.
Signed-off-by: Thomas Petazzoni thomas.petazzoni@free-electrons.com --- drivers/cpufreq/cpufreq-dt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 987bbfa..b14628f 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -147,8 +147,8 @@ try_again: goto try_again; }
- dev_warn(cpu_dev, "failed to get cpu%d regulator: %ld\n", - cpu, PTR_ERR(cpu_reg)); + dev_dbg(cpu_dev, "no regulator for cpu%d: %ld\n", + cpu, PTR_ERR(cpu_reg)); }
cpu_clk = clk_get(cpu_dev, NULL);
On 10 September 2014 17:38, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
The cpufreq-dt driver tries to get a regulator for each CPU. This regulator is optional, but when not present, a scary message "failed to get cpuX regulator" is displayed. To solve this, we reduce the severity of the message from dev_warn() to dev_dbg() and we reword the message to not be as scary.
Signed-off-by: Thomas Petazzoni thomas.petazzoni@free-electrons.com
drivers/cpufreq/cpufreq-dt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 987bbfa..b14628f 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -147,8 +147,8 @@ try_again: goto try_again; }
dev_warn(cpu_dev, "failed to get cpu%d regulator: %ld\n",cpu, PTR_ERR(cpu_reg));
dev_dbg(cpu_dev, "no regulator for cpu%d: %ld\n",cpu, PTR_ERR(cpu_reg)); } cpu_clk = clk_get(cpu_dev, NULL);
Acked-by: Viresh Kumar viresh.kumar@linaro.org
On 10 September 2014 17:38, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
Hello,
Here is the v2 of the platform_data based proposal to make cpufreq-dt understand when it has a shared clock by all CPUs vs. independent clock for each CPU.
Changes since v1:
Use a 'void *driver_data' in cpufreq_driver instead of extending the existing flags field. Suggested by Viresh.
Keep the regulator related warning in cpufreq-dt, but reword it, and move it to the dev_dbg() level instead of dev_warn().
Thanks,
Thomas
Thomas Petazzoni (4): cpufreq: allow driver-specific data cpufreq: cpufreq-dt: extend with platform_data ARM: mvebu: use the cpufreq-dt platform_data for independent clocks cpufreq: cpufreq-dt: adjust message related to regulators
After Arnd's reply:
Acked-by: Viresh Kumar viresh.kumar@linaro.org
Hello,
On Wed, 10 Sep 2014 14:08:24 +0200, Thomas Petazzoni wrote:
Thomas Petazzoni (4): cpufreq: allow driver-specific data cpufreq: cpufreq-dt: extend with platform_data ARM: mvebu: use the cpufreq-dt platform_data for independent clocks cpufreq: cpufreq-dt: adjust message related to regulators
arch/arm/mach-mvebu/pmsu.c | 8 +++++++- drivers/cpufreq/cpufreq-dt.c | 21 +++++++++++++++++---- drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ include/linux/cpufreq.h | 2 ++ 5 files changed, 63 insertions(+), 5 deletions(-) create mode 100644 include/linux/cpufreq-dt.h
Any news on getting this merged for 3.18 ?
Thanks,
Thomas
Hello,
On Wed, 10 Sep 2014 14:08:24 +0200, Thomas Petazzoni wrote:
Thomas Petazzoni (4): cpufreq: allow driver-specific data cpufreq: cpufreq-dt: extend with platform_data ARM: mvebu: use the cpufreq-dt platform_data for independent clocks cpufreq: cpufreq-dt: adjust message related to regulators
arch/arm/mach-mvebu/pmsu.c | 8 +++++++- drivers/cpufreq/cpufreq-dt.c | 21 +++++++++++++++++---- drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ include/linux/cpufreq.h | 2 ++ 5 files changed, 63 insertions(+), 5 deletions(-) create mode 100644 include/linux/cpufreq-dt.h
Since this series was Acked-by Viresh, would it be possible to get it merged for 3.18 ?
Thanks,
Thomas
On Monday, October 06, 2014 09:19:05 AM Thomas Petazzoni wrote:
Hello,
On Wed, 10 Sep 2014 14:08:24 +0200, Thomas Petazzoni wrote:
Thomas Petazzoni (4): cpufreq: allow driver-specific data cpufreq: cpufreq-dt: extend with platform_data ARM: mvebu: use the cpufreq-dt platform_data for independent clocks cpufreq: cpufreq-dt: adjust message related to regulators
arch/arm/mach-mvebu/pmsu.c | 8 +++++++- drivers/cpufreq/cpufreq-dt.c | 21 +++++++++++++++++---- drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ include/linux/cpufreq.h | 2 ++ 5 files changed, 63 insertions(+), 5 deletions(-) create mode 100644 include/linux/cpufreq-dt.h
Since this series was Acked-by Viresh, would it be possible to get it merged for 3.18 ?
I'm still seeing discussion going on about patch [1/4], though.
Is that irrelevant?
Also I'd prefer Viresh to tell me that this is 3.18 material instead of sending ACKs in random replies and hoping that I'll notice them. I may not.
On 7 October 2014 04:23, Rafael J. Wysocki rjw@rjwysocki.net wrote:
I'm still seeing discussion going on about patch [1/4], though.
That's V1's 1/4 and we finalized V2..
Is that irrelevant?
No.
Also I'd prefer Viresh to tell me that this is 3.18 material instead of sending ACKs in random replies and hoping that I'll notice them. I may not.
Hmm, so yeah its for 3.18..
On Tuesday, October 07, 2014 09:00:16 AM Viresh Kumar wrote:
On 7 October 2014 04:23, Rafael J. Wysocki rjw@rjwysocki.net wrote:
I'm still seeing discussion going on about patch [1/4], though.
That's V1's 1/4 and we finalized V2..
Is that irrelevant?
No.
Also I'd prefer Viresh to tell me that this is 3.18 material instead of sending ACKs in random replies and hoping that I'll notice them. I may not.
Hmm, so yeah its for 3.18..
OK
So can you please send me links to the patches? I probably can find them, but honestly I have too little time for other stuff already.
On 8 October 2014 05:29, Rafael J. Wysocki rjw@rjwysocki.net wrote:
So can you please send me links to the patches? I probably can find them, but honestly I have too little time for other stuff already.
Sure:
https://patchwork.kernel.org/patch/4876641/ https://patchwork.kernel.org/patch/4876651/ https://patchwork.kernel.org/patch/4876661/ https://patchwork.kernel.org/patch/4876671/
On Wednesday, October 08, 2014 11:21:42 AM Viresh Kumar wrote:
On 8 October 2014 05:29, Rafael J. Wysocki rjw@rjwysocki.net wrote:
So can you please send me links to the patches? I probably can find them, but honestly I have too little time for other stuff already.
Sure:
https://patchwork.kernel.org/patch/4876641/ https://patchwork.kernel.org/patch/4876651/ https://patchwork.kernel.org/patch/4876661/ https://patchwork.kernel.org/patch/4876671/
They didn't apply cleanly for me and I didn't have the time to rebase them myself.
Thomas, care to rebase the v2 on top of the current Linus' tree and resend?
Dear Rafael J. Wysocki,
On Sun, 12 Oct 2014 22:25:02 +0200, Rafael J. Wysocki wrote:
Thomas, care to rebase the v2 on top of the current Linus' tree and resend?
I've just resent the patches. See:
https://patchwork.kernel.org/patch/5100691/ https://patchwork.kernel.org/patch/5100731/ https://patchwork.kernel.org/patch/5100711/
There are only 3 patches in this version, compared to four in the previous version, because one of the patch touches arch/arm/mach-mvebu/, and should therefore go through the mvebu maintainers.
Thanks,
Thomas
linaro-kernel@lists.linaro.org
-
Arnd Bergmann -
Mike Turquette -
Mike Turquette -
Rafael J. Wysocki -
Stephen Boyd -
Thomas Petazzoni -
Viresh Kumar