Hi Guys,
The aim of the series is to kill the users of cpufreq-dt's platform data, i.e. mvebu. And because that required a new API to the OPP core, this just became a mix of cpufreq and OPP patches.
The first four patches does some sort of cleanup of the OPP core to base the next patches. Fifth one sets the shared_opp flag in opp-tables (required by later patches). Sixth one adds a new API for opp-v1 users, to get the CPUs sharing an OPP table.
Seventh to tenth update cpufreq-dt and mvebu to get rid of platform data.
@Rafael: The last patch touches the compatible-list array in cpufreq-dt-platdev.c, which is also touched by '[PATCH 0/8] cpufreq: dt: Don't create platform-device from platform code'.
So, the last one has some sort of dependency on that series, otherwise all other patches can be applied cleanly.
@Arnd: Can you please look at patches 7-10 ..
Viresh Kumar (10): PM / OPP: Propagate the error returned by _find_opp_table() PM / OPP: Add missing doc style comments PM / OPP: dev_pm_opp_set_sharing_cpus() doesn't depend on CONFIG_OF PM / OPP: Relocate dev_pm_opp_set_sharing_cpus() PM / OPP: Mark shared-opp for non-dt case PM / OPP: Add dev_pm_opp_get_sharing_cpus() cpufreq: dt: Identify cpu-sharing for platforms without operating-points-v2 mvebu: Use dev_pm_opp_set_sharing_cpus() to mark OPP tables as shared cpufreq: dt: Kill platform-data cpufreq: mvebu: Use generic platdev driver
arch/arm/mach-mvebu/pmsu.c | 14 ++- drivers/base/power/opp/cpu.c | 187 +++++++++++++++++++++++++++-------- drivers/cpufreq/cpufreq-dt-platdev.c | 2 + drivers/cpufreq/cpufreq-dt.c | 22 ++--- include/linux/cpufreq-dt.h | 24 ----- include/linux/pm_opp.h | 18 ++-- 6 files changed, 176 insertions(+), 91 deletions(-) delete mode 100644 include/linux/cpufreq-dt.h
Don't send -EINVAL and propagate what's received from _find_opp_table().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c index ba2bdbd932ef..b7411a3cdcb1 100644 --- a/drivers/base/power/opp/cpu.c +++ b/drivers/base/power/opp/cpu.c @@ -131,7 +131,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask)
opp_table = _find_opp_table(cpu_dev); if (IS_ERR(opp_table)) { - ret = -EINVAL; + ret = PTR_ERR(opp_table); goto unlock; }
On 04/21, Viresh Kumar wrote:
Don't send -EINVAL and propagate what's received from _find_opp_table().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
Few of the routines in cpu.c were missing these, add them.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp/cpu.c | 59 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c index b7411a3cdcb1..b151401d1513 100644 --- a/drivers/base/power/opp/cpu.c +++ b/drivers/base/power/opp/cpu.c @@ -119,7 +119,22 @@ void dev_pm_opp_free_cpufreq_table(struct device *dev, EXPORT_SYMBOL_GPL(dev_pm_opp_free_cpufreq_table); #endif /* CONFIG_CPU_FREQ */
-/* Required only for V1 bindings, as v2 can manage it from DT itself */ +/** + * dev_pm_opp_set_sharing_cpus() - Mark OPP table as shared by few CPUs + * @cpu_dev: CPU device for which we do this operation + * @cpumask: cpumask of the CPUs which share the OPP table with @cpu_dev + * + * This marks OPP table of the @cpu_dev as shared by the CPUs present in + * @cpumask. + * + * Returns -ENODEV if OPP table isn't already present. + * + * Locking: The internal opp_table and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + */ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) { struct opp_device *opp_dev; @@ -161,6 +176,18 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) EXPORT_SYMBOL_GPL(dev_pm_opp_set_sharing_cpus);
#ifdef CONFIG_OF +/** + * dev_pm_opp_of_cpumask_remove_table() - Removes OPP table for @cpumask + * @cpumask: cpumask for which OPP table needs to be removed + * + * This removes the OPP tables for CPUs present in the @cpumask. + * + * Locking: The internal opp_table and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + */ void dev_pm_opp_of_cpumask_remove_table(cpumask_var_t cpumask) { struct device *cpu_dev; @@ -181,6 +208,18 @@ void dev_pm_opp_of_cpumask_remove_table(cpumask_var_t cpumask) } EXPORT_SYMBOL_GPL(dev_pm_opp_of_cpumask_remove_table);
+/** + * dev_pm_opp_of_cpumask_add_table() - Adds OPP table for @cpumask + * @cpumask: cpumask for which OPP table needs to be added. + * + * This adds the OPP tables for CPUs present in the @cpumask. + * + * Locking: The internal opp_table and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + */ int dev_pm_opp_of_cpumask_add_table(cpumask_var_t cpumask) { struct device *cpu_dev; @@ -216,6 +255,24 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_cpumask_add_table); * * Returns -ENOENT if operating-points-v2 bindings aren't supported. */ +/** + * dev_pm_opp_of_get_sharing_cpus() - Get cpumask of CPUs sharing OPPs with + * @cpu_dev using operating-points-v2 + * bindings. + * + * @cpu_dev: CPU device for which we do this operation + * @cpumask: cpumask to update with information of sharing CPUs + * + * This updates the @cpumask with CPUs that are sharing OPPs with @cpu_dev. + * + * Returns -ENOENT if operating-points-v2 isn't present for @cpu_dev. + * + * Locking: The internal opp_table and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + */ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) { struct device_node *np, *tmp_np;
On 04/21, Viresh Kumar wrote:
Few of the routines in cpu.c were missing these, add them.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
dev_pm_opp_set_sharing_cpus() doesn't do any DT specific stuff and its declarations are added within the CONFIG_OF ifdef by mistake. Take them out of that.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/pm_opp.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index cccaf4a29e9f..5b6ad31403a5 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -65,6 +65,7 @@ void dev_pm_opp_put_prop_name(struct device *dev); int dev_pm_opp_set_regulator(struct device *dev, const char *name); void dev_pm_opp_put_regulator(struct device *dev); int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq); +int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask); #else static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp) { @@ -178,6 +179,11 @@ static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_f return -EINVAL; }
+static inline int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) +{ + return -ENOSYS; +} + #endif /* CONFIG_PM_OPP */
#if defined(CONFIG_PM_OPP) && defined(CONFIG_OF) @@ -186,7 +192,6 @@ void dev_pm_opp_of_remove_table(struct device *dev); int dev_pm_opp_of_cpumask_add_table(cpumask_var_t cpumask); void dev_pm_opp_of_cpumask_remove_table(cpumask_var_t cpumask); int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask); -int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask); #else static inline int dev_pm_opp_of_add_table(struct device *dev) { @@ -210,11 +215,6 @@ static inline int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, cpumask { return -ENOSYS; } - -static inline int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) -{ - return -ENOSYS; -} #endif
#endif /* __LINUX_OPP_H__ */
On 04/21, Viresh Kumar wrote:
dev_pm_opp_set_sharing_cpus() doesn't do any DT specific stuff and its declarations are added within the CONFIG_OF ifdef by mistake. Take them out of that.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
Move dev_pm_opp_set_sharing_cpus() towards the end of the file. This is required for better readability after the next patch is applied, which adds dev_pm_opp_get_sharing_cpus().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp/cpu.c | 112 +++++++++++++++++++++---------------------- 1 file changed, 56 insertions(+), 56 deletions(-)
diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c index b151401d1513..491e8684bd5f 100644 --- a/drivers/base/power/opp/cpu.c +++ b/drivers/base/power/opp/cpu.c @@ -119,62 +119,6 @@ void dev_pm_opp_free_cpufreq_table(struct device *dev, EXPORT_SYMBOL_GPL(dev_pm_opp_free_cpufreq_table); #endif /* CONFIG_CPU_FREQ */
-/** - * dev_pm_opp_set_sharing_cpus() - Mark OPP table as shared by few CPUs - * @cpu_dev: CPU device for which we do this operation - * @cpumask: cpumask of the CPUs which share the OPP table with @cpu_dev - * - * This marks OPP table of the @cpu_dev as shared by the CPUs present in - * @cpumask. - * - * Returns -ENODEV if OPP table isn't already present. - * - * Locking: The internal opp_table and opp structures are RCU protected. - * Hence this function internally uses RCU updater strategy with mutex locks - * to keep the integrity of the internal data structures. Callers should ensure - * that this function is *NOT* called under RCU protection or in contexts where - * mutex cannot be locked. - */ -int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) -{ - struct opp_device *opp_dev; - struct opp_table *opp_table; - struct device *dev; - int cpu, ret = 0; - - mutex_lock(&opp_table_lock); - - opp_table = _find_opp_table(cpu_dev); - if (IS_ERR(opp_table)) { - ret = PTR_ERR(opp_table); - goto unlock; - } - - for_each_cpu(cpu, cpumask) { - if (cpu == cpu_dev->id) - continue; - - dev = get_cpu_device(cpu); - if (!dev) { - dev_err(cpu_dev, "%s: failed to get cpu%d device\n", - __func__, cpu); - continue; - } - - opp_dev = _add_opp_dev(dev, opp_table); - if (!opp_dev) { - dev_err(dev, "%s: failed to add opp-dev for cpu%d device\n", - __func__, cpu); - continue; - } - } -unlock: - mutex_unlock(&opp_table_lock); - - return ret; -} -EXPORT_SYMBOL_GPL(dev_pm_opp_set_sharing_cpus); - #ifdef CONFIG_OF /** * dev_pm_opp_of_cpumask_remove_table() - Removes OPP table for @cpumask @@ -326,3 +270,59 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask } EXPORT_SYMBOL_GPL(dev_pm_opp_of_get_sharing_cpus); #endif + +/** + * dev_pm_opp_set_sharing_cpus() - Mark OPP table as shared by few CPUs + * @cpu_dev: CPU device for which we do this operation + * @cpumask: cpumask of the CPUs which share the OPP table with @cpu_dev + * + * This marks OPP table of the @cpu_dev as shared by the CPUs present in + * @cpumask. + * + * Returns -ENODEV if OPP table isn't already present. + * + * Locking: The internal opp_table and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + */ +int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) +{ + struct opp_device *opp_dev; + struct opp_table *opp_table; + struct device *dev; + int cpu, ret = 0; + + mutex_lock(&opp_table_lock); + + opp_table = _find_opp_table(cpu_dev); + if (IS_ERR(opp_table)) { + ret = PTR_ERR(opp_table); + goto unlock; + } + + for_each_cpu(cpu, cpumask) { + if (cpu == cpu_dev->id) + continue; + + dev = get_cpu_device(cpu); + if (!dev) { + dev_err(cpu_dev, "%s: failed to get cpu%d device\n", + __func__, cpu); + continue; + } + + opp_dev = _add_opp_dev(dev, opp_table); + if (!opp_dev) { + dev_err(dev, "%s: failed to add opp-dev for cpu%d device\n", + __func__, cpu); + continue; + } + } +unlock: + mutex_unlock(&opp_table_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(dev_pm_opp_set_sharing_cpus);
On 04/21, Viresh Kumar wrote:
Move dev_pm_opp_set_sharing_cpus() towards the end of the file. This is required for better readability after the next patch is applied, which adds dev_pm_opp_get_sharing_cpus().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
Although the next patch is not the one that needs this...
opp core allows OPPs to be explicitly marked as shared from platform code, in case of operating-point v1 bindings.
Though we do everything fine in that case, we don't set the flag in the opp-table to indicate that the OPPs are shared. It works fine today as the flag isn't used anywhere else in the core, but we should be doing the right thing by marking it set.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp/cpu.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c index 491e8684bd5f..55cbf9bd8707 100644 --- a/drivers/base/power/opp/cpu.c +++ b/drivers/base/power/opp/cpu.c @@ -319,6 +319,9 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) __func__, cpu); continue; } + + /* Mark opp-table as multiple CPUs are sharing it now */ + opp_table->shared_opp = true; } unlock: mutex_unlock(&opp_table_lock);
On 04/21, Viresh Kumar wrote:
opp core allows OPPs to be explicitly marked as shared from platform code, in case of operating-point v1 bindings.
Though we do everything fine in that case, we don't set the flag in the opp-table to indicate that the OPPs are shared. It works fine today as the flag isn't used anywhere else in the core, but we should be doing the right thing by marking it set.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
OPP core allows a platform to mark OPP table as shared, when the platform isn't using operating-points-v2 bindings.
And, so there should be a non DT way of finding out if the OPP table is shared or not.
This patch adds dev_pm_opp_get_sharing_cpus(), which first tries to get OPP sharing information from the opp-table (in case it is already marked as shared), otherwise it uses the existing DT way of finding sharing information.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 6 ++++++ 2 files changed, 51 insertions(+)
diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c index 55cbf9bd8707..9c4eb90759fb 100644 --- a/drivers/base/power/opp/cpu.c +++ b/drivers/base/power/opp/cpu.c @@ -329,3 +329,48 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) return ret; } EXPORT_SYMBOL_GPL(dev_pm_opp_set_sharing_cpus); + +/** + * dev_pm_opp_get_sharing_cpus() - Get cpumask of CPUs sharing OPPs with @cpu_dev + * @cpu_dev: CPU device for which we do this operation + * @cpumask: cpumask to update with information of sharing CPUs + * + * This updates the @cpumask with CPUs that are sharing OPPs with @cpu_dev. + * + * Returns -ENODEV if OPP table isn't already present. + * + * Locking: The internal opp_table and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + */ +int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) +{ + struct opp_device *opp_dev; + struct opp_table *opp_table; + int ret = 0; + + mutex_lock(&opp_table_lock); + + opp_table = _find_opp_table(cpu_dev); + if (IS_ERR(opp_table)) { + ret = PTR_ERR(opp_table); + goto unlock; + } + + cpumask_clear(cpumask); + + if (opp_table->shared_opp) { + list_for_each_entry(opp_dev, &opp_table->dev_list, node) + cpumask_set_cpu(opp_dev->dev->id, cpumask); + } else { + cpumask_set_cpu(cpu_dev->id, cpumask); + } + +unlock: + mutex_unlock(&opp_table_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(dev_pm_opp_get_sharing_cpus); diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 5b6ad31403a5..d6effc931013 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -66,6 +66,7 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name); void dev_pm_opp_put_regulator(struct device *dev); int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq); int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask); +int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask); #else static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp) { @@ -184,6 +185,11 @@ static inline int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_va return -ENOSYS; }
+static inline int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) +{ + return -ENOSYS; +} + #endif /* CONFIG_PM_OPP */
#if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
On 04/21, Viresh Kumar wrote:
diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c index 55cbf9bd8707..9c4eb90759fb 100644 --- a/drivers/base/power/opp/cpu.c +++ b/drivers/base/power/opp/cpu.c @@ -329,3 +329,48 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) return ret; } EXPORT_SYMBOL_GPL(dev_pm_opp_set_sharing_cpus);
+/**
- dev_pm_opp_get_sharing_cpus() - Get cpumask of CPUs sharing OPPs with @cpu_dev
- @cpu_dev: CPU device for which we do this operation
- @cpumask: cpumask to update with information of sharing CPUs
- This updates the @cpumask with CPUs that are sharing OPPs with @cpu_dev.
- Returns -ENODEV if OPP table isn't already present.
- Locking: The internal opp_table and opp structures are RCU protected.
- Hence this function internally uses RCU updater strategy with mutex locks
- to keep the integrity of the internal data structures. Callers should ensure
- that this function is *NOT* called under RCU protection or in contexts where
- mutex cannot be locked.
- */
+int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask)
Is there a reason we use cpumask_var_t instead of struct cpumask * here? My understanding is that cpumask_var_t is for stack declarations.
On 22-04-16, 15:21, Stephen Boyd wrote:
On 04/21, Viresh Kumar wrote:
diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c index 55cbf9bd8707..9c4eb90759fb 100644 --- a/drivers/base/power/opp/cpu.c +++ b/drivers/base/power/opp/cpu.c @@ -329,3 +329,48 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) return ret; } EXPORT_SYMBOL_GPL(dev_pm_opp_set_sharing_cpus);
+/**
- dev_pm_opp_get_sharing_cpus() - Get cpumask of CPUs sharing OPPs with @cpu_dev
- @cpu_dev: CPU device for which we do this operation
- @cpumask: cpumask to update with information of sharing CPUs
- This updates the @cpumask with CPUs that are sharing OPPs with @cpu_dev.
- Returns -ENODEV if OPP table isn't already present.
- Locking: The internal opp_table and opp structures are RCU protected.
- Hence this function internally uses RCU updater strategy with mutex locks
- to keep the integrity of the internal data structures. Callers should ensure
- that this function is *NOT* called under RCU protection or in contexts where
- mutex cannot be locked.
- */
+int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask)
Is there a reason we use cpumask_var_t instead of struct cpumask * here? My understanding is that cpumask_var_t is for stack declarations.
Just because we have been using that *consistently* everywhere in cpufreq and OPP core.
I am fine with cpumask * as well, but we should be consistent.
So, I will keep it cpumask_var_t for this patch, and lets see if we want to change all occurrences of the same in cpufreq and OPP core.
Sounds reasonable ?
Existing platforms, which do not support operating-points-v2, can explicitly tell the opp core that some of the CPUs share opp tables, with help of dev_pm_opp_set_sharing_cpus().
For such platforms, explicitly ask the opp core to provide list of CPUs sharing the opp table with current cpu device, before falling back to platform data.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-dt.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 5f8dbe640a20..aca9bec00f91 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -147,7 +147,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) struct clk *cpu_clk; struct dev_pm_opp *suspend_opp; unsigned int transition_latency; - bool opp_v1 = false; + bool fallback = false; const char *name; int ret;
@@ -167,14 +167,16 @@ static int cpufreq_init(struct cpufreq_policy *policy) /* Get OPP-sharing information from "operating-points-v2" bindings */ ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, policy->cpus); if (ret) { + if (ret != -ENOENT) + goto out_put_clk; + /* * operating-points-v2 not supported, fallback to old method of - * finding shared-OPPs for backward compatibility. + * finding shared-OPPs for backward compatibility if the + * platform hasn't set sharing CPUs. */ - if (ret == -ENOENT) - opp_v1 = true; - else - goto out_put_clk; + if (dev_pm_opp_get_sharing_cpus(cpu_dev, policy->cpus)) + fallback = true; }
/* @@ -214,7 +216,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) goto out_free_opp; }
- if (opp_v1) { + if (fallback) { struct cpufreq_dt_platform_data *pd = cpufreq_get_driver_data();
if (!pd || !pd->independent_clocks)
On 04/21, Viresh Kumar wrote:
@@ -167,14 +167,16 @@ static int cpufreq_init(struct cpufreq_policy *policy) /* Get OPP-sharing information from "operating-points-v2" bindings */ ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, policy->cpus); if (ret) {
if (ret != -ENOENT)goto out_put_clk;- /*
- operating-points-v2 not supported, fallback to old method of
* finding shared-OPPs for backward compatibility.
* finding shared-OPPs for backward compatibility if the* platform hasn't set sharing CPUs.
if (ret == -ENOENT)opp_v1 = true;elsegoto out_put_clk;
if (dev_pm_opp_get_sharing_cpus(cpu_dev, policy->cpus))fallback = true;
I'm sort of lost, we make the same call twice here. Why would the return value change between the first time and the second?
On 22-04-16, 15:27, Stephen Boyd wrote:
On 04/21, Viresh Kumar wrote:
@@ -167,14 +167,16 @@ static int cpufreq_init(struct cpufreq_policy *policy) /* Get OPP-sharing information from "operating-points-v2" bindings */ ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, policy->cpus); if (ret) {
if (ret != -ENOENT)goto out_put_clk;- /*
- operating-points-v2 not supported, fallback to old method of
* finding shared-OPPs for backward compatibility.
* finding shared-OPPs for backward compatibility if the* platform hasn't set sharing CPUs.
if (ret == -ENOENT)opp_v1 = true;elsegoto out_put_clk;
if (dev_pm_opp_get_sharing_cpus(cpu_dev, policy->cpus))fallback = true;I'm sort of lost, we make the same call twice here. Why would the return value change between the first time and the second?
Two different APIs, which look similar :)
The first one tries to find the sharing-cpus relation from DT, the other one is for v1 bindings and finds it due to platform code dev_pm_opp_set_sharing_cpus() call.
On 04/25, Viresh Kumar wrote:
On 22-04-16, 15:27, Stephen Boyd wrote:
On 04/21, Viresh Kumar wrote:
@@ -167,14 +167,16 @@ static int cpufreq_init(struct cpufreq_policy *policy) /* Get OPP-sharing information from "operating-points-v2" bindings */ ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, policy->cpus);
[..]
if (dev_pm_opp_get_sharing_cpus(cpu_dev, policy->cpus))fallback = true;I'm sort of lost, we make the same call twice here. Why would the return value change between the first time and the second?
Two different APIs, which look similar :)
The first one tries to find the sharing-cpus relation from DT, the other one is for v1 bindings and finds it due to platform code dev_pm_opp_set_sharing_cpus() call.
Ah thanks. My eyes glossed over the "of" part. Sounds fine.
On Mon, Apr 25, 2016 at 11:45 PM, Stephen Boyd sboyd@codeaurora.org wrote:
On 04/25, Viresh Kumar wrote:
On 22-04-16, 15:27, Stephen Boyd wrote:
On 04/21, Viresh Kumar wrote:
@@ -167,14 +167,16 @@ static int cpufreq_init(struct cpufreq_policy *policy) /* Get OPP-sharing information from "operating-points-v2" bindings */ ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, policy->cpus);
[..]
if (dev_pm_opp_get_sharing_cpus(cpu_dev, policy->cpus))fallback = true;I'm sort of lost, we make the same call twice here. Why would the return value change between the first time and the second?
Two different APIs, which look similar :)
The first one tries to find the sharing-cpus relation from DT, the other one is for v1 bindings and finds it due to platform code dev_pm_opp_set_sharing_cpus() call.
Ah thanks. My eyes glossed over the "of" part. Sounds fine.
So that would be an "ACK", right?
On 04/25, Rafael J. Wysocki wrote:
On Mon, Apr 25, 2016 at 11:45 PM, Stephen Boyd sboyd@codeaurora.org wrote:
On 04/25, Viresh Kumar wrote:
On 22-04-16, 15:27, Stephen Boyd wrote:
On 04/21, Viresh Kumar wrote:
@@ -167,14 +167,16 @@ static int cpufreq_init(struct cpufreq_policy *policy) /* Get OPP-sharing information from "operating-points-v2" bindings */ ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, policy->cpus);
[..]
if (dev_pm_opp_get_sharing_cpus(cpu_dev, policy->cpus))fallback = true;I'm sort of lost, we make the same call twice here. Why would the return value change between the first time and the second?
Two different APIs, which look similar :)
The first one tries to find the sharing-cpus relation from DT, the other one is for v1 bindings and finds it due to platform code dev_pm_opp_set_sharing_cpus() call.
Ah thanks. My eyes glossed over the "of" part. Sounds fine.
So that would be an "ACK", right?
Sure, I thought this was going for another round though.
I had to go back and re-read the patch once more, but you can have my reviewed-by on this one too.
On Monday, April 25, 2016 02:56:08 PM Stephen Boyd wrote:
On 04/25, Rafael J. Wysocki wrote:
On Mon, Apr 25, 2016 at 11:45 PM, Stephen Boyd sboyd@codeaurora.org wrote:
On 04/25, Viresh Kumar wrote:
On 22-04-16, 15:27, Stephen Boyd wrote:
On 04/21, Viresh Kumar wrote:
@@ -167,14 +167,16 @@ static int cpufreq_init(struct cpufreq_policy *policy) /* Get OPP-sharing information from "operating-points-v2" bindings */ ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, policy->cpus);
[..]
if (dev_pm_opp_get_sharing_cpus(cpu_dev, policy->cpus))fallback = true;I'm sort of lost, we make the same call twice here. Why would the return value change between the first time and the second?
Two different APIs, which look similar :)
The first one tries to find the sharing-cpus relation from DT, the other one is for v1 bindings and finds it due to platform code dev_pm_opp_set_sharing_cpus() call.
Ah thanks. My eyes glossed over the "of" part. Sounds fine.
So that would be an "ACK", right?
Sure, I thought this was going for another round though.
OK
I had to go back and re-read the patch once more, but you can have my reviewed-by on this one too.
Well, I'm still unsure what about the [6/10].
I have applied [1-5/10] for now and I'll be expecting updates or resends of the rest.
Thanks, Rafael
That will allow us to avoid using cpufreq-dt platform data.
Cc: Thomas Petazzoni thomas.petazzoni@free-electrons.com Cc: Jason Cooper jason@lakedaemon.net Cc: Andrew Lunn andrew@lunn.ch Cc: Gregory Clement gregory.clement@free-electrons.com Cc: Sebastian Hesselbarth sebastian.hesselbarth@gmail.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/arm/mach-mvebu/pmsu.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c index ed8fda4cd055..79d0a5d9da8e 100644 --- a/arch/arm/mach-mvebu/pmsu.c +++ b/arch/arm/mach-mvebu/pmsu.c @@ -20,7 +20,6 @@
#include <linux/clk.h> #include <linux/cpu_pm.h> -#include <linux/cpufreq-dt.h> #include <linux/delay.h> #include <linux/init.h> #include <linux/io.h> @@ -609,10 +608,6 @@ int mvebu_pmsu_dfs_request(int cpu) return 0; }
-struct cpufreq_dt_platform_data cpufreq_dt_pd = { - .independent_clocks = true, -}; - static int __init armada_xp_pmsu_cpufreq_init(void) { struct device_node *np; @@ -683,10 +678,15 @@ static int __init armada_xp_pmsu_cpufreq_init(void) clk_put(clk); return ret; } + + ret = dev_pm_opp_set_sharing_cpus(cpu_dev, + (struct cpumask *) cpumask_of(cpu_dev->id)); + if (ret) + dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", + __func__, ret); }
- platform_device_register_data(NULL, "cpufreq-dt", -1, - &cpufreq_dt_pd, sizeof(cpufreq_dt_pd)); + platform_device_register_simple("cpufreq-dt", -1, NULL, 0); return 0; }
On 04/21, Viresh Kumar wrote:
@@ -683,10 +678,15 @@ static int __init armada_xp_pmsu_cpufreq_init(void) clk_put(clk); return ret; }
ret = dev_pm_opp_set_sharing_cpus(cpu_dev,(struct cpumask *) cpumask_of(cpu_dev->id));
This cast doesn't look good. Why are we casting away const? Shouldn't dev_pm_opp_set_sharing_cpus() take a const mask anyway?
There are no more users of platform-data for cpufreq-dt driver, get rid of it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-dt.c | 6 +----- include/linux/cpufreq-dt.h | 24 ------------------------ 2 files changed, 1 insertion(+), 29 deletions(-) delete mode 100644 include/linux/cpufreq-dt.h
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index aca9bec00f91..3957de801ae8 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -15,7 +15,6 @@ #include <linux/cpu.h> #include <linux/cpu_cooling.h> #include <linux/cpufreq.h> -#include <linux/cpufreq-dt.h> #include <linux/cpumask.h> #include <linux/err.h> #include <linux/module.h> @@ -217,10 +216,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) }
if (fallback) { - struct cpufreq_dt_platform_data *pd = cpufreq_get_driver_data(); - - if (!pd || !pd->independent_clocks) - cpumask_setall(policy->cpus); + cpumask_setall(policy->cpus);
/* * OPP tables are initialized only for policy->cpu, do it for diff --git a/include/linux/cpufreq-dt.h b/include/linux/cpufreq-dt.h deleted file mode 100644 index a87335a1660c..000000000000 --- a/include/linux/cpufreq-dt.h +++ /dev/null @@ -1,24 +0,0 @@ -/* - * Copyright (C) 2014 Marvell - * Thomas Petazzoni thomas.petazzoni@free-electrons.com - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 as - * published by the Free Software Foundation. - */ - -#ifndef __CPUFREQ_DT_H__ -#define __CPUFREQ_DT_H__ - -#include <linux/types.h> - -struct cpufreq_dt_platform_data { - /* - * True when each CPU has its own clock to control its - * frequency, false when all CPUs are controlled by a single - * clock. - */ - bool independent_clocks; -}; - -#endif /* __CPUFREQ_DT_H__ */
On 04/21, Viresh Kumar wrote:
There are no more users of platform-data for cpufreq-dt driver, get rid of it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
The cpufreq-dt-platdev driver supports creation of cpufreq-dt platform device now, reuse that and remove similar code from platform code.
Cc: Thomas Petazzoni thomas.petazzoni@free-electrons.com Cc: Jason Cooper jason@lakedaemon.net Cc: Andrew Lunn andrew@lunn.ch Cc: Gregory Clement gregory.clement@free-electrons.com Cc: Sebastian Hesselbarth sebastian.hesselbarth@gmail.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/arm/mach-mvebu/pmsu.c | 2 -- drivers/cpufreq/cpufreq-dt-platdev.c | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c index 79d0a5d9da8e..f24f46776fbb 100644 --- a/arch/arm/mach-mvebu/pmsu.c +++ b/arch/arm/mach-mvebu/pmsu.c @@ -685,8 +685,6 @@ static int __init armada_xp_pmsu_cpufreq_init(void) dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", __func__, ret); } - - platform_device_register_simple("cpufreq-dt", -1, NULL, 0); return 0; }
diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c index 69b2a222c84e..5704a92c52dc 100644 --- a/drivers/cpufreq/cpufreq-dt-platdev.c +++ b/drivers/cpufreq/cpufreq-dt-platdev.c @@ -33,6 +33,8 @@ static const struct of_device_id machines[] = {
{ .compatible = "marvell,berlin", },
+ { .compatible = "marvell,armadaxp", }, + { .compatible = "samsung,exynos3250", }, { .compatible = "samsung,exynos4210", }, { .compatible = "samsung,exynos4212", },
On Thursday 21 April 2016 14:29:02 Viresh Kumar wrote:
diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c index 79d0a5d9da8e..f24f46776fbb 100644 --- a/arch/arm/mach-mvebu/pmsu.c +++ b/arch/arm/mach-mvebu/pmsu.c @@ -685,8 +685,6 @@ static int __init armada_xp_pmsu_cpufreq_init(void) dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", __func__, ret); }
platform_device_register_simple("cpufreq-dt", -1, NULL, 0); return 0;} diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c index 69b2a222c84e..5704a92c52dc 100644 --- a/drivers/cpufreq/cpufreq-dt-platdev.c +++ b/drivers/cpufreq/cpufreq-dt-platdev.c @@ -33,6 +33,8 @@ static const struct of_device_id machines[] = { { .compatible = "marvell,berlin", },
{ .compatible = "marvell,armadaxp", },{ .compatible = "samsung,exynos3250", }, { .compatible = "samsung,exynos4210", }, { .compatible = "samsung,exynos4212", },
I think to make it clear that the ordering is significant here, I would leave this platform_device_register_simple() in armada_xp_pmsu_cpufreq_init().
However, it might be helpful to move that function into a new file in drivers/cpufreq/ if that works.
Arnd
On 23-04-16, 00:42, Arnd Bergmann wrote:
On Thursday 21 April 2016 14:29:02 Viresh Kumar wrote:
diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c index 79d0a5d9da8e..f24f46776fbb 100644 --- a/arch/arm/mach-mvebu/pmsu.c +++ b/arch/arm/mach-mvebu/pmsu.c @@ -685,8 +685,6 @@ static int __init armada_xp_pmsu_cpufreq_init(void) dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", __func__, ret); }
platform_device_register_simple("cpufreq-dt", -1, NULL, 0); return 0;} diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c index 69b2a222c84e..5704a92c52dc 100644 --- a/drivers/cpufreq/cpufreq-dt-platdev.c +++ b/drivers/cpufreq/cpufreq-dt-platdev.c @@ -33,6 +33,8 @@ static const struct of_device_id machines[] = { { .compatible = "marvell,berlin", },
{ .compatible = "marvell,armadaxp", },{ .compatible = "samsung,exynos3250", }, { .compatible = "samsung,exynos4210", }, { .compatible = "samsung,exynos4212", },I think to make it clear that the ordering is significant here, I would leave this platform_device_register_simple() in armada_xp_pmsu_cpufreq_init().
However, it might be helpful to move that function into a new file in drivers/cpufreq/ if that works.
We just can't get wrong with the ordering here, as this is done from device_initcall() and so that point is out of question.
The other thing that can happen is that armada_xp_pmsu_cpufreq_init() call can fail. In that case, most of the times cpufreq-dt ->init() will fail as well, so even that is fine for me.
And, so I think we can keep this patch as is.
Do you agree ?
On Monday 25 April 2016 08:30:41 Viresh Kumar wrote:
On 23-04-16, 00:42, Arnd Bergmann wrote:
On Thursday 21 April 2016 14:29:02 Viresh Kumar wrote:
diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c index 79d0a5d9da8e..f24f46776fbb 100644 --- a/arch/arm/mach-mvebu/pmsu.c +++ b/arch/arm/mach-mvebu/pmsu.c @@ -685,8 +685,6 @@ static int __init armada_xp_pmsu_cpufreq_init(void) dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", __func__, ret); }
platform_device_register_simple("cpufreq-dt", -1, NULL, 0); return 0;} diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c index 69b2a222c84e..5704a92c52dc 100644 --- a/drivers/cpufreq/cpufreq-dt-platdev.c +++ b/drivers/cpufreq/cpufreq-dt-platdev.c @@ -33,6 +33,8 @@ static const struct of_device_id machines[] = { { .compatible = "marvell,berlin", },
{ .compatible = "marvell,armadaxp", },{ .compatible = "samsung,exynos3250", }, { .compatible = "samsung,exynos4210", }, { .compatible = "samsung,exynos4212", },I think to make it clear that the ordering is significant here, I would leave this platform_device_register_simple() in armada_xp_pmsu_cpufreq_init().
However, it might be helpful to move that function into a new file in drivers/cpufreq/ if that works.
We just can't get wrong with the ordering here, as this is done from device_initcall() and so that point is out of question.
I realize that the ordering is fixed through the way that the kernel is linked, my worry is more about someone changing the code in some way because it's not obvious from reading the code that the dependency exists. If either the armada_xp_pmsu_cpufreq_init() initcall gets changed so it does not always get called, or the cpufreq_dt_platdev_init() initcall gets changed so it comes a little earlier, things will break.
The other thing that can happen is that armada_xp_pmsu_cpufreq_init() call can fail. In that case, most of the times cpufreq-dt ->init() will fail as well, so even that is fine for me.
And, so I think we can keep this patch as is.
What are the downsides of moving armada_xp_pmsu_cpufreq_init() into drivers/cpufreq?
Arnd
On 25-04-16, 14:53, Arnd Bergmann wrote:
On Monday 25 April 2016 08:30:41 Viresh Kumar wrote:
I realize that the ordering is fixed through the way that the kernel is linked, my worry is more about someone changing the code in some way because it's not obvious from reading the code that the dependency exists. If either the armada_xp_pmsu_cpufreq_init() initcall gets changed so it does not always get called, or the cpufreq_dt_platdev_init() initcall gets changed so it comes a little earlier, things will break.
cpufreq-dt will just error out in that case, because it wouldn't find any OPPs registered to the OPP-core. It *shouldn't* crash and if it does, then we have a problem to fix.
The other thing that can happen is that armada_xp_pmsu_cpufreq_init() call can fail. In that case, most of the times cpufreq-dt ->init() will fail as well, so even that is fine for me.
And, so I think we can keep this patch as is.
What are the downsides of moving armada_xp_pmsu_cpufreq_init() into drivers/cpufreq?
More special code :)
On Monday 25 April 2016 18:26:05 Viresh Kumar wrote:
On 25-04-16, 14:53, Arnd Bergmann wrote:
On Monday 25 April 2016 08:30:41 Viresh Kumar wrote:
I realize that the ordering is fixed through the way that the kernel is linked, my worry is more about someone changing the code in some way because it's not obvious from reading the code that the dependency exists. If either the armada_xp_pmsu_cpufreq_init() initcall gets changed so it does not always get called, or the cpufreq_dt_platdev_init() initcall gets changed so it comes a little earlier, things will break.
cpufreq-dt will just error out in that case, because it wouldn't find any OPPs registered to the OPP-core. It *shouldn't* crash and if it does, then we have a problem to fix.
Ok.
The other thing that can happen is that armada_xp_pmsu_cpufreq_init() call can fail. In that case, most of the times cpufreq-dt ->init() will fail as well, so even that is fine for me.
And, so I think we can keep this patch as is.
What are the downsides of moving armada_xp_pmsu_cpufreq_init() into drivers/cpufreq?
More special code :)
Of course the special code still exists, it seems more like neither of us wants to have it in the portion of the kernel that he maintains ;-)
Maybe the mvebu maintainers have a preference where they'd like the code to be, they are the ones that are most impacted if anything goes wrong.
Arnd
On 25-04-16, 17:26, Arnd Bergmann wrote:
On Monday 25 April 2016 18:26:05 Viresh Kumar wrote:
On 25-04-16, 14:53, Arnd Bergmann wrote:
On Monday 25 April 2016 08:30:41 Viresh Kumar wrote:
I realize that the ordering is fixed through the way that the kernel is linked, my worry is more about someone changing the code in some way because it's not obvious from reading the code that the dependency exists. If either the armada_xp_pmsu_cpufreq_init() initcall gets changed so it does not always get called, or the cpufreq_dt_platdev_init() initcall gets changed so it comes a little earlier, things will break.
cpufreq-dt will just error out in that case, because it wouldn't find any OPPs registered to the OPP-core. It *shouldn't* crash and if it does, then we have a problem to fix.
Ok.
The other thing that can happen is that armada_xp_pmsu_cpufreq_init() call can fail. In that case, most of the times cpufreq-dt ->init() will fail as well, so even that is fine for me.
And, so I think we can keep this patch as is.
What are the downsides of moving armada_xp_pmsu_cpufreq_init() into drivers/cpufreq?
More special code :)
Of course the special code still exists, it seems more like neither of us wants to have it in the portion of the kernel that he maintains ;-)
Hehe.. But after $subject patch, we don't have any special code for creating the device, isn't it?
Maybe the mvebu maintainers have a preference where they'd like the code to be, they are the ones that are most impacted if anything goes wrong.
What code are you talking about? Initializing the OPPs or adding the cpufreq-dt device? The first one (or whatever is left now in that function) can stay anywhere, even as a cpufreq driver. I was talking about the fact that we don't have a sequence problem to solve here.
On Monday 25 April 2016 20:59:14 Viresh Kumar wrote:
On 25-04-16, 17:26, Arnd Bergmann wrote:
On Monday 25 April 2016 18:26:05 Viresh Kumar wrote:
On 25-04-16, 14:53, Arnd Bergmann wrote:
What are the downsides of moving armada_xp_pmsu_cpufreq_init() into drivers/cpufreq?
More special code :)
Of course the special code still exists, it seems more like neither of us wants to have it in the portion of the kernel that he maintains ;-)
Hehe.. But after $subject patch, we don't have any special code for creating the device, isn't it?
Maybe the mvebu maintainers have a preference where they'd like the code to be, they are the ones that are most impacted if anything goes wrong.
What code are you talking about? Initializing the OPPs or adding the cpufreq-dt device? The first one (or whatever is left now in that function) can stay anywhere, even as a cpufreq driver. I was talking about the fact that we don't have a sequence problem to solve here.
My line of thinking was that the armada_xp_pmsu_cpufreq_init() function makes sense by itself and feels like it should be one file in drivers/cpufreq, including the creation of the device.
Even without the argument of the sequencing, they two parts sort of belong together because the cpufreq-dt driver depends on both of them being run before it can function. It's also the same amount of code, as you are replacing one line in armada_xp_pmsu_cpufreq_init with one line in "struct of_device_id machines".
It's not really that important, just a feeling I had that it could be done better. Unless the mvebu maintainers feel strongly about it, just do as you prefer.
Arnd
Hello,
On Mon, 25 Apr 2016 17:46:53 +0200, Arnd Bergmann wrote:
What code are you talking about? Initializing the OPPs or adding the cpufreq-dt device? The first one (or whatever is left now in that function) can stay anywhere, even as a cpufreq driver. I was talking about the fact that we don't have a sequence problem to solve here.
My line of thinking was that the armada_xp_pmsu_cpufreq_init() function makes sense by itself and feels like it should be one file in drivers/cpufreq, including the creation of the device.
Even without the argument of the sequencing, they two parts sort of belong together because the cpufreq-dt driver depends on both of them being run before it can function. It's also the same amount of code, as you are replacing one line in armada_xp_pmsu_cpufreq_init with one line in "struct of_device_id machines".
It's not really that important, just a feeling I had that it could be done better. Unless the mvebu maintainers feel strongly about it, just do as you prefer.
As a mvebu folk, I don't really have a strong opinion on this. We also have some cpufreq device registration code in arch/arm/mach-mvebu/kirkwood.c for the older Kirkwood platform, though this one uses a custom cpufreq driver and not the generic cpufreq-dt driver.
Ideally, in the grand direction of removing as much things as possible from mach-<foo> directories, it would be great to move such initializations somewhere else. But cpufreq is not by far not the only reason why we still have code in mach-<foo>, at least in the mvebu land.
Thomas
On Thursday, April 21, 2016 02:28:52 PM Viresh Kumar wrote:
Hi Guys,
The aim of the series is to kill the users of cpufreq-dt's platform data, i.e. mvebu. And because that required a new API to the OPP core, this just became a mix of cpufreq and OPP patches.
The first four patches does some sort of cleanup of the OPP core to base the next patches. Fifth one sets the shared_opp flag in opp-tables (required by later patches). Sixth one adds a new API for opp-v1 users, to get the CPUs sharing an OPP table.
Seventh to tenth update cpufreq-dt and mvebu to get rid of platform data.
@Rafael: The last patch touches the compatible-list array in cpufreq-dt-platdev.c, which is also touched by '[PATCH 0/8] cpufreq: dt: Don't create platform-device from platform code'.
So, the last one has some sort of dependency on that series, otherwise all other patches can be applied cleanly.
@Arnd: Can you please look at patches 7-10 ..
Viresh Kumar (10): PM / OPP: Propagate the error returned by _find_opp_table() PM / OPP: Add missing doc style comments PM / OPP: dev_pm_opp_set_sharing_cpus() doesn't depend on CONFIG_OF PM / OPP: Relocate dev_pm_opp_set_sharing_cpus() PM / OPP: Mark shared-opp for non-dt case PM / OPP: Add dev_pm_opp_get_sharing_cpus() cpufreq: dt: Identify cpu-sharing for platforms without operating-points-v2 mvebu: Use dev_pm_opp_set_sharing_cpus() to mark OPP tables as shared cpufreq: dt: Kill platform-data cpufreq: mvebu: Use generic platdev driver
arch/arm/mach-mvebu/pmsu.c | 14 ++- drivers/base/power/opp/cpu.c | 187 +++++++++++++++++++++++++++-------- drivers/cpufreq/cpufreq-dt-platdev.c | 2 + drivers/cpufreq/cpufreq-dt.c | 22 ++--- include/linux/cpufreq-dt.h | 24 ----- include/linux/pm_opp.h | 18 ++-- 6 files changed, 176 insertions(+), 91 deletions(-) delete mode 100644 include/linux/cpufreq-dt.h
As usual, I'd like to get some ACKs for the OPP stuff from Stephen or other people with vested interest.
Thanks, Rafael
Some of the routines have use -ENOSYS, which is supposed to be used only for syscalls. Replace that with -EINVAL.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- I am including this patch into the series and this one will be the first patch of the series. Also, later patches will be updated to *not* use -ENOSYS.
I will send out the series again once some sort of reviews are done.
include/linux/pm_opp.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index cccaf4a29e9f..2605ed66f1bd 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -199,7 +199,7 @@ static inline void dev_pm_opp_of_remove_table(struct device *dev)
static inline int dev_pm_opp_of_cpumask_add_table(cpumask_var_t cpumask) { - return -ENOSYS; + return -EINVAL; }
static inline void dev_pm_opp_of_cpumask_remove_table(cpumask_var_t cpumask) @@ -208,12 +208,12 @@ static inline void dev_pm_opp_of_cpumask_remove_table(cpumask_var_t cpumask)
static inline int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) { - return -ENOSYS; + return -EINVAL; }
static inline int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) { - return -ENOSYS; + return -EINVAL; } #endif
On Friday, April 22, 2016 08:46:51 AM Viresh Kumar wrote:
Some of the routines have use -ENOSYS, which is supposed to be used only for syscalls. Replace that with -EINVAL.
-EINVAL specifically means "invalid argument".
What about using -ENXIO instead?
On Fri, 22 Apr 2016 14:42:31 +0200 "Rafael J. Wysocki" rjw@rjwysocki.net wrote:
On Friday, April 22, 2016 08:46:51 AM Viresh Kumar wrote:
Some of the routines have use -ENOSYS, which is supposed to be used only for syscalls. Replace that with -EINVAL.
-EINVAL specifically means "invalid argument".
What about using -ENXIO instead?
That specifically means "device not present", but might be reasonable. Quite a bit of the kernel uses EOPNOTSUPP (operation not supported).
Before you change it though please check how existing userspace does error handling. It's nice to use more "correct" error codes, but that's not sufficient reason if it turns out that existing user space checks for ENOSYS for example.
Alan
On 22-04-16, 15:59, One Thousand Gnomes wrote:
On Fri, 22 Apr 2016 14:42:31 +0200 "Rafael J. Wysocki" rjw@rjwysocki.net wrote:
On Friday, April 22, 2016 08:46:51 AM Viresh Kumar wrote:
Some of the routines have use -ENOSYS, which is supposed to be used only for syscalls. Replace that with -EINVAL.
-EINVAL specifically means "invalid argument".
What about using -ENXIO instead?
That specifically means "device not present", but might be reasonable. Quite a bit of the kernel uses EOPNOTSUPP (operation not supported).
That looks reasonable to me.. Will switch to that.
Before you change it though please check how existing userspace does error handling. It's nice to use more "correct" error codes, but that's not sufficient reason if it turns out that existing user space checks for ENOSYS for example.
Userspace doesn't interact directly with this stuff, its pretty much within the kernel. So it should be fine.
Thanks Alan.
On Thursday 21 April 2016 14:28:52 Viresh Kumar wrote:
@Arnd: Can you please look at patches 7-10 ..
Looks good to me overall once you address Stephen's comments (I would have said the same things on patches 7 and 8.
I also have a suggestion for improving patch 10.
Arnd
linaro-kernel@lists.linaro.org
-
Arnd Bergmann -
One Thousand Gnomes -
Rafael J. Wysocki -
Rafael J. Wysocki -
Stephen Boyd -
Thomas Petazzoni -
Viresh Kumar