[PATCH 0/5] CPUFreq: Initialize CPU's OPP tables from core

[PATCH 0/5] CPUFreq: Initialize CPU's OPP tables from core

[Viresh Kumar]

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.

First patch initializes OPPs as soon as CPU device is registered in
register_cpu(). Following patches get rid of these calls from individual drivers
which are currently initializing OPPs.

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

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>

Viresh Kumar (5):
  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                   | 14 ++++++++++++--
 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 +-------------------
 7 files changed, 28 insertions(+), 84 deletions(-)

-- 
2.0.0.rc2

[PATCH 1/5] driver/core: cpu: initialize opp table

[Viresh Kumar]

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 <gregkh@linuxfoundation.org>
Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/cpu.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 006b1bc..c787b5b 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -16,6 +16,7 @@
 #include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/cpufeature.h>
+#include <linux/pm_opp.h>
 
 #include "base.h"
 
@@ -349,11 +350,20 @@ 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)
+	if (!error) {
 		per_cpu(cpu_sys_devices, num) = &cpu->dev;
-	if (!error)
 		register_cpu_under_node(num, cpu_to_node(num));
 
+		/* Initialize CPUs OPP table */
+		if (of_node_get(cpu->dev.of_node)) {
+			error = of_init_opp_table(&cpu->dev);
+			if (error && error != -ENODEV)
+				pr_err("****%s: failed to init OPP table for cpu%d, err: %d\n",
+					__func__, num, error);
+			of_node_put(cpu->dev.of_node);
+		}
+	}
+
 	return error;
 }
 
-- 
2.0.0.rc2

[PATCH 2/5] cpufreq: arm_big_little: don't initialize opp table

[Viresh Kumar]

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 <sudeep.holla@arm.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/arm_big_little.c    | 12 +++++++-----
 drivers/cpufreq/arm_big_little_dt.c | 18 ------------------
 2 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
index 1f4d4e3..561261e 100644
--- a/drivers/cpufreq/arm_big_little.c
+++ b/drivers/cpufreq/arm_big_little.c
@@ -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;
 	}
diff --git a/drivers/cpufreq/arm_big_little_dt.c b/drivers/cpufreq/arm_big_little_dt.c
index 8d9d591..502182d 100644
--- a/drivers/cpufreq/arm_big_little_dt.c
+++ b/drivers/cpufreq/arm_big_little_dt.c
@@ -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

[PATCH 3/5] cpufreq: imx6q: don't initialize opp table

[Viresh Kumar]

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

Cc: Shawn Guo <shawn.guo@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 arch/arm/mach-imx/mach-imx6q.c  | 36 ++++++++----------------------------
 drivers/cpufreq/imx6q-cpufreq.c | 20 +-------------------
 2 files changed, 9 insertions(+), 47 deletions(-)

diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index e60456d..03819e7 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -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);
 	}
 }
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index e27fca8..c4c4e70 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -191,26 +191,8 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 		goto put_node;
 	}
 
-	/*
-	 * 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_node;
-		}
-
-		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_node;
-		}
-	}
+	WARN_ON(num < 0);
 
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
 	if (ret) {
-- 
2.0.0.rc2

[PATCH 4/5] cpufreq: cpufreq-cpu0: don't initialize opp table

[Viresh Kumar]

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

Cc: Shawn Guo <shawn.guo@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-cpu0.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
index 1bf6bba..4301c7c 100644
--- a/drivers/cpufreq/cpufreq-cpu0.c
+++ b/drivers/cpufreq/cpufreq-cpu0.c
@@ -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

[PATCH 5/5] cpufreq: exynos5440: don't initialize opp table

[Viresh Kumar]

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

Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/exynos5440-cpufreq.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c
index f33f25b..72a4206 100644
--- a/drivers/cpufreq/exynos5440-cpufreq.c
+++ b/drivers/cpufreq/exynos5440-cpufreq.c
@@ -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

[PATCH Resend] driver/core: cpu: initialize opp table

[Viresh Kumar]

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 <gregkh@linuxfoundation.org>
Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V1-V2:
A colleague spotted some extra debug prints in my first mail :(

Replace
+ pr_err("****%s: failed to init OPP table for cpu%d, err: %d\n",
with
+ pr_err("%s: failed to init OPP table for cpu%d, err: %d\n",

 drivers/base/cpu.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 006b1bc..74ce944 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -16,6 +16,7 @@
 #include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/cpufeature.h>
+#include <linux/pm_opp.h>
 
 #include "base.h"
 
@@ -349,11 +350,20 @@ 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)
+	if (!error) {
 		per_cpu(cpu_sys_devices, num) = &cpu->dev;
-	if (!error)
 		register_cpu_under_node(num, cpu_to_node(num));
 
+		/* Initialize CPUs OPP table */
+		if (of_node_get(cpu->dev.of_node)) {
+			error = of_init_opp_table(&cpu->dev);
+			if (error && error != -ENODEV)
+				pr_err("%s: failed to init OPP table for cpu%d, err: %d\n",
+					__func__, num, error);
+			of_node_put(cpu->dev.of_node);
+		}
+	}
+
 	return error;
 }
 
-- 
2.0.0.rc2

Re: [PATCH Resend] driver/core: cpu: initialize opp table

[Rafael J. Wysocki]

On Monday, May 19, 2014 11:59:11 AM Viresh Kumar wrote:
> 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 <gregkh@linuxfoundation.org>
> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V1-V2:
> A colleague spotted some extra debug prints in my first mail :(
> 
> Replace
> + pr_err("****%s: failed to init OPP table for cpu%d, err: %d\n",
> with
> + pr_err("%s: failed to init OPP table for cpu%d, err: %d\n",
> 
>  drivers/base/cpu.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 006b1bc..74ce944 100644
> --- a/drivers/base/cpu.c
> +++ b/drivers/base/cpu.c
> @@ -16,6 +16,7 @@
>  #include <linux/acpi.h>
>  #include <linux/of.h>
>  #include <linux/cpufeature.h>
> +#include <linux/pm_opp.h>
>  
>  #include "base.h"
>  
> @@ -349,11 +350,20 @@ 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)

What about

	if (error)
		return error;

and then you'd save an indentation level?

Anyway, I find adding of_node* stuff directly to the driver core this way
kind of disgusting as there still are platforms that don't use it.

Can we have a call to a function that will change into an empty stub on such
platforms here, please?

> +	if (!error) {
>  		per_cpu(cpu_sys_devices, num) = &cpu->dev;
> -	if (!error)
>  		register_cpu_under_node(num, cpu_to_node(num));
>  
> +		/* Initialize CPUs OPP table */
> +		if (of_node_get(cpu->dev.of_node)) {
> +			error = of_init_opp_table(&cpu->dev);
> +			if (error && error != -ENODEV)
> +				pr_err("%s: failed to init OPP table for cpu%d, err: %d\n",
> +					__func__, num, error);
> +			of_node_put(cpu->dev.of_node);
> +		}
> +	}
> +
>  	return error;
>  }
>  
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH Resend] driver/core: cpu: initialize opp table

[Rafael J. Wysocki]

On Monday, May 19, 2014 11:13:24 PM Rafael J. Wysocki wrote:
> On Monday, May 19, 2014 11:59:11 AM Viresh Kumar wrote:
> > 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 <gregkh@linuxfoundation.org>
> > Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
> > Cc: Kukjin Kim <kgene.kim@samsung.com>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Do patches [2-5/5] depend on this one BTW?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH Resend] driver/core: cpu: initialize opp table

[Viresh Kumar]

On 20 May 2014 02:43, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> What about
>
>         if (error)
>                 return error;
>
> and then you'd save an indentation level?

Yes.

> Anyway, I find adding of_node* stuff directly to the driver core this way
> kind of disgusting as there still are platforms that don't use it.
>
> Can we have a call to a function that will change into an empty stub on such
> platforms here, please?

Okay. But can you explain a bit more about how and where those stubs
would be implemented?

Re: [PATCH Resend] driver/core: cpu: initialize opp table

[Viresh Kumar]

On 20 May 2014 02:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Do patches [2-5/5] depend on this one BTW?

Yes.

Re: [PATCH 0/5] CPUFreq: Initialize CPU's OPP tables from core

[Shawn Guo]

On Mon, May 19, 2014 at 11:47:20AM +0530, Viresh Kumar wrote:
> Viresh Kumar (5):
...
>   cpufreq: imx6q: don't initialize opp table
>   cpufreq: cpufreq-cpu0: don't initialize opp table

Acked-by: Shawn Guo <shawn.guo@linaro.org>

Re: [PATCH 0/5] CPUFreq: Initialize CPU's OPP tables from core

[Shawn Guo]

On Mon, May 19, 2014 at 11:47:20AM +0530, Viresh Kumar wrote:
> Viresh Kumar (5):
...
>   cpufreq: imx6q: don't initialize opp table
>   cpufreq: cpufreq-cpu0: don't initialize opp table

Acked-by: Shawn Guo <shawn.guo@linaro.org>

Re: [PATCH Resend] driver/core: cpu: initialize opp table

[Sudeep Holla]

On 19/05/14 07:29, Viresh Kumar wrote:
> 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().
>

This is really nice getting rid of all duplicate code.

> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V1-V2:
> A colleague spotted some extra debug prints in my first mail :(
>
> Replace
> + pr_err("****%s: failed to init OPP table for cpu%d, err: %d\n",
> with
> + pr_err("%s: failed to init OPP table for cpu%d, err: %d\n",
>
>   drivers/base/cpu.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 006b1bc..74ce944 100644
> --- a/drivers/base/cpu.c
> +++ b/drivers/base/cpu.c
> @@ -16,6 +16,7 @@
>   #include <linux/acpi.h>
>   #include <linux/of.h>
>   #include <linux/cpufeature.h>
> +#include <linux/pm_opp.h>
>
>   #include "base.h"
>
> @@ -349,11 +350,20 @@ 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)
> +	if (!error) {
>   		per_cpu(cpu_sys_devices, num) = &cpu->dev;
> -	if (!error)
>   		register_cpu_under_node(num, cpu_to_node(num));
>
> +		/* Initialize CPUs OPP table */
> +		if (of_node_get(cpu->dev.of_node)) {
> +			error = of_init_opp_table(&cpu->dev);
> +			if (error && error != -ENODEV)

As Rafael mentioned it's better to have a wrapper function to hide these
details. You should consider the fact that of_init_opp_table returns -EINVAL if
CONFIG_PM_OPP not defined as well as when the list is invalid in the DT.
IMO we can return -ENOSYS if not implemented(i.e. !CONFIG_PM_OPP)

> +				pr_err("%s: failed to init OPP table for cpu%d, err: %d\n",
> +					__func__, num, error);
> +			of_node_put(cpu->dev.of_node);
> +		}
> +	}
> +
>   	return error;
>   }
>

Regards,
Sudeep

Re: [PATCH Resend] driver/core: cpu: initialize opp table

[Viresh Kumar]

On 21 May 2014 15:05, Sudeep Holla <sudeep.holla@arm.com> wrote:
> As Rafael mentioned it's better to have a wrapper function to hide these
> details. You should consider the fact that of_init_opp_table returns -EINVAL
> if
> CONFIG_PM_OPP not defined as well as when the list is invalid in the DT.
> IMO we can return -ENOSYS if not implemented(i.e. !CONFIG_PM_OPP)

I didn't understood Rafael's comment as I couldn't figure out if he is just
pointing to CONFIG_** or some arch specific thing..

But it looks more obvious that he asked me something similar to what you
are saying :)

Why do we need to return anything? Let that function have return type 'void'?
Also would it make sense to move this into it as well?

cpu->dev.of_node = of_get_cpu_node(num, NULL);

Re: [PATCH Resend] driver/core: cpu: initialize opp table

[Sudeep Holla]

On 21/05/14 10:41, Viresh Kumar wrote:
> On 21 May 2014 15:05, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> As Rafael mentioned it's better to have a wrapper function to hide these
>> details. You should consider the fact that of_init_opp_table returns -EINVAL
>> if
>> CONFIG_PM_OPP not defined as well as when the list is invalid in the DT.
>> IMO we can return -ENOSYS if not implemented(i.e. !CONFIG_PM_OPP)
>
> I didn't understood Rafael's comment as I couldn't figure out if he is just
> pointing to CONFIG_** or some arch specific thing..
>
> But it looks more obvious that he asked me something similar to what you
> are saying :)
>

I believe so, mainly the non-DT case, since you are checking for error, it will
end up with spurious messages as the return value is -EINVAL. Hence I was
suggesting return -ENOSYS(which means Function not implemented)

> Why do we need to return anything? Let that function have return type 'void'?

Hmm, don't we still need to throw error if DT has invalid OPP ?
It doesn't may sense to me if no errors is returned and still CPUFreq fails
later.

> Also would it make sense to move this into it as well?
>
> cpu->dev.of_node = of_get_cpu_node(num, NULL);
>

I don't quite understand what you mean here ?

Regards,
Sudeep

Re: [PATCH Resend] driver/core: cpu: initialize opp table

[Viresh Kumar]

On 21 May 2014 15:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> Why do we need to return anything? Let that function have return type
>> 'void'?
>
>
> Hmm, don't we still need to throw error if DT has invalid OPP ?
> It doesn't may sense to me if no errors is returned and still CPUFreq fails
> later.

I wasn't sure if we should fail CPU's registration if it had bad OPPs.
Maybe just print a good message and go ahead. CPU is usable atleast :)

>> Also would it make sense to move this into it as well?
>>
>> cpu->dev.of_node = of_get_cpu_node(num, NULL);
>>
>
> I don't quite understand what you mean here ?

This piece of code was added by you earlier to register_cpu(). As this was
also DT specific, should I move this as well to the new routine we are creating?

Re: [PATCH Resend] driver/core: cpu: initialize opp table

[Rafael J. Wysocki]

On Tuesday, May 20, 2014 08:08:11 AM Viresh Kumar wrote:
> On 20 May 2014 02:43, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > What about
> >
> >         if (error)
> >                 return error;
> >
> > and then you'd save an indentation level?
> 
> Yes.
> 
> > Anyway, I find adding of_node* stuff directly to the driver core this way
> > kind of disgusting as there still are platforms that don't use it.
> >
> > Can we have a call to a function that will change into an empty stub on such
> > platforms here, please?
> 
> Okay. But can you explain a bit more about how and where those stubs
> would be implemented?

In a header file included by cpu.c.  Something like

#if defined(CONFIG_OF) && defined(CONFIG_PM_OPP)
<function header>
#else
static inline <stub>
#endif


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH Resend] driver/core: cpu: initialize opp table

[Viresh Kumar]

On 22 May 2014 05:27, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> In a header file included by cpu.c.  Something like
>
> #if defined(CONFIG_OF) && defined(CONFIG_PM_OPP)
> <function header>
> #else
> static inline <stub>
> #endif

Thanks, Sudeep already helped me in understanding that :) ..
Already implemented that in cpu.c only ..