[PATCH 01/10] PM / OPP: Propagate the error returned by _find_opp_table()

[PATCH 01/10] PM / OPP: Propagate the error returned by _find_opp_table()

[Viresh Kumar]

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;
 	}
 
-- 
2.7.1.410.g6faf27b

Re: [PATCH 01/10] PM / OPP: Propagate the error returned by _find_opp_table()

[Stephen Boyd]

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>

-- 
X-Patchwork-State: Not Applicable
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 02/10] PM / OPP: Add missing doc style comments

[Viresh Kumar]

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;
-- 
2.7.1.410.g6faf27b

Re: [PATCH 02/10] PM / OPP: Add missing doc style comments

[Stephen Boyd]

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>

-- 
X-Patchwork-State: Not Applicable
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 03/10] PM / OPP: dev_pm_opp_set_sharing_cpus() doesn't depend on CONFIG_OF

[Viresh Kumar]

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__ */
-- 
2.7.1.410.g6faf27b

Re: [PATCH 03/10] PM / OPP: dev_pm_opp_set_sharing_cpus() doesn't depend on CONFIG_OF

[Stephen Boyd]

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>

-- 
X-Patchwork-State: Not Applicable
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 04/10] PM / OPP: Relocate dev_pm_opp_set_sharing_cpus()

[Viresh Kumar]

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);
-- 
2.7.1.410.g6faf27b

Re: [PATCH 04/10] PM / OPP: Relocate dev_pm_opp_set_sharing_cpus()

[Stephen Boyd]

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 05/10] PM / OPP: Mark shared-opp for non-dt case

[Viresh Kumar]

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);
-- 
2.7.1.410.g6faf27b

Re: [PATCH 05/10] PM / OPP: Mark shared-opp for non-dt case

[Stephen Boyd]

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>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 06/10] PM / OPP: Add dev_pm_opp_get_sharing_cpus()

[Viresh Kumar]

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)
-- 
2.7.1.410.g6faf27b

Re: [PATCH 06/10] PM / OPP: Add dev_pm_opp_get_sharing_cpus()

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 06/10] PM / OPP: Add dev_pm_opp_get_sharing_cpus()

[Viresh Kumar]

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 ?

-- 
viresh

[PATCH 07/10] cpufreq: dt: Identify cpu-sharing for platforms without operating-points-v2

[Viresh Kumar]

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)
-- 
2.7.1.410.g6faf27b

Re: [PATCH 07/10] cpufreq: dt: Identify cpu-sharing for platforms without operating-points-v2

[Stephen Boyd]

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;
> -		else
> -			goto 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?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 07/10] cpufreq: dt: Identify cpu-sharing for platforms without operating-points-v2

[Viresh Kumar]

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;
> > -		else
> > -			goto 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.

-- 
viresh

Re: [PATCH 07/10] cpufreq: dt: Identify cpu-sharing for platforms without operating-points-v2

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 07/10] cpufreq: dt: Identify cpu-sharing for platforms without operating-points-v2

[Rafael J. Wysocki]

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?

Re: [PATCH 07/10] cpufreq: dt: Identify cpu-sharing for platforms without operating-points-v2

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 07/10] cpufreq: dt: Identify cpu-sharing for platforms without operating-points-v2

[Rafael J. Wysocki]

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

[PATCH 08/10] mvebu: Use dev_pm_opp_set_sharing_cpus() to mark OPP tables as shared

[Viresh Kumar]

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;
 }
 
-- 
2.7.1.410.g6faf27b

Re: [PATCH 08/10] mvebu: Use dev_pm_opp_set_sharing_cpus() to mark OPP tables as shared

[Stephen Boyd]

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?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 09/10] cpufreq: dt: Kill platform-data

[Viresh Kumar]

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__ */
-- 
2.7.1.410.g6faf27b

Re: [PATCH 09/10] cpufreq: dt: Kill platform-data

[Stephen Boyd]

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>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 10/10] cpufreq: mvebu: Use generic platdev driver

[Viresh Kumar]

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", },
-- 
2.7.1.410.g6faf27b

Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver

[Arnd Bergmann]

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

Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver

[Viresh Kumar]

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 ?

-- 
viresh

Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver

[Arnd Bergmann]

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

Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver

[Viresh Kumar]

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 :)

-- 
viresh

Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver

[Arnd Bergmann]

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

Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver

[Viresh Kumar]

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.

-- 
viresh

Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver

[Arnd Bergmann]

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

Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] PM / OPP: -ENOSYS is applicable only to syscalls

[Viresh Kumar]

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

Re: [PATCH] PM / OPP: -ENOSYS is applicable only to syscalls

[Rafael J. Wysocki]

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?

Re: [PATCH] PM / OPP: -ENOSYS is applicable only to syscalls

[One Thousand Gnomes]

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

Re: [PATCH] PM / OPP: -ENOSYS is applicable only to syscalls

[Viresh Kumar]

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.

-- 
viresh