2014-07-01 16:33:26

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

V1: https://lkml.org/lkml/2014/6/25/152

Stephen Boyd sent few patches some time back around a new cpufreq driver for
Qualcomm's Krait SoC: https://lkml.org/lkml/2014/6/24/918.

Krait couldn't use existing cpufreq-cpu0 driver as it doesn't have support for
SoC's with multiple clusters or SoC's which don't share clock line across all
CPUs.

This patchset is all about extending support beyond CPU0. It can be used for
platforms like Krait or ARM big LITTLE architecture now.

First two patches add helper routine for of and clk layer, which would be used
by later patches.

Third one adds space for driver specific data in 'struct cpufreq_policy' and
later ones migrate cpufreq-cpu0 to cpufreq-generic, i.e. can be used for SoCs
which don't share clock lines across all CPUs.

@Stephen: I haven't added your Tested-by as there were few modifications since
the time you tested it last.

Pushed here:
Rebased over v3.16-rc3:
git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/cpu0-krait-v2

For guys looking to test on exynos, rebased over linux-next + some patches from
Thomas Abraham to use cpufreq-cpu0 for exynos:
git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/cpu0-exynos-v2

Cc: [email protected]
Cc: Kukjin Kim <[email protected]>
Cc: Michal Simek <[email protected]>
Cc: Mike Turquette <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Santosh Shilimkar <[email protected]>
Cc: Simon Horman <[email protected]>

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

--
2.0.0.rc2


2014-07-01 16:33:37

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 01/14] of: Create of_property_match()

Create a new routine of_property_match() that will match a property between two
nodes. If they are exactly same, it returns 1 else 0. If the property isn't
available in any of the nodes or the API isn't implemented, proper error codes
would be returned.

The first user of this routine would be cpufreq-cpu0 driver, which requires to
match "clock" property between CPU nodes to identify which CPUs share clock
line. And hence this routine.

Cc: Rob Herring <[email protected]>
Cc: [email protected]
Signed-off-by: Viresh Kumar <[email protected]>
---
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)
--
2.0.0.rc2

2014-07-01 16:33:48

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 02/14] clk: Create of_clk_shared_by_cpus()

Create a new routine of_clk_shared_by_cpus() that finds if clock lines are
shared between two CPUs. This is verified by comparing "clocks" property from
CPU's DT node.

Returns 1 if clock line is shared between them, 0 if clock isn't shared and
return appropriate errors in case nodes/properties are missing.

Cc: Mike Turquette <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/clk/clk.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/clk.h | 6 ++++++
2 files changed, 62 insertions(+)

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
--
2.0.0.rc2

2014-07-01 16:33:58

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 03/14] cpufreq: Add support for per-policy driver data

Drivers supporting multiple clusters or multiple 'struct cpufreq_policy'
instances may need to keep per-policy data. If the core doesn't support them,
they might do it in the most unoptimized way: 'per-cpu' data.

This patch adds another field in 'struct cpufreq_policy': 'driver_data'. It
isn't accessed by core and is for driver's internal use.

Signed-off-by: Viresh Kumar <[email protected]>
---
include/linux/cpufreq.h | 3 +++
1 file changed, 3 insertions(+)

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 */
--
2.0.0.rc2

2014-07-01 16:34:08

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 04/14] cpufreq: cpu0: Add Module Author

Two people are maintaining it now, Viresh and Shawn. Add Viresh's details in
MODULE_AUTHOR() and copyright section.

Suggested-by: Shawn Guo <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/cpufreq/cpufreq-cpu0.c | 4 ++++
1 file changed, 4 insertions(+)

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 <[email protected]>
+ *
* The OPP code in function cpu0_set_target() is reused from
* drivers/cpufreq/omap-cpufreq.c
*
@@ -246,6 +249,7 @@ static struct platform_driver cpu0_cpufreq_platdrv = {
};
module_platform_driver(cpu0_cpufreq_platdrv);

+MODULE_AUTHOR("Viresh Kumar <[email protected]>");
MODULE_AUTHOR("Shawn Guo <[email protected]>");
MODULE_DESCRIPTION("Generic CPU0 cpufreq driver");
MODULE_LICENSE("GPL");
--
2.0.0.rc2

2014-07-01 16:34:18

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 05/14] cpufreq: cpu0: don't validate clock on clk_put()

CPU clk is not optional for this driver and probe would fail if it couldn't find
a suitable clock.

And so, while calling clk_put() we don't need to validate clocks.

Signed-off-by: Viresh Kumar <[email protected]>
---
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);
--
2.0.0.rc2

2014-07-01 16:34:27

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 06/14] cpufreq: cpu0: defer probe if clock isn't registered yet

Currently, we defer probe if regulator_get() returned -EPROBE_DEFER, i.e.
regulator isn't registered yet.

The same is true for clock as well, so lets defer in that case as well.

Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/cpufreq/cpufreq-cpu0.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

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

--
2.0.0.rc2

2014-07-01 16:34:38

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime

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 <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
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) {
--
2.0.0.rc2

2014-07-01 16:34:48

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 08/14] cpufreq: cpu0: use dev_{err|warn|dbg} instead of pr_{err|warn|debug}

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 <[email protected]>
---
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);
--
2.0.0.rc2

2014-07-01 16:34:57

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()

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 <[email protected]>
---
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;
}

--
2.0.0.rc2

2014-07-01 16:35:08

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 10/14] cpufreq: cpu0: try regulators with name "cpu-supply"

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 <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
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));
}
--
2.0.0.rc2

2014-07-01 16:35:21

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 11/14] cpufreq: cpu0: Make allocate_resources() work for any CPU

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 <[email protected]>
---
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;

--
2.0.0.rc2

2014-07-01 16:35:31

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 12/14] cpufreq: cpu0: Extend support beyond CPU0

Most of the infrastructure is in place now, with only little left. How to find
siblings ?

Stephen Boyd suggested to compare "clocks" properties from CPU's DT node and
siblings should match. This patch adds another routine find_siblings() which
calls of_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: [email protected]
Signed-off-by: Viresh Kumar <[email protected]>
---
.../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;

--
2.0.0.rc2

2014-07-01 16:35:39

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 13/14] cpufreq: cpu0: rename driver and internals to 'cpufreq_generic'

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 <[email protected]>
---
.../{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 <[email protected]>
*
- * The OPP code in function cpu0_set_target() is reused from
+ * The OPP code in function set_target() is reused from
* drivers/cpufreq/omap-cpufreq.c
*
* This program is free software; you can redistribute it and/or modify
@@ -35,7 +35,7 @@ struct private_data {
unsigned int voltage_tolerance; /* in percentage */
};

-static int cpu0_set_target(struct cpufreq_policy *policy, unsigned int index)
+static int set_target(struct cpufreq_policy *policy, unsigned int index)
{
struct dev_pm_opp *opp;
struct cpufreq_frequency_table *freq_table = policy->freq_table;
@@ -201,7 +201,7 @@ static void find_siblings(struct cpufreq_policy *policy)
}
}

-static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
+static int cpufreq_init(struct cpufreq_policy *policy)
{
struct cpufreq_frequency_table *freq_table;
struct thermal_cooling_device *cdev;
@@ -318,7 +318,7 @@ out_put_reg_clk:
return ret;
}

-static int cpu0_cpufreq_exit(struct cpufreq_policy *policy)
+static int cpufreq_exit(struct cpufreq_policy *policy)
{
struct private_data *priv = policy->driver_data;

@@ -332,18 +332,18 @@ static int cpu0_cpufreq_exit(struct cpufreq_policy *policy)
return 0;
}

-static struct cpufreq_driver cpu0_cpufreq_driver = {
+static struct cpufreq_driver generic_cpufreq_driver = {
.flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK,
.verify = cpufreq_generic_frequency_table_verify,
- .target_index = cpu0_set_target,
+ .target_index = set_target,
.get = cpufreq_generic_get,
- .init = cpu0_cpufreq_init,
- .exit = cpu0_cpufreq_exit,
- .name = "generic_cpu0",
+ .init = cpufreq_init,
+ .exit = cpufreq_exit,
+ .name = "cpufreq-generic",
.attr = cpufreq_generic_attr,
};

-static int cpu0_cpufreq_probe(struct platform_device *pdev)
+static int generic_cpufreq_probe(struct platform_device *pdev)
{
struct device *cpu_dev;
struct regulator *cpu_reg;
@@ -365,30 +365,30 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)
if (!IS_ERR(cpu_reg))
regulator_put(cpu_reg);

- ret = cpufreq_register_driver(&cpu0_cpufreq_driver);
+ ret = cpufreq_register_driver(&generic_cpufreq_driver);
if (ret)
dev_err(cpu_dev, "failed register driver: %d\n", ret);

return ret;
}

-static int cpu0_cpufreq_remove(struct platform_device *pdev)
+static int generic_cpufreq_remove(struct platform_device *pdev)
{
- cpufreq_unregister_driver(&cpu0_cpufreq_driver);
+ cpufreq_unregister_driver(&generic_cpufreq_driver);
return 0;
}

-static struct platform_driver cpu0_cpufreq_platdrv = {
+static struct platform_driver generic_cpufreq_platdrv = {
.driver = {
.name = "cpufreq-cpu0",
.owner = THIS_MODULE,
},
- .probe = cpu0_cpufreq_probe,
- .remove = cpu0_cpufreq_remove,
+ .probe = generic_cpufreq_probe,
+ .remove = generic_cpufreq_remove,
};
-module_platform_driver(cpu0_cpufreq_platdrv);
+module_platform_driver(generic_cpufreq_platdrv);

MODULE_AUTHOR("Viresh Kumar <[email protected]>");
MODULE_AUTHOR("Shawn Guo <[email protected]>");
-MODULE_DESCRIPTION("Generic CPU0 cpufreq driver");
+MODULE_DESCRIPTION("Generic cpufreq driver");
MODULE_LICENSE("GPL");
--
2.0.0.rc2

2014-07-01 16:35:51

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 14/14] cpufreq: generic: set platform_{driver|device} '.name' to 'cpufreq-generic'

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 <[email protected]>
Cc: Santosh Shilimkar <[email protected]>
Cc: Simon Horman <[email protected]>
Cc: Michal Simek <[email protected]>
Cc: Kukjin Kim <[email protected]>
Cc: Rob Herring <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
@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:
--
2.0.0.rc2

2014-07-01 18:00:37

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 02/14] clk: Create of_clk_shared_by_cpus()

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 <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> drivers/clk/clk.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/clk.h | 6 ++++++

This doesn't make much sense to me. This function doesn't deal with
struct clk pointers or any of the internals of the common clock
framework so why put it in clk.c? It looks more like an internal
function that the cpufreq-generic driver should have.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

2014-07-01 18:02:51

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime

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 <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> drivers/cpufreq/cpufreq-cpu0.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)

Please update the binding as well to indicate that this property is now
optional.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

2014-07-02 01:57:40

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 02/14] clk: Create of_clk_shared_by_cpus()

On 1 July 2014 23:30, Stephen Boyd <[email protected]> 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 <[email protected]>
>> Signed-off-by: Viresh Kumar <[email protected]>
>> ---
>> drivers/clk/clk.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/clk.h | 6 ++++++
>
> This doesn't make much sense to me. This function doesn't deal with
> struct clk pointers or any of the internals of the common clock
> framework so why put it in clk.c? It looks more like an internal
> function that the cpufreq-generic driver should have.

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.

2014-07-02 02:03:04

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime

On 1 July 2014 23:32, Stephen Boyd <[email protected]> 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.

2014-07-02 04:03:42

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH V2 Resend 07/14] cpufreq: cpu0: OPPs can be populated at runtime

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 <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
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) {
--
2.0.0.rc2

2014-07-02 04:03:51

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH V2 Resend 12/14] cpufreq: cpu0: Extend support beyond CPU0

Most of the infrastructure is in place now, with only little left. How to find
siblings ?

Stephen Boyd suggested to compare "clocks" properties from CPU's DT node and
siblings should match. This patch adds another routine find_siblings() which
calls of_property_match() to find if CPUs share clock line or not.

If of_property_match() returns error, we fallback to all CPUs sharing clock line
assumption as existing platforms don't have "clocks" property in all CPU nodes
and would fail from of_property_match().

Cc: [email protected]
Signed-off-by: Viresh Kumar <[email protected]>
---
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;

--
2.0.0.rc2

2014-07-02 04:12:58

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On 1 July 2014 22:02, Viresh Kumar <[email protected]> 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

2014-07-02 05:55:40

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 06/14] cpufreq: cpu0: defer probe if clock isn't registered yet

On 2 July 2014 11:23, Shawn Guo <[email protected]> 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.

2014-07-02 06:08:21

by Shawn Guo

[permalink] [raw]
Subject: Re: [PATCH 06/14] cpufreq: cpu0: defer probe if clock isn't registered yet

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 <[email protected]>
> ---
> 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);
> + }
> goto out_put_reg;
> }
>
> --
> 2.0.0.rc2
>

2014-07-02 11:32:17

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 06/14] cpufreq: cpu0: defer probe if clock isn't registered yet

On 2 July 2014 11:25, Viresh Kumar <[email protected]> wrote:
> On 2 July 2014 11:23, Shawn Guo <[email protected]> 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 <[email protected]>
---
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;
}

2014-07-03 00:38:35

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 06/14] cpufreq: cpu0: defer probe if clock isn't registered yet

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.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

2014-07-03 00:43:09

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()

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?

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

2014-07-03 01:24:08

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On 07/01/14 21:12, Viresh Kumar wrote:
> On 1 July 2014 22:02, Viresh Kumar <[email protected]> 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 <[email protected]>

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


--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

2014-07-03 02:11:37

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()

On 3 July 2014 06:13, Stephen Boyd <[email protected]> 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;

2014-07-03 02:19:32

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 06/14] cpufreq: cpu0: defer probe if clock isn't registered yet

On 3 July 2014 06:08, Stephen Boyd <[email protected]> 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 <[email protected]>
Date: Thu Jun 26 10:40:21 2014 +0530

cpufreq: cpu0: print relevant error when we defer probe

Currently, we defer probe if regulator_get() returned -EPROBE_DEFER, i.e.
regulator isn't registered yet. We do a dev_err() in this case. Sending a
message to the log on probe defer just duplicates what the driver core is
already doing. Convert it to dev_dbg() instead.

We should defer in case of clk_get() as well.

Current code already does it, but it wasn't intentional probably.
Its just that
we are returning the right error with wrong print message.

Fix print message to convey right error.

Signed-off-by: Viresh Kumar <[email protected]>
---
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;
}

2014-07-03 02:44:08

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On 3 July 2014 06:54, Stephen Boyd <[email protected]> wrote:
> I gave it a spin. It works so you can have my
>
> Tested-by: Stephen Boyd <[email protected]>

Thanks, all suggested improvements are made and pushed again with
your Tested-by..

> 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 ? :)

2014-07-03 22:16:26

by Mike Turquette

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

Quoting Viresh Kumar (2014-07-02 19:44:04)
> On 3 July 2014 06:54, Stephen Boyd <[email protected]> wrote:
> > I gave it a spin. It works so you can have my
> >
> > Tested-by: Stephen Boyd <[email protected]>
>
> Thanks, all suggested improvements are made and pushed again with
> your Tested-by..
>
> > 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 ? :)

2014-07-04 04:21:42

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On 4 July 2014 03:46, Mike Turquette <[email protected]> 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

2014-07-08 04:50:36

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On 4 July 2014 09:51, Viresh Kumar <[email protected]> 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

2014-07-09 14:33:38

by Santosh Shilimkar

[permalink] [raw]
Subject: Re: [PATCH 03/14] cpufreq: Add support for per-policy driver data

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 <[email protected]>
> ---
> 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

2014-07-09 14:38:53

by Santosh Shilimkar

[permalink] [raw]
Subject: Re: [PATCH 02/14] clk: Create of_clk_shared_by_cpus()

+ 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 <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> 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



2014-07-09 14:42:32

by Santosh Shilimkar

[permalink] [raw]
Subject: Re: [PATCH 04/14] cpufreq: cpu0: Add Module Author

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 <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> 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

2014-07-09 14:43:09

by Santosh Shilimkar

[permalink] [raw]
Subject: Re: [PATCH 05/14] cpufreq: cpu0: don't validate clock on clk_put()

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 <[email protected]>
> ---
Acked-by: Santosh Shilimkar <[email protected]>

2014-07-09 14:44:15

by Santosh Shilimkar

[permalink] [raw]
Subject: Re: [PATCH 06/14] cpufreq: cpu0: defer probe if clock isn't registered yet

On Wednesday 02 July 2014 10:19 PM, Viresh Kumar wrote:
> On 3 July 2014 06:08, Stephen Boyd <[email protected]> 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 <[email protected]>
> Date: Thu Jun 26 10:40:21 2014 +0530
>
> cpufreq: cpu0: print relevant error when we defer probe
>
> Currently, we defer probe if regulator_get() returned -EPROBE_DEFER, i.e.
> regulator isn't registered yet. We do a dev_err() in this case. Sending a
> message to the log on probe defer just duplicates what the driver core is
> already doing. Convert it to dev_dbg() instead.
>
> We should defer in case of clk_get() as well.
>
> Current code already does it, but it wasn't intentional probably.
> Its just that
> we are returning the right error with wrong print message.
>
> Fix print message to convey right error.
>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> drivers/cpufreq/cpufreq-cpu0.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
Looks good to me.
Acked-by: Santosh Shilimkar <[email protected]>

2014-07-09 14:45:13

by Santosh Shilimkar

[permalink] [raw]
Subject: Re: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime

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 <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
Assuming you are updating bidnings as suggested by Stephen,
patch looks good to me.
Acked-by: Santosh Shilimkar <[email protected]>

2014-07-09 14:45:46

by Santosh Shilimkar

[permalink] [raw]
Subject: Re: [PATCH 08/14] cpufreq: cpu0: use dev_{err|warn|dbg} instead of pr_{err|warn|debug}

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 <[email protected]>
> ---
> drivers/cpufreq/cpufreq-cpu0.c | 32 ++++++++++++++++++--------------
> 1 file changed, 18 insertions(+), 14 deletions(-)
>
Looks good to me.
Acked-by: Santosh Shilimkar <[email protected]>

2014-07-09 14:54:16

by Santosh Shilimkar

[permalink] [raw]
Subject: Re: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()


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 <[email protected]>
> ---
Would you able to give some idea about what will it take to address that one
remainder case as well as part of this series.

Regards,
Santosh

2014-07-09 15:07:53

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 03/14] cpufreq: Add support for per-policy driver data

On 9 July 2014 20:03, Santosh Shilimkar <[email protected]> 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 :)

2014-07-09 15:08:34

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 04/14] cpufreq: cpu0: Add Module Author

On 9 July 2014 20:12, Santosh Shilimkar <[email protected]> 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.

2014-07-09 15:09:28

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime

On 9 July 2014 20:14, Santosh Shilimkar <[email protected]> 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 <[email protected]>

Thanks.

2014-07-09 15:17:19

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()

On 9 July 2014 20:23, Santosh Shilimkar <[email protected]> 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 <[email protected]>
>> ---
> Would you able to give some idea about what will it take to address that one
> remainder case as well as part of this series.

Which one? This:

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

2014-07-09 15:20:56

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 02/14] clk: Create of_clk_shared_by_cpus()

On 9 July 2014 20:08, Santosh Shilimkar <[email protected]> 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.

2014-07-09 15:25:38

by Santosh Shilimkar

[permalink] [raw]
Subject: Re: [PATCH 04/14] cpufreq: cpu0: Add Module Author

On Wednesday 09 July 2014 11:08 AM, Viresh Kumar wrote:
> On 9 July 2014 20:12, Santosh Shilimkar <[email protected]> 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 :)

2014-07-09 15:26:29

by Santosh Shilimkar

[permalink] [raw]
Subject: Re: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()

On Wednesday 09 July 2014 11:17 AM, Viresh Kumar wrote:
> On 9 July 2014 20:23, Santosh Shilimkar <[email protected]> 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 <[email protected]>
>>> ---
>> Would you able to give some idea about what will it take to address that one
>> remainder case as well as part of this series.
>
> Which one? This:
>
>> 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

2014-07-09 15:27:50

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()

On 9 July 2014 20:56, Santosh Shilimkar <[email protected]> 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

2014-07-09 15:30:11

by Santosh Shilimkar

[permalink] [raw]
Subject: Re: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()

On Wednesday 09 July 2014 11:27 AM, Viresh Kumar wrote:
> On 9 July 2014 20:56, Santosh Shilimkar <[email protected]> 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

2014-07-09 15:33:39

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 09/14] cpufreq: cpu0: Move per-cluster initialization code to ->init()

On 9 July 2014 20:59, Santosh Shilimkar <[email protected]> wrote:
> On Wednesday 09 July 2014 11:27 AM, Viresh Kumar wrote:
>> On 9 July 2014 20:56, Santosh Shilimkar <[email protected]> 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 :)

2014-07-09 17:41:40

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On 07/07/14 21:50, Viresh Kumar wrote:
> On 4 July 2014 09:51, Viresh Kumar <[email protected]> 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.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

2014-07-10 11:19:56

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime

On 9 July 2014 20:14, Santosh Shilimkar <[email protected]> wrote:
> Assuming you are updating bidnings as suggested by Stephen,
> patch looks good to me.
> Acked-by: Santosh Shilimkar <[email protected]>

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?

2014-07-10 12:39:13

by Nishanth Menon

[permalink] [raw]
Subject: Re: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime

On Thu, Jul 10, 2014 at 6:19 AM, Viresh Kumar <[email protected]> wrote:
> On 9 July 2014 20:14, Santosh Shilimkar <[email protected]> wrote:
>> Assuming you are updating bidnings as suggested by Stephen,
>> patch looks good to me.
>> Acked-by: Santosh Shilimkar <[email protected]>
>
> 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

2014-07-10 13:31:53

by Santosh Shilimkar

[permalink] [raw]
Subject: Re: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime

On Thursday 10 July 2014 08:39 AM, Nishanth Menon wrote:
> On Thu, Jul 10, 2014 at 6:19 AM, Viresh Kumar <[email protected]> wrote:
>> On 9 July 2014 20:14, Santosh Shilimkar <[email protected]> wrote:
>>> Assuming you are updating bidnings as suggested by Stephen,
>>> patch looks good to me.
>>> Acked-by: Santosh Shilimkar <[email protected]>
>>
>> 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


2014-07-10 13:36:47

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 07/14] cpufreq: cpu0: OPPs can be populated at runtime

On 10 July 2014 19:01, Santosh Shilimkar <[email protected]> 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.

2014-07-16 16:01:57

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

Cc'ing Thomas,

On 8 July 2014 10:20, Viresh Kumar <[email protected]> wrote:
> On 4 July 2014 09:51, Viresh Kumar <[email protected]> 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

2014-07-16 21:00:42

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On Wednesday, July 16, 2014 09:31:54 PM Viresh Kumar wrote:
> Cc'ing Thomas,
>
> On 8 July 2014 10:20, Viresh Kumar <[email protected]> wrote:
> > On 4 July 2014 09:51, Viresh Kumar <[email protected]> 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

2014-07-17 00:28:25

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On 17 July 2014 02:48, Rafael J. Wysocki <[email protected]> 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

2014-07-17 07:35:26

by Thomas Petazzoni

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

Dear Viresh Kumar,

On Thu, 17 Jul 2014 05:58:22 +0530, Viresh Kumar wrote:
> On 17 July 2014 02:48, Rafael J. Wysocki <[email protected]> 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
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

2014-07-17 07:41:48

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On 17 July 2014 13:05, Thomas Petazzoni
<[email protected]> 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..

2014-07-18 00:44:34

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On Thursday, July 17, 2014 01:11:45 PM Viresh Kumar wrote:
> On 17 July 2014 13:05, Thomas Petazzoni
> <[email protected]> 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

2014-07-18 04:17:27

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On 18 July 2014 06:32, Rafael J. Wysocki <[email protected]> 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

2014-07-24 10:48:10

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On 18 July 2014 09:47, Viresh Kumar <[email protected]> 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

2014-07-25 14:29:32

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On Thu, Jul 24, 2014 at 5:48 AM, Viresh Kumar <[email protected]> wrote:
> On 18 July 2014 09:47, Viresh Kumar <[email protected]> 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

2014-07-25 14:34:20

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

On 25 July 2014 19:59, Rob Herring <[email protected]> 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.

2014-07-25 15:41:26

by Thomas Petazzoni

[permalink] [raw]
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

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
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

2014-07-28 14:01:43

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH 02/14] clk: Create of_clk_shared_by_cpus()

On Tue, 1 Jul 2014 22:02:31 +0530, Viresh Kumar <[email protected]> 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 <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> 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");

This looks naïve. It is entirely possible for different clock specifiers
to resolve to the same clock line, or for there to be multple clocks in
the clocks property. This looks like a buggy way to do it. The only
reliable way to determine if two clocks resolve to the same thing is to
resolve the references with the clock controller.

g.