[PATCH V3 0/6] PM / Domains: Allow performance state propagation

[PATCH V3 0/6] PM / Domains: Allow performance state propagation

[Viresh Kumar]

Hi,

This series adds performance state propagation support in genpd core.
The propagation happens from the sub-domains to their masters. More
details can be found in the individual commit logs.

This is tested on hikey960 by faking power domains in such a way that
the CPU devices have two power domains and both of them have the same
master domain. The CPU device, as well as its power domains have
"required-opps" property set and the performance requirement from the
CPU eventually configures all the domains (2 sub-domains and 1 master).

Based on opp/linux-next branch (which is 4.20-rc1 +
multiple-power-domain-support-in-opp-core + some OPP fixes).

Rajendra has already tested the previous version of this series and so I
have included his Tested-by for all patches.

V2->V3:
- Include Ulf's patch (sent separately earlier) with this series.
- The performance state update code doesn't rely anymore on the power
  on/off state of the genpd, it sets and propagates rate in all cases.
- That simplified a lot of code from V2 in _genpd_power_on().
- commit logs improved for few commits.
- s/mstate/master_state/
- and few more minor changes.

v1->V2:
- First patch (1/5) is new and an improvement to earlier stuff.
- Move genpd_status_on() check to _genpd_reeval_performance_state() from
  _genpd_set_performance_state().
- Improve dev_pm_opp_xlate_performance_state() to handle 1:1 pstate
  mapping between genpd and its master and also to fix a problem while
  finding the dst_table.
- Handle pstate=0 case properly.

--
viresh

Ulf Hansson (1):
  PM / Domains: Make genpd performance states orthogonal to the
    idlestates

Viresh Kumar (5):
  OPP: Improve _find_table_of_opp_np()
  OPP: Add dev_pm_opp_xlate_performance_state() helper
  PM / Domains: Save OPP table pointer in genpd
  PM / Domains: Factorize dev_pm_genpd_set_performance_state()
  PM / Domains: Propagate performance state updates

 drivers/base/power/domain.c | 207 ++++++++++++++++++++++++++----------
 drivers/opp/core.c          |  59 ++++++++++
 drivers/opp/of.c            |  14 ++-
 include/linux/pm_domain.h   |   6 ++
 include/linux/pm_opp.h      |   7 ++
 5 files changed, 235 insertions(+), 58 deletions(-)

-- 
2.19.1.568.g152ad8e3369a

[PATCH V3 1/6] PM / Domains: Make genpd performance states orthogonal to the idlestates

[Viresh Kumar]

From: Ulf Hansson <ulf.hansson@linaro.org>

It's quite questionable whether genpd internally should care about if the
corresponding PM domain for a device is powered on, as to allow setting a
new performance state for it. The assumptions creates an unnecessary
limitation at this point, for both consumers and providers, but more
importantly it also makes the code more complicated.

Therefore, let's simplify the code to allow setting a performance state, by
invoking the ->set_performance_state() callback, no matter whether the PM
domain is powered on or off.

Do note, this change means genpd providers needs to restore the performance
state themselves during power on, via the ->power_on() callback. Moreover,
they may also need to check that the PM domain is powered on, from their
->set_performance_state() callback, before deciding to update the state.

Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/domain.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 8e554e6a82a2..4a4e39d12354 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -311,12 +311,10 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 	 */
 
 update_state:
-	if (genpd_status_on(genpd)) {
-		ret = genpd->set_performance_state(genpd, state);
-		if (ret) {
-			gpd_data->performance_state = prev;
-			goto unlock;
-		}
+	ret = genpd->set_performance_state(genpd, state);
+	if (ret) {
+		gpd_data->performance_state = prev;
+		goto unlock;
 	}
 
 	genpd->performance_state = state;
@@ -347,15 +345,6 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 		return ret;
 
 	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
-
-	if (unlikely(genpd->set_performance_state)) {
-		ret = genpd->set_performance_state(genpd, genpd->performance_state);
-		if (ret) {
-			pr_warn("%s: Failed to set performance state %d (%d)\n",
-				genpd->name, genpd->performance_state, ret);
-		}
-	}
-
 	if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
 		return ret;
 
-- 
2.19.1.568.g152ad8e3369a

[PATCH V3 2/6] OPP: Improve _find_table_of_opp_np()

[Viresh Kumar]

Make _find_table_of_opp_np() more efficient by using of_get_parent() to
find the parent OPP table node.

Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/of.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 3ef7f38c0986..8e57d257be77 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -114,19 +114,25 @@ static struct device_node *of_parse_required_opp(struct device_node *np,
 static struct opp_table *_find_table_of_opp_np(struct device_node *opp_np)
 {
 	struct opp_table *opp_table;
-	struct dev_pm_opp *opp;
+	struct device_node *opp_table_np;
 
 	lockdep_assert_held(&opp_table_lock);
 
+	opp_table_np = of_get_parent(opp_np);
+	if (!opp_table_np)
+		goto err;
+
+	/* It is safe to put the node now as all we need now is its address */
+	of_node_put(opp_table_np);
+
 	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);
+		if (opp_table_np == opp_table->np) {
 			_get_opp_table_kref(opp_table);
 			return opp_table;
 		}
 	}
 
+err:
 	return ERR_PTR(-ENODEV);
 }
 
-- 
2.19.1.568.g152ad8e3369a

[PATCH V3 3/6] OPP: Add dev_pm_opp_xlate_performance_state() helper

[Viresh Kumar]

dev_pm_genpd_set_performance_state() needs to handle performance state
propagation going forward. Currently this routine only gets the required
performance state of the device's genpd as an argument, but it doesn't
know how to translate that to master genpd(s) of the device's genpd.

Introduce a new helper dev_pm_opp_xlate_performance_state() which will
be used to translate from performance state of a device (or genpd
sub-domain) to another device (or master genpd).

Normally the src_table (of genpd sub-domain) will have the
"required_opps" property set to point to one of the OPPs in the
dst_table (of master genpd), but in some cases the genpd and its master
have one to one mapping of performance states and so none of them have
the "required-opps" property set. Return the performance state of the
src_table as it is in such cases.

Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 59 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  7 +++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 98e60f0ed8b0..386095e2f4f7 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1713,6 +1713,65 @@ void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table,
 		dev_err(virt_dev, "Failed to find required device entry\n");
 }
 
+/**
+ * dev_pm_opp_xlate_performance_state() - Find required OPP's pstate for src_table.
+ * @src_table: OPP table which has dst_table as one of its required OPP table.
+ * @dst_table: Required OPP table of the src_table.
+ * @pstate: Current performance state of the src_table.
+ *
+ * This Returns pstate of the OPP (present in @dst_table) pointed out by the
+ * "required-opps" property of the OPP (present in @src_table) which has
+ * performance state set to @pstate.
+ *
+ * Return: Positive performance state on success, otherwise 0 on errors.
+ */
+unsigned int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
+						struct opp_table *dst_table,
+						unsigned int pstate)
+{
+	struct dev_pm_opp *opp;
+	unsigned int dest_pstate = 0;
+	int i;
+
+	/*
+	 * Normally the src_table will have the "required_opps" property set to
+	 * point to one of the OPPs in the dst_table, but in some cases the
+	 * genpd and its master have one to one mapping of performance states
+	 * and so none of them have the "required-opps" property set. Return the
+	 * pstate of the src_table as it is in such cases.
+	 */
+	if (!src_table->required_opp_count)
+		return pstate;
+
+	for (i = 0; i < src_table->required_opp_count; i++) {
+		if (src_table->required_opp_tables[i]->np == dst_table->np)
+			break;
+	}
+
+	if (unlikely(i == src_table->required_opp_count)) {
+		pr_err("%s: Couldn't find matching OPP table (%p: %p)\n",
+		       __func__, src_table, dst_table);
+		return 0;
+	}
+
+	mutex_lock(&src_table->lock);
+
+	list_for_each_entry(opp, &src_table->opp_list, node) {
+		if (opp->pstate == pstate) {
+			dest_pstate = opp->required_opps[i]->pstate;
+			goto unlock;
+		}
+	}
+
+	pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, src_table,
+	       dst_table);
+
+unlock:
+	mutex_unlock(&src_table->lock);
+
+	return dest_pstate;
+}
+
 /**
  * dev_pm_opp_add()  - Add an OPP table from a table definitions
  * @dev:	device for which we do this operation
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 2b2c3fd985ab..5a64a81a1789 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -128,6 +128,7 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*s
 void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
 struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev, struct device *virt_dev, int index);
 void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table, struct device *virt_dev);
+unsigned int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
 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);
@@ -280,6 +281,12 @@ static inline struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev
 }
 
 static inline void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table, struct device *virt_dev) {}
+
+static inline unsigned int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate)
+{
+	return 0;
+}
+
 static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 {
 	return -ENOTSUPP;
-- 
2.19.1.568.g152ad8e3369a

[PATCH V3 4/6] PM / Domains: Save OPP table pointer in genpd

[Viresh Kumar]

dev_pm_genpd_set_performance_state() will be required to call
dev_pm_opp_xlate_performance_state() going forward to translate from
performance state of a sub-domain to performance state of its master.
And dev_pm_opp_xlate_performance_state() needs pointers to the OPP
tables of both genpd and its master.

Lets fetch and save them while the OPP tables are added. Fetching the
OPP tables should never fail as we just added the OPP tables and so add
a WARN_ON() for such a bug instead of full error paths.

Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/domain.c | 23 +++++++++++++++++++++--
 include/linux/pm_domain.h   |  2 ++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 4a4e39d12354..1e98c637e069 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1896,12 +1896,21 @@ int of_genpd_add_provider_simple(struct device_node *np,
 				ret);
 			goto unlock;
 		}
+
+		/*
+		 * Save table for faster processing while setting performance
+		 * state.
+		 */
+		genpd->opp_table = dev_pm_opp_get_opp_table(&genpd->dev);
+		WARN_ON(!genpd->opp_table);
 	}
 
 	ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
 	if (ret) {
-		if (genpd->set_performance_state)
+		if (genpd->set_performance_state) {
+			dev_pm_opp_put_opp_table(genpd->opp_table);
 			dev_pm_opp_of_remove_table(&genpd->dev);
+		}
 
 		goto unlock;
 	}
@@ -1954,6 +1963,13 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 					i, ret);
 				goto error;
 			}
+
+			/*
+			 * Save table for faster processing while setting
+			 * performance state.
+			 */
+			genpd->opp_table = dev_pm_opp_get_opp_table_indexed(&genpd->dev, i);
+			WARN_ON(!genpd->opp_table);
 		}
 
 		genpd->provider = &np->fwnode;
@@ -1978,8 +1994,10 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 		genpd->provider = NULL;
 		genpd->has_provider = false;
 
-		if (genpd->set_performance_state)
+		if (genpd->set_performance_state) {
+			dev_pm_opp_put_opp_table(genpd->opp_table);
 			dev_pm_opp_of_remove_table(&genpd->dev);
+		}
 	}
 
 	mutex_unlock(&gpd_list_lock);
@@ -2013,6 +2031,7 @@ void of_genpd_del_provider(struct device_node *np)
 					if (!gpd->set_performance_state)
 						continue;
 
+					dev_pm_opp_put_opp_table(gpd->opp_table);
 					dev_pm_opp_of_remove_table(&gpd->dev);
 				}
 			}
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 642036952553..9ad101362aef 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -73,6 +73,7 @@ struct genpd_power_state {
 
 struct genpd_lock_ops;
 struct dev_pm_opp;
+struct opp_table;
 
 struct generic_pm_domain {
 	struct device dev;
@@ -94,6 +95,7 @@ struct generic_pm_domain {
 	unsigned int performance_state;	/* Aggregated max performance state */
 	int (*power_off)(struct generic_pm_domain *domain);
 	int (*power_on)(struct generic_pm_domain *domain);
+	struct opp_table *opp_table;	/* OPP table of the genpd */
 	unsigned int (*opp_to_performance_state)(struct generic_pm_domain *genpd,
 						 struct dev_pm_opp *opp);
 	int (*set_performance_state)(struct generic_pm_domain *genpd,
-- 
2.19.1.568.g152ad8e3369a

[PATCH V3 5/6] PM / Domains: Factorize dev_pm_genpd_set_performance_state()

[Viresh Kumar]

Separate out _genpd_set_performance_state() and
_genpd_reeval_performance_state() from
dev_pm_genpd_set_performance_state() to handle performance state update
related stuff. This will be used by a later commit.

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/domain.c | 96 +++++++++++++++++++++----------------
 1 file changed, 56 insertions(+), 40 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 1e98c637e069..32ecbefbd191 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -239,6 +239,58 @@ static void genpd_update_accounting(struct generic_pm_domain *genpd)
 static inline void genpd_update_accounting(struct generic_pm_domain *genpd) {}
 #endif
 
+static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
+					unsigned int state)
+{
+	int ret;
+
+	ret = genpd->set_performance_state(genpd, state);
+	if (ret)
+		return ret;
+
+	genpd->performance_state = state;
+	return 0;
+}
+
+static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd,
+					   unsigned int state)
+{
+	struct generic_pm_domain_data *pd_data;
+	struct pm_domain_data *pdd;
+
+	/* New requested state is same as Max requested state */
+	if (state == genpd->performance_state)
+		return 0;
+
+	/* New requested state is higher than Max requested state */
+	if (state > genpd->performance_state)
+		goto update_state;
+
+	/* Traverse all devices within the domain */
+	list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+		pd_data = to_gpd_data(pdd);
+
+		if (pd_data->performance_state > state)
+			state = pd_data->performance_state;
+	}
+
+	if (state == genpd->performance_state)
+		return 0;
+
+	/*
+	 * We aren't propagating performance state changes of a subdomain to its
+	 * masters as we don't have hardware that needs it. Over that, the
+	 * performance states of subdomain and its masters may not have
+	 * one-to-one mapping and would require additional information. We can
+	 * get back to this once we have hardware that needs it. For that
+	 * reason, we don't have to consider performance state of the subdomains
+	 * of genpd here.
+	 */
+
+update_state:
+	return _genpd_set_performance_state(genpd, state);
+}
+
 /**
  * dev_pm_genpd_set_performance_state- Set performance state of device's power
  * domain.
@@ -257,10 +309,9 @@ static inline void genpd_update_accounting(struct generic_pm_domain *genpd) {}
 int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 {
 	struct generic_pm_domain *genpd;
-	struct generic_pm_domain_data *gpd_data, *pd_data;
-	struct pm_domain_data *pdd;
+	struct generic_pm_domain_data *gpd_data;
 	unsigned int prev;
-	int ret = 0;
+	int ret;
 
 	genpd = dev_to_genpd(dev);
 	if (IS_ERR(genpd))
@@ -281,45 +332,10 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 	prev = gpd_data->performance_state;
 	gpd_data->performance_state = state;
 
-	/* New requested state is same as Max requested state */
-	if (state == genpd->performance_state)
-		goto unlock;
-
-	/* New requested state is higher than Max requested state */
-	if (state > genpd->performance_state)
-		goto update_state;
-
-	/* Traverse all devices within the domain */
-	list_for_each_entry(pdd, &genpd->dev_list, list_node) {
-		pd_data = to_gpd_data(pdd);
-
-		if (pd_data->performance_state > state)
-			state = pd_data->performance_state;
-	}
-
-	if (state == genpd->performance_state)
-		goto unlock;
-
-	/*
-	 * We aren't propagating performance state changes of a subdomain to its
-	 * masters as we don't have hardware that needs it. Over that, the
-	 * performance states of subdomain and its masters may not have
-	 * one-to-one mapping and would require additional information. We can
-	 * get back to this once we have hardware that needs it. For that
-	 * reason, we don't have to consider performance state of the subdomains
-	 * of genpd here.
-	 */
-
-update_state:
-	ret = genpd->set_performance_state(genpd, state);
-	if (ret) {
+	ret = _genpd_reeval_performance_state(genpd, state);
+	if (ret)
 		gpd_data->performance_state = prev;
-		goto unlock;
-	}
 
-	genpd->performance_state = state;
-
-unlock:
 	genpd_unlock(genpd);
 
 	return ret;
-- 
2.19.1.568.g152ad8e3369a

[PATCH V3 6/6] PM / Domains: Propagate performance state updates

[Viresh Kumar]

This commit updates genpd core to start propagating performance state
updates to master domains that have their set_performance_state()
callback set.

Currently a genpd only handles the performance state requirements from
the devices under its control. This commit extends that to also handle
the performance state requirement(s) put on the master genpd by its
sub-domains. There is a separate value required for each master that
the genpd has and so a new field is added to the struct gpd_link
(link->performance_state), which represents the link between a genpd and
its master. The struct gpd_link also got another field
prev_performance_state, which is used by genpd core as a temporary
variable during transitions.

Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/domain.c | 105 ++++++++++++++++++++++++++++++------
 include/linux/pm_domain.h   |   4 ++
 2 files changed, 94 insertions(+), 15 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 32ecbefbd191..5e0479b2e976 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -239,24 +239,90 @@ static void genpd_update_accounting(struct generic_pm_domain *genpd)
 static inline void genpd_update_accounting(struct generic_pm_domain *genpd) {}
 #endif
 
+static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd,
+					   unsigned int state, int depth);
+
 static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
-					unsigned int state)
+					unsigned int state, int depth)
 {
+	struct generic_pm_domain *master;
+	struct gpd_link *link;
+	unsigned int master_state;
 	int ret;
 
+	/* Propagate to masters of genpd */
+	list_for_each_entry(link, &genpd->slave_links, slave_node) {
+		master = link->master;
+
+		if (!master->set_performance_state)
+			continue;
+
+		if (unlikely(!state)) {
+			master_state = 0;
+		} else {
+			/* Find master's performance state */
+			master_state = dev_pm_opp_xlate_performance_state(genpd->opp_table,
+						master->opp_table, state);
+			if (unlikely(!master_state)) {
+				ret = -EINVAL;
+				goto err;
+			}
+		}
+
+		genpd_lock_nested(master, depth + 1);
+
+		link->prev_performance_state = link->performance_state;
+		link->performance_state = master_state;
+		ret = _genpd_reeval_performance_state(master, master_state,
+						      depth + 1);
+		if (ret)
+			link->performance_state = link->prev_performance_state;
+
+		genpd_unlock(master);
+
+		if (ret)
+			goto err;
+	}
+
 	ret = genpd->set_performance_state(genpd, state);
 	if (ret)
-		return ret;
+		goto err;
 
 	genpd->performance_state = state;
 	return 0;
+
+err:
+	/* Encountered an error, lets rollback */
+	list_for_each_entry_continue_reverse(link, &genpd->slave_links,
+					     slave_node) {
+		master = link->master;
+
+		if (!master->set_performance_state)
+			continue;
+
+		genpd_lock_nested(master, depth + 1);
+
+		master_state = link->prev_performance_state;
+		link->performance_state = master_state;
+
+		if (_genpd_reeval_performance_state(master, master_state,
+						    depth + 1)) {
+			pr_err("%s: Failed to roll back to %d performance state\n",
+			       master->name, master_state);
+		}
+
+		genpd_unlock(master);
+	}
+
+	return ret;
 }
 
 static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd,
-					   unsigned int state)
+					   unsigned int state, int depth)
 {
 	struct generic_pm_domain_data *pd_data;
 	struct pm_domain_data *pdd;
+	struct gpd_link *link;
 
 	/* New requested state is same as Max requested state */
 	if (state == genpd->performance_state)
@@ -274,21 +340,30 @@ static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd,
 			state = pd_data->performance_state;
 	}
 
-	if (state == genpd->performance_state)
-		return 0;
-
 	/*
-	 * We aren't propagating performance state changes of a subdomain to its
-	 * masters as we don't have hardware that needs it. Over that, the
-	 * performance states of subdomain and its masters may not have
-	 * one-to-one mapping and would require additional information. We can
-	 * get back to this once we have hardware that needs it. For that
-	 * reason, we don't have to consider performance state of the subdomains
-	 * of genpd here.
+	 * Traverse all sub-domains within the domain. This can be
+	 * done without any additional locking as the link->performance_state
+	 * field is protected by the master genpd->lock, which is already taken.
+	 *
+	 * Also note that link->performance_state (subdomain's performance state
+	 * requirement to master domain) is different from
+	 * link->slave->performance_state (current performance state requirement
+	 * of the devices/sub-domains of the subdomain) and so can have a
+	 * different value.
+	 *
+	 * Note that we also take vote from powered-off sub-domains into account
+	 * as the same is done for devices right now.
 	 */
+	list_for_each_entry(link, &genpd->master_links, master_node) {
+		if (link->performance_state > state)
+			state = link->performance_state;
+	}
+
+	if (state == genpd->performance_state)
+		return 0;
 
 update_state:
-	return _genpd_set_performance_state(genpd, state);
+	return _genpd_set_performance_state(genpd, state, depth);
 }
 
 /**
@@ -332,7 +407,7 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 	prev = gpd_data->performance_state;
 	gpd_data->performance_state = state;
 
-	ret = _genpd_reeval_performance_state(genpd, state);
+	ret = _genpd_reeval_performance_state(genpd, state, 0);
 	if (ret)
 		gpd_data->performance_state = prev;
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 9ad101362aef..dd364abb649a 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -136,6 +136,10 @@ struct gpd_link {
 	struct list_head master_node;
 	struct generic_pm_domain *slave;
 	struct list_head slave_node;
+
+	/* Sub-domain's per-master domain performance state */
+	unsigned int performance_state;
+	unsigned int prev_performance_state;
 };
 
 struct gpd_timing_data {
-- 
2.19.1.568.g152ad8e3369a

Re: [PATCH V3 0/6] PM / Domains: Allow performance state propagation

[Rajendra Nayak]

On 12/12/2018 4:27 PM, Viresh Kumar wrote:
> Hi,
> 
> This series adds performance state propagation support in genpd core.
> The propagation happens from the sub-domains to their masters. More
> details can be found in the individual commit logs.
> 
> This is tested on hikey960 by faking power domains in such a way that
> the CPU devices have two power domains and both of them have the same
> master domain. The CPU device, as well as its power domains have
> "required-opps" property set and the performance requirement from the
> CPU eventually configures all the domains (2 sub-domains and 1 master).
> 
> Based on opp/linux-next branch (which is 4.20-rc1 +
> multiple-power-domain-support-in-opp-core + some OPP fixes).
> 
> Rajendra has already tested the previous version of this series and so I
> have included his Tested-by for all patches.

I also tested this on top of my v7 [1] of rpm power domain series and things
continue to work as expected for the cx/mx propagation so I am fine with my
Tested-by on all the patches.

[1] https://lkml.org/lkml/2018/12/12/174

> 
> V2->V3:
> - Include Ulf's patch (sent separately earlier) with this series.
> - The performance state update code doesn't rely anymore on the power
>    on/off state of the genpd, it sets and propagates rate in all cases.
> - That simplified a lot of code from V2 in _genpd_power_on().
> - commit logs improved for few commits.
> - s/mstate/master_state/
> - and few more minor changes.
> 
> v1->V2:
> - First patch (1/5) is new and an improvement to earlier stuff.
> - Move genpd_status_on() check to _genpd_reeval_performance_state() from
>    _genpd_set_performance_state().
> - Improve dev_pm_opp_xlate_performance_state() to handle 1:1 pstate
>    mapping between genpd and its master and also to fix a problem while
>    finding the dst_table.
> - Handle pstate=0 case properly.
> 
> --
> viresh
> 
> Ulf Hansson (1):
>    PM / Domains: Make genpd performance states orthogonal to the
>      idlestates
> 
> Viresh Kumar (5):
>    OPP: Improve _find_table_of_opp_np()
>    OPP: Add dev_pm_opp_xlate_performance_state() helper
>    PM / Domains: Save OPP table pointer in genpd
>    PM / Domains: Factorize dev_pm_genpd_set_performance_state()
>    PM / Domains: Propagate performance state updates
> 
>   drivers/base/power/domain.c | 207 ++++++++++++++++++++++++++----------
>   drivers/opp/core.c          |  59 ++++++++++
>   drivers/opp/of.c            |  14 ++-
>   include/linux/pm_domain.h   |   6 ++
>   include/linux/pm_opp.h      |   7 ++
>   5 files changed, 235 insertions(+), 58 deletions(-)
> 

Re: [PATCH V3 3/6] OPP: Add dev_pm_opp_xlate_performance_state() helper

[Ulf Hansson]

On Wed, 12 Dec 2018 at 11:57, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> dev_pm_genpd_set_performance_state() needs to handle performance state
> propagation going forward. Currently this routine only gets the required
> performance state of the device's genpd as an argument, but it doesn't
> know how to translate that to master genpd(s) of the device's genpd.
>
> Introduce a new helper dev_pm_opp_xlate_performance_state() which will
> be used to translate from performance state of a device (or genpd
> sub-domain) to another device (or master genpd).
>
> Normally the src_table (of genpd sub-domain) will have the
> "required_opps" property set to point to one of the OPPs in the
> dst_table (of master genpd), but in some cases the genpd and its master
> have one to one mapping of performance states and so none of them have
> the "required-opps" property set. Return the performance state of the
> src_table as it is in such cases.
>
> Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c     | 59 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  7 +++++
>  2 files changed, 66 insertions(+)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 98e60f0ed8b0..386095e2f4f7 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1713,6 +1713,65 @@ void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table,
>                 dev_err(virt_dev, "Failed to find required device entry\n");
>  }
>
> +/**
> + * dev_pm_opp_xlate_performance_state() - Find required OPP's pstate for src_table.
> + * @src_table: OPP table which has dst_table as one of its required OPP table.
> + * @dst_table: Required OPP table of the src_table.
> + * @pstate: Current performance state of the src_table.
> + *
> + * This Returns pstate of the OPP (present in @dst_table) pointed out by the
> + * "required-opps" property of the OPP (present in @src_table) which has
> + * performance state set to @pstate.
> + *
> + * Return: Positive performance state on success, otherwise 0 on errors.

To simplify for callers, I would rather not have the return code 0
being treated as an error. Instead I would prefer 0 to mean that the
state shall be set to zero (which means it should be perfectly fine to
also give 0 as inparam for pstate)

For errors, I think it's better to return regular negative error
codes, as it's standard and more flexible.

Kind regards
Uffe

> + */
> +unsigned int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
> +                                               struct opp_table *dst_table,
> +                                               unsigned int pstate)
> +{
> +       struct dev_pm_opp *opp;
> +       unsigned int dest_pstate = 0;
> +       int i;
> +
> +       /*
> +        * Normally the src_table will have the "required_opps" property set to
> +        * point to one of the OPPs in the dst_table, but in some cases the
> +        * genpd and its master have one to one mapping of performance states
> +        * and so none of them have the "required-opps" property set. Return the
> +        * pstate of the src_table as it is in such cases.
> +        */
> +       if (!src_table->required_opp_count)
> +               return pstate;
> +
> +       for (i = 0; i < src_table->required_opp_count; i++) {
> +               if (src_table->required_opp_tables[i]->np == dst_table->np)
> +                       break;
> +       }
> +
> +       if (unlikely(i == src_table->required_opp_count)) {
> +               pr_err("%s: Couldn't find matching OPP table (%p: %p)\n",
> +                      __func__, src_table, dst_table);
> +               return 0;
> +       }
> +
> +       mutex_lock(&src_table->lock);
> +
> +       list_for_each_entry(opp, &src_table->opp_list, node) {
> +               if (opp->pstate == pstate) {
> +                       dest_pstate = opp->required_opps[i]->pstate;
> +                       goto unlock;
> +               }
> +       }
> +
> +       pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, src_table,
> +              dst_table);
> +
> +unlock:
> +       mutex_unlock(&src_table->lock);
> +
> +       return dest_pstate;
> +}
> +
>  /**
>   * dev_pm_opp_add()  - Add an OPP table from a table definitions
>   * @dev:       device for which we do this operation
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 2b2c3fd985ab..5a64a81a1789 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -128,6 +128,7 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*s
>  void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
>  struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev, struct device *virt_dev, int index);
>  void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table, struct device *virt_dev);
> +unsigned int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
>  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);
> @@ -280,6 +281,12 @@ static inline struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev
>  }
>
>  static inline void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table, struct device *virt_dev) {}
> +
> +static inline unsigned int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate)
> +{
> +       return 0;
> +}
> +
>  static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  {
>         return -ENOTSUPP;
> --
> 2.19.1.568.g152ad8e3369a
>

Re: [PATCH V3 6/6] PM / Domains: Propagate performance state updates

[Ulf Hansson]

On Wed, 12 Dec 2018 at 11:58, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> This commit updates genpd core to start propagating performance state
> updates to master domains that have their set_performance_state()
> callback set.

I would appreciate some more words of what happens during the
propagation. For example, how OPP tables are used and how mapping
between performance states are done from a sub-domain to a
master-domain. At least a high level description would be nice, I
think.

>
> Currently a genpd only handles the performance state requirements from
> the devices under its control. This commit extends that to also handle
> the performance state requirement(s) put on the master genpd by its
> sub-domains. There is a separate value required for each master that
> the genpd has and so a new field is added to the struct gpd_link
> (link->performance_state), which represents the link between a genpd and
> its master. The struct gpd_link also got another field
> prev_performance_state, which is used by genpd core as a temporary
> variable during transitions.
>
> Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/domain.c | 105 ++++++++++++++++++++++++++++++------
>  include/linux/pm_domain.h   |   4 ++
>  2 files changed, 94 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 32ecbefbd191..5e0479b2e976 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -239,24 +239,90 @@ static void genpd_update_accounting(struct generic_pm_domain *genpd)
>  static inline void genpd_update_accounting(struct generic_pm_domain *genpd) {}
>  #endif
>
> +static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd,
> +                                          unsigned int state, int depth);
> +
>  static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
> -                                       unsigned int state)
> +                                       unsigned int state, int depth)
>  {
> +       struct generic_pm_domain *master;
> +       struct gpd_link *link;
> +       unsigned int master_state;
>         int ret;
>
> +       /* Propagate to masters of genpd */
> +       list_for_each_entry(link, &genpd->slave_links, slave_node) {
> +               master = link->master;
> +
> +               if (!master->set_performance_state)
> +                       continue;
> +
> +               if (unlikely(!state)) {
> +                       master_state = 0;
> +               } else {
> +                       /* Find master's performance state */
> +                       master_state = dev_pm_opp_xlate_performance_state(genpd->opp_table,
> +                                               master->opp_table, state);
> +                       if (unlikely(!master_state)) {
> +                               ret = -EINVAL;
> +                               goto err;
> +                       }
> +               }

According to my comment for patch3, the above can be simplified.
Moreover, the "unlikely" thingy above is a bit questionable, as we
can't really know what is "unlikely" here.

> +
> +               genpd_lock_nested(master, depth + 1);
> +
> +               link->prev_performance_state = link->performance_state;
> +               link->performance_state = master_state;
> +               ret = _genpd_reeval_performance_state(master, master_state,
> +                                                     depth + 1);
> +               if (ret)
> +                       link->performance_state = link->prev_performance_state;
> +
> +               genpd_unlock(master);
> +
> +               if (ret)
> +                       goto err;
> +       }
> +
>         ret = genpd->set_performance_state(genpd, state);
>         if (ret)
> -               return ret;
> +               goto err;
>
>         genpd->performance_state = state;
>         return 0;
> +
> +err:
> +       /* Encountered an error, lets rollback */
> +       list_for_each_entry_continue_reverse(link, &genpd->slave_links,
> +                                            slave_node) {
> +               master = link->master;
> +
> +               if (!master->set_performance_state)
> +                       continue;
> +
> +               genpd_lock_nested(master, depth + 1);
> +
> +               master_state = link->prev_performance_state;
> +               link->performance_state = master_state;
> +
> +               if (_genpd_reeval_performance_state(master, master_state,
> +                                                   depth + 1)) {
> +                       pr_err("%s: Failed to roll back to %d performance state\n",
> +                              master->name, master_state);
> +               }
> +
> +               genpd_unlock(master);
> +       }
> +
> +       return ret;
>  }
>
>  static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd,
> -                                          unsigned int state)
> +                                          unsigned int state, int depth)
>  {
>         struct generic_pm_domain_data *pd_data;
>         struct pm_domain_data *pdd;
> +       struct gpd_link *link;
>
>         /* New requested state is same as Max requested state */
>         if (state == genpd->performance_state)
> @@ -274,21 +340,30 @@ static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd,
>                         state = pd_data->performance_state;
>         }
>
> -       if (state == genpd->performance_state)
> -               return 0;
> -
>         /*
> -        * We aren't propagating performance state changes of a subdomain to its
> -        * masters as we don't have hardware that needs it. Over that, the
> -        * performance states of subdomain and its masters may not have
> -        * one-to-one mapping and would require additional information. We can
> -        * get back to this once we have hardware that needs it. For that
> -        * reason, we don't have to consider performance state of the subdomains
> -        * of genpd here.
> +        * Traverse all sub-domains within the domain. This can be
> +        * done without any additional locking as the link->performance_state
> +        * field is protected by the master genpd->lock, which is already taken.
> +        *
> +        * Also note that link->performance_state (subdomain's performance state
> +        * requirement to master domain) is different from
> +        * link->slave->performance_state (current performance state requirement
> +        * of the devices/sub-domains of the subdomain) and so can have a
> +        * different value.
> +        *
> +        * Note that we also take vote from powered-off sub-domains into account
> +        * as the same is done for devices right now.
>          */
> +       list_for_each_entry(link, &genpd->master_links, master_node) {
> +               if (link->performance_state > state)
> +                       state = link->performance_state;
> +       }
> +
> +       if (state == genpd->performance_state)
> +               return 0;
>
>  update_state:
> -       return _genpd_set_performance_state(genpd, state);
> +       return _genpd_set_performance_state(genpd, state, depth);

Instead of calling _genpd_set_performance_state() from here, I suggest
to let the caller do it. Simply return the aggregated new state, if it
needs to be updated - and zero if no update is needed.

Why? I think it may clarify and simplify the code, in regards to the
actual set/propagation of state changes. Another side-effect, is that
you should be able to avoid the forward declaration of
_genpd_reeval_performance_state(), which I think is nice as well.

I guess changing this, should already be done in patch 5, so patch 6
can build on it.

>  }
>
>  /**
> @@ -332,7 +407,7 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
>         prev = gpd_data->performance_state;
>         gpd_data->performance_state = state;
>
> -       ret = _genpd_reeval_performance_state(genpd, state);
> +       ret = _genpd_reeval_performance_state(genpd, state, 0);
>         if (ret)
>                 gpd_data->performance_state = prev;
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 9ad101362aef..dd364abb649a 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -136,6 +136,10 @@ struct gpd_link {
>         struct list_head master_node;
>         struct generic_pm_domain *slave;
>         struct list_head slave_node;
> +
> +       /* Sub-domain's per-master domain performance state */
> +       unsigned int performance_state;
> +       unsigned int prev_performance_state;

Probably a leftover from the earlier versions, please remove.

>  };
>
>  struct gpd_timing_data {
> --
> 2.19.1.568.g152ad8e3369a
>

Kind regards
Uffe

Re: [PATCH V3 6/6] PM / Domains: Propagate performance state updates

[Viresh Kumar]

On 13-12-18, 16:53, Ulf Hansson wrote:
> On Wed, 12 Dec 2018 at 11:58, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >  update_state:
> > -       return _genpd_set_performance_state(genpd, state);
> > +       return _genpd_set_performance_state(genpd, state, depth);
> 
> Instead of calling _genpd_set_performance_state() from here, I suggest
> to let the caller do it. Simply return the aggregated new state, if it
> needs to be updated - and zero if no update is needed.
> 
> Why? I think it may clarify and simplify the code, in regards to the
> actual set/propagation of state changes. Another side-effect, is that
> you should be able to avoid the forward declaration of
> _genpd_reeval_performance_state(), which I think is nice as well.

_genpd_reeval_performance_state() is currently called from 3 different
places and with the suggested change those sites will have this diff.

-               ret = _genpd_reeval_performance_state(master, master_state,
-                                                     depth + 1);
+               master_state = _genpd_reeval_performance_state(master,
+                                               master_state);
+               ret = _genpd_set_performance_state(genpd, master_state, depth);

To be honest, I don't like it. Probably because I don't find the extra
declaration of _genpd_reeval_performance_state() that bad. If two
routines are always going to get called together it is worth calling
the second one from the first one for me.

But anyway, I am fine with it if you are. Please let me know.

> >  }
> >
> >  /**
> > @@ -332,7 +407,7 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
> >         prev = gpd_data->performance_state;
> >         gpd_data->performance_state = state;
> >
> > -       ret = _genpd_reeval_performance_state(genpd, state);
> > +       ret = _genpd_reeval_performance_state(genpd, state, 0);
> >         if (ret)
> >                 gpd_data->performance_state = prev;
> >
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index 9ad101362aef..dd364abb649a 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -136,6 +136,10 @@ struct gpd_link {
> >         struct list_head master_node;
> >         struct generic_pm_domain *slave;
> >         struct list_head slave_node;
> > +
> > +       /* Sub-domain's per-master domain performance state */
> > +       unsigned int performance_state;
> > +       unsigned int prev_performance_state;
> 
> Probably a leftover from the earlier versions, please remove.

No, these are still getting used.

-- 
viresh