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

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

[Viresh Kumar]

This is second attempt to initialize CPU's OPPs from CPU core code. First one
was here: https://lkml.org/lkml/2014/5/19/57

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 patchset is all about that.

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

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

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>

Viresh Kumar (7):
  opp: remove -ENOSYS from dummy implementation of of_init_opp_table()
  opp: handle of_node_{get|put}() inside 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/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, 47 insertions(+), 87 deletions(-)

-- 
2.0.0.rc2

[PATCH V2 1/7] opp: remove -ENOSYS from dummy implementation of of_init_opp_table()

[Viresh Kumar]

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 <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 V2 2/7] opp: handle of_node_{get|put}() inside of_init_opp_table()

[Viresh Kumar]

All callers of of_init_opp_table() are required to call of_node_{get|put}()
before and after calling it. It would be better if this can be handled within
of_init_opp_table().

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 V2 3/7] 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>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/cpu.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 006b1bc..853e99e 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"
 
@@ -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_info(&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

[PATCH V2 4/7] 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 V2 5/7] 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.

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 V2 6/7] 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.

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 V2 7/7] 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.

Cd: Amit Daniel Kachhap <amit.daniel@samsung.com>
Cd: 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 V2 4/7] cpufreq: arm_big_little: don't initialize opp table

[Inderpal Singh]

On Wed, May 21, 2014 at 4:40 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 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.
>

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.


> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 1/7] opp: remove -ENOSYS from dummy implementation of of_init_opp_table()

[Sudeep Holla]

Hi Viresh,

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

On 21/05/14 12:09, Viresh Kumar wrote:
> 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.
>
[Nit] How about "it's more appropriate to return -ENOSYS as since the function
is not implemented" ?

Otherwise it looks fine to me.
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

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

Re: [PATCH V2 1/7] opp: remove -ENOSYS from dummy implementation of of_init_opp_table()

[Viresh Kumar]

On 21 May 2014 18:46, Sudeep Holla <sudeep.holla@arm.com> wrote:
> Hi Viresh,
>
> $subject perhaps should be "replace/remove -EINVAL.." instead of "remove
> -ENOSYS ..."

What's going on with my logs? Really annoying for me as well..

Re: [PATCH V2 4/7] cpufreq: arm_big_little: don't initialize opp table

[Viresh Kumar]

On 21 May 2014 17:05, Inderpal Singh <inderpal.s@samsung.com> wrote:
> 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.

When I started implementation, I added code to cpufreq core's init
routine and later moved it to base/cpu.c .. And just forgot to update
it later :(

Thanks for letting me know

Re: [PATCH V2 3/7] driver/core: cpu: initialize opp table

[Sudeep Holla]

On 21/05/14 12:10, 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>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/base/cpu.c | 30 ++++++++++++++++++++++++++----
>   1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 006b1bc..853e99e 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"
>
> @@ -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_info(&cpu->dev, "%s: created OPP table for cpu: %d\n",
> +			 __func__, cpu->dev.id);

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

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

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.

Regards,
Sudeep

Re: [PATCH V2 3/7] driver/core: cpu: initialize opp table

[Viresh Kumar]

On 21 May 2014 19:15, Sudeep Holla <sudeep.holla@arm.com> wrote:
> [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 :)

>> +       /* 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
>> +
>
>
> 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 ?

Re: [PATCH V2 5/7] cpufreq: imx6q: don't initialize opp table

[Sudeep Holla]

On 21/05/14 12:10, Viresh Kumar wrote:
> 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 <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();

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

Regards,
Sudeep

Re: [PATCH V2 5/7] cpufreq: imx6q: don't initialize opp table

[Shawn Guo]

On Wed, May 21, 2014 at 03:07:36PM +0100, Sudeep Holla wrote:
> >@@ -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.

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.

Shawn

Re: [PATCH V2 5/7] cpufreq: imx6q: don't initialize opp table

[Sudeep Holla]

On 21/05/14 15:16, Shawn Guo wrote:
> On Wed, May 21, 2014 at 03:07:36PM +0100, Sudeep Holla wrote:
>>> @@ -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.
>
> 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.
>

Ah right, I missed that. Thanks for clarifying.

Regards,
Sudeep

Re: [PATCH V2 3/7] driver/core: cpu: initialize opp table

[Sudeep Holla]

On 21/05/14 15:01, Viresh Kumar wrote:
> On 21 May 2014 19:15, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> [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.
>

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

> 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 :)
>
>>> +       /* 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
>>> +
>>
>>
>> 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 ?
>

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

Regards,
Sudeep

Re: [PATCH V2 3/7] driver/core: cpu: initialize opp table

[Viresh Kumar]

On 21 May 2014 20:12, Sudeep Holla <sudeep.holla@arm.com> wrote:
> Hmm agreed, but there are SoCs that support per CPU DVFS ;)

Lets see what's Nishanth/Rafael's view on this. I am more or less in
favor of it. But isn't a big thing. Can convert it to dbg if it annoys you :)

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

I never said that it has anything cpu specific.. What I said was that this
routine wouldn't have existed if Rafael 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. :)
But in case it is, we can add it later.

Re: [PATCH V2 3/7] driver/core: cpu: initialize opp table

[Sudeep Holla]

On 21/05/14 12:10, 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>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/base/cpu.c | 30 ++++++++++++++++++++++++++----
>   1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 006b1bc..853e99e 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"
>
> @@ -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_info(&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) {}

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.

Regards,
Sudeep

Re: [PATCH V2 3/7] driver/core: cpu: initialize opp table

[Viresh Kumar]

On 21 May 2014 22:56, Sudeep Holla <sudeep.holla@arm.com> wrote:
> 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.

x-( (That's my angry face ..)

I still added it so that the next few conditional statements (error checking)
also go away for non-DT platforms.. So, lets keep for now. We may have
more additions into it in future..