[PATCH 1/3] PM / OPP: Add missing doc comments

[PATCH 1/3] PM / OPP: Add missing doc comments

[Viresh Kumar]

Few doc-style comments were missing, add them. Rearrange another one to
match the sequence within the structure.

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

diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index a6bd8d2c2b47..b8880c7f8be1 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -51,8 +51,8 @@ extern struct mutex dev_opp_list_lock;
  *		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
  * @suspend:	true if suspend OPP
  * @rate:	Frequency in hertz
@@ -126,7 +126,9 @@ struct device_list_opp {
  * @dev_list:	list of devices that share these OPPs
  * @opp_list:	list of opps
  * @np:		struct device_node pointer for opp's DT node.
+ * @clock_latency_ns_max: Max clock latency in nanoseconds.
  * @shared_opp: OPP is shared between multiple devices.
+ * @suspend_opp: Pointer to OPP to be used during device suspend.
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
-- 
2.6.2.198.g614a2ac

Re: [PATCH 1/3] PM / OPP: Add missing doc comments

[Pavel Machek]

On Mon 2015-11-16 16:07:26, Viresh Kumar wrote:
> Few doc-style comments were missing, add them. Rearrange another one to
> match the sequence within the structure.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/3] PM / OPP: Add missing doc comments

[Stephen Boyd]

On 11/16, Viresh Kumar wrote:
> Few doc-style comments were missing, add them. Rearrange another one to
> match the sequence within the structure.
> 
> 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 2/3] PM / OPP: Parse 'opp-supported-hw' binding

[Viresh Kumar]

OPP bindings allow a platform to enable OPPs based on the version of the
hardware they are used for.

Add support to the OPP-core to parse these bindings, by introducing
dev_pm_opp_{set|put}_supported_hw() APIs.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 153 ++++++++++++++++++++++++++++++++++++++++++
 drivers/base/power/opp/opp.h  |   5 ++
 include/linux/pm_opp.h        |  12 ++++
 3 files changed, 170 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 6aa172be6e8e..29fe251bf9ec 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -559,6 +559,9 @@ static void _remove_device_opp(struct device_opp *dev_opp)
 	if (!list_empty(&dev_opp->opp_list))
 		return;
 
+	if (dev_opp->supported_hw)
+		return;
+
 	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
 				    node);
 
@@ -834,6 +837,150 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev)
 }
 
 /**
+ * dev_pm_opp_set_supported_hw() - Set supported platforms
+ * @dev: Device for which the regulator has to be set.
+ * @versions: Array of hierarchy of versions to match.
+ * @count: Number of elements in the array.
+ *
+ * This is required only for the V2 bindings, and it enables a platform to
+ * specify the hierarchy of versions it supports. OPP layer will then enable
+ * OPPs, which are available for those versions, based on its 'opp-supported-hw'
+ * property.
+ */
+int dev_pm_opp_set_supported_hw(struct device *dev, u32 *versions,
+				unsigned int count)
+{
+	struct device_opp *dev_opp;
+	int ret = 0;
+
+	if (!dev || !versions || !count) {
+		pr_err("%s: Invalid arguments, dev:0x%p, ver:0x%p, count:%u\n",
+		       __func__, dev, versions, count);
+		return -EINVAL;
+	}
+
+	/* Operations on OPP structures must be done from within rcu locks */
+	rcu_read_lock();
+
+	dev_opp = _add_device_opp(dev);
+	if (!dev_opp)
+		return -ENOMEM;
+
+	/* Do we already have a version hierarchy associated with dev_opp? */
+	if (dev_opp->supported_hw) {
+		dev_err(dev, "%s: Already have supported hardware list\n",
+			__func__);
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	dev_opp->supported_hw = kmemdup(versions, count * sizeof(*versions),
+					GFP_KERNEL);
+	if (!dev_opp->supported_hw) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	dev_opp->supported_hw_count = count;
+
+unlock:
+	rcu_read_unlock();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_supported_hw);
+
+/**
+ * dev_pm_opp_put_supported_hw() - Releases resources blocked for supported hw
+ * @dev: Device for which the regulator has to be set.
+ *
+ * This is required only for the V2 bindings, and is called for a matching
+ * dev_pm_opp_set_supported_hw(). Until this is called, the device_opp structure
+ * will not be freed.
+ */
+void dev_pm_opp_put_supported_hw(struct device *dev)
+{
+	struct device_opp *dev_opp;
+
+	if (!dev) {
+		pr_err("%s: Invalid argument dev:0x%p\n", __func__, dev);
+		return;
+	}
+
+	/* Operations on OPP structures must be done from within rcu locks */
+	rcu_read_lock();
+
+	/* Check for existing list for 'dev' first */
+	dev_opp = _find_device_opp(dev);
+	if (IS_ERR(dev_opp)) {
+		dev_err(dev, "Failed to find dev_opp: %ld\n", PTR_ERR(dev_opp));
+		goto unlock;
+	}
+
+	if (!dev_opp->supported_hw) {
+		dev_err(dev, "%s: Doesn't have supported hardware list\n",
+			__func__);
+		goto unlock;
+	}
+
+	kfree(dev_opp->supported_hw);
+	dev_opp->supported_hw = NULL;
+	dev_opp->supported_hw_count = 0;
+
+	/* Try freeing device_opp if this was the last blocking resource */
+	_remove_device_opp(dev_opp);
+
+unlock:
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_put_supported_hw);
+
+static bool _opp_is_supported(struct device *dev, struct device_opp *dev_opp,
+			      struct device_node *np)
+{
+	unsigned int count;
+	u32 *versions;
+	bool supported = true;
+	int ret;
+
+	if (!dev_opp->supported_hw)
+		return true;
+
+	count = of_property_count_u32_elems(np, "opp-supported-hw");
+	if (count != dev_opp->supported_hw_count) {
+		dev_warn(dev, "%s: supported-hw count mismatch, plat:%u != DT:%u\n",
+			 __func__, dev_opp->supported_hw_count, count);
+		return false;
+	}
+
+	versions = kcalloc(count, sizeof(*versions), GFP_KERNEL);
+	if (!versions)
+		return false;
+
+	ret = of_property_read_u32_array(np, "opp-supported-hw", versions,
+					 count);
+	if (ret) {
+		dev_warn(dev, "%s: failed to read opp-supported-hw property: %d\n",
+			 __func__, ret);
+		supported = false;
+		goto free_versions;
+	}
+
+	while (count--) {
+		/* Both of these are bitwise masks of the versions */
+		if (!(versions[count] & dev_opp->supported_hw[count])) {
+			supported = false;
+			break;
+		}
+	}
+
+free_versions:
+	kfree(versions);
+
+	return supported;
+}
+
+/**
  * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
  * @dev:	device for which we do this operation
  * @np:		device node
@@ -879,6 +1026,12 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 		goto free_opp;
 	}
 
+	/* Check if the OPP supports hardware's hierarchy of versions or not */
+	if (!_opp_is_supported(dev, dev_opp, np)) {
+		dev_dbg(dev, "OPP not supported by hardware: %llu\n", rate);
+		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
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index b8880c7f8be1..70f4564a6ab9 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -129,6 +129,8 @@ struct device_list_opp {
  * @clock_latency_ns_max: Max clock latency in nanoseconds.
  * @shared_opp: OPP is shared between multiple devices.
  * @suspend_opp: Pointer to OPP to be used during device suspend.
+ * @supported_hw: Array of version number to support.
+ * @supported_hw_count: Number of elements in supported_hw array.
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
@@ -153,6 +155,9 @@ struct device_opp {
 	bool shared_opp;
 	struct dev_pm_opp *suspend_opp;
 
+	unsigned int *supported_hw;
+	unsigned int supported_hw_count;
+
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
 	char dentry_name[NAME_MAX];
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 9a2e50337af9..d12471ed14a2 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -55,6 +55,9 @@ int dev_pm_opp_enable(struct device *dev, unsigned long freq);
 int dev_pm_opp_disable(struct device *dev, unsigned long freq);
 
 struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
+int dev_pm_opp_set_supported_hw(struct device *dev, u32 *versions,
+				unsigned int count);
+void dev_pm_opp_put_supported_hw(struct device *dev);
 #else
 static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 {
@@ -129,6 +132,15 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
 {
 	return ERR_PTR(-EINVAL);
 }
+
+static inline int dev_pm_opp_set_supported_hw(struct device *dev, u32 *versions,
+					      unsigned int count)
+{
+	return -EINVAL;
+}
+
+static inline void dev_pm_opp_put_supported_hw(struct device *dev) {}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
-- 
2.6.2.198.g614a2ac

Re: [PATCH 2/3] PM / OPP: Parse 'opp-supported-hw' binding

[Stephen Boyd]

On 11/16, Viresh Kumar wrote:
> @@ -834,6 +837,150 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev)
>  }
>  
>  /**
> + * dev_pm_opp_set_supported_hw() - Set supported platforms
> + * @dev: Device for which the regulator has to be set.
> + * @versions: Array of hierarchy of versions to match.
> + * @count: Number of elements in the array.
> + *
> + * This is required only for the V2 bindings, and it enables a platform to
> + * specify the hierarchy of versions it supports. OPP layer will then enable
> + * OPPs, which are available for those versions, based on its 'opp-supported-hw'
> + * property.
> + */
> +int dev_pm_opp_set_supported_hw(struct device *dev, u32 *versions,

versions could be const.

> +				unsigned int count)
> +{
> +	struct device_opp *dev_opp;
> +	int ret = 0;
> +
> +	if (!dev || !versions || !count) {
> +		pr_err("%s: Invalid arguments, dev:0x%p, ver:0x%p, count:%u\n",
> +		       __func__, dev, versions, count);

Weird 0x(null) prints may be here. Do we really need this check
at all though? Users should know what they're doing.

> +		return -EINVAL;
> +	}
> +
> +	/* Operations on OPP structures must be done from within rcu locks */
> +	rcu_read_lock();
> +
> +	dev_opp = _add_device_opp(dev);
> +	if (!dev_opp)
> +		return -ENOMEM;
> +
> +	/* Do we already have a version hierarchy associated with dev_opp? */
> +	if (dev_opp->supported_hw) {
> +		dev_err(dev, "%s: Already have supported hardware list\n",
> +			__func__);
> +		ret = -EINVAL;

Maybe -EBUSY is more appropriate?

> +		goto unlock;
> +	}
> +
> +	dev_opp->supported_hw = kmemdup(versions, count * sizeof(*versions),
> +					GFP_KERNEL);
> +	if (!dev_opp->supported_hw) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	dev_opp->supported_hw_count = count;
> +
> +unlock:
> +	rcu_read_unlock();
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_set_supported_hw);
> +
> +/**
> + * dev_pm_opp_put_supported_hw() - Releases resources blocked for supported hw
> + * @dev: Device for which the regulator has to be set.

regulator or OPP?

> + *
> + * This is required only for the V2 bindings, and is called for a matching
> + * dev_pm_opp_set_supported_hw(). Until this is called, the device_opp structure
> + * will not be freed.
> + */
> +void dev_pm_opp_put_supported_hw(struct device *dev)
> +{
> +	struct device_opp *dev_opp;
> +
> +	if (!dev) {
> +		pr_err("%s: Invalid argument dev:0x%p\n", __func__, dev);

dev is NULL.. so this prints :0x(null) all the time? And really,
who is calling this with a NULL dev? 

> +		return;
> +	}
> +
> +	/* Operations on OPP structures must be done from within rcu locks */
> +	rcu_read_lock();
> +
> +	/* Check for existing list for 'dev' first */
> +	dev_opp = _find_device_opp(dev);

Plus this checks for a NULL dev so we really don't need that
check for a NULL dev at the top at all.

> +	if (IS_ERR(dev_opp)) {
> +		dev_err(dev, "Failed to find dev_opp: %ld\n", PTR_ERR(dev_opp));
> +		goto unlock;
> +	}
> +
[...]
> +
> +static bool _opp_is_supported(struct device *dev, struct device_opp *dev_opp,
> +			      struct device_node *np)
> +{
> +	unsigned int count;
> +	u32 *versions;
> +	bool supported = true;
> +	int ret;
> +
> +	if (!dev_opp->supported_hw)
> +		return true;
> +
> +	count = of_property_count_u32_elems(np, "opp-supported-hw");
> +	if (count != dev_opp->supported_hw_count) {
> +		dev_warn(dev, "%s: supported-hw count mismatch, plat:%u != DT:%u\n",
> +			 __func__, dev_opp->supported_hw_count, count);
> +		return false;
> +	}
> +
> +	versions = kcalloc(count, sizeof(*versions), GFP_KERNEL);
> +	if (!versions)
> +		return false;
> +
> +	ret = of_property_read_u32_array(np, "opp-supported-hw", versions,
> +					 count);
> +	if (ret) {
> +		dev_warn(dev, "%s: failed to read opp-supported-hw property: %d\n",
> +			 __func__, ret);
> +		supported = false;
> +		goto free_versions;
> +	}
> +
> +	while (count--) {
> +		/* Both of these are bitwise masks of the versions */
> +		if (!(versions[count] & dev_opp->supported_hw[count])) {
> +			supported = false;
> +			break;
> +		}
> +	}
> +
> +free_versions:
> +	kfree(versions);

Why do we need to allocate an array to check the property a u32
at a time? We should be able to call of_property_read_u32_index()
in a loop and check that against the version array. No allocation
needed.

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

Re: [PATCH 2/3] PM / OPP: Parse 'opp-supported-hw' binding

[Viresh Kumar]

On 18-11-15, 14:53, Stephen Boyd wrote:
> Why do we need to allocate an array to check the property a u32
> at a time? We should be able to call of_property_read_u32_index()
> in a loop and check that against the version array. No allocation
> needed.

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

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 15 Sep 2015 12:53:00 +0530
Subject: [PATCH] PM / OPP: Parse 'opp-supported-hw' binding

OPP bindings allow a platform to enable OPPs based on the version of the
hardware they are used for.

Add support to the OPP-core to parse these bindings, by introducing
dev_pm_opp_{set|put}_supported_hw() APIs.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 124 ++++++++++++++++++++++++++++++++++++++++++
 drivers/base/power/opp/opp.h  |   5 ++
 include/linux/pm_opp.h        |  13 +++++
 3 files changed, 142 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 6aa172be6e8e..93f7b8c87a26 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -559,6 +559,9 @@ static void _remove_device_opp(struct device_opp *dev_opp)
 	if (!list_empty(&dev_opp->opp_list))
 		return;
 
+	if (dev_opp->supported_hw)
+		return;
+
 	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
 				    node);
 
@@ -834,6 +837,121 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev)
 }
 
 /**
+ * dev_pm_opp_set_supported_hw() - Set supported platforms
+ * @dev: Device for which supported-hw has to be set.
+ * @versions: Array of hierarchy of versions to match.
+ * @count: Number of elements in the array.
+ *
+ * This is required only for the V2 bindings, and it enables a platform to
+ * specify the hierarchy of versions it supports. OPP layer will then enable
+ * OPPs, which are available for those versions, based on its 'opp-supported-hw'
+ * property.
+ */
+int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
+				unsigned int count)
+{
+	struct device_opp *dev_opp;
+	int ret = 0;
+
+	/* Operations on OPP structures must be done from within rcu locks */
+	rcu_read_lock();
+
+	dev_opp = _add_device_opp(dev);
+	if (!dev_opp)
+		return -ENOMEM;
+
+	/* Do we already have a version hierarchy associated with dev_opp? */
+	if (dev_opp->supported_hw) {
+		dev_err(dev, "%s: Already have supported hardware list\n",
+			__func__);
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	dev_opp->supported_hw = kmemdup(versions, count * sizeof(*versions),
+					GFP_KERNEL);
+	if (!dev_opp->supported_hw) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	dev_opp->supported_hw_count = count;
+
+unlock:
+	rcu_read_unlock();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_supported_hw);
+
+/**
+ * dev_pm_opp_put_supported_hw() - Releases resources blocked for supported hw
+ * @dev: Device for which supported-hw has to be set.
+ *
+ * This is required only for the V2 bindings, and is called for a matching
+ * dev_pm_opp_set_supported_hw(). Until this is called, the device_opp structure
+ * will not be freed.
+ */
+void dev_pm_opp_put_supported_hw(struct device *dev)
+{
+	struct device_opp *dev_opp;
+
+	/* Operations on OPP structures must be done from within rcu locks */
+	rcu_read_lock();
+
+	/* Check for existing list for 'dev' first */
+	dev_opp = _find_device_opp(dev);
+	if (IS_ERR(dev_opp)) {
+		dev_err(dev, "Failed to find dev_opp: %ld\n", PTR_ERR(dev_opp));
+		goto unlock;
+	}
+
+	if (!dev_opp->supported_hw) {
+		dev_err(dev, "%s: Doesn't have supported hardware list\n",
+			__func__);
+		goto unlock;
+	}
+
+	kfree(dev_opp->supported_hw);
+	dev_opp->supported_hw = NULL;
+	dev_opp->supported_hw_count = 0;
+
+	/* Try freeing device_opp if this was the last blocking resource */
+	_remove_device_opp(dev_opp);
+
+unlock:
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_put_supported_hw);
+
+static bool _opp_is_supported(struct device *dev, struct device_opp *dev_opp,
+			      struct device_node *np)
+{
+	unsigned int count = dev_opp->supported_hw_count;
+	u32 version;
+	int ret;
+
+	if (!dev_opp->supported_hw)
+		return true;
+
+	while (count--) {
+		ret = of_property_read_u32_index(np, "opp-supported-hw", count,
+						 &version);
+		if (ret) {
+			dev_warn(dev, "%s: failed to read opp-supported-hw property at index %d: %d\n",
+					__func__, count, ret);
+			return false;
+		}
+
+		/* Both of these are bitwise masks of the versions */
+		if (!(version & dev_opp->supported_hw[count]))
+			return false;
+	}
+
+	return true;
+}
+
+/**
  * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
  * @dev:	device for which we do this operation
  * @np:		device node
@@ -879,6 +997,12 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 		goto free_opp;
 	}
 
+	/* Check if the OPP supports hardware's hierarchy of versions or not */
+	if (!_opp_is_supported(dev, dev_opp, np)) {
+		dev_dbg(dev, "OPP not supported by hardware: %llu\n", rate);
+		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
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index b8880c7f8be1..70f4564a6ab9 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -129,6 +129,8 @@ struct device_list_opp {
  * @clock_latency_ns_max: Max clock latency in nanoseconds.
  * @shared_opp: OPP is shared between multiple devices.
  * @suspend_opp: Pointer to OPP to be used during device suspend.
+ * @supported_hw: Array of version number to support.
+ * @supported_hw_count: Number of elements in supported_hw array.
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
@@ -153,6 +155,9 @@ struct device_opp {
 	bool shared_opp;
 	struct dev_pm_opp *suspend_opp;
 
+	unsigned int *supported_hw;
+	unsigned int supported_hw_count;
+
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
 	char dentry_name[NAME_MAX];
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 9a2e50337af9..3a85110242f0 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -55,6 +55,9 @@ int dev_pm_opp_enable(struct device *dev, unsigned long freq);
 int dev_pm_opp_disable(struct device *dev, unsigned long freq);
 
 struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
+int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
+				unsigned int count);
+void dev_pm_opp_put_supported_hw(struct device *dev);
 #else
 static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 {
@@ -129,6 +132,16 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
 {
 	return ERR_PTR(-EINVAL);
 }
+
+static inline int dev_pm_opp_set_supported_hw(struct device *dev,
+					      const u32 *versions,
+					      unsigned int count)
+{
+	return -EINVAL;
+}
+
+static inline void dev_pm_opp_put_supported_hw(struct device *dev) {}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)

-- 
viresh

Re: [PATCH 2/3] PM / OPP: Parse 'opp-supported-hw' binding

[Viresh Kumar]

On 19-11-15, 07:30, Viresh Kumar wrote:
> -------------------------8<-------------------------
> 
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Tue, 15 Sep 2015 12:53:00 +0530
> Subject: [PATCH] PM / OPP: Parse 'opp-supported-hw' binding
> 
> OPP bindings allow a platform to enable OPPs based on the version of the
> hardware they are used for.
> 
> Add support to the OPP-core to parse these bindings, by introducing
> dev_pm_opp_{set|put}_supported_hw() APIs.

Please ignore this, more corrections required based on 3/3. Will
resend it shortly.

-- 
viresh

[PATCH 3/3] PM / OPP: Parse 'opp-<prop>-<name>' bindings

[Viresh Kumar]

OPP bindings (for few properties) allow a platform to choose a
value/range among a set of available options. The options are present as
opp-<prop>-<name>, where the platform needs to supply the <name> string.

The OPP properties which allow such an option are: opp-microvolt and
opp-microamp.

Add support to the OPP-core to parse these bindings, by introducing
dev_pm_opp_{set|put}_prop_name() APIs.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 148 ++++++++++++++++++++++++++++++++++++++----
 drivers/base/power/opp/opp.h  |   2 +
 include/linux/pm_opp.h        |   9 +++
 3 files changed, 146 insertions(+), 13 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 29fe251bf9ec..b3750aa9552f 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -105,6 +105,15 @@ struct device_opp *_find_device_opp(struct device *dev)
 	return ERR_PTR(-ENODEV);
 }
 
+static void _opp_create_prop_name(char *name, const char *prefix,
+				  const char *postfix)
+{
+	if (postfix)
+		sprintf(name, "%s-%s", prefix, postfix);
+	else
+		sprintf(name, "%s", prefix);
+}
+
 /**
  * dev_pm_opp_get_voltage() - Gets the voltage corresponding to an opp
  * @opp:	opp for which voltage has to be returned for
@@ -562,6 +571,9 @@ static void _remove_device_opp(struct device_opp *dev_opp)
 	if (dev_opp->supported_hw)
 		return;
 
+	if (dev_opp->prop_name)
+		return;
+
 	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
 				    node);
 
@@ -794,20 +806,32 @@ static int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
 }
 
 /* TODO: Support multiple regulators */
-static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev)
+static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
+			      struct device_opp *dev_opp)
 {
 	u32 microvolt[3] = {0};
 	u32 val;
 	int count, ret;
+	struct property *prop;
+	char name[NAME_MAX];
 
-	/* Missing property isn't a problem, but an invalid entry is */
-	if (!of_find_property(opp->np, "opp-microvolt", NULL))
-		return 0;
+	/* Search for both "opp-microvolt-<name>" and "opp-microvolt" */
+	_opp_create_prop_name(name, "opp-microvolt", dev_opp->prop_name);
 
-	count = of_property_count_u32_elems(opp->np, "opp-microvolt");
+	prop = of_find_property(opp->np, name, NULL);
+	if (!prop) {
+		_opp_create_prop_name(name, "opp-microvolt", NULL);
+		prop = of_find_property(opp->np, name, NULL);
+
+		/* Missing property isn't a problem, but an invalid entry is */
+		if (!prop)
+			return 0;
+	}
+
+	count = of_property_count_u32_elems(opp->np, name);
 	if (count < 0) {
-		dev_err(dev, "%s: Invalid opp-microvolt property (%d)\n",
-			__func__, count);
+		dev_err(dev, "%s: Invalid %s property (%d)\n",
+			__func__, name, count);
 		return count;
 	}
 
@@ -818,11 +842,9 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev)
 		return -EINVAL;
 	}
 
-	ret = of_property_read_u32_array(opp->np, "opp-microvolt", microvolt,
-					 count);
+	ret = of_property_read_u32_array(opp->np, name, microvolt, count);
 	if (ret) {
-		dev_err(dev, "%s: error parsing opp-microvolt: %d\n", __func__,
-			ret);
+		dev_err(dev, "%s: error parsing %s: %d\n", __func__, name, ret);
 		return -EINVAL;
 	}
 
@@ -830,7 +852,15 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev)
 	opp->u_volt_min = microvolt[1];
 	opp->u_volt_max = microvolt[2];
 
-	if (!of_property_read_u32(opp->np, "opp-microamp", &val))
+	/* Search for both "opp-microamp-<name>" and "opp-microamp" */
+	_opp_create_prop_name(name, "opp-microamp", dev_opp->prop_name);
+	prop = of_find_property(opp->np, name, NULL);
+	if (!prop) {
+		_opp_create_prop_name(name, "opp-microamp", NULL);
+		prop = of_find_property(opp->np, name, NULL);
+	}
+
+	if (prop && !of_property_read_u32(opp->np, "opp-microamp", &val))
 		opp->u_amp = val;
 
 	return 0;
@@ -935,6 +965,98 @@ void dev_pm_opp_put_supported_hw(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_put_supported_hw);
 
+/**
+ * dev_pm_opp_set_prop_name() - Set prop-extn name
+ * @dev: Device for which the regulator has to be set.
+ * @name: name to postfix to properties.
+ *
+ * This is required only for the V2 bindings, and it enables a platform to
+ * specify the extn to be used for certain property names. The properties to
+ * which the extension will apply are opp-microvolt and opp-microamp. OPP core
+ * should postfix the property name with -<name> while looking for them.
+ */
+int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
+{
+	struct device_opp *dev_opp;
+	int ret = 0;
+
+	if (!dev || !name) {
+		pr_err("%s: Invalid arguments, dev:0x%p, name:%p\n", __func__,
+		       dev, name);
+		return -EINVAL;
+	}
+
+	/* Operations on OPP structures must be done from within rcu locks */
+	rcu_read_lock();
+
+	dev_opp = _add_device_opp(dev);
+	if (!dev_opp)
+		return -ENOMEM;
+
+	/* Do we already have a prop-name associated with dev_opp? */
+	if (dev_opp->prop_name) {
+		dev_err(dev, "%s: Already have prop-name %s\n", __func__,
+			dev_opp->prop_name);
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	dev_opp->prop_name = kstrdup(name, GFP_KERNEL);
+	if (!dev_opp->prop_name) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+unlock:
+	rcu_read_unlock();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_prop_name);
+
+/**
+ * dev_pm_opp_put_prop_name() - Releases resources blocked for prop-name
+ * @dev: Device for which the regulator has to be set.
+ *
+ * This is required only for the V2 bindings, and is called for a matching
+ * dev_pm_opp_set_prop_name(). Until this is called, the device_opp structure
+ * will not be freed.
+ */
+void dev_pm_opp_put_prop_name(struct device *dev)
+{
+	struct device_opp *dev_opp;
+
+	if (!dev) {
+		pr_err("%s: Invalid argument dev:0x%p\n", __func__, dev);
+		return;
+	}
+
+	/* Operations on OPP structures must be done from within rcu locks */
+	rcu_read_lock();
+
+	/* Check for existing list for 'dev' first */
+	dev_opp = _find_device_opp(dev);
+	if (IS_ERR(dev_opp)) {
+		dev_err(dev, "Failed to find dev_opp: %ld\n", PTR_ERR(dev_opp));
+		goto unlock;
+	}
+
+	if (!dev_opp->prop_name) {
+		dev_err(dev, "%s: Doesn't have a prop-name\n", __func__);
+		goto unlock;
+	}
+
+	kfree(dev_opp->prop_name);
+	dev_opp->prop_name = NULL;
+
+	/* Try freeing device_opp if this was the last blocking resource */
+	_remove_device_opp(dev_opp);
+
+unlock:
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
+
 static bool _opp_is_supported(struct device *dev, struct device_opp *dev_opp,
 			      struct device_node *np)
 {
@@ -1047,7 +1169,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 	if (!of_property_read_u32(np, "clock-latency-ns", &val))
 		new_opp->clock_latency_ns = val;
 
-	ret = opp_parse_supplies(new_opp, dev);
+	ret = opp_parse_supplies(new_opp, dev, dev_opp);
 	if (ret)
 		goto free_opp;
 
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 70f4564a6ab9..690638ef36ee 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -131,6 +131,7 @@ struct device_list_opp {
  * @suspend_opp: Pointer to OPP to be used during device suspend.
  * @supported_hw: Array of version number to support.
  * @supported_hw_count: Number of elements in supported_hw array.
+ * @prop_name: A name to postfix to many DT properties, while parsing them.
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
@@ -157,6 +158,7 @@ struct device_opp {
 
 	unsigned int *supported_hw;
 	unsigned int supported_hw_count;
+	const char *prop_name;
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index d12471ed14a2..eec973130951 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -58,6 +58,8 @@ struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
 int dev_pm_opp_set_supported_hw(struct device *dev, u32 *versions,
 				unsigned int count);
 void dev_pm_opp_put_supported_hw(struct device *dev);
+int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
+void dev_pm_opp_put_prop_name(struct device *dev);
 #else
 static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 {
@@ -141,6 +143,13 @@ static inline int dev_pm_opp_set_supported_hw(struct device *dev, u32 *versions,
 
 static inline void dev_pm_opp_put_supported_hw(struct device *dev) {}
 
+static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
+{
+	return -EINVAL;
+}
+
+static inline void dev_pm_opp_put_prop_name(struct device *dev) {}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
-- 
2.6.2.198.g614a2ac

Re: [PATCH 3/3] PM / OPP: Parse 'opp-<prop>-<name>' bindings

[Stephen Boyd]

On 11/16, Viresh Kumar wrote:
> @@ -794,20 +806,32 @@ static int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
>  }
>  
>  /* TODO: Support multiple regulators */
> -static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev)
> +static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
> +			      struct device_opp *dev_opp)
>  {
>  	u32 microvolt[3] = {0};
>  	u32 val;
>  	int count, ret;
> +	struct property *prop;
> +	char name[NAME_MAX];
>  
> -	/* Missing property isn't a problem, but an invalid entry is */
> -	if (!of_find_property(opp->np, "opp-microvolt", NULL))
> -		return 0;
> +	/* Search for both "opp-microvolt-<name>" and "opp-microvolt" */
> +	_opp_create_prop_name(name, "opp-microvolt", dev_opp->prop_name);
>  
> -	count = of_property_count_u32_elems(opp->np, "opp-microvolt");
> +	prop = of_find_property(opp->np, name, NULL);
> +	if (!prop) {
> +		_opp_create_prop_name(name, "opp-microvolt", NULL);

Why not just NUL terminate the string after the t in microvolt
that's already there?

> +		prop = of_find_property(opp->np, name, NULL);
> +
> +		/* Missing property isn't a problem, but an invalid entry is */
> +		if (!prop)
> +			return 0;
> +	}
> +
> +	count = of_property_count_u32_elems(opp->np, name);
>  	if (count < 0) {
> -		dev_err(dev, "%s: Invalid opp-microvolt property (%d)\n",
> -			__func__, count);
> +		dev_err(dev, "%s: Invalid %s property (%d)\n",
> +			__func__, name, count);
>  		return count;
>  	}
>  
> @@ -830,7 +852,15 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev)
>  	opp->u_volt_min = microvolt[1];
>  	opp->u_volt_max = microvolt[2];
>  
> -	if (!of_property_read_u32(opp->np, "opp-microamp", &val))
> +	/* Search for both "opp-microamp-<name>" and "opp-microamp" */
> +	_opp_create_prop_name(name, "opp-microamp", dev_opp->prop_name);
> +	prop = of_find_property(opp->np, name, NULL);
> +	if (!prop) {
> +		_opp_create_prop_name(name, "opp-microamp", NULL);

Same comment here.

> +		prop = of_find_property(opp->np, name, NULL);
> +	}
> +
> +	if (prop && !of_property_read_u32(opp->np, "opp-microamp", &val))
>  		opp->u_amp = val;
>  
>  	return 0;
> @@ -935,6 +965,98 @@ void dev_pm_opp_put_supported_hw(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_put_supported_hw);
>  
> +/**
> + * dev_pm_opp_set_prop_name() - Set prop-extn name
> + * @dev: Device for which the regulator has to be set.
> + * @name: name to postfix to properties.
> + *
> + * This is required only for the V2 bindings, and it enables a platform to
> + * specify the extn to be used for certain property names. The properties to
> + * which the extension will apply are opp-microvolt and opp-microamp. OPP core
> + * should postfix the property name with -<name> while looking for them.
> + */
> +int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
> +{
> +	struct device_opp *dev_opp;
> +	int ret = 0;
> +
> +	if (!dev || !name) {
> +		pr_err("%s: Invalid arguments, dev:0x%p, name:%p\n", __func__,
> +		       dev, name);
> +		return -EINVAL;

Same defensive programming and printing NULL comments from patch
2 apply here.

> +	}
> +
> +	/* Operations on OPP structures must be done from within rcu locks */
> +	rcu_read_lock();
> +
> +	dev_opp = _add_device_opp(dev);
> +	if (!dev_opp)

goto unlock?

> +		return -ENOMEM;
> +
> +	/* Do we already have a prop-name associated with dev_opp? */
> +	if (dev_opp->prop_name) {
> +		dev_err(dev, "%s: Already have prop-name %s\n", __func__,
> +			dev_opp->prop_name);
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	dev_opp->prop_name = kstrdup(name, GFP_KERNEL);

I'm very confused now on the whole locking scheme. How can we be
modifying the dev_opp under an rcu_read_lock? Don't we need to
hold a stronger lock than a read lock because _add_device_opp()
has already published the structure we're modifying here?

> +	if (!dev_opp->prop_name) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +unlock:
> +	rcu_read_unlock();
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_set_prop_name);
> +
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index d12471ed14a2..eec973130951 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -141,6 +143,13 @@ static inline int dev_pm_opp_set_supported_hw(struct device *dev, u32 *versions,
>  
>  static inline void dev_pm_opp_put_supported_hw(struct device *dev) {}
>  
> +static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline void dev_pm_opp_put_prop_name(struct device *dev) {}

How is cpufreq-dt going to be changed to support this? I wonder
if it would be better to hide these sorts of things behind a
wrapper on top of the guts of dev_pm_opp_of_add_table(). That way
the lifetime of the prop_name and hw_versions are contained to
the same lifetime rules as the device's OPP table. Right now it
seems like we're asking OPP initializers to call two or three
functions in the right order to get the table initialized
properly, which is not as easy as calling one function.

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

Re: [PATCH 3/3] PM / OPP: Parse 'opp-<prop>-<name>' bindings

[Viresh Kumar]

On 18-11-15, 17:12, Stephen Boyd wrote:
> > +	/* Operations on OPP structures must be done from within rcu locks */
> > +	rcu_read_lock();
> > +
> > +	dev_opp = _add_device_opp(dev);
> > +	if (!dev_opp)
> > +		return -ENOMEM;
> > +
> > +	/* Do we already have a prop-name associated with dev_opp? */
> > +	if (dev_opp->prop_name) {
> > +		dev_err(dev, "%s: Already have prop-name %s\n", __func__,
> > +			dev_opp->prop_name);
> > +		ret = -EINVAL;
> > +		goto unlock;
> > +	}
> > +
> > +	dev_opp->prop_name = kstrdup(name, GFP_KERNEL);
> 
> I'm very confused now on the whole locking scheme. How can we be
> modifying the dev_opp under an rcu_read_lock? Don't we need to
> hold a stronger lock than a read lock because _add_device_opp()
> has already published the structure we're modifying here?

Yeah, it should be called from within the mutex lock we have.

> How is cpufreq-dt going to be changed to support this?

Maybe not at all :)

> I wonder
> if it would be better to hide these sorts of things behind a
> wrapper on top of the guts of dev_pm_opp_of_add_table(). That way
> the lifetime of the prop_name and hw_versions are contained to
> the same lifetime rules as the device's OPP table. Right now it
> seems like we're asking OPP initializers to call two or three
> functions in the right order to get the table initialized
> properly, which is not as easy as calling one function.

Yeah, so things should happen in order, but the caller for the new
routines can't be cpufreq-dt (well it can be, but then some
information is required via platform data).

These routines are supposed to be called by some sort of platform
specific code, as we need to read some efuses from hardware to know
about all this. And cpufreq-dt can't decide that.

Though, to keep things simple, we can put that information in platform
data, so that only cpufreq-dt does all the stuff in the correct order.

We can always add a wrapper which will add hardware-versions,
named-properties and finally initialize opp table. But I would like to
wait a bit for that.

Now that I need to update 2/3 again, due to your comments on this
patch, I will repost this series again instead of messing up here.

-- 
viresh