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
Cc: devicetree@vger.kernel.org Cc: Kukjin Kim kgene.kim@samsung.com Cc: Michal Simek michal.simek@xilinx.com Cc: Mike Turquette mturquette@linaro.org Cc: Rob Herring rob.herring@linaro.org Cc: Santosh Shilimkar santosh.shilimkar@ti.com Cc: Simon Horman horms@verge.net.au
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
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.
Cc: Rob Herring rob.herring@linaro.org Cc: devicetree@vger.kernel.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/of/base.c | 29 +++++++++++++++++++++++++++++ include/linux/of.h | 10 ++++++++++ 2 files changed, 39 insertions(+)
diff --git a/drivers/of/base.c b/drivers/of/base.c index b986480..a036c91 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -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; diff --git a/include/linux/of.h b/include/linux/of.h index 196b34c..4e9cf5a 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -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)
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 mturquette@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/clk/clk.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 6 ++++++ 2 files changed, 62 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 8b73ede..497735c 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -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; diff --git a/include/linux/clk.h b/include/linux/clk.h index fb5e097..58e281a 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -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
On 07/01/14 09:32, Viresh Kumar wrote:
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 mturquette@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
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.
On 1 July 2014 23:30, Stephen Boyd sboyd@codeaurora.org wrote:
On 07/01/14 09:32, Viresh Kumar wrote:
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 mturquette@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
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.
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.
+ Lorenzo
On Tuesday 01 July 2014 12:32 PM, Viresh Kumar wrote:
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 mturquette@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/clk/clk.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 6 ++++++ 2 files changed, 62 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 8b73ede..497735c 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -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)
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
On 9 July 2014 20:08, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
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.
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 viresh.kumar@linaro.org --- include/linux/cpufreq.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index ec4112d..d4b1108 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -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 */
On Tuesday 01 July 2014 12:32 PM, Viresh Kumar wrote:
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 viresh.kumar@linaro.org
include/linux/cpufreq.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index ec4112d..d4b1108 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -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
On 9 July 2014 20:03, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index ec4112d..d4b1108 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -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.
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 :)
Two people are maintaining it now, Viresh and Shawn. Add Viresh's details in MODULE_AUTHOR() and copyright section.
Suggested-by: Shawn Guo shawn.guo@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-cpu0.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index ee1ae30..7e191db 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -1,6 +1,9 @@ /* * Copyright (C) 2012 Freescale Semiconductor, Inc. * + * Copyright (C) 2014 Linaro. + * Viresh Kumar viresh.kumar@linaro.org + * * 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 viresh.kumar@linaro.org"); MODULE_AUTHOR("Shawn Guo shawn.guo@linaro.org"); MODULE_DESCRIPTION("Generic CPU0 cpufreq driver"); MODULE_LICENSE("GPL");
On Tuesday 01 July 2014 12:32 PM, Viresh Kumar wrote:
Two people are maintaining it now, Viresh and Shawn. Add Viresh's details in MODULE_AUTHOR() and copyright section.
Suggested-by: Shawn Guo shawn.guo@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq-cpu0.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index ee1ae30..7e191db 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c
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
On 9 July 2014 20:12, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
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.
On Wednesday 09 July 2014 11:08 AM, Viresh Kumar wrote:
On 9 July 2014 20:12, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
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.
Sounds good :)
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 viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-cpu0.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 7e191db..4273a5f 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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);
On Tuesday 01 July 2014 12:32 PM, Viresh Kumar wrote:
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 viresh.kumar@linaro.org
Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com
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 viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-cpu0.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 4273a5f..b5b8e1c 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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; }
On Tue, Jul 01, 2014 at 10:02:35PM +0530, Viresh Kumar wrote:
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 viresh.kumar@linaro.org
drivers/cpufreq/cpufreq-cpu0.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 4273a5f..b5b8e1c 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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 ...
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 ...
dev_err(cpu_dev, "cpu0 clock not ready, retry\n");ret = -EPROBE_DEFER;
... this can be saved ...
} else {ret = PTR_ERR(cpu_clk);
... and this as well.
Shawn
dev_err(cpu_dev, "failed to get cpu0 clock: %d\n", ret);}2.0.0.rc2
On 2 July 2014 11:23, Shawn Guo shawn.guo@linaro.org wrote:
On Tue, Jul 01, 2014 at 10:02:35PM +0530, Viresh Kumar wrote:
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 4273a5f..b5b8e1c 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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 ...
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 ...
dev_err(cpu_dev, "cpu0 clock not ready, retry\n");ret = -EPROBE_DEFER;... this can be saved ...
} else {ret = PTR_ERR(cpu_clk);... and this as well.
All accepted. Thanks.
On 2 July 2014 11:25, Viresh Kumar viresh.kumar@linaro.org wrote:
On 2 July 2014 11:23, Shawn Guo shawn.guo@linaro.org wrote:
On Tue, Jul 01, 2014 at 10:02:35PM +0530, Viresh Kumar wrote:
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 4273a5f..b5b8e1c 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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 ...
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 ...
dev_err(cpu_dev, "cpu0 clock not ready, retry\n");ret = -EPROBE_DEFER;... this can be saved ...
} 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.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-cpu0.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 4273a5f..0c16b2f 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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; }
On 07/02/14 04:32, Viresh Kumar wrote:
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 4273a5f..0c16b2f 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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");
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.
On 3 July 2014 06:08, Stephen Boyd sboyd@codeaurora.org wrote:
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 viresh.kumar@linaro.org 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.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-cpu0.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 4273a5f..8a1166c 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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; }
On Wednesday 02 July 2014 10:19 PM, Viresh Kumar wrote:
On 3 July 2014 06:08, Stephen Boyd sboyd@codeaurora.org wrote:
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 viresh.kumar@linaro.org 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. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
drivers/cpufreq/cpufreq-cpu0.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
Looks good to me. Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com
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.
To fix this, remove error checking and call dev_pm_opp_init_cpufreq_table() unconditionally.
Suggested-by: Stephen Boyd sboyd@codeaurora.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-cpu0.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index b5b8e1c..f47f703 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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) {
On 07/01/14 09:32, Viresh Kumar wrote:
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.
To fix this, remove error checking and call dev_pm_opp_init_cpufreq_table() unconditionally.
Suggested-by: Stephen Boyd sboyd@codeaurora.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
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.
On 1 July 2014 23:32, Stephen Boyd sboyd@codeaurora.org wrote:
Please update the binding as well to indicate that this property is now optional.
Does this look fine..
diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt index f055515..366690c 100644 --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt @@ -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.
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.
To fix this, remove error checking and call dev_pm_opp_init_cpufreq_table() unconditionally.
Update bindings as well.
Suggested-by: Stephen Boyd sboyd@codeaurora.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- 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(-)
diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt index f055515..366690c 100644 --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt @@ -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. diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index b5b8e1c..f47f703 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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) {
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().
Cc: devicetree@vger.kernel.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- 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(-)
diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt index 366690c..9d65799 100644 --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt @@ -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>; }; }; diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 44633f6..0f2fe76 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -177,6 +177,57 @@ 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) +{ + struct private_data *priv = policy->driver_data; + struct device *cpu1_dev = priv->cpu_dev, *cpu2_dev; + struct device_node *np1, *np2; + int cpu, ret, set_all = 1; + + np1 = of_node_get(cpu1_dev->of_node); + + for_each_possible_cpu(cpu) { + if (cpu == policy->cpu) + continue; + + cpu2_dev = get_cpu_device(cpu); + if (!cpu2_dev) { + dev_err(cpu1_dev, "%s: failed to cpu_dev for cpu%d\n", + __func__, cpu); + goto out_set_all; + } + + np2 = of_node_get(cpu2_dev->of_node); + if (!np2) { + dev_err(cpu1_dev, "failed to find cpu%d node\n", cpu); + goto out_set_all; + } + + ret = of_property_match(np1, np2, "clocks"); + of_node_put(np2); + + /* Error while parsing nodes, fallback to set-all */ + if (ret < 0) + goto out_set_all; + else if (ret == 1) + cpumask_set_cpu(cpu, policy->cpus); + } + + /* All processors don't share clock and voltage */ + set_all = 0; + +out_set_all: + /* All processors share clock and voltage */ + if (set_all) + cpumask_setall(policy->cpus); + + of_node_put(np1); +} + static int cpu0_cpufreq_init(struct cpufreq_policy *policy) { struct cpufreq_frequency_table *freq_table; @@ -266,9 +317,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;
On Tuesday 01 July 2014 12:32 PM, Viresh Kumar wrote:
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.
To fix this, remove error checking and call dev_pm_opp_init_cpufreq_table() unconditionally.
Suggested-by: Stephen Boyd sboyd@codeaurora.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Assuming you are updating bidnings as suggested by Stephen, patch looks good to me. Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com
On 9 July 2014 20:14, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
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.
Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com
Thanks.
On 9 July 2014 20:14, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
Assuming you are updating bidnings as suggested by Stephen, patch looks good to me. Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com
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?
On Thu, Jul 10, 2014 at 6:19 AM, Viresh Kumar viresh.kumar@linaro.org wrote:
On 9 July 2014 20:14, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
Assuming you are updating bidnings as suggested by Stephen, patch looks good to me. Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com
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?
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
On Thursday 10 July 2014 08:39 AM, Nishanth Menon wrote:
On Thu, Jul 10, 2014 at 6:19 AM, Viresh Kumar viresh.kumar@linaro.org wrote:
On 9 July 2014 20:14, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
Assuming you are updating bidnings as suggested by Stephen, patch looks good to me. Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com
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?
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.
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
On 10 July 2014 19:01, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
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.
We 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.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-cpu0.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index f47f703..2b7b0ea 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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);
On Tuesday 01 July 2014 12:32 PM, Viresh Kumar wrote:
We 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.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq-cpu0.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
Looks good to me. Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com
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 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.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-cpu0.c | 189 +++++++++++++++++++++++++++++------------ 1 file changed, 136 insertions(+), 53 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 2b7b0ea..15b8e7a 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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; }
On 07/01/14 09:32, Viresh Kumar wrote:
+static int cpu0_cpufreq_init(struct cpufreq_policy *policy) +{
- struct cpufreq_frequency_table *freq_table;
- struct thermal_cooling_device *cdev;
This..
- struct device_node *np;
- struct private_data *priv;
[...]
ing 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;
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?
On 3 July 2014 06:13, Stephen Boyd sboyd@codeaurora.org wrote:
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:
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index e6dc8ea..05a18bd 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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;
On Tuesday 01 July 2014 12:32 PM, Viresh Kumar wrote:
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 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.
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.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
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
On 9 July 2014 20:23, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
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 ..
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
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:
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.
On Wednesday 09 July 2014 11:17 AM, Viresh Kumar wrote:
On 9 July 2014 20:23, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
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 ..
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
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:
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
On 9 July 2014 20:56, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
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 ?
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
On Wednesday 09 July 2014 11:27 AM, Viresh Kumar wrote:
On 9 July 2014 20:56, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
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
On 9 July 2014 20:59, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
On Wednesday 09 July 2014 11:27 AM, Viresh Kumar wrote:
On 9 July 2014 20:56, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
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 :)
Currently, 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".
Suggested-by: Stephen Boyd sboyd@codeaurora.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-cpu0.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 15b8e7a..17001a8 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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)); }
Currently 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.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-cpu0.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 17001a8..44633f6 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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;
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_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().
Cc: devicetree@vger.kernel.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt | 72 ++++++++++++++++++++-- drivers/cpufreq/cpufreq-cpu0.c | 35 ++++++++++- 2 files changed, 101 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt index f055515..4b83c1a 100644 --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt @@ -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>; }; }; diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 44633f6..b3f2bf0 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -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;
This 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.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- .../{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%)
diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-generic.txt similarity index 100% rename from Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt rename to Documentation/devicetree/bindings/cpufreq/cpufreq-generic.txt diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index ffe350f..22b42d5 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -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.
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index ebac671..16bdd31 100644 --- a/drivers/cpufreq/Kconfig.arm +++ b/drivers/cpufreq/Kconfig.arm @@ -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 diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile index 738c8b7..0d0cddc 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -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. diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-generic.c similarity index 91% rename from drivers/cpufreq/cpufreq-cpu0.c rename to drivers/cpufreq/cpufreq-generic.c index b3f2bf0..ac8fd9f 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-generic.c @@ -4,7 +4,7 @@ * Copyright (C) 2014 Linaro. * Viresh Kumar viresh.kumar@linaro.org * - * 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 viresh.kumar@linaro.org"); MODULE_AUTHOR("Shawn Guo shawn.guo@linaro.org"); -MODULE_DESCRIPTION("Generic CPU0 cpufreq driver"); +MODULE_DESCRIPTION("Generic cpufreq driver"); MODULE_LICENSE("GPL");
We 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.
Cc: Shawn Guo shawn.guo@freescale.com Cc: Santosh Shilimkar santosh.shilimkar@ti.com Cc: Simon Horman horms@verge.net.au Cc: Michal Simek michal.simek@xilinx.com Cc: Kukjin Kim kgene.kim@samsung.com Cc: Rob Herring rob.herring@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- @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(-)
diff --git a/arch/arm/mach-imx/imx27-dt.c b/arch/arm/mach-imx/imx27-dt.c index 17bd405..02376e1 100644 --- a/arch/arm/mach-imx/imx27-dt.c +++ b/arch/arm/mach-imx/imx27-dt.c @@ -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();
diff --git a/arch/arm/mach-imx/imx51-dt.c b/arch/arm/mach-imx/imx51-dt.c index b8cd968..66d415a 100644 --- a/arch/arm/mach-imx/imx51-dt.c +++ b/arch/arm/mach-imx/imx51-dt.c @@ -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();
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c index 828aee9..7ec2fbb 100644 --- a/arch/arm/mach-omap2/pm.c +++ b/arch/arm/mach-omap2/pm.c @@ -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); }
diff --git a/arch/arm/mach-shmobile/board-ape6evm-reference.c b/arch/arm/mach-shmobile/board-ape6evm-reference.c index 3276afc..94eb59d 100644 --- a/arch/arm/mach-shmobile/board-ape6evm-reference.c +++ b/arch/arm/mach-shmobile/board-ape6evm-reference.c @@ -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 = { diff --git a/arch/arm/mach-shmobile/setup-sh73a0.c b/arch/arm/mach-shmobile/setup-sh73a0.c index ad00724..360eace 100644 --- a/arch/arm/mach-shmobile/setup-sh73a0.c +++ b/arch/arm/mach-shmobile/setup-sh73a0.c @@ -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); }
diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c index 31a6fa4..79f3c61 100644 --- a/arch/arm/mach-zynq/common.c +++ b/arch/arm/mach-zynq/common.c @@ -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; diff --git a/drivers/cpufreq/cpufreq-generic.c b/drivers/cpufreq/cpufreq-generic.c index ac8fd9f..d47b4a3 100644 --- a/drivers/cpufreq/cpufreq-generic.c +++ b/drivers/cpufreq/cpufreq-generic.c @@ -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, diff --git a/drivers/cpufreq/exynos4x12-cpufreq.c b/drivers/cpufreq/exynos4x12-cpufreq.c index 351a207..7bbe5ef 100644 --- a/drivers/cpufreq/exynos4x12-cpufreq.c +++ b/drivers/cpufreq/exynos4x12-cpufreq.c @@ -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) { diff --git a/drivers/cpufreq/highbank-cpufreq.c b/drivers/cpufreq/highbank-cpufreq.c index bf8902a..dc7a9ab 100644 --- a/drivers/cpufreq/highbank-cpufreq.c +++ b/drivers/cpufreq/highbank-cpufreq.c @@ -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:
On 1 July 2014 22:02, Viresh Kumar viresh.kumar@linaro.org wrote:
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
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
On 07/01/14 21:12, Viresh Kumar wrote:
On 1 July 2014 22:02, Viresh Kumar viresh.kumar@linaro.org wrote:
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
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
I gave it a spin. It works so you can have my
Tested-by: Stephen Boyd sboyd@codeaurora.org
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; }
On 3 July 2014 06:54, Stephen Boyd sboyd@codeaurora.org wrote:
I gave it a spin. It works so you can have my
Tested-by: Stephen Boyd sboyd@codeaurora.org
Thanks, all suggested improvements are made and pushed again with your Tested-by..
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.
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;}
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)
On 3 July 2014 06:54, Stephen Boyd sboyd@codeaurora.org wrote:
I gave it a spin. It works so you can have my
Tested-by: Stephen Boyd sboyd@codeaurora.org
Thanks, all suggested improvements are made and pushed again with your Tested-by..
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.
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;}
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
@Mike: Is this less ugly ? :)
On 4 July 2014 03:46, Mike Turquette mturquette@linaro.org wrote:
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
On 4 July 2014 09:51, Viresh Kumar viresh.kumar@linaro.org wrote:
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
On 07/07/14 21:50, Viresh Kumar wrote:
On 4 July 2014 09:51, Viresh Kumar viresh.kumar@linaro.org wrote:
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?
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.
Cc'ing Thomas,
On 8 July 2014 10:20, Viresh Kumar viresh.kumar@linaro.org wrote:
On 4 July 2014 09:51, Viresh Kumar viresh.kumar@linaro.org wrote:
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.
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,
On Wed, 16 Jul 2014 21:31:54 +0530, Viresh Kumar wrote:
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).
On 8 July 2014 10:20, Viresh Kumar viresh.kumar@linaro.org wrote:
On 4 July 2014 09:51, Viresh Kumar viresh.kumar@linaro.org wrote:
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.
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.
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
On Wednesday, July 16, 2014 07:28:15 PM Thomas Petazzoni wrote:
Dear Viresh Kumar,
On Wed, 16 Jul 2014 21:31:54 +0530, Viresh Kumar wrote:
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).
On 8 July 2014 10:20, Viresh Kumar viresh.kumar@linaro.org wrote:
On 4 July 2014 09:51, Viresh Kumar viresh.kumar@linaro.org wrote:
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.
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.
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.
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
On Wednesday, July 16, 2014 09:31:54 PM Viresh Kumar wrote:
Cc'ing Thomas,
On 8 July 2014 10:20, Viresh Kumar viresh.kumar@linaro.org wrote:
On 4 July 2014 09:51, Viresh Kumar viresh.kumar@linaro.org wrote:
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.
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 don't like that idea, but I wonder what other people think.
Rafael
On 17 July 2014 02:48, Rafael J. Wysocki rjw@rjwysocki.net wrote:
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,
On Thu, 17 Jul 2014 05:58:22 +0530, Viresh Kumar wrote:
On 17 July 2014 02:48, Rafael J. Wysocki rjw@rjwysocki.net wrote:
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
On 17 July 2014 13:05, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
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.
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..
On Thursday, July 17, 2014 01:11:45 PM Viresh Kumar wrote:
On 17 July 2014 13:05, Thomas Petazzoni thomas.petazzoni@free-electrons.com wrote:
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.
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..
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
On 18 July 2014 06:32, Rafael J. Wysocki rjw@rjwysocki.net wrote:
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..
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
On 18 July 2014 09:47, Viresh Kumar viresh.kumar@linaro.org wrote:
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..
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
On Thu, Jul 24, 2014 at 5:48 AM, Viresh Kumar viresh.kumar@linaro.org wrote:
On 18 July 2014 09:47, Viresh Kumar viresh.kumar@linaro.org wrote:
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..
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
On 25 July 2014 19:59, Rob Herring robherring2@gmail.com wrote:
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..
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 ?
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.
Hello,
On Fri, 25 Jul 2014 09:29:09 -0500, Rob Herring wrote:
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.
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
linaro-kernel@lists.linaro.org
-
Mike Turquette -
Nishanth Menon -
Rafael J. Wysocki -
Rob Herring -
Santosh Shilimkar -
Shawn Guo -
Stephen Boyd -
Thomas Petazzoni -
Viresh Kumar