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(-)
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
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
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..
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);
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; }
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
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 forcpu%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 ?
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 forcpu%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
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.
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
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..
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)
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
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
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) {
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
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
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
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);
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) {
linaro-kernel@lists.linaro.org
-
Inderpal Singh -
Shawn Guo -
Sudeep Holla -
Viresh Kumar