[PATCH 00/10] OPP: Add code to support operating-points-v2 bindings

[PATCH 00/10] OPP: Add code to support operating-points-v2 bindings

[Viresh Kumar]

Hi Guys,

This adds code to support operating-points-v2 bindings. Not everything
is supported yet, but most of the basic stuff is.

Tested with: Exynos 5250, dual cortex A15 board. With both old and new
bindings.

Bindings are already frozen:
http://marc.info/?i=cover.1433434659.git.viresh.kumar%40linaro.org

Pushed here as well for reference:
ssh://git at git.linaro.org/people/viresh.kumar/linux.git opp/v2

Viresh Kumar (10):
  opp: Relocate few routines
  OPP: Create _remove_device_opp() for freeing dev_opp
  OPP: Allocate dev_opp from _add_device_opp()
  OPP: Break _opp_add_dynamic() into smaller functions
  opp: Add support to parse "operating-points-v2" bindings
  OPP: Add clock-latency-ns support
  opp: Add OPP sharing information to OPP library
  OPP: Add support for opp-suspend
  opp: Add helpers for initializing CPU OPPs
  cpufreq-dt: Add support for operating-points-v2 bindings

 drivers/base/power/opp.c     | 1063 ++++++++++++++++++++++++++++++++----------
 drivers/cpufreq/cpufreq-dt.c |   58 ++-
 include/linux/pm_opp.h       |   29 ++
 3 files changed, 902 insertions(+), 248 deletions(-)

-- 
2.4.0

[PATCH 01/10] opp: Relocate few routines

[Viresh Kumar]

In order to prepare for the later commits, this relocates few routines towards
the top as they will be used earlier in the code.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 277 ++++++++++++++++++++++++-----------------------
 1 file changed, 139 insertions(+), 138 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 677fb2843553..8c3fd57975fb 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -438,6 +438,102 @@ static struct device_opp *_add_device_opp(struct device *dev)
 }
 
 /**
+ * _kfree_device_rcu() - Free device_opp RCU handler
+ * @head:	RCU head
+ */
+static void _kfree_device_rcu(struct rcu_head *head)
+{
+	struct device_opp *device_opp = container_of(head, struct device_opp, rcu_head);
+
+	kfree_rcu(device_opp, rcu_head);
+}
+
+/**
+ * _kfree_opp_rcu() - Free OPP RCU handler
+ * @head:	RCU head
+ */
+static void _kfree_opp_rcu(struct rcu_head *head)
+{
+	struct dev_pm_opp *opp = container_of(head, struct dev_pm_opp, rcu_head);
+
+	kfree_rcu(opp, rcu_head);
+}
+
+/**
+ * _opp_remove()  - Remove an OPP from a table definition
+ * @dev_opp:	points back to the device_opp struct this opp belongs to
+ * @opp:	pointer to the OPP to remove
+ *
+ * This function removes an opp definition from the opp list.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * It is assumed that the caller holds required mutex for an RCU updater
+ * strategy.
+ */
+static void _opp_remove(struct device_opp *dev_opp,
+			struct dev_pm_opp *opp)
+{
+	/*
+	 * Notify the changes in the availability of the operable
+	 * frequency/voltage list.
+	 */
+	srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
+	list_del_rcu(&opp->node);
+	call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
+
+	if (list_empty(&dev_opp->opp_list)) {
+		list_del_rcu(&dev_opp->node);
+		call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,
+			  _kfree_device_rcu);
+	}
+}
+
+/**
+ * dev_pm_opp_remove()  - Remove an OPP from OPP list
+ * @dev:	device for which we do this operation
+ * @freq:	OPP to remove with matching 'freq'
+ *
+ * This function removes an opp from the opp list.
+ *
+ * Locking: The internal device_opp 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_remove(struct device *dev, unsigned long freq)
+{
+	struct dev_pm_opp *opp;
+	struct device_opp *dev_opp;
+	bool found = false;
+
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
+	dev_opp = _find_device_opp(dev);
+	if (IS_ERR(dev_opp))
+		goto unlock;
+
+	list_for_each_entry(opp, &dev_opp->opp_list, node) {
+		if (opp->rate == freq) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		dev_warn(dev, "%s: Couldn't find OPP with freq: %lu\n",
+			 __func__, freq);
+		goto unlock;
+	}
+
+	_opp_remove(dev_opp, opp);
+unlock:
+	mutex_unlock(&dev_opp_list_lock);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
+
+/**
  * _opp_add_dynamic() - Allocate a dynamic OPP.
  * @dev:	device for which we do this operation
  * @freq:	Frequency in Hz for this OPP
@@ -570,102 +666,6 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
 EXPORT_SYMBOL_GPL(dev_pm_opp_add);
 
 /**
- * _kfree_opp_rcu() - Free OPP RCU handler
- * @head:	RCU head
- */
-static void _kfree_opp_rcu(struct rcu_head *head)
-{
-	struct dev_pm_opp *opp = container_of(head, struct dev_pm_opp, rcu_head);
-
-	kfree_rcu(opp, rcu_head);
-}
-
-/**
- * _kfree_device_rcu() - Free device_opp RCU handler
- * @head:	RCU head
- */
-static void _kfree_device_rcu(struct rcu_head *head)
-{
-	struct device_opp *device_opp = container_of(head, struct device_opp, rcu_head);
-
-	kfree_rcu(device_opp, rcu_head);
-}
-
-/**
- * _opp_remove()  - Remove an OPP from a table definition
- * @dev_opp:	points back to the device_opp struct this opp belongs to
- * @opp:	pointer to the OPP to remove
- *
- * This function removes an opp definition from the opp list.
- *
- * Locking: The internal device_opp and opp structures are RCU protected.
- * It is assumed that the caller holds required mutex for an RCU updater
- * strategy.
- */
-static void _opp_remove(struct device_opp *dev_opp,
-			struct dev_pm_opp *opp)
-{
-	/*
-	 * Notify the changes in the availability of the operable
-	 * frequency/voltage list.
-	 */
-	srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
-	list_del_rcu(&opp->node);
-	call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
-
-	if (list_empty(&dev_opp->opp_list)) {
-		list_del_rcu(&dev_opp->node);
-		call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,
-			  _kfree_device_rcu);
-	}
-}
-
-/**
- * dev_pm_opp_remove()  - Remove an OPP from OPP list
- * @dev:	device for which we do this operation
- * @freq:	OPP to remove with matching 'freq'
- *
- * This function removes an opp from the opp list.
- *
- * Locking: The internal device_opp 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_remove(struct device *dev, unsigned long freq)
-{
-	struct dev_pm_opp *opp;
-	struct device_opp *dev_opp;
-	bool found = false;
-
-	/* Hold our list modification lock here */
-	mutex_lock(&dev_opp_list_lock);
-
-	dev_opp = _find_device_opp(dev);
-	if (IS_ERR(dev_opp))
-		goto unlock;
-
-	list_for_each_entry(opp, &dev_opp->opp_list, node) {
-		if (opp->rate == freq) {
-			found = true;
-			break;
-		}
-	}
-
-	if (!found) {
-		dev_warn(dev, "%s: Couldn't find OPP with freq: %lu\n",
-			 __func__, freq);
-		goto unlock;
-	}
-
-	_opp_remove(dev_opp, opp);
-unlock:
-	mutex_unlock(&dev_opp_list_lock);
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
-
-/**
  * _opp_set_availability() - helper to set the availability of an opp
  * @dev:		device for which we do this operation
  * @freq:		OPP frequency to modify availability
@@ -825,6 +825,49 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_notifier);
 
 #ifdef CONFIG_OF
 /**
+ * of_free_opp_table() - Free OPP table entries created from static DT entries
+ * @dev:	device pointer used to lookup device OPPs.
+ *
+ * Free OPPs created using static entries present in DT.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function indirectly 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 of_free_opp_table(struct device *dev)
+{
+	struct device_opp *dev_opp;
+	struct dev_pm_opp *opp, *tmp;
+
+	/* Check for existing list for 'dev' */
+	dev_opp = _find_device_opp(dev);
+	if (IS_ERR(dev_opp)) {
+		int error = PTR_ERR(dev_opp);
+
+		if (error != -ENODEV)
+			WARN(1, "%s: dev_opp: %d\n",
+			     IS_ERR_OR_NULL(dev) ?
+					"Invalid device" : dev_name(dev),
+			     error);
+		return;
+	}
+
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
+	/* Free static OPPs */
+	list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) {
+		if (!opp->dynamic)
+			_opp_remove(dev_opp, opp);
+	}
+
+	mutex_unlock(&dev_opp_list_lock);
+}
+EXPORT_SYMBOL_GPL(of_free_opp_table);
+
+/**
  * of_init_opp_table() - Initialize opp table from device tree
  * @dev:	device pointer used to lookup device OPPs.
  *
@@ -882,46 +925,4 @@ int of_init_opp_table(struct device *dev)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
-
-/**
- * of_free_opp_table() - Free OPP table entries created from static DT entries
- * @dev:	device pointer used to lookup device OPPs.
- *
- * Free OPPs created using static entries present in DT.
- *
- * Locking: The internal device_opp and opp structures are RCU protected.
- * Hence this function indirectly 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 of_free_opp_table(struct device *dev)
-{
-	struct device_opp *dev_opp;
-	struct dev_pm_opp *opp, *tmp;
-
-	/* Check for existing list for 'dev' */
-	dev_opp = _find_device_opp(dev);
-	if (IS_ERR(dev_opp)) {
-		int error = PTR_ERR(dev_opp);
-		if (error != -ENODEV)
-			WARN(1, "%s: dev_opp: %d\n",
-			     IS_ERR_OR_NULL(dev) ?
-					"Invalid device" : dev_name(dev),
-			     error);
-		return;
-	}
-
-	/* Hold our list modification lock here */
-	mutex_lock(&dev_opp_list_lock);
-
-	/* Free static OPPs */
-	list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) {
-		if (!opp->dynamic)
-			_opp_remove(dev_opp, opp);
-	}
-
-	mutex_unlock(&dev_opp_list_lock);
-}
-EXPORT_SYMBOL_GPL(of_free_opp_table);
 #endif
-- 
2.4.0

[PATCH 01/10] opp: Relocate few routines

[Stephen Boyd]

On 06/15, Viresh Kumar wrote:
> In order to prepare for the later commits, this relocates few routines towards
> the top as they will be used earlier in the code.
> 
> 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 01/10] opp: Relocate few routines

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday, June 15, 2015 05:27:27 PM Viresh Kumar wrote:
> In order to prepare for the later commits, this relocates few routines towards
> the top as they will be used earlier in the code.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 02/10] OPP: Create _remove_device_opp() for freeing dev_opp

[Viresh Kumar]

This will be used from multiple places later. Lets create a separate
routine for that.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 8c3fd57975fb..7895fdd64192 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -449,6 +449,22 @@ static void _kfree_device_rcu(struct rcu_head *head)
 }
 
 /**
+ * _remove_device_opp() - Removes a device OPP table
+ * @dev_opp: device OPP table to be removed.
+ *
+ * Removes/frees device OPP table it it doesn't contain any OPPs.
+ */
+static void _remove_device_opp(struct device_opp *dev_opp)
+{
+	if (!list_empty(&dev_opp->opp_list))
+		return;
+
+	list_del_rcu(&dev_opp->node);
+	call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,
+		  _kfree_device_rcu);
+}
+
+/**
  * _kfree_opp_rcu() - Free OPP RCU handler
  * @head:	RCU head
  */
@@ -481,11 +497,7 @@ static void _opp_remove(struct device_opp *dev_opp,
 	list_del_rcu(&opp->node);
 	call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
 
-	if (list_empty(&dev_opp->opp_list)) {
-		list_del_rcu(&dev_opp->node);
-		call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,
-			  _kfree_device_rcu);
-	}
+	_remove_device_opp(dev_opp);
 }
 
 /**
-- 
2.4.0

[PATCH 02/10] OPP: Create _remove_device_opp() for freeing dev_opp

[Stephen Boyd]

On 06/15, Viresh Kumar wrote:
> This will be used from multiple places later. Lets create a separate
> routine for that.
> 
> 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 02/10] OPP: Create _remove_device_opp() for freeing dev_opp

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday, June 15, 2015 05:27:28 PM Viresh Kumar wrote:
> This will be used from multiple places later. Lets create a separate
> routine for that.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 03/10] OPP: Allocate dev_opp from _add_device_opp()

[Viresh Kumar]

There is no need to complicate _opp_add_dynamic() with allocation of
dev_opp as well. Allocate it from _add_device_opp() instead.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 7895fdd64192..4b646f36f252 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -408,11 +408,11 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
 /**
- * _add_device_opp() - Allocate a new device OPP table
+ * _add_device_opp() - Returns device OPP table
  * @dev:	device for which we do this operation
  *
- * New device node which uses OPPs - used when multiple devices with OPP tables
- * are maintained.
+ * It tries to find an existing table first, if it couldn't find one, it
+ * allocates a new OPP table and returns that.
  *
  * Return: valid device_opp pointer if success, else NULL.
  */
@@ -420,6 +420,11 @@ static struct device_opp *_add_device_opp(struct device *dev)
 {
 	struct device_opp *dev_opp;
 
+	/* Check for existing list for 'dev' first */
+	dev_opp = _find_device_opp(dev);
+	if (!IS_ERR(dev_opp))
+		return dev_opp;
+
 	/*
 	 * Allocate a new device OPP table. In the infrequent case where a new
 	 * device is needed to be added, we pay this penalty.
@@ -575,8 +580,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
 static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 			    long u_volt, bool dynamic)
 {
-	struct device_opp *dev_opp = NULL;
-	struct dev_pm_opp *opp, *new_opp;
+	struct device_opp *dev_opp;
+	struct dev_pm_opp *opp = NULL, *new_opp;
 	struct list_head *head;
 	int ret;
 
@@ -592,19 +597,11 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 	new_opp->rate = freq;
 	new_opp->u_volt = u_volt;
 	new_opp->available = true;
-	new_opp->dynamic = dynamic;
 
-	/* Check for existing list for 'dev' */
-	dev_opp = _find_device_opp(dev);
-	if (IS_ERR(dev_opp)) {
-		dev_opp = _add_device_opp(dev);
-		if (!dev_opp) {
-			ret = -ENOMEM;
-			goto free_opp;
-		}
-
-		head = &dev_opp->opp_list;
-		goto list_add;
+	dev_opp = _add_device_opp(dev);
+	if (!dev_opp) {
+		ret = -ENOMEM;
+		goto free_opp;
 	}
 
 	/*
@@ -620,17 +617,17 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 	}
 
 	/* Duplicate OPPs ? */
-	if (new_opp->rate == opp->rate) {
+	if (opp && new_opp->rate == opp->rate) {
 		ret = opp->available && new_opp->u_volt == opp->u_volt ?
 			0 : -EEXIST;
 
 		dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
 			 __func__, opp->rate, opp->u_volt, opp->available,
 			 new_opp->rate, new_opp->u_volt, new_opp->available);
-		goto free_opp;
+		goto remove_dev_opp;
 	}
 
-list_add:
+	new_opp->dynamic = dynamic;
 	new_opp->dev_opp = dev_opp;
 	list_add_rcu(&new_opp->node, head);
 	mutex_unlock(&dev_opp_list_lock);
@@ -642,6 +639,8 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 	srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp);
 	return 0;
 
+remove_dev_opp:
+	_remove_device_opp(dev_opp);
 free_opp:
 	mutex_unlock(&dev_opp_list_lock);
 	kfree(new_opp);
-- 
2.4.0

[PATCH 03/10] OPP: Allocate dev_opp from _add_device_opp()

[Stephen Boyd]

On 06/15/2015 04:57 AM, Viresh Kumar wrote:
> There is no need to complicate _opp_add_dynamic() with allocation of
> dev_opp as well. Allocate it from _add_device_opp() instead.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/opp.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 7895fdd64192..4b646f36f252 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -408,11 +408,11 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
>  EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
>  
>  /**
> - * _add_device_opp() - Allocate a new device OPP table
> + * _add_device_opp() - Returns device OPP table

Find device OPP table, or allocate a new one?

>   * @dev:	device for which we do this operation
>   *
> - * New device node which uses OPPs - used when multiple devices with OPP tables
> - * are maintained.
> + * It tries to find an existing table first, if it couldn't find one, it
> + * allocates a new OPP table and returns that.
>   *
>   * Return: valid device_opp pointer if success, else NULL.
>   */
> @@ -420,6 +420,11 @@ static struct device_opp *_add_device_opp(struct device *dev)
>  {
>  	struct device_opp *dev_opp;
>  
> +	/* Check for existing list for 'dev' first */
> +	dev_opp = _find_device_opp(dev);
> +	if (!IS_ERR(dev_opp))
> +		return dev_opp;
> +
>  	/*
>  	 * Allocate a new device OPP table. In the infrequent case where a new
>  	 * device is needed to be added, we pay this penalty.
> @@ -575,8 +580,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
>  static int _opp_add_dynamic(struct device *dev, unsigned long freq,
>  			    long u_volt, bool dynamic)
>  {
> -	struct device_opp *dev_opp = NULL;
> -	struct dev_pm_opp *opp, *new_opp;
> +	struct device_opp *dev_opp;
> +	struct dev_pm_opp *opp = NULL, *new_opp;

Hm...

>  	struct list_head *head;
>  	int ret;
>  
> @@ -592,19 +597,11 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
>  	new_opp->rate = freq;
>  	new_opp->u_volt = u_volt;
>  	new_opp->available = true;
> -	new_opp->dynamic = dynamic;
>  
> -	/* Check for existing list for 'dev' */
> -	dev_opp = _find_device_opp(dev);
> -	if (IS_ERR(dev_opp)) {
> -		dev_opp = _add_device_opp(dev);
> -		if (!dev_opp) {
> -			ret = -ENOMEM;
> -			goto free_opp;
> -		}
> -
> -		head = &dev_opp->opp_list;
> -		goto list_add;
> +	dev_opp = _add_device_opp(dev);
> +	if (!dev_opp) {
> +		ret = -ENOMEM;
> +		goto free_opp;
>  	}
>  
>  	/*
> @@ -620,17 +617,17 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
>  	}
>  
>  	/* Duplicate OPPs ? */
> -	if (new_opp->rate == opp->rate) {
> +	if (opp && new_opp->rate == opp->rate) {

Isn't opp always non-NULL at this point? Maybe this if statement should
be moved into the list_for_each_entry_rcu() loop.

>  		ret = opp->available && new_opp->u_volt == opp->u_volt ?
>  			0 : -EEXIST;
>  
>  		dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
>  			 __func__, opp->rate, opp->u_volt, opp->available,
>  			 new_opp->rate, new_opp->u_volt, new_opp->available);
> -		goto free_opp;
> +		goto remove_dev_opp;
>  	}
>  
> -list_add:
> +	new_opp->dynamic = dynamic;
>  	new_opp->dev_opp = dev_opp;
>  	list_add_rcu(&new_opp->node, head);
>  	mutex_unlock(&dev_opp_list_lock);
> @@ -642,6 +639,8 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
>  	srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp);
>  	return 0;
>  
> +remove_dev_opp:
> +	_remove_device_opp(dev_opp);

Doesn't this just return early because dev_opp has something in it?

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

[PATCH 03/10] OPP: Allocate dev_opp from _add_device_opp()

[Viresh Kumar]

On 01-07-15, 18:02, Stephen Boyd wrote:
> On 06/15/2015 04:57 AM, Viresh Kumar wrote:
> > + * _add_device_opp() - Returns device OPP table
> 
> Find device OPP table, or allocate a new one?

Sure.

> >  static int _opp_add_dynamic(struct device *dev, unsigned long freq,
> >  			    long u_volt, bool dynamic)
> >  {
> > -	struct device_opp *dev_opp = NULL;
> > -	struct dev_pm_opp *opp, *new_opp;
> > +	struct device_opp *dev_opp;
> > +	struct dev_pm_opp *opp = NULL, *new_opp;
> 
> Hm...

> >  	/* Duplicate OPPs ? */
> > -	if (new_opp->rate == opp->rate) {
> > +	if (opp && new_opp->rate == opp->rate) {
> 
> Isn't opp always non-NULL at this point? Maybe this if statement should
> be moved into the list_for_each_entry_rcu() loop.

when the dev_opp was getting created for the first time, before this
patch, we used to do a 'goto list_add'. And so control never reached
here, but now we might find a empty list of OPPs and so 'opp' can be
NULL here.

Not sure about moving this to the loop, as we are already taking
'dev_opp_list_lock' in this routine.

> > +remove_dev_opp:
> > +	_remove_device_opp(dev_opp);
> 
> Doesn't this just return early because dev_opp has something in it?

Hmm, it isn't required probably.

---------------------8<-----------------------

Message-Id: <b13057f196330d0e08f3c517676cc3116b1be4ae.1435818265.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Fri, 12 Jun 2015 12:43:14 +0530
Subject: [PATCH] OPP: Allocate dev_opp from _add_device_opp()

There is no need to complicate _opp_add_dynamic() with allocation of
dev_opp as well. Allocate it from _add_device_opp() instead.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 7895fdd64192..f8df0ad3e822 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -408,11 +408,11 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
 /**
- * _add_device_opp() - Allocate a new device OPP table
+ * _add_device_opp() - Find device OPP table or allocate a new one
  * @dev:	device for which we do this operation
  *
- * New device node which uses OPPs - used when multiple devices with OPP tables
- * are maintained.
+ * It tries to find an existing table first, if it couldn't find one, it
+ * allocates a new OPP table and returns that.
  *
  * Return: valid device_opp pointer if success, else NULL.
  */
@@ -420,6 +420,11 @@ static struct device_opp *_add_device_opp(struct device *dev)
 {
 	struct device_opp *dev_opp;
 
+	/* Check for existing list for 'dev' first */
+	dev_opp = _find_device_opp(dev);
+	if (!IS_ERR(dev_opp))
+		return dev_opp;
+
 	/*
 	 * Allocate a new device OPP table. In the infrequent case where a new
 	 * device is needed to be added, we pay this penalty.
@@ -575,8 +580,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
 static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 			    long u_volt, bool dynamic)
 {
-	struct device_opp *dev_opp = NULL;
-	struct dev_pm_opp *opp, *new_opp;
+	struct device_opp *dev_opp;
+	struct dev_pm_opp *opp = NULL, *new_opp;
 	struct list_head *head;
 	int ret;
 
@@ -592,19 +597,11 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 	new_opp->rate = freq;
 	new_opp->u_volt = u_volt;
 	new_opp->available = true;
-	new_opp->dynamic = dynamic;
 
-	/* Check for existing list for 'dev' */
-	dev_opp = _find_device_opp(dev);
-	if (IS_ERR(dev_opp)) {
-		dev_opp = _add_device_opp(dev);
-		if (!dev_opp) {
-			ret = -ENOMEM;
-			goto free_opp;
-		}
-
-		head = &dev_opp->opp_list;
-		goto list_add;
+	dev_opp = _add_device_opp(dev);
+	if (!dev_opp) {
+		ret = -ENOMEM;
+		goto free_opp;
 	}
 
 	/*
@@ -620,7 +617,7 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 	}
 
 	/* Duplicate OPPs ? */
-	if (new_opp->rate == opp->rate) {
+	if (opp && new_opp->rate == opp->rate) {
 		ret = opp->available && new_opp->u_volt == opp->u_volt ?
 			0 : -EEXIST;
 
@@ -630,7 +627,7 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 		goto free_opp;
 	}
 
-list_add:
+	new_opp->dynamic = dynamic;
 	new_opp->dev_opp = dev_opp;
 	list_add_rcu(&new_opp->node, head);
 	mutex_unlock(&dev_opp_list_lock);

[PATCH 03/10] OPP: Allocate dev_opp from _add_device_opp()

[Stephen Boyd]

On 07/01/2015 11:24 PM, Viresh Kumar wrote:
> On 01-07-15, 18:02, Stephen Boyd wrote:
>> On 06/15/2015 04:57 AM, Viresh Kumar wrote:
>>>  	/* Duplicate OPPs ? */
>>> -	if (new_opp->rate == opp->rate) {
>>> +	if (opp && new_opp->rate == opp->rate) {
>> Isn't opp always non-NULL at this point? Maybe this if statement should
>> be moved into the list_for_each_entry_rcu() loop.
> when the dev_opp was getting created for the first time, before this
> patch, we used to do a 'goto list_add'. And so control never reached
> here, but now we might find a empty list of OPPs and so 'opp' can be
> NULL here.

Yes, but doesn't list_for_each_entry_rcu() always assign opp to be at
least &dev_opp->opp_list->next which will be pointing at
&dev_opp->opp_list if the list is empty (modulo ->member)?

> Not sure about moving this to the loop, as we are already taking
> 'dev_opp_list_lock' in this routine.

Sorry, I lost you here. How does that matter? I was suggesting moving
the duplicate OPP check into the list_for_each_entry_rcu() loop so that
this NULL check doesn't matter.

>
>>> +remove_dev_opp:
>>> +	_remove_device_opp(dev_opp);
>> Doesn't this just return early because dev_opp has something in it?
> Hmm, it isn't required probably.
>
> ---------------------8<-----------------------
>
> Message-Id: <b13057f196330d0e08f3c517676cc3116b1be4ae.1435818265.git.viresh.kumar@linaro.org>
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Fri, 12 Jun 2015 12:43:14 +0530
> Subject: [PATCH] OPP: Allocate dev_opp from _add_device_opp()
>
> There is no need to complicate _opp_add_dynamic() with allocation of
> dev_opp as well. Allocate it from _add_device_opp() instead.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>

Looking better.

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

[PATCH 03/10] OPP: Allocate dev_opp from _add_device_opp()

[Viresh Kumar]

On 02-07-15, 16:46, Stephen Boyd wrote:
> Looking better.

And does this look perfect ? :)

------------------8<----------------

Message-Id: <7c23b93b6b328e0b14b5c5096c3c3322e3d8ee4b.1435905782.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Fri, 12 Jun 2015 12:43:14 +0530
Subject: [PATCH] OPP: Allocate dev_opp from _add_device_opp()

There is no need to complicate _opp_add_dynamic() with allocation of
dev_opp as well. Allocate it from _add_device_opp() instead.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 60 +++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 31 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 7895fdd64192..5869a8524244 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -408,11 +408,11 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
 /**
- * _add_device_opp() - Allocate a new device OPP table
+ * _add_device_opp() - Find device OPP table or allocate a new one
  * @dev:	device for which we do this operation
  *
- * New device node which uses OPPs - used when multiple devices with OPP tables
- * are maintained.
+ * It tries to find an existing table first, if it couldn't find one, it
+ * allocates a new OPP table and returns that.
  *
  * Return: valid device_opp pointer if success, else NULL.
  */
@@ -420,6 +420,11 @@ static struct device_opp *_add_device_opp(struct device *dev)
 {
 	struct device_opp *dev_opp;
 
+	/* Check for existing list for 'dev' first */
+	dev_opp = _find_device_opp(dev);
+	if (!IS_ERR(dev_opp))
+		return dev_opp;
+
 	/*
 	 * Allocate a new device OPP table. In the infrequent case where a new
 	 * device is needed to be added, we pay this penalty.
@@ -575,7 +580,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
 static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 			    long u_volt, bool dynamic)
 {
-	struct device_opp *dev_opp = NULL;
+	struct device_opp *dev_opp;
 	struct dev_pm_opp *opp, *new_opp;
 	struct list_head *head;
 	int ret;
@@ -592,19 +597,11 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 	new_opp->rate = freq;
 	new_opp->u_volt = u_volt;
 	new_opp->available = true;
-	new_opp->dynamic = dynamic;
-
-	/* Check for existing list for 'dev' */
-	dev_opp = _find_device_opp(dev);
-	if (IS_ERR(dev_opp)) {
-		dev_opp = _add_device_opp(dev);
-		if (!dev_opp) {
-			ret = -ENOMEM;
-			goto free_opp;
-		}
 
-		head = &dev_opp->opp_list;
-		goto list_add;
+	dev_opp = _add_device_opp(dev);
+	if (!dev_opp) {
+		ret = -ENOMEM;
+		goto free_opp;
 	}
 
 	/*
@@ -613,24 +610,25 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 	 */
 	head = &dev_opp->opp_list;
 	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
-		if (new_opp->rate <= opp->rate)
-			break;
-		else
+		if (new_opp->rate > opp->rate) {
 			head = &opp->node;
+		} else if (new_opp->rate < opp->rate) {
+			break;
+		} else {
+			/* Duplicate OPPs */
+			ret = opp->available && new_opp->u_volt == opp->u_volt ?
+				0 : -EEXIST;
+
+			dev_warn(dev,
+				 "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
+				 __func__, opp->rate, opp->u_volt,
+				 opp->available, new_opp->rate, new_opp->u_volt,
+				 new_opp->available);
+			goto free_opp;
+		}
 	}
 
-	/* Duplicate OPPs ? */
-	if (new_opp->rate == opp->rate) {
-		ret = opp->available && new_opp->u_volt == opp->u_volt ?
-			0 : -EEXIST;
-
-		dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
-			 __func__, opp->rate, opp->u_volt, opp->available,
-			 new_opp->rate, new_opp->u_volt, new_opp->available);
-		goto free_opp;
-	}
-
-list_add:
+	new_opp->dynamic = dynamic;
 	new_opp->dev_opp = dev_opp;
 	list_add_rcu(&new_opp->node, head);
 	mutex_unlock(&dev_opp_list_lock);

[PATCH 03/10] OPP: Allocate dev_opp from _add_device_opp()

[Stephen Boyd]

On 07/02/2015 11:45 PM, Viresh Kumar wrote:
> On 02-07-15, 16:46, Stephen Boyd wrote:
>> Looking better.
> And does this look perfect ? :)
>

Yep.

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

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

[PATCH 03/10] OPP: Allocate dev_opp from _add_device_opp()

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday, July 03, 2015 12:15:54 PM Viresh Kumar wrote:
> On 02-07-15, 16:46, Stephen Boyd wrote:
> > Looking better.
> 
> And does this look perfect ? :)
> 
> ------------------8<----------------
> 
> Message-Id: <7c23b93b6b328e0b14b5c5096c3c3322e3d8ee4b.1435905782.git.viresh.kumar@linaro.org>
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Fri, 12 Jun 2015 12:43:14 +0530
> Subject: [PATCH] OPP: Allocate dev_opp from _add_device_opp()
> 
> There is no need to complicate _opp_add_dynamic() with allocation of
> dev_opp as well. Allocate it from _add_device_opp() instead.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 04/10] OPP: Break _opp_add_dynamic() into smaller functions

[Viresh Kumar]

Later commits would add support for new OPP bindings and this would be
required then. So, lets do it in a separate patch to make it easily
reviewable.

Another change worth noticing is INIT_LIST_HEAD(&opp->node). We weren't
doing it earlier as we never tried to delete a list node before it is
added to list. But this wouldn't be the case anymore. We might try to
delete a node (just to reuse the same code paths), without it being
getting added to the list.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 115 +++++++++++++++++++++++++++++------------------
 1 file changed, 71 insertions(+), 44 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 4b646f36f252..2ac48ff9c1ef 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -484,6 +484,7 @@ static void _kfree_opp_rcu(struct rcu_head *head)
  * _opp_remove()  - Remove an OPP from a table definition
  * @dev_opp:	points back to the device_opp struct this opp belongs to
  * @opp:	pointer to the OPP to remove
+ * @notify:	OPP_EVENT_REMOVE notification should be sent or not
  *
  * This function removes an opp definition from the opp list.
  *
@@ -492,13 +493,14 @@ static void _kfree_opp_rcu(struct rcu_head *head)
  * strategy.
  */
 static void _opp_remove(struct device_opp *dev_opp,
-			struct dev_pm_opp *opp)
+			struct dev_pm_opp *opp, bool notify)
 {
 	/*
 	 * Notify the changes in the availability of the operable
 	 * frequency/voltage list.
 	 */
-	srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
+	if (notify)
+		srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
 	list_del_rcu(&opp->node);
 	call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
 
@@ -544,12 +546,65 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq)
 		goto unlock;
 	}
 
-	_opp_remove(dev_opp, opp);
+	_opp_remove(dev_opp, opp, true);
 unlock:
 	mutex_unlock(&dev_opp_list_lock);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
 
+static struct dev_pm_opp *_allocate_opp(struct device *dev,
+					struct device_opp **dev_opp)
+{
+	struct dev_pm_opp *opp;
+
+	/* allocate new OPP node */
+	opp = kzalloc(sizeof(*opp), GFP_KERNEL);
+	if (!opp)
+		return NULL;
+
+	INIT_LIST_HEAD(&opp->node);
+
+	*dev_opp = _add_device_opp(dev);
+	if (!*dev_opp) {
+		kfree(opp);
+		return NULL;
+	}
+
+	return opp;
+}
+
+static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp)
+{
+	struct dev_pm_opp *opp = NULL;
+	struct list_head *head = &dev_opp->opp_list;
+
+	/*
+	 * Insert new OPP in order of increasing frequency
+	 * and discard if already present
+	 */
+	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
+		if (new_opp->rate <= opp->rate)
+			break;
+
+		head = &opp->node;
+	}
+
+	/* Duplicate OPPs ? */
+	if (opp && new_opp->rate == opp->rate) {
+		dev_warn(dev_opp->dev,
+			 "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
+			 __func__, opp->rate, opp->u_volt, opp->available,
+			 new_opp->rate, new_opp->u_volt, new_opp->available);
+		return opp->available && new_opp->u_volt == opp->u_volt ?
+			0 : -EEXIST;
+	}
+
+	new_opp->dev_opp = dev_opp;
+	list_add_rcu(&new_opp->node, head);
+
+	return 0;
+}
+
 /**
  * _opp_add_dynamic() - Allocate a dynamic OPP.
  * @dev:	device for which we do this operation
@@ -581,55 +636,28 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 			    long u_volt, bool dynamic)
 {
 	struct device_opp *dev_opp;
-	struct dev_pm_opp *opp = NULL, *new_opp;
-	struct list_head *head;
+	struct dev_pm_opp *new_opp;
 	int ret;
 
-	/* allocate new OPP node */
-	new_opp = kzalloc(sizeof(*new_opp), GFP_KERNEL);
-	if (!new_opp)
-		return -ENOMEM;
-
 	/* Hold our list modification lock here */
 	mutex_lock(&dev_opp_list_lock);
 
+	new_opp = _allocate_opp(dev, &dev_opp);
+	if (!new_opp) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
 	/* populate the opp table */
 	new_opp->rate = freq;
 	new_opp->u_volt = u_volt;
 	new_opp->available = true;
+	new_opp->dynamic = dynamic;
 
-	dev_opp = _add_device_opp(dev);
-	if (!dev_opp) {
-		ret = -ENOMEM;
+	ret = _opp_add(new_opp, dev_opp);
+	if (ret)
 		goto free_opp;
-	}
-
-	/*
-	 * Insert new OPP in order of increasing frequency
-	 * and discard if already present
-	 */
-	head = &dev_opp->opp_list;
-	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
-		if (new_opp->rate <= opp->rate)
-			break;
-		else
-			head = &opp->node;
-	}
-
-	/* Duplicate OPPs ? */
-	if (opp && new_opp->rate == opp->rate) {
-		ret = opp->available && new_opp->u_volt == opp->u_volt ?
-			0 : -EEXIST;
-
-		dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
-			 __func__, opp->rate, opp->u_volt, opp->available,
-			 new_opp->rate, new_opp->u_volt, new_opp->available);
-		goto remove_dev_opp;
-	}
 
-	new_opp->dynamic = dynamic;
-	new_opp->dev_opp = dev_opp;
-	list_add_rcu(&new_opp->node, head);
 	mutex_unlock(&dev_opp_list_lock);
 
 	/*
@@ -639,11 +667,10 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 	srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp);
 	return 0;
 
-remove_dev_opp:
-	_remove_device_opp(dev_opp);
 free_opp:
+	_opp_remove(dev_opp, new_opp, false);
+unlock:
 	mutex_unlock(&dev_opp_list_lock);
-	kfree(new_opp);
 	return ret;
 }
 
@@ -871,7 +898,7 @@ void of_free_opp_table(struct device *dev)
 	/* Free static OPPs */
 	list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) {
 		if (!opp->dynamic)
-			_opp_remove(dev_opp, opp);
+			_opp_remove(dev_opp, opp, true);
 	}
 
 	mutex_unlock(&dev_opp_list_lock);
-- 
2.4.0

[PATCH 04/10] OPP: Break _opp_add_dynamic() into smaller functions

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday, June 15, 2015 05:27:30 PM Viresh Kumar wrote:
> Later commits would add support for new OPP bindings and this would be
> required then. So, lets do it in a separate patch to make it easily
> reviewable.
> 
> Another change worth noticing is INIT_LIST_HEAD(&opp->node). We weren't
> doing it earlier as we never tried to delete a list node before it is
> added to list. But this wouldn't be the case anymore. We might try to
> delete a node (just to reuse the same code paths), without it being
> getting added to the list.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Viresh Kumar]

This adds support in OPP library to parse and create list of OPPs from
operating-points-v2 bindings. It takes care of most of the properties of
new bindings (except shared-opp, which will be handled separately).

For backward compatibility, we keep supporting earlier bindings. We try
to search for the new bindings first, in case they aren't present we
look for the old deprecated ones.

There are few things marked as TODO:
- Support for multiple OPP tables
- Support for multiple regulators

They should be fixed separately.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 238 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 213 insertions(+), 25 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 2ac48ff9c1ef..3198c3e77224 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -49,12 +49,17 @@
  *		are protected by the dev_opp_list_lock for integrity.
  *		IMPORTANT: the opp nodes should be maintained in increasing
  *		order.
- * @dynamic:	not-created from static DT entries.
  * @available:	true/false - marks if this OPP as available or not
+ * @dynamic:	not-created from static DT entries.
+ * @turbo:	true if turbo (boost) OPP
  * @rate:	Frequency in hertz
- * @u_volt:	Nominal voltage in microvolts corresponding to this OPP
+ * @u_volt:	Target voltage in microvolts corresponding to this OPP
+ * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
+ * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
+ * @u_amp:	Maximum current drawn by the device in microamperes
  * @dev_opp:	points back to the device_opp struct this opp belongs to
  * @rcu_head:	RCU callback head used for deferred freeing
+ * @np:		OPP's device node.
  *
  * This structure stores the OPP information for a given device.
  */
@@ -63,11 +68,22 @@ struct dev_pm_opp {
 
 	bool available;
 	bool dynamic;
+	bool turbo;
 	unsigned long rate;
+
+	/*
+	 * Order in which u_volt{_min|_max} are present in this structure
+	 * shouldn't be changed.
+	 */
 	unsigned long u_volt;
+	unsigned long u_volt_min;
+	unsigned long u_volt_max;
+	unsigned long u_amp;
 
 	struct device_opp *dev_opp;
 	struct rcu_head rcu_head;
+
+	struct device_node *np;
 };
 
 /**
@@ -501,6 +517,7 @@ static void _opp_remove(struct device_opp *dev_opp,
 	 */
 	if (notify)
 		srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
+
 	list_del_rcu(&opp->node);
 	call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
 
@@ -675,6 +692,100 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 }
 
 /**
+ * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
+ * @dev:	device for which we do this operation
+ * @np:		device node
+ *
+ * This function adds an opp definition to the opp list and returns status. The
+ * opp can be controlled using dev_pm_opp_enable/disable functions and may be
+ * removed by dev_pm_opp_remove.
+ *
+ * Locking: The internal device_opp 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.
+ *
+ * Return:
+ * 0		On success OR
+ *		Duplicate OPPs (both freq and volt are same) and opp->available
+ * -EEXIST	Freq are same and volt are different OR
+ *		Duplicate OPPs (both freq and volt are same) and !opp->available
+ * -ENOMEM	Memory allocation failure
+ * -EINVAL	Failed parsing the OPP node
+ */
+static int _opp_add_static_v2(struct device *dev, struct device_node *np)
+{
+	struct device_opp *dev_opp;
+	struct dev_pm_opp *new_opp;
+	int ret;
+
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
+	new_opp = _allocate_opp(dev, &dev_opp);
+	if (!new_opp) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	ret = of_property_read_u32(np, "opp-hz", (u32 *)&new_opp->rate);
+	if (ret < 0) {
+		dev_err(dev, "%s: opp-hz not found\n", __func__);
+		goto free_opp;
+	}
+
+	if (of_get_property(np, "turbo-mode", NULL))
+		new_opp->turbo = true;
+
+	new_opp->np = np;
+	new_opp->dynamic = false;
+	new_opp->available = true;
+
+	/*
+	 * TODO: Support multiple regulators
+	 *
+	 * read opp-microvolt array
+	 */
+	ret = of_property_count_u32_elems(np, "opp-microvolt");
+	if (ret == 1 || ret == 3) {
+		/* There can be one or three elements here */
+		ret = of_property_read_u32_array(np, "opp-microvolt",
+						 (u32 *)&new_opp->u_volt, ret);
+		if (ret) {
+			dev_err(dev, "%s: error parsing opp-microvolt: %d\n",
+				__func__, ret);
+			goto free_opp;
+		}
+	}
+
+	of_property_read_u32(np, "opp-microamp", (u32 *)&new_opp->u_amp);
+
+	ret = _opp_add(new_opp, dev_opp);
+	if (ret)
+		goto free_opp;
+
+	pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu\n",
+		 __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt,
+		 new_opp->u_volt_min, new_opp->u_volt_max);
+
+	mutex_unlock(&dev_opp_list_lock);
+
+	/*
+	 * Notify the changes in the availability of the operable
+	 * frequency/voltage list.
+	 */
+	srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp);
+	return 0;
+
+free_opp:
+	_opp_remove(dev_opp, new_opp, false);
+unlock:
+	mutex_unlock(&dev_opp_list_lock);
+	return ret;
+}
+
+/**
  * dev_pm_opp_add()  - Add an OPP table from a table definitions
  * @dev:	device for which we do this operation
  * @freq:	Frequency in Hz for this OPP
@@ -905,29 +1016,64 @@ void of_free_opp_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(of_free_opp_table);
 
-/**
- * of_init_opp_table() - Initialize opp table from device tree
- * @dev:	device pointer used to lookup device OPPs.
- *
- * Register the initial OPP table with the OPP library for given device.
- *
- * Locking: The internal device_opp and opp structures are RCU protected.
- * Hence this function indirectly 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.
- *
- * Return:
- * 0		On success OR
- *		Duplicate OPPs (both freq and volt are same) and opp->available
- * -EEXIST	Freq are same and volt are different OR
- *		Duplicate OPPs (both freq and volt are same) and !opp->available
- * -ENOMEM	Memory allocation failure
- * -ENODEV	when 'operating-points' property is not found or is invalid data
- *		in device node.
- * -ENODATA	when empty 'operating-points' property is found
- */
-int of_init_opp_table(struct device *dev)
+/* Returns opp descriptor node from its phandle. Caller must do of_node_put() */
+static struct device_node *
+_of_get_opp_desc_node_from_prop(struct device *dev, const struct property *prop)
+{
+	struct device_node *opp_np;
+
+	opp_np = of_find_node_by_phandle(be32_to_cpup(prop->value));
+	if (!opp_np) {
+		dev_err(dev, "%s: Prop: %s contains invalid opp desc phandle\n",
+			__func__, prop->name);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return opp_np;
+}
+
+/* Initializes OPP tables based on new bindings */
+static int _of_init_opp_table_v2(struct device *dev,
+				 const struct property *prop)
+{
+	struct device_node *opp_np, *np;
+	int ret = 0, count = 0;
+
+	if (!prop->value)
+		return -ENODATA;
+
+	/* Get opp node */
+	opp_np = _of_get_opp_desc_node_from_prop(dev, prop);
+	if (IS_ERR(opp_np))
+		return PTR_ERR(opp_np);
+
+	/* We have opp-list node now, iterate over it and add OPPs */
+	for_each_available_child_of_node(opp_np, np) {
+		count++;
+
+		ret = _opp_add_static_v2(dev, np);
+		if (ret) {
+			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
+				ret);
+			break;
+		}
+	}
+
+	/* There should be one of more OPP defined */
+	if (WARN_ON(!count))
+		goto put_opp_np;
+
+	if (ret)
+		of_free_opp_table(dev);
+
+put_opp_np:
+	of_node_put(opp_np);
+
+	return ret;
+}
+
+/* Initializes OPP tables based on old-deprecated bindings */
+static int _of_init_opp_table_v1(struct device *dev)
 {
 	const struct property *prop;
 	const __be32 *val;
@@ -962,5 +1108,47 @@ int of_init_opp_table(struct device *dev)
 
 	return 0;
 }
+
+/**
+ * of_init_opp_table() - Initialize opp table from device tree
+ * @dev:	device pointer used to lookup device OPPs.
+ *
+ * Register the initial OPP table with the OPP library for given device.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function indirectly 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.
+ *
+ * Return:
+ * 0		On success OR
+ *		Duplicate OPPs (both freq and volt are same) and opp->available
+ * -EEXIST	Freq are same and volt are different OR
+ *		Duplicate OPPs (both freq and volt are same) and !opp->available
+ * -ENOMEM	Memory allocation failure
+ * -ENODEV	when 'operating-points' property is not found or is invalid data
+ *		in device node.
+ * -ENODATA	when empty 'operating-points' property is found
+ */
+int of_init_opp_table(struct device *dev)
+{
+	const struct property *prop;
+
+	/*
+	 * OPPs have two version of bindings now. The older one is deprecated,
+	 * try for the new binding first.
+	 */
+	prop = of_find_property(dev->of_node, "operating-points-v2", NULL);
+	if (!prop) {
+		/*
+		 * Try old-deprecated bindings for backward compatibility with
+		 * older dtbs.
+		 */
+		return _of_init_opp_table_v1(dev);
+	}
+
+	return _of_init_opp_table_v2(dev, prop);
+}
 EXPORT_SYMBOL_GPL(of_init_opp_table);
 #endif
-- 
2.4.0

[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Stephen Boyd]

On 06/15/2015 04:57 AM, Viresh Kumar wrote:
> ---
>  drivers/base/power/opp.c | 238 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 213 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 2ac48ff9c1ef..3198c3e77224 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -49,12 +49,17 @@
>   *		are protected by the dev_opp_list_lock for integrity.
>   *		IMPORTANT: the opp nodes should be maintained in increasing
>   *		order.
> - * @dynamic:	not-created from static DT entries.
>   * @available:	true/false - marks if this OPP as available or not
> + * @dynamic:	not-created from static DT entries.

Why move dynamic?

> + * @turbo:	true if turbo (boost) OPP
>   * @rate:	Frequency in hertz
> - * @u_volt:	Nominal voltage in microvolts corresponding to this OPP
> + * @u_volt:	Target voltage in microvolts corresponding to this OPP
> + * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
> + * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
> + * @u_amp:	Maximum current drawn by the device in microamperes
>   * @dev_opp:	points back to the device_opp struct this opp belongs to
>   * @rcu_head:	RCU callback head used for deferred freeing
> + * @np:		OPP's device node.
>   *
>   * This structure stores the OPP information for a given device.
>   */
> @@ -63,11 +68,22 @@ struct dev_pm_opp {
>  
>  	bool available;
>  	bool dynamic;
> +	bool turbo;
>  	unsigned long rate;
> +
> +	/*
> +	 * Order in which u_volt{_min|_max} are present in this structure
> +	 * shouldn't be changed.
> +	 */
>  	unsigned long u_volt;
> +	unsigned long u_volt_min;
> +	unsigned long u_volt_max;
> +	unsigned long u_amp;
>  
>  	struct device_opp *dev_opp;
>  	struct rcu_head rcu_head;
> +
> +	struct device_node *np;
>  };
>  
>  /**
> @@ -501,6 +517,7 @@ static void _opp_remove(struct device_opp *dev_opp,
>  	 */
>  	if (notify)
>  		srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
> +
>  	list_del_rcu(&opp->node);
>  	call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
>  

Please remove this hunk of noise.

> @@ -675,6 +692,100 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
>  }
>  
>  /**
> + * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> + * @dev:	device for which we do this operation
> + * @np:		device node
> + *
> + * This function adds an opp definition to the opp list and returns status. The
> + * opp can be controlled using dev_pm_opp_enable/disable functions and may be
> + * removed by dev_pm_opp_remove.
> + *
> + * Locking: The internal device_opp 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.
> + *
> + * Return:
> + * 0		On success OR
> + *		Duplicate OPPs (both freq and volt are same) and opp->available
> + * -EEXIST	Freq are same and volt are different OR
> + *		Duplicate OPPs (both freq and volt are same) and !opp->available
> + * -ENOMEM	Memory allocation failure
> + * -EINVAL	Failed parsing the OPP node
> + */
> +static int _opp_add_static_v2(struct device *dev, struct device_node *np)
> +{
> +	struct device_opp *dev_opp;
> +	struct dev_pm_opp *new_opp;
> +	int ret;
> +
> +	/* Hold our list modification lock here */
> +	mutex_lock(&dev_opp_list_lock);
> +
> +	new_opp = _allocate_opp(dev, &dev_opp);
> +	if (!new_opp) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	ret = of_property_read_u32(np, "opp-hz", (u32 *)&new_opp->rate);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: opp-hz not found\n", __func__);
> +		goto free_opp;
> +	}
> +
> +	if (of_get_property(np, "turbo-mode", NULL))
> +		new_opp->turbo = true;

new_opp->turbo = of_property_read_bool(np, "turbo-mode");

> +
> +	new_opp->np = np;
> +	new_opp->dynamic = false;
> +	new_opp->available = true;
> +
> +	/*
> +	 * TODO: Support multiple regulators
> +	 *
> +	 * read opp-microvolt array
> +	 */
> +	ret = of_property_count_u32_elems(np, "opp-microvolt");
> +	if (ret == 1 || ret == 3) {
> +		/* There can be one or three elements here */
> +		ret = of_property_read_u32_array(np, "opp-microvolt",
> +						 (u32 *)&new_opp->u_volt, ret);

It seems fragile to rely on the struct packing here. Maybe the same
comment in the struct should be copied here, and possibly some better
way of doing this so the code can't be subtly broken?

> +		if (ret) {
> +			dev_err(dev, "%s: error parsing opp-microvolt: %d\n",
> +				__func__, ret);
> +			goto free_opp;
> +		}
> +	}
> +
> +	of_property_read_u32(np, "opp-microamp", (u32 *)&new_opp->u_amp);
> +
> +	ret = _opp_add(new_opp, dev_opp);
> +	if (ret)
> +		goto free_opp;
> +
> +	pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu\n",
> +		 __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt,
> +		 new_opp->u_volt_min, new_opp->u_volt_max);
> +
> +	mutex_unlock(&dev_opp_list_lock);

We can pr_debug after the unlock?

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

[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Viresh Kumar]

On 01-07-15, 18:13, Stephen Boyd wrote:
> On 06/15/2015 04:57 AM, Viresh Kumar wrote:
> > ---
> >  drivers/base/power/opp.c | 238 ++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 213 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> > index 2ac48ff9c1ef..3198c3e77224 100644
> > --- a/drivers/base/power/opp.c
> > +++ b/drivers/base/power/opp.c
> > @@ -49,12 +49,17 @@
> >   *		are protected by the dev_opp_list_lock for integrity.
> >   *		IMPORTANT: the opp nodes should be maintained in increasing
> >   *		order.
> > - * @dynamic:	not-created from static DT entries.
> >   * @available:	true/false - marks if this OPP as available or not
> > + * @dynamic:	not-created from static DT entries.
> 
> Why move dynamic?

To match its position, as it is present in the struct below. Yes it
could have been done in a separate patch, but its also fine to fix
such silly mistakes in another patch :)

> > + * @turbo:	true if turbo (boost) OPP
> >   * @rate:	Frequency in hertz
> > - * @u_volt:	Nominal voltage in microvolts corresponding to this OPP
> > + * @u_volt:	Target voltage in microvolts corresponding to this OPP
> > + * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
> > + * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
> > + * @u_amp:	Maximum current drawn by the device in microamperes
> >   * @dev_opp:	points back to the device_opp struct this opp belongs to
> >   * @rcu_head:	RCU callback head used for deferred freeing
> > + * @np:		OPP's device node.
> >   *
> >   * This structure stores the OPP information for a given device.
> >   */
> > @@ -63,11 +68,22 @@ struct dev_pm_opp {
> >  
> >  	bool available;
> >  	bool dynamic;
> > +	bool turbo;
> >  	unsigned long rate;
> > +
> > +	/*
> > +	 * Order in which u_volt{_min|_max} are present in this structure
> > +	 * shouldn't be changed.
> > +	 */
> >  	unsigned long u_volt;
> > +	unsigned long u_volt_min;
> > +	unsigned long u_volt_max;
> > +	unsigned long u_amp;
> >  
> >  	struct device_opp *dev_opp;
> >  	struct rcu_head rcu_head;
> > +
> > +	struct device_node *np;
> >  };
> >  
> >  /**
> > @@ -501,6 +517,7 @@ static void _opp_remove(struct device_opp *dev_opp,
> >  	 */
> >  	if (notify)
> >  		srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
> > +
> >  	list_del_rcu(&opp->node);
> >  	call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
> >  
> 
> Please remove this hunk of noise.

Sigh

> > @@ -675,6 +692,100 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
> >  }
> >  
> >  /**
> > + * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> > + * @dev:	device for which we do this operation
> > + * @np:		device node
> > + *
> > + * This function adds an opp definition to the opp list and returns status. The
> > + * opp can be controlled using dev_pm_opp_enable/disable functions and may be
> > + * removed by dev_pm_opp_remove.
> > + *
> > + * Locking: The internal device_opp 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.
> > + *
> > + * Return:
> > + * 0		On success OR
> > + *		Duplicate OPPs (both freq and volt are same) and opp->available
> > + * -EEXIST	Freq are same and volt are different OR
> > + *		Duplicate OPPs (both freq and volt are same) and !opp->available
> > + * -ENOMEM	Memory allocation failure
> > + * -EINVAL	Failed parsing the OPP node
> > + */
> > +static int _opp_add_static_v2(struct device *dev, struct device_node *np)
> > +{
> > +	struct device_opp *dev_opp;
> > +	struct dev_pm_opp *new_opp;
> > +	int ret;
> > +
> > +	/* Hold our list modification lock here */
> > +	mutex_lock(&dev_opp_list_lock);
> > +
> > +	new_opp = _allocate_opp(dev, &dev_opp);
> > +	if (!new_opp) {
> > +		ret = -ENOMEM;
> > +		goto unlock;
> > +	}
> > +
> > +	ret = of_property_read_u32(np, "opp-hz", (u32 *)&new_opp->rate);
> > +	if (ret < 0) {
> > +		dev_err(dev, "%s: opp-hz not found\n", __func__);
> > +		goto free_opp;
> > +	}
> > +
> > +	if (of_get_property(np, "turbo-mode", NULL))
> > +		new_opp->turbo = true;
> 
> new_opp->turbo = of_property_read_bool(np, "turbo-mode");

Sure.

> > +
> > +	new_opp->np = np;
> > +	new_opp->dynamic = false;
> > +	new_opp->available = true;
> > +
> > +	/*
> > +	 * TODO: Support multiple regulators
> > +	 *
> > +	 * read opp-microvolt array
> > +	 */
> > +	ret = of_property_count_u32_elems(np, "opp-microvolt");
> > +	if (ret == 1 || ret == 3) {
> > +		/* There can be one or three elements here */
> > +		ret = of_property_read_u32_array(np, "opp-microvolt",
> > +						 (u32 *)&new_opp->u_volt, ret);
> 
> It seems fragile to rely on the struct packing here. Maybe the same
> comment in the struct should be copied here, and possibly some better
> way of doing this so the code can't be subtly broken?

Any example of how things will break? Aren't these guaranteed to be
present at 3 consecutive 32 bit positions ?

> > +
> > +	pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu\n",
> > +		 __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt,
> > +		 new_opp->u_volt_min, new_opp->u_volt_max);
> > +
> > +	mutex_unlock(&dev_opp_list_lock);
> 
> We can pr_debug after the unlock?

Okay

-- 
viresh

[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Stephen Boyd]

On 07/01/2015 11:38 PM, Viresh Kumar wrote:
> On 01-07-15, 18:13, Stephen Boyd wrote:
>> On 06/15/2015 04:57 AM, Viresh Kumar wrote:
>>> ---
>>>  drivers/base/power/opp.c | 238 ++++++++++++++++++++++++++++++++++++++++++-----
>>>  1 file changed, 213 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>>> index 2ac48ff9c1ef..3198c3e77224 100644
>>> --- a/drivers/base/power/opp.c
>>> +++ b/drivers/base/power/opp.c
>>> @@ -49,12 +49,17 @@
>>>   *		are protected by the dev_opp_list_lock for integrity.
>>>   *		IMPORTANT: the opp nodes should be maintained in increasing
>>>   *		order.
>>> - * @dynamic:	not-created from static DT entries.
>>>   * @available:	true/false - marks if this OPP as available or not
>>> + * @dynamic:	not-created from static DT entries.
>> Why move dynamic?
> To match its position, as it is present in the struct below. Yes it
> could have been done in a separate patch, but its also fine to fix
> such silly mistakes in another patch :)
>
>

Oh I thought kernel-doc didn't care what order things were documented
in, so moving it around doesn't really help unless someone cares that
they match the struct ordering.

>>> +
>>> +	new_opp->np = np;
>>> +	new_opp->dynamic = false;
>>> +	new_opp->available = true;
>>> +
>>> +	/*
>>> +	 * TODO: Support multiple regulators
>>> +	 *
>>> +	 * read opp-microvolt array
>>> +	 */
>>> +	ret = of_property_count_u32_elems(np, "opp-microvolt");
>>> +	if (ret == 1 || ret == 3) {
>>> +		/* There can be one or three elements here */
>>> +		ret = of_property_read_u32_array(np, "opp-microvolt",
>>> +						 (u32 *)&new_opp->u_volt, ret);
>> It seems fragile to rely on the struct packing here. Maybe the same
>> comment in the struct should be copied here, and possibly some better
>> way of doing this so the code can't be subtly broken?
> Any example of how things will break? Aren't these guaranteed to be
> present at 3 consecutive 32 bit positions ?

I'm mostly worried about someone jumbling fields in the struct. Maybe
I'm too paranoid... Maybe we can have some sort of BUILD_BUG_ON() check
here?

BUILD_BUG_ON(offsetof(struct dev_pm_opp, u_volt_max) - offsetof(struct
dev_pm_opp, u_volt) != sizeof(u32) * 3);

Have you tried compiling that on 64-bit? sizeof(unsigned long) !=
sizeof(u32).

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

[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Viresh Kumar]

On 02-07-15, 09:07, Stephen Boyd wrote:
> Oh I thought kernel-doc didn't care what order things were documented
> in, so moving it around doesn't really help unless someone cares that
> they match the struct ordering.

That was just for better readability, nothing more. And I really
prefer that. Consider a case where we have 15 fields in a struct and
the doc comment is all mixed up. Would be very difficult to read
explanations of individual fields. Of course you can do a simple
search in vim to match fields with comments, but its really good if we
follow the same order in comments as well.

Anyway, will drop it from this patch at least :)

> >>> +
> >>> +	new_opp->np = np;
> >>> +	new_opp->dynamic = false;
> >>> +	new_opp->available = true;
> >>> +
> >>> +	/*
> >>> +	 * TODO: Support multiple regulators
> >>> +	 *
> >>> +	 * read opp-microvolt array
> >>> +	 */
> >>> +	ret = of_property_count_u32_elems(np, "opp-microvolt");
> >>> +	if (ret == 1 || ret == 3) {
> >>> +		/* There can be one or three elements here */
> >>> +		ret = of_property_read_u32_array(np, "opp-microvolt",
> >>> +						 (u32 *)&new_opp->u_volt, ret);
> >> It seems fragile to rely on the struct packing here. Maybe the same
> >> comment in the struct should be copied here, and possibly some better
> >> way of doing this so the code can't be subtly broken?
> > Any example of how things will break? Aren't these guaranteed to be
> > present at 3 consecutive 32 bit positions ?
> 
> I'm mostly worried about someone jumbling fields in the struct. Maybe
> I'm too paranoid... Maybe we can have some sort of BUILD_BUG_ON() check
> here?
> 
> BUILD_BUG_ON(offsetof(struct dev_pm_opp, u_volt_max) - offsetof(struct
> dev_pm_opp, u_volt) != sizeof(u32) * 3);

> Have you tried compiling that on 64-bit? sizeof(unsigned long) !=
> sizeof(u32).

I see how it will break for 64 bit systems now, fixed it now.

-------------------8<---------------

Message-Id: <dc35329bc328e0ecb8534a3e1709a0d12cadb311.1435903622.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Mon, 15 Jun 2015 16:27:21 +0530
Subject: [PATCH] opp: Add support to parse "operating-points-v2" bindings

This adds support in OPP library to parse and create list of OPPs from
operating-points-v2 bindings. It takes care of most of the properties of
new bindings (except shared-opp, which will be handled separately).

For backward compatibility, we keep supporting earlier bindings. We try
to search for the new bindings first, in case they aren't present we
look for the old deprecated ones.

There are few things marked as TODO:
- Support for multiple OPP tables
- Support for multiple regulators

They should be fixed separately.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 249 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 225 insertions(+), 24 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index df5e5f14c368..c5829c86aa2a 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -51,10 +51,15 @@
  *		order.
  * @dynamic:	not-created from static DT entries.
  * @available:	true/false - marks if this OPP as available or not
+ * @turbo:	true if turbo (boost) OPP
  * @rate:	Frequency in hertz
- * @u_volt:	Nominal voltage in microvolts corresponding to this OPP
+ * @u_volt:	Target voltage in microvolts corresponding to this OPP
+ * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
+ * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
+ * @u_amp:	Maximum current drawn by the device in microamperes
  * @dev_opp:	points back to the device_opp struct this opp belongs to
  * @rcu_head:	RCU callback head used for deferred freeing
+ * @np:		OPP's device node.
  *
  * This structure stores the OPP information for a given device.
  */
@@ -63,11 +68,18 @@ struct dev_pm_opp {
 
 	bool available;
 	bool dynamic;
+	bool turbo;
 	unsigned long rate;
+
 	unsigned long u_volt;
+	unsigned long u_volt_min;
+	unsigned long u_volt_max;
+	unsigned long u_amp;
 
 	struct device_opp *dev_opp;
 	struct rcu_head rcu_head;
+
+	struct device_node *np;
 };
 
 /**
@@ -674,6 +686,118 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 	return ret;
 }
 
+/* TODO: Support multiple regulators */
+static int opp_get_microvolt(struct dev_pm_opp *opp, struct device *dev)
+{
+	u32 microvolt[3] = {0};
+	int count, ret;
+
+	count = of_property_count_u32_elems(opp->np, "opp-microvolt");
+	if (!count)
+		return 0;
+
+	/* There can be one or three elements here */
+	if (count != 1 && count != 3) {
+		dev_err(dev, "%s: Invalid number of elements in opp-microvolt property (%d)\n",
+			__func__, count);
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32_array(opp->np, "opp-microvolt", microvolt,
+					 count);
+	if (ret) {
+		dev_err(dev, "%s: error parsing opp-microvolt: %d\n", __func__,
+			ret);
+		return -EINVAL;
+	}
+
+	opp->u_volt = microvolt[0];
+	opp->u_volt_min = microvolt[1];
+	opp->u_volt_max = microvolt[2];
+
+	return 0;
+}
+
+/**
+ * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
+ * @dev:	device for which we do this operation
+ * @np:		device node
+ *
+ * This function adds an opp definition to the opp list and returns status. The
+ * opp can be controlled using dev_pm_opp_enable/disable functions and may be
+ * removed by dev_pm_opp_remove.
+ *
+ * Locking: The internal device_opp 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.
+ *
+ * Return:
+ * 0		On success OR
+ *		Duplicate OPPs (both freq and volt are same) and opp->available
+ * -EEXIST	Freq are same and volt are different OR
+ *		Duplicate OPPs (both freq and volt are same) and !opp->available
+ * -ENOMEM	Memory allocation failure
+ * -EINVAL	Failed parsing the OPP node
+ */
+static int _opp_add_static_v2(struct device *dev, struct device_node *np)
+{
+	struct device_opp *dev_opp;
+	struct dev_pm_opp *new_opp;
+	int ret;
+
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
+	new_opp = _allocate_opp(dev, &dev_opp);
+	if (!new_opp) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	ret = of_property_read_u32(np, "opp-hz", (u32 *)&new_opp->rate);
+	if (ret < 0) {
+		dev_err(dev, "%s: opp-hz not found\n", __func__);
+		goto free_opp;
+	}
+
+	new_opp->turbo = of_property_read_bool(np, "turbo-mode");
+
+	new_opp->np = np;
+	new_opp->dynamic = false;
+	new_opp->available = true;
+
+	ret = opp_get_microvolt(new_opp, dev);
+	if (ret)
+		goto free_opp;
+
+	of_property_read_u32(np, "opp-microamp", (u32 *)&new_opp->u_amp);
+
+	ret = _opp_add(new_opp, dev_opp);
+	if (ret)
+		goto free_opp;
+
+	mutex_unlock(&dev_opp_list_lock);
+
+	pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu\n",
+		 __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt,
+		 new_opp->u_volt_min, new_opp->u_volt_max);
+
+	/*
+	 * Notify the changes in the availability of the operable
+	 * frequency/voltage list.
+	 */
+	srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp);
+	return 0;
+
+free_opp:
+	_opp_remove(dev_opp, new_opp, false);
+unlock:
+	mutex_unlock(&dev_opp_list_lock);
+	return ret;
+}
+
 /**
  * dev_pm_opp_add()  - Add an OPP table from a table definitions
  * @dev:	device for which we do this operation
@@ -905,29 +1029,64 @@ void of_free_opp_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(of_free_opp_table);
 
-/**
- * of_init_opp_table() - Initialize opp table from device tree
- * @dev:	device pointer used to lookup device OPPs.
- *
- * Register the initial OPP table with the OPP library for given device.
- *
- * Locking: The internal device_opp and opp structures are RCU protected.
- * Hence this function indirectly 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.
- *
- * Return:
- * 0		On success OR
- *		Duplicate OPPs (both freq and volt are same) and opp->available
- * -EEXIST	Freq are same and volt are different OR
- *		Duplicate OPPs (both freq and volt are same) and !opp->available
- * -ENOMEM	Memory allocation failure
- * -ENODEV	when 'operating-points' property is not found or is invalid data
- *		in device node.
- * -ENODATA	when empty 'operating-points' property is found
- */
-int of_init_opp_table(struct device *dev)
+/* Returns opp descriptor node from its phandle. Caller must do of_node_put() */
+static struct device_node *
+_of_get_opp_desc_node_from_prop(struct device *dev, const struct property *prop)
+{
+	struct device_node *opp_np;
+
+	opp_np = of_find_node_by_phandle(be32_to_cpup(prop->value));
+	if (!opp_np) {
+		dev_err(dev, "%s: Prop: %s contains invalid opp desc phandle\n",
+			__func__, prop->name);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return opp_np;
+}
+
+/* Initializes OPP tables based on new bindings */
+static int _of_init_opp_table_v2(struct device *dev,
+				 const struct property *prop)
+{
+	struct device_node *opp_np, *np;
+	int ret = 0, count = 0;
+
+	if (!prop->value)
+		return -ENODATA;
+
+	/* Get opp node */
+	opp_np = _of_get_opp_desc_node_from_prop(dev, prop);
+	if (IS_ERR(opp_np))
+		return PTR_ERR(opp_np);
+
+	/* We have opp-list node now, iterate over it and add OPPs */
+	for_each_available_child_of_node(opp_np, np) {
+		count++;
+
+		ret = _opp_add_static_v2(dev, np);
+		if (ret) {
+			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
+				ret);
+			break;
+		}
+	}
+
+	/* There should be one of more OPP defined */
+	if (WARN_ON(!count))
+		goto put_opp_np;
+
+	if (ret)
+		of_free_opp_table(dev);
+
+put_opp_np:
+	of_node_put(opp_np);
+
+	return ret;
+}
+
+/* Initializes OPP tables based on old-deprecated bindings */
+static int _of_init_opp_table_v1(struct device *dev)
 {
 	const struct property *prop;
 	const __be32 *val;
@@ -962,5 +1121,47 @@ int of_init_opp_table(struct device *dev)
 
 	return 0;
 }
+
+/**
+ * of_init_opp_table() - Initialize opp table from device tree
+ * @dev:	device pointer used to lookup device OPPs.
+ *
+ * Register the initial OPP table with the OPP library for given device.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function indirectly 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.
+ *
+ * Return:
+ * 0		On success OR
+ *		Duplicate OPPs (both freq and volt are same) and opp->available
+ * -EEXIST	Freq are same and volt are different OR
+ *		Duplicate OPPs (both freq and volt are same) and !opp->available
+ * -ENOMEM	Memory allocation failure
+ * -ENODEV	when 'operating-points' property is not found or is invalid data
+ *		in device node.
+ * -ENODATA	when empty 'operating-points' property is found
+ */
+int of_init_opp_table(struct device *dev)
+{
+	const struct property *prop;
+
+	/*
+	 * OPPs have two version of bindings now. The older one is deprecated,
+	 * try for the new binding first.
+	 */
+	prop = of_find_property(dev->of_node, "operating-points-v2", NULL);
+	if (!prop) {
+		/*
+		 * Try old-deprecated bindings for backward compatibility with
+		 * older dtbs.
+		 */
+		return _of_init_opp_table_v1(dev);
+	}
+
+	return _of_init_opp_table_v2(dev, prop);
+}
 EXPORT_SYMBOL_GPL(of_init_opp_table);
 #endif

[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday, June 15, 2015 05:27:31 PM Viresh Kumar wrote:
> This adds support in OPP library to parse and create list of OPPs from
> operating-points-v2 bindings. It takes care of most of the properties of
> new bindings (except shared-opp, which will be handled separately).
> 
> For backward compatibility, we keep supporting earlier bindings. We try
> to search for the new bindings first, in case they aren't present we
> look for the old deprecated ones.
> 
> There are few things marked as TODO:
> - Support for multiple OPP tables
> - Support for multiple regulators
> 
> They should be fixed separately.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/opp.c | 238 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 213 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 2ac48ff9c1ef..3198c3e77224 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c

[...]

> @@ -675,6 +692,100 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
>  }
>  
>  /**
> + * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> + * @dev:	device for which we do this operation
> + * @np:		device node
> + *
> + * This function adds an opp definition to the opp list and returns status. The
> + * opp can be controlled using dev_pm_opp_enable/disable functions and may be
> + * removed by dev_pm_opp_remove.
> + *
> + * Locking: The internal device_opp 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.
> + *
> + * Return:
> + * 0		On success OR
> + *		Duplicate OPPs (both freq and volt are same) and opp->available
> + * -EEXIST	Freq are same and volt are different OR
> + *		Duplicate OPPs (both freq and volt are same) and !opp->available
> + * -ENOMEM	Memory allocation failure
> + * -EINVAL	Failed parsing the OPP node
> + */
> +static int _opp_add_static_v2(struct device *dev, struct device_node *np)
> +{
> +	struct device_opp *dev_opp;
> +	struct dev_pm_opp *new_opp;
> +	int ret;
> +
> +	/* Hold our list modification lock here */
> +	mutex_lock(&dev_opp_list_lock);
> +
> +	new_opp = _allocate_opp(dev, &dev_opp);
> +	if (!new_opp) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	ret = of_property_read_u32(np, "opp-hz", (u32 *)&new_opp->rate);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: opp-hz not found\n", __func__);
> +		goto free_opp;
> +	}

Isn't using u32 for storing frequency (in Hz) too small by today's
standards?

[ Please note that the old v1 binding uses kHz not Hz. ]

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Viresh Kumar]

On 08-07-15, 15:41, Bartlomiej Zolnierkiewicz wrote:
> Isn't using u32 for storing frequency (in Hz) too small by today's
> standards?
> 
> [ Please note that the old v1 binding uses kHz not Hz. ]

Hmm, probably yes we need it to be u64. Thanks for pointing out.

-- 
viresh

[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Bartlomiej Zolnierkiewicz]

Hi,

On Thursday, July 09, 2015 10:48:46 AM Viresh Kumar wrote:
> On 08-07-15, 15:41, Bartlomiej Zolnierkiewicz wrote:
> > Isn't using u32 for storing frequency (in Hz) too small by today's
> > standards?
> > 
> > [ Please note that the old v1 binding uses kHz not Hz. ]
> 
> Hmm, probably yes we need it to be u64. Thanks for pointing out.

There is also a minor issue with of_init_opp_table() documentation
(the function can now return -EINVAL in some cases).  Except these
two things the patch looks fine and once it is fixed you can add:

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Viresh Kumar]

On 24-07-15, 20:02, Bartlomiej Zolnierkiewicz wrote:
> There is also a minor issue with of_init_opp_table() documentation
> (the function can now return -EINVAL in some cases).  Except these
> two things the patch looks fine and once it is fixed you can add:
> 
> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Fixed the documentation issue as:

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index c4ca176061b8..3ad93a7e76e5 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -1144,6 +1144,7 @@ static int _of_init_opp_table_v1(struct device *dev)
  * -ENODEV     when 'operating-points' property is not found or is invalid data
  *             in device node.
  * -ENODATA    when empty 'operating-points' property is found
+ * -EINVAL     when invalid entries are found in opp-v2 table
  */
 int of_init_opp_table(struct device *dev)
 {


Please give your reviewed-by again as I wouldn't be fixing the u32
type issue for now and wasn't sure if you will give your RBY without
that :)

-- 
viresh

[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Viresh Kumar]

On 08-07-15, 15:41, Bartlomiej Zolnierkiewicz wrote:
> Isn't using u32 for storing frequency (in Hz) too small by today's
> standards?
> 
> [ Please note that the old v1 binding uses kHz not Hz. ]

I have thought about this a bit more and I am somewhat confused. Yes I
agree that u32 isn't big enough for frequencies in Hz, i.e. Max value
of 4294967295 ~ 4.29 GHz.

But the bigger problem lies with the clk API that we have today. It
declares clk-rate as a unsigned-long which is 32 bits on a 32 bit
machine and 64 bits on a 64 bit machine. And every single piece of
code reading "clock-frequency" DT property, reads it as a 32 bit value
as we reserve only a single cell for it.

Now, if we wanna change that, we need to start changing from the
clk-API itself and that's not gonna be a small task and I would leave
it to Mike/Stephen for obvious reasons :)

So, I will keep this code in sync with rest of the kernel and lets see
what Mike has to say.

-- 
viresh

[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Stephen Boyd]

On 07/26/2015 08:02 PM, Viresh Kumar wrote:
> On 08-07-15, 15:41, Bartlomiej Zolnierkiewicz wrote:
>> Isn't using u32 for storing frequency (in Hz) too small by today's
>> standards?
>>
>> [ Please note that the old v1 binding uses kHz not Hz. ]
> I have thought about this a bit more and I am somewhat confused. Yes I
> agree that u32 isn't big enough for frequencies in Hz, i.e. Max value
> of 4294967295 ~ 4.29 GHz.
>
> But the bigger problem lies with the clk API that we have today. It
> declares clk-rate as a unsigned-long which is 32 bits on a 32 bit
> machine and 64 bits on a 64 bit machine. And every single piece of
> code reading "clock-frequency" DT property, reads it as a 32 bit value
> as we reserve only a single cell for it.
>
> Now, if we wanna change that, we need to start changing from the
> clk-API itself and that's not gonna be a small task and I would leave
> it to Mike/Stephen for obvious reasons :)
>
> So, I will keep this code in sync with rest of the kernel and lets see
> what Mike has to say.
>

There is talk about moving to 64 bits for the frequency in the clk API.
It's not actively being developed though and I'm not sure when we'll get
there. From a DT perspective though, I don't see why it would be bad to
have two cells instead of one cell for the frequency here. It would
allow up to 64 bits of frequency so that when we change the clk API we
won't need to do anything in the OPP bindings to handle it. Just because
we have problems in the kernel doesn't mean we should put the same
problems into DT.

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

[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Viresh Kumar]

On 28-07-15, 16:03, Stephen Boyd wrote:
> There is talk about moving to 64 bits for the frequency in the clk API.
> It's not actively being developed though and I'm not sure when we'll get
> there. From a DT perspective though, I don't see why it would be bad to
> have two cells instead of one cell for the frequency here. It would
> allow up to 64 bits of frequency so that when we change the clk API we
> won't need to do anything in the OPP bindings to handle it. Just because
> we have problems in the kernel doesn't mean we should put the same
> problems into DT.

Okay, I am going to send a patch for that as well and will respin the
series again.

-- 
viresh

[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Viresh Kumar]

On 28-07-15, 16:03, Stephen Boyd wrote:
> There is talk about moving to 64 bits for the frequency in the clk API.
> It's not actively being developed though and I'm not sure when we'll get
> there. From a DT perspective though, I don't see why it would be bad to
> have two cells instead of one cell for the frequency here. It would
> allow up to 64 bits of frequency so that when we change the clk API we
> won't need to do anything in the OPP bindings to handle it. Just because
> we have problems in the kernel doesn't mean we should put the same
> problems into DT.

Updated the code as:

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index d9ebbd739b23..73e4e1ce60a1 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -743,6 +743,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 {
        struct device_opp *dev_opp;
        struct dev_pm_opp *new_opp;
+       u64 rate;
        int ret;
 
        /* Hold our list modification lock here */
@@ -754,12 +755,18 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
                goto unlock;
        }
 
-       ret = of_property_read_u32(np, "opp-hz", (u32 *)&new_opp->rate);
+       ret = of_property_read_u64(np, "opp-hz", &rate);
        if (ret < 0) {
                dev_err(dev, "%s: opp-hz not found\n", __func__);
                goto free_opp;
        }
 
+       /*
+        * Rate is defined as an unsigned long in clk API, and so casting
+        * explicitly to its type. Must be fixed once rate is 64 bit
+        * guaranteed in clk API.
+        */
+       new_opp->rate = (unsigned long)rate;
        new_opp->turbo = of_property_read_bool(np, "turbo-mode");
 
        new_opp->np = np;


-- 
viresh

[PATCH 06/10] OPP: Add clock-latency-ns support

[Viresh Kumar]

With "operating-points-v2" bindings, clock-latency is defined per OPP.
Users of this value expect a single value which defines the latency to
switch to any clock rate. Find maximum clock-latency-ns from the OPP
table to service requests from such users.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 41 +++++++++++++++++++++++++++++++++++++++--
 include/linux/pm_opp.h   |  6 ++++++
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 3198c3e77224..fe79e5146a29 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -57,6 +57,8 @@
  * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
  * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
  * @u_amp:	Maximum current drawn by the device in microamperes
+ * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
+ *		frequency from any other OPP's frequency.
  * @dev_opp:	points back to the device_opp struct this opp belongs to
  * @rcu_head:	RCU callback head used for deferred freeing
  * @np:		OPP's device node.
@@ -79,6 +81,7 @@ struct dev_pm_opp {
 	unsigned long u_volt_min;
 	unsigned long u_volt_max;
 	unsigned long u_amp;
+	unsigned long clock_latency_ns;
 
 	struct device_opp *dev_opp;
 	struct rcu_head rcu_head;
@@ -113,6 +116,8 @@ struct device_opp {
 	struct srcu_notifier_head srcu_head;
 	struct rcu_head rcu_head;
 	struct list_head opp_list;
+
+	unsigned long clock_latency_ns_max;
 };
 
 /*
@@ -230,6 +235,32 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
 
 /**
+ * dev_pm_opp_get_max_clock_latency() - Get max clock latency in nanoseconds
+ * @dev:	device for which we do this operation
+ *
+ * Return: This function returns the max clock latency in nanoseconds.
+ *
+ * Locking: This function takes rcu_read_lock().
+ */
+unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev)
+{
+	struct device_opp *dev_opp;
+	unsigned long clock_latency_ns;
+
+	rcu_read_lock();
+
+	dev_opp = _find_device_opp(dev);
+	if (IS_ERR(dev_opp))
+		clock_latency_ns = 0;
+	else
+		clock_latency_ns = dev_opp->clock_latency_ns_max;
+
+	rcu_read_unlock();
+	return clock_latency_ns;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_clock_latency);
+
+/**
  * dev_pm_opp_get_opp_count() - Get number of opps available in the opp list
  * @dev:	device for which we do this operation
  *
@@ -741,6 +772,8 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 	new_opp->np = np;
 	new_opp->dynamic = false;
 	new_opp->available = true;
+	of_property_read_u32(np, "clock-latency-ns",
+			     (u32 *)&new_opp->clock_latency_ns);
 
 	/*
 	 * TODO: Support multiple regulators
@@ -765,9 +798,13 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 	if (ret)
 		goto free_opp;
 
-	pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu\n",
+	if (new_opp->clock_latency_ns > dev_opp->clock_latency_ns_max)
+		dev_opp->clock_latency_ns_max = new_opp->clock_latency_ns;
+
+	pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n",
 		 __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt,
-		 new_opp->u_volt_min, new_opp->u_volt_max);
+		 new_opp->u_volt_min, new_opp->u_volt_max,
+		 new_opp->clock_latency_ns);
 
 	mutex_unlock(&dev_opp_list_lock);
 
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index cec2d4540914..20324b579adc 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -31,6 +31,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
 unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp);
 
 int dev_pm_opp_get_opp_count(struct device *dev);
+unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev);
 
 struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 					      unsigned long freq,
@@ -67,6 +68,11 @@ static inline int dev_pm_opp_get_opp_count(struct device *dev)
 	return 0;
 }
 
+static inline unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev)
+{
+	return 0;
+}
+
 static inline struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 					unsigned long freq, bool available)
 {
-- 
2.4.0

[PATCH 06/10] OPP: Add clock-latency-ns support

[Stephen Boyd]

On 06/15, Viresh Kumar wrote:
> With "operating-points-v2" bindings, clock-latency is defined per OPP.
> Users of this value expect a single value which defines the latency to
> switch to any clock rate. Find maximum clock-latency-ns from the OPP
> table to service requests from such users.
> 
> 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 07/10] opp: Add OPP sharing information to OPP library

[Viresh Kumar]

An opp can be shared by multiple devices, for example its very common
for CPUs to share the OPPs, i.e. when they share clock/voltage rails.

This patch adds support of shared OPPs to the OPP library.

Instead of a single device, dev_opp will not contain a list of devices
that use it. It also senses if the device (we are trying to initialize
OPPs for) shares OPPs with a device added earlier and in that case we
update the list of devices managed by OPPs instead of duplicating OPPs
again.

The same infrastructure will be used for the old OPP bindings, with
later patches.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 176 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 152 insertions(+), 24 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index fe79e5146a29..6b554e417b1f 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -90,16 +90,33 @@ struct dev_pm_opp {
 };
 
 /**
+ * struct device_list_opp - devices managed by 'struct device_opp'
+ * @node:	list node
+ * @dev:	device to which the struct object belongs
+ * @rcu_head:	RCU callback head used for deferred freeing
+ *
+ * This is an internal data structure maintaining the list of devices that are
+ * managed by 'struct device_opp'.
+ */
+struct device_list_opp {
+	struct list_head node;
+	struct device *dev;
+	struct rcu_head rcu_head;
+};
+
+/**
  * struct device_opp - Device opp structure
  * @node:	list node - contains the devices with OPPs that
  *		have been registered. Nodes once added are not modified in this
  *		list.
  *		RCU usage: nodes are not modified in the list of device_opp,
  *		however addition is possible and is secured by dev_opp_list_lock
- * @dev:	device pointer
  * @srcu_head:	notifier head to notify the OPP availability changes.
  * @rcu_head:	RCU callback head used for deferred freeing
+ * @dev_list:	list of devices that share these OPPs
  * @opp_list:	list of opps
+ * @np:		struct device_node pointer for opp's DT node.
+ * @shared_opp: OPP is shared between multiple devices.
  *
  * This is an internal data structure maintaining the link to opps attached to
  * a device. This structure is not meant to be shared to users as it is
@@ -112,12 +129,14 @@ struct dev_pm_opp {
 struct device_opp {
 	struct list_head node;
 
-	struct device *dev;
 	struct srcu_notifier_head srcu_head;
 	struct rcu_head rcu_head;
+	struct list_head dev_list;
 	struct list_head opp_list;
 
+	struct device_node *np;
 	unsigned long clock_latency_ns_max;
+	bool shared_opp;
 };
 
 /*
@@ -137,6 +156,40 @@ do {									\
 			   "dev_opp_list_lock protection");		\
 } while (0)
 
+static struct device_list_opp *_find_list_dev(struct device *dev,
+					      struct device_opp *dev_opp)
+{
+	struct device_list_opp *list_dev;
+
+	list_for_each_entry(list_dev, &dev_opp->dev_list, node)
+		if (list_dev->dev == dev)
+			return list_dev;
+
+	return NULL;
+}
+
+static struct device_opp *_managed_opp(struct device_node *np)
+{
+	struct device_opp *dev_opp;
+
+	list_for_each_entry_rcu(dev_opp, &dev_opp_list, node)
+		if (dev_opp->np == np) {
+			/*
+			 * Multiple devices can point to the same OPP table and
+			 * so will have same node-pointer, np.
+			 *
+			 * But the OPPs will be considered as shared only if the
+			 * OPP table contains a "opp-shared" property.
+			 */
+			if (dev_opp->shared_opp)
+				return dev_opp;
+			else
+				return NULL;
+		}
+
+	return NULL;
+}
+
 /**
  * _find_device_opp() - find device_opp struct using device pointer
  * @dev:	device pointer used to lookup device OPPs
@@ -153,21 +206,18 @@ do {									\
  */
 static struct device_opp *_find_device_opp(struct device *dev)
 {
-	struct device_opp *tmp_dev_opp, *dev_opp = ERR_PTR(-ENODEV);
+	struct device_opp *dev_opp;
 
 	if (unlikely(IS_ERR_OR_NULL(dev))) {
 		pr_err("%s: Invalid parameters\n", __func__);
 		return ERR_PTR(-EINVAL);
 	}
 
-	list_for_each_entry_rcu(tmp_dev_opp, &dev_opp_list, node) {
-		if (tmp_dev_opp->dev == dev) {
-			dev_opp = tmp_dev_opp;
-			break;
-		}
-	}
+	list_for_each_entry_rcu(dev_opp, &dev_opp_list, node)
+		if (_find_list_dev(dev, dev_opp))
+			return dev_opp;
 
-	return dev_opp;
+	return ERR_PTR(-ENODEV);
 }
 
 /**
@@ -454,6 +504,39 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
+/* List-dev Helpers */
+static void _kfree_list_dev_rcu(struct rcu_head *head)
+{
+	struct device_list_opp *list_dev;
+
+	list_dev = container_of(head, struct device_list_opp, rcu_head);
+	kfree_rcu(list_dev, rcu_head);
+}
+
+static void _remove_list_dev(struct device_list_opp *list_dev,
+			     struct device_opp *dev_opp)
+{
+	list_del(&list_dev->node);
+	call_srcu(&dev_opp->srcu_head.srcu, &list_dev->rcu_head,
+		  _kfree_list_dev_rcu);
+}
+
+static struct device_list_opp *_add_list_dev(struct device *dev,
+					     struct device_opp *dev_opp)
+{
+	struct device_list_opp *list_dev;
+
+	list_dev = kzalloc(sizeof(*list_dev), GFP_KERNEL);
+	if (!list_dev)
+		return NULL;
+
+	/* Initialize list-dev */
+	list_add_rcu(&list_dev->node, &dev_opp->dev_list);
+	list_dev->dev = dev;
+
+	return list_dev;
+}
+
 /**
  * _add_device_opp() - Returns device OPP table
  * @dev:	device for which we do this operation
@@ -466,6 +549,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 static struct device_opp *_add_device_opp(struct device *dev)
 {
 	struct device_opp *dev_opp;
+	struct device_list_opp *list_dev;
 
 	/* Check for existing list for 'dev' first */
 	dev_opp = _find_device_opp(dev);
@@ -480,7 +564,14 @@ static struct device_opp *_add_device_opp(struct device *dev)
 	if (!dev_opp)
 		return NULL;
 
-	dev_opp->dev = dev;
+	INIT_LIST_HEAD(&dev_opp->dev_list);
+
+	list_dev = _add_list_dev(dev, dev_opp);
+	if (!list_dev) {
+		kfree(dev_opp);
+		return NULL;
+	}
+
 	srcu_init_notifier_head(&dev_opp->srcu_head);
 	INIT_LIST_HEAD(&dev_opp->opp_list);
 
@@ -508,9 +599,19 @@ static void _kfree_device_rcu(struct rcu_head *head)
  */
 static void _remove_device_opp(struct device_opp *dev_opp)
 {
+	struct device_list_opp *list_dev;
+
 	if (!list_empty(&dev_opp->opp_list))
 		return;
 
+	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
+				    node);
+
+	_remove_list_dev(list_dev, dev_opp);
+
+	/* dev_list must be empty now */
+	WARN_ON(!list_empty(&dev_opp->dev_list));
+
 	list_del_rcu(&dev_opp->node);
 	call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,
 		  _kfree_device_rcu);
@@ -621,7 +722,8 @@ static struct dev_pm_opp *_allocate_opp(struct device *dev,
 	return opp;
 }
 
-static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp)
+static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
+		    struct device_opp *dev_opp)
 {
 	struct dev_pm_opp *opp = NULL;
 	struct list_head *head = &dev_opp->opp_list;
@@ -639,7 +741,7 @@ static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp)
 
 	/* Duplicate OPPs ? */
 	if (opp && new_opp->rate == opp->rate) {
-		dev_warn(dev_opp->dev,
+		dev_warn(dev,
 			 "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
 			 __func__, opp->rate, opp->u_volt, opp->available,
 			 new_opp->rate, new_opp->u_volt, new_opp->available);
@@ -702,7 +804,7 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 	new_opp->available = true;
 	new_opp->dynamic = dynamic;
 
-	ret = _opp_add(new_opp, dev_opp);
+	ret = _opp_add(dev, new_opp, dev_opp);
 	if (ret)
 		goto free_opp;
 
@@ -794,7 +896,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 
 	of_property_read_u32(np, "opp-microamp", (u32 *)&new_opp->u_amp);
 
-	ret = _opp_add(new_opp, dev_opp);
+	ret = _opp_add(dev, new_opp, dev_opp);
 	if (ret)
 		goto free_opp;
 
@@ -1027,6 +1129,9 @@ void of_free_opp_table(struct device *dev)
 	struct device_opp *dev_opp;
 	struct dev_pm_opp *opp, *tmp;
 
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
 	/* Check for existing list for 'dev' */
 	dev_opp = _find_device_opp(dev);
 	if (IS_ERR(dev_opp)) {
@@ -1037,18 +1142,21 @@ void of_free_opp_table(struct device *dev)
 			     IS_ERR_OR_NULL(dev) ?
 					"Invalid device" : dev_name(dev),
 			     error);
-		return;
+		goto unlock;
 	}
 
-	/* Hold our list modification lock here */
-	mutex_lock(&dev_opp_list_lock);
-
-	/* Free static OPPs */
-	list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) {
-		if (!opp->dynamic)
-			_opp_remove(dev_opp, opp, true);
+	/* Find if dev_opp manages a single device */
+	if (list_is_singular(&dev_opp->dev_list)) {
+		/* Free static OPPs */
+		list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) {
+			if (!opp->dynamic)
+				_opp_remove(dev_opp, opp, true);
+		}
+	} else {
+		_remove_list_dev(_find_list_dev(dev, dev_opp), dev_opp);
 	}
 
+unlock:
 	mutex_unlock(&dev_opp_list_lock);
 }
 EXPORT_SYMBOL_GPL(of_free_opp_table);
@@ -1074,6 +1182,7 @@ static int _of_init_opp_table_v2(struct device *dev,
 				 const struct property *prop)
 {
 	struct device_node *opp_np, *np;
+	struct device_opp *dev_opp;
 	int ret = 0, count = 0;
 
 	if (!prop->value)
@@ -1084,6 +1193,14 @@ static int _of_init_opp_table_v2(struct device *dev,
 	if (IS_ERR(opp_np))
 		return PTR_ERR(opp_np);
 
+	dev_opp = _managed_opp(opp_np);
+	if (dev_opp) {
+		/* OPPs are already managed */
+		if (!_add_list_dev(dev, dev_opp))
+			ret = -ENOMEM;
+		goto put_opp_np;
+	}
+
 	/* We have opp-list node now, iterate over it and add OPPs */
 	for_each_available_child_of_node(opp_np, np) {
 		count++;
@@ -1100,8 +1217,19 @@ static int _of_init_opp_table_v2(struct device *dev,
 	if (WARN_ON(!count))
 		goto put_opp_np;
 
-	if (ret)
+	if (!ret) {
+		if (!dev_opp) {
+			dev_opp = _find_device_opp(dev);
+			if (WARN_ON(!dev_opp))
+				goto put_opp_np;
+		}
+
+		dev_opp->np = opp_np;
+		if (of_get_property(opp_np, "opp-shared", NULL))
+			dev_opp->shared_opp = true;
+	} else {
 		of_free_opp_table(dev);
+	}
 
 put_opp_np:
 	of_node_put(opp_np);
-- 
2.4.0

[PATCH 07/10] opp: Add OPP sharing information to OPP library

[Stephen Boyd]

On 06/15/2015 04:57 AM, Viresh Kumar wrote:
> @@ -112,12 +129,14 @@ struct dev_pm_opp {
>   struct device_opp {
>   	struct list_head node;
>   
> -	struct device *dev;
>   	struct srcu_notifier_head srcu_head;
>   	struct rcu_head rcu_head;
> +	struct list_head dev_list;
>   	struct list_head opp_list;
>   
> +	struct device_node *np;
>   	unsigned long clock_latency_ns_max;
> +	bool shared_opp;
>   };
>   
>   /*
> @@ -137,6 +156,40 @@ do {									\
>   			   "dev_opp_list_lock protection");		\
>   } while (0)
>   
> +static struct device_list_opp *_find_list_dev(struct device *dev,

const device?

> +					      struct device_opp *dev_opp)
> +{
> +	struct device_list_opp *list_dev;
> +
> +	list_for_each_entry(list_dev, &dev_opp->dev_list, node)
> +		if (list_dev->dev == dev)
> +			return list_dev;
> +
> +	return NULL;
> +}
> +
> +static struct device_opp *_managed_opp(struct device_node *np)

const device_node?

> +{
> +	struct device_opp *dev_opp;
> +
> +	list_for_each_entry_rcu(dev_opp, &dev_opp_list, node)
> +		if (dev_opp->np == np) {
> +			/*
> +			 * Multiple devices can point to the same OPP table and
> +			 * so will have same node-pointer, np.
> +			 *
> +			 * But the OPPs will be considered as shared only if the
> +			 * OPP table contains a "opp-shared" property.
> +			 */
> +			if (dev_opp->shared_opp)
> +				return dev_opp;
> +			else
> +				return NULL;
> +		}
> +
> +	return NULL;
> +}
> +
>   /**
>    * _find_device_opp() - find device_opp struct using device pointer
>    * @dev:	device pointer used to lookup device OPPs
> @@ -454,6 +504,39 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
>   }
>   EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
>   
> +/* List-dev Helpers */
> +static void _kfree_list_dev_rcu(struct rcu_head *head)
> +{
> +	struct device_list_opp *list_dev;
> +
> +	list_dev = container_of(head, struct device_list_opp, rcu_head);
> +	kfree_rcu(list_dev, rcu_head);
> +}
> +
> +static void _remove_list_dev(struct device_list_opp *list_dev,
> +			     struct device_opp *dev_opp)
> +{
> +	list_del(&list_dev->node);
> +	call_srcu(&dev_opp->srcu_head.srcu, &list_dev->rcu_head,
> +		  _kfree_list_dev_rcu);
> +}
> +
> +static struct device_list_opp *_add_list_dev(struct device *dev,

Can dev be const here too?

> +					     struct device_opp *dev_opp)
> +{
> +	struct device_list_opp *list_dev;
> +
> +	list_dev = kzalloc(sizeof(*list_dev), GFP_KERNEL);
> +	if (!list_dev)
> +		return NULL;
> +
> +	/* Initialize list-dev */
> +	list_add_rcu(&list_dev->node, &dev_opp->dev_list);
> +	list_dev->dev = dev;
> +
> +	return list_dev;
> +}
> +
>   /**
>    * _add_device_opp() - Returns device OPP table
>    * @dev:	device for which we do this operation
> @@ -466,6 +549,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
>   static struct device_opp *_add_device_opp(struct device *dev)
>   {
>   	struct device_opp *dev_opp;
> +	struct device_list_opp *list_dev;
>   
>   	/* Check for existing list for 'dev' first */
>   	dev_opp = _find_device_opp(dev);
> @@ -621,7 +722,8 @@ static struct dev_pm_opp *_allocate_opp(struct device *dev,
>   	return opp;
>   }
>   
> -static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp)
> +static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
> +		    struct device_opp *dev_opp)
>   {
>   	struct dev_pm_opp *opp = NULL;
>   	struct list_head *head = &dev_opp->opp_list;
>
>
>
> @@ -1100,8 +1217,19 @@ static int _of_init_opp_table_v2(struct device *dev,
>   	if (WARN_ON(!count))
>   		goto put_opp_np;
>   
> -	if (ret)
> +	if (!ret) {
> +		if (!dev_opp) {
> +			dev_opp = _find_device_opp(dev);
> +			if (WARN_ON(!dev_opp))
> +				goto put_opp_np;
> +		}
> +
> +		dev_opp->np = opp_np;
> +		if (of_get_property(opp_np, "opp-shared", NULL))

dev_opp->shared_opp = of_property_read_bool(opp_np, "opp-shared")?

> +			dev_opp->shared_opp = true;
> +	} else {
>   		of_free_opp_table(dev);
> +	}
>   
>   put_opp_np:
>   	of_node_put(opp_np);


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

[PATCH 07/10] opp: Add OPP sharing information to OPP library

[Viresh Kumar]

On 17-07-15, 15:51, Stephen Boyd wrote:
> >+static struct device_list_opp *_find_list_dev(struct device *dev,
> 
> const device?

Diff:

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 1af7ceee5433..ff4a3b267ca7 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -96,7 +96,7 @@ struct dev_pm_opp {
  */
 struct device_list_opp {
        struct list_head node;
-       struct device *dev;
+       const struct device *dev;
        struct rcu_head rcu_head;
 };
 
@@ -152,7 +152,7 @@ do {                                                                        \
                           "dev_opp_list_lock protection");             \
 } while (0)
 
-static struct device_list_opp *_find_list_dev(struct device *dev,
+static struct device_list_opp *_find_list_dev(const struct device *dev,
                                              struct device_opp *dev_opp)
 {
        struct device_list_opp *list_dev;
@@ -164,7 +164,7 @@ static struct device_list_opp *_find_list_dev(struct device *dev,
        return NULL;
 }
 
-static struct device_opp *_managed_opp(struct device_node *np)
+static struct device_opp *_managed_opp(const struct device_node *np)
 {
        struct device_opp *dev_opp;
 
@@ -517,7 +517,7 @@ static void _remove_list_dev(struct device_list_opp *list_dev,
                  _kfree_list_dev_rcu);
 }
 
-static struct device_list_opp *_add_list_dev(struct device *dev,
+static struct device_list_opp *_add_list_dev(const struct device *dev,
                                             struct device_opp *dev_opp)
 {
        struct device_list_opp *list_dev;
@@ -1239,8 +1239,8 @@ static int _of_init_opp_table_v2(struct device *dev,
                }
 
                dev_opp->np = opp_np;
-               if (of_get_property(opp_np, "opp-shared", NULL))
-                       dev_opp->shared_opp = true;
+               dev_opp->shared_opp = of_property_read_bool(opp_np,
+                                                           "opp-shared");
        } else {
                of_free_opp_table(dev);
        }


-----------------------------8<----------------------------

Message-Id: <26828cb0b4b22e5a466c066814bbe39a3c985ca9.1437200372.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 11 Feb 2015 16:16:28 +0800
Subject: [PATCH] opp: Add OPP sharing information to OPP library

An opp can be shared by multiple devices, for example its very common
for CPUs to share the OPPs, i.e. when they share clock/voltage rails.

This patch adds support of shared OPPs to the OPP library.

Instead of a single device, dev_opp will not contain a list of devices
that use it. It also senses if the device (we are trying to initialize
OPPs for) shares OPPs with a device added earlier and in that case we
update the list of devices managed by OPPs instead of duplicating OPPs
again.

The same infrastructure will be used for the old OPP bindings, with
later patches.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 176 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 152 insertions(+), 24 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 940c49152b68..ff4a3b267ca7 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -86,16 +86,33 @@ struct dev_pm_opp {
 };
 
 /**
+ * struct device_list_opp - devices managed by 'struct device_opp'
+ * @node:	list node
+ * @dev:	device to which the struct object belongs
+ * @rcu_head:	RCU callback head used for deferred freeing
+ *
+ * This is an internal data structure maintaining the list of devices that are
+ * managed by 'struct device_opp'.
+ */
+struct device_list_opp {
+	struct list_head node;
+	const struct device *dev;
+	struct rcu_head rcu_head;
+};
+
+/**
  * struct device_opp - Device opp structure
  * @node:	list node - contains the devices with OPPs that
  *		have been registered. Nodes once added are not modified in this
  *		list.
  *		RCU usage: nodes are not modified in the list of device_opp,
  *		however addition is possible and is secured by dev_opp_list_lock
- * @dev:	device pointer
  * @srcu_head:	notifier head to notify the OPP availability changes.
  * @rcu_head:	RCU callback head used for deferred freeing
+ * @dev_list:	list of devices that share these OPPs
  * @opp_list:	list of opps
+ * @np:		struct device_node pointer for opp's DT node.
+ * @shared_opp: OPP is shared between multiple devices.
  *
  * This is an internal data structure maintaining the link to opps attached to
  * a device. This structure is not meant to be shared to users as it is
@@ -108,12 +125,14 @@ struct dev_pm_opp {
 struct device_opp {
 	struct list_head node;
 
-	struct device *dev;
 	struct srcu_notifier_head srcu_head;
 	struct rcu_head rcu_head;
+	struct list_head dev_list;
 	struct list_head opp_list;
 
+	struct device_node *np;
 	unsigned long clock_latency_ns_max;
+	bool shared_opp;
 };
 
 /*
@@ -133,6 +152,40 @@ do {									\
 			   "dev_opp_list_lock protection");		\
 } while (0)
 
+static struct device_list_opp *_find_list_dev(const struct device *dev,
+					      struct device_opp *dev_opp)
+{
+	struct device_list_opp *list_dev;
+
+	list_for_each_entry(list_dev, &dev_opp->dev_list, node)
+		if (list_dev->dev == dev)
+			return list_dev;
+
+	return NULL;
+}
+
+static struct device_opp *_managed_opp(const struct device_node *np)
+{
+	struct device_opp *dev_opp;
+
+	list_for_each_entry_rcu(dev_opp, &dev_opp_list, node)
+		if (dev_opp->np == np) {
+			/*
+			 * Multiple devices can point to the same OPP table and
+			 * so will have same node-pointer, np.
+			 *
+			 * But the OPPs will be considered as shared only if the
+			 * OPP table contains a "opp-shared" property.
+			 */
+			if (dev_opp->shared_opp)
+				return dev_opp;
+			else
+				return NULL;
+		}
+
+	return NULL;
+}
+
 /**
  * _find_device_opp() - find device_opp struct using device pointer
  * @dev:	device pointer used to lookup device OPPs
@@ -149,21 +202,18 @@ do {									\
  */
 static struct device_opp *_find_device_opp(struct device *dev)
 {
-	struct device_opp *tmp_dev_opp, *dev_opp = ERR_PTR(-ENODEV);
+	struct device_opp *dev_opp;
 
 	if (unlikely(IS_ERR_OR_NULL(dev))) {
 		pr_err("%s: Invalid parameters\n", __func__);
 		return ERR_PTR(-EINVAL);
 	}
 
-	list_for_each_entry_rcu(tmp_dev_opp, &dev_opp_list, node) {
-		if (tmp_dev_opp->dev == dev) {
-			dev_opp = tmp_dev_opp;
-			break;
-		}
-	}
+	list_for_each_entry_rcu(dev_opp, &dev_opp_list, node)
+		if (_find_list_dev(dev, dev_opp))
+			return dev_opp;
 
-	return dev_opp;
+	return ERR_PTR(-ENODEV);
 }
 
 /**
@@ -450,6 +500,39 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
+/* List-dev Helpers */
+static void _kfree_list_dev_rcu(struct rcu_head *head)
+{
+	struct device_list_opp *list_dev;
+
+	list_dev = container_of(head, struct device_list_opp, rcu_head);
+	kfree_rcu(list_dev, rcu_head);
+}
+
+static void _remove_list_dev(struct device_list_opp *list_dev,
+			     struct device_opp *dev_opp)
+{
+	list_del(&list_dev->node);
+	call_srcu(&dev_opp->srcu_head.srcu, &list_dev->rcu_head,
+		  _kfree_list_dev_rcu);
+}
+
+static struct device_list_opp *_add_list_dev(const struct device *dev,
+					     struct device_opp *dev_opp)
+{
+	struct device_list_opp *list_dev;
+
+	list_dev = kzalloc(sizeof(*list_dev), GFP_KERNEL);
+	if (!list_dev)
+		return NULL;
+
+	/* Initialize list-dev */
+	list_add_rcu(&list_dev->node, &dev_opp->dev_list);
+	list_dev->dev = dev;
+
+	return list_dev;
+}
+
 /**
  * _add_device_opp() - Find device OPP table or allocate a new one
  * @dev:	device for which we do this operation
@@ -462,6 +545,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 static struct device_opp *_add_device_opp(struct device *dev)
 {
 	struct device_opp *dev_opp;
+	struct device_list_opp *list_dev;
 
 	/* Check for existing list for 'dev' first */
 	dev_opp = _find_device_opp(dev);
@@ -476,7 +560,14 @@ static struct device_opp *_add_device_opp(struct device *dev)
 	if (!dev_opp)
 		return NULL;
 
-	dev_opp->dev = dev;
+	INIT_LIST_HEAD(&dev_opp->dev_list);
+
+	list_dev = _add_list_dev(dev, dev_opp);
+	if (!list_dev) {
+		kfree(dev_opp);
+		return NULL;
+	}
+
 	srcu_init_notifier_head(&dev_opp->srcu_head);
 	INIT_LIST_HEAD(&dev_opp->opp_list);
 
@@ -504,9 +595,19 @@ static void _kfree_device_rcu(struct rcu_head *head)
  */
 static void _remove_device_opp(struct device_opp *dev_opp)
 {
+	struct device_list_opp *list_dev;
+
 	if (!list_empty(&dev_opp->opp_list))
 		return;
 
+	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
+				    node);
+
+	_remove_list_dev(list_dev, dev_opp);
+
+	/* dev_list must be empty now */
+	WARN_ON(!list_empty(&dev_opp->dev_list));
+
 	list_del_rcu(&dev_opp->node);
 	call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,
 		  _kfree_device_rcu);
@@ -616,7 +717,8 @@ static struct dev_pm_opp *_allocate_opp(struct device *dev,
 	return opp;
 }
 
-static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp)
+static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
+		    struct device_opp *dev_opp)
 {
 	struct dev_pm_opp *opp;
 	struct list_head *head = &dev_opp->opp_list;
@@ -632,7 +734,7 @@ static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp)
 			break;
 		} else {
 			/* Duplicate OPPs */
-			dev_warn(dev_opp->dev,
+			dev_warn(dev,
 				 "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
 				 __func__, opp->rate, opp->u_volt,
 				 opp->available, new_opp->rate, new_opp->u_volt,
@@ -698,7 +800,7 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 	new_opp->available = true;
 	new_opp->dynamic = dynamic;
 
-	ret = _opp_add(new_opp, dev_opp);
+	ret = _opp_add(dev, new_opp, dev_opp);
 	if (ret)
 		goto free_opp;
 
@@ -808,7 +910,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 
 	of_property_read_u32(np, "opp-microamp", (u32 *)&new_opp->u_amp);
 
-	ret = _opp_add(new_opp, dev_opp);
+	ret = _opp_add(dev, new_opp, dev_opp);
 	if (ret)
 		goto free_opp;
 
@@ -1041,6 +1143,9 @@ void of_free_opp_table(struct device *dev)
 	struct device_opp *dev_opp;
 	struct dev_pm_opp *opp, *tmp;
 
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
 	/* Check for existing list for 'dev' */
 	dev_opp = _find_device_opp(dev);
 	if (IS_ERR(dev_opp)) {
@@ -1051,18 +1156,21 @@ void of_free_opp_table(struct device *dev)
 			     IS_ERR_OR_NULL(dev) ?
 					"Invalid device" : dev_name(dev),
 			     error);
-		return;
+		goto unlock;
 	}
 
-	/* Hold our list modification lock here */
-	mutex_lock(&dev_opp_list_lock);
-
-	/* Free static OPPs */
-	list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) {
-		if (!opp->dynamic)
-			_opp_remove(dev_opp, opp, true);
+	/* Find if dev_opp manages a single device */
+	if (list_is_singular(&dev_opp->dev_list)) {
+		/* Free static OPPs */
+		list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) {
+			if (!opp->dynamic)
+				_opp_remove(dev_opp, opp, true);
+		}
+	} else {
+		_remove_list_dev(_find_list_dev(dev, dev_opp), dev_opp);
 	}
 
+unlock:
 	mutex_unlock(&dev_opp_list_lock);
 }
 EXPORT_SYMBOL_GPL(of_free_opp_table);
@@ -1088,6 +1196,7 @@ static int _of_init_opp_table_v2(struct device *dev,
 				 const struct property *prop)
 {
 	struct device_node *opp_np, *np;
+	struct device_opp *dev_opp;
 	int ret = 0, count = 0;
 
 	if (!prop->value)
@@ -1098,6 +1207,14 @@ static int _of_init_opp_table_v2(struct device *dev,
 	if (IS_ERR(opp_np))
 		return PTR_ERR(opp_np);
 
+	dev_opp = _managed_opp(opp_np);
+	if (dev_opp) {
+		/* OPPs are already managed */
+		if (!_add_list_dev(dev, dev_opp))
+			ret = -ENOMEM;
+		goto put_opp_np;
+	}
+
 	/* We have opp-list node now, iterate over it and add OPPs */
 	for_each_available_child_of_node(opp_np, np) {
 		count++;
@@ -1114,8 +1231,19 @@ static int _of_init_opp_table_v2(struct device *dev,
 	if (WARN_ON(!count))
 		goto put_opp_np;
 
-	if (ret)
+	if (!ret) {
+		if (!dev_opp) {
+			dev_opp = _find_device_opp(dev);
+			if (WARN_ON(!dev_opp))
+				goto put_opp_np;
+		}
+
+		dev_opp->np = opp_np;
+		dev_opp->shared_opp = of_property_read_bool(opp_np,
+							    "opp-shared");
+	} else {
 		of_free_opp_table(dev);
+	}
 
 put_opp_np:
 	of_node_put(opp_np);
-- 
viresh

[PATCH 07/10] opp: Add OPP sharing information to OPP library

[Stephen Boyd]

On 07/17/2015 11:33 PM, Viresh Kumar wrote:
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Wed, 11 Feb 2015 16:16:28 +0800
> Subject: [PATCH] opp: Add OPP sharing information to OPP library
>
> An opp can be shared by multiple devices, for example its very common
> for CPUs to share the OPPs, i.e. when they share clock/voltage rails.
>
> This patch adds support of shared OPPs to the OPP library.
>
> Instead of a single device, dev_opp will not contain a list of devices
> that use it. It also senses if the device (we are trying to initialize
> OPPs for) shares OPPs with a device added earlier and in that case we
> update the list of devices managed by OPPs instead of duplicating OPPs
> again.
>
> The same infrastructure will be used for the old OPP bindings, with
> later patches.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

> +static struct device_opp *_managed_opp(const struct device_node *np)
> +{
> +	struct device_opp *dev_opp;
> +
> +	list_for_each_entry_rcu(dev_opp, &dev_opp_list, node)
> +		if (dev_opp->np == np) {
> +			/*
> +			 * Multiple devices can point to the same OPP table and
> +			 * so will have same node-pointer, np.
> +			 *
> +			 * But the OPPs will be considered as shared only if the
> +			 * OPP table contains a "opp-shared" property.
> +			 */
> +			if (dev_opp->shared_opp)
> +				return dev_opp;
> +			else
> +				return NULL;
> +		

The janitors will probably find this and say that it could be simplified 
to an if () and a return without the else.

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

[PATCH 07/10] opp: Add OPP sharing information to OPP library

[Viresh Kumar]

On 20-07-15, 10:46, Stephen Boyd wrote:
> The janitors will probably find this and say that it could be
> simplified to an if () and a return without the else.

Ack.

-- 
viresh

[PATCH 07/10] opp: Add OPP sharing information to OPP library

[Viresh Kumar]

On 20-07-15, 10:46, Stephen Boyd wrote:
> >+static struct device_opp *_managed_opp(const struct device_node *np)
> >+{
> >+	struct device_opp *dev_opp;
> >+
> >+	list_for_each_entry_rcu(dev_opp, &dev_opp_list, node)
> >+		if (dev_opp->np == np) {
> >+			/*
> >+			 * Multiple devices can point to the same OPP table and
> >+			 * so will have same node-pointer, np.
> >+			 *
> >+			 * But the OPPs will be considered as shared only if the
> >+			 * OPP table contains a "opp-shared" property.
> >+			 */
> >+			if (dev_opp->shared_opp)
> >+				return dev_opp;
> >+			else
> >+				return NULL;
> >+		
> 
> The janitors will probably find this and say that it could be
> simplified to an if () and a return without the else.

And I am trying to make the janitors happy with this:

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index eb920e3f115b..8c81784fe473 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -168,7 +168,7 @@ static struct device_opp *_managed_opp(const struct device_node *np)
 {
        struct device_opp *dev_opp;
 
-       list_for_each_entry_rcu(dev_opp, &dev_opp_list, node)
+       list_for_each_entry_rcu(dev_opp, &dev_opp_list, node) {
                if (dev_opp->np == np) {
                        /*
                         * Multiple devices can point to the same OPP table and
@@ -177,11 +177,9 @@ static struct device_opp *_managed_opp(const struct device_node *np)
                         * But the OPPs will be considered as shared only if the
                         * OPP table contains a "opp-shared" property.
                         */
-                       if (dev_opp->shared_opp)
-                               return dev_opp;
-                       else
-                               return NULL;
+                       return dev_opp->shared_opp ? dev_opp : NULL;
                }
+       }
 
        return NULL;
 }

-- 
viresh

[PATCH 08/10] OPP: Add support for opp-suspend

[Viresh Kumar]

With "operating-points-v2" bindings, its possible to specify the OPP to
which the device must be switched, before suspending.

This patch adds support for getting that information.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 6b554e417b1f..0022453e4b60 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -137,6 +137,7 @@ struct device_opp {
 	struct device_node *np;
 	unsigned long clock_latency_ns_max;
 	bool shared_opp;
+	struct dev_pm_opp *suspend_opp;
 };
 
 /*
@@ -1218,6 +1219,8 @@ static int _of_init_opp_table_v2(struct device *dev,
 		goto put_opp_np;
 
 	if (!ret) {
+		const phandle *handle;
+
 		if (!dev_opp) {
 			dev_opp = _find_device_opp(dev);
 			if (WARN_ON(!dev_opp))
@@ -1227,6 +1230,25 @@ static int _of_init_opp_table_v2(struct device *dev,
 		dev_opp->np = opp_np;
 		if (of_get_property(opp_np, "opp-shared", NULL))
 			dev_opp->shared_opp = true;
+
+		/* OPP to select on device suspend */
+		handle = of_get_property(opp_np, "opp-suspend", NULL);
+		if (handle) {
+			struct device_node *suspend_opp_np;
+			struct dev_pm_opp *opp;
+
+			suspend_opp_np = of_find_node_by_phandle(be32_to_cpup(handle));
+
+			list_for_each_entry_rcu(opp, &dev_opp->opp_list, node)
+				if (opp->np == suspend_opp_np) {
+					dev_opp->suspend_opp = opp;
+					break;
+				}
+
+			if (!dev_opp->suspend_opp)
+				dev_err(dev, "%s: Invalid opp-suspend\n",
+					__func__);
+		}
 	} else {
 		of_free_opp_table(dev);
 	}
-- 
2.4.0

[PATCH 08/10] OPP: Add support for opp-suspend

[Stephen Boyd]

On 06/15/2015 04:57 AM, Viresh Kumar wrote:
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 6b554e417b1f..0022453e4b60 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -1218,6 +1219,8 @@ static int _of_init_opp_table_v2(struct device *dev,
>   		goto put_opp_np;
>   
>   	if (!ret) {
> +		const phandle *handle;
> +
>   		if (!dev_opp) {
>   			dev_opp = _find_device_opp(dev);
>   			if (WARN_ON(!dev_opp))
> @@ -1227,6 +1230,25 @@ static int _of_init_opp_table_v2(struct device *dev,
>   		dev_opp->np = opp_np;
>   		if (of_get_property(opp_np, "opp-shared", NULL))
>   			dev_opp->shared_opp = true;
> +
> +		/* OPP to select on device suspend */
> +		handle = of_get_property(opp_np, "opp-suspend", NULL);
> +		if (handle) {
> +			struct device_node *suspend_opp_np;
> +			struct dev_pm_opp *opp;
> +
> +			suspend_opp_np = of_find_node_by_phandle(be32_to_cpup(handle));

Couldn't this be done with of_parse_phandle() instead? Otherwise the 
patch looks ok.

> +
> +			list_for_each_entry_rcu(opp, &dev_opp->opp_list, node)
> +				if (opp->np == suspend_opp_np) {
> +					dev_opp->suspend_opp = opp;
> +					break;
> +				}
> +
> +			if (!dev_opp->suspend_opp)
> +				dev_err(dev, "%s: Invalid opp-suspend\n",
> +					__func__);
> +		}
>   	} else {
>   		of_free_opp_table(dev);
>   	}


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

[PATCH 08/10] OPP: Add support for opp-suspend

[Viresh Kumar]

On 17-07-15, 12:22, Stephen Boyd wrote:
> On 06/15/2015 04:57 AM, Viresh Kumar wrote:
> >diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> >index 6b554e417b1f..0022453e4b60 100644
> >--- a/drivers/base/power/opp.c
> >+++ b/drivers/base/power/opp.c
> >@@ -1218,6 +1219,8 @@ static int _of_init_opp_table_v2(struct device *dev,
> >  		goto put_opp_np;
> >  	if (!ret) {
> >+		const phandle *handle;
> >+
> >  		if (!dev_opp) {
> >  			dev_opp = _find_device_opp(dev);
> >  			if (WARN_ON(!dev_opp))
> >@@ -1227,6 +1230,25 @@ static int _of_init_opp_table_v2(struct device *dev,
> >  		dev_opp->np = opp_np;
> >  		if (of_get_property(opp_np, "opp-shared", NULL))
> >  			dev_opp->shared_opp = true;
> >+
> >+		/* OPP to select on device suspend */
> >+		handle = of_get_property(opp_np, "opp-suspend", NULL);
> >+		if (handle) {
> >+			struct device_node *suspend_opp_np;
> >+			struct dev_pm_opp *opp;
> >+
> >+			suspend_opp_np = of_find_node_by_phandle(be32_to_cpup(handle));
> 
> Couldn't this be done with of_parse_phandle() instead? Otherwise the
> patch looks ok.

Yeah, it could have been. Over that, this patch requires certain
modifications as the bindings changed this property before merging.
Its now part of individual OPPs and so no phandles.

I will reiterate the patches soon with updates.

-- 
viresh

[PATCH 09/10] opp: Add helpers for initializing CPU OPPs

[Viresh Kumar]

With "operating-points-v2" its possible to tell which devices share
OPPs. We already have infrastructure to decode that information.

This patch adds following APIs:
- of_get_cpus_sharing_opps: Returns cpumask of CPUs sharing OPPs (only
  valid with v2 bindings).
- of_cpumask_init_opp_table: Initializes OPPs for all CPUs present in
  cpumask.
- of_cpumask_free_opp_table: Frees OPPs for all CPUs present in cpumask.

- set_cpus_sharing_opps: Sets which CPUs share OPPs (only valid with old
  OPP bindings, as this information isn't present in DT).

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h   |  23 +++++++
 2 files changed, 198 insertions(+)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 0022453e4b60..e24502a2692f 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -11,6 +11,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/cpu.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/err.h>
@@ -1162,6 +1163,26 @@ void of_free_opp_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(of_free_opp_table);
 
+void of_cpumask_free_opp_table(cpumask_var_t cpumask)
+{
+	struct device *cpu_dev;
+	int cpu;
+
+	WARN_ON(cpumask_empty(cpumask));
+
+	for_each_cpu(cpu, cpumask) {
+		cpu_dev = get_cpu_device(cpu);
+		if (!cpu_dev) {
+			pr_err("%s: failed to get cpu%d device\n", __func__,
+			       cpu);
+			continue;
+		}
+
+		of_free_opp_table(cpu_dev);
+	}
+}
+EXPORT_SYMBOL_GPL(of_cpumask_free_opp_table);
+
 /* Returns opp descriptor node from its phandle. Caller must do of_node_put() */
 static struct device_node *
 _of_get_opp_desc_node_from_prop(struct device *dev, const struct property *prop)
@@ -1178,6 +1199,31 @@ _of_get_opp_desc_node_from_prop(struct device *dev, const struct property *prop)
 	return opp_np;
 }
 
+/* Returns opp descriptor node for a device. Caller must do of_node_put() */
+static struct device_node *_of_get_opp_desc_node(struct device *dev)
+{
+	const struct property *prop;
+
+	prop = of_find_property(dev->of_node, "operating-points-v2", NULL);
+	if (!prop)
+		return ERR_PTR(-ENODEV);
+	if (!prop->value)
+		return ERR_PTR(-ENODATA);
+
+	/*
+	 * TODO: Support for multiple OPP tables.
+	 *
+	 * There should be only ONE phandle present in "operating-points-v2"
+	 * property.
+	 */
+	if (prop->length != sizeof(__be32)) {
+		dev_err(dev, "%s: Invalid opp desc phandle\n", __func__);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return _of_get_opp_desc_node_from_prop(dev, prop);
+}
+
 /* Initializes OPP tables based on new bindings */
 static int _of_init_opp_table_v2(struct device *dev,
 				 const struct property *prop)
@@ -1338,4 +1384,133 @@ int of_init_opp_table(struct device *dev)
 	return _of_init_opp_table_v2(dev, prop);
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
+
+int of_cpumask_init_opp_table(cpumask_var_t cpumask)
+{
+	struct device *cpu_dev;
+	int cpu, ret = 0;
+
+	WARN_ON(cpumask_empty(cpumask));
+
+	for_each_cpu(cpu, cpumask) {
+		cpu_dev = get_cpu_device(cpu);
+		if (!cpu_dev) {
+			pr_err("%s: failed to get cpu%d device\n", __func__,
+			       cpu);
+			continue;
+		}
+
+		ret = of_init_opp_table(cpu_dev);
+		if (ret) {
+			pr_err("%s: couldn't find opp table for cpu:%d, %d\n",
+			       __func__, cpu, ret);
+
+			/* Free all other OPPs */
+			of_cpumask_free_opp_table(cpumask);
+			break;
+		}
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_cpumask_init_opp_table);
+
+/* Required only for V1 bindings, as v2 can manage it from DT itself */
+int set_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
+{
+	struct device_list_opp *list_dev;
+	struct device_opp *dev_opp;
+	struct device *dev;
+	int cpu, ret = 0;
+
+	rcu_read_lock();
+
+	dev_opp = _find_device_opp(cpu_dev);
+	if (IS_ERR(dev_opp)) {
+		ret = -EINVAL;
+		goto out_rcu_read_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;
+		}
+
+		list_dev = _add_list_dev(dev, dev_opp);
+		if (!list_dev) {
+			dev_err(dev, "%s: failed to add list-dev for cpu%d device\n",
+				__func__, cpu);
+			continue;
+		}
+	}
+out_rcu_read_unlock:
+	rcu_read_unlock();
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(set_cpus_sharing_opps);
+
+/*
+ * Works only for OPP v2 bindings.
+ *
+ * cpumask should be already set to mask of cpu_dev->id.
+ * Returns -ENOENT if operating-points-v2 bindings aren't supported.
+ */
+int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
+{
+	struct device_node *np, *tmp_np;
+	struct device *tcpu_dev;
+	int cpu, ret = 0;
+
+	/* Get OPP descriptor node */
+	np = _of_get_opp_desc_node(cpu_dev);
+	if (IS_ERR(np)) {
+		dev_dbg(cpu_dev, "%s: Couldn't find opp node: %ld\n", __func__,
+			PTR_ERR(np));
+		return -ENOENT;
+	}
+
+	/* OPPs are shared ? */
+	if (!of_get_property(np, "opp-shared", NULL))
+		goto put_cpu_node;
+
+	for_each_possible_cpu(cpu) {
+		if (cpu == cpu_dev->id)
+			continue;
+
+		tcpu_dev = get_cpu_device(cpu);
+		if (!tcpu_dev) {
+			dev_err(cpu_dev, "%s: failed to get cpu%d device\n",
+				__func__, cpu);
+			ret = -ENODEV;
+			goto put_cpu_node;
+		}
+
+		/* Get OPP descriptor node */
+		tmp_np = _of_get_opp_desc_node(tcpu_dev);
+		if (IS_ERR(tmp_np)) {
+			dev_info(tcpu_dev, "%s: Couldn't find opp node: %ld\n",
+				 __func__, PTR_ERR(tmp_np));
+			ret = -EINVAL;
+			goto put_cpu_node;
+		}
+
+		/* CPUs are sharing opp node */
+		if (np == tmp_np)
+			cpumask_set_cpu(cpu, cpumask);
+
+		of_node_put(tmp_np);
+	}
+
+put_cpu_node:
+	of_node_put(np);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_get_cpus_sharing_opps);
 #endif
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 20324b579adc..bb52fae5b921 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -121,6 +121,10 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
 int of_init_opp_table(struct device *dev);
 void of_free_opp_table(struct device *dev);
+int of_cpumask_init_opp_table(cpumask_var_t cpumask);
+void of_cpumask_free_opp_table(cpumask_var_t cpumask);
+int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask);
+int set_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask);
 #else
 static inline int of_init_opp_table(struct device *dev)
 {
@@ -130,6 +134,25 @@ static inline int of_init_opp_table(struct device *dev)
 static inline void of_free_opp_table(struct device *dev)
 {
 }
+
+static inline int of_cpumask_init_opp_table(cpumask_var_t cpumask)
+{
+	return -ENOSYS;
+}
+
+static inline void of_cpumask_free_opp_table(cpumask_var_t cpumask)
+{
+}
+
+static inline int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
+{
+	return -ENOSYS;
+}
+
+static inline int set_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
+{
+	return -ENOSYS;
+}
 #endif
 
 #endif		/* __LINUX_OPP_H__ */
-- 
2.4.0

[PATCH 10/10] cpufreq-dt: Add support for operating-points-v2 bindings

[Viresh Kumar]

Support for parsing operating-points-v2 bindings is in place now, lets
modify cpufreq-dt driver to use them.

For backward compatibility we will continue to support earlier bindings.
Special handling for that is required, to make sure OPPs are initialized
for all the CPUs.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c | 58 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 11 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index bab67db54b7e..f7bb2976059a 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -184,7 +184,6 @@ static int allocate_resources(int cpu, struct device **cdev,
 
 static int cpufreq_init(struct cpufreq_policy *policy)
 {
-	struct cpufreq_dt_platform_data *pd;
 	struct cpufreq_frequency_table *freq_table;
 	struct device_node *np;
 	struct private_data *priv;
@@ -192,7 +191,8 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	struct regulator *cpu_reg;
 	struct clk *cpu_clk;
 	unsigned long min_uV = ~0, max_uV = 0;
-	unsigned int transition_latency;
+	unsigned int transition_latency = 0;
+	bool need_update = false;
 	int ret;
 
 	ret = allocate_resources(policy->cpu, &cpu_dev, &cpu_reg, &cpu_clk);
@@ -208,8 +208,47 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		goto out_put_reg_clk;
 	}
 
-	/* OPPs might be populated at runtime, don't check for error here */
-	of_init_opp_table(cpu_dev);
+	/* Get OPP-sharing information from "operating-points-v2" bindings */
+	ret = of_get_cpus_sharing_opps(cpu_dev, policy->cpus);
+	if (ret) {
+		/*
+		 * operating-points-v2 not supported, fallback to old method of
+		 * finding shared-OPPs for backward compatibility.
+		 */
+		if (ret == -ENOENT)
+			need_update = true;
+		else
+			goto out_node_put;
+	}
+
+	/*
+	 * Initialize OPP tables for all policy->cpus. They will be shared by
+	 * all CPUs which have marked their CPUs shared with OPP bindings.
+	 *
+	 * For platforms not using operating-points-v2 bindings, we do this
+	 * before updating policy->cpus. Otherwise, we will end up creating
+	 * duplicate OPPs for policy->cpus.
+	 *
+	 * OPPs might be populated at runtime, don't check for error here
+	 */
+	of_cpumask_init_opp_table(policy->cpus);
+
+	if (need_update) {
+		struct cpufreq_dt_platform_data *pd = cpufreq_get_driver_data();
+
+		if (!pd || !pd->independent_clocks)
+			cpumask_setall(policy->cpus);
+
+		/*
+		 * OPP tables are initialized only for policy->cpu, do it for
+		 * others as well.
+		 */
+		set_cpus_sharing_opps(cpu_dev, policy->cpus);
+
+		of_property_read_u32(np, "clock-latency", &transition_latency);
+	} else {
+		transition_latency = dev_pm_opp_get_max_clock_latency(cpu_dev);
+	}
 
 	/*
 	 * But we need OPP table to function so if it is not there let's
@@ -230,7 +269,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 
 	of_property_read_u32(np, "voltage-tolerance", &priv->voltage_tolerance);
 
-	if (of_property_read_u32(np, "clock-latency", &transition_latency))
+	if (!transition_latency)
 		transition_latency = CPUFREQ_ETERNAL;
 
 	if (!IS_ERR(cpu_reg)) {
@@ -293,10 +332,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 
 	policy->cpuinfo.transition_latency = transition_latency;
 
-	pd = cpufreq_get_driver_data();
-	if (!pd || !pd->independent_clocks)
-		cpumask_setall(policy->cpus);
-
 	of_node_put(np);
 
 	return 0;
@@ -306,7 +341,8 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 out_free_priv:
 	kfree(priv);
 out_free_opp:
-	of_free_opp_table(cpu_dev);
+	of_cpumask_free_opp_table(policy->cpus);
+out_node_put:
 	of_node_put(np);
 out_put_reg_clk:
 	clk_put(cpu_clk);
@@ -322,7 +358,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 
 	cpufreq_cooling_unregister(priv->cdev);
 	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
-	of_free_opp_table(priv->cpu_dev);
+	of_cpumask_free_opp_table(policy->related_cpus);
 	clk_put(policy->clk);
 	if (!IS_ERR(priv->cpu_reg))
 		regulator_put(priv->cpu_reg);
-- 
2.4.0

[PATCH 10/10] cpufreq-dt: Add support for operating-points-v2 bindings

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday, June 15, 2015 05:27:36 PM Viresh Kumar wrote:
> Support for parsing operating-points-v2 bindings is in place now, lets
> modify cpufreq-dt driver to use them.

I believe that following patches:

* [PATCH v2 1/7] opp: add dev_pm_opp_get_turbo_mode_setting() helper
  (http://lkml.org/lkml/2015/7/9/420)

* [PATCH v2 2/7] cpufreq: opp: fix handling of turbo modes
  (http://lkml.org/lkml/2015/7/9/421)

should be integrated into this patch series before patch #10 (the current
one) can be applied.

[ Please see http://lkml.org/lkml/2015/7/9/419 for more details. ]

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 10/10] cpufreq-dt: Add support for operating-points-v2 bindings

[Bartlomiej Zolnierkiewicz]

On Thursday, July 09, 2015 06:13:54 PM Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Monday, June 15, 2015 05:27:36 PM Viresh Kumar wrote:
> > Support for parsing operating-points-v2 bindings is in place now, lets
> > modify cpufreq-dt driver to use them.
> 
> I believe that following patches:
> 
> * [PATCH v2 1/7] opp: add dev_pm_opp_get_turbo_mode_setting() helper
>   (http://lkml.org/lkml/2015/7/9/420)
> 
> * [PATCH v2 2/7] cpufreq: opp: fix handling of turbo modes
>   (http://lkml.org/lkml/2015/7/9/421)
> 
> should be integrated into this patch series before patch #10 (the current
> one) can be applied.
> 
> [ Please see http://lkml.org/lkml/2015/7/9/419 for more details. ]

On the 2nd thought here is the issue description, so people don't
have to search for it:

With the current code the turbo-mode opp-s are not distinguished in
any way in the cpufreq subsystem and once somebody defines them in
their DTS file they are treated as normal modes. I.e. if you define
opp-s using opp-v2 bindings in your DTS file (for use by cpufreq-dt
driver) and some are marked as turbo modes then the freq_table (build
by cpufreq core) will contain turbo mode frequencies and cpufreq-dt
driver will use them as normal frequencies.  This is certainly not
a desired behavior.

To fix it I added opp core helper to check whether opp is a turbo
mode and during build of freq_table turbo mode frequencies are
marked with special flag (CPUFREQ_BOOST_FREQ).  Such frequencies
are ignored by cpufreq core unless 'boost' mode is enabled.
'boost' mode support needs to be explicitly supported & enabled in
a specific cpufreq driver.  For the moment it is only available for
Exynos4x12 cpufreq driver but my patchset (converting Exynos4x12
platforms to use cpufreq-dt driver) makes it available for use in
cpufreq-dt (to be enabled from your platform support when needed).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 10/10] cpufreq-dt: Add support for operating-points-v2 bindings

[Viresh Kumar]

On 09-07-15, 18:13, Bartlomiej Zolnierkiewicz wrote:
> I believe that following patches:
> 
> * [PATCH v2 1/7] opp: add dev_pm_opp_get_turbo_mode_setting() helper
>   (http://lkml.org/lkml/2015/7/9/420)
> 
> * [PATCH v2 2/7] cpufreq: opp: fix handling of turbo modes
>   (http://lkml.org/lkml/2015/7/9/421)
> 
> should be integrated into this patch series before patch #10 (the current
> one) can be applied.

Hi Bartlomiej,

I haven't tried for the complete support yet, I will take care of
boost frequencies as well, but not in this series.

-- 
viresh

[PATCH 00/10] OPP: Add code to support operating-points-v2 bindings

[Viresh Kumar]

On 15 June 2015 at 17:27, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Hi Guys,
>
> This adds code to support operating-points-v2 bindings. Not everything
> is supported yet, but most of the basic stuff is.
>
> Tested with: Exynos 5250, dual cortex A15 board. With both old and new
> bindings.
>
> Bindings are already frozen:
> http://marc.info/?i=cover.1433434659.git.viresh.kumar%40linaro.org
>
> Pushed here as well for reference:
> ssh://git at git.linaro.org/people/viresh.kumar/linux.git opp/v2

I am really hoping to get all basic stuff pushed in for 4.3, can we have
some review of the code please?

--
viresh

[PATCH 00/10] OPP: Add code to support operating-points-v2 bindings

[Viresh Kumar]

On 30-06-15, 22:14, Viresh Kumar wrote:
> On 15 June 2015 at 17:27, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > Hi Guys,
> >
> > This adds code to support operating-points-v2 bindings. Not everything
> > is supported yet, but most of the basic stuff is.
> >
> > Tested with: Exynos 5250, dual cortex A15 board. With both old and new
> > bindings.
> >
> > Bindings are already frozen:
> > http://marc.info/?i=cover.1433434659.git.viresh.kumar%40linaro.org
> >
> > Pushed here as well for reference:
> > ssh://git at git.linaro.org/people/viresh.kumar/linux.git opp/v2
> 
> I am really hoping to get all basic stuff pushed in for 4.3, can we have
> some review of the code please?

@Stephen: Can you please review rest of the stuff?

@Nishanth: Can you please review the series, now that you are back :)

@Rafael: Can you please apply the patches Acked by Stephen for 4.3, so
that I don't have to resend them again ?

-- 
viresh