[PATCH V2 0/9] OPP: Support multiple power-domains per device

linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH V2 0/9] OPP: Support multiple power-domains per device
@ 2018-10-12 11:11 Viresh Kumar
  2018-10-12 11:11 ` [PATCH V2 1/9] OPP: Identify and mark genpd OPP tables Viresh Kumar
                   ` (8 more replies)
  0 siblings, 9 replies; 18+ messages in thread
From: Viresh Kumar @ 2018-10-12 11:11 UTC (permalink / raw)
  To: ulf.hansson, Kevin Hilman, Len Brown, Nishanth Menon,
	Pavel Machek, Rafael J. Wysocki, Stephen Boyd, Viresh Kumar
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, niklas.cassel, rnayak,
	linux-kernel

Hello,

We are already at 4.19-rc7 and there may not enough time to get this
reviewed/tested/ready for the upcoming merge window and so there is no
hurry in getting this merged. The purpose to send now is to get this
reviewed/tested, which would allow more more development to happen on
top of this.

This series improves the OPP core (and a bit of genpd core as well) to
support multiple phandles in the "required-opps" property, which are
only used for multiple power-domains per device for now.

We still don't propagate the changes to master domains for the
sub-domains, but this patchset is an important stepping stone for that
to happen.

Tested on Hikey960 after faking some power domains for CPUs.

V1->V2:
- Had a discussion at Linaro connect with Ulf regarding the changes V1
  did in the genpd core and what his objections are to them. Based on
  his suggestions many changes are made.
- The OPP core still needs the virtual device pointers to set the
  performance state for multiple domains, but the genpd core doesn't
  provide them automatically to the OPP core. One of the reasons behind
  this is to give more power to the consumer drivers which may not want
  to enable all the genpds at once.
- The consumer drivers would now need to call the APIs
  dev_pm_opp_{set|put}_genpd_device() in order to set/reset these
  virtual device pointers.
- More locking is put in place to protect the genpd device pointers in
  OPP core.
- Reorg of the code at many places to make code less redundant.

--
viresh
 
Viresh Kumar (9):
  OPP: Identify and mark genpd OPP tables
  OPP: Separate out custom OPP handler specific code
  OPP: Populate required opp tables from "required-opps" property
  OPP: Populate OPPs from "required-opps" property
  PM / Domains: Add genpd_opp_to_performance_state()
  OPP: Add dev_pm_opp_{set|put}_genpd_device() helper
  OPP: Configure all required OPPs
  OPP: Rename and relocate of_genpd_opp_to_performance_state()
  OPP: Remove of_dev_pm_opp_find_required_opp()

 drivers/base/power/domain.c |  51 +++---
 drivers/opp/core.c          | 261 ++++++++++++++++++++++--------
 drivers/opp/of.c            | 313 +++++++++++++++++++++++++++++++-----
 drivers/opp/opp.h           |  20 +++
 include/linux/pm_domain.h   |   7 +-
 include/linux/pm_opp.h      |  16 +-
 6 files changed, 519 insertions(+), 149 deletions(-)

-- 
2.18.0.rc1.242.g61856ae69a2c


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH V2 1/9] OPP: Identify and mark genpd OPP tables
  2018-10-12 11:11 [PATCH V2 0/9] OPP: Support multiple power-domains per device Viresh Kumar
@ 2018-10-12 11:11 ` Viresh Kumar
  2018-10-12 15:11   ` Ulf Hansson
  2018-10-12 11:11 ` [PATCH V2 2/9] OPP: Separate out custom OPP handler specific code Viresh Kumar
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 18+ messages in thread
From: Viresh Kumar @ 2018-10-12 11:11 UTC (permalink / raw)
  To: ulf.hansson, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	niklas.cassel, rnayak, linux-kernel

We need to handle genpd OPP tables differently, this is already the case
at one location and will be extended going forward. Add another field to
the OPP table to check if the table belongs to a genpd or not.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/of.c  | 6 ++++--
 drivers/opp/opp.h | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 5a4b47958073..5f114cd3d88c 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -92,6 +92,9 @@ void _of_init_opp_table(struct opp_table *opp_table, struct device *dev,
 	of_property_read_u32(np, "voltage-tolerance",
 			     &opp_table->voltage_tolerance_v1);
 
+	if (of_find_property(np, "#power-domain-cells", NULL))
+		opp_table->is_genpd = true;
+
 	/* Get OPP table node */
 	opp_np = _opp_of_get_opp_desc_node(np, index);
 	of_node_put(np);
@@ -326,8 +329,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	ret = of_property_read_u64(np, "opp-hz", &rate);
 	if (ret < 0) {
 		/* "opp-hz" is optional for devices like power domains. */
-		if (!of_find_property(dev->of_node, "#power-domain-cells",
-				      NULL)) {
+		if (!opp_table->is_genpd) {
 			dev_err(dev, "%s: opp-hz not found\n", __func__);
 			goto free_opp;
 		}
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 9c6544b4f4f9..cdb0c2b095e2 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -140,6 +140,7 @@ enum opp_table_access {
  * @regulators: Supply regulators
  * @regulator_count: Number of power supply regulators
  * @genpd_performance_state: Device's power domain support performance state.
+ * @is_genpd: Marks if the OPP table belongs to a genpd.
  * @set_opp: Platform specific set_opp callback
  * @set_opp_data: Data to be passed to set_opp callback
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
@@ -178,6 +179,7 @@ struct opp_table {
 	struct regulator **regulators;
 	unsigned int regulator_count;
 	bool genpd_performance_state;
+	bool is_genpd;
 
 	int (*set_opp)(struct dev_pm_set_opp_data *data);
 	struct dev_pm_set_opp_data *set_opp_data;
-- 
2.18.0.rc1.242.g61856ae69a2c


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH V2 2/9] OPP: Separate out custom OPP handler specific code
  2018-10-12 11:11 [PATCH V2 0/9] OPP: Support multiple power-domains per device Viresh Kumar
  2018-10-12 11:11 ` [PATCH V2 1/9] OPP: Identify and mark genpd OPP tables Viresh Kumar
@ 2018-10-12 11:11 ` Viresh Kumar
  2018-10-12 15:11   ` Ulf Hansson
  2018-10-12 11:11 ` [PATCH V2 3/9] OPP: Populate required opp tables from "required-opps" property Viresh Kumar
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 18+ messages in thread
From: Viresh Kumar @ 2018-10-12 11:11 UTC (permalink / raw)
  To: ulf.hansson, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	niklas.cassel, rnayak, linux-kernel

Create a separate routine to take care of custom set_opp() handler
specific stuff.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 67 +++++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 2c2df4e4fc14..ebb3b648e0fd 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -635,6 +635,34 @@ static int _generic_set_opp_regulator(const struct opp_table *opp_table,
 	return ret;
 }
 
+static int _set_opp_custom(const struct opp_table *opp_table,
+			   struct device *dev, unsigned long old_freq,
+			   unsigned long freq,
+			   struct dev_pm_opp_supply *old_supply,
+			   struct dev_pm_opp_supply *new_supply)
+{
+	struct dev_pm_set_opp_data *data;
+	int size;
+
+	data = opp_table->set_opp_data;
+	data->regulators = opp_table->regulators;
+	data->regulator_count = opp_table->regulator_count;
+	data->clk = opp_table->clk;
+	data->dev = dev;
+
+	data->old_opp.rate = old_freq;
+	size = sizeof(*old_supply) * opp_table->regulator_count;
+	if (IS_ERR(old_supply))
+		memset(data->old_opp.supplies, 0, size);
+	else
+		memcpy(data->old_opp.supplies, old_supply, size);
+
+	data->new_opp.rate = freq;
+	memcpy(data->new_opp.supplies, new_supply, size);
+
+	return opp_table->set_opp(data);
+}
+
 /**
  * dev_pm_opp_set_rate() - Configure new OPP based on frequency
  * @dev:	 device for which we do this operation
@@ -649,7 +677,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	unsigned long freq, old_freq;
 	struct dev_pm_opp *old_opp, *opp;
 	struct clk *clk;
-	int ret, size;
+	int ret;
 
 	if (unlikely(!target_freq)) {
 		dev_err(dev, "%s: Invalid target frequency %lu\n", __func__,
@@ -702,8 +730,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
 		old_freq, freq);
 
-	/* Only frequency scaling */
-	if (!opp_table->regulators) {
+	if (opp_table->set_opp) {
+		ret = _set_opp_custom(opp_table, dev, old_freq, freq,
+				      IS_ERR(old_opp) ? NULL : old_opp->supplies,
+				      opp->supplies);
+	} else if (opp_table->regulators) {
+		ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
+						 IS_ERR(old_opp) ? NULL : old_opp->supplies,
+						 opp->supplies);
+	} else {
+		/* Only frequency scaling */
+
 		/*
 		 * We don't support devices with both regulator and
 		 * domain performance-state for now.
@@ -714,30 +751,6 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 						      opp->pstate);
 		else
 			ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
-	} else if (!opp_table->set_opp) {
-		ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
-						 IS_ERR(old_opp) ? NULL : old_opp->supplies,
-						 opp->supplies);
-	} else {
-		struct dev_pm_set_opp_data *data;
-
-		data = opp_table->set_opp_data;
-		data->regulators = opp_table->regulators;
-		data->regulator_count = opp_table->regulator_count;
-		data->clk = clk;
-		data->dev = dev;
-
-		data->old_opp.rate = old_freq;
-		size = sizeof(*opp->supplies) * opp_table->regulator_count;
-		if (IS_ERR(old_opp))
-			memset(data->old_opp.supplies, 0, size);
-		else
-			memcpy(data->old_opp.supplies, old_opp->supplies, size);
-
-		data->new_opp.rate = freq;
-		memcpy(data->new_opp.supplies, opp->supplies, size);
-
-		ret = opp_table->set_opp(data);
 	}
 
 	dev_pm_opp_put(opp);
-- 
2.18.0.rc1.242.g61856ae69a2c


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH V2 3/9] OPP: Populate required opp tables from "required-opps" property
  2018-10-12 11:11 [PATCH V2 0/9] OPP: Support multiple power-domains per device Viresh Kumar
  2018-10-12 11:11 ` [PATCH V2 1/9] OPP: Identify and mark genpd OPP tables Viresh Kumar
  2018-10-12 11:11 ` [PATCH V2 2/9] OPP: Separate out custom OPP handler specific code Viresh Kumar
@ 2018-10-12 11:11 ` Viresh Kumar
  2018-10-12 15:11   ` Ulf Hansson
  2018-10-12 11:11 ` [PATCH V2 4/9] OPP: Populate OPPs " Viresh Kumar
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 18+ messages in thread
From: Viresh Kumar @ 2018-10-12 11:11 UTC (permalink / raw)
  To: ulf.hansson, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	niklas.cassel, rnayak, linux-kernel

The current implementation works only for the case where a single
phandle is present in the "required-opps" property, while DT allows
multiple phandles to be present there.

This patch adds new infrastructure to parse all the phandles present in
"required-opps" property and save pointers of the required OPP's OPP
tables. These will be used by later commits.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c |   2 +
 drivers/opp/of.c   | 147 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/opp/opp.h  |   8 +++
 3 files changed, 157 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index ebb3b648e0fd..85174a5c4850 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -901,6 +901,8 @@ static void _opp_table_kref_release(struct kref *kref)
 	struct opp_table *opp_table = container_of(kref, struct opp_table, kref);
 	struct opp_device *opp_dev, *temp;
 
+	_of_clear_opp_table(opp_table);
+
 	/* Release clk */
 	if (!IS_ERR(opp_table->clk))
 		clk_put(opp_table->clk);
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 5f114cd3d88c..b5605196122a 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -73,6 +73,147 @@ struct opp_table *_managed_opp(struct device *dev, int index)
 	return managed_table;
 }
 
+/* The caller must call dev_pm_opp_put() after the OPP is used */
+static struct dev_pm_opp *_find_opp_of_np(struct opp_table *opp_table,
+					  struct device_node *opp_np)
+{
+	struct dev_pm_opp *opp;
+
+	lockdep_assert_held(&opp_table_lock);
+
+	mutex_lock(&opp_table->lock);
+
+	list_for_each_entry(opp, &opp_table->opp_list, node) {
+		if (opp->np == opp_np) {
+			dev_pm_opp_get(opp);
+			mutex_unlock(&opp_table->lock);
+			return opp;
+		}
+	}
+
+	mutex_unlock(&opp_table->lock);
+
+	return NULL;
+}
+
+static struct device_node *of_parse_required_opp(struct device_node *np,
+						 int index)
+{
+	struct device_node *required_np;
+
+	required_np = of_parse_phandle(np, "required-opps", index);
+	if (unlikely(!required_np)) {
+		pr_err("%s: Unable to parse required-opps: %pOF, index: %d\n",
+		       __func__, np, index);
+	}
+
+	return required_np;
+}
+
+/* The caller must call dev_pm_opp_put_opp_table() after the table is used */
+static struct opp_table *_find_table_of_opp_np(struct device_node *opp_np)
+{
+	struct opp_table *opp_table;
+	struct dev_pm_opp *opp;
+
+	lockdep_assert_held(&opp_table_lock);
+
+	list_for_each_entry(opp_table, &opp_tables, node) {
+		opp = _find_opp_of_np(opp_table, opp_np);
+		if (opp) {
+			dev_pm_opp_put(opp);
+			_get_opp_table_kref(opp_table);
+			return opp_table;
+		}
+	}
+
+	return ERR_PTR(-ENODEV);
+}
+
+/* Free resources previously acquired by _opp_table_alloc_required_tables() */
+static void _opp_table_free_required_tables(struct opp_table *opp_table)
+{
+	struct opp_table **required_opp_tables = opp_table->required_opp_tables;
+	int i;
+
+	if (!required_opp_tables)
+		return;
+
+	for (i = 0; i < opp_table->required_opp_count; i++) {
+		if (IS_ERR_OR_NULL(required_opp_tables[i]))
+			break;
+
+		dev_pm_opp_put_opp_table(required_opp_tables[i]);
+	}
+
+	kfree(required_opp_tables);
+
+	opp_table->required_opp_count = 0;
+	opp_table->required_opp_tables = NULL;
+}
+
+/*
+ * Populate all devices and opp tables which are part of "required-opps" list.
+ * Checking only the first OPP node should be enough.
+ */
+static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
+					     struct device *dev,
+					     struct device_node *opp_np)
+{
+	struct opp_table **required_opp_tables;
+	struct device_node *required_np, *np;
+	int count, i;
+
+	/* Traversing the first OPP node is all we need */
+	np = of_get_next_available_child(opp_np, NULL);
+	if (!np) {
+		dev_err(dev, "Empty OPP table\n");
+		return;
+	}
+
+	count = of_count_phandle_with_args(np, "required-opps", NULL);
+	if (!count)
+		goto put_np;
+
+	required_opp_tables = kcalloc(count, sizeof(*required_opp_tables),
+				      GFP_KERNEL);
+	if (!required_opp_tables)
+		goto put_np;
+
+	opp_table->required_opp_tables = required_opp_tables;
+	opp_table->required_opp_count = count;
+
+	for (i = 0; i < count; i++) {
+		required_np = of_parse_required_opp(np, i);
+		if (!required_np)
+			goto free_required_tables;
+
+		required_opp_tables[i] = _find_table_of_opp_np(required_np);
+		of_node_put(required_np);
+
+		if (IS_ERR(required_opp_tables[i]))
+			goto free_required_tables;
+
+		/*
+		 * We only support genpd's OPPs in the "required-opps" for now,
+		 * as we don't know how much about other cases. Error out if the
+		 * required OPP doesn't belong to a genpd.
+		 */
+		if (!required_opp_tables[i]->is_genpd) {
+			dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
+				required_np);
+			goto free_required_tables;
+		}
+	}
+
+	goto put_np;
+
+free_required_tables:
+	_opp_table_free_required_tables(opp_table);
+put_np:
+	of_node_put(np);
+}
+
 void _of_init_opp_table(struct opp_table *opp_table, struct device *dev,
 			int index)
 {
@@ -109,9 +250,15 @@ void _of_init_opp_table(struct opp_table *opp_table, struct device *dev,
 
 	opp_table->np = opp_np;
 
+	_opp_table_alloc_required_tables(opp_table, dev, opp_np);
 	of_node_put(opp_np);
 }
 
+void _of_clear_opp_table(struct opp_table *opp_table)
+{
+	_opp_table_free_required_tables(opp_table);
+}
+
 static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
 			      struct device_node *np)
 {
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index cdb0c2b095e2..024e1be23d37 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -133,6 +133,9 @@ enum opp_table_access {
  * @parsed_static_opps: True if OPPs are initialized from DT.
  * @shared_opp: OPP is shared between multiple devices.
  * @suspend_opp: Pointer to OPP to be used during device suspend.
+ * @required_opp_tables: List of device OPP tables that are required by OPPs in
+ *		this table.
+ * @required_opp_count: Number of required devices.
  * @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.
@@ -172,6 +175,9 @@ struct opp_table {
 	enum opp_table_access shared_opp;
 	struct dev_pm_opp *suspend_opp;
 
+	struct opp_table **required_opp_tables;
+	unsigned int required_opp_count;
+
 	unsigned int *supported_hw;
 	unsigned int supported_hw_count;
 	const char *prop_name;
@@ -208,9 +214,11 @@ void _put_opp_list_kref(struct opp_table *opp_table);
 
 #ifdef CONFIG_OF
 void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index);
+void _of_clear_opp_table(struct opp_table *opp_table);
 struct opp_table *_managed_opp(struct device *dev, int index);
 #else
 static inline void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index) {}
+static inline void _of_clear_opp_table(struct opp_table *opp_table) {}
 static inline struct opp_table *_managed_opp(struct device *dev, int index) { return NULL; }
 #endif
 
-- 
2.18.0.rc1.242.g61856ae69a2c


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH V2 4/9] OPP: Populate OPPs from "required-opps" property
  2018-10-12 11:11 [PATCH V2 0/9] OPP: Support multiple power-domains per device Viresh Kumar
                   ` (2 preceding siblings ...)
  2018-10-12 11:11 ` [PATCH V2 3/9] OPP: Populate required opp tables from "required-opps" property Viresh Kumar
@ 2018-10-12 11:11 ` Viresh Kumar
  2018-10-12 15:11   ` Ulf Hansson
  2018-10-12 11:11 ` [PATCH V2 5/9] PM / Domains: Add genpd_opp_to_performance_state() Viresh Kumar
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 18+ messages in thread
From: Viresh Kumar @ 2018-10-12 11:11 UTC (permalink / raw)
  To: ulf.hansson, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	niklas.cassel, rnayak, linux-kernel

An earlier commit populated the OPP tables from the "required-opps"
property, this commit populates the individual OPPs. This is repeated
for each OPP in the OPP table and these populated OPPs will be used by
later commits.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c |  1 +
 drivers/opp/of.c   | 81 ++++++++++++++++++++++++++++++++++++++++++++--
 drivers/opp/opp.h  |  6 ++++
 3 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 85174a5c4850..02a69a62dac8 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -976,6 +976,7 @@ static void _opp_kref_release(struct kref *kref)
 	 * frequency/voltage list.
 	 */
 	blocking_notifier_call_chain(&opp_table->head, OPP_EVENT_REMOVE, opp);
+	_of_opp_free_required_opps(opp_table, opp);
 	opp_debug_remove_one(opp);
 	list_del(&opp->node);
 	kfree(opp);
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index b5605196122a..ffaeefef98ce 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -259,6 +259,77 @@ void _of_clear_opp_table(struct opp_table *opp_table)
 	_opp_table_free_required_tables(opp_table);
 }
 
+/*
+ * Release all resources previously acquired with a call to
+ * _of_opp_alloc_required_opps().
+ */
+void _of_opp_free_required_opps(struct opp_table *opp_table,
+				struct dev_pm_opp *opp)
+{
+	struct dev_pm_opp **required_opps = opp->required_opps;
+	int i;
+
+	if (!required_opps)
+		return;
+
+	for (i = 0; i < opp_table->required_opp_count; i++) {
+		if (!required_opps[i])
+			break;
+
+		/* Put the reference back */
+		dev_pm_opp_put(required_opps[i]);
+	}
+
+	kfree(required_opps);
+	opp->required_opps = NULL;
+}
+
+/* Populate all required OPPs which are part of "required-opps" list */
+static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
+				       struct dev_pm_opp *opp)
+{
+	struct dev_pm_opp **required_opps;
+	struct opp_table *required_table;
+	struct device_node *np;
+	int i, ret, count = opp_table->required_opp_count;
+
+	if (!count)
+		return 0;
+
+	required_opps = kcalloc(count, sizeof(*required_opps), GFP_KERNEL);
+	if (!required_opps)
+		return -ENOMEM;
+
+	opp->required_opps = required_opps;
+
+	for (i = 0; i < count; i++) {
+		required_table = opp_table->required_opp_tables[i];
+
+		np = of_parse_required_opp(opp->np, i);
+		if (unlikely(!np)) {
+			ret = -ENODEV;
+			goto free_required_opps;
+		}
+
+		required_opps[i] = _find_opp_of_np(required_table, np);
+		of_node_put(np);
+
+		if (!required_opps[i]) {
+			pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
+			       __func__, opp->np, i);
+			ret = -ENODEV;
+			goto free_required_opps;
+		}
+	}
+
+	return 0;
+
+free_required_opps:
+	_of_opp_free_required_opps(opp_table, opp);
+
+	return ret;
+}
+
 static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
 			      struct device_node *np)
 {
@@ -503,6 +574,10 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	new_opp->dynamic = false;
 	new_opp->available = true;
 
+	ret = _of_opp_alloc_required_opps(opp_table, new_opp);
+	if (ret)
+		goto free_opp;
+
 	if (!of_property_read_u32(np, "clock-latency-ns", &val))
 		new_opp->clock_latency_ns = val;
 
@@ -510,14 +585,14 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 
 	ret = opp_parse_supplies(new_opp, dev, opp_table);
 	if (ret)
-		goto free_opp;
+		goto free_required_opps;
 
 	ret = _opp_add(dev, new_opp, opp_table, rate_not_available);
 	if (ret) {
 		/* Don't return error for duplicate OPPs */
 		if (ret == -EBUSY)
 			ret = 0;
-		goto free_opp;
+		goto free_required_opps;
 	}
 
 	/* OPP to select on device suspend */
@@ -547,6 +622,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	blocking_notifier_call_chain(&opp_table->head, OPP_EVENT_ADD, new_opp);
 	return new_opp;
 
+free_required_opps:
+	_of_opp_free_required_opps(opp_table, new_opp);
 free_opp:
 	_opp_free(new_opp);
 
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 024e1be23d37..24b340ad18d1 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -63,6 +63,7 @@ extern struct list_head opp_tables;
  * @supplies:	Power supplies voltage/current values
  * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
  *		frequency from any other OPP's frequency.
+ * @required_opps: List of OPPs that are required by this OPP.
  * @opp_table:	points back to the opp_table struct this opp belongs to
  * @np:		OPP's device node.
  * @dentry:	debugfs dentry pointer (per opp)
@@ -84,6 +85,7 @@ struct dev_pm_opp {
 
 	unsigned long clock_latency_ns;
 
+	struct dev_pm_opp **required_opps;
 	struct opp_table *opp_table;
 
 	struct device_node *np;
@@ -216,10 +218,14 @@ void _put_opp_list_kref(struct opp_table *opp_table);
 void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index);
 void _of_clear_opp_table(struct opp_table *opp_table);
 struct opp_table *_managed_opp(struct device *dev, int index);
+void _of_opp_free_required_opps(struct opp_table *opp_table,
+				struct dev_pm_opp *opp);
 #else
 static inline void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index) {}
 static inline void _of_clear_opp_table(struct opp_table *opp_table) {}
 static inline struct opp_table *_managed_opp(struct device *dev, int index) { return NULL; }
+static inline void _of_opp_free_required_opps(struct opp_table *opp_table,
+					      struct dev_pm_opp *opp) {}
 #endif
 
 #ifdef CONFIG_DEBUG_FS
-- 
2.18.0.rc1.242.g61856ae69a2c


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH V2 5/9] PM / Domains: Add genpd_opp_to_performance_state()
  2018-10-12 11:11 [PATCH V2 0/9] OPP: Support multiple power-domains per device Viresh Kumar
                   ` (3 preceding siblings ...)
  2018-10-12 11:11 ` [PATCH V2 4/9] OPP: Populate OPPs " Viresh Kumar
@ 2018-10-12 11:11 ` Viresh Kumar
  2018-10-12 15:07   ` Ulf Hansson
  2018-10-12 11:11 ` [PATCH V2 6/9] OPP: Add dev_pm_opp_{set|put}_genpd_device() helper Viresh Kumar
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 18+ messages in thread
From: Viresh Kumar @ 2018-10-12 11:11 UTC (permalink / raw)
  To: ulf.hansson, Rafael J. Wysocki, Kevin Hilman, Pavel Machek, Len Brown
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Stephen Boyd,
	Nishanth Menon, niklas.cassel, rnayak, linux-kernel

The OPP core currently stores the performance state in the consumer
device's OPP table, but that is going to change going forward and
performance state will rather be set directly in the genpd's OPP table.

For that we need to get the performance state for genpd's device
structure instead of the consumer device's structure. Add a new helper
to do that.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/domain.c | 39 +++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  8 ++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 4b5714199490..2c82194d2a30 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2508,6 +2508,45 @@ int of_genpd_parse_idle_states(struct device_node *dn,
 }
 EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
 
+/**
+ * genpd_opp_to_performance_state- Gets performance state of the genpd from its OPP node.
+ *
+ * @genpd_dev: Genpd's device for which the performance-state needs to be found.
+ * @opp: struct dev_pm_opp of the OPP for which we need to find performance
+ *	state.
+ *
+ * Returns performance state encoded in the OPP of the genpd. This calls
+ * platform specific genpd->opp_to_performance_state() callback to translate
+ * power domain OPP to performance state.
+ *
+ * Returns performance state on success and 0 on failure.
+ */
+unsigned int genpd_opp_to_performance_state(struct device *genpd_dev,
+					    struct dev_pm_opp *opp)
+{
+	struct generic_pm_domain *genpd = NULL, *temp;
+	int state;
+
+	lockdep_assert_held(&gpd_list_lock);
+
+	list_for_each_entry(temp, &gpd_list, gpd_list_node) {
+		if (&temp->dev == genpd_dev) {
+			genpd = temp;
+			break;
+		}
+	}
+
+	if (unlikely(!genpd || !genpd->opp_to_performance_state))
+		return 0;
+
+	genpd_lock(genpd);
+	state = genpd->opp_to_performance_state(genpd, opp);
+	genpd_unlock(genpd);
+
+	return state;
+}
+EXPORT_SYMBOL_GPL(genpd_opp_to_performance_state);
+
 /**
  * of_genpd_opp_to_performance_state- Gets performance state of device's
  * power domain corresponding to a DT node's "required-opps" property.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 776c546d581a..e03f300f7468 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -233,6 +233,8 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent,
 struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
 int of_genpd_parse_idle_states(struct device_node *dn,
 			       struct genpd_power_state **states, int *n);
+unsigned int genpd_opp_to_performance_state(struct device *genpd_dev,
+					    struct dev_pm_opp *opp);
 unsigned int of_genpd_opp_to_performance_state(struct device *dev,
 				struct device_node *np);
 
@@ -274,6 +276,12 @@ static inline int of_genpd_parse_idle_states(struct device_node *dn,
 	return -ENODEV;
 }
 
+static inline unsigned int
+genpd_opp_to_performance_state(struct device *genpd_dev, struct dev_pm_opp *opp)
+{
+	return 0;
+}
+
 static inline unsigned int
 of_genpd_opp_to_performance_state(struct device *dev,
 				  struct device_node *np)
-- 
2.18.0.rc1.242.g61856ae69a2c


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH V2 6/9] OPP: Add dev_pm_opp_{set|put}_genpd_device() helper
  2018-10-12 11:11 [PATCH V2 0/9] OPP: Support multiple power-domains per device Viresh Kumar
                   ` (4 preceding siblings ...)
  2018-10-12 11:11 ` [PATCH V2 5/9] PM / Domains: Add genpd_opp_to_performance_state() Viresh Kumar
@ 2018-10-12 11:11 ` Viresh Kumar
  2018-10-12 14:46   ` Ulf Hansson
  2018-10-12 11:11 ` [PATCH V2 7/9] OPP: Configure all required OPPs Viresh Kumar
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 18+ messages in thread
From: Viresh Kumar @ 2018-10-12 11:11 UTC (permalink / raw)
  To: ulf.hansson, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	Rafael J. Wysocki
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, niklas.cassel, rnayak,
	linux-kernel

Multiple generic power domains for a consumer device are supported with
the help of virtual devices, which are created for each consumer device
- genpd pair. These are the device structures which are attached to the
power domain and are required by the OPP core to set the performance
state of the genpd.

The helpers added by this commit are required to be called once for each
of these virtual devices. These are required only if multiple domains
are available for a device, otherwise the actual device structure will
be used instead by the OPP core.

The new helpers also support the complex cases where the consumer device
wouldn't always require all the domains. For example, a camera may
require only one power domain during normal operations but two during
high resolution operations. The consumer driver can call
dev_pm_opp_put_genpd_device(high_resolution_genpd_dev) if it is
currently operating in the normal mode and doesn't have any performance
requirements from the genpd which manages high resolution power
requirements. The consumer driver can later call
dev_pm_opp_set_genpd_device(high_resolution_genpd_dev) once it switches
back to the high resolution mode.

The new helpers differ from other OPP set/put helpers as the new ones
can be called with OPPs initialized for the table as we may need to call
them on the fly because of the complex case explained above. For this
reason it is possible that the genpd_device structure may be used in
parallel while the new helpers are running and a new mutex is added to
protect against that. We didn't use the existing opp_table->lock mutex
as that is widely used in the OPP core and we will need a lock in the
hotpath now, i.e.  while changing OPP and we need to make sure there is
not much contention while doing that.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 88 ++++++++++++++++++++++++++++++++++++++++++
 drivers/opp/of.c       | 16 +++++++-
 drivers/opp/opp.h      |  4 ++
 include/linux/pm_opp.h |  8 ++++
 4 files changed, 115 insertions(+), 1 deletion(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 02a69a62dac8..8d7bf9d83752 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -823,6 +823,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 		return NULL;
 
 	mutex_init(&opp_table->lock);
+	mutex_init(&opp_table->genpd_dev_lock);
 	INIT_LIST_HEAD(&opp_table->dev_list);
 
 	opp_dev = _add_opp_dev(dev, opp_table);
@@ -920,6 +921,7 @@ static void _opp_table_kref_release(struct kref *kref)
 		_remove_opp_dev(opp_dev, opp_table);
 	}
 
+	mutex_destroy(&opp_table->genpd_dev_lock);
 	mutex_destroy(&opp_table->lock);
 	list_del(&opp_table->node);
 	kfree(opp_table);
@@ -1602,6 +1604,92 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_set_opp_helper);
 
+/**
+ * dev_pm_opp_set_genpd_device - Set virtual genpd device for an index
+ * @dev: Consumer device for which the genpd device is getting set.
+ * @genpd_dev: virtual genpd device.
+ * @index: index.
+ *
+ * Multiple generic power domains for a device are supported with the help of
+ * virtual genpd devices, which are created for each consumer device - genpd
+ * pair. These are the device structures which are attached to the power domain
+ * and are required by the OPP core to set the performance state of the genpd.
+ *
+ * This helper will normally be called by the consumer driver of the device
+ * "dev", as only that has details of the genpd devices.
+ *
+ * This helper needs to be called once for each of those virtual devices, but
+ * only if multiple domains are available for a device. Otherwise the original
+ * device structure will be used instead by the OPP core.
+ */
+struct opp_table *dev_pm_opp_set_genpd_device(struct device *dev,
+					      struct device *genpd_device,
+					      int index)
+{
+	struct opp_table *opp_table;
+
+	opp_table = dev_pm_opp_get_opp_table(dev);
+	if (!opp_table)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&opp_table->genpd_dev_lock);
+
+	if (unlikely(!opp_table->genpd_devices ||
+		     index >= opp_table->required_opp_count ||
+		     opp_table->genpd_devices[index])) {
+
+		dev_err(dev, "Invalid request to set required device\n");
+		dev_pm_opp_put_opp_table(opp_table);
+		mutex_unlock(&opp_table->genpd_dev_lock);
+
+		return ERR_PTR(-EINVAL);
+	}
+
+	opp_table->genpd_devices[index] = genpd_device;
+	mutex_unlock(&opp_table->genpd_dev_lock);
+
+	return opp_table;
+}
+
+/**
+ * dev_pm_opp_put_genpd_device() - Releases resources blocked for genpd device.
+ * @opp_table: OPP table returned by dev_pm_opp_set_genpd_device().
+ * @genpd_device: virtual genpd device.
+ *
+ * This releases the resource previously acquired with a call to
+ * dev_pm_opp_set_genpd_device(). The consumer driver shall call this helper if
+ * it doesn't want OPP core to update performance state of a power domain
+ * anymore.
+ */
+void dev_pm_opp_put_genpd_device(struct opp_table *opp_table,
+				 struct device *genpd_device)
+{
+	int i;
+
+	/*
+	 * Acquire genpd_dev_lock to make sure genpd_device isn't getting used
+	 * in parallel.
+	 */
+	mutex_lock(&opp_table->genpd_dev_lock);
+
+	for (i = 0; i < opp_table->required_opp_count; i++) {
+		if (opp_table->genpd_devices[i] != genpd_device)
+			continue;
+
+		opp_table->genpd_devices[i] = NULL;
+		dev_pm_opp_put_opp_table(opp_table);
+
+		/* Drop the vote */
+		dev_pm_genpd_set_performance_state(genpd_device, 0);
+		break;
+	}
+
+	mutex_unlock(&opp_table->genpd_dev_lock);
+
+	if (unlikely(i == opp_table->required_opp_count))
+		dev_err(genpd_device, "Failed to find required device entry\n");
+}
+
 /**
  * dev_pm_opp_add()  - Add an OPP table from a table definitions
  * @dev:	device for which we do this operation
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index ffaeefef98ce..fd67c9b55d7d 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -134,6 +134,7 @@ static struct opp_table *_find_table_of_opp_np(struct device_node *opp_np)
 static void _opp_table_free_required_tables(struct opp_table *opp_table)
 {
 	struct opp_table **required_opp_tables = opp_table->required_opp_tables;
+	struct device **genpd_devices = opp_table->genpd_devices;
 	int i;
 
 	if (!required_opp_tables)
@@ -147,8 +148,10 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table)
 	}
 
 	kfree(required_opp_tables);
+	kfree(genpd_devices);
 
 	opp_table->required_opp_count = 0;
+	opp_table->genpd_devices = NULL;
 	opp_table->required_opp_tables = NULL;
 }
 
@@ -161,6 +164,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 					     struct device_node *opp_np)
 {
 	struct opp_table **required_opp_tables;
+	struct device **genpd_devices = NULL;
 	struct device_node *required_np, *np;
 	int count, i;
 
@@ -175,11 +179,21 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 	if (!count)
 		goto put_np;
 
+	if (count > 1) {
+		genpd_devices = kcalloc(count, sizeof(*genpd_devices),
+					GFP_KERNEL);
+		if (!genpd_devices)
+			goto put_np;
+	}
+
 	required_opp_tables = kcalloc(count, sizeof(*required_opp_tables),
 				      GFP_KERNEL);
-	if (!required_opp_tables)
+	if (!required_opp_tables) {
+		kfree(genpd_devices);
 		goto put_np;
+	}
 
+	opp_table->genpd_devices = genpd_devices;
 	opp_table->required_opp_tables = required_opp_tables;
 	opp_table->required_opp_count = count;
 
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 24b340ad18d1..aea69a24c26b 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -135,6 +135,8 @@ enum opp_table_access {
  * @parsed_static_opps: True if OPPs are initialized from DT.
  * @shared_opp: OPP is shared between multiple devices.
  * @suspend_opp: Pointer to OPP to be used during device suspend.
+ * @genpd_dev_lock: Mutex protecting the genpd device pointers.
+ * @genpd_devices: List of virtual devices for multiple genpd support.
  * @required_opp_tables: List of device OPP tables that are required by OPPs in
  *		this table.
  * @required_opp_count: Number of required devices.
@@ -177,6 +179,8 @@ struct opp_table {
 	enum opp_table_access shared_opp;
 	struct dev_pm_opp *suspend_opp;
 
+	struct mutex genpd_dev_lock;
+	struct device **genpd_devices;
 	struct opp_table **required_opp_tables;
 	unsigned int required_opp_count;
 
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 5d399eeef172..b14600ce078f 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -126,6 +126,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char * name);
 void dev_pm_opp_put_clkname(struct opp_table *opp_table);
 struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
 void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
+struct opp_table *dev_pm_opp_set_genpd_device(struct device *dev, struct device *genpd_device, int index);
+void dev_pm_opp_put_genpd_device(struct opp_table *opp_table, struct device *genpd_device);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -272,6 +274,12 @@ static inline struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const
 
 static inline void dev_pm_opp_put_clkname(struct opp_table *opp_table) {}
 
+static inline struct opp_table *dev_pm_opp_set_genpd_device(struct device *dev, struct device *genpd_device, int index)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
+static inline void dev_pm_opp_put_genpd_device(struct opp_table *opp_table, struct device *genpd_device) {}
 static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 {
 	return -ENOTSUPP;
-- 
2.18.0.rc1.242.g61856ae69a2c


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH V2 7/9] OPP: Configure all required OPPs
  2018-10-12 11:11 [PATCH V2 0/9] OPP: Support multiple power-domains per device Viresh Kumar
                   ` (5 preceding siblings ...)
  2018-10-12 11:11 ` [PATCH V2 6/9] OPP: Add dev_pm_opp_{set|put}_genpd_device() helper Viresh Kumar
@ 2018-10-12 11:11 ` Viresh Kumar
  2018-10-12 11:11 ` [PATCH V2 8/9] OPP: Rename and relocate of_genpd_opp_to_performance_state() Viresh Kumar
  2018-10-12 11:11 ` [PATCH V2 9/9] OPP: Remove of_dev_pm_opp_find_required_opp() Viresh Kumar
  8 siblings, 0 replies; 18+ messages in thread
From: Viresh Kumar @ 2018-10-12 11:11 UTC (permalink / raw)
  To: ulf.hansson, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	niklas.cassel, rnayak, linux-kernel

Now that all the infrastructure is in place to support multiple required
OPPs, lets switch over to using it.

A new internal routine _set_required_opps() takes care of updating
performance state for all the required OPPs. With this the performance
state updates are supported even when the end device needs to configure
regulators as well, that wasn't the case earlier.

The pstates were earlier stored in the end device's OPP structures, that
also changes now as those values are stored in the genpd's OPP
structures. And so we switch over to using
genpd_opp_to_performance_state() instead of
of_genpd_opp_to_performance_state() to get performance state for the
genpd OPPs.

The routine _generic_set_opp_domain() is not required anymore and is
removed.

On errors we don't try to recover by reverting to old settings as things
are really complex now and the calls here should never really fail
unless there is a bug. There is no point increasing the complexity, for
code which will never be executed.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 113 ++++++++++++++++++++++++++-------------------
 drivers/opp/of.c   |   5 +-
 2 files changed, 68 insertions(+), 50 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 8d7bf9d83752..1eeb89ab9a68 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -548,44 +548,6 @@ _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
 	return ret;
 }
 
-static inline int
-_generic_set_opp_domain(struct device *dev, struct clk *clk,
-			unsigned long old_freq, unsigned long freq,
-			unsigned int old_pstate, unsigned int new_pstate)
-{
-	int ret;
-
-	/* Scaling up? Scale domain performance state before frequency */
-	if (freq > old_freq) {
-		ret = dev_pm_genpd_set_performance_state(dev, new_pstate);
-		if (ret)
-			return ret;
-	}
-
-	ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
-	if (ret)
-		goto restore_domain_state;
-
-	/* Scaling down? Scale domain performance state after frequency */
-	if (freq < old_freq) {
-		ret = dev_pm_genpd_set_performance_state(dev, new_pstate);
-		if (ret)
-			goto restore_freq;
-	}
-
-	return 0;
-
-restore_freq:
-	if (_generic_set_opp_clk_only(dev, clk, freq, old_freq))
-		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
-			__func__, old_freq);
-restore_domain_state:
-	if (freq > old_freq)
-		dev_pm_genpd_set_performance_state(dev, old_pstate);
-
-	return ret;
-}
-
 static int _generic_set_opp_regulator(const struct opp_table *opp_table,
 				      struct device *dev,
 				      unsigned long old_freq,
@@ -663,6 +625,56 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 	return opp_table->set_opp(data);
 }
 
+/* This is only called for PM domain for now */
+static int _set_required_opps(struct device *dev,
+			      struct opp_table *opp_table,
+			      struct dev_pm_opp *opp)
+{
+	struct opp_table **required_opp_tables = opp_table->required_opp_tables;
+	struct device **genpd_devices = opp_table->genpd_devices;
+	unsigned int pstate;
+	int i, ret = 0;
+
+	if (!required_opp_tables)
+		return 0;
+
+	/* Single genpd case */
+	if (!genpd_devices) {
+		pstate = opp->required_opps[0]->pstate;
+		ret = dev_pm_genpd_set_performance_state(dev, pstate);
+		if (ret) {
+			dev_err(dev, "Failed to set performance state of %s: %d (%d)\n",
+				dev_name(dev), pstate, ret);
+		}
+		return ret;
+	}
+
+	/* Multiple genpd case */
+
+	/*
+	 * Acquire genpd_dev_lock to make sure we don't use a genpd_dev after it
+	 * is freed from another thread.
+	 */
+	mutex_lock(&opp_table->genpd_dev_lock);
+
+	for (i = 0; i < opp_table->required_opp_count; i++) {
+		pstate = opp->required_opps[i]->pstate;
+
+		if (!genpd_devices[i])
+			continue;
+
+		ret = dev_pm_genpd_set_performance_state(genpd_devices[i], pstate);
+		if (ret) {
+			dev_err(dev, "Failed to set performance rate of %s: %d (%d)\n",
+				dev_name(genpd_devices[i]), pstate, ret);
+			break;
+		}
+	}
+	mutex_unlock(&opp_table->genpd_dev_lock);
+
+	return ret;
+}
+
 /**
  * dev_pm_opp_set_rate() - Configure new OPP based on frequency
  * @dev:	 device for which we do this operation
@@ -730,6 +742,13 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
 		old_freq, freq);
 
+	/* Scaling up? Configure required OPPs before frequency */
+	if (freq > old_freq) {
+		ret = _set_required_opps(dev, opp_table, opp);
+		if (ret)
+			goto put_opp;
+	}
+
 	if (opp_table->set_opp) {
 		ret = _set_opp_custom(opp_table, dev, old_freq, freq,
 				      IS_ERR(old_opp) ? NULL : old_opp->supplies,
@@ -740,19 +759,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 						 opp->supplies);
 	} else {
 		/* Only frequency scaling */
+		ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
+	}
 
-		/*
-		 * We don't support devices with both regulator and
-		 * domain performance-state for now.
-		 */
-		if (opp_table->genpd_performance_state)
-			ret = _generic_set_opp_domain(dev, clk, old_freq, freq,
-						      IS_ERR(old_opp) ? 0 : old_opp->pstate,
-						      opp->pstate);
-		else
-			ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
+	/* Scaling down? Configure required OPPs after frequency */
+	if (!ret && freq < old_freq) {
+		ret = _set_required_opps(dev, opp_table, opp);
+		if (ret)
+			dev_err(dev, "Failed to set required opps: %d\n", ret);
 	}
 
+put_opp:
 	dev_pm_opp_put(opp);
 put_old_opp:
 	if (!IS_ERR(old_opp))
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index fd67c9b55d7d..f4e8895542ca 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -595,12 +595,13 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	if (!of_property_read_u32(np, "clock-latency-ns", &val))
 		new_opp->clock_latency_ns = val;
 
-	new_opp->pstate = of_genpd_opp_to_performance_state(dev, np);
-
 	ret = opp_parse_supplies(new_opp, dev, opp_table);
 	if (ret)
 		goto free_required_opps;
 
+	if (opp_table->is_genpd)
+		new_opp->pstate = genpd_opp_to_performance_state(dev, new_opp);
+
 	ret = _opp_add(dev, new_opp, opp_table, rate_not_available);
 	if (ret) {
 		/* Don't return error for duplicate OPPs */
-- 
2.18.0.rc1.242.g61856ae69a2c


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH V2 8/9] OPP: Rename and relocate of_genpd_opp_to_performance_state()
  2018-10-12 11:11 [PATCH V2 0/9] OPP: Support multiple power-domains per device Viresh Kumar
                   ` (6 preceding siblings ...)
  2018-10-12 11:11 ` [PATCH V2 7/9] OPP: Configure all required OPPs Viresh Kumar
@ 2018-10-12 11:11 ` Viresh Kumar
  2018-10-12 11:11 ` [PATCH V2 9/9] OPP: Remove of_dev_pm_opp_find_required_opp() Viresh Kumar
  8 siblings, 0 replies; 18+ messages in thread
From: Viresh Kumar @ 2018-10-12 11:11 UTC (permalink / raw)
  To: ulf.hansson, Rafael J. Wysocki, Kevin Hilman, Pavel Machek,
	Len Brown, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, niklas.cassel, rnayak,
	linux-kernel

The OPP core already has the performance state values for each of the
genpd's OPPs and there is no need to call the genpd callback again to
get the performance state for the case where the end device doesn't have
an OPP table and has the "required-opps" property directly in its node.

This commit renames of_genpd_opp_to_performance_state() as
of_get_required_opp_performance_state() and moves it to the OPP core, as
it is all about OPP stuff now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/domain.c | 48 -------------------------------------
 drivers/opp/of.c            | 44 ++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  9 -------
 include/linux/pm_opp.h      |  5 ++++
 4 files changed, 49 insertions(+), 57 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 2c82194d2a30..55f3a172f6b4 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2547,54 +2547,6 @@ unsigned int genpd_opp_to_performance_state(struct device *genpd_dev,
 }
 EXPORT_SYMBOL_GPL(genpd_opp_to_performance_state);
 
-/**
- * of_genpd_opp_to_performance_state- Gets performance state of device's
- * power domain corresponding to a DT node's "required-opps" property.
- *
- * @dev: Device for which the performance-state needs to be found.
- * @np: DT node where the "required-opps" property is present. This can be
- *	the device node itself (if it doesn't have an OPP table) or a node
- *	within the OPP table of a device (if device has an OPP table).
- *
- * Returns performance state corresponding to the "required-opps" property of
- * a DT node. This calls platform specific genpd->opp_to_performance_state()
- * callback to translate power domain OPP to performance state.
- *
- * Returns performance state on success and 0 on failure.
- */
-unsigned int of_genpd_opp_to_performance_state(struct device *dev,
-					       struct device_node *np)
-{
-	struct generic_pm_domain *genpd;
-	struct dev_pm_opp *opp;
-	int state = 0;
-
-	genpd = dev_to_genpd(dev);
-	if (IS_ERR(genpd))
-		return 0;
-
-	if (unlikely(!genpd->set_performance_state))
-		return 0;
-
-	genpd_lock(genpd);
-
-	opp = of_dev_pm_opp_find_required_opp(&genpd->dev, np);
-	if (IS_ERR(opp)) {
-		dev_err(dev, "Failed to find required OPP: %ld\n",
-			PTR_ERR(opp));
-		goto unlock;
-	}
-
-	state = genpd->opp_to_performance_state(genpd, opp);
-	dev_pm_opp_put(opp);
-
-unlock:
-	genpd_unlock(genpd);
-
-	return state;
-}
-EXPORT_SYMBOL_GPL(of_genpd_opp_to_performance_state);
-
 static int __init genpd_bus_init(void)
 {
 	return bus_register(&genpd_bus_type);
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index f4e8895542ca..b059d833f920 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -969,6 +969,50 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_of_get_sharing_cpus);
 
+/**
+ * of_get_required_opp_performance_state() - Search for required OPP and return its performance state.
+ * @np: Node that contains the "required-opps" property.
+ * @index: Index of the phandle to parse.
+ *
+ * Returns the performance state of the OPP pointed out by the "required-opps"
+ * property at @index in @np.
+ *
+ * Return: Positive performance state on success, otherwise 0 on errors.
+ */
+unsigned int of_get_required_opp_performance_state(struct device_node *np,
+						   int index)
+{
+	struct dev_pm_opp *opp;
+	struct device_node *required_np;
+	struct opp_table *opp_table;
+	unsigned int pstate = 0;
+
+	required_np = of_parse_required_opp(np, index);
+	if (!required_np)
+		return -ENODEV;
+
+	opp_table = _find_table_of_opp_np(required_np);
+	if (IS_ERR(opp_table)) {
+		pr_err("%s: Failed to find required OPP table %pOF: %ld\n",
+		       __func__, np, PTR_ERR(opp_table));
+		goto put_required_np;
+	}
+
+	opp = _find_opp_of_np(opp_table, required_np);
+	if (opp) {
+		pstate = opp->pstate;
+		dev_pm_opp_put(opp);
+	}
+
+	dev_pm_opp_put_opp_table(opp_table);
+
+put_required_np:
+	of_node_put(required_np);
+
+	return pstate;
+}
+EXPORT_SYMBOL_GPL(of_get_required_opp_performance_state);
+
 /**
  * of_dev_pm_opp_find_required_opp() - Search for required OPP.
  * @dev: The device whose OPP node is referenced by the 'np' DT node.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index e03f300f7468..80672bcb1216 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -235,8 +235,6 @@ int of_genpd_parse_idle_states(struct device_node *dn,
 			       struct genpd_power_state **states, int *n);
 unsigned int genpd_opp_to_performance_state(struct device *genpd_dev,
 					    struct dev_pm_opp *opp);
-unsigned int of_genpd_opp_to_performance_state(struct device *dev,
-				struct device_node *np);
 
 int genpd_dev_pm_attach(struct device *dev);
 struct device *genpd_dev_pm_attach_by_id(struct device *dev,
@@ -282,13 +280,6 @@ genpd_opp_to_performance_state(struct device *genpd_dev, struct dev_pm_opp *opp)
 	return 0;
 }
 
-static inline unsigned int
-of_genpd_opp_to_performance_state(struct device *dev,
-				  struct device_node *np)
-{
-	return 0;
-}
-
 static inline int genpd_dev_pm_attach(struct device *dev)
 {
 	return 0;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index b14600ce078f..f440e625f1fd 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -315,6 +315,7 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpuma
 struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev);
 struct dev_pm_opp *of_dev_pm_opp_find_required_opp(struct device *dev, struct device_node *np);
 struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp);
+unsigned int of_get_required_opp_performance_state(struct device_node *np, int index);
 #else
 static inline int dev_pm_opp_of_add_table(struct device *dev)
 {
@@ -357,6 +358,10 @@ static inline struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp)
 {
 	return NULL;
 }
+static inline unsigned int of_get_required_opp_performance_state(struct device_node *np, int index)
+{
+	return 0;
+}
 #endif
 
 #endif		/* __LINUX_OPP_H__ */
-- 
2.18.0.rc1.242.g61856ae69a2c


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH V2 9/9] OPP: Remove of_dev_pm_opp_find_required_opp()
  2018-10-12 11:11 [PATCH V2 0/9] OPP: Support multiple power-domains per device Viresh Kumar
                   ` (7 preceding siblings ...)
  2018-10-12 11:11 ` [PATCH V2 8/9] OPP: Rename and relocate of_genpd_opp_to_performance_state() Viresh Kumar
@ 2018-10-12 11:11 ` Viresh Kumar
  8 siblings, 0 replies; 18+ messages in thread
From: Viresh Kumar @ 2018-10-12 11:11 UTC (permalink / raw)
  To: ulf.hansson, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	Rafael J. Wysocki
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, niklas.cassel, rnayak,
	linux-kernel

This isn't used anymore, remove it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/of.c       | 54 ------------------------------------------
 include/linux/pm_opp.h |  5 ----
 2 files changed, 59 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index b059d833f920..623f651f9e11 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -1013,60 +1013,6 @@ unsigned int of_get_required_opp_performance_state(struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(of_get_required_opp_performance_state);
 
-/**
- * of_dev_pm_opp_find_required_opp() - Search for required OPP.
- * @dev: The device whose OPP node is referenced by the 'np' DT node.
- * @np: Node that contains the "required-opps" property.
- *
- * Returns the OPP of the device 'dev', whose phandle is present in the "np"
- * node. Although the "required-opps" property supports having multiple
- * phandles, this helper routine only parses the very first phandle in the list.
- *
- * Return: Matching opp, else returns ERR_PTR in case of error and should be
- * handled using IS_ERR.
- *
- * The callers are required to call dev_pm_opp_put() for the returned OPP after
- * use.
- */
-struct dev_pm_opp *of_dev_pm_opp_find_required_opp(struct device *dev,
-						   struct device_node *np)
-{
-	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ENODEV);
-	struct device_node *required_np;
-	struct opp_table *opp_table;
-
-	opp_table = _find_opp_table(dev);
-	if (IS_ERR(opp_table))
-		return ERR_CAST(opp_table);
-
-	required_np = of_parse_phandle(np, "required-opps", 0);
-	if (unlikely(!required_np)) {
-		dev_err(dev, "Unable to parse required-opps\n");
-		goto put_opp_table;
-	}
-
-	mutex_lock(&opp_table->lock);
-
-	list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
-		if (temp_opp->available && temp_opp->np == required_np) {
-			opp = temp_opp;
-
-			/* Increment the reference count of OPP */
-			dev_pm_opp_get(opp);
-			break;
-		}
-	}
-
-	mutex_unlock(&opp_table->lock);
-
-	of_node_put(required_np);
-put_opp_table:
-	dev_pm_opp_put_opp_table(opp_table);
-
-	return opp;
-}
-EXPORT_SYMBOL_GPL(of_dev_pm_opp_find_required_opp);
-
 /**
  * dev_pm_opp_get_of_node() - Gets the DT node corresponding to an opp
  * @opp:	opp for which DT node has to be returned for
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index f440e625f1fd..fdd87a21dac4 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -313,7 +313,6 @@ int dev_pm_opp_of_cpumask_add_table(const struct cpumask *cpumask);
 void dev_pm_opp_of_cpumask_remove_table(const struct cpumask *cpumask);
 int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
 struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev);
-struct dev_pm_opp *of_dev_pm_opp_find_required_opp(struct device *dev, struct device_node *np);
 struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp);
 unsigned int of_get_required_opp_performance_state(struct device_node *np, int index);
 #else
@@ -350,10 +349,6 @@ static inline struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device
 	return NULL;
 }
 
-static inline struct dev_pm_opp *of_dev_pm_opp_find_required_opp(struct device *dev, struct device_node *np)
-{
-	return NULL;
-}
 static inline struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp)
 {
 	return NULL;
-- 
2.18.0.rc1.242.g61856ae69a2c


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH V2 6/9] OPP: Add dev_pm_opp_{set|put}_genpd_device() helper
  2018-10-12 11:11 ` [PATCH V2 6/9] OPP: Add dev_pm_opp_{set|put}_genpd_device() helper Viresh Kumar
@ 2018-10-12 14:46   ` Ulf Hansson
  2018-10-12 15:43     ` Viresh Kumar
  0 siblings, 1 reply; 18+ messages in thread
From: Ulf Hansson @ 2018-10-12 14:46 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Linux PM, Vincent Guittot, Niklas Cassel, Rajendra Nayak,
	Linux Kernel Mailing List

On 12 October 2018 at 13:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Multiple generic power domains for a consumer device are supported with
> the help of virtual devices, which are created for each consumer device
> - genpd pair. These are the device structures which are attached to the
> power domain and are required by the OPP core to set the performance
> state of the genpd.
>
> The helpers added by this commit are required to be called once for each
> of these virtual devices. These are required only if multiple domains
> are available for a device, otherwise the actual device structure will
> be used instead by the OPP core.
>
> The new helpers also support the complex cases where the consumer device
> wouldn't always require all the domains. For example, a camera may
> require only one power domain during normal operations but two during
> high resolution operations. The consumer driver can call
> dev_pm_opp_put_genpd_device(high_resolution_genpd_dev) if it is
> currently operating in the normal mode and doesn't have any performance
> requirements from the genpd which manages high resolution power
> requirements. The consumer driver can later call
> dev_pm_opp_set_genpd_device(high_resolution_genpd_dev) once it switches
> back to the high resolution mode.
>
> The new helpers differ from other OPP set/put helpers as the new ones
> can be called with OPPs initialized for the table as we may need to call
> them on the fly because of the complex case explained above. For this
> reason it is possible that the genpd_device structure may be used in

This is a bit unclear. What is really the genpd_device structure?
Could you please clarify that here?

> parallel while the new helpers are running and a new mutex is added to
> protect against that. We didn't use the existing opp_table->lock mutex
> as that is widely used in the OPP core and we will need a lock in the
> hotpath now, i.e.  while changing OPP and we need to make sure there is
> not much contention while doing that.

I not sure this needs to be considered as hotpath. I would be
surprised if changing genpd virtual devices for OPP, is something that
is going to be done frequently. Rather, this is more depending on the
use case, like the camera case you describe above.

In other words, do you really need a new lock?

Other than that, this looks good to me.

Kind regards
Uffe

>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c     | 88 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/opp/of.c       | 16 +++++++-
>  drivers/opp/opp.h      |  4 ++
>  include/linux/pm_opp.h |  8 ++++
>  4 files changed, 115 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 02a69a62dac8..8d7bf9d83752 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -823,6 +823,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>                 return NULL;
>
>         mutex_init(&opp_table->lock);
> +       mutex_init(&opp_table->genpd_dev_lock);
>         INIT_LIST_HEAD(&opp_table->dev_list);
>
>         opp_dev = _add_opp_dev(dev, opp_table);
> @@ -920,6 +921,7 @@ static void _opp_table_kref_release(struct kref *kref)
>                 _remove_opp_dev(opp_dev, opp_table);
>         }
>
> +       mutex_destroy(&opp_table->genpd_dev_lock);
>         mutex_destroy(&opp_table->lock);
>         list_del(&opp_table->node);
>         kfree(opp_table);
> @@ -1602,6 +1604,92 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_set_opp_helper);
>
> +/**
> + * dev_pm_opp_set_genpd_device - Set virtual genpd device for an index
> + * @dev: Consumer device for which the genpd device is getting set.
> + * @genpd_dev: virtual genpd device.
> + * @index: index.
> + *
> + * Multiple generic power domains for a device are supported with the help of
> + * virtual genpd devices, which are created for each consumer device - genpd
> + * pair. These are the device structures which are attached to the power domain
> + * and are required by the OPP core to set the performance state of the genpd.
> + *
> + * This helper will normally be called by the consumer driver of the device
> + * "dev", as only that has details of the genpd devices.
> + *
> + * This helper needs to be called once for each of those virtual devices, but
> + * only if multiple domains are available for a device. Otherwise the original
> + * device structure will be used instead by the OPP core.
> + */
> +struct opp_table *dev_pm_opp_set_genpd_device(struct device *dev,
> +                                             struct device *genpd_device,
> +                                             int index)
> +{
> +       struct opp_table *opp_table;
> +
> +       opp_table = dev_pm_opp_get_opp_table(dev);
> +       if (!opp_table)
> +               return ERR_PTR(-ENOMEM);
> +
> +       mutex_lock(&opp_table->genpd_dev_lock);
> +
> +       if (unlikely(!opp_table->genpd_devices ||
> +                    index >= opp_table->required_opp_count ||
> +                    opp_table->genpd_devices[index])) {
> +
> +               dev_err(dev, "Invalid request to set required device\n");
> +               dev_pm_opp_put_opp_table(opp_table);
> +               mutex_unlock(&opp_table->genpd_dev_lock);
> +
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       opp_table->genpd_devices[index] = genpd_device;
> +       mutex_unlock(&opp_table->genpd_dev_lock);
> +
> +       return opp_table;
> +}
> +
> +/**
> + * dev_pm_opp_put_genpd_device() - Releases resources blocked for genpd device.
> + * @opp_table: OPP table returned by dev_pm_opp_set_genpd_device().
> + * @genpd_device: virtual genpd device.
> + *
> + * This releases the resource previously acquired with a call to
> + * dev_pm_opp_set_genpd_device(). The consumer driver shall call this helper if
> + * it doesn't want OPP core to update performance state of a power domain
> + * anymore.
> + */
> +void dev_pm_opp_put_genpd_device(struct opp_table *opp_table,
> +                                struct device *genpd_device)
> +{
> +       int i;
> +
> +       /*
> +        * Acquire genpd_dev_lock to make sure genpd_device isn't getting used
> +        * in parallel.
> +        */
> +       mutex_lock(&opp_table->genpd_dev_lock);
> +
> +       for (i = 0; i < opp_table->required_opp_count; i++) {
> +               if (opp_table->genpd_devices[i] != genpd_device)
> +                       continue;
> +
> +               opp_table->genpd_devices[i] = NULL;
> +               dev_pm_opp_put_opp_table(opp_table);
> +
> +               /* Drop the vote */
> +               dev_pm_genpd_set_performance_state(genpd_device, 0);
> +               break;
> +       }
> +
> +       mutex_unlock(&opp_table->genpd_dev_lock);
> +
> +       if (unlikely(i == opp_table->required_opp_count))
> +               dev_err(genpd_device, "Failed to find required device entry\n");
> +}
> +
>  /**
>   * dev_pm_opp_add()  - Add an OPP table from a table definitions
>   * @dev:       device for which we do this operation
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index ffaeefef98ce..fd67c9b55d7d 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -134,6 +134,7 @@ static struct opp_table *_find_table_of_opp_np(struct device_node *opp_np)
>  static void _opp_table_free_required_tables(struct opp_table *opp_table)
>  {
>         struct opp_table **required_opp_tables = opp_table->required_opp_tables;
> +       struct device **genpd_devices = opp_table->genpd_devices;
>         int i;
>
>         if (!required_opp_tables)
> @@ -147,8 +148,10 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table)
>         }
>
>         kfree(required_opp_tables);
> +       kfree(genpd_devices);
>
>         opp_table->required_opp_count = 0;
> +       opp_table->genpd_devices = NULL;
>         opp_table->required_opp_tables = NULL;
>  }
>
> @@ -161,6 +164,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>                                              struct device_node *opp_np)
>  {
>         struct opp_table **required_opp_tables;
> +       struct device **genpd_devices = NULL;
>         struct device_node *required_np, *np;
>         int count, i;
>
> @@ -175,11 +179,21 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>         if (!count)
>                 goto put_np;
>
> +       if (count > 1) {
> +               genpd_devices = kcalloc(count, sizeof(*genpd_devices),
> +                                       GFP_KERNEL);
> +               if (!genpd_devices)
> +                       goto put_np;
> +       }
> +
>         required_opp_tables = kcalloc(count, sizeof(*required_opp_tables),
>                                       GFP_KERNEL);
> -       if (!required_opp_tables)
> +       if (!required_opp_tables) {
> +               kfree(genpd_devices);
>                 goto put_np;
> +       }
>
> +       opp_table->genpd_devices = genpd_devices;
>         opp_table->required_opp_tables = required_opp_tables;
>         opp_table->required_opp_count = count;
>
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 24b340ad18d1..aea69a24c26b 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -135,6 +135,8 @@ enum opp_table_access {
>   * @parsed_static_opps: True if OPPs are initialized from DT.
>   * @shared_opp: OPP is shared between multiple devices.
>   * @suspend_opp: Pointer to OPP to be used during device suspend.
> + * @genpd_dev_lock: Mutex protecting the genpd device pointers.
> + * @genpd_devices: List of virtual devices for multiple genpd support.
>   * @required_opp_tables: List of device OPP tables that are required by OPPs in
>   *             this table.
>   * @required_opp_count: Number of required devices.
> @@ -177,6 +179,8 @@ struct opp_table {
>         enum opp_table_access shared_opp;
>         struct dev_pm_opp *suspend_opp;
>
> +       struct mutex genpd_dev_lock;
> +       struct device **genpd_devices;
>         struct opp_table **required_opp_tables;
>         unsigned int required_opp_count;
>
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 5d399eeef172..b14600ce078f 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -126,6 +126,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char * name);
>  void dev_pm_opp_put_clkname(struct opp_table *opp_table);
>  struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
>  void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
> +struct opp_table *dev_pm_opp_set_genpd_device(struct device *dev, struct device *genpd_device, int index);
> +void dev_pm_opp_put_genpd_device(struct opp_table *opp_table, struct device *genpd_device);
>  int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
>  int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
>  int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
> @@ -272,6 +274,12 @@ static inline struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const
>
>  static inline void dev_pm_opp_put_clkname(struct opp_table *opp_table) {}
>
> +static inline struct opp_table *dev_pm_opp_set_genpd_device(struct device *dev, struct device *genpd_device, int index)
> +{
> +       return ERR_PTR(-ENOTSUPP);
> +}
> +
> +static inline void dev_pm_opp_put_genpd_device(struct opp_table *opp_table, struct device *genpd_device) {}
>  static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  {
>         return -ENOTSUPP;
> --
> 2.18.0.rc1.242.g61856ae69a2c
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V2 5/9] PM / Domains: Add genpd_opp_to_performance_state()
  2018-10-12 11:11 ` [PATCH V2 5/9] PM / Domains: Add genpd_opp_to_performance_state() Viresh Kumar
@ 2018-10-12 15:07   ` Ulf Hansson
  2018-10-12 15:40     ` Viresh Kumar
  0 siblings, 1 reply; 18+ messages in thread
From: Ulf Hansson @ 2018-10-12 15:07 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael J. Wysocki, Kevin Hilman, Pavel Machek, Len Brown,
	Linux PM, Vincent Guittot, Stephen Boyd, Nishanth Menon,
	Niklas Cassel, Rajendra Nayak, Linux Kernel Mailing List

On 12 October 2018 at 13:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The OPP core currently stores the performance state in the consumer
> device's OPP table, but that is going to change going forward and
> performance state will rather be set directly in the genpd's OPP table.
>
> For that we need to get the performance state for genpd's device
> structure instead of the consumer device's structure. Add a new helper
> to do that.

I guess what puzzles me a bit here is that we are using a struct
device, while we actually should be talking about an OPP cookie
instead, right?

So the "genpd's device structure" here is not the same as the virtual
devices created by genpd to support multiple PM domains, right? Or is
it?

>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/domain.c | 39 +++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  8 ++++++++
>  2 files changed, 47 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 4b5714199490..2c82194d2a30 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2508,6 +2508,45 @@ int of_genpd_parse_idle_states(struct device_node *dn,
>  }
>  EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>
> +/**
> + * genpd_opp_to_performance_state- Gets performance state of the genpd from its OPP node.

Please rename to:

pm_genpd_opp_to_perfromance_state().

> + *
> + * @genpd_dev: Genpd's device for which the performance-state needs to be found.

Maybe "genpd_dev" is the correct name to use here, as I understand
it's actually the device representing the genpd. However, in other
patches in this series you are also using "genpd_dev", while those
instead corresponds to the virtual created devices by genpd.

I would appreciate if we could make that more clear in the code.

Maybe distinguish them as:

genpd_dev
genpd_virt_dev

or just:

dev
virt_dev

> + * @opp: struct dev_pm_opp of the OPP for which we need to find performance
> + *     state.
> + *
> + * Returns performance state encoded in the OPP of the genpd. This calls
> + * platform specific genpd->opp_to_performance_state() callback to translate
> + * power domain OPP to performance state.
> + *
> + * Returns performance state on success and 0 on failure.
> + */
> +unsigned int genpd_opp_to_performance_state(struct device *genpd_dev,
> +                                           struct dev_pm_opp *opp)
> +{
> +       struct generic_pm_domain *genpd = NULL, *temp;
> +       int state;
> +
> +       lockdep_assert_held(&gpd_list_lock);

What's this?

> +
> +       list_for_each_entry(temp, &gpd_list, gpd_list_node) {
> +               if (&temp->dev == genpd_dev) {
> +                       genpd = temp;
> +                       break;
> +               }
> +       }

I think we can do better than this.

We really don't want to walk the list of genpds while doing this. The
caller should already know (if not now, we should fix it) that the
struct device is used to represent a genpd.

In other words, I am thinking using a container_of() or a finding a
function pointer through the struct device, in any case, it would be
better.

If it all sounds fuzzy, let me think a bit about it and come back with
a more clear suggestion.

> +
> +       if (unlikely(!genpd || !genpd->opp_to_performance_state))
> +               return 0;
> +
> +       genpd_lock(genpd);

.... so in the end, this is the only lock that should be needed.

> +       state = genpd->opp_to_performance_state(genpd, opp);
> +       genpd_unlock(genpd);
> +
> +       return state;
> +}
> +EXPORT_SYMBOL_GPL(genpd_opp_to_performance_state);
> +
>  /**
>   * of_genpd_opp_to_performance_state- Gets performance state of device's
>   * power domain corresponding to a DT node's "required-opps" property.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 776c546d581a..e03f300f7468 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -233,6 +233,8 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent,
>  struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
>  int of_genpd_parse_idle_states(struct device_node *dn,
>                                struct genpd_power_state **states, int *n);
> +unsigned int genpd_opp_to_performance_state(struct device *genpd_dev,
> +                                           struct dev_pm_opp *opp);
>  unsigned int of_genpd_opp_to_performance_state(struct device *dev,
>                                 struct device_node *np);
>
> @@ -274,6 +276,12 @@ static inline int of_genpd_parse_idle_states(struct device_node *dn,
>         return -ENODEV;
>  }
>
> +static inline unsigned int
> +genpd_opp_to_performance_state(struct device *genpd_dev, struct dev_pm_opp *opp)
> +{
> +       return 0;
> +}
> +
>  static inline unsigned int
>  of_genpd_opp_to_performance_state(struct device *dev,
>                                   struct device_node *np)
> --
> 2.18.0.rc1.242.g61856ae69a2c
>

Besides the comments above, I am all in for the approach as such.

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V2 1/9] OPP: Identify and mark genpd OPP tables
  2018-10-12 11:11 ` [PATCH V2 1/9] OPP: Identify and mark genpd OPP tables Viresh Kumar
@ 2018-10-12 15:11   ` Ulf Hansson
  0 siblings, 0 replies; 18+ messages in thread
From: Ulf Hansson @ 2018-10-12 15:11 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, Linux PM,
	Vincent Guittot, Rafael Wysocki, Niklas Cassel, Rajendra Nayak,
	Linux Kernel Mailing List

On 12 October 2018 at 13:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> We need to handle genpd OPP tables differently, this is already the case
> at one location and will be extended going forward. Add another field to
> the OPP table to check if the table belongs to a genpd or not.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/opp/of.c  | 6 ++++--
>  drivers/opp/opp.h | 2 ++
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 5a4b47958073..5f114cd3d88c 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -92,6 +92,9 @@ void _of_init_opp_table(struct opp_table *opp_table, struct device *dev,
>         of_property_read_u32(np, "voltage-tolerance",
>                              &opp_table->voltage_tolerance_v1);
>
> +       if (of_find_property(np, "#power-domain-cells", NULL))
> +               opp_table->is_genpd = true;
> +
>         /* Get OPP table node */
>         opp_np = _opp_of_get_opp_desc_node(np, index);
>         of_node_put(np);
> @@ -326,8 +329,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
>         ret = of_property_read_u64(np, "opp-hz", &rate);
>         if (ret < 0) {
>                 /* "opp-hz" is optional for devices like power domains. */
> -               if (!of_find_property(dev->of_node, "#power-domain-cells",
> -                                     NULL)) {
> +               if (!opp_table->is_genpd) {
>                         dev_err(dev, "%s: opp-hz not found\n", __func__);
>                         goto free_opp;
>                 }
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 9c6544b4f4f9..cdb0c2b095e2 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -140,6 +140,7 @@ enum opp_table_access {
>   * @regulators: Supply regulators
>   * @regulator_count: Number of power supply regulators
>   * @genpd_performance_state: Device's power domain support performance state.
> + * @is_genpd: Marks if the OPP table belongs to a genpd.
>   * @set_opp: Platform specific set_opp callback
>   * @set_opp_data: Data to be passed to set_opp callback
>   * @dentry:    debugfs dentry pointer of the real device directory (not links).
> @@ -178,6 +179,7 @@ struct opp_table {
>         struct regulator **regulators;
>         unsigned int regulator_count;
>         bool genpd_performance_state;
> +       bool is_genpd;
>
>         int (*set_opp)(struct dev_pm_set_opp_data *data);
>         struct dev_pm_set_opp_data *set_opp_data;
> --
> 2.18.0.rc1.242.g61856ae69a2c
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V2 2/9] OPP: Separate out custom OPP handler specific code
  2018-10-12 11:11 ` [PATCH V2 2/9] OPP: Separate out custom OPP handler specific code Viresh Kumar
@ 2018-10-12 15:11   ` Ulf Hansson
  0 siblings, 0 replies; 18+ messages in thread
From: Ulf Hansson @ 2018-10-12 15:11 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, Linux PM,
	Vincent Guittot, Rafael Wysocki, Niklas Cassel, Rajendra Nayak,
	Linux Kernel Mailing List

On 12 October 2018 at 13:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Create a separate routine to take care of custom set_opp() handler
> specific stuff.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/opp/core.c | 67 +++++++++++++++++++++++++++-------------------
>  1 file changed, 40 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 2c2df4e4fc14..ebb3b648e0fd 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -635,6 +635,34 @@ static int _generic_set_opp_regulator(const struct opp_table *opp_table,
>         return ret;
>  }
>
> +static int _set_opp_custom(const struct opp_table *opp_table,
> +                          struct device *dev, unsigned long old_freq,
> +                          unsigned long freq,
> +                          struct dev_pm_opp_supply *old_supply,
> +                          struct dev_pm_opp_supply *new_supply)
> +{
> +       struct dev_pm_set_opp_data *data;
> +       int size;
> +
> +       data = opp_table->set_opp_data;
> +       data->regulators = opp_table->regulators;
> +       data->regulator_count = opp_table->regulator_count;
> +       data->clk = opp_table->clk;
> +       data->dev = dev;
> +
> +       data->old_opp.rate = old_freq;
> +       size = sizeof(*old_supply) * opp_table->regulator_count;
> +       if (IS_ERR(old_supply))
> +               memset(data->old_opp.supplies, 0, size);
> +       else
> +               memcpy(data->old_opp.supplies, old_supply, size);
> +
> +       data->new_opp.rate = freq;
> +       memcpy(data->new_opp.supplies, new_supply, size);
> +
> +       return opp_table->set_opp(data);
> +}
> +
>  /**
>   * dev_pm_opp_set_rate() - Configure new OPP based on frequency
>   * @dev:        device for which we do this operation
> @@ -649,7 +677,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>         unsigned long freq, old_freq;
>         struct dev_pm_opp *old_opp, *opp;
>         struct clk *clk;
> -       int ret, size;
> +       int ret;
>
>         if (unlikely(!target_freq)) {
>                 dev_err(dev, "%s: Invalid target frequency %lu\n", __func__,
> @@ -702,8 +730,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>         dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
>                 old_freq, freq);
>
> -       /* Only frequency scaling */
> -       if (!opp_table->regulators) {
> +       if (opp_table->set_opp) {
> +               ret = _set_opp_custom(opp_table, dev, old_freq, freq,
> +                                     IS_ERR(old_opp) ? NULL : old_opp->supplies,
> +                                     opp->supplies);
> +       } else if (opp_table->regulators) {
> +               ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
> +                                                IS_ERR(old_opp) ? NULL : old_opp->supplies,
> +                                                opp->supplies);
> +       } else {
> +               /* Only frequency scaling */
> +
>                 /*
>                  * We don't support devices with both regulator and
>                  * domain performance-state for now.
> @@ -714,30 +751,6 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>                                                       opp->pstate);
>                 else
>                         ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
> -       } else if (!opp_table->set_opp) {
> -               ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
> -                                                IS_ERR(old_opp) ? NULL : old_opp->supplies,
> -                                                opp->supplies);
> -       } else {
> -               struct dev_pm_set_opp_data *data;
> -
> -               data = opp_table->set_opp_data;
> -               data->regulators = opp_table->regulators;
> -               data->regulator_count = opp_table->regulator_count;
> -               data->clk = clk;
> -               data->dev = dev;
> -
> -               data->old_opp.rate = old_freq;
> -               size = sizeof(*opp->supplies) * opp_table->regulator_count;
> -               if (IS_ERR(old_opp))
> -                       memset(data->old_opp.supplies, 0, size);
> -               else
> -                       memcpy(data->old_opp.supplies, old_opp->supplies, size);
> -
> -               data->new_opp.rate = freq;
> -               memcpy(data->new_opp.supplies, opp->supplies, size);
> -
> -               ret = opp_table->set_opp(data);
>         }
>
>         dev_pm_opp_put(opp);
> --
> 2.18.0.rc1.242.g61856ae69a2c
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V2 3/9] OPP: Populate required opp tables from "required-opps" property
  2018-10-12 11:11 ` [PATCH V2 3/9] OPP: Populate required opp tables from "required-opps" property Viresh Kumar
@ 2018-10-12 15:11   ` Ulf Hansson
  0 siblings, 0 replies; 18+ messages in thread
From: Ulf Hansson @ 2018-10-12 15:11 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, Linux PM,
	Vincent Guittot, Rafael Wysocki, Niklas Cassel, Rajendra Nayak,
	Linux Kernel Mailing List

On 12 October 2018 at 13:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The current implementation works only for the case where a single
> phandle is present in the "required-opps" property, while DT allows
> multiple phandles to be present there.
>
> This patch adds new infrastructure to parse all the phandles present in
> "required-opps" property and save pointers of the required OPP's OPP
> tables. These will be used by later commits.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/opp/core.c |   2 +
>  drivers/opp/of.c   | 147 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/opp/opp.h  |   8 +++
>  3 files changed, 157 insertions(+)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index ebb3b648e0fd..85174a5c4850 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -901,6 +901,8 @@ static void _opp_table_kref_release(struct kref *kref)
>         struct opp_table *opp_table = container_of(kref, struct opp_table, kref);
>         struct opp_device *opp_dev, *temp;
>
> +       _of_clear_opp_table(opp_table);
> +
>         /* Release clk */
>         if (!IS_ERR(opp_table->clk))
>                 clk_put(opp_table->clk);
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 5f114cd3d88c..b5605196122a 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -73,6 +73,147 @@ struct opp_table *_managed_opp(struct device *dev, int index)
>         return managed_table;
>  }
>
> +/* The caller must call dev_pm_opp_put() after the OPP is used */
> +static struct dev_pm_opp *_find_opp_of_np(struct opp_table *opp_table,
> +                                         struct device_node *opp_np)
> +{
> +       struct dev_pm_opp *opp;
> +
> +       lockdep_assert_held(&opp_table_lock);
> +
> +       mutex_lock(&opp_table->lock);
> +
> +       list_for_each_entry(opp, &opp_table->opp_list, node) {
> +               if (opp->np == opp_np) {
> +                       dev_pm_opp_get(opp);
> +                       mutex_unlock(&opp_table->lock);
> +                       return opp;
> +               }
> +       }
> +
> +       mutex_unlock(&opp_table->lock);
> +
> +       return NULL;
> +}
> +
> +static struct device_node *of_parse_required_opp(struct device_node *np,
> +                                                int index)
> +{
> +       struct device_node *required_np;
> +
> +       required_np = of_parse_phandle(np, "required-opps", index);
> +       if (unlikely(!required_np)) {
> +               pr_err("%s: Unable to parse required-opps: %pOF, index: %d\n",
> +                      __func__, np, index);
> +       }
> +
> +       return required_np;
> +}
> +
> +/* The caller must call dev_pm_opp_put_opp_table() after the table is used */
> +static struct opp_table *_find_table_of_opp_np(struct device_node *opp_np)
> +{
> +       struct opp_table *opp_table;
> +       struct dev_pm_opp *opp;
> +
> +       lockdep_assert_held(&opp_table_lock);
> +
> +       list_for_each_entry(opp_table, &opp_tables, node) {
> +               opp = _find_opp_of_np(opp_table, opp_np);
> +               if (opp) {
> +                       dev_pm_opp_put(opp);
> +                       _get_opp_table_kref(opp_table);
> +                       return opp_table;
> +               }
> +       }
> +
> +       return ERR_PTR(-ENODEV);
> +}
> +
> +/* Free resources previously acquired by _opp_table_alloc_required_tables() */
> +static void _opp_table_free_required_tables(struct opp_table *opp_table)
> +{
> +       struct opp_table **required_opp_tables = opp_table->required_opp_tables;
> +       int i;
> +
> +       if (!required_opp_tables)
> +               return;
> +
> +       for (i = 0; i < opp_table->required_opp_count; i++) {
> +               if (IS_ERR_OR_NULL(required_opp_tables[i]))
> +                       break;
> +
> +               dev_pm_opp_put_opp_table(required_opp_tables[i]);
> +       }
> +
> +       kfree(required_opp_tables);
> +
> +       opp_table->required_opp_count = 0;
> +       opp_table->required_opp_tables = NULL;
> +}
> +
> +/*
> + * Populate all devices and opp tables which are part of "required-opps" list.
> + * Checking only the first OPP node should be enough.
> + */
> +static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> +                                            struct device *dev,
> +                                            struct device_node *opp_np)
> +{
> +       struct opp_table **required_opp_tables;
> +       struct device_node *required_np, *np;
> +       int count, i;
> +
> +       /* Traversing the first OPP node is all we need */
> +       np = of_get_next_available_child(opp_np, NULL);
> +       if (!np) {
> +               dev_err(dev, "Empty OPP table\n");
> +               return;
> +       }
> +
> +       count = of_count_phandle_with_args(np, "required-opps", NULL);
> +       if (!count)
> +               goto put_np;
> +
> +       required_opp_tables = kcalloc(count, sizeof(*required_opp_tables),
> +                                     GFP_KERNEL);
> +       if (!required_opp_tables)
> +               goto put_np;
> +
> +       opp_table->required_opp_tables = required_opp_tables;
> +       opp_table->required_opp_count = count;
> +
> +       for (i = 0; i < count; i++) {
> +               required_np = of_parse_required_opp(np, i);
> +               if (!required_np)
> +                       goto free_required_tables;
> +
> +               required_opp_tables[i] = _find_table_of_opp_np(required_np);
> +               of_node_put(required_np);
> +
> +               if (IS_ERR(required_opp_tables[i]))
> +                       goto free_required_tables;
> +
> +               /*
> +                * We only support genpd's OPPs in the "required-opps" for now,
> +                * as we don't know how much about other cases. Error out if the
> +                * required OPP doesn't belong to a genpd.
> +                */
> +               if (!required_opp_tables[i]->is_genpd) {
> +                       dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
> +                               required_np);
> +                       goto free_required_tables;
> +               }
> +       }
> +
> +       goto put_np;
> +
> +free_required_tables:
> +       _opp_table_free_required_tables(opp_table);
> +put_np:
> +       of_node_put(np);
> +}
> +
>  void _of_init_opp_table(struct opp_table *opp_table, struct device *dev,
>                         int index)
>  {
> @@ -109,9 +250,15 @@ void _of_init_opp_table(struct opp_table *opp_table, struct device *dev,
>
>         opp_table->np = opp_np;
>
> +       _opp_table_alloc_required_tables(opp_table, dev, opp_np);
>         of_node_put(opp_np);
>  }
>
> +void _of_clear_opp_table(struct opp_table *opp_table)
> +{
> +       _opp_table_free_required_tables(opp_table);
> +}
> +
>  static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
>                               struct device_node *np)
>  {
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index cdb0c2b095e2..024e1be23d37 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -133,6 +133,9 @@ enum opp_table_access {
>   * @parsed_static_opps: True if OPPs are initialized from DT.
>   * @shared_opp: OPP is shared between multiple devices.
>   * @suspend_opp: Pointer to OPP to be used during device suspend.
> + * @required_opp_tables: List of device OPP tables that are required by OPPs in
> + *             this table.
> + * @required_opp_count: Number of required devices.
>   * @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.
> @@ -172,6 +175,9 @@ struct opp_table {
>         enum opp_table_access shared_opp;
>         struct dev_pm_opp *suspend_opp;
>
> +       struct opp_table **required_opp_tables;
> +       unsigned int required_opp_count;
> +
>         unsigned int *supported_hw;
>         unsigned int supported_hw_count;
>         const char *prop_name;
> @@ -208,9 +214,11 @@ void _put_opp_list_kref(struct opp_table *opp_table);
>
>  #ifdef CONFIG_OF
>  void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index);
> +void _of_clear_opp_table(struct opp_table *opp_table);
>  struct opp_table *_managed_opp(struct device *dev, int index);
>  #else
>  static inline void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index) {}
> +static inline void _of_clear_opp_table(struct opp_table *opp_table) {}
>  static inline struct opp_table *_managed_opp(struct device *dev, int index) { return NULL; }
>  #endif
>
> --
> 2.18.0.rc1.242.g61856ae69a2c
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V2 4/9] OPP: Populate OPPs from "required-opps" property
  2018-10-12 11:11 ` [PATCH V2 4/9] OPP: Populate OPPs " Viresh Kumar
@ 2018-10-12 15:11   ` Ulf Hansson
  0 siblings, 0 replies; 18+ messages in thread
From: Ulf Hansson @ 2018-10-12 15:11 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, Linux PM,
	Vincent Guittot, Rafael Wysocki, Niklas Cassel, Rajendra Nayak,
	Linux Kernel Mailing List

On 12 October 2018 at 13:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> An earlier commit populated the OPP tables from the "required-opps"
> property, this commit populates the individual OPPs. This is repeated
> for each OPP in the OPP table and these populated OPPs will be used by
> later commits.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/opp/core.c |  1 +
>  drivers/opp/of.c   | 81 ++++++++++++++++++++++++++++++++++++++++++++--
>  drivers/opp/opp.h  |  6 ++++
>  3 files changed, 86 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 85174a5c4850..02a69a62dac8 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -976,6 +976,7 @@ static void _opp_kref_release(struct kref *kref)
>          * frequency/voltage list.
>          */
>         blocking_notifier_call_chain(&opp_table->head, OPP_EVENT_REMOVE, opp);
> +       _of_opp_free_required_opps(opp_table, opp);
>         opp_debug_remove_one(opp);
>         list_del(&opp->node);
>         kfree(opp);
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index b5605196122a..ffaeefef98ce 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -259,6 +259,77 @@ void _of_clear_opp_table(struct opp_table *opp_table)
>         _opp_table_free_required_tables(opp_table);
>  }
>
> +/*
> + * Release all resources previously acquired with a call to
> + * _of_opp_alloc_required_opps().
> + */
> +void _of_opp_free_required_opps(struct opp_table *opp_table,
> +                               struct dev_pm_opp *opp)
> +{
> +       struct dev_pm_opp **required_opps = opp->required_opps;
> +       int i;
> +
> +       if (!required_opps)
> +               return;
> +
> +       for (i = 0; i < opp_table->required_opp_count; i++) {
> +               if (!required_opps[i])
> +                       break;
> +
> +               /* Put the reference back */
> +               dev_pm_opp_put(required_opps[i]);
> +       }
> +
> +       kfree(required_opps);
> +       opp->required_opps = NULL;
> +}
> +
> +/* Populate all required OPPs which are part of "required-opps" list */
> +static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
> +                                      struct dev_pm_opp *opp)
> +{
> +       struct dev_pm_opp **required_opps;
> +       struct opp_table *required_table;
> +       struct device_node *np;
> +       int i, ret, count = opp_table->required_opp_count;
> +
> +       if (!count)
> +               return 0;
> +
> +       required_opps = kcalloc(count, sizeof(*required_opps), GFP_KERNEL);
> +       if (!required_opps)
> +               return -ENOMEM;
> +
> +       opp->required_opps = required_opps;
> +
> +       for (i = 0; i < count; i++) {
> +               required_table = opp_table->required_opp_tables[i];
> +
> +               np = of_parse_required_opp(opp->np, i);
> +               if (unlikely(!np)) {
> +                       ret = -ENODEV;
> +                       goto free_required_opps;
> +               }
> +
> +               required_opps[i] = _find_opp_of_np(required_table, np);
> +               of_node_put(np);
> +
> +               if (!required_opps[i]) {
> +                       pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
> +                              __func__, opp->np, i);
> +                       ret = -ENODEV;
> +                       goto free_required_opps;
> +               }
> +       }
> +
> +       return 0;
> +
> +free_required_opps:
> +       _of_opp_free_required_opps(opp_table, opp);
> +
> +       return ret;
> +}
> +
>  static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
>                               struct device_node *np)
>  {
> @@ -503,6 +574,10 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
>         new_opp->dynamic = false;
>         new_opp->available = true;
>
> +       ret = _of_opp_alloc_required_opps(opp_table, new_opp);
> +       if (ret)
> +               goto free_opp;
> +
>         if (!of_property_read_u32(np, "clock-latency-ns", &val))
>                 new_opp->clock_latency_ns = val;
>
> @@ -510,14 +585,14 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
>
>         ret = opp_parse_supplies(new_opp, dev, opp_table);
>         if (ret)
> -               goto free_opp;
> +               goto free_required_opps;
>
>         ret = _opp_add(dev, new_opp, opp_table, rate_not_available);
>         if (ret) {
>                 /* Don't return error for duplicate OPPs */
>                 if (ret == -EBUSY)
>                         ret = 0;
> -               goto free_opp;
> +               goto free_required_opps;
>         }
>
>         /* OPP to select on device suspend */
> @@ -547,6 +622,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
>         blocking_notifier_call_chain(&opp_table->head, OPP_EVENT_ADD, new_opp);
>         return new_opp;
>
> +free_required_opps:
> +       _of_opp_free_required_opps(opp_table, new_opp);
>  free_opp:
>         _opp_free(new_opp);
>
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 024e1be23d37..24b340ad18d1 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -63,6 +63,7 @@ extern struct list_head opp_tables;
>   * @supplies:  Power supplies voltage/current values
>   * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
>   *             frequency from any other OPP's frequency.
> + * @required_opps: List of OPPs that are required by this OPP.
>   * @opp_table: points back to the opp_table struct this opp belongs to
>   * @np:                OPP's device node.
>   * @dentry:    debugfs dentry pointer (per opp)
> @@ -84,6 +85,7 @@ struct dev_pm_opp {
>
>         unsigned long clock_latency_ns;
>
> +       struct dev_pm_opp **required_opps;
>         struct opp_table *opp_table;
>
>         struct device_node *np;
> @@ -216,10 +218,14 @@ void _put_opp_list_kref(struct opp_table *opp_table);
>  void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index);
>  void _of_clear_opp_table(struct opp_table *opp_table);
>  struct opp_table *_managed_opp(struct device *dev, int index);
> +void _of_opp_free_required_opps(struct opp_table *opp_table,
> +                               struct dev_pm_opp *opp);
>  #else
>  static inline void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index) {}
>  static inline void _of_clear_opp_table(struct opp_table *opp_table) {}
>  static inline struct opp_table *_managed_opp(struct device *dev, int index) { return NULL; }
> +static inline void _of_opp_free_required_opps(struct opp_table *opp_table,
> +                                             struct dev_pm_opp *opp) {}
>  #endif
>
>  #ifdef CONFIG_DEBUG_FS
> --
> 2.18.0.rc1.242.g61856ae69a2c
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V2 5/9] PM / Domains: Add genpd_opp_to_performance_state()
  2018-10-12 15:07   ` Ulf Hansson
@ 2018-10-12 15:40     ` Viresh Kumar
  0 siblings, 0 replies; 18+ messages in thread
From: Viresh Kumar @ 2018-10-12 15:40 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Rafael J. Wysocki, Kevin Hilman, Pavel Machek, Len Brown,
	Linux PM, Vincent Guittot, Stephen Boyd, Nishanth Menon,
	Niklas Cassel, Rajendra Nayak, Linux Kernel Mailing List

On 12 October 2018 at 20:37, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 12 October 2018 at 13:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> The OPP core currently stores the performance state in the consumer
>> device's OPP table, but that is going to change going forward and
>> performance state will rather be set directly in the genpd's OPP table.
>>
>> For that we need to get the performance state for genpd's device
>> structure instead of the consumer device's structure. Add a new helper
>> to do that.
>
> I guess what puzzles me a bit here is that we are using a struct
> device, while we actually should be talking about an OPP cookie
> instead, right?

The OPP cookie wouldn't get us to the platform specific conversion
handler, for that we need something from the genpd itself and so its
structure.

> So the "genpd's device structure" here is not the same as the virtual
> devices created by genpd to support multiple PM domains, right? Or is
> it?

You already found that I believe, it is genpd->dev.

>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>  drivers/base/power/domain.c | 39 +++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |  8 ++++++++
>>  2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 4b5714199490..2c82194d2a30 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -2508,6 +2508,45 @@ int of_genpd_parse_idle_states(struct device_node *dn,
>>  }
>>  EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>>
>> +/**
>> + * genpd_opp_to_performance_state- Gets performance state of the genpd from its OPP node.
>
> Please rename to:
>
> pm_genpd_opp_to_perfromance_state().

Ok.

>> + *
>> + * @genpd_dev: Genpd's device for which the performance-state needs to be found.
>
> Maybe "genpd_dev" is the correct name to use here, as I understand
> it's actually the device representing the genpd. However, in other
> patches in this series you are also using "genpd_dev", while those
> instead corresponds to the virtual created devices by genpd.

Naming is a mess because I tried to follow the names you followed in
your multiple domain support. You used genpd_dev for the virtual device :)

> I would appreciate if we could make that more clear in the code.
>
> Maybe distinguish them as:
>
> genpd_dev
> genpd_virt_dev
> or just:
>
> dev
> virt_dev

Maybe, perhaps we should change domain.c with same naming for the internal
coding handling multiple domains as well ? I will send the patch for
that if you agree.

>> + * @opp: struct dev_pm_opp of the OPP for which we need to find performance
>> + *     state.
>> + *
>> + * Returns performance state encoded in the OPP of the genpd. This calls
>> + * platform specific genpd->opp_to_performance_state() callback to translate
>> + * power domain OPP to performance state.
>> + *
>> + * Returns performance state on success and 0 on failure.
>> + */
>> +unsigned int genpd_opp_to_performance_state(struct device *genpd_dev,
>> +                                           struct dev_pm_opp *opp)
>> +{
>> +       struct generic_pm_domain *genpd = NULL, *temp;
>> +       int state;
>> +
>> +       lockdep_assert_held(&gpd_list_lock);
>
> What's this?

Don't we need to protect with a lock while traversing the below list?
Above is just a check to make sure lock is taken.

>> +
>> +       list_for_each_entry(temp, &gpd_list, gpd_list_node) {
>> +               if (&temp->dev == genpd_dev) {
>> +                       genpd = temp;
>> +                       break;
>> +               }
>> +       }
>
> I think we can do better than this.

I really want to :)

> We really don't want to walk the list of genpds while doing this. The
> caller should already know (if not now, we should fix it) that the
> struct device is used to represent a genpd.

Caller knows that genpd_dev here is genpd->dev really. But it doesn't
have pointer of the genpd itself.

> In other words, I am thinking using a container_of() or a finding a
> function pointer through the struct device, in any case, it would be
> better.

I am stupid. Container-of will work just fine I belive.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V2 6/9] OPP: Add dev_pm_opp_{set|put}_genpd_device() helper
  2018-10-12 14:46   ` Ulf Hansson
@ 2018-10-12 15:43     ` Viresh Kumar
  0 siblings, 0 replies; 18+ messages in thread
From: Viresh Kumar @ 2018-10-12 15:43 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
	Linux PM, Vincent Guittot, Niklas Cassel, Rajendra Nayak,
	Linux Kernel Mailing List

On 12 October 2018 at 20:16, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 12 October 2018 at 13:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> Multiple generic power domains for a consumer device are supported with
>> the help of virtual devices, which are created for each consumer device
>> - genpd pair. These are the device structures which are attached to the
>> power domain and are required by the OPP core to set the performance
>> state of the genpd.
>>
>> The helpers added by this commit are required to be called once for each
>> of these virtual devices. These are required only if multiple domains
>> are available for a device, otherwise the actual device structure will
>> be used instead by the OPP core.
>>
>> The new helpers also support the complex cases where the consumer device
>> wouldn't always require all the domains. For example, a camera may
>> require only one power domain during normal operations but two during
>> high resolution operations. The consumer driver can call
>> dev_pm_opp_put_genpd_device(high_resolution_genpd_dev) if it is
>> currently operating in the normal mode and doesn't have any performance
>> requirements from the genpd which manages high resolution power
>> requirements. The consumer driver can later call
>> dev_pm_opp_set_genpd_device(high_resolution_genpd_dev) once it switches
>> back to the high resolution mode.
>>
>> The new helpers differ from other OPP set/put helpers as the new ones
>> can be called with OPPs initialized for the table as we may need to call
>> them on the fly because of the complex case explained above. For this
>> reason it is possible that the genpd_device structure may be used in
>
> This is a bit unclear. What is really the genpd_device structure?
> Could you please clarify that here?

It is the virtual device structures created by genpd.

>> parallel while the new helpers are running and a new mutex is added to
>> protect against that. We didn't use the existing opp_table->lock mutex
>> as that is widely used in the OPP core and we will need a lock in the
>> hotpath now, i.e.  while changing OPP and we need to make sure there is
>> not much contention while doing that.
>
> I not sure this needs to be considered as hotpath. I would be
> surprised if changing genpd virtual devices for OPP, is something that
> is going to be done frequently. Rather, this is more depending on the
> use case, like the camera case you describe above.
>
> In other words, do you really need a new lock?

My bad. I didn't explain it well. If you see a later patch where we started
configuring the required OPPs, we use genpd_dev or virt-dev within
opp_set_rate() which also needs this lock. And that is the hot path.

--
viresh

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2018-10-12 15:43 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-12 11:11 [PATCH V2 0/9] OPP: Support multiple power-domains per device Viresh Kumar
2018-10-12 11:11 ` [PATCH V2 1/9] OPP: Identify and mark genpd OPP tables Viresh Kumar
2018-10-12 15:11   ` Ulf Hansson
2018-10-12 11:11 ` [PATCH V2 2/9] OPP: Separate out custom OPP handler specific code Viresh Kumar
2018-10-12 15:11   ` Ulf Hansson
2018-10-12 11:11 ` [PATCH V2 3/9] OPP: Populate required opp tables from "required-opps" property Viresh Kumar
2018-10-12 15:11   ` Ulf Hansson
2018-10-12 11:11 ` [PATCH V2 4/9] OPP: Populate OPPs " Viresh Kumar
2018-10-12 15:11   ` Ulf Hansson
2018-10-12 11:11 ` [PATCH V2 5/9] PM / Domains: Add genpd_opp_to_performance_state() Viresh Kumar
2018-10-12 15:07   ` Ulf Hansson
2018-10-12 15:40     ` Viresh Kumar
2018-10-12 11:11 ` [PATCH V2 6/9] OPP: Add dev_pm_opp_{set|put}_genpd_device() helper Viresh Kumar
2018-10-12 14:46   ` Ulf Hansson
2018-10-12 15:43     ` Viresh Kumar
2018-10-12 11:11 ` [PATCH V2 7/9] OPP: Configure all required OPPs Viresh Kumar
2018-10-12 11:11 ` [PATCH V2 8/9] OPP: Rename and relocate of_genpd_opp_to_performance_state() Viresh Kumar
2018-10-12 11:11 ` [PATCH V2 9/9] OPP: Remove of_dev_pm_opp_find_required_opp() Viresh Kumar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).