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

[PATCH 0/4] 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).

@Rajendra: Will it be possible for you to run some tests and tell me if
some stuff is still missing as per Qcom requirements ?

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

--
viresh

Viresh Kumar (4):
  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 | 204 +++++++++++++++++++++++++++---------
 drivers/opp/core.c          |  49 +++++++++
 include/linux/pm_domain.h   |   6 ++
 include/linux/pm_opp.h      |   7 ++
 4 files changed, 217 insertions(+), 49 deletions(-)

-- 
2.19.1.568.g152ad8e3369a

[PATCH 1/4] OPP: Add dev_pm_opp_xlate_performance_state() helper

[Viresh Kumar]

Introduce a new helper dev_pm_opp_xlate_performance_state() which will
be used to translate from pstate of a device to another one.

Initially this will be used by genpd to find pstate of a master domain
using its sub-domain's pstate.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 49 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  7 ++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 0eaa954b3f6c..010a4268e8dd 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1707,6 +1707,55 @@ 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;
+
+	for (i = 0; i < src_table->required_opp_count; i++) {
+		if (src_table->required_opp_tables[i] == dst_table)
+			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 2/4] PM / Domains: Save OPP table pointer in genpd

[Viresh Kumar]

We will need these going forward in hotpath, i.e. from within
dev_pm_genpd_set_performance_state().

Lets fetch and save them while the OPP tables are added.

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 8e554e6a82a2..0d928359b880 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1907,12 +1907,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;
 	}
@@ -1965,6 +1974,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;
@@ -1989,8 +2005,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);
@@ -2024,6 +2042,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 3/4] 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.

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

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0d928359b880..6d2e9b3406f1 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -239,6 +239,62 @@ 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;
+
+	if (!genpd_status_on(genpd))
+		goto out;
+
+	ret = genpd->set_performance_state(genpd, state);
+	if (ret)
+		return ret;
+
+out:
+	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 +313,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,47 +336,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:
-	if (genpd_status_on(genpd)) {
-		ret = genpd->set_performance_state(genpd, state);
-		if (ret) {
-			gpd_data->performance_state = prev;
-			goto unlock;
-		}
-	}
-
-	genpd->performance_state = state;
+	ret = _genpd_reeval_performance_state(genpd, state);
+	if (ret)
+		gpd_data->performance_state = prev;
 
-unlock:
 	genpd_unlock(genpd);
 
 	return ret;
-- 
2.19.1.568.g152ad8e3369a

[PATCH 4/4] 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.

A genpd handles two type of performance states now. The first one is the
performance state requirement put on the genpd by the devices and
sub-domains under it, which is already represented by
genpd->performance_state. The second type, introduced in this commit, is
the performance state requirement(s) put by the genpd on its master
domain(s). 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.

We need to propagate setting performance state while powering-on a genpd
as well, as we ignore performance state requirements from sub-domains
which are powered-off. For this reason _genpd_power_on() also received
the additional parameter, depth, which is used for hierarchical locking
within genpd.

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

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 6d2e9b3406f1..81e02c5f753f 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -239,28 +239,86 @@ 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 mstate;
 	int ret;
 
 	if (!genpd_status_on(genpd))
 		goto out;
 
+	/* Propagate to masters of genpd */
+	list_for_each_entry(link, &genpd->slave_links, slave_node) {
+		master = link->master;
+
+		if (!master->set_performance_state)
+			continue;
+
+		/* Find master's performance state */
+		mstate = dev_pm_opp_xlate_performance_state(genpd->opp_table,
+				master->opp_table, state);
+		if (unlikely(!mstate))
+			goto err;
+
+		genpd_lock_nested(master, depth + 1);
+
+		link->prev_performance_state = link->performance_state;
+		link->performance_state = mstate;
+		ret = _genpd_reeval_performance_state(master, mstate, 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;
 
 out:
 	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);
+
+		mstate = link->prev_performance_state;
+		link->performance_state = mstate;
+
+		if (_genpd_reeval_performance_state(master, mstate, depth + 1)) {
+			pr_err("%s: Failed to roll back to %d performance state\n",
+			       master->name, mstate);
+		}
+
+		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)
@@ -278,21 +336,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 powered-on subdomains 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.
 	 */
+	list_for_each_entry(link, &genpd->master_links, master_node) {
+		if (!genpd_status_on(link->slave))
+			continue;
+
+		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);
 }
 
 /**
@@ -336,7 +403,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;
 
@@ -346,7 +413,8 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 }
 EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
 
-static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
+static int
+_genpd_power_on(struct generic_pm_domain *genpd, bool timed, int depth)
 {
 	unsigned int state_idx = genpd->state_idx;
 	ktime_t time_start;
@@ -367,7 +435,8 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 	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);
+		ret = _genpd_set_performance_state(genpd,
+					genpd->performance_state, depth);
 		if (ret) {
 			pr_warn("%s: Failed to set performance state %d (%d)\n",
 				genpd->name, genpd->performance_state, ret);
@@ -557,7 +626,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth)
 		}
 	}
 
-	ret = _genpd_power_on(genpd, true);
+	ret = _genpd_power_on(genpd, true, depth);
 	if (ret)
 		goto err;
 
@@ -962,7 +1031,7 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock,
 			genpd_unlock(link->master);
 	}
 
-	_genpd_power_on(genpd, false);
+	_genpd_power_on(genpd, false, depth);
 
 	genpd->status = GPD_STATE_ACTIVE;
 }
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 4/4] PM / Domains: Propagate performance state updates

[Rajendra Nayak]

Hi Viresh,

On 11/5/2018 12:06 PM, Viresh Kumar wrote:
> This commit updates genpd core to start propagating performance state
> updates to master domains that have their set_performance_state()
> callback set.
> 
> A genpd handles two type of performance states now. The first one is the
> performance state requirement put on the genpd by the devices and
> sub-domains under it, which is already represented by
> genpd->performance_state. The second type, introduced in this commit, is
> the performance state requirement(s) put by the genpd on its master
> domain(s). 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.
> 
> We need to propagate setting performance state while powering-on a genpd
> as well, as we ignore performance state requirements from sub-domains
> which are powered-off. For this reason _genpd_power_on() also received
> the additional parameter, depth, which is used for hierarchical locking
> within genpd.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/base/power/domain.c | 107 +++++++++++++++++++++++++++++-------
>   include/linux/pm_domain.h   |   4 ++
>   2 files changed, 92 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 6d2e9b3406f1..81e02c5f753f 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -239,28 +239,86 @@ 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 mstate;
>   	int ret;
>   
>   	if (!genpd_status_on(genpd))
>   		goto out;

This check here would mean we only propogate performance state
to masters if the genpd is ON?

thanks,
Rajendra

>   
> +	/* Propagate to masters of genpd */
> +	list_for_each_entry(link, &genpd->slave_links, slave_node) {
> +		master = link->master;
> +
> +		if (!master->set_performance_state)
> +			continue;
> +
> +		/* Find master's performance state */
> +		mstate = dev_pm_opp_xlate_performance_state(genpd->opp_table,
> +				master->opp_table, state);
> +		if (unlikely(!mstate))
> +			goto err;
> +
> +		genpd_lock_nested(master, depth + 1);
> +
> +		link->prev_performance_state = link->performance_state;
> +		link->performance_state = mstate;
> +		ret = _genpd_reeval_performance_state(master, mstate, 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;
>   
>   out:
>   	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);
> +
> +		mstate = link->prev_performance_state;
> +		link->performance_state = mstate;
> +
> +		if (_genpd_reeval_performance_state(master, mstate, depth + 1)) {
> +			pr_err("%s: Failed to roll back to %d performance state\n",
> +			       master->name, mstate);
> +		}
> +
> +		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)
> @@ -278,21 +336,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 powered-on subdomains 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.
>   	 */
> +	list_for_each_entry(link, &genpd->master_links, master_node) {
> +		if (!genpd_status_on(link->slave))
> +			continue;
> +
> +		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);
>   }
>   
>   /**
> @@ -336,7 +403,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;
>   
> @@ -346,7 +413,8 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
>   }
>   EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
>   
> -static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
> +static int
> +_genpd_power_on(struct generic_pm_domain *genpd, bool timed, int depth)
>   {
>   	unsigned int state_idx = genpd->state_idx;
>   	ktime_t time_start;
> @@ -367,7 +435,8 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>   	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);
> +		ret = _genpd_set_performance_state(genpd,
> +					genpd->performance_state, depth);
>   		if (ret) {
>   			pr_warn("%s: Failed to set performance state %d (%d)\n",
>   				genpd->name, genpd->performance_state, ret);
> @@ -557,7 +626,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth)
>   		}
>   	}
>   
> -	ret = _genpd_power_on(genpd, true);
> +	ret = _genpd_power_on(genpd, true, depth);
>   	if (ret)
>   		goto err;
>   
> @@ -962,7 +1031,7 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock,
>   			genpd_unlock(link->master);
>   	}
>   
> -	_genpd_power_on(genpd, false);
> +	_genpd_power_on(genpd, false, depth);
>   
>   	genpd->status = GPD_STATE_ACTIVE;
>   }
> 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 {
> 

Re: [PATCH 1/4] OPP: Add dev_pm_opp_xlate_performance_state() helper

[Rajendra Nayak]

On 11/5/2018 12:06 PM, Viresh Kumar wrote:
> Introduce a new helper dev_pm_opp_xlate_performance_state() which will
> be used to translate from pstate of a device to another one.
> 
> Initially this will be used by genpd to find pstate of a master domain
> using its sub-domain's pstate.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/opp/core.c     | 49 ++++++++++++++++++++++++++++++++++++++++++
>   include/linux/pm_opp.h |  7 ++++++
>   2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0eaa954b3f6c..010a4268e8dd 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1707,6 +1707,55 @@ 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.

So I have a case where the src_table and dst_table are shared/same. Can you explain how would
it work in such a case?

> + * @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;
> +
> +	for (i = 0; i < src_table->required_opp_count; i++) {
> +		if (src_table->required_opp_tables[i] == dst_table)
> +			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;
> 

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

[Viresh Kumar]

On 21-11-18, 10:33, Rajendra Nayak wrote:
> Hi Viresh,
> 
> On 11/5/2018 12:06 PM, Viresh Kumar wrote:
> > This commit updates genpd core to start propagating performance state
> > updates to master domains that have their set_performance_state()
> > callback set.
> > 
> > A genpd handles two type of performance states now. The first one is the
> > performance state requirement put on the genpd by the devices and
> > sub-domains under it, which is already represented by
> > genpd->performance_state. The second type, introduced in this commit, is
> > the performance state requirement(s) put by the genpd on its master
> > domain(s). 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.
> > 
> > We need to propagate setting performance state while powering-on a genpd
> > as well, as we ignore performance state requirements from sub-domains
> > which are powered-off. For this reason _genpd_power_on() also received
> > the additional parameter, depth, which is used for hierarchical locking
> > within genpd.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >   drivers/base/power/domain.c | 107 +++++++++++++++++++++++++++++-------
> >   include/linux/pm_domain.h   |   4 ++
> >   2 files changed, 92 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 6d2e9b3406f1..81e02c5f753f 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -239,28 +239,86 @@ 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 mstate;
> >   	int ret;
> >   	if (!genpd_status_on(genpd))
> >   		goto out;
> 
> This check here would mean we only propogate performance state
> to masters if the genpd is ON?

Yeah, isn't that what should we do anyway? It is similar to clk-enable
etc. Why increase the reference count if the device isn't enabled and
isn't using the clock ?

Also you can see that I have updated the genpd power-on code to start
propagation to make sure we don't miss anything.

-- 
viresh

Re: [PATCH 1/4] OPP: Add dev_pm_opp_xlate_performance_state() helper

[Viresh Kumar]

On 21-11-18, 10:34, Rajendra Nayak wrote:
> 
> 
> On 11/5/2018 12:06 PM, Viresh Kumar wrote:
> > Introduce a new helper dev_pm_opp_xlate_performance_state() which will
> > be used to translate from pstate of a device to another one.
> > 
> > Initially this will be used by genpd to find pstate of a master domain
> > using its sub-domain's pstate.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >   drivers/opp/core.c     | 49 ++++++++++++++++++++++++++++++++++++++++++
> >   include/linux/pm_opp.h |  7 ++++++
> >   2 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 0eaa954b3f6c..010a4268e8dd 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1707,6 +1707,55 @@ 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.
> 
> So I have a case where the src_table and dst_table are shared/same. Can you explain how would
> it work in such a case?

Can you give the example, as I am finding some issues with such shared
tables. Though the code may work just fine btw.

-- 
viresh

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

[Rajendra Nayak]

On 11/21/2018 10:46 AM, Viresh Kumar wrote:
> On 21-11-18, 10:33, Rajendra Nayak wrote:
>> Hi Viresh,
>>
>> On 11/5/2018 12:06 PM, Viresh Kumar wrote:
>>> This commit updates genpd core to start propagating performance state
>>> updates to master domains that have their set_performance_state()
>>> callback set.
>>>
>>> A genpd handles two type of performance states now. The first one is the
>>> performance state requirement put on the genpd by the devices and
>>> sub-domains under it, which is already represented by
>>> genpd->performance_state. The second type, introduced in this commit, is
>>> the performance state requirement(s) put by the genpd on its master
>>> domain(s). 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.
>>>
>>> We need to propagate setting performance state while powering-on a genpd
>>> as well, as we ignore performance state requirements from sub-domains
>>> which are powered-off. For this reason _genpd_power_on() also received
>>> the additional parameter, depth, which is used for hierarchical locking
>>> within genpd.
>>>
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> ---
>>>    drivers/base/power/domain.c | 107 +++++++++++++++++++++++++++++-------
>>>    include/linux/pm_domain.h   |   4 ++
>>>    2 files changed, 92 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index 6d2e9b3406f1..81e02c5f753f 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -239,28 +239,86 @@ 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 mstate;
>>>    	int ret;
>>>    	if (!genpd_status_on(genpd))
>>>    		goto out;
>>
>> This check here would mean we only propogate performance state
>> to masters if the genpd is ON?
> 
> Yeah, isn't that what should we do anyway? It is similar to clk-enable
> etc. Why increase the reference count if the device isn't enabled and
> isn't using the clock ?

I would think this is analogous to a driver calling clk_set_rate() first and
then a clk_enable(), which is certainly valid.
So my question is, if calling a dev_pm_genpd_set_performance_state()
and then runtime enabling the device would work (and take care of propagating the performance
state). In my testing I found it does not.

> 
> Also you can see that I have updated the genpd power-on code to start
> propagation to make sure we don't miss anything.
> 

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

[Viresh Kumar]

On 21-11-18, 11:01, Rajendra Nayak wrote:
> I would think this is analogous to a driver calling clk_set_rate() first and
> then a clk_enable(), which is certainly valid.
> So my question is, if calling a dev_pm_genpd_set_performance_state()
> and then runtime enabling the device would work (and take care of propagating the performance
> state).

Two things here..

- First, it should work.

- Second, we don't look at device RPM states but only if a genpd is on
  or off. Maybe we should look at device states as well, but then we
  need to initiate set-performance-state routine from runtime enable
  as well. But that is completely different work and would require its
  own series.

> In my testing I found it does not.

As I said, we don't do anything in RPM enable of the device currently,
but only during genpd_power_on(). So if the genpd was previously
disabled, RPM enable of the device should have done propagation of
states as well. Otherwise, we should already have take care of the
vote from the device. So, it should have worked.

Needs some investigation on why this isn't working for you.

-- 
viresh

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

[Rajendra Nayak]

On 11/21/2018 11:01 AM, Rajendra Nayak wrote:
> 
> 
> On 11/21/2018 10:46 AM, Viresh Kumar wrote:
>> On 21-11-18, 10:33, Rajendra Nayak wrote:
>>> Hi Viresh,
>>>
>>> On 11/5/2018 12:06 PM, Viresh Kumar wrote:
>>>> This commit updates genpd core to start propagating performance state
>>>> updates to master domains that have their set_performance_state()
>>>> callback set.
>>>>
>>>> A genpd handles two type of performance states now. The first one is the
>>>> performance state requirement put on the genpd by the devices and
>>>> sub-domains under it, which is already represented by
>>>> genpd->performance_state. The second type, introduced in this commit, is
>>>> the performance state requirement(s) put by the genpd on its master
>>>> domain(s). 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.
>>>>
>>>> We need to propagate setting performance state while powering-on a genpd
>>>> as well, as we ignore performance state requirements from sub-domains
>>>> which are powered-off. For this reason _genpd_power_on() also received
>>>> the additional parameter, depth, which is used for hierarchical locking
>>>> within genpd.
>>>>
>>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>> ---
>>>>    drivers/base/power/domain.c | 107 +++++++++++++++++++++++++++++-------
>>>>    include/linux/pm_domain.h   |   4 ++
>>>>    2 files changed, 92 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>>> index 6d2e9b3406f1..81e02c5f753f 100644
>>>> --- a/drivers/base/power/domain.c
>>>> +++ b/drivers/base/power/domain.c
>>>> @@ -239,28 +239,86 @@ 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 mstate;
>>>>        int ret;
>>>>        if (!genpd_status_on(genpd))
>>>>            goto out;
>>>
>>> This check here would mean we only propogate performance state
>>> to masters if the genpd is ON?
>>
>> Yeah, isn't that what should we do anyway? It is similar to clk-enable
>> etc. Why increase the reference count if the device isn't enabled and
>> isn't using the clock ?
> 
> I would think this is analogous to a driver calling clk_set_rate() first and
> then a clk_enable(), which is certainly valid.
> So my question is, if calling a dev_pm_genpd_set_performance_state()
> and then runtime enabling the device would work (and take care of propagating the performance
> state). In my testing I found it does not.

And the reason for that seems to be that we update the genpd status to GPD_STATE_ACTIVE
*after* we try to set the performance state, so we always hit this check which bails out
thinking the genpd is not ON.

> 
>>
>> Also you can see that I have updated the genpd power-on code to start
>> propagation to make sure we don't miss anything.
>>

Re: [PATCH 1/4] OPP: Add dev_pm_opp_xlate_performance_state() helper

[Viresh Kumar]

On 21-11-18, 10:47, Viresh Kumar wrote:
> On 21-11-18, 10:34, Rajendra Nayak wrote:
> > 
> > 
> > On 11/5/2018 12:06 PM, Viresh Kumar wrote:
> > > Introduce a new helper dev_pm_opp_xlate_performance_state() which will
> > > be used to translate from pstate of a device to another one.
> > > 
> > > Initially this will be used by genpd to find pstate of a master domain
> > > using its sub-domain's pstate.
> > > 
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > >   drivers/opp/core.c     | 49 ++++++++++++++++++++++++++++++++++++++++++
> > >   include/linux/pm_opp.h |  7 ++++++
> > >   2 files changed, 56 insertions(+)
> > > 
> > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > > index 0eaa954b3f6c..010a4268e8dd 100644
> > > --- a/drivers/opp/core.c
> > > +++ b/drivers/opp/core.c
> > > @@ -1707,6 +1707,55 @@ 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.
> > 
> > So I have a case where the src_table and dst_table are shared/same. Can you explain how would
> > it work in such a case?
> 
> Can you give the example, as I am finding some issues with such shared
> tables. Though the code may work just fine btw.

I may have found the problem you are facing here. Please try this diff
and tell me if you hitting it, check this in dmesg.

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 3740822b4197..6c45774306b0 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -225,6 +225,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 free_required_tables:
        _opp_table_free_required_tables(opp_table);
 put_np:
+       dev_err(dev, "Failed to allocate required OPP tables\n");
        of_node_put(np);
 }


-- 
viresh

Re: [PATCH 1/4] OPP: Add dev_pm_opp_xlate_performance_state() helper

[Rajendra Nayak]

On 11/21/2018 11:36 AM, Viresh Kumar wrote:
> On 21-11-18, 10:47, Viresh Kumar wrote:
>> On 21-11-18, 10:34, Rajendra Nayak wrote:
>>>
>>>
>>> On 11/5/2018 12:06 PM, Viresh Kumar wrote:
>>>> Introduce a new helper dev_pm_opp_xlate_performance_state() which will
>>>> be used to translate from pstate of a device to another one.
>>>>
>>>> Initially this will be used by genpd to find pstate of a master domain
>>>> using its sub-domain's pstate.
>>>>
>>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>> ---
>>>>    drivers/opp/core.c     | 49 ++++++++++++++++++++++++++++++++++++++++++
>>>>    include/linux/pm_opp.h |  7 ++++++
>>>>    2 files changed, 56 insertions(+)
>>>>
>>>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>>>> index 0eaa954b3f6c..010a4268e8dd 100644
>>>> --- a/drivers/opp/core.c
>>>> +++ b/drivers/opp/core.c
>>>> @@ -1707,6 +1707,55 @@ 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.
>>>
>>> So I have a case where the src_table and dst_table are shared/same. Can you explain how would
>>> it work in such a case?
>>
>> Can you give the example, as I am finding some issues with such shared
>> tables. Though the code may work just fine btw.
> 
> I may have found the problem you are facing here. Please try this diff
> and tell me if you hitting it, check this in dmesg.

Yes, I do seem to be hitting this.

> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 3740822b4197..6c45774306b0 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -225,6 +225,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>   free_required_tables:
>          _opp_table_free_required_tables(opp_table);
>   put_np:
> +       dev_err(dev, "Failed to allocate required OPP tables\n");
>          of_node_put(np);
>   }
> 
> 

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

[Viresh Kumar]

On 21-11-18, 11:12, Rajendra Nayak wrote:
> And the reason for that seems to be that we update the genpd status to GPD_STATE_ACTIVE
> *after* we try to set the performance state, so we always hit this check which bails out
> thinking the genpd is not ON.

Thanks for looking at it. Here is the (untested) fix, please try it
out.

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 84c13695af65..92be4a224b45 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -250,9 +250,6 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
        unsigned int mstate;
        int ret;
 
-       if (!genpd_status_on(genpd))
-               goto out;
-
        /* Propagate to masters of genpd */
        list_for_each_entry(link, &genpd->slave_links, slave_node) {
                master = link->master;
@@ -286,7 +283,6 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
        if (ret)
                goto err;
 
-out:
        genpd->performance_state = state;
        return 0;
 
@@ -361,6 +357,11 @@ static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd,
                return 0;
 
 update_state:
+       if (!genpd_status_on(genpd)) {
+               genpd->performance_state = state;
+               return 0;
+       }
+
        return _genpd_set_performance_state(genpd, state, depth);
 }

-- 
viresh

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

[Rajendra Nayak]

On 11/21/2018 12:06 PM, Viresh Kumar wrote:
> On 21-11-18, 11:12, Rajendra Nayak wrote:
>> And the reason for that seems to be that we update the genpd status to GPD_STATE_ACTIVE
>> *after* we try to set the performance state, so we always hit this check which bails out
>> thinking the genpd is not ON.
> 
> Thanks for looking at it. Here is the (untested) fix, please try it
> out.

Thanks, yes, this does seem to work.

> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 84c13695af65..92be4a224b45 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -250,9 +250,6 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
>          unsigned int mstate;
>          int ret;
>   
> -       if (!genpd_status_on(genpd))
> -               goto out;
> -
>          /* Propagate to masters of genpd */
>          list_for_each_entry(link, &genpd->slave_links, slave_node) {
>                  master = link->master;
> @@ -286,7 +283,6 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
>          if (ret)
>                  goto err;
>   
> -out:
>          genpd->performance_state = state;
>          return 0;
>   
> @@ -361,6 +357,11 @@ static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd,
>                  return 0;
>   
>   update_state:
> +       if (!genpd_status_on(genpd)) {
> +               genpd->performance_state = state;
> +               return 0;
> +       }
> +
>          return _genpd_set_performance_state(genpd, state, depth);
>   }
> 

Re: [PATCH 1/4] OPP: Add dev_pm_opp_xlate_performance_state() helper

[Viresh Kumar]

On 21-11-18, 11:48, Rajendra Nayak wrote:
> 
> 
> On 11/21/2018 11:36 AM, Viresh Kumar wrote:
> > On 21-11-18, 10:47, Viresh Kumar wrote:
> > > On 21-11-18, 10:34, Rajendra Nayak wrote:
> > > > 
> > > > 
> > > > On 11/5/2018 12:06 PM, Viresh Kumar wrote:
> > > > > Introduce a new helper dev_pm_opp_xlate_performance_state() which will
> > > > > be used to translate from pstate of a device to another one.
> > > > > 
> > > > > Initially this will be used by genpd to find pstate of a master domain
> > > > > using its sub-domain's pstate.
> > > > > 
> > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > > > ---
> > > > >    drivers/opp/core.c     | 49 ++++++++++++++++++++++++++++++++++++++++++
> > > > >    include/linux/pm_opp.h |  7 ++++++
> > > > >    2 files changed, 56 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > > > > index 0eaa954b3f6c..010a4268e8dd 100644
> > > > > --- a/drivers/opp/core.c
> > > > > +++ b/drivers/opp/core.c
> > > > > @@ -1707,6 +1707,55 @@ 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.
> > > > 
> > > > So I have a case where the src_table and dst_table are shared/same. Can you explain how would
> > > > it work in such a case?
> > > 
> > > Can you give the example, as I am finding some issues with such shared
> > > tables. Though the code may work just fine btw.
> > 
> > I may have found the problem you are facing here. Please try this diff
> > and tell me if you hitting it, check this in dmesg.
> 
> Yes, I do seem to be hitting this.

So there are few complexities in the case where an OPP table points to itself in
the required-opp field. I looked at solving it up in the opp core but that gets
more and more messy.

Now there is actually a assumption within the OPP core. Your Mx domain should
get initialized before the Cx domain, as that is when the OPP tables are created
as well. This is because Cx's OPP table will point to Mx's OPP table (doesn't
matter if they share the same table or not) and so Mx's OPP table should come
first.

Can you check if that is already the case for you? If not, please try doing it
and lemme know if it works. It should.

I just want to avoid too much complexity in OPP core without much use.

-- 
viresh

Re: [PATCH 1/4] OPP: Add dev_pm_opp_xlate_performance_state() helper

[Viresh Kumar]

On 22-11-18, 11:38, Viresh Kumar wrote:
> So there are few complexities in the case where an OPP table points to itself in
> the required-opp field. I looked at solving it up in the opp core but that gets
> more and more messy.
> 
> Now there is actually a assumption within the OPP core. Your Mx domain should
> get initialized before the Cx domain, as that is when the OPP tables are created
> as well. This is because Cx's OPP table will point to Mx's OPP table (doesn't
> matter if they share the same table or not) and so Mx's OPP table should come
> first.
> 
> Can you check if that is already the case for you? If not, please try doing it
> and lemme know if it works. It should.
> 
> I just want to avoid too much complexity in OPP core without much use.

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 7f338d39809a..09df3fbdb555 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1734,7 +1734,7 @@ unsigned int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
        int i;
 
        for (i = 0; i < src_table->required_opp_count; i++) {
-               if (src_table->required_opp_tables[i] == dst_table)
+               if (src_table->required_opp_tables[i]->np == dst_table->np)
                        break;
        }

Try this fix and it should make it work for you.

Having said that, the way you are representing all your domains with a
single OPP table look incorrect. You are using the same OPP table for
around 10 power domains, all provided by a single provider. This is
absolutely fine. But the Mx domain doesn't have any dependency on any
other domain actually and so its OPPs should never have the
"required-opps" property, but it has those properties in your setup as
you are trying to use the same OPP table. That may all work fine with
the above patch (which is required anyway as it fixes a valid issue),
but you may see a error warning that something failed for Mx domain,
as it has required-opps property but no required device or genpd.

Anyway, please test above first and then you can fix your tables :)

-- 
viresh

Re: [PATCH 1/4] OPP: Add dev_pm_opp_xlate_performance_state() helper

[Viresh Kumar]

On 23-11-18, 14:41, Viresh Kumar wrote:
> On 22-11-18, 11:38, Viresh Kumar wrote:
> > So there are few complexities in the case where an OPP table points to itself in
> > the required-opp field. I looked at solving it up in the opp core but that gets
> > more and more messy.
> > 
> > Now there is actually a assumption within the OPP core. Your Mx domain should
> > get initialized before the Cx domain, as that is when the OPP tables are created
> > as well. This is because Cx's OPP table will point to Mx's OPP table (doesn't
> > matter if they share the same table or not) and so Mx's OPP table should come
> > first.
> > 
> > Can you check if that is already the case for you? If not, please try doing it
> > and lemme know if it works. It should.
> > 
> > I just want to avoid too much complexity in OPP core without much use.
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 7f338d39809a..09df3fbdb555 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1734,7 +1734,7 @@ unsigned int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
>         int i;
>  
>         for (i = 0; i < src_table->required_opp_count; i++) {
> -               if (src_table->required_opp_tables[i] == dst_table)
> +               if (src_table->required_opp_tables[i]->np == dst_table->np)
>                         break;
>         }
> 
> Try this fix and it should make it work for you.
> 
> Having said that, the way you are representing all your domains with a
> single OPP table look incorrect. You are using the same OPP table for
> around 10 power domains, all provided by a single provider. This is
> absolutely fine. But the Mx domain doesn't have any dependency on any
> other domain actually and so its OPPs should never have the
> "required-opps" property, but it has those properties in your setup as
> you are trying to use the same OPP table. That may all work fine with
> the above patch (which is required anyway as it fixes a valid issue),
> but you may see a error warning that something failed for Mx domain,
> as it has required-opps property but no required device or genpd.
> 
> Anyway, please test above first and then you can fix your tables :)

Here is the fix you need for the case where cx and mx have 1:1 mapping
and we don't need to duplicate OPP tables in DT.

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index ad944d75b40b..99b71f60b6d8 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1727,6 +1727,16 @@ unsigned int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
        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;

-- 
viresh