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

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

[Viresh Kumar]

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

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

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

This patchset is all about that.

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

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

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

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

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

-- 
2.0.0.rc2

[PATCH V3 1/8] cpufreq: cpufreq-cpu0: remove dependency on thermal

[Viresh Kumar]

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

Remove it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

[PATCH V3 2/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 V3 3/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 V3 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 adds another routine in cpu.c: of_init_cpu_opp_table() and calls it
right after CPU device is registered in register_cpu(). A dummy implementation
is also provided to make it lightweight for platforms that don't need it.

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..818cfe8 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_dbg(&cpu->dev, "%s: created OPP table for cpu: %d\n",
+			__func__, cpu->dev.id);
+	/* Print error only if there is an issue with OPP table */
+	else if (error != -ENOSYS && error != -ENODEV)
+		dev_err(&cpu->dev, "%s: failed to init OPP table for cpu%d, err: %d\n",
+			__func__, cpu->dev.id, error);
+}
+#else
+static inline void of_init_cpu_opp_table(struct cpu *cpu) {}
+#endif
+
 /*
  * register_cpu - Setup a sysfs device for a CPU.
  * @cpu - cpu->hotpluggable field set to 1 will generate a control file in
@@ -349,10 +369,12 @@ int register_cpu(struct cpu *cpu, int num)
 	if (cpu->hotpluggable)
 		cpu->dev.groups = hotplugable_cpu_attr_groups;
 	error = device_register(&cpu->dev);
-	if (!error)
-		per_cpu(cpu_sys_devices, num) = &cpu->dev;
-	if (!error)
-		register_cpu_under_node(num, cpu_to_node(num));
+	if (error)
+		return error;
+
+	per_cpu(cpu_sys_devices, num) = &cpu->dev;
+	register_cpu_under_node(num, cpu_to_node(num));
+	of_init_cpu_opp_table(cpu);
 
 	return error;
 }
-- 
2.0.0.rc2

[PATCH V3 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 V3 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 V3 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 V3 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 V3 1/8] cpufreq: cpufreq-cpu0: remove dependency on thermal

[Eduardo Valentin]

Hello Viresh,

On Thu, May 22, 2014 at 11:07:25AM +0530, Viresh Kumar wrote:
> cpufreq-cpu0 uses thermal framework to register a cooling device, but doesn't
> depend on it as there are dummy calls provided by thermal layer when
> CONFIG_THERMAL=n. So, we don't really need to mention thermal as a dependency
> for cpufreq-cpu0 in Kconfig.


I see your point.
> 
> Remove it.

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

> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index 1fbe11f..4310997 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -185,7 +185,7 @@ config CPU_FREQ_GOV_CONSERVATIVE
>  
>  config GENERIC_CPUFREQ_CPU0
>  	tristate "Generic CPU0 cpufreq driver"
> -	depends on HAVE_CLK && REGULATOR && OF && THERMAL && CPU_THERMAL
> +	depends on HAVE_CLK && REGULATOR && OF
>  	select PM_OPP
>  	help
>  	  This adds a generic cpufreq driver for CPU0 frequency management.
> -- 
> 2.0.0.rc2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V3 1/8] cpufreq: cpufreq-cpu0: remove dependency on thermal

[Viresh Kumar]

On 22 May 2014 20:22, Eduardo Valentin <edubezval@gmail.com> wrote:
> However, on CPUs that needs thermal managment, it makes sense to have
> such dependency, from functional perspective. Mainly because scaling
> frequency and voltage up would be allowed only when thermal management
> is enabled.

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

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

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

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

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

Re: [PATCH V3 1/8] cpufreq: cpufreq-cpu0: remove dependency on thermal

[Eduardo Valentin]

Hi Viresh,

On Fri, May 23, 2014 at 10:03:27AM +0530, Viresh Kumar wrote:
> On 22 May 2014 20:22, Eduardo Valentin <edubezval@gmail.com> wrote:
> > However, on CPUs that needs thermal managment, it makes sense to have
> > such dependency, from functional perspective. Mainly because scaling
> > frequency and voltage up would be allowed only when thermal management
> > is enabled.
> 
> AFAIK, dependencies in KCONFIG are only for fixing compilation time issues.


Actually, they also impose module loading sequencing. 

> As some APIs wouldn't be available without enabling some config options..
> 
> If drivers fail at runtime because some API returned error, fix it for your
> platform instead and not bug KCONFIG for that.

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

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

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

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

> 
> Though I still feel that this driver should still support platforms without
> regulators (atleast in software, they might always have them on board :))..
> And so dependencies for regulators may also die out one day..
> 
> The dependencies here mean: "This driver would never ever work/compile
> if the dependencies aren't met.."

Re: [PATCH V3 1/8] cpufreq: cpufreq-cpu0: remove dependency on thermal

[Viresh Kumar]

> On 23-May-2014, at 6:51 pm, Eduardo Valentin <edubezval@gmail.com> wrote:
> I agree. We need to have runtime dependency, and that is the major
> concern on my behalf. The problem of Kconfig dependency is that it
> imposes sequencing only on module loading, not at boot sequencing.
> 
> Another way around is returning -EPROBE_DEFER when some API is not ready at device probing for instance.

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

Re: [PATCH V3 1/8] cpufreq: cpufreq-cpu0: remove dependency on thermal

[Pavel Machek]

On Fri 2014-05-23 10:03:27, Viresh Kumar wrote:
> On 22 May 2014 20:22, Eduardo Valentin <edubezval@gmail.com> wrote:
> > However, on CPUs that needs thermal managment, it makes sense to have
> > such dependency, from functional perspective. Mainly because scaling
> > frequency and voltage up would be allowed only when thermal management
> > is enabled.
> 
> AFAIK, dependencies in KCONFIG are only for fixing compilation time issues.

I do not think that's correct.

Re: [PATCH V3 1/8] cpufreq: cpufreq-cpu0: remove dependency on thermal

[Viresh Kumar]

On 24 May 2014 18:29, Pavel Machek <pavel@ucw.cz> wrote:
> On Fri 2014-05-23 10:03:27, Viresh Kumar wrote:
>> On 22 May 2014 20:22, Eduardo Valentin <edubezval@gmail.com> wrote:
>> > However, on CPUs that needs thermal managment, it makes sense to have
>> > such dependency, from functional perspective. Mainly because scaling
>> > frequency and voltage up would be allowed only when thermal management
>> > is enabled.
>>
>> AFAIK, dependencies in KCONFIG are only for fixing compilation time issues.
>
> I do not think that's correct.

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

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

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

[Rafael J. Wysocki]

On Thursday, May 22, 2014 11:07:28 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 adds another routine in cpu.c: of_init_cpu_opp_table() and calls it
> right after CPU device is registered in register_cpu(). A dummy implementation
> is also provided to make it lightweight for platforms that don't need it.
> 
> 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..818cfe8 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)

Do you actually use cpu anywere in this function for anything other than
just accessing cpu->dev?  If not, why not to pass cpu->dev to it and
move it somewhere in the OPP core?

> +{
> +	int error;
> +
> +	/* Initialize CPU's OPP table */
> +	error = of_init_opp_table(&cpu->dev);
> +	if (!error)
> +		dev_dbg(&cpu->dev, "%s: created OPP table for cpu: %d\n",
> +			__func__, cpu->dev.id);
> +	/* Print error only if there is an issue with OPP table */
> +	else if (error != -ENOSYS && error != -ENODEV)
> +		dev_err(&cpu->dev, "%s: failed to init OPP table for cpu%d, err: %d\n",
> +			__func__, cpu->dev.id, error);
> +}
> +#else
> +static inline void of_init_cpu_opp_table(struct cpu *cpu) {}
> +#endif
> +
>  /*
>   * register_cpu - Setup a sysfs device for a CPU.
>   * @cpu - cpu->hotpluggable field set to 1 will generate a control file in
> @@ -349,10 +369,12 @@ int register_cpu(struct cpu *cpu, int num)
>  	if (cpu->hotpluggable)
>  		cpu->dev.groups = hotplugable_cpu_attr_groups;
>  	error = device_register(&cpu->dev);
> -	if (!error)
> -		per_cpu(cpu_sys_devices, num) = &cpu->dev;
> -	if (!error)
> -		register_cpu_under_node(num, cpu_to_node(num));
> +	if (error)
> +		return error;
> +
> +	per_cpu(cpu_sys_devices, num) = &cpu->dev;
> +	register_cpu_under_node(num, cpu_to_node(num));
> +	of_init_cpu_opp_table(cpu);
>  
>  	return error;
>  }
> 

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

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

[Viresh Kumar]

On 27 May 2014 05:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Do you actually use cpu anywere in this function for anything other than
> just accessing cpu->dev?  If not, why not to pass cpu->dev to it and
> move it somewhere in the OPP core?

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

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

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

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

[Viresh Kumar]

On 27 May 2014 05:34, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> We are also using it for cpu->dev_id, but that's not so important. This
> routine wouldn't have existed if you wouldn't have asked for it. It is
> just a wrapper over of_init_opp_table, which also has a dummy
> implementation when its not supported.
>
> So, it might not be worth enough for any other code to use it. :)
> And so I added it here.
>
> Let me know how/where do you want it and I will resend it quickly.

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

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

[Rafael J. Wysocki]

On Tuesday, May 27, 2014 05:48:27 AM Viresh Kumar wrote:
> On 27 May 2014 05:34, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > We are also using it for cpu->dev_id, but that's not so important. This
> > routine wouldn't have existed if you wouldn't have asked for it. It is
> > just a wrapper over of_init_opp_table, which also has a dummy
> > implementation when its not supported.
> >
> > So, it might not be worth enough for any other code to use it. :)
> > And so I added it here.
> >
> > Let me know how/where do you want it and I will resend it quickly.
> 
> There is another option here, add these print messages in
> of_init_opp_table() ? That's the only difference new wrapper has.

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

Rafael