CPUFreq: Initialize CPU's OPP tables from CPU core

When any of CONFIG_PM_OPP or CONFIG_OF isn't enabled we have a dummy implementation of of_init_opp_table() routine, it returns -EINVAL currently. -EINVAL can be returned from other places within the real implementations of of_init_opp_table() and so caller wouldn't be able to differentiate between those two cases.

Make it return -ENOSYS instead for better handling.

Suggested-by Sudeep Holla <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

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

include/linux/pm_opp.h +1 -1

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
 

Hi Viresh,

$subject perhaps should be "replace/remove -EINVAL.." instead of "remove -ENOSYS ..."

All drivers expecting CPU's OPPs from device tree initialize OPP table using of_init_opp_table() and there is nothing driver specific in that. They all do it in the same way adding to code redundancy.

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

This patch initializes OPPs as soon as CPU device is registered in register_cpu().

Cc Greg Kroah-Hartman <[email protected]>

Cc Amit Daniel Kachhap <[email protected]>

Cc Kukjin Kim <[email protected]>

Cc Shawn Guo <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/base/cpu.c +1 -0

index 006b1bc..853e99e 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
 

[Nit] Not sure if we need this logging on each cpu, may be dev_dbg if you still
fancy one ? :)

Quoted 8 lines

drivers/base/cpu.c +3 -0

+       /* 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",

drivers/base/cpu.c +4 -0

+                       __func__, cpu->dev.id, error);
+}
+#else
+static inline void of_init_cpu_opp_table(struct cpu *cpu) {}

drivers/base/cpu.c static int cpu_uevent(struct device *dev, struct kobj_uevent_env *env) · line 344

Quoted 2 lines

drivers/base/cpu.c +2 -0

+#endif
+

IMO this could be more generic and applicable for any device if you replace "struct cpu" with "struct device". I know this is currently used by cpu but we can extend it's use for any device if needed in future. You can then move this to pm_opp.h as of_init_dev_opp_table may be.

Quoted 3 lines · Sudeep Holla

[Nit] Not sure if we need this logging on each cpu, may be dev_dbg if you still
fancy one ? :)

It wouldn't happen on each CPU, but one CPU per policy as others would return -ENODEV.

I added dev_dbg earlier but then I thought dev_info is better as we may better show this to everybody as it about the most important device, i.e. CPU :)

Quoted 8 lines

Quoted 8 lines

drivers/base/cpu.c +3 -0

+       /* 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",

drivers/base/cpu.c +4 -0

+                       __func__, cpu->dev.id, error);
+}
+#else
+static inline void of_init_cpu_opp_table(struct cpu *cpu) {}

Quoted 8 lines

drivers/base/cpu.c static int cpu_uevent(struct device *dev, struct kobj_uevent_env *env) · line 344

Quoted 2 lines

drivers/base/cpu.c +2 -0

+#endif
+

IMO this could be more generic and applicable for any device if you replace "struct cpu" with "struct device". I know this is currently used by cpu but we can extend it's use for any device if needed in future. You can then move this to pm_opp.h as of_init_dev_opp_table may be.

Probably we will call of_init_opp_table() directly for other devices, as this function doesn't do anything else, apart from some prints.. So, probably leave is as is for now, unless a real need arises ?

Hmm agreed, but there are SoCs that support per CPU DVFS ;)

Quoted 4 lines

Probably we will call of_init_opp_table() directly for other devices, as this
function doesn't do anything else, apart from some prints.. So, probably leave is as is for now, unless a real need arises ?

I don't see anything cpu specific there, but that's just my opinion.

All drivers expecting CPU's OPPs from device tree initialize OPP table using of_init_opp_table() and there is nothing driver specific in that. They all do it in the same way adding to code redundancy.

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

This patch initializes OPPs as soon as CPU device is registered in register_cpu().

Cc Greg Kroah-Hartman <[email protected]>

Cc Amit Daniel Kachhap <[email protected]>

Cc Kukjin Kim <[email protected]>

Cc Shawn Guo <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/base/cpu.c +8 -0

index 006b1bc..853e99e 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);

Sorry I missed this earlier, main idea of this wrapper is not to have any config dependency and hide error handling details for non-DT platforms. Since
of_init_opp_table has dummy implementation, you really don't need this dummy implementation again here.

OPP tables are already initialized for all CPUs by cpufreq core and so we don't need to reinitialize them from arm_big_little_dt driver.

Also, as the arm_big_little_dt driver doesn't have a .init_opp_table() callback anymore, make it optional in arm_big_little driver.

Cc Sudeep Holla <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/arm_big_little.c +7 -5

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

drivers/cpufreq/arm_big_little_dt.c +0 -19

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

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I guess you wanted to say "cpu core code" instead of "cpufreq core". Correction may be needed here and in the subsequent patches as well.

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

Acked-by Shawn Guo <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

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

arch/arm/mach-imx/mach-imx6q.c +7 -27

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

arch/arm/mach-imx/mach-imx6q.c static void __init imx6q_init_late(void) · line 359

Quoted 6 lines

arch/arm/mach-imx/mach-imx6q.c +1 -1

@@ -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();

[Query] Not exactly related to this patch, but asking it here to get clarified.

This OPP limiting is done as part of late initcall and if the cpufreq driver is built in the kernel, there will be a small window where the OPPs not supported are still enabled ? Will that not be an issue if say performance governor is selected by default.

Quoted 9 lines · Sudeep Holla

arch/arm/mach-imx/mach-imx6q.c static void __init imx6q_init_late(void) · line 359

Quoted 6 lines

arch/arm/mach-imx/mach-imx6q.c +1 -1

@@ -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();

[Query] Not exactly related to this patch, but asking it here to get clarified.

Quoted 4 lines

This OPP limiting is done as part of late initcall and if the cpufreq driver is built in the kernel, there will be a small window where the OPPs not supported are still enabled ? Will that not be an issue if say performance governor is selected by default.

Even if cpufreq driver is built in the kernel, it won't be probed until platform_device_register(&imx6q_cpufreq_pdev) is called. And we make this call only after the OPP limiting.