cpufreq: cpu0: Extend support beyond CPU0, V2
Subsystems (14)
Viresh Kumar
maintainercover cpufreq: cpu0: Extend support beyond CPU0, V2
V1: https://lkml.org/lkml/2014/6/25/152
Stephen Boyd sent few patches some time back around a new cpufreq driver for Qualcomm's Krait SoC: https://lkml.org/lkml/2014/6/24/918.
Krait couldn't use existing cpufreq-cpu0 driver as it doesn't have support for SoC's with multiple clusters or SoC's which don't share clock line across all CPUs.
This patchset is all about extending support beyond CPU0. It can be used for platforms like Krait or ARM big LITTLE architecture now.
First two patches add helper routine for of and clk layer, which would be used by later patches.
Third one adds space for driver specific data in 'struct cpufreq_policy' and later ones migrate cpufreq-cpu0 to cpufreq-generic, i.e. can be used for SoCs which don't share clock lines across all CPUs.
@Stephen: I haven't added your Tested-by as there were few modifications since the time you tested it last.
Pushed here:
Rebased over v3.16-rc3:git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/cpu0-krait-v2
For guys looking to test on exynos, rebased over linux-next + some patches from Thomas Abraham to use cpufreq-cpu0 for exynos:git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/cpu0-exynos-v2
Viresh Kumar (14):
of: Create of_property_match()
clk: Create of_clk_shared_by_cpus()
cpufreq: Add support for per-policy driver data
cpufreq: cpu0: Add Module Author
cpufreq: cpu0: don't validate clock on clk_put()
cpufreq: cpu0: defer probe if clock isn't registered yet
cpufreq: cpu0: OPPs can be populated at runtime
cpufreq: cpu0: use dev_{err|warn|dbg} instead of pr_{err|warn|debug}
cpufreq: cpu0: Move per-cluster initialization code to ->init()
cpufreq: cpu0: try regulators with name "cpu-supply"
cpufreq: cpu0: Make allocate_resources() work for any CPU
cpufreq: cpu0: Extend support beyond CPU0
cpufreq: cpu0: rename driver and internals to 'cpufreq_generic'
cpufreq: generic: set platform_{driver|device} '.name' to
'cpufreq-generic'
.../devicetree/bindings/cpufreq/cpufreq-cpu0.txt | 62 ---- .../bindings/cpufreq/cpufreq-generic.txt | 126 +++++++ arch/arm/mach-imx/imx27-dt.c | 2 +- arch/arm/mach-imx/imx51-dt.c | 2 +- arch/arm/mach-omap2/pm.c | 2 +- arch/arm/mach-shmobile/board-ape6evm-reference.c | 2 +- arch/arm/mach-shmobile/setup-sh73a0.c | 4 +- arch/arm/mach-zynq/common.c | 2 +- drivers/clk/clk.c | 56 +++ drivers/cpufreq/Kconfig | 10 +- drivers/cpufreq/Kconfig.arm | 2 +- drivers/cpufreq/Makefile | 2 +- drivers/cpufreq/cpufreq-cpu0.c | 251 ------------- drivers/cpufreq/cpufreq-generic.c | 394 +++++++++++++++++++++ drivers/cpufreq/exynos4x12-cpufreq.c | 2 +- drivers/cpufreq/highbank-cpufreq.c | 6 +- drivers/of/base.c | 29 ++ include/linux/clk.h | 6 + include/linux/cpufreq.h | 3 + include/linux/of.h | 10 + 20 files changed, 642 insertions(+), 331 deletions(-) delete mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-generic.txt delete mode 100644 drivers/cpufreq/cpufreq-cpu0.c create mode 100644 drivers/cpufreq/cpufreq-generic.c
Signature · 1 line
2.0.0.rc2
Viresh Kumar
maintainerPATCH 1/14 of: Create of_property_match()
Create a new routine of_property_match() that will match a property between two nodes. If they are exactly same, it returns 1 else 0. If the property isn't available in any of the nodes or the API isn't implemented, proper error codes would be returned.
The first user of this routine would be cpufreq-cpu0 driver, which requires to match "clock" property between CPU nodes to identify which CPUs share clock line. And hence this routine.
--- drivers/of/base.c | 29 +++++++++++++++++++++++++++++ include/linux/of.h | 10 ++++++++++ 2 files changed, 39 insertions(+)
drivers/of/base.c +29 -0
@@ -1490,6 +1490,35 @@ int of_property_count_strings(struct device_node *np, const char *propname) }EXPORT_SYMBOL_GPL (of_property_count_strings); +/** +* of_property_match - Match 'prop' property between two nodes +* @np1, np2: Nodes to match for property +* @prop_name: property to match +* +* Returns 1 on match, 0 on no match, and error for missing property. +*/ +int of_property_match (const struct device_node *np1, +const struct device_node *np2,const char *prop_name) +{ +const __be32 *prop1, *prop2; +int size1, size2; + +/* Retrieve property from both nodes */ + prop1 =of_get_property (np1, prop_name, &size1); +if (!prop1) +return -ENOENT ; + + prop2 =of_get_property (np2, prop_name, &size2); +if (!prop2) +return -ENOENT ; + +if (size1 != size2) +return 0 ; + +return !memcmp (prop1, prop2, size1); +} +EXPORT_SYMBOL_GPL (of_property_match); +void of_print_phandle_args (const char *msg,const struct of_phandle_args *args) {int i;
include/linux/of.h +10 -0
@@ -275,6 +275,9 @@ extern int of_property_match_string(struct device_node *np, const char *string);extern int of_property_count_strings (struct device_node *np,const char *propname); +extern int of_property_match (const struct device_node *np1, +const struct device_node *np2, +const char *prop_name);extern int of_device_is_compatible (const struct device_node *device,const char *);extern int of_device_is_available (const struct device_node *device);
@@ -498,6 +501,13 @@ static inline int of_property_count_strings(struct device_node *np,return -ENOSYS ; } +static inline int of_property_match (const struct device_node *np1, +const struct device_node *np2, +const char *prop_name) +{ +return -ENOSYS ; +} +static inline const void *of_get_property (const struct device_node *node,const char *name,int *lenp)
Signature · 1 line
2.0.0.rc2
Viresh Kumar
maintainerPATCH 2/14 clk: Create of_clk_shared_by_cpus()
In reply to Viresh KumarCreate a new routine of_clk_shared_by_cpus() that finds if clock lines are shared between two CPUs. This is verified by comparing "clocks" property from CPU's DT node.
Returns 1 if clock line is shared between them, 0 if clock isn't shared and return appropriate errors in case nodes/properties are missing.
--- drivers/clk/clk.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 6 ++++++ 2 files changed, 62 insertions(+)
drivers/clk/clk.c +56 -0
@@ -10,6 +10,7 @@*/ #include <linux/clk-private.h> +#include <linux/cpu.h> #include <linux/module.h> #include <linux/mutex.h> #include <linux/spinlock.h>
@@ -2528,6 +2529,61 @@ const char *of_clk_get_parent_name(struct device_node *np, int index) }EXPORT_SYMBOL_GPL (of_clk_get_parent_name); +/** +* of_clk_shared_by_cpus - Finds if clock line is shared between CPUs +* @cpu1, cpu2: CPU numbers +* +* Finds if clock lines are shared by two CPUs. This is verified by comparing +* "clocks" property from CPU's DT node. +* +* Returns 1 if clock line is shared between them, 0 if clock isn't shared. +* Return appropriate errors in case some requirements aren't met. +*/ +int of_clk_shared_by_cpus (int cpu1,int cpu2) +{ +struct device *cpu1_dev, *cpu2_dev; +struct device_node *np1, *np2; +int ret; + + cpu1_dev =get_cpu_device (cpu1); +if (!cpu1_dev) { +pr_err ("%s: failed to get cpu_dev for cpu%d\n" , __func__, cpu1); +return -ENODEV ; + } + + cpu2_dev =get_cpu_device (cpu2); +if (!cpu2_dev) { +pr_err ("%s: failed to get cpu_dev for cpu%d\n" , __func__, cpu2); +return -ENODEV ; + } + + np1 =of_node_get (cpu1_dev->of_node); +if (!np1) { +pr_err ("%s: failed to find of_node for cpu%d\n" , __func__, + cpu1); +return -ENODEV ; + } + + np2 =of_node_get (cpu2_dev->of_node); +if (!np2) { +pr_err ("%s: failed to find of_node for cpu%d\n" , __func__, + cpu2); + ret = -ENODEV ; + goto put_np1; + } + +/* Match "clocks" property */ + ret =of_property_match (np1, np2,"clocks" ); + +of_node_put (np2); + +put_np1: +of_node_put (np1); + +return ret; +} +EXPORT_SYMBOL_GPL (of_clk_shared_by_cpus); +struct clock_provider {of_clk_init_cb_t clk_init_cb;struct device_node *np;
include/linux/clk.h +6 -0
@@ -399,6 +399,7 @@ struct of_phandle_args;struct clk *of_clk_get (struct device_node *np,int index);struct clk *of_clk_get_by_name (struct device_node *np,const char *name);struct clk *of_clk_get_from_provider (struct of_phandle_args *clkspec); +int of_clk_shared_by_cpus (int cpu1,int cpu2);#else static inline struct clk *of_clk_get (struct device_node *np,int index) {
@@ -409,6 +410,11 @@ static inline struct clk *of_clk_get_by_name(struct device_node *np, {return ERR_PTR (-ENOENT ); } + +static inline int of_clk_shared_by_cpus (int cpu1,int cpu2) +{ +return -ENOSYS ; +}#endif #endif
Signature · 1 line
2.0.0.rc2
Re: [PATCH 02/14] clk: Create of_clk_shared_by_cpus()
Quoted 4 lines · Viresh Kumar
Create a new routine
of_clk_shared_by_cpus()that finds if clock lines are shared between two CPUs. This is verified by comparing "clocks" property from CPU's DT node.
Quoted 8 lines
Returns 1 if clock line is shared between them, 0 if clock isn't shared and return appropriate errors in case nodes/properties are missing.
Cc Mike Turquette <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/clk/clk.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 6 ++++++
This doesn't make much sense to me. This function doesn't deal with struct clk pointers or any of the internals of the common clock framework so why put it in clk.c? It looks more like an internal function that the cpufreq-generic driver should have.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Viresh Kumar
maintainerRe: [PATCH 02/14] clk: Create of_clk_shared_by_cpus()
Quoted 14 lines · Stephen Boyd
Quoted 4 lines · Viresh Kumar
Create a new routine
of_clk_shared_by_cpus()that finds if clock lines are shared between two CPUs. This is verified by comparing "clocks" property from CPU's DT node.Quoted 8 lines
Returns 1 if clock line is shared between them, 0 if clock isn't shared and return appropriate errors in case nodes/properties are missing.
Cc Mike Turquette <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/clk/clk.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 6 ++++++
Quoted 4 lines
This doesn't make much sense to me. This function doesn't deal with struct clk pointers or any of the internals of the common clock framework so why put it in
clk.c? It looks more like an internal function that the cpufreq-generic driver should have.
I thought this is what Rob suggested when he said:
"I think a clock api function would be better."
I had it in cpufreq-cpu0 driver earlier and moved it to a separate API yesterday only.
Sorry if I misunderstood his comment.
Re: [PATCH 02/14] clk: Create of_clk_shared_by_cpus()
In reply to Viresh Kumar+ Lorenzo
Quoted 31 lines · Viresh Kumar
Create a new routine
of_clk_shared_by_cpus()that finds if clock lines are shared between two CPUs. This is verified by comparing "clocks" property from CPU's DT node.Returns 1 if clock line is shared between them, 0 if clock isn't shared and return appropriate errors in case nodes/properties are missing.
Cc Mike Turquette <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/clk/clk.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 6 ++++++ 2 files changed, 62 insertions(+)drivers/clk/clk.c +2
-0@@ -10,6 +10,7 @@*/ #include <linux/clk-private.h> +#include <linux/cpu.h> #include <linux/module.h> #include <linux/mutex.h> #include <linux/spinlock.h> @@ -2528,6 +2529,61 @@ const char *of_clk_get_parent_name(struct device_node *np, int index) }EXPORT_SYMBOL_GPL (of_clk_get_parent_name); +/**
drivers/clk/clk.c const char *of_clk_get_parent_name(struct device_node *np, int index) · line 2542
Quoted 10 lines
drivers/clk/clk.c +10
-0+* of_clk_shared_by_cpus - Finds if clock line is shared between CPUs +* @cpu1, cpu2: CPU numbers +* +* Finds if clock lines are shared by two CPUs. This is verified by comparing +* "clocks" property from CPU's DT node. +* +* Returns 1 if clock line is shared between them, 0 if clock isn't shared. +* Return appropriate errors in case some requirements aren't met. +*/ +int of_clk_shared_by_cpus (int cpu1,int cpu2)
I might be wrong but this API seems to bit short cited. We should probably handle it bit better since there are more cases instead of just checking CPU tuple. More than two CPUs may share the clock so discovering that in one iteration is better. I also think this is closely related to CPU topology.
- CPUs part of 'a cluster' shares same clock.
- Multiple clusters may share same clock
- Every CPU might be clocked from separate PLL.
What you say ?
Regards,
Santosh
Viresh Kumar
maintainerRe: [PATCH 02/14] clk: Create of_clk_shared_by_cpus()
Quoted 8 lines · Santosh Shilimkar
I might be wrong but this API seems to bit short cited. We should probably handle it bit better since there are more cases instead of just checking CPU tuple. More than two CPUs may share the clock so discovering that in one iteration is better. I also think this is closely related to CPU topology.
- CPUs part of 'a cluster' shares same clock.
- Multiple clusters may share same clock
- Every CPU might be clocked from separate PLL.
All these configurations are currently supported by this series.
A message that is not in the archive (sent off-list or not yet received): 5f7164d789e87c62d722b575980c92dfd0504334.1404231535.git.viresh.kumar@linar
o.org
Re: [PATCH 02/14] clk: Create of_clk_shared_by_cpus()
Quoted 65 lines · Viresh Kumar
Create a new routine
of_clk_shared_by_cpus()that finds if clock lines are shared between two CPUs. This is verified by comparing "clocks" property from CPU's DT node.Returns 1 if clock line is shared between them, 0 if clock isn't shared and return appropriate errors in case nodes/properties are missing.
Cc Mike Turquette <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/clk/clk.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 6 ++++++ 2 files changed, 62 insertions(+)drivers/clk/clk.c +36
-0@@ -10,6 +10,7 @@*/ #include <linux/clk-private.h> +#include <linux/cpu.h> #include <linux/module.h> #include <linux/mutex.h> #include <linux/spinlock.h> @@ -2528,6 +2529,61 @@ const char *of_clk_get_parent_name(struct device_node *np, int index) }EXPORT_SYMBOL_GPL (of_clk_get_parent_name); +/** +* of_clk_shared_by_cpus - Finds if clock line is shared between CPUs +* @cpu1, cpu2: CPU numbers +* +* Finds if clock lines are shared by two CPUs. This is verified by comparing +* "clocks" property from CPU's DT node. +* +* Returns 1 if clock line is shared between them, 0 if clock isn't shared. +* Return appropriate errors in case some requirements aren't met. +*/ +int of_clk_shared_by_cpus (int cpu1,int cpu2) +{ +struct device *cpu1_dev, *cpu2_dev; +struct device_node *np1, *np2; +int ret; + + cpu1_dev =get_cpu_device (cpu1); +if (!cpu1_dev) { +pr_err ("%s: failed to get cpu_dev for cpu%d\n" , __func__, cpu1); +return -ENODEV ; + } + + cpu2_dev =get_cpu_device (cpu2); +if (!cpu2_dev) { +pr_err ("%s: failed to get cpu_dev for cpu%d\n" , __func__, cpu2); +return -ENODEV ; + } + + np1 =of_node_get (cpu1_dev->of_node); +if (!np1) { +pr_err ("%s: failed to find of_node for cpu%d\n" , __func__, + cpu1); +return -ENODEV ; + } +
drivers/clk/clk.c const char *of_clk_get_parent_name(struct device_node *np, int index) · line 2576
Quoted 10 lines
drivers/clk/clk.c +10
-0+ np2 =of_node_get (cpu2_dev->of_node); +if (!np2) { +pr_err ("%s: failed to find of_node for cpu%d\n" , __func__, + cpu2); + ret = -ENODEV ; + goto put_np1; + } + +/* Match "clocks" property */ + ret =of_property_match (np1, np2,"clocks" );
This looks naïve. It is entirely possible for different clock specifiers to resolve to the same clock line, or for there to be multple clocks in the clocks property. This looks like a buggy way to do it. The only reliable way to determine if two clocks resolve to the same thing is to resolve the references with the clock controller.
g.
Viresh Kumar
maintainerPATCH 3/14 cpufreq: Add support for per-policy driver data
In reply to Viresh KumarDrivers supporting multiple clusters or multiple 'struct cpufreq_policy' instances may need to keep per-policy data. If the core doesn't support them, they might do it in the most unoptimized way: 'per-cpu' data.
This patch adds another field in 'struct cpufreq_policy': 'driver_data'. It isn't accessed by core and is for driver's internal use.
--- include/linux/cpufreq.h | 3 +++ 1 file changed, 3 insertions(+)
include/linux/cpufreq.h +3 -0
@@ -112,6 +112,9 @@ struct cpufreq_policy {spinlock_t transition_lock;wait_queue_head_t transition_wait;struct task_struct *transition_task;/* Task which is doing the transition */ + +/* For cpufreq driver's internal use */ +void *driver_data; };/* Only for ACPI */
Signature · 1 line
2.0.0.rc2
Re: [PATCH 03/14] cpufreq: Add support for per-policy driver data
Quoted 17 lines · Viresh Kumar
Drivers supporting multiple clusters or multiple 'struct
cpufreq_policy' instances may need to keep per-policy data. If the core doesn't support them, they might do it in the most unoptimized way: 'per-cpu' data.This patch adds another field in 'struct
cpufreq_policy': 'driver_data'. It isn't accessed by core and is for driver's internal use.Signed-off-by Viresh Kumar <[email protected]>--- include/linux/cpufreq.h | 3 +++ 1 file changed, 3 insertions(+)include/linux/cpufreq.h +0
-0
include/linux/cpufreq.h struct cpufreq_policy { · line 119
Quoted 8 lines
include/linux/cpufreq.h +3
-0@@ -112,6 +112,9 @@ struct cpufreq_policy {spinlock_t transition_lock;wait_queue_head_t transition_wait;struct task_struct *transition_task;/* Task which is doing the transition */ + +/* For cpufreq driver's internal use */ +void *driver_data; };
Minor comment for consistency either maintain same commenting style for the above structure (description after the variable) or may be clean up the comments in another patch.
Regards,
Santosh
Viresh Kumar
maintainerRe: [PATCH 03/14] cpufreq: Add support for per-policy driver data
Quoted 8 lines · Santosh Shilimkar
Quoted 8 lines
include/linux/cpufreq.h +0
-0@@ -112,6 +112,9 @@ struct cpufreq_policy {spinlock_t transition_lock;wait_queue_head_t transition_wait;struct task_struct *transition_task;/* Task which is doing the transition */
Quoted 8 lines
include/linux/cpufreq.h struct cpufreq_policy { · line 119
Quoted 5 lines
include/linux/cpufreq.h +3
-0+ +/* For cpufreq driver's internal use */ +void *driver_data; };Minor comment for consistency either maintain same commenting style for the above structure (description after the variable) or may be clean up the comments in another patch.
Adding these to the end of variable makes it take multiple lines as we need to break after 80 columns. And both the styles are already mixed in existing file. So, would fix it separately in case I go for it :)
Viresh Kumar
maintainerPATCH 4/14 cpufreq: cpu0: Add Module Author
In reply to Viresh KumarTwo people are maintaining it now, Viresh and Shawn. Add Viresh's details in MODULE_AUTHOR() and copyright section.
--- drivers/cpufreq/cpufreq-cpu0.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/cpufreq/cpufreq-cpu0.c +4 -0
@@ -1,6 +1,9 @@/* * Copyright (C) 2012 Freescale Semiconductor, Inc. * +* Copyright (C) 2014 Linaro. +* Viresh Kumar <[email protected]> +* * The OPP code in function cpu0_set_target() is reused from * drivers/cpufreq/omap-cpufreq.c *
@@ -246,6 +249,7 @@ static struct platform_driver cpu0_cpufreq_platdrv = { };module_platform_driver (cpu0_cpufreq_platdrv); +MODULE_AUTHOR ("Viresh Kumar <[email protected]>" );MODULE_AUTHOR ("Shawn Guo <[email protected]>" );MODULE_DESCRIPTION ("Generic CPU0 cpufreq driver" );MODULE_LICENSE ("GPL" );
Signature · 1 line
2.0.0.rc2
Re: [PATCH 04/14] cpufreq: cpu0: Add Module Author
drivers/cpufreq/cpufreq-cpu0.c
Quoted 13 lines · Viresh Kumar
Two people are maintaining it now, Viresh and Shawn. Add Viresh's details in
MODULE_AUTHOR()and copyright section.Suggested-by Shawn Guo <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq-cpu0.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/cpufreq/cpufreq-cpu0.c +0
-0
Not related to this patch but I think its time to change the name of this driver. I never liked this 'cpufreq-cpu0.c' and already mentioned that during the reviews of this driver.
Not sure if there are still sentiments about it but considering the additional functionality coming in, the name will definitely miss-leading.
Regards,
Santosh
Viresh Kumar
maintainerRe: [PATCH 04/14] cpufreq: cpu0: Add Module Author
Quoted 8 lines · Santosh Shilimkar
Not related to this patch but I think its time to change the name of this driver. I never liked this '
cpufreq-cpu0.c' and already mentioned that during the reviews of this driver.Not sure if there are still sentiments about it but considering the additional functionality coming in, the name will definitely miss-leading.
Check last patch, its called cpufreq-generic now.
Re: [PATCH 04/14] cpufreq: cpu0: Add Module Author
Quoted 6 lines · Viresh Kumar
Quoted 5 lines · Santosh Shilimkar
Not related to this patch but I think its time to change the name of this driver. I never liked this '
cpufreq-cpu0.c' and already mentioned that during the reviews of this driver.
Quoted 6 lines
Quoted 3 lines
Not sure if there are still sentiments about it but considering the additional functionality coming in, the name will definitely miss-leading.
Check last patch, its called cpufreq-generic now.
Sounds good :)
Viresh Kumar
maintainerPATCH 5/14 cpufreq: cpu0: don't validate clock on clk_put()
In reply to Viresh KumarCPU clk is not optional for this driver and probe would fail if it couldn't find a suitable clock.
And so, while calling clk_put() we don't need to validate clocks.
--- drivers/cpufreq/cpufreq-cpu0.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/cpufreq/cpufreq-cpu0.c +1 -2
@@ -220,8 +220,7 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) out_free_table:dev_pm_opp_free_cpufreq_table (cpu_dev, &freq_table); out_put_clk: -if (!IS_ERR (cpu_clk)) -clk_put (cpu_clk); +clk_put (cpu_clk); out_put_reg:if (!IS_ERR (cpu_reg))regulator_put (cpu_reg);
Signature · 1 line
2.0.0.rc2
Re: [PATCH 05/14] cpufreq: cpu0: don't validate clock on clk_put()
Quoted 7 lines · Viresh Kumar
CPU clk is not optional for this driver and probe would fail if it couldn't find a suitable clock.
And so, while calling
clk_put()we don't need to validate clocks.Signed-off-by Viresh Kumar <[email protected]>---
Viresh Kumar
maintainerPATCH 6/14 cpufreq: cpu0: defer probe if clock isn't registered yet
In reply to Viresh KumarCurrently, we defer probe if regulator_get() returned -EPROBE_DEFER, i.e. regulator isn't registered yet.
The same is true for clock as well, so lets defer in that case as well.
--- drivers/cpufreq/cpufreq-cpu0.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/cpufreq/cpufreq-cpu0.c +11 -2
@@ -150,8 +150,17 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) cpu_clk =clk_get (cpu_dev,NULL );if (IS_ERR (cpu_clk)) { - ret =PTR_ERR (cpu_clk); -pr_err ("failed to get cpu0 clock: %d\n" , ret); +/* +* If cpu's clk node is present, but clock is not yet +* registered, we should try defering probe. +*/ +if (PTR_ERR (cpu_clk) == -EPROBE_DEFER ) { +dev_err (cpu_dev,"cpu0 clock not ready, retry\n" ); + ret = -EPROBE_DEFER ; + }else { + ret =PTR_ERR (cpu_clk); +dev_err (cpu_dev,"failed to get cpu0 clock: %d\n" , ret); + } goto out_put_reg; }
Signature · 1 line
2.0.0.rc2
Re: [PATCH 06/14] cpufreq: cpu0: defer probe if clock isn't registered yet
Quoted 15 lines · Viresh Kumar
Currently, we defer probe if
regulator_get()returned -EPROBE_DEFER, i.e. regulator isn't registered yet.The same is true for clock as well, so lets defer in that case as well.
Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq-cpu0.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)drivers/cpufreq/cpufreq-cpu0.c +0
-0
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 153
Quoted 4 lines
drivers/cpufreq/cpufreq-cpu0.c +0
-1@@ -150,8 +150,17 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) cpu_clk =clk_get (cpu_dev,NULL );if (IS_ERR (cpu_clk)) { - ret =PTR_ERR (cpu_clk);
If you keep this ...
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 157
Quoted 6 lines
drivers/cpufreq/cpufreq-cpu0.c +5
-1-pr_err ("failed to get cpu0 clock: %d\n" , ret); +/* +* If cpu's clk node is present, but clock is not yet +* registered, we should try defering probe. +*/ +if (PTR_ERR (cpu_clk) == -EPROBE_DEFER ) {
... you can use 'ret' here ...
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 159
Quoted 2 lines
drivers/cpufreq/cpufreq-cpu0.c +2
-0+dev_err (cpu_dev,"cpu0 clock not ready, retry\n" ); + ret = -EPROBE_DEFER ;
... this can be saved ...
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 161
Quoted 2 lines
drivers/cpufreq/cpufreq-cpu0.c +2
-0+ }else { + ret =PTR_ERR (cpu_clk);
... and this as well.
Shawn
Quoted 8 lines
drivers/cpufreq/cpufreq-cpu0.c +2
-0+dev_err (cpu_dev,"failed to get cpu0 clock: %d\n" , ret); + } goto out_put_reg; }Signature · 1 line
2.0.0.rc2
Viresh Kumar
maintainerRe: [PATCH 06/14] cpufreq: cpu0: defer probe if clock isn't registered yet
Quoted 26 lines · Shawn Guo
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 153
Quoted 9 lines · Viresh Kumar
drivers/cpufreq/cpufreq-cpu0.c +0
-1@@ -150,8 +150,17 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) cpu_clk =clk_get (cpu_dev,NULL );if (IS_ERR (cpu_clk)) { - ret =PTR_ERR (cpu_clk);If you keep this ...
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 157
Quoted 6 lines
drivers/cpufreq/cpufreq-cpu0.c +5
-1-pr_err ("failed to get cpu0 clock: %d\n" , ret); +/* +* If cpu's clk node is present, but clock is not yet +* registered, we should try defering probe. +*/ +if (PTR_ERR (cpu_clk) == -EPROBE_DEFER ) {... you can use 'ret' here ...
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 159
Quoted 2 lines
drivers/cpufreq/cpufreq-cpu0.c +2
-0+dev_err (cpu_dev,"cpu0 clock not ready, retry\n" ); + ret = -EPROBE_DEFER ;
Quoted 6 lines
... this can be saved ...
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 161
Quoted 2 lines
drivers/cpufreq/cpufreq-cpu0.c +2
-0+ }else { + ret =PTR_ERR (cpu_clk);... and this as well.
All accepted. Thanks.
Viresh Kumar
maintainerRe: [PATCH 06/14] cpufreq: cpu0: defer probe if clock isn't registered yet
Quoted 27 lines · Viresh Kumar
Quoted 26 lines · Shawn Guo
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 153
Quoted 9 lines · Viresh Kumar
drivers/cpufreq/cpufreq-cpu0.c +0
-1@@ -150,8 +150,17 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) cpu_clk =clk_get (cpu_dev,NULL );if (IS_ERR (cpu_clk)) { - ret =PTR_ERR (cpu_clk);If you keep this ...
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 157
Quoted 6 lines
drivers/cpufreq/cpufreq-cpu0.c +5
-1-pr_err ("failed to get cpu0 clock: %d\n" , ret); +/* +* If cpu's clk node is present, but clock is not yet +* registered, we should try defering probe. +*/ +if (PTR_ERR (cpu_clk) == -EPROBE_DEFER ) {... you can use 'ret' here ...
Quoted 2 lines
drivers/cpufreq/cpufreq-cpu0.c +2
-0+dev_err (cpu_dev,"cpu0 clock not ready, retry\n" ); + ret = -EPROBE_DEFER ;
Quoted 8 lines
Quoted 6 lines
... this can be saved ...
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 161
Quoted 2 lines
drivers/cpufreq/cpufreq-cpu0.c +2
-0+ }else { + ret =PTR_ERR (cpu_clk);... and this as well.
All accepted. Thanks.
The motive of this patch is changed completely after what you suggested and so logs are updated as well:
cpufreq: cpu0: print relevant error when we defer probe
Currently, we defer probe if regulator_get() returned -EPROBE_DEFER, i.e. regulator isn't registered yet.
The same is true for clock as well and should be done there.
Current code already does it, but it wasn't intentional probably.
Its just that
we are returning the right error with wrong print message.
Fix print message to convey right error.
--- drivers/cpufreq/cpufreq-cpu0.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
drivers/cpufreq/cpufreq-cpu0.c +10 -1
@@ -151,7 +151,16 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) cpu_clk =clk_get (cpu_dev,NULL );if (IS_ERR (cpu_clk)) { ret =PTR_ERR (cpu_clk); -pr_err ("failed to get cpu0 clock: %d\n" , ret); + +/* +* If cpu's clk node is present, but clock is not yet +* registered, we should try defering probe. +*/ +if (ret == -EPROBE_DEFER ) +dev_err (cpu_dev,"cpu0 clock not ready, retry\n" ); +else +dev_err (cpu_dev,"failed to get cpu0 clock: %d\n" , ret); + goto out_put_reg; }
Re: [PATCH 06/14] cpufreq: cpu0: defer probe if clock isn't registered yet
Quoted 7 lines · Viresh Kumar
drivers/cpufreq/cpufreq-cpu0.c +0
-0@@ -151,7 +151,16 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) cpu_clk =clk_get (cpu_dev,NULL );
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 160
Quoted 10 lines
drivers/cpufreq/cpufreq-cpu0.c +7
-1if (IS_ERR (cpu_clk)) { ret =PTR_ERR (cpu_clk); -pr_err ("failed to get cpu0 clock: %d\n" , ret); + +/* +* If cpu's clk node is present, but clock is not yet +* registered, we should try defering probe. +*/ +if (ret == -EPROBE_DEFER ) +dev_err (cpu_dev,"cpu0 clock not ready, retry\n" );
Please make this a dev_dbg() or just remove it entirely. Sending a message to the log on probe defer just duplicates what the driver core is already doing.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Viresh Kumar
maintainerRe: [PATCH 06/14] cpufreq: cpu0: defer probe if clock isn't registered yet
Quoted 3 lines · Stephen Boyd
Please make this a
dev_dbg()or just remove it entirely. Sending a message to the log on probe defer just duplicates what the driver core is already doing.
Updated as:
Author: Viresh Kumar <[email protected]> Date: Thu Jun 26 10:40:21 2014 +0530
cpufreq: cpu0: print relevant error when we defer probe
Currently, we defer probe if regulator_get() returned -EPROBE_DEFER, i.e. regulator isn't registered yet. We do a dev_err() in this case. Sending a message to the log on probe defer just duplicates what the driver core is already doing. Convert it to dev_dbg() instead.
We should defer in case of clk_get() as well.
Current code already does it, but it wasn't intentional probably.
Its just that
we are returning the right error with wrong print message.
Fix print message to convey right error.
--- drivers/cpufreq/cpufreq-cpu0.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/cpufreq/cpufreq-cpu0.c +11 -2
@@ -140,7 +140,7 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)* not yet registered, we should try defering probe. */ if (PTR_ERR (cpu_reg) == -EPROBE_DEFER ) { -dev_err (cpu_dev,"cpu0 regulator not ready, retry\n" ); +dev_dbg (cpu_dev,"cpu0 regulator not ready, retry\n" ); ret = -EPROBE_DEFER ; goto out_put_node; }
@@ -151,7 +151,16 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) cpu_clk =clk_get (cpu_dev,NULL );if (IS_ERR (cpu_clk)) { ret =PTR_ERR (cpu_clk); -pr_err ("failed to get cpu0 clock: %d\n" , ret); + +/* +* If cpu's clk node is present, but clock is not yet +* registered, we should try defering probe. +*/ +if (ret == -EPROBE_DEFER ) +dev_dbg (cpu_dev,"cpu0 clock not ready, retry\n" ); +else +dev_err (cpu_dev,"failed to get cpu0 clock: %d\n" , ret); + goto out_put_reg; }
Santosh Shilimkar
Acked-byRe: [PATCH 06/14] cpufreq: cpu0: defer probe if clock isn't registered yet
Quoted 23 lines · Viresh Kumar
Quoted 3 lines · Stephen Boyd
Please make this a
dev_dbg()or just remove it entirely. Sending a message to the log on probe defer just duplicates what the driver core is already doing.Updated as:
Author: Viresh Kumar <[email protected]> Date: Thu Jun 26 10:40:21 2014 +0530cpufreq: cpu0: print relevant error when we defer probe
Currently, we defer probe if
regulator_get()returned -EPROBE_DEFER, i.e. regulator isn't registered yet. We do adev_err()in this case. Sending a message to the log on probe defer just duplicates what the driver core is already doing. Convert it todev_dbg()instead.We should defer in case of
clk_get()as well.Current code already does it, but it wasn't intentional probably.
Its just that
we are returning the right error with wrong print message.
Quoted 7 lines
Fix print message to convey right error.
Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq-cpu0.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
Looks good to me.
Viresh Kumar
maintainerPATCH 7/14 cpufreq: cpu0: OPPs can be populated at runtime
In reply to Viresh KumarOPPs can be populated statically, via DT, or added at run time with dev_pm_opp_add().
While this driver handles the first case correctly, it would fail to populate OPPs added at runtime. Because call to of_init_opp_table() would fail as there are no OPPs in DT and probe will return early.
To fix this, remove error checking and call dev_pm_opp_init_cpufreq_table() unconditionally.
--- drivers/cpufreq/cpufreq-cpu0.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/cpufreq/cpufreq-cpu0.c +2 -5
@@ -164,11 +164,8 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) goto out_put_reg; } - ret =of_init_opp_table (cpu_dev); -if (ret) { -pr_err ("failed to init OPP table: %d\n" , ret); - goto out_put_clk; - } +/* OPPs might be populated at runtime, don't check for error here */ +of_init_opp_table (cpu_dev); ret =dev_pm_opp_init_cpufreq_table (cpu_dev, &freq_table);if (ret) {
Signature · 1 line
2.0.0.rc2
Re: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime
Quoted 7 lines · Viresh Kumar
OPPs can be populated statically, via DT, or added at run time with
dev_pm_opp_add().While this driver handles the first case correctly, it would fail to populate OPPs added at runtime. Because call to
of_init_opp_table()would fail as there are no OPPs in DT and probe will return early.
Quoted 8 lines
To fix this, remove error checking and call
dev_pm_opp_init_cpufreq_table()unconditionally.Suggested-by Stephen Boyd <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq-cpu0.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
Please update the binding as well to indicate that this property is now optional.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Viresh Kumar
maintainerRe: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime
Quoted 2 lines · Stephen Boyd
Please update the binding as well to indicate that this property is now optional.
Does this look fine..
(quoted diff) +0 -0
b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt index f055515..366690c 100644
Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt +0 -0
@@ -8,10 +8,12 @@ Both required and optional properties listed below
must be defined
under node /cpus/cpu@0.
Required properties:
-- operating-points: Refer to Documentation/devicetree/bindings/power/opp.txt
- for details
+- None
Optional properties:
+- operating-points: Refer to Documentation/devicetree/bindings/power/opp.txt for
+ details. OPPs *must* be supplied either via DT, i.e. this property, or
+ populated at runtime.
- clock-latency: Specify the possible maximum transition latency for clock, in unit of nanoseconds.
- voltage-tolerance: Specify the CPU voltage tolerance in percentage.
Viresh Kumar
maintainerPATCH RESEND v2 7/14 cpufreq: cpu0: OPPs can be populated at runtime
In reply to Viresh KumarOPPs can be populated statically, via DT, or added at run time with dev_pm_opp_add().
While this driver handles the first case correctly, it would fail to populate OPPs added at runtime. Because call to of_init_opp_table() would fail as there are no OPPs in DT and probe will return early.
To fix this, remove error checking and call dev_pm_opp_init_cpufreq_table() unconditionally.
Update bindings as well.
---
V2 Resend: Update bindings as well
Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt | 6 ++++-- drivers/cpufreq/cpufreq-cpu0.c | 7 ++----- 2 files changed, 6 insertions(+), 7 deletions(-)
Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt +4 -2
@@ -8,10 +8,12 @@ Both required and optional properties listed below must be defined under node /cpus/cpu@0. Required properties: -- operating-points: Refer to Documentation/devicetree/bindings/power/opp.txt - for details +- None Optional properties: +- operating-points: Refer to Documentation/devicetree/bindings/power/opp.txt for + details. OPPs *must* be supplied either via DT, i.e. this property, or + populated at runtime. - clock-latency: Specify the possible maximum transition latency for clock, in unit of nanoseconds. - voltage-tolerance: Specify the CPU voltage tolerance in percentage.
drivers/cpufreq/cpufreq-cpu0.c +2 -5
@@ -164,11 +164,8 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) goto out_put_reg; } - ret =of_init_opp_table (cpu_dev); -if (ret) { -pr_err ("failed to init OPP table: %d\n" , ret); - goto out_put_clk; - } +/* OPPs might be populated at runtime, don't check for error here */ +of_init_opp_table (cpu_dev); ret =dev_pm_opp_init_cpufreq_table (cpu_dev, &freq_table);if (ret) {
Signature · 1 line
2.0.0.rc2
Viresh Kumar
maintainerPATCH RESEND v2 12/14 cpufreq: cpu0: Extend support beyond CPU0
Most of the infrastructure is in place now, with only little left. How to find siblings ?
Stephen Boyd suggested to compare "clocks" properties from CPU's DT node and siblings should match. This patch adds another routine find_siblings() which calls of_property_match() to find if CPUs share clock line or not.
If of_property_match() returns error, we fallback to all CPUs sharing clock line assumption as existing platforms don't have "clocks" property in all CPU nodes and would fail from of_property_match().
---
V2 Resend: Use of_property_match() directly instead of of_clk_shared_by_cpus() which would be dropped now.
.../devicetree/bindings/cpufreq/cpufreq-cpu0.txt | 72 ++++++++++++++++++++-- drivers/cpufreq/cpufreq-cpu0.c | 62 ++++++++++++++++++- 2 files changed, 128 insertions(+), 6 deletions(-)
Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt +68 -4
@@ -1,11 +1,11 @@ -Generic CPU0 cpufreq driver +Generic cpufreq driver -It is a generic cpufreq driver for CPU0 frequency management. It +It is a generic cpufreq driver for frequency management. It supports both uniprocessor (UP) and symmetric multiprocessor (SMP) -systems which share clock and voltage across all CPUs. +systems which may or maynot share clock and voltage across all CPUs. Both required and optional properties listed below must be defined -under node /cpus/cpu@0. +under node /cpus/cpu@x. Where x is the first cpu inside a cluster. Required properties: - None
@@ -21,9 +21,16 @@ Optional properties: - cooling-min-level: - cooling-max-level: Please refer to Documentation/devicetree/bindings/thermal/thermal.txt. +- clocks: If CPU clock is populated from DT, "clocks" property must be copied to + every cpu node sharing clock with cpu@x. Generic cpufreq driver compares + "clocks" to find siblings, i.e. to see which CPUs share clock/voltages. If + only cpu@0 contains "clocks" property it is assumed that all CPUs share clock + line. Examples: +1. All CPUs share clock/voltages + cpus { #address-cells = <1>; #size-cells = <0>;
@@ -38,6 +45,8 @@ cpus { 396000 950000 198000 850000 >; + clocks = <&clock CLK_ARM_CLK>; + clock-names = "cpu"; clock-latency = <61036>; /* two CLK32 periods */ #cooling-cells = <2>; cooling-min-level = <0>;
@@ -48,17 +57,72 @@ cpus { compatible = "arm,cortex-a9"; reg = <1>; next-level-cache = <&L2>; + clocks = <&clock CLK_ARM_CLK>; }; cpu@2 { compatible = "arm,cortex-a9"; reg = <2>; next-level-cache = <&L2>; + clocks = <&clock CLK_ARM_CLK>; }; cpu@3 { compatible = "arm,cortex-a9"; reg = <3>; next-level-cache = <&L2>; + clocks = <&clock CLK_ARM_CLK>; + }; +}; + + +2. All CPUs inside a cluster share clock/voltages, there are multiple clusters. + +cpus { + #address-cells = <1>; + #size-cells = <0>; + + cpu@0 { + compatible = "arm,cortex-a15"; + reg = <0>; + next-level-cache = <&L2>; + operating-points = < + /* kHz uV */ + 792000 1100000 + 396000 950000 + 198000 850000 + >; + clocks = <&clock CLK_ARM1_CLK>; + clock-names = "cpu"; + clock-latency = <61036>; /* two CLK32 periods */ + }; + + cpu@1 { + compatible = "arm,cortex-a15"; + reg = <1>; + next-level-cache = <&L2>; + clocks = <&clock CLK_ARM1_CLK>; + }; + + cpu@100 { + compatible = "arm,cortex-a7"; + reg = <100>; + next-level-cache = <&L2>; + operating-points = < + /* kHz uV */ + 792000 950000 + 396000 750000 + 198000 450000 + >; + clocks = <&clock CLK_ARM2_CLK>; + clock-names = "cpu"; + clock-latency = <61036>; /* two CLK32 periods */ + }; + + cpu@101 { + compatible = "arm,cortex-a7"; + reg = <101>; + next-level-cache = <&L2>; + clocks = <&clock CLK_ARM2_CLK>; }; };
drivers/cpufreq/cpufreq-cpu0.c +60 -2
75 lines not on this page · raw message
Signature · 1 line
2.0.0.rc2
Re: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime
In reply to Viresh KumarQuoted 7 lines · Viresh Kumar
OPPs can be populated statically, via DT, or added at run time with
dev_pm_opp_add().While this driver handles the first case correctly, it would fail to populate OPPs added at runtime. Because call to
of_init_opp_table()would fail as there are no OPPs in DT and probe will return early.
Quoted 6 lines
To fix this, remove error checking and call
dev_pm_opp_init_cpufreq_table()unconditionally.Suggested-by Stephen Boyd <[email protected]>Signed-off-by Viresh Kumar <[email protected]>---
Assuming you are updating bidnings as suggested by Stephen, patch looks good to me.
Viresh Kumar
maintainerRe: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime
Quoted 2 lines · Santosh Shilimkar
Assuming you are updating bidnings as suggested by Stephen, patch looks good to me.
I have already sent a patch in reply to this one earlier with updated bindings.
Quoted 1 line
Thanks.
Viresh Kumar
maintainerRe: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime
In reply to Santosh ShilimkarQuoted 3 lines · Santosh Shilimkar
Assuming you are updating bidnings as suggested by Stephen, patch looks good to me.
Why do you still have a separate cpufreq driver for omap? Would this patch help getting that out?
I see this for omap:
static inline void omap_init_cpufreq(void)
{
struct platform_device_info devinfo = { };
if (!of_have_populated_dt())
devinfo.name = "omap-cpufreq";
else
devinfo.name = "cpufreq-generic";
platform_device_register_full(&devinfo);
}and it makes me believe that you were just waiting for this patch?
Re: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime
Quoted 21 lines · Viresh Kumar
Quoted 3 lines · Santosh Shilimkar
Assuming you are updating bidnings as suggested by Stephen, patch looks good to me.
Why do you still have a separate cpufreq driver for omap? Would this patch help getting that out?
I see this for omap:
static inline void
omap_init_cpufreq(void){ struct platform_device_info devinfo = { }; if (!of_have_populated_dt()) devinfo.name = "omap-cpufreq"; else devinfo.name = "cpufreq-generic"; platform_device_register_full(&devinfo); }
Quoted 1 line
and it makes me believe that you were just waiting for this patch?
Sorry, am away on vacation and slow on emails. The plan was to kill omap cpufreq once all platforms convert to device tree only boot. Only platform left is OMAP3 based platforms - though the date for removing non-dt support has changed a couple of kernel revisions - but we should be able to remove that entire file with this change.
We will need this support to go with the solution recommended for opp modifier series[1] - where platform code will populate or add OPPs based on "speed grade" sample detection.
[1]http://comments.gmane.org/gmane.linux.ports.arm.kernel/309466
---
Regards,
Nishanth Menon
Re: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime
Quoted 30 lines · Nishanth Menon
Quoted 21 lines · Viresh Kumar
Quoted 3 lines · Santosh Shilimkar
Assuming you are updating bidnings as suggested by Stephen, patch looks good to me.
Why do you still have a separate cpufreq driver for omap? Would this patch help getting that out?
I see this for omap:
static inline void
omap_init_cpufreq(void){ struct platform_device_info devinfo = { }; if (!of_have_populated_dt()) devinfo.name = "omap-cpufreq"; else devinfo.name = "cpufreq-generic"; platform_device_register_full(&devinfo); }Quoted 1 line
and it makes me believe that you were just waiting for this patch?
Sorry, am away on vacation and slow on emails. The plan was to kill omap cpufreq once all platforms convert to device tree only boot. Only platform left is OMAP3 based platforms - though the date for removing non-dt support has changed a couple of kernel revisions - but we should be able to remove that entire file with this change.
Quoted 4 lines
We will need this support to go with the solution recommended for opp modifier series[1] - where platform code will populate or add OPPs based on "speed grade" sample detection.
Yep. Last time I blocked the series because all the DT conversions were not done. Considering now the cpufreq-generic can work on non DT platforms, I am ok to remove the omap-cpufreq.
Regards,
Santosh
Viresh Kumar
maintainerRe: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime
Quoted 3 lines · Santosh Shilimkar
Yep. Last time I blocked the series because all the DT conversions were not done. Considering now the cpufreq-generic can work on non DT platforms, I am ok to remove the omap-cpufreq.
Great.
Viresh Kumar
maintainerPATCH 8/14 cpufreq: cpu0: use dev_{err|warn|dbg} instead of pr_{err|warn|debug}
In reply to Viresh KumarWe already have cpu_dev and is used at multiple places for printing errors using dev_*(). But some prints are still using pr_*(). Lets make it consistent and replace those pr_*() macros with dev_*() macros.
--- drivers/cpufreq/cpufreq-cpu0.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
drivers/cpufreq/cpufreq-cpu0.c +18 -14
@@ -58,7 +58,8 @@ static int cpu0_set_target(struct cpufreq_policy *policy, unsigned int index) opp =dev_pm_opp_find_freq_ceil (cpu_dev, &freq_Hz);if (IS_ERR (opp)) {rcu_read_unlock (); -pr_err ("failed to find OPP for %ld\n" , freq_Hz); +dev_err (cpu_dev,"failed to find OPP for %ld\n" , + freq_Hz);return PTR_ERR (opp); } volt =dev_pm_opp_get_voltage (opp);
@@ -67,22 +68,23 @@ static int cpu0_set_target(struct cpufreq_policy *policy, unsigned int index) volt_old =regulator_get_voltage (cpu_reg); } -pr_debug ("%u MHz, %ld mV --> %u MHz, %ld mV\n" , - old_freq /1000 , volt_old ? volt_old /1000 :-1 , - new_freq /1000 , volt ? volt /1000 :-1 ); +dev_dbg (cpu_dev,"%u MHz, %ld mV --> %u MHz, %ld mV\n" , + old_freq /1000 , volt_old ? volt_old /1000 :-1 , + new_freq /1000 , volt ? volt /1000 :-1 );/* scaling up? scale voltage before frequency */ if (!IS_ERR (cpu_reg) && new_freq > old_freq) { ret =regulator_set_voltage_tol (cpu_reg, volt, tol);if (ret) { -pr_err ("failed to scale voltage up: %d\n" , ret); +dev_err (cpu_dev,"failed to scale voltage up: %d\n" , + ret);return ret; } } ret =clk_set_rate (cpu_clk, freq_exact);if (ret) { -pr_err ("failed to set clock rate: %d\n" , ret); +dev_err (cpu_dev,"failed to set clock rate: %d\n" , ret);if (!IS_ERR (cpu_reg))regulator_set_voltage_tol (cpu_reg, volt_old, tol);return ret;
@@ -92,7 +94,8 @@ static int cpu0_set_target(struct cpufreq_policy *policy, unsigned int index)if (!IS_ERR (cpu_reg) && new_freq < old_freq) { ret =regulator_set_voltage_tol (cpu_reg, volt, tol);if (ret) { -pr_err ("failed to scale voltage down: %d\n" , ret); +dev_err (cpu_dev,"failed to scale voltage down: %d\n" , + ret);clk_set_rate (cpu_clk, old_freq *1000 ); } }
@@ -129,7 +132,7 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) np =of_node_get (cpu_dev->of_node);if (!np) { -pr_err ("failed to find cpu0 node\n" ); +dev_err (cpu_dev,"failed to find cpu0 node\n" );return -ENOENT ; }
@@ -144,8 +147,8 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) ret = -EPROBE_DEFER ; goto out_put_node; } -pr_warn ("failed to get cpu0 regulator: %ld\n" , -PTR_ERR (cpu_reg)); +dev_warn (cpu_dev,"failed to get cpu0 regulator: %ld\n" , +PTR_ERR (cpu_reg)); } cpu_clk =clk_get (cpu_dev,NULL );
@@ -169,7 +172,7 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) ret =dev_pm_opp_init_cpufreq_table (cpu_dev, &freq_table);if (ret) { -pr_err ("failed to init cpufreq table: %d\n" , ret); +dev_err (cpu_dev,"failed to init cpufreq table: %d\n" , ret); goto out_put_clk; }
@@ -205,7 +208,7 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) ret =cpufreq_register_driver (&cpu0_cpufreq_driver);if (ret) { -pr_err ("failed register driver: %d\n" , ret); +dev_err (cpu_dev,"failed to register driver: %d\n" , ret); goto out_free_table; }
@@ -216,8 +219,9 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)if (of_find_property (np,"#cooling-cells" ,NULL )) { cdev =of_cpufreq_cooling_register (np, cpu_present_mask);if (IS_ERR (cdev)) -pr_err ("running cpufreq without cooling device: %ld\n" , -PTR_ERR (cdev)); +dev_err (cpu_dev, +"running cpufreq without cooling device: %ld\n" , +PTR_ERR (cdev)); }of_node_put (np);
Signature · 1 line
2.0.0.rc2
Re: [PATCH 08/14] cpufreq: cpu0: use dev_{err|warn|dbg} instead of pr_{err|warn|debug}
Quoted 9 lines · Viresh Kumar
We already have
cpu_devand is used at multiple places for printing errors using dev_*(). But some prints are still using pr_*(). Lets make it consistent and replace those pr_*() macros with dev_*() macros.Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq-cpu0.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
Looks good to me.
Viresh Kumar
maintainerPATCH 9/14 cpufreq: cpu0: Move per-cluster initialization code to ->init()
In reply to Viresh KumarCurrently this driver only support platforms on which all CPUs share clock & voltage lines and there is requirement to support platforms which have separate clock & voltage lines for CPUs, like Qualcomm's Krait and ARM's big LITTLE.
Each group of CPUs sharing clock/voltage lines are represented by 'struct cpufreq_policy' in cpufreq framework. And core calls ->init() once for each policy.
Currently we do all initialization/allocation from probe() which wouldn't work for above scenario. To make it work for these platforms, the first step is to move all initialization/allocation to ->init() and add ->exit() to do the reverse of it.
Also, remove all global variables and allocate space for them at runtime.
This patch creates 'struct private_data' for keeping all such information and a pointer to that would be stored in policy->driver_data.
The changed probe() routine now tries to see if regulator/clocks are available or we need to defer probe. In case they are available, it registers cpufreq driver. Otherwise, returns with -EPROBE_DEFER.
We still *don't* support platforms with separate clock/voltage lines for CPUs. This would be done in a separate patch.
--- drivers/cpufreq/cpufreq-cpu0.c | 189 +++++++++++++++++++++++++++++------------ 1 file changed, 136 insertions(+), 53 deletions(-)
drivers/cpufreq/cpufreq-cpu0.c +136 -53
@@ -28,18 +28,21 @@#include <linux/slab.h> #include <linux/thermal.h> -static unsigned int transition_latency; -static unsigned int voltage_tolerance;/* in percentage */ - -static struct device *cpu_dev; -static struct clk *cpu_clk; -static struct regulator *cpu_reg; -static struct cpufreq_frequency_table *freq_table; -static struct thermal_cooling_device *cdev; +struct private_data { +struct device *cpu_dev; +struct regulator *cpu_reg; +struct thermal_cooling_device *cdev; +unsigned int voltage_tolerance;/* in percentage */ +};static int cpu0_set_target (struct cpufreq_policy *policy,unsigned int index) {struct dev_pm_opp *opp; +struct cpufreq_frequency_table *freq_table = policy->freq_table; +struct clk *cpu_clk = policy->clk; +struct private_data *priv = policy->driver_data; +struct device *cpu_dev = priv->cpu_dev; +struct regulator *cpu_reg = priv->cpu_reg;unsigned long volt =0 , volt_old =0 , tol =0 ;unsigned int old_freq, new_freq;long freq_Hz, freq_exact;
@@ -64,7 +67,7 @@ static int cpu0_set_target(struct cpufreq_policy *policy, unsigned int index) } volt =dev_pm_opp_get_voltage (opp);rcu_read_unlock (); - tol = volt * voltage_tolerance /100 ; + tol = volt * priv->voltage_tolerance /100 ; volt_old =regulator_get_voltage (cpu_reg); }
@@ -103,26 +106,13 @@ static int cpu0_set_target(struct cpufreq_policy *policy, unsigned int index)return ret; } -static int cpu0_cpufreq_init (struct cpufreq_policy *policy) -{ - policy->clk = cpu_clk; -return cpufreq_generic_init (policy, freq_table, transition_latency); -} - -static struct cpufreq_driver cpu0_cpufreq_driver = { - .flags =CPUFREQ_STICKY |CPUFREQ_NEED_INITIAL_FREQ_CHECK , - .verify = cpufreq_generic_frequency_table_verify, - .target_index = cpu0_set_target, - .get = cpufreq_generic_get, - .init = cpu0_cpufreq_init, - .name ="generic_cpu0" , - .attr = cpufreq_generic_attr, -}; - -static int cpu0_cpufreq_probe (struct platform_device *pdev) +static int allocate_resources (struct device **cdev, +struct regulator **creg,struct clk **cclk) { -struct device_node *np; -int ret; +struct device *cpu_dev; +struct regulator *cpu_reg; +struct clk *cpu_clk; +int ret =0 ; cpu_dev =get_cpu_device (0 );if (!cpu_dev) {
@@ -130,12 +120,6 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)return -ENODEV ; } - np =of_node_get (cpu_dev->of_node); -if (!np) { -dev_err (cpu_dev,"failed to find cpu0 node\n" ); -return -ENOENT ; - } - cpu_reg =regulator_get_optional (cpu_dev,"cpu0" );if (IS_ERR (cpu_reg)) {/*
@@ -144,8 +128,7 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)*/ if (PTR_ERR (cpu_reg) == -EPROBE_DEFER ) {dev_err (cpu_dev,"cpu0 regulator not ready, retry\n" ); - ret = -EPROBE_DEFER ; - goto out_put_node; +return -EPROBE_DEFER ; }dev_warn (cpu_dev,"failed to get cpu0 regulator: %ld\n" ,PTR_ERR (cpu_reg));
@@ -153,6 +136,10 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) cpu_clk =clk_get (cpu_dev,NULL );if (IS_ERR (cpu_clk)) { +/* put regulator */ +if (!IS_ERR (cpu_reg)) +regulator_put (cpu_reg); +/* * If cpu's clk node is present, but clock is not yet * registered, we should try defering probe.
@@ -164,7 +151,39 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) ret =PTR_ERR (cpu_clk);dev_err (cpu_dev,"failed to get cpu0 clock: %d\n" , ret); } - goto out_put_reg; + }else { + *cdev = cpu_dev; + *creg = cpu_reg; + *cclk = cpu_clk; + } + +return ret; +} + +static int cpu0_cpufreq_init (struct cpufreq_policy *policy) +{ +struct cpufreq_frequency_table *freq_table; +struct thermal_cooling_device *cdev; +struct device_node *np; +struct private_data *priv; +struct device *cpu_dev; +struct regulator *cpu_reg; +struct clk *cpu_clk; +unsigned int transition_latency; +int ret; + +/* We only support cpu0 currently */ + ret =allocate_resources (&cpu_dev, &cpu_reg, &cpu_clk); +if (ret) { +pr_err ("%s: Failed to allocate resources\n: %d" , __func__, ret); +return ret; + } + + np =of_node_get (cpu_dev->of_node); +if (!np) { +dev_err (cpu_dev,"failed to find cpu%d node\n" , policy->cpu); + ret = -ENOENT ; + goto out_put_reg_clk; }/* OPPs might be populated at runtime, don't check for error here */
@@ -173,10 +192,16 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) ret =dev_pm_opp_init_cpufreq_table (cpu_dev, &freq_table);if (ret) {dev_err (cpu_dev,"failed to init cpufreq table: %d\n" , ret); - goto out_put_clk; + goto out_put_node; + } + + priv =kzalloc (sizeof (*priv),GFP_KERNEL ); +if (!priv) { + ret = -ENOMEM ; + goto out_free_table; } -of_property_read_u32 (np,"voltage-tolerance" , &voltage_tolerance); +of_property_read_u32 (np,"voltage-tolerance" , &priv->voltage_tolerance);if (of_property_read_u32 (np,"clock-latency" , &transition_latency)) transition_latency =CPUFREQ_ETERNAL ;
@@ -206,12 +231,6 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) transition_latency += ret *1000 ; } - ret =cpufreq_register_driver (&cpu0_cpufreq_driver); -if (ret) { -dev_err (cpu_dev,"failed to register driver: %d\n" , ret); - goto out_free_table; - } -/* * For now, just loading the cooling device; * thermal DT code takes care of matching them.
@@ -223,28 +242,92 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)"running cpufreq without cooling device: %ld\n" , PTR_ERR(cdev)); } -of_node_put (np); + + priv->cdev = cdev; + priv->cpu_dev = cpu_dev; + priv->cpu_reg = cpu_reg; + policy->driver_data = priv; + + policy->clk = cpu_clk; + ret =cpufreq_generic_init (policy, freq_table, transition_latency); +if (ret) + goto out_cooling_unregister; +return 0 ; +out_cooling_unregister: +cpufreq_cooling_unregister (priv->cdev); +kfree (priv); out_free_table:dev_pm_opp_free_cpufreq_table (cpu_dev, &freq_table); -out_put_clk: +out_put_node: +of_node_put (np); +out_put_reg_clk:clk_put (cpu_clk); -out_put_reg:if (!IS_ERR (cpu_reg))regulator_put (cpu_reg); -out_put_node: -of_node_put (np); + +return ret; +} + +static int cpu0_cpufreq_exit (struct cpufreq_policy *policy) +{ +struct private_data *priv = policy->driver_data; + +cpufreq_cooling_unregister (priv->cdev); +dev_pm_opp_free_cpufreq_table (priv->cpu_dev, &policy->freq_table); +clk_put (policy->clk); +if (!IS_ERR (priv->cpu_reg)) +regulator_put (priv->cpu_reg); +kfree (priv); + +return 0 ; +} + +static struct cpufreq_driver cpu0_cpufreq_driver = { + .flags =CPUFREQ_STICKY |CPUFREQ_NEED_INITIAL_FREQ_CHECK , + .verify = cpufreq_generic_frequency_table_verify, + .target_index = cpu0_set_target, + .get = cpufreq_generic_get, + .init = cpu0_cpufreq_init, + .exit = cpu0_cpufreq_exit, + .name ="generic_cpu0" , + .attr = cpufreq_generic_attr, +}; + +static int cpu0_cpufreq_probe (struct platform_device *pdev) +{ +struct device *cpu_dev; +struct regulator *cpu_reg; +struct clk *cpu_clk; +int ret; + +/* +* All per-cluster (CPUs sharing clock/voltages) initialization is done +* from ->init(). In probe(), we just need to make sure that clk and +* regulators are available. Else defer probe and retry. +* +* FIXME: Is checking this only for CPU0 sufficient ? +*/ + ret =allocate_resources (&cpu_dev, &cpu_reg, &cpu_clk); +if (ret) +return ret; + +clk_put (cpu_clk); +if (!IS_ERR (cpu_reg)) +regulator_put (cpu_reg); + + ret =cpufreq_register_driver (&cpu0_cpufreq_driver); +if (ret) +dev_err (cpu_dev,"failed register driver: %d\n" , ret); +return ret; }static int cpu0_cpufreq_remove (struct platform_device *pdev) { -cpufreq_cooling_unregister (cdev);cpufreq_unregister_driver (&cpu0_cpufreq_driver); -dev_pm_opp_free_cpufreq_table (cpu_dev, &freq_table); -return 0 ; }
Signature · 1 line
2.0.0.rc2
Re: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 166
Quoted 4 lines · Viresh Kumar
drivers/cpufreq/cpufreq-cpu0.c +4
-0+static int cpu0_cpufreq_init (struct cpufreq_policy *policy) +{ +struct cpufreq_frequency_table *freq_table; +struct thermal_cooling_device *cdev;
This..
Quoted 3 lines
drivers/cpufreq/cpufreq-cpu0.c +3
-0+struct device_node *np; +struct private_data *priv; +
[...]
drivers/cpufreq/cpufreq-cpu0.c static int cpu0_cpufreq_probe(struct platform_device *pdev) · line 247
Quoted 9 lines
ing them.
drivers/cpufreq/cpufreq-cpu0.c +2
-1@@ -223,28 +242,92 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)"running cpufreq without cooling device: %ld\n" , PTR_ERR(cdev)); } -of_node_put (np); + + priv->cdev = cdev;
Causes a build warning:
drivers/cpufreq/cpufreq-generic.c:313:13: warning: 'cdev' may be used uninitialized in this function [-Wmaybe-uninitialized]
So I guess we should initialize it to NULL?
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Viresh Kumar
maintainerRe: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()
Quoted 4 lines · Stephen Boyd
drivers/cpufreq/cpufreq-generic.c:313:13: warning: 'cdev' may be used uninitialized in this function [-Wmaybe-uninitialized]So I guess we should initialize it to NULL?
I somehow didn't got this, I checked again. I have fixed it this way:
drivers/cpufreq/cpufreq-cpu0.c +0 -0
@@ -240,10 +240,11 @@ static int cpu0_cpufreq_init(struct
cpufreq_policy *policy)
dev_err(cpu_dev,
"running cpufreq without cooling device: %ld\n", PTR_ERR(cdev));
+ else
+ priv->cdev = cdev;
}
of_node_put(np);- priv->cdev = cdev;
priv->cpu_dev = cpu_dev;
priv->cpu_reg = cpu_reg;
policy->driver_data = priv;Re: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()
In reply to Viresh KumarQuoted 18 lines · Viresh Kumar
Currently this driver only support platforms on which all CPUs share clock & voltage lines and there is requirement to support platforms which have separate clock & voltage lines for CPUs, like Qualcomm's Krait and ARM's big LITTLE.
Each group of CPUs sharing clock/voltage lines are represented by 'struct
cpufreq_policy' in cpufreq framework. And core calls ->init() once for each policy.Currently we do all initialization/allocation from
probe()which wouldn't work for above scenario. To make it work for these platforms, the first step is to move all initialization/allocation to ->init() and add ->exit() to do the reverse of it.Also, remove all global variables and allocate space for them at runtime.
This patch creates 'struct
private_data' for keeping all such information and a pointer to that would be stored inpolicy->driver_data.
Quoted 7 lines
The changed
probe()routine now tries to see if regulator/clocks are available or we need to defer probe. In case they are available, it registers cpufreq driver. Otherwise, returns with -EPROBE_DEFER.We still *don't* support platforms with separate clock/voltage lines for CPUs. This would be done in a separate patch.
I scanned this patch and subsequent patches from the series. Since you are modifying the interfaces and bindings, I just think its better if we can address the cases where separate clock lines will be used by CPUs.
Surely don't want to increase your work neither want hold the progress of the series but if you look at the changes to the interfaces, they give you a feeling of incompleteness.
Quoted 2 lines
Signed-off-by Viresh Kumar <[email protected]>---
Would you able to give some idea about what will it take to address that one remainder case as well as part of this series.
Regards,
Santosh
Viresh Kumar
maintainerRe: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()
Quoted 7 lines · Santosh Shilimkar
I scanned this patch and subsequent patches from the series. Since you are modifying the interfaces and bindings, I just think its better if we can address the cases where separate clock lines will be used by CPUs.
Surely don't want to increase your work neither want hold the progress of the series but if you look at the changes to the interfaces, they give you a feeling of incompleteness.
Lets talk in the other thread you raised, 2/14 ..
Quoted 4 lines
Quoted 2 lines
Signed-off-by Viresh Kumar <[email protected]>---
Would you able to give some idea about what will it take to address that one remainder case as well as part of this series.
Which one? This:
Quoted 2 lines
We still *don't* support platforms with separate clock/voltage lines for CPUs. This would be done in a separate patch.
Its already fixed as part of this series.
Re: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()
Quoted 16 lines · Viresh Kumar
Quoted 7 lines · Santosh Shilimkar
I scanned this patch and subsequent patches from the series. Since you are modifying the interfaces and bindings, I just think its better if we can address the cases where separate clock lines will be used by CPUs.
Surely don't want to increase your work neither want hold the progress of the series but if you look at the changes to the interfaces, they give you a feeling of incompleteness.
Lets talk in the other thread you raised, 2/14 ..
Quoted 4 lines
Quoted 2 lines
Signed-off-by Viresh Kumar <[email protected]>---
Would you able to give some idea about what will it take to address that one remainder case as well as part of this series.
Quoted 7 lines
Which one? This:
Quoted 2 lines
We still *don't* support platforms with separate clock/voltage lines for CPUs. This would be done in a separate patch.
Its already fixed as part of this series.
I suggest you word the commit in that case. Saying subsequent patch adds support for the remainder case. Let me scan the remainder patches again to see how its done.
Regards,
Santosh
Viresh Kumar
maintainerRe: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()
Quoted 2 lines · Santosh Shilimkar
Quoted 2 lines
Quoted 2 lines
We still *don't* support platforms with separate clock/voltage lines for CPUs. This would be done in a separate patch.
Probably s/separate/later would be enough to make you happy ?
Quoted 8 lines
Quoted 2 lines
Its already fixed as part of this series.
I suggest you word the commit in that case. Saying subsequent patch adds support for the remainder case. Let me scan the remainder patches again to see how its done.
Regards,
Santosh
Re: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()
Quoted 6 lines · Viresh Kumar
Quoted 2 lines · Santosh Shilimkar
Quoted 2 lines
Quoted 2 lines
We still *don't* support platforms with separate clock/voltage lines for CPUs. This would be done in a separate patch.
Probably s/separate/later would be enough to make you happy ?
For reviewer, its easy if this later is "in the current series" or really a "later" series.
Anyway I understood it now, so its upto you.
regards,
Santosh
Viresh Kumar
maintainerRe: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()
Quoted 9 lines · Santosh Shilimkar
Quoted 6 lines · Viresh Kumar
Quoted 2 lines · Santosh Shilimkar
Quoted 2 lines
Quoted 2 lines
We still *don't* support platforms with separate clock/voltage lines for CPUs. This would be done in a separate patch.
Probably s/separate/later would be enough to make you happy ?
For reviewer, its easy if this later is "in the current series" or really a "later" series.
As you say BOSS :)
Viresh Kumar
maintainerPATCH 10/14 cpufreq: cpu0: try regulators with name "cpu-supply"
In reply to Viresh KumarCurrently, we expect regulator name to be "cpu0", but as we are going to support multiple cpu-blocks (all CPUs in a block share clock/voltage) later, we need to pass some generic string instead of that.
For backwards compatibility try for "cpu0" first and if it fails, then try for "cpu".
--- drivers/cpufreq/cpufreq-cpu0.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
drivers/cpufreq/cpufreq-cpu0.c +13 -1
@@ -113,6 +113,7 @@ static int allocate_resources(struct device **cdev,struct regulator *cpu_reg;struct clk *cpu_clk;int ret =0 ; +char *reg_cpu0 ="cpu0" , *reg_cpu ="cpu" , *reg; cpu_dev =get_cpu_device (0 );if (!cpu_dev) {
@@ -120,7 +121,11 @@ static int allocate_resources(struct device **cdev,return -ENODEV ; } - cpu_reg =regulator_get_optional (cpu_dev,"cpu0" ); +/* Try "cpu0" for older DTs */ + reg = reg_cpu0; + +try_again: + cpu_reg =regulator_get_optional (cpu_dev, reg);if (IS_ERR (cpu_reg)) {/* * If cpu0 regulator supply node is present, but regulator is
@@ -130,6 +135,13 @@ static int allocate_resources(struct device **cdev,dev_err (cpu_dev,"cpu0 regulator not ready, retry\n" );return -EPROBE_DEFER ; } + +/* Try with "cpu-supply" */ +if (reg == reg_cpu0) { + reg = reg_cpu; + goto try_again; + } +dev_warn (cpu_dev,"failed to get cpu0 regulator: %ld\n" ,PTR_ERR (cpu_reg)); }
Signature · 1 line
2.0.0.rc2
Viresh Kumar
maintainerPATCH 11/14 cpufreq: cpu0: Make allocate_resources() work for any CPU
In reply to Viresh KumarCurrently allocate_resources() supports only CPU0 and it would need to allocate resources for any CPU going forward.
Add another argument to it, i.e. cpu, and update code accordingly.
--- drivers/cpufreq/cpufreq-cpu0.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)
drivers/cpufreq/cpufreq-cpu0.c +17 -13
@@ -106,7 +106,7 @@ static int cpu0_set_target(struct cpufreq_policy *policy, unsigned int index)return ret; } -static int allocate_resources (struct device **cdev, +static int allocate_resources (int cpu,struct device **cdev,struct regulator **creg,struct clk **cclk) {struct device *cpu_dev;
@@ -115,24 +115,28 @@ static int allocate_resources(struct device **cdev,int ret =0 ;char *reg_cpu0 ="cpu0" , *reg_cpu ="cpu" , *reg; - cpu_dev =get_cpu_device (0 ); + cpu_dev =get_cpu_device (cpu);if (!cpu_dev) { -pr_err ("failed to get cpu0 device\n" ); +pr_err ("failed to get cpu%d device\n" , cpu);return -ENODEV ; }/* Try "cpu0" for older DTs */ - reg = reg_cpu0; +if (!cpu) + reg = reg_cpu0; +else + reg = reg_cpu; try_again: cpu_reg =regulator_get_optional (cpu_dev, reg);if (IS_ERR (cpu_reg)) {/* -* If cpu0 regulator supply node is present, but regulator is +* If cpu's regulator supply node is present, but regulator is * not yet registered, we should try defering probe. */ if (PTR_ERR (cpu_reg) == -EPROBE_DEFER ) { -dev_err (cpu_dev,"cpu0 regulator not ready, retry\n" ); +dev_err (cpu_dev,"cpu%d regulator not ready, retry\n" , + cpu);return -EPROBE_DEFER ; }
@@ -142,8 +146,8 @@ try_again: goto try_again; } -dev_warn (cpu_dev,"failed to get cpu0 regulator: %ld\n" , -PTR_ERR (cpu_reg)); +dev_warn (cpu_dev,"failed to get cpu%d regulator: %ld\n" , + cpu,PTR_ERR (cpu_reg)); } cpu_clk =clk_get (cpu_dev,NULL );
@@ -157,11 +161,12 @@ try_again:* registered, we should try defering probe. */ if (PTR_ERR (cpu_clk) == -EPROBE_DEFER ) { -dev_err (cpu_dev,"cpu0 clock not ready, retry\n" ); +dev_err (cpu_dev,"cpu%d clock not ready, retry\n" , cpu); ret = -EPROBE_DEFER ; }else { ret =PTR_ERR (cpu_clk); -dev_err (cpu_dev,"failed to get cpu0 clock: %d\n" , ret); +dev_err (cpu_dev,"failed to get cpu%d clock: %d\n" , ret, + cpu); } }else { *cdev = cpu_dev;
@@ -184,8 +189,7 @@ static int cpu0_cpufreq_init(struct cpufreq_policy *policy)unsigned int transition_latency;int ret; -/* We only support cpu0 currently */ - ret =allocate_resources (&cpu_dev, &cpu_reg, &cpu_clk); + ret =allocate_resources (policy->cpu, &cpu_dev, &cpu_reg, &cpu_clk);if (ret) {pr_err ("%s: Failed to allocate resources\n: %d" , __func__, ret);return ret;
@@ -322,7 +326,7 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)* * FIXME: Is checking this only for CPU0 sufficient ? */ - ret =allocate_resources (&cpu_dev, &cpu_reg, &cpu_clk); + ret =allocate_resources (0 , &cpu_dev, &cpu_reg, &cpu_clk);if (ret)return ret;
Signature · 1 line
2.0.0.rc2
Viresh Kumar
maintainerPATCH 12/14 cpufreq: cpu0: Extend support beyond CPU0
In reply to Viresh KumarMost of the infrastructure is in place now, with only little left. How to find siblings ?
Stephen Boyd suggested to compare "clocks" properties from CPU's DT node and siblings should match. This patch adds another routine find_siblings() which calls of_clk_shared_by_cpus() to find if CPUs share clock line or not.
If of_clk_shared_by_cpus() returns error, we fallback to all CPUs sharing clock line assumption as existing platforms don't have "clocks" property in all CPU nodes and would fail from of_clk_shared_by_cpus().
--- .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt | 72 ++++++++++++++++++++-- drivers/cpufreq/cpufreq-cpu0.c | 35 ++++++++++- 2 files changed, 101 insertions(+), 6 deletions(-)
Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt +68 -4
@@ -1,11 +1,11 @@ -Generic CPU0 cpufreq driver +Generic cpufreq driver -It is a generic cpufreq driver for CPU0 frequency management. It +It is a generic cpufreq driver for frequency management. It supports both uniprocessor (UP) and symmetric multiprocessor (SMP) -systems which share clock and voltage across all CPUs. +systems which may or maynot share clock and voltage across all CPUs. Both required and optional properties listed below must be defined -under node /cpus/cpu@0. +under node /cpus/cpu@x. Where x is the first cpu inside a cluster. Required properties: - operating-points: Refer to Documentation/devicetree/bindings/power/opp.txt
@@ -19,9 +19,16 @@ Optional properties: - cooling-min-level: - cooling-max-level: Please refer to Documentation/devicetree/bindings/thermal/thermal.txt. +- clocks: If CPU clock is populated from DT, "clocks" property must be copied to + every cpu node sharing clock with cpu@x. Generic cpufreq driver compares + "clocks" to find siblings, i.e. to see which CPUs share clock/voltages. If + only cpu@0 contains "clocks" property it is assumed that all CPUs share clock + line. Examples: +1. All CPUs share clock/voltages + cpus { #address-cells = <1>; #size-cells = <0>;
@@ -36,6 +43,8 @@ cpus { 396000 950000 198000 850000 >; + clocks = <&clock CLK_ARM_CLK>; + clock-names = "cpu"; clock-latency = <61036>; /* two CLK32 periods */ #cooling-cells = <2>; cooling-min-level = <0>;
@@ -46,17 +55,72 @@ cpus { compatible = "arm,cortex-a9"; reg = <1>; next-level-cache = <&L2>; + clocks = <&clock CLK_ARM_CLK>; }; cpu@2 { compatible = "arm,cortex-a9"; reg = <2>; next-level-cache = <&L2>; + clocks = <&clock CLK_ARM_CLK>; }; cpu@3 { compatible = "arm,cortex-a9"; reg = <3>; next-level-cache = <&L2>; + clocks = <&clock CLK_ARM_CLK>; + }; +}; + + +2. All CPUs inside a cluster share clock/voltages, there are multiple clusters. + +cpus { + #address-cells = <1>; + #size-cells = <0>; + + cpu@0 { + compatible = "arm,cortex-a15"; + reg = <0>; + next-level-cache = <&L2>; + operating-points = < + /* kHz uV */ + 792000 1100000 + 396000 950000 + 198000 850000 + >; + clocks = <&clock CLK_ARM1_CLK>; + clock-names = "cpu"; + clock-latency = <61036>; /* two CLK32 periods */ + }; + + cpu@1 { + compatible = "arm,cortex-a15"; + reg = <1>; + next-level-cache = <&L2>; + clocks = <&clock CLK_ARM1_CLK>; + }; + + cpu@100 { + compatible = "arm,cortex-a7"; + reg = <100>; + next-level-cache = <&L2>; + operating-points = < + /* kHz uV */ + 792000 950000 + 396000 750000 + 198000 450000 + >; + clocks = <&clock CLK_ARM2_CLK>; + clock-names = "cpu"; + clock-latency = <61036>; /* two CLK32 periods */ + }; + + cpu@101 { + compatible = "arm,cortex-a7"; + reg = <101>; + next-level-cache = <&L2>; + clocks = <&clock CLK_ARM2_CLK>; }; };
drivers/cpufreq/cpufreq-cpu0.c +33 -2
@@ -177,6 +177,30 @@ try_again:return ret; } +/* +* Sets all CPUs as sibling if any cpu doesn't have a "clocks" property, +* Otherwise it matches "clocks" property to find siblings. +*/ +static void find_siblings (struct cpufreq_policy *policy) +{ +int cpu, ret; + +for_each_possible_cpu (cpu) { +if (cpu == policy->cpu) +continue ; + + ret =of_clk_shared_by_cpus (policy->cpu, cpu); + +/* Error while parsing nodes, fallback to set-all */ +if (ret <0 ) { +cpumask_setall (policy->cpus); +return ; + }else if (ret ==1 ) { +cpumask_set_cpu (cpu, policy->cpus); + } + } +} +static int cpu0_cpufreq_init (struct cpufreq_policy *policy) {struct cpufreq_frequency_table *freq_table;
@@ -266,9 +290,16 @@ static int cpu0_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) + +find_siblings (policy); + 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;return 0 ;
Signature · 1 line
2.0.0.rc2
Viresh Kumar
maintainerPATCH 13/14 cpufreq: cpu0: rename driver and internals to 'cpufreq_generic'
In reply to Viresh KumarThis is not a cpu0 driver anymore as it supports any number of clusters with separate clock/voltage lines.
Lets rename it to 'cpufreq_generic' from 'cpufreq_cpu0'.
'.name' field of platform driver/devices isn't renamed yet, would be done separately.
---
.../{cpufreq-cpu0.txt => cpufreq-generic.txt} | 0
drivers/cpufreq/Kconfig | 10 +++---
drivers/cpufreq/Kconfig.arm | 2 +-
drivers/cpufreq/Makefile | 2 +-
.../cpufreq/{cpufreq-cpu0.c => cpufreq-generic.c} | 36 +++++++++++-----------
5 files changed, 25 insertions(+), 25 deletions(-)
rename Documentation/devicetree/bindings/cpufreq/{cpufreq-cpu0.txt => cpufreq-generic.txt} (100%)
rename drivers/cpufreq/{cpufreq-cpu0.c => cpufreq-generic.c} (91%)Documentation/devicetree/bindings/cpufreq/cpufreq-generic.txt renamed from Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt +0 -0
drivers/cpufreq/Kconfig +5 -5
@@ -183,16 +183,16 @@ config CPU_FREQ_GOV_CONSERVATIVE If in doubt, say N. -config GENERIC_CPUFREQ_CPU0 - tristate "Generic CPU0 cpufreq driver" +config CPUFREQ_GENERIC + tristate "Generic cpufreq driver" depends on HAVE_CLK && OF - # if CPU_THERMAL is on and THERMAL=m, CPU0 cannot be =y: + # if CPU_THERMAL is on and THERMAL=m, GENERIC cannot be =y: depends on !CPU_THERMAL || THERMAL select PM_OPP help - This adds a generic cpufreq driver for CPU0 frequency management. + This adds a generic cpufreq driver for frequency management. It supports both uniprocessor (UP) and symmetric multiprocessor (SMP) - systems which share clock and voltage across all CPUs. + systems which may or maynot share clock and voltage across all CPUs. If in doubt, say N.
drivers/cpufreq/Kconfig.arm +1 -1
@@ -92,7 +92,7 @@ config ARM_EXYNOS_CPU_FREQ_BOOST_SW config ARM_HIGHBANK_CPUFREQ tristate "Calxeda Highbank-based" - depends on ARCH_HIGHBANK && GENERIC_CPUFREQ_CPU0 && REGULATOR + depends on ARCH_HIGHBANK && CPUFREQ_GENERIC && REGULATOR default m help This adds the CPUFreq driver for Calxeda Highbank SoC
drivers/cpufreq/Makefile +1 -1
@@ -13,7 +13,7 @@ obj-$(CONFIG_CPU_FREQ_GOV_ONDEMAND) += cpufreq_ondemand.o obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE) += cpufreq_conservative.o obj-$(CONFIG_CPU_FREQ_GOV_COMMON) += cpufreq_governor.o -obj-$(CONFIG_GENERIC_CPUFREQ_CPU0) += cpufreq-cpu0.o +obj-$(CONFIG_CPUFREQ_GENERIC) += cpufreq-generic.o ################################################################################## # x86 drivers.
drivers/cpufreq/cpufreq-generic.c renamed from drivers/cpufreq/cpufreq-cpu0.c +18 -18
@@ -4,7 +4,7 @@* Copyright (C) 2014 Linaro. * Viresh Kumar <[email protected]> * -* The OPP code in function cpu0_set_target() is reused from +* The OPP code in function set_target() is reused from * drivers/cpufreq/omap-cpufreq.c * * This program is free software; you can redistribute it and/or modify
@@ -35,7 +35,7 @@ struct private_data {unsigned int voltage_tolerance;/* in percentage */ }; -static int cpu0_set_target (struct cpufreq_policy *policy,unsigned int index) +static int set_target (struct cpufreq_policy *policy,unsigned int index) {struct dev_pm_opp *opp;struct cpufreq_frequency_table *freq_table = policy->freq_table;
@@ -201,7 +201,7 @@ static void find_siblings(struct cpufreq_policy *policy) } } -static int cpu0_cpufreq_init (struct cpufreq_policy *policy) +static int cpufreq_init (struct cpufreq_policy *policy) {struct cpufreq_frequency_table *freq_table;struct thermal_cooling_device *cdev;
@@ -318,7 +318,7 @@ out_put_reg_clk:return ret; } -static int cpu0_cpufreq_exit (struct cpufreq_policy *policy) +static int cpufreq_exit (struct cpufreq_policy *policy) {struct private_data *priv = policy->driver_data;
@@ -332,18 +332,18 @@ static int cpu0_cpufreq_exit(struct cpufreq_policy *policy)return 0 ; } -static struct cpufreq_driver cpu0_cpufreq_driver = { +static struct cpufreq_driver generic_cpufreq_driver = { .flags =CPUFREQ_STICKY |CPUFREQ_NEED_INITIAL_FREQ_CHECK , .verify = cpufreq_generic_frequency_table_verify, - .target_index = cpu0_set_target, + .target_index = set_target, .get = cpufreq_generic_get, - .init = cpu0_cpufreq_init, - .exit = cpu0_cpufreq_exit, - .name ="generic_cpu0" , + .init = cpufreq_init, + .exit = cpufreq_exit, + .name ="cpufreq-generic" , .attr = cpufreq_generic_attr, }; -static int cpu0_cpufreq_probe (struct platform_device *pdev) +static int generic_cpufreq_probe (struct platform_device *pdev) {struct device *cpu_dev;struct regulator *cpu_reg;
@@ -365,30 +365,30 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)if (!IS_ERR (cpu_reg))regulator_put (cpu_reg); - ret =cpufreq_register_driver (&cpu0_cpufreq_driver); + ret =cpufreq_register_driver (&generic_cpufreq_driver);if (ret)dev_err (cpu_dev,"failed register driver: %d\n" , ret);return ret; } -static int cpu0_cpufreq_remove (struct platform_device *pdev) +static int generic_cpufreq_remove (struct platform_device *pdev) { -cpufreq_unregister_driver (&cpu0_cpufreq_driver); +cpufreq_unregister_driver (&generic_cpufreq_driver);return 0 ; } -static struct platform_driver cpu0_cpufreq_platdrv = { +static struct platform_driver generic_cpufreq_platdrv = { .driver = { .name ="cpufreq-cpu0" , .owner =THIS_MODULE , }, - .probe = cpu0_cpufreq_probe, - .remove = cpu0_cpufreq_remove, + .probe = generic_cpufreq_probe, + .remove = generic_cpufreq_remove, }; -module_platform_driver (cpu0_cpufreq_platdrv); +module_platform_driver (generic_cpufreq_platdrv);MODULE_AUTHOR ("Viresh Kumar <[email protected]>" );MODULE_AUTHOR ("Shawn Guo <[email protected]>" ); -MODULE_DESCRIPTION ("Generic CPU0 cpufreq driver" ); +MODULE_DESCRIPTION ("Generic cpufreq driver" );MODULE_LICENSE ("GPL" );
Signature · 1 line
2.0.0.rc2
Viresh Kumar
maintainerPATCH 14/14 cpufreq: generic: set platform_{driver|device} '.name' to 'cpufreq-generic'
In reply to Viresh KumarWe have already renamed cpufreq-cpu0 to cpufreq-generic at most of the places, the only one left is in the '.name' field of platform driver and devices.
Lets do it now for all users.
Updates cpufreq-cpu0 from comments as well.
---
@Santosh: Probably you can drop omap's cpufreq driver now?
arch/arm/mach-imx/imx27-dt.c | 2 +- arch/arm/mach-imx/imx51-dt.c | 2 +- arch/arm/mach-omap2/pm.c | 2 +- arch/arm/mach-shmobile/board-ape6evm-reference.c | 2 +- arch/arm/mach-shmobile/setup-sh73a0.c | 4 ++-- arch/arm/mach-zynq/common.c | 2 +- drivers/cpufreq/cpufreq-generic.c | 2 +- drivers/cpufreq/exynos4x12-cpufreq.c | 2 +- drivers/cpufreq/highbank-cpufreq.c | 6 +++--- 9 files changed, 12 insertions(+), 12 deletions(-)
arch/arm/mach-imx/imx27-dt.c +1 -1
@@ -20,7 +20,7 @@static void __init imx27_dt_init (void ) { -struct platform_device_info devinfo = { .name ="cpufreq-cpu0" , }; +struct platform_device_info devinfo = { .name ="cpufreq-generic" , };mxc_arch_reset_init_dt ();
arch/arm/mach-imx/imx51-dt.c +1 -1
@@ -21,7 +21,7 @@static void __init imx51_dt_init (void ) { -struct platform_device_info devinfo = { .name ="cpufreq-cpu0" , }; +struct platform_device_info devinfo = { .name ="cpufreq-generic" , };mxc_arch_reset_init_dt ();
arch/arm/mach-omap2/pm.c +1 -1
@@ -282,7 +282,7 @@ static inline void omap_init_cpufreq(void)if (!of_have_populated_dt ()) devinfo.name ="omap-cpufreq" ;else - devinfo.name ="cpufreq-cpu0" ; + devinfo.name ="cpufreq-generic" ;platform_device_register_full (&devinfo); }
arch/arm/mach-shmobile/board-ape6evm-reference.c +1 -1
@@ -48,7 +48,7 @@ static void __init ape6evm_add_standard_devices(void)r8a73a4_add_dt_devices ();of_platform_populate (NULL , of_default_bus_match_table,NULL ,NULL ); -platform_device_register_simple ("cpufreq-cpu0" ,-1 ,NULL ,0 ); +platform_device_register_simple ("cpufreq-generic" ,-1 ,NULL ,0 ); }static const char *ape6evm_boards_compat_dt[]__initdata = {
arch/arm/mach-shmobile/setup-sh73a0.c +2 -2
@@ -774,7 +774,7 @@ void __init sh73a0_add_early_devices(void)void __init sh73a0_add_standard_devices_dt (void ) { -struct platform_device_info devinfo = { .name ="cpufreq-cpu0" , .id =-1 , }; +struct platform_device_info devinfo = { .name ="cpufreq-generic" , .id =-1 , };/* clocks are setup late during boot in the case of DT */ sh73a0_clock_init ();
@@ -783,7 +783,7 @@ void __init sh73a0_add_standard_devices_dt(void) ARRAY_SIZE(sh73a0_devices_dt));of_platform_populate (NULL , of_default_bus_match_table,NULL ,NULL ); -/* Instantiate cpufreq-cpu0 */ +/* Instantiate cpufreq-generic */ platform_device_register_full (&devinfo); }
arch/arm/mach-zynq/common.c +1 -1
@@ -104,7 +104,7 @@ static int __init zynq_get_revision(void)*/ static void __init zynq_init_machine (void ) { -struct platform_device_info devinfo = { .name ="cpufreq-cpu0" , }; +struct platform_device_info devinfo = { .name ="cpufreq-generic" , };struct soc_device_attribute *soc_dev_attr;struct soc_device *soc_dev;struct device *parent =NULL ;
drivers/cpufreq/cpufreq-generic.c +1 -1
@@ -380,7 +380,7 @@ static int generic_cpufreq_remove(struct platform_device *pdev)static struct platform_driver generic_cpufreq_platdrv = { .driver = { - .name ="cpufreq-cpu0" , + .name ="cpufreq-generic" , .owner =THIS_MODULE , }, .probe = generic_cpufreq_probe,
drivers/cpufreq/exynos4x12-cpufreq.c +1 -1
@@ -174,7 +174,7 @@ int exynos4x12_cpufreq_init(struct exynos_dvfs_info *info)* dependencies on platform headers. It is necessary to enable * Exynos multi-platform support and will be removed together with * this whole driver as soon as Exynos gets migrated to use -* cpufreq-cpu0 driver. +* cpufreq-generic driver. */ np =of_find_compatible_node (NULL ,NULL ,"samsung,exynos4412-clock" );if (!np) {
drivers/cpufreq/highbank-cpufreq.c +3 -3
@@ -6,7 +6,7 @@* published by the Free Software Foundation. * * This driver provides the clk notifier callbacks that are used when -* the cpufreq-cpu0 driver changes to frequency to alert the highbank +* the cpufreq-generic driver changes to frequency to alert the highbank * EnergyCore Management Engine (ECME) about the need to change * voltage. The ECME interfaces with the actual voltage regulators. */
@@ -60,7 +60,7 @@ static struct notifier_block hb_cpufreq_clk_nb = {static int hb_cpufreq_driver_init (void ) { -struct platform_device_info devinfo = { .name ="cpufreq-cpu0" , }; +struct platform_device_info devinfo = { .name ="cpufreq-generic" , };struct device *cpu_dev;struct clk *cpu_clk;struct device_node *np;
@@ -95,7 +95,7 @@ static int hb_cpufreq_driver_init(void) goto out_put_node; } -/* Instantiate cpufreq-cpu0 */ +/* Instantiate cpufreq-generic */ platform_device_register_full (&devinfo); out_put_node:
Signature · 1 line
2.0.0.rc2
Viresh Kumar
maintainerQuoted 19 lines · Viresh Kumar
V1: https://lkml.org/lkml/2014/6/25/152
Stephen Boyd sent few patches some time back around a new cpufreq driver for Qualcomm's Krait SoC: https://lkml.org/lkml/2014/6/24/918.
Krait couldn't use existing cpufreq-cpu0 driver as it doesn't have support for SoC's with multiple clusters or SoC's which don't share clock line across all CPUs.
This patchset is all about extending support beyond CPU0. It can be used for platforms like Krait or ARM big LITTLE architecture now.
First two patches add helper routine for of and clk layer, which would be used by later patches.
Third one adds space for driver specific data in 'struct
cpufreq_policy' and later ones migrate cpufreq-cpu0 to cpufreq-generic, i.e. can be used for SoCs which don't share clock lines across all CPUs.
Quoted 6 lines
@Stephen: I haven't added your Tested-by as there were few modifications since the time you tested it last.
Pushed here:
Rebased over v3.16-rc3:git://git.linaro.org/people/viresh.kumar/linux.gitcpufreq/cpu0-krait-v2
Hi Stephen,
As suggested by you I have updated patch
7/14: cpufreq: cpu0: OPPs can be populated at runtime 12/14: cpufreq: cpu0: Extend support beyond CPU0
and dropped
2/14: clk: Create of_clk_shared_by_cpus()
Please see if they look fine and work for you.
git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/cpu0-krait-v3
Stephen Boyd
Tested-byQuoted 33 lines · Viresh Kumar
Quoted 19 lines · Viresh Kumar
V1: https://lkml.org/lkml/2014/6/25/152
Stephen Boyd sent few patches some time back around a new cpufreq driver for Qualcomm's Krait SoC: https://lkml.org/lkml/2014/6/24/918.
Krait couldn't use existing cpufreq-cpu0 driver as it doesn't have support for SoC's with multiple clusters or SoC's which don't share clock line across all CPUs.
This patchset is all about extending support beyond CPU0. It can be used for platforms like Krait or ARM big LITTLE architecture now.
First two patches add helper routine for of and clk layer, which would be used by later patches.
Third one adds space for driver specific data in 'struct
cpufreq_policy' and later ones migrate cpufreq-cpu0 to cpufreq-generic, i.e. can be used for SoCs which don't share clock lines across all CPUs.Quoted 6 lines
@Stephen: I haven't added your Tested-by as there were few modifications since the time you tested it last.
Pushed here:
Rebased over v3.16-rc3:git://git.linaro.org/people/viresh.kumar/linux.gitcpufreq/cpu0-krait-v2Hi Stephen,
As suggested by you I have updated patch
7/14: cpufreq: cpu0: OPPs can be populated at runtime 12/14: cpufreq: cpu0: Extend support beyond CPU0
Quoted 6 lines
and dropped
2/14: clk: Createof_clk_shared_by_cpus()Please see if they look fine and work for you.
git://git.linaro.org/people/viresh.kumar/linux.gitcpufreq/cpu0-krait-v3
I gave it a spin. It works so you can have my
I'm still concerned about the patch where we figure out if the clocks are shared. I worry about a configuration where there are different clocks for on/off (i.e. gates) that are per-cpu but they all source from the same divider or something that is per-cluster. In DT this may be described as different clock provider outputs for the gates and in the cpu node we would have different clock specifiers but in reality all the CPUs in that cluster are affected by the same frequency scaling.
In this case we'll need to get help from the clock framework to determine that those gates clocks don't have any .set_rate() callback so they can't actually change rate independently, and then walk up the tree to their parents to see if they have a common ancestor that does change rates. That's where it becomes useful to have a clock framework API for this, like clk_shares_rate(struct clk *clk, struct clk *peer) or something that can hide all this from cpufreq. Here's what I think it would look like (totally untested/uncompiled):
static struct clk *find_rate_changer(struct clk *clk) {
if (!clk)
return NULL;
do {
/* Rate could change by changing parents */
if (clk->num_parents > 1)
return clk;
/* Rate could change by calling clk_set_rate() */
if (clk->ops->set_rate)
return clk;
/*
* This is odd, clk_set_rate() doesn't propagate
* and this clock can't change rate or parents
* so we must be at the root and the clock we
* started at can't change rates. Just return the
* root so that we can see if the other clock shares
* the same root although CPUfreq should never care.
*/
if (!(clk->flags & CLK_SET_RATE_PARENT))
return clk;
} while ((clk = clk->parent))
return NULL;
}bool clk_shares_rate(struct clk *clk, struct clk *peer)
{
struct clk *p1, *p2;
p1 = find_rate_changer(clk);
p2 = find_rate_changer(peer)
return p1 == p2;
}Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Viresh Kumar
maintainerQuoted 3 lines · Stephen Boyd
I gave it a spin. It works so you can have my
Tested-by Stephen Boyd <[email protected]>
Thanks, all suggested improvements are made and pushed again with your Tested-by..
Quoted 7 lines
I'm still concerned about the patch where we figure out if the clocks are shared. I worry about a configuration where there are different clocks for on/off (i.e. gates) that are per-cpu but they all source from the same divider or something that is per-cluster. In DT this may be described as different clock provider outputs for the gates and in the cpu node we would have different clock specifiers but in reality all the CPUs in that cluster are affected by the same frequency scaling.
Yeah, this is probably what matches with Rob's doubt. These can actually be different. Good point.
Quoted 40 lines
In this case we'll need to get help from the clock framework to determine that those gates clocks don't have any .set_rate() callback so they can't actually change rate independently, and then walk up the tree to their parents to see if they have a common ancestor that does change rates. That's where it becomes useful to have a clock framework API for this, like
clk_shares_rate(struct clk *clk, struct clk *peer)or something that can hide all this from cpufreq. Here's what I think it would look like (totally untested/uncompiled):static struct clk *find_rate_changer(struct clk *clk) {
if (!clk) return NULL; do { /* Rate could change by changing parents */ if (clk->num_parents > 1) return clk; /* Rate could change by calling clk_set_rate() */ if (clk->ops->set_rate) return clk; /* * This is odd, clk_set_rate() doesn't propagate * and this clock can't change rate or parents * so we must be at the root and the clock we * started at can't change rates. Just return the * root so that we can see if the other clock shares * the same root although CPUfreq should never care. */ if (!(clk->flags & CLK_SET_RATE_PARENT)) return clk; } while ((clk = clk->parent)) return NULL; }bool
clk_shares_rate(struct clk *clk, struct clk *peer)
Quoted 8 lines
{
struct clk *p1, *p2;
p1 = find_rate_changer(clk);
p2 = find_rate_changer(peer)
return p1 == p2;
}I find it much better then doing what I did initially, simply matching clk_get() outputs. Lets see what Mike has to say..
@Mike: Is this less ugly ? :)
Quoting Viresh Kumar (2014-07-02 19:44:04)
Quoted 62 lines · Stephen Boyd
Quoted 3 lines
I gave it a spin. It works so you can have my
Tested-by Stephen Boyd <[email protected]>Thanks, all suggested improvements are made and pushed again with your Tested-by..
Quoted 7 lines
I'm still concerned about the patch where we figure out if the clocks are shared. I worry about a configuration where there are different clocks for on/off (i.e. gates) that are per-cpu but they all source from the same divider or something that is per-cluster. In DT this may be described as different clock provider outputs for the gates and in the cpu node we would have different clock specifiers but in reality all the CPUs in that cluster are affected by the same frequency scaling.
Yeah, this is probably what matches with Rob's doubt. These can actually be different. Good point.
Quoted 43 lines
In this case we'll need to get help from the clock framework to determine that those gates clocks don't have any .set_rate() callback so they can't actually change rate independently, and then walk up the tree to their parents to see if they have a common ancestor that does change rates. That's where it becomes useful to have a clock framework API for this, like
clk_shares_rate(struct clk *clk, struct clk *peer)or something that can hide all this from cpufreq. Here's what I think it would look like (totally untested/uncompiled):static struct clk *find_rate_changer(struct clk *clk) {
if (!clk) return NULL; do { /* Rate could change by changing parents */ if (clk->num_parents > 1) return clk; /* Rate could change by calling clk_set_rate() */ if (clk->ops->set_rate) return clk; /* * This is odd, clk_set_rate() doesn't propagate * and this clock can't change rate or parents * so we must be at the root and the clock we * started at can't change rates. Just return the * root so that we can see if the other clock shares * the same root although CPUfreq should never care. */ if (!(clk->flags & CLK_SET_RATE_PARENT)) return clk; } while ((clk = clk->parent)) return NULL; }bool
clk_shares_rate(struct clk *clk, struct clk *peer){ struct clk *p1, *p2;
Quoted 8 lines
Quoted 5 lines
p1 = find_rate_changer(clk); p2 = find_rate_changer(peer) return p1 == p2; }I find it much better then doing what I did initially, simply matching
clk_get()outputs. Lets see what Mike has to say..
Sorry for being dense, but I still do not get why trying to dynamically discover a shared rate-changeable clock is a better approach than simply describing the hardware in DT?
Is adding a property to the CPU binding that describes how the CPUs in a cluster expect to use a clock somehow a non-starter? It is certainly a win for readability when staring at DT and trying to understand how DVFS on that CPU is meant to work (as opposed to hiding that knowledge behind a tree walk).
Regards,
Mike
Quoted 2 lines
@Mike: Is this less ugly ? :)
Viresh Kumar
maintainerQuoted 9 lines · Mike Turquette
Sorry for being dense, but I still do not get why trying to dynamically discover a shared rate-changeable clock is a better approach than simply describing the hardware in DT?
Is adding a property to the CPU binding that describes how the CPUs in a cluster expect to use a clock somehow a non-starter? It is certainly a win for readability when staring at DT and trying to understand how DVFS on that CPU is meant to work (as opposed to hiding that knowledge behind a tree walk).
Yeah, having something like what you suggested from DT is the perfect solution to get over this. The only reason why I am not touching that here is to not delay other patches just because of that.
There are separate threads going on for that and probably somebody else was trying to push for that.
That's it, nothing more. I would definitely like to use those bindings instead
of the crazy routines we are trying here, once that is finalized :)
--
viresh
Viresh Kumar
maintainerQuoted 9 lines · Viresh Kumar
Yeah, having something like what you suggested from DT is the perfect solution to get over this. The only reason why I am not touching that here is to not delay other patches just because of that.
There are separate threads going on for that and probably somebody else was trying to push for that.
That's it, nothing more. I would definitely like to use those bindings instead
of the crazy routines we are trying here, once that is finalized :)
Do we have some kind of agreement for this temporary solution? Anyways I will kick the other thread again to resolve the bindings soon.
@Stephen: Do you still want find_rate_changer() stuff in this series only and or this can go into 3.17 without anymore changes and lets finalize the bindings Mike suggested and then update this code?
--
viresh
Quoted 8 lines · Viresh Kumar
Quoted 7 lines · Viresh Kumar
Yeah, having something like what you suggested from DT is the perfect solution to get over this. The only reason why I am not touching that here is to not delay other patches just because of that.
There are separate threads going on for that and probably somebody else was trying to push for that.
Quoted 10 lines
Quoted 2 lines
That's it, nothing more. I would definitely like to use those bindings instead
of the crazy routines we are trying here, once that is finalized :)Do we have some kind of agreement for this temporary solution? Anyways I will kick the other thread again to resolve the bindings soon.
@Stephen: Do you still want
find_rate_changer()stuff in this series only and or this can go into 3.17 without anymore changes and lets finalize the bindings Mike suggested and then update this code?
Finding the rate change is not necessary for me. I don't imagine my code will be getting into 3.17 anyway though so I'm no in a rush to merge anything immediately.
I still prefer the common clock framework API. If we go ahead with the find_rate_changer() stuff we've pretty well insulated this driver from any DTisms, which is nice. The only thing left would be transition-latency and voltage-tolerance, but those are already optional so you really don't need to even run on a DT enabled platform to use this code.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Viresh Kumar
maintainerCc'ing Thomas,
Quoted 5 lines · Viresh Kumar
Quoted 4 lines · Viresh Kumar
Yeah, having something like what you suggested from DT is the perfect solution to get over this. The only reason why I am not touching that here is to not delay other patches just because of that.
Quoted 8 lines
Quoted 5 lines
There are separate threads going on for that and probably somebody else was trying to push for that.
That's it, nothing more. I would definitely like to use those bindings instead
of the crazy routines we are trying here, once that is finalized :)Do we have some kind of agreement for this temporary solution? Anyways I will kick the other thread again to resolve the bindings soon.
Hi Mike/Rafael,
Thomas has a dependency on the patch adding "find_siblings()":
http://www.spinics.net/lists/arm-kernel/msg348080.html
Would it be fine to get this temporary solution (Only within cpufreq-cpu0 file, so that it doesn't become an API) for now and updating it later once bindings are closed?
I have tried pinging on the other thread as well, but no one replied. And it looks those bindings are going to take some time to settle.
--
viresh
Dear Viresh Kumar,
Quoted 1 line · Viresh Kumar
Cc'ing Thomas,
Thanks. Also adding Jason Cooper, Andrew Lunn, Grégory Clement and Sebastian Hesselbart (the mvebu platform maintainers, which include Marvell Armada XP).
Quoted 20 lines · Viresh Kumar
Quoted 5 lines · Viresh Kumar
Quoted 4 lines
Yeah, having something like what you suggested from DT is the perfect solution to get over this. The only reason why I am not touching that here is to not delay other patches just because of that.
Quoted 8 lines
Quoted 5 lines
There are separate threads going on for that and probably somebody else was trying to push for that.
That's it, nothing more. I would definitely like to use those bindings instead
of the crazy routines we are trying here, once that is finalized :)Do we have some kind of agreement for this temporary solution? Anyways I will kick the other thread again to resolve the bindings soon.
Hi Mike/Rafael,
Thomas has a dependency on the patch adding "
find_siblings()":
http://www.spinics.net/lists/arm-kernel/msg348080.html
Quoted 6 lines
Would it be fine to get this temporary solution (Only within cpufreq-cpu0 file, so that it doesn't become an API) for now and updating it later once bindings are closed?
I have tried pinging on the other thread as well, but no one replied. And it looks those bindings are going to take some time to settle.
Alternatively, I could respin my Armada XP specific cpufreq driver, until a proper cpufreq-generic driver is sorted out, if that's needed. Since I was told by Viresh that the cpufreq-generic driver supporting independent per-CPU clocks would be merged for 3.17, I based my cpufreq development on that. Please let us know quickly if that's not going to be the case, so that we can enable cpufreq on Armada XP in 3.17, even if it's done with a separate cpufreq driver, until cpufreq-generic issues get sorted out.
Thanks a lot!
Thomas
Signature · 8 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com _______________________________________________ linaro-kernel mailing list [email protected] http://lists.linaro.org/mailman/listinfo/linaro-kernel
Quoted 36 lines · Thomas Petazzoni
Dear Viresh Kumar,
Quoted 1 line · Viresh Kumar
Cc'ing Thomas,
Thanks. Also adding Jason Cooper, Andrew Lunn, Grégory Clement and Sebastian Hesselbart (the mvebu platform maintainers, which include Marvell Armada XP).
Quoted 20 lines · Viresh Kumar
Quoted 5 lines · Viresh Kumar
Quoted 4 lines
Yeah, having something like what you suggested from DT is the perfect solution to get over this. The only reason why I am not touching that here is to not delay other patches just because of that.
Quoted 8 lines
Quoted 5 lines
There are separate threads going on for that and probably somebody else was trying to push for that.
That's it, nothing more. I would definitely like to use those bindings instead
of the crazy routines we are trying here, once that is finalized :)Do we have some kind of agreement for this temporary solution? Anyways I will kick the other thread again to resolve the bindings soon.
Hi Mike/Rafael,
Thomas has a dependency on the patch adding "
find_siblings()":
http://www.spinics.net/lists/arm-kernel/msg348080.htmlQuoted 6 lines
Would it be fine to get this temporary solution (Only within cpufreq-cpu0 file, so that it doesn't become an API) for now and updating it later once bindings are closed?
I have tried pinging on the other thread as well, but no one replied. And it looks those bindings are going to take some time to settle.
Quoted 6 lines
Alternatively, I could respin my Armada XP specific cpufreq driver, until a proper cpufreq-generic driver is sorted out, if that's needed. Since I was told by Viresh that the cpufreq-generic driver supporting independent per-CPU clocks would be merged for 3.17, I based my cpufreq development on that. Please let us know quickly if that's not going to be the case,
First off, I'm sorry I may not be able to do that quickly. I'll let you know when I get to that material.
Quoted 3 lines
so that we can enable cpufreq on Armada XP in 3.17, even if it's done with a separate cpufreq driver, until cpufreq-generic issues get sorted out.
If you target the generic one, I'd strongly recommend against trying to do a separate driver even if the generic thing is not ready for the merge window.
Kind regards,
Rafael
_______________________________________________
linaro-kernel mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/linaro-kernel
Quoted 17 lines · Viresh Kumar
Cc'ing Thomas,
Quoted 5 lines · Viresh Kumar
Quoted 4 lines · Viresh Kumar
Yeah, having something like what you suggested from DT is the perfect solution to get over this. The only reason why I am not touching that here is to not delay other patches just because of that.
Quoted 8 lines
Quoted 5 lines
There are separate threads going on for that and probably somebody else was trying to push for that.
That's it, nothing more. I would definitely like to use those bindings instead
of the crazy routines we are trying here, once that is finalized :)Do we have some kind of agreement for this temporary solution? Anyways I will kick the other thread again to resolve the bindings soon.
Quoted 8 lines
Hi Mike/Rafael,
Thomas has a dependency on the patch adding "
find_siblings()":
http://www.spinics.net/lists/arm-kernel/msg348080.htmlWould it be fine to get this temporary solution (Only within cpufreq-cpu0 file, so that it doesn't become an API) for now and updating it later once bindings are closed?
I don't like that idea, but I wonder what other people think.
Rafael
Viresh Kumar
maintainerQuoted 1 line · Rafael J. Wysocki
I don't like that idea, but I wonder what other people think.
Hmm, the other thread around looking at the bindings is really slow.
One common thing around the platforms which want to use cpufreq-cpu0 is they have different clocks for ALL CPUs.
I was wondering if instead of a clock-matching routine, we can provide some temporary relief to them via some other means.
I meant we can allow cpufreq-cpu0/generic to either set policy->cpus to ALL CPUs or just 1. So that existing and these new platforms can atleast get going..
But don't know how should we do that. Not a binding ofcourse, a Kconfig option could work but multiplatform stuff would break. What else?
Maybe platform data as we are handling cpufreq-cpu0 with a platform device?
--
viresh
Dear Viresh Kumar,
Quoted 4 lines · Viresh Kumar
Quoted 1 line · Rafael J. Wysocki
I don't like that idea, but I wonder what other people think.
Hmm, the other thread around looking at the bindings is really slow.
Could you summarize what is the issue with the binding?
At least for the case where we have one clock per CPU, the DT binding is really dead simple: each CPU node can carry a "clocks" property, and a "clock-latency" property. I really don't see why a long discussion is needed to agree on such a binding.
Now, if the DT binding problem is related to those cases where you have siblings, i.e one clock controlling *some* of the CPUs, but not all CPUs or just one CPU, then maybe we could leave this aside for now, only support the following cases:
* One clock for all CPUs
* One clock for each CPU
Thanks,
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Viresh Kumar
maintainerQuoted 10 lines · Thomas Petazzoni
Could you summarize what is the issue with the binding?
At least for the case where we have one clock per CPU, the DT binding is really dead simple: each CPU node can carry a "clocks" property, and a "clock-latency" property. I really don't see why a long discussion is needed to agree on such a binding.
Now, if the DT binding problem is related to those cases where you have siblings, i.e one clock controlling *some* of the CPUs, but not all CPUs or just one CPU, then maybe we could leave this aside for now,
Yeah, we are stuck on that for now.
Quoted 4 lines
only support the following cases:
* One clock for all CPUs
* One clock for each CPU
Yeah, so I also proposed this yesterday that we stick to only these two implementations for now. And was looking at how would the cpufreq-generic driver come to know about this.
So, one way out now is to see if "clocks" property is defined in multiple cpu nodes, if yes don't compare them and consider separate clocks for each cpu. We don't have to try matching that to any other node, as that's a very bad idea. Mike was already very upset with that :)
@Stephen/Rafael: Does that sound any better? Ofcourse the final thing is to get bindings to figure out relations between CPUs..
Quoted 24 lines · Viresh Kumar
Quoted 10 lines · Thomas Petazzoni
Could you summarize what is the issue with the binding?
At least for the case where we have one clock per CPU, the DT binding is really dead simple: each CPU node can carry a "clocks" property, and a "clock-latency" property. I really don't see why a long discussion is needed to agree on such a binding.
Now, if the DT binding problem is related to those cases where you have siblings, i.e one clock controlling *some* of the CPUs, but not all CPUs or just one CPU, then maybe we could leave this aside for now,
Yeah, we are stuck on that for now.
Quoted 4 lines
only support the following cases:
* One clock for all CPUs
* One clock for each CPUYeah, so I also proposed this yesterday that we stick to only these two implementations for now. And was looking at how would the cpufreq-generic driver come to know about this.
Quoted 7 lines
So, one way out now is to see if "clocks" property is defined in multiple cpu nodes, if yes don't compare them and consider separate clocks for each cpu. We don't have to try matching that to any other node, as that's a very bad idea. Mike was already very upset with that :)
@Stephen/Rafael: Does that sound any better? Ofcourse the final thing is to get bindings to figure out relations between CPUs..
Before I apply anything in this area, I need a clear statement from the ARM people as a group on what the approach is going to be.
Rafael
Viresh Kumar
maintainerQuoted 14 lines · Rafael J. Wysocki
Quoted 14 lines
Quoted 4 lines
only support the following cases:
* One clock for all CPUs
* One clock for each CPUYeah, so I also proposed this yesterday that we stick to only these two implementations for now. And was looking at how would the cpufreq-generic driver come to know about this.
So, one way out now is to see if "clocks" property is defined in multiple cpu nodes, if yes don't compare them and consider separate clocks for each cpu. We don't have to try matching that to any other node, as that's a very bad idea. Mike was already very upset with that :)
Quoted 5 lines
Quoted 2 lines
@Stephen/Rafael: Does that sound any better? Ofcourse the final thing is to get bindings to figure out relations between CPUs..
Before I apply anything in this area, I need a clear statement from the ARM people as a group on what the approach is going to be.
Thanks for your response Rafael.
Mike/Rob/Stephen: I believe Atleast three of you should express your views now :)
So, this is what I propose:
- I will start another thread with a new DT binding, something like:
"clocks-ganged" = <&cpu0>
and then we can decide on naming/etc ..
- I will drop the patch which matches clock nodes from DT and introduce another one that will just check if "clocks" is mentioned in more than one CPU. If yes, then we behave as if all CPUs have separate clock lines.
That will work for Krait/mvebu and all existing users.
Does that sound good?
--
viresh
Viresh Kumar
maintainerQuoted 2 lines · Viresh Kumar
Quoted 2 lines
Before I apply anything in this area, I need a clear statement from the ARM people as a group on what the approach is going to be.
@Rafael: The only patch which has blocked this set is:
cpufreq: cpu0: Extend support beyond CPU0
This is about finding which CPUs share clock line..
Other patches are sort of independent of this one and cpufreq-cpu0 would continue to work for existing platforms if we apply these patches.
I was wondering if you would like to apply other patches leaving this one? I will then rebase stuff again and send non-controversial patches for 3.17. So, that we get something in 3.17 and the last change can be merged during rc's as well once we finalize on bindings..
Quoted 2 lines
Mike/Rob/Stephen: I believe Atleast three of you should express your views now :)
Any idea on how can we get some temporary solution for 3.17 as we didn't conclude anything yet on bindings ?
--
viresh
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 18 lines · Viresh Kumar
Quoted 2 lines · Viresh Kumar
Quoted 2 lines
Before I apply anything in this area, I need a clear statement from the ARM people as a group on what the approach is going to be.
@Rafael: The only patch which has blocked this set is:
cpufreq: cpu0: Extend support beyond CPU0
This is about finding which CPUs share clock line..
Other patches are sort of independent of this one and cpufreq-cpu0 would continue to work for existing platforms if we apply these patches.
I was wondering if you would like to apply other patches leaving this one? I will then rebase stuff again and send non-controversial patches for 3.17. So, that we get something in 3.17 and the last change can be merged during rc's as well once we finalize on bindings..
Quoted 5 lines
Quoted 2 lines
Mike/Rob/Stephen: I believe Atleast three of you should express your views now :)
Any idea on how can we get some temporary solution for 3.17 as we didn't conclude anything yet on bindings ?
A temporary solution would have to be NOT in DT because once you add something into DT you are stuck with it for some time. You could simply support independent clocks by looking at the platform type, but that is still risky since you may want to define the OPP in DT differently for the 2 cases.
The other problem with temporary solutions is once they are accepted, people loose motivation to create the permanent solution. "Can you take this now and I'll fix the issues later" is a red flag to maintainers.
Rob
Viresh Kumar
maintainerQuoted 2 lines · Rob Herring
A temporary solution would have to be NOT in DT because once you add something into DT you are stuck with it for some time. You could
I agree..
Quoted 3 lines
simply support independent clocks by looking at the platform type, but that is still risky since you may want to define the OPP in DT differently for the 2 cases.
Or because we are going to create platform devices for now, lets have some platform data for cpufreq-cpu0 ?
Quoted 4 lines
The other problem with temporary solutions is once they are accepted, people loose motivation to create the permanent solution. "Can you take this now and I'll fix the issues later" is a red flag to maintainers.
I completely agree, but there are few points here which might make the red flag yellow if not green :)
- I co-maintain this framework and so I do care about it :)
- Even with the temporary stuff the solution wouldn't be complete for platforms with multiple clusters having separate clock lines.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello,
Quoted 9 lines · Rob Herring
Quoted 2 lines
Any idea on how can we get some temporary solution for 3.17 as we didn't conclude anything yet on bindings ?
A temporary solution would have to be NOT in DT because once you add something into DT you are stuck with it for some time. You could simply support independent clocks by looking at the platform type, but that is still risky since you may want to define the OPP in DT differently for the 2 cases.
Quoted 4 lines
The other problem with temporary solutions is once they are accepted, people loose motivation to create the permanent solution. "Can you take this now and I'll fix the issues later" is a red flag to maintainers.
On the Marvell Armada XP side of things, the OPPs are registered dynamically because they change from one board to the other. The only thing that is in the DT for cpufreq-generic is the clock-latency parameter.
Thanks,
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com