CPUFreq: Initialize CPU's OPP tables from CPU core

This is third attempt to initialize CPU's OPPs from CPU core code. First two are
here: https://lkml.org/lkml/2014/5/19/57 and https://lkml.org/lkml/2014/5/21/199

Drivers expecting CPU's OPPs from device tree initialize OPP table themselves by calling of_init_opp_table() and there is nothing driver specific in that. They all do it in the same redundant way.

It would be better if we can get rid of redundancy by initializing CPU OPPs from CPU core code for all CPUs (that have a "operating-points" property defined in their node).

This patchset is all about that.

The idea was initially discussed here: https://lkml.org/lkml/2014/5/17/123

V2->V3:
- s/dev_info/dev_dbg
- Fixed changelogs

V1->V2:
- Addition of two new patches: 1/2 & 2/2
- Created separate routine of_init_cpu_opp_table() which wouldn't add any overhead for the platforms which don't have OPP or OF enabled.
- Added a print for success case as well
- Added Acks from Shawn
- Got rid of extra indentation level by returning early from register_cpu().

Viresh Kumar (8):
cpufreq: cpufreq-cpu0: remove dependency on thermal
opp: of_init_opp_table(): return -ENOSYS when feature isn't
implemented
opp: call of_node_{get|put}() from of_init_opp_table() driver/core: cpu: initialize opp table
cpufreq: arm_big_little: don't initialize opp table
cpufreq: imx6q: don't initialize opp table
cpufreq: cpufreq-cpu0: don't initialize opp table
cpufreq: exynos5440: don't initialize opp table

 arch/arm/mach-imx/mach-imx6q.c       | 36 ++++++++----------------------------
 drivers/base/cpu.c                   | 30 ++++++++++++++++++++++++++----
 drivers/base/power/opp.c             |  4 ++++
 drivers/cpufreq/Kconfig              |  2 +-
 drivers/cpufreq/arm_big_little.c     | 12 +++++++-----
 drivers/cpufreq/arm_big_little_dt.c  | 18 ------------------
 drivers/cpufreq/cpufreq-cpu0.c       |  6 ------
 drivers/cpufreq/exynos5440-cpufreq.c |  6 ------
 drivers/cpufreq/imx6q-cpufreq.c      | 20 +-------------------
 include/linux/pm_opp.h               |  2 +-
 10 files changed, 48 insertions(+), 88 deletions(-)

2.0.0.rc2

cpufreq-cpu0 uses thermal framework to register a cooling device, but doesn't depend on it as there are dummy calls provided by thermal layer when CONFIG_THERMAL=n. So, we don't really need to mention thermal as a dependency for cpufreq-cpu0 in Kconfig.

Remove it.

---
 drivers/cpufreq/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1fbe11f..4310997 100644
@@ -185,7 +185,7 @@ config CPU_FREQ_GOV_CONSERVATIVE
 
 config GENERIC_CPUFREQ_CPU0
 	tristate "Generic CPU0 cpufreq driver"
-	depends on HAVE_CLK && REGULATOR && OF && THERMAL && CPU_THERMAL
+	depends on HAVE_CLK && REGULATOR && OF
 	select PM_OPP
 	help
 	  This adds a generic cpufreq driver for CPU0 frequency management.

2.0.0.rc2

Hello Viresh,

I see your point.

However, on CPUs that needs thermal managment, it makes sense to have such dependency, from functional perspective. Mainly because scaling frequency and voltage up would be allowed only when thermal management is enabled.

AFAIK, dependencies in KCONFIG are only for fixing compilation time issues. As some APIs wouldn't be available without enabling some config options..

If drivers fail at runtime because some API returned error, fix it for your platform instead and not bug KCONFIG for that.

Thought we might consider some runtime dependencies here as well. For example regulators. There probably are dummy routine available for cases where CONFIG_REGULATOR (or whatever) isn't enabled and driver would still compile, but it is guaranteed to fail as we don't continue when we get errors from regulator APIs..

Though I still feel that this driver should still support platforms without regulators (atleast in software, they might always have them on board :)).. And so dependencies for regulators may also die out one day..

The dependencies here mean: "This driver would never ever work/compile if the dependencies aren't met.."

Hi Viresh,

Actually, they also impose module loading sequencing.

Agreed, but I don't think this is the point of this thread, as you already stated in your patch description.

I agree. We need to have runtime dependency, and that is the major concern on my behalf. The problem of Kconfig dependency is that it imposes sequencing only on module loading, not at boot sequencing.

Another way around is returning -EPROBE_DEFER when some API is not ready at device probing for instance.

Okay, coming back to this patch. This driver is usable without thermal. And so the dependencies better be dropped ?

I do not think that's correct.

Yeah, that what I accepted later as well.. Dependency is whatever without which the module is unusable.

And cpufreq-cpu0 is usable without THERMAL and so this dependency should go away..

When none of CONFIG_PM_OPP or CONFIG_OF is enabled we use the dummy implementation of of_init_opp_table() routine, which returns -EINVAL currently. -EINVAL can confuse the callers a bit as it can have other meanings for the actual implementation of this routine.

It is more appropriate to return -ENOSYS instead to avoid confusion at caller.

---
 include/linux/pm_opp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 0330217..6668150 100644
@@ -112,7 +112,7 @@ int of_init_opp_table(struct device *dev);
 #else
 static inline int of_init_opp_table(struct device *dev)
 {
-	return -EINVAL;
+	return -ENOSYS;
 }
 #endif
 

2.0.0.rc2

All callers of of_init_opp_table() are required to take reference of dev->of_node, by initiating calls to of_node_{get|put}(), before and after calling of_init_opp_table().

Its better to call these from within of_init_opp_table(), no fun adding redundant code to every caller.

---
 drivers/base/power/opp.c | 4 ++++
 1 file changed, 4 insertions(+)

index faae9cf..2b615b9 100644
@@ -622,6 +622,9 @@ int of_init_opp_table(struct device *dev)
 	const __be32 *val;
 	int nr;
 
+	if (!of_node_get(dev->of_node))
+		return -ENODEV;
+
 	prop = of_find_property(dev->of_node, "operating-points", NULL);
 	if (!prop)
 		return -ENODEV;

@@ -649,6 +652,7 @@ int of_init_opp_table(struct device *dev)
 		nr -= 2;
 	}
 
+	of_node_put(dev->of_node);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);

2.0.0.rc2

Drivers expecting CPU's OPPs from device tree initialize OPP table themselves by calling of_init_opp_table() and there is nothing driver specific in that. They all do it in the same redundant way.

It would be better if we can get rid of redundancy by initializing CPU OPPs from CPU core code for all CPUs (that have a "operating-points" property defined in their node).

This patch adds another routine in cpu.c: of_init_cpu_opp_table() and calls it right after CPU device is registered in register_cpu(). A dummy implementation is also provided to make it lightweight for platforms that don't need it.

---
 drivers/base/cpu.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

index 006b1bc..818cfe8 100644
@@ -16,6 +16,7 @@
 #include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/cpufeature.h>
+#include <linux/pm_opp.h>
 
 #include "base.h"
 

@@ -322,6 +323,25 @@ static int cpu_uevent(struct device *dev, struct kobj_uevent_env *env)
 }
 #endif
 
+#if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
+static inline void of_init_cpu_opp_table(struct cpu *cpu)
+{
+	int error;
+
+	/* Initialize CPU's OPP table */
+	error = of_init_opp_table(&cpu->dev);
+	if (!error)
+		dev_dbg(&cpu->dev, "%s: created OPP table for cpu: %d\n",
+			__func__, cpu->dev.id);
+	/* Print error only if there is an issue with OPP table */
+	else if (error != -ENOSYS && error != -ENODEV)
+		dev_err(&cpu->dev, "%s: failed to init OPP table for cpu%d, err: %d\n",
+			__func__, cpu->dev.id, error);
+}
+#else
+static inline void of_init_cpu_opp_table(struct cpu *cpu) {}
+#endif
+
 /*
  * register_cpu - Setup a sysfs device for a CPU.
  * @cpu - cpu->hotpluggable field set to 1 will generate a control file in

@@ -349,10 +369,12 @@ int register_cpu(struct cpu *cpu, int num)
 	if (cpu->hotpluggable)
 		cpu->dev.groups = hotplugable_cpu_attr_groups;
 	error = device_register(&cpu->dev);
-	if (!error)
-		per_cpu(cpu_sys_devices, num) = &cpu->dev;
-	if (!error)
-		register_cpu_under_node(num, cpu_to_node(num));
+	if (error)
+		return error;
+
+	per_cpu(cpu_sys_devices, num) = &cpu->dev;
+	register_cpu_under_node(num, cpu_to_node(num));
+	of_init_cpu_opp_table(cpu);
 
 	return error;
 }

2.0.0.rc2

We are also using it for cpu->dev_id, but that's not so important. This routine wouldn't have existed if you wouldn't have asked for it. It is just a wrapper over of_init_opp_table, which also has a dummy implementation when its not supported.

So, it might not be worth enough for any other code to use it. :) And so I added it here.

Let me know how/where do you want it and I will resend it quickly.

There is another option here, add these print messages in of_init_opp_table() ? That's the only difference new wrapper has.

Yeah, put them into of_init_opp_table() rather I'd say.

Rafael

CPU OPP tables are already initialized by CPU core and we don't need to reinitialize them from arm_big_little_dt driver.

As the arm_big_little_dt driver doesn't have a .init_opp_table() callback anymore, make this callback optional.

---
 drivers/cpufreq/arm_big_little.c    | 12 +++++++-----
 drivers/cpufreq/arm_big_little_dt.c | 18 ------------------
 2 files changed, 7 insertions(+), 23 deletions(-)

index 1f4d4e3..561261e 100644
@@ -325,11 +325,13 @@ static int _get_cluster_clk_and_freq_table(struct device *cpu_dev)
 	if (freq_table[cluster])
 		return 0;
 
-	ret = arm_bL_ops->init_opp_table(cpu_dev);
-	if (ret) {
-		dev_err(cpu_dev, "%s: init_opp_table failed, cpu: %d, err: %d\n",
+	if (arm_bL_ops->init_opp_table) {
+		ret = arm_bL_ops->init_opp_table(cpu_dev);
+		if (ret) {
+			dev_err(cpu_dev, "%s: init_opp_table failed, cpu: %d, err: %d\n",
 				__func__, cpu_dev->id, ret);
-		goto out;
+			goto out;
+		}
 	}
 
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table[cluster]);

@@ -542,7 +544,7 @@ int bL_cpufreq_register(struct cpufreq_arm_bL_ops *ops)
 		return -EBUSY;
 	}
 
-	if (!ops || !strlen(ops->name) || !ops->init_opp_table) {
+	if (!ops || !strlen(ops->name)) {
 		pr_err("%s: Invalid arm_bL_ops, exiting\n", __func__);
 		return -ENODEV;
 	}

index 8d9d591..502182d 100644
@@ -43,23 +43,6 @@ static struct device_node *get_cpu_node_with_valid_op(int cpu)
 	return np;
 }
 
-static int dt_init_opp_table(struct device *cpu_dev)
-{
-	struct device_node *np;
-	int ret;
-
-	np = of_node_get(cpu_dev->of_node);
-	if (!np) {
-		pr_err("failed to find cpu%d node\n", cpu_dev->id);
-		return -ENOENT;
-	}
-
-	ret = of_init_opp_table(cpu_dev);
-	of_node_put(np);
-
-	return ret;
-}
-
 static int dt_get_transition_latency(struct device *cpu_dev)
 {
 	struct device_node *np;

@@ -81,7 +64,6 @@ static int dt_get_transition_latency(struct device *cpu_dev)
 static struct cpufreq_arm_bL_ops dt_bL_ops = {
 	.name	= "dt-bl",
 	.get_transition_latency = dt_get_transition_latency,
-	.init_opp_table = dt_init_opp_table,
 };
 
 static int generic_bL_probe(struct platform_device *pdev)

2.0.0.rc2

CPU OPP tables are already initialized by CPU core and we don't need to reinitialize them from imx6q specific code.

---
 arch/arm/mach-imx/mach-imx6q.c  | 36 ++++++++----------------------------
 drivers/cpufreq/imx6q-cpufreq.c | 20 +-------------------
 2 files changed, 9 insertions(+), 47 deletions(-)

index e60456d..03819e7 100644
@@ -290,12 +290,18 @@ static void __init imx6q_init_machine(void)
 #define OCOTP_CFG3_SPEED_996MHZ		0x2
 #define OCOTP_CFG3_SPEED_852MHZ		0x1
 
-static void __init imx6q_opp_check_speed_grading(struct device *cpu_dev)
+static void __init imx6q_opp_check_speed_grading(void)
 {
+	struct device *cpu_dev = get_cpu_device(0);
 	struct device_node *np;
 	void __iomem *base;
 	u32 val;
 
+	if (!cpu_dev) {
+		pr_warn("failed to get cpu0 device\n");
+		return;
+	}
+
 	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp");
 	if (!np) {
 		pr_warn("failed to find ocotp node\n");

@@ -336,32 +342,6 @@ put_node:
 	of_node_put(np);
 }
 
-static void __init imx6q_opp_init(void)
-{
-	struct device_node *np;
-	struct device *cpu_dev = get_cpu_device(0);
-
-	if (!cpu_dev) {
-		pr_warn("failed to get cpu0 device\n");
-		return;
-	}
-	np = of_node_get(cpu_dev->of_node);
-	if (!np) {
-		pr_warn("failed to find cpu0 node\n");
-		return;
-	}
-
-	if (of_init_opp_table(cpu_dev)) {
-		pr_warn("failed to init OPP table\n");
-		goto put_node;
-	}
-
-	imx6q_opp_check_speed_grading(cpu_dev);
-
-put_node:
-	of_node_put(np);
-}
-
 static struct platform_device imx6q_cpufreq_pdev = {
 	.name = "imx6q-cpufreq",
 };

@@ -376,7 +356,7 @@ static void __init imx6q_init_late(void)
 		imx6q_cpuidle_init();
 
 	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ)) {
-		imx6q_opp_init();
+		imx6q_opp_check_speed_grading();
 		platform_device_register(&imx6q_cpufreq_pdev);
 	}
 }

index af366c2..b72e94f 100644
@@ -190,26 +190,8 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 		goto put_reg;
 	}
 
-	/*
-	 * We expect an OPP table supplied by platform.
-	 * Just, incase the platform did not supply the OPP
-	 * table, it will try to get it.
-	 */
 	num = dev_pm_opp_get_opp_count(cpu_dev);
-	if (num < 0) {
-		ret = of_init_opp_table(cpu_dev);
-		if (ret < 0) {
-			dev_err(cpu_dev, "failed to init OPP table: %d\n", ret);
-			goto put_reg;
-		}
-
-		num = dev_pm_opp_get_opp_count(cpu_dev);
-		if (num < 0) {
-			ret = num;
-			dev_err(cpu_dev, "no OPP table is found: %d\n", ret);
-			goto put_reg;
-		}
-	}
+	WARN_ON(num < 0);
 
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
 	if (ret) {

2.0.0.rc2

CPU OPP tables are already initialized by CPU core and we don't need to reinitialize them from cpufreq-cpu0 driver.

---
 drivers/cpufreq/cpufreq-cpu0.c | 6 ------
 1 file changed, 6 deletions(-)

index 1bf6bba..4301c7c 100644
@@ -152,12 +152,6 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)
 		goto out_put_node;
 	}
 
-	ret = of_init_opp_table(cpu_dev);
-	if (ret) {
-		pr_err("failed to init OPP table: %d\n", ret);
-		goto out_put_node;
-	}
-
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
 	if (ret) {
 		pr_err("failed to init cpufreq table: %d\n", ret);

2.0.0.rc2

CPU OPP tables are already initialized by CPU core and we don't need to reinitialize them from exynos5440's driver.

---
 drivers/cpufreq/exynos5440-cpufreq.c | 6 ------
 1 file changed, 6 deletions(-)

index f33f25b..72a4206 100644
@@ -360,12 +360,6 @@ static int exynos_cpufreq_probe(struct platform_device *pdev)
 		goto err_put_node;
 	}
 
-	ret = of_init_opp_table(dvfs_info->dev);
-	if (ret) {
-		dev_err(dvfs_info->dev, "failed to init OPP table: %d\n", ret);
-		goto err_put_node;
-	}
-
 	ret = dev_pm_opp_init_cpufreq_table(dvfs_info->dev,
 					    &dvfs_info->freq_table);
 	if (ret) {

2.0.0.rc2