[PATCH V4 0/8] driver/core: cpu: initialize opp table

[PATCH V4 0/8] driver/core: cpu: initialize opp table

[Viresh Kumar]

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 callsl of_init_opp_table() directly from register_cpu() right
after CPU device is registered. Later patches updates existing cpufreq drivers
which were calling of_init_opp_table() directly.

V3->V4:
- Remove of_init_cpu_opp_table() and use of_init_opp_table() instead after
  adding more print messages to it.
- Drop a rather unrelated patch which was getting rid of dependency on thermal
  for cpufreq-cpu0. Will be sent separately.

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 (8):
  opp: of_init_opp_table(): return -ENOSYS when feature isn't
    implemented
  opp: call of_node_{get|put}() from of_init_opp_table()
  opp: Enhance debug messages in 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                   | 11 +++++++----
 drivers/base/power/opp.c             | 11 ++++++++++-
 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 +-
 9 files changed, 34 insertions(+), 88 deletions(-)

-- 
2.0.0.rc2

[PATCH V4 1/8] opp: of_init_opp_table(): return -ENOSYS when feature isn't implemented

[Viresh Kumar]

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.

Suggested-and-reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/pm_opp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 0330217..6668150 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -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

[PATCH V4 2/8] opp: call of_node_{get|put}() from of_init_opp_table()

[Viresh Kumar]

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.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index faae9cf..2b615b9 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -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

[PATCH V4 3/8] opp: Enhance debug messages in of_init_opp_table()

[Viresh Kumar]

Enhance print messages for debugging purposes. Add a dev_err() whenever we fail
to initialize OPP table due to some error in the table present in dts and add a
dev_dbg() for success case.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 2b615b9..ed5d580 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -628,8 +628,11 @@ int of_init_opp_table(struct device *dev)
 	prop = of_find_property(dev->of_node, "operating-points", NULL);
 	if (!prop)
 		return -ENODEV;
-	if (!prop->value)
+	if (!prop->value) {
+		dev_err(dev, "%s: failed to init OPP: prop->value is NULL\n",
+			__func__);
 		return -ENODATA;
+	}
 
 	/*
 	 * Each OPP is a set of tuples consisting of frequency and
@@ -653,6 +656,8 @@ int of_init_opp_table(struct device *dev)
 	}
 
 	of_node_put(dev->of_node);
+
+	dev_dbg(dev, "%s: successfully created OPP table\n", __func__);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
-- 
2.0.0.rc2

[PATCH V4 4/8] driver/core: cpu: initialize opp table

[Viresh Kumar]

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 calls of_init_opp_table() right after CPU device is registered in
register_cpu(). of_init_opp_table() also has a dummy implementation which simply
returns -ENOSYS when CONFIG_OPP or CONFIG_OF isn't supported by some platform.

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 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 006b1bc..790183f 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,10 +350,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_opp_table(&cpu->dev);
 
 	return error;
 }
-- 
2.0.0.rc2

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

[Viresh Kumar]

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.

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 V4 6/8] cpufreq: imx6q: don't initialize opp table

[Viresh Kumar]

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

Acked-by: 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 af366c2..b72e94f 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -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

[PATCH V4 7/8] cpufreq: cpufreq-cpu0: don't initialize opp table

[Viresh Kumar]

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

Acked-by: 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 V4 8/8] cpufreq: exynos5440: don't initialize opp table

[Viresh Kumar]

CPU OPP tables are already initialized by CPU core and 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

Re: [PATCH V4 1/8] opp: of_init_opp_table(): return -ENOSYS when feature isn't implemented

[Nishanth Menon]

minor (Rafael usually fixes it up, but still):
$subject: PM / OPP:

On 05/27/2014 06:50 AM, Viresh Kumar wrote:
> 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.
> 
> Suggested-and-reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

otherwise,

Acked-by: Nishanth Menon <nm@ti.com>

> ---
>  include/linux/pm_opp.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 0330217..6668150 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -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
>  
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH V4 2/8] opp: call of_node_{get|put}() from of_init_opp_table()

[Nishanth Menon]

On 05/27/2014 06:50 AM, Viresh Kumar wrote:
> 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.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/opp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index faae9cf..2b615b9 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -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);
> 
With the same $subject comment as patch #1:
Acked-by: Nishanth Menon <nm@ti.com>

-- 
Regards,
Nishanth Menon

Re: [PATCH V4 3/8] opp: Enhance debug messages in of_init_opp_table()

[Nishanth Menon]

On 05/27/2014 06:50 AM, Viresh Kumar wrote:
> Enhance print messages for debugging purposes. Add a dev_err() whenever we fail
> to initialize OPP table due to some error in the table present in dts and add a
> dev_dbg() for success case.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/opp.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 2b615b9..ed5d580 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -628,8 +628,11 @@ int of_init_opp_table(struct device *dev)
>  	prop = of_find_property(dev->of_node, "operating-points", NULL);
>  	if (!prop)
>  		return -ENODEV;
> -	if (!prop->value)
> +	if (!prop->value) {
> +		dev_err(dev, "%s: failed to init OPP: prop->value is NULL\n",
> +			__func__);
>  		return -ENODATA;
> +	}
>  
>  	/*
>  	 * Each OPP is a set of tuples consisting of frequency and
> @@ -653,6 +656,8 @@ int of_init_opp_table(struct device *dev)
>  	}
>  
>  	of_node_put(dev->of_node);
> +
> +	dev_dbg(dev, "%s: successfully created OPP table\n", __func__);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(of_init_opp_table);
> 
With the same $subject comment as patch #1:
Acked-by: Nishanth Menon <nm@ti.com>

-- 
Regards,
Nishanth Menon

Re: [PATCH V4 4/8] driver/core: cpu: initialize opp table

[Nishanth Menon]

On 05/27/2014 06:50 AM, Viresh Kumar wrote:
> 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 calls of_init_opp_table() right after CPU device is registered in
> register_cpu(). of_init_opp_table() also has a dummy implementation which simply
> returns -ENOSYS when CONFIG_OPP or CONFIG_OF isn't supported by some platform.
> 
> 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 | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 006b1bc..790183f 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,10 +350,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));

The change so far is a improvement in error handling -> which,
personally I find nice, but not necessarily related to the $subject or
covered in commit message. I suggest splitting that specific change
out as a patch of it's own.

> +	of_init_opp_table(&cpu->dev);
tricky... :) I finally did catchup on previous discussions
https://patchwork.kernel.org/patch/4199601/
https://patchwork.kernel.org/patch/4199741/
https://patchwork.kernel.org/patch/4215901/
https://patchwork.kernel.org/patch/4220431/

So, will let Rafael comment better here.

>  
>  	return error;
>  }
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH V4 2/8] opp: call of_node_{get|put}() from of_init_opp_table()

[Sachin Kamat]

Hi Viresh,

On 27 May 2014 17:20, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 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.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/opp.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index faae9cf..2b615b9 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -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;

What about of_node_put before returning here and other places in this function?

-- 
With warm regards,
Sachin

Re: [PATCH V4 2/8] opp: call of_node_{get|put}() from of_init_opp_table()

[Rafael J. Wysocki]

On Friday, May 30, 2014 12:03:08 PM Sachin Kamat wrote:
> Hi Viresh,
> 
> On 27 May 2014 17:20, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > 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.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/base/power/opp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> > index faae9cf..2b615b9 100644
> > --- a/drivers/base/power/opp.c
> > +++ b/drivers/base/power/opp.c
> > @@ -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;
> 
> What about of_node_put before returning here and other places in this function?

Yeah, that needs to be fixed.

The rest of the series looks good to me, so Viresh, please fix this one ASAP and
send an update (of this particular patch only).

Thanks!

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

Re: [PATCH V4 4/8] driver/core: cpu: initialize opp table

[Viresh Kumar]

On 30 May 2014 04:11, Nishanth Menon <nm@ti.com> wrote:
> The change so far is a improvement in error handling -> which,
> personally I find nice, but not necessarily related to the $subject or
> covered in commit message. I suggest splitting that specific change
> out as a patch of it's own.

I would be required to do this here:

if (!error)
    of_init_opp_table(**);

And so did this change as part of this patch :), probably rafael doesn't
feel a need of this and so has asked me to do a resend of a single patch
(reported by Sachin) ..

So, will leave it as is unless he wants me to send it again :)

For this and other patches, thanks for your review.

[PATCH V5 2/8] opp: call of_node_{get|put}() from of_init_opp_table()

[Viresh Kumar]

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.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V4->V5: do of_node_put() on errors as well

 drivers/base/power/opp.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 89ced95..28696af 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -643,13 +643,21 @@ int of_init_opp_table(struct device *dev)
 {
 	const struct property *prop;
 	const __be32 *val;
-	int nr;
+	int nr, ret = 0;
 
-	prop = of_find_property(dev->of_node, "operating-points", NULL);
-	if (!prop)
+	if (!of_node_get(dev->of_node))
 		return -ENODEV;
-	if (!prop->value)
-		return -ENODATA;
+
+	prop = of_find_property(dev->of_node, "operating-points", NULL);
+	if (!prop) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	if (!prop->value) {
+		ret = -ENODATA;
+		goto out;
+	}
 
 	/*
 	 * Each OPP is a set of tuples consisting of frequency and
@@ -658,7 +666,8 @@ int of_init_opp_table(struct device *dev)
 	nr = prop->length / sizeof(u32);
 	if (nr % 2) {
 		dev_err(dev, "%s: Invalid OPP list\n", __func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 
 	val = prop->value;
@@ -672,6 +681,8 @@ int of_init_opp_table(struct device *dev)
 		nr -= 2;
 	}
 
+out:
+	of_node_put(dev->of_node);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
-- 
2.0.0.rc2

[PATCH V5 3/8] opp: Enhance debug messages in of_init_opp_table()

[Viresh Kumar]

Enhance print messages for debugging purposes. Add a dev_err() whenever we fail
to initialize OPP table due to some error in the table present in dts and add a
dev_dbg() for success case.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V4->V5: Fix rebase conflict as 2/8 has changed a bit

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

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 28696af..ba20510 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -655,6 +655,8 @@ int of_init_opp_table(struct device *dev)
 	}
 
 	if (!prop->value) {
+		dev_err(dev, "%s: failed to init OPP: prop->value is NULL\n",
+			__func__);
 		ret = -ENODATA;
 		goto out;
 	}
@@ -681,6 +683,8 @@ int of_init_opp_table(struct device *dev)
 		nr -= 2;
 	}
 
+	dev_dbg(dev, "%s: successfully created OPP table\n", __func__);
+
 out:
 	of_node_put(dev->of_node);
 	return 0;
-- 
2.0.0.rc2

Re: [PATCH V4 4/8] driver/core: cpu: initialize opp table

[Rafael J. Wysocki]

On Tuesday, May 27, 2014 05:20:53 PM Viresh Kumar wrote:
> 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 calls of_init_opp_table() right after CPU device is registered in
> register_cpu(). of_init_opp_table() also has a dummy implementation which simply
> returns -ENOSYS when CONFIG_OPP or CONFIG_OF isn't supported by some platform.
> 
> 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 | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 006b1bc..790183f 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,10 +350,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_opp_table(&cpu->dev);

Having thought a bit more about that I don't really like this.

In the ACPI land we have a processor driver that binds to CPU devices and
carries out similar initialization.  I kind of prefer that to littering
core code with platform-specific stuff.

Rafael