From 68de2fe57a8f2746db1064d39c697595cd76bb16 Mon Sep 17 00:00:00 2001 From: Ulf Hansson Date: Tue, 11 Dec 2018 11:04:55 +0100 Subject: PM / Domains: Make genpd performance states orthogonal to the idlestates 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 Acked-by: Viresh Kumar Signed-off-by: Ulf Hansson Signed-off-by: Viresh Kumar --- drivers/base/power/domain.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) (limited to 'drivers/base') 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; -- cgit v1.2.3 From 1067ae3e427fba60965fc519e20d54d0b210fd27 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 2 Nov 2018 11:18:08 +0530 Subject: PM / Domains: Save OPP table pointer in genpd 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 Reviewed-by: Ulf Hansson Signed-off-by: Viresh Kumar --- drivers/base/power/domain.c | 23 +++++++++++++++++++++-- include/linux/pm_domain.h | 2 ++ 2 files changed, 23 insertions(+), 2 deletions(-) (limited to 'drivers/base') 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 @@ error: 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, -- cgit v1.2.3 From cd50c6d3eb91bdff9ac37ee645c49ae274385d35 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 31 Oct 2018 14:56:54 +0530 Subject: PM / Domains: Factorize dev_pm_genpd_set_performance_state() 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. Tested-by: Rajendra Nayak Reviewed-by: Ulf Hansson Signed-off-by: Viresh Kumar --- drivers/base/power/domain.c | 95 ++++++++++++++++++++++++++------------------- 1 file changed, 55 insertions(+), 40 deletions(-) (limited to 'drivers/base') diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 1e98c637e069..808ba41b6580 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -239,6 +239,56 @@ 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) +{ + 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 state; + + /* New requested state is higher than Max requested state */ + if (state > genpd->performance_state) + return 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; + } + + /* + * 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. + */ + return state; +} + +static int _genpd_set_performance_state(struct generic_pm_domain *genpd, + unsigned int state) +{ + int ret; + + if (state == genpd->performance_state) + return 0; + + ret = genpd->set_performance_state(genpd, state); + if (ret) + return ret; + + genpd->performance_state = state; + return 0; +} + /** * dev_pm_genpd_set_performance_state- Set performance state of device's power * domain. @@ -257,10 +307,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 +330,11 @@ 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) { + state = _genpd_reeval_performance_state(genpd, state); + ret = _genpd_set_performance_state(genpd, state); + if (ret) gpd_data->performance_state = prev; - goto unlock; - } - genpd->performance_state = state; - -unlock: genpd_unlock(genpd); return ret; -- cgit v1.2.3 From 18edf49c45544cfb93002b3b31fe8fc7fc14d95c Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 2 Nov 2018 14:40:19 +0530 Subject: PM / Domains: Propagate performance state updates 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. On a call to dev_pm_genpd_set_performance_state(), the genpd core first updates the performance state of the masters of the device's genpd and then updates the performance state of the genpd. The masters do the same and propagate performance state updates to their masters before updating their own. The performance state transition from genpd to its master is done with the help of dev_pm_opp_xlate_performance_state(), which looks at the OPP tables of both the domains to translate the state. Tested-by: Rajendra Nayak Reviewed-by: Ulf Hansson Signed-off-by: Viresh Kumar --- drivers/base/power/domain.c | 93 +++++++++++++++++++++++++++++++++++++++------ include/linux/pm_domain.h | 4 ++ 2 files changed, 86 insertions(+), 11 deletions(-) (limited to 'drivers/base') diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 808ba41b6580..611c0ccbad5f 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -244,6 +244,7 @@ static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd, { 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) @@ -262,31 +263,101 @@ static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd, } /* - * 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; + } + return state; } static int _genpd_set_performance_state(struct generic_pm_domain *genpd, - unsigned int state) + unsigned int state, int depth) { - int ret; + struct generic_pm_domain *master; + struct gpd_link *link; + int master_state, ret; if (state == genpd->performance_state) return 0; + /* 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 */ + ret = dev_pm_opp_xlate_performance_state(genpd->opp_table, + master->opp_table, + state); + if (unlikely(ret < 0)) + goto err; + + master_state = ret; + + genpd_lock_nested(master, depth + 1); + + link->prev_performance_state = link->performance_state; + link->performance_state = master_state; + master_state = _genpd_reeval_performance_state(master, + master_state); + ret = _genpd_set_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; + + master_state = _genpd_reeval_performance_state(master, + master_state); + if (_genpd_set_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; } /** @@ -331,7 +402,7 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) gpd_data->performance_state = state; state = _genpd_reeval_performance_state(genpd, state); - ret = _genpd_set_performance_state(genpd, state); + ret = _genpd_set_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 { -- cgit v1.2.3