PM / Domains: Allow performance state propagation
Subsystems (6)
v4 cover PM / Domains: Allow performance state propagation
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.
V3->V4:
- dev_pm_opp_xlate_performance_state() returns negative values on error now.
- of_get_required_opp_performance_state() is also updated to do the same.
- _genpd_set_performance_state() is not called anymore from _genpd_reeval_performance_state() and we get rid of the extra declaration.
- Improved commit log in the last patch.
- dropped an unlikely.
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 (6):
OPP: Improve _find_table_of_opp_np()
OPP: Add dev_pm_opp_xlate_performance_state() helper
OPP: Don't return 0 on error from
of_get_required_opp_performance_state()
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 | 202 ++++++++++++++++++++++++++---------- drivers/opp/core.c | 63 +++++++++++ drivers/opp/of.c | 24 +++-- include/linux/pm_domain.h | 6 ++ include/linux/pm_opp.h | 13 ++- 5 files changed, 242 insertions(+), 66 deletions(-)
Signature · 1 line
2.19.1.568.g152ad8e3369a
PATCH v4 1/7 PM / Domains: Make genpd performance states orthogonal to the idlestates
From: Ulf Hansson <[email protected]>
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.
--- drivers/base/power/domain.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-)
drivers/base/power/domain.c +4 -15
@@ -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;
Signature · 1 line
2.19.1.568.g152ad8e3369a
PATCH v4 2/7 OPP: Improve _find_table_of_opp_np()
In reply to Viresh KumarMake _find_table_of_opp_np() more efficient by using of_get_parent() to find the parent OPP table node.
--- drivers/opp/of.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/opp/of.c +10 -4
@@ -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 ); }
Signature · 1 line
2.19.1.568.g152ad8e3369a
Re: [PATCH V4 2/7] OPP: Improve _find_table_of_opp_np()
Quoted 6 lines · Viresh Kumar
Make
_find_table_of_opp_np()more efficient by usingof_get_parent()to find the parent OPP table node.Tested-by Rajendra Nayak <[email protected]>Signed-off-by Viresh Kumar <[email protected]>
Kind regards
Uffe
Quoted 41 lines
--- drivers/opp/of.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)drivers/opp/of.c +10
-5@@ -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 v4 3/7 OPP: Add dev_pm_opp_xlate_performance_state() helper
In reply to Viresh Kumardev_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.
--- drivers/opp/core.c | 63 ++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 7 +++++ 2 files changed, 70 insertions(+)
drivers/opp/core.c +63 -0
@@ -1713,6 +1713,69 @@ 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: Zero or positive performance state on success, otherwise negative +* value on errors. +*/ +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; +int dest_pstate = -EINVAL ; +int i; + +if (!pstate) +return 0 ; + +/* +* 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 -EINVAL ; + } + +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
include/linux/pm_opp.h +7 -0
@@ -128,6 +128,7 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*svoid 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); +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 int dev_pm_opp_xlate_performance_state (struct opp_table *src_table,struct opp_table *dst_table,unsigned int pstate) +{ +return -ENOTSUPP ; +} +static inline int dev_pm_opp_set_rate (struct device *dev,unsigned long target_freq) {return -ENOTSUPP ;
Signature · 1 line
2.19.1.568.g152ad8e3369a
Re: [PATCH V4 3/7] OPP: Add dev_pm_opp_xlate_performance_state() helper
Quoted 17 lines · 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 thedst_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 thesrc_tableas it is in such cases.
Quoted 2 lines
Tested-by Rajendra Nayak <[email protected]>Signed-off-by Viresh Kumar <[email protected]>
Kind regards
Uffe
Quoted 107 lines
--- drivers/opp/core.c | 63 ++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 7 +++++ 2 files changed, 70 insertions(+)drivers/opp/core.c +63
-0@@ -1713,6 +1713,69 @@ 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: Zero or positive performance state on success, otherwise negative +* value on errors. +*/ +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; +int dest_pstate = -EINVAL ; +int i; + +if (!pstate) +return 0 ; + +/* +* 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 -EINVAL ; + } + +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 include/linux/pm_opp.h +7
-1@@ -128,6 +128,7 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*svoid 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); +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 int dev_pm_opp_xlate_performance_state (struct opp_table *src_table,struct opp_table *dst_table,unsigned int pstate) +{ +return -ENOTSUPP ; +} +static inline int dev_pm_opp_set_rate (struct device *dev,unsigned long target_freq) {return -ENOTSUPP ; --2.19.1.568.g152ad8e3369a
PATCH v4 4/7 OPP: Don't return 0 on error from of_get_required_opp_performance_state()
In reply to Viresh Kumarof_get_required_opp_performance_state() returns 0 on errors currently and a positive performance state otherwise. Since 0 is a valid performance state (representing off), it would be better if this routine returns negative values on error.
That will also make it behave similar to dev_pm_opp_xlate_performance_state(), which also returns performance states and returns negative values on error. Change the return type of the function to "int" in order to return negative values.
This doesn't have any users for now and so no other part of the kernel will be impacted with this change.
--- drivers/opp/of.c | 10 +++++----- include/linux/pm_opp.h | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-)
drivers/opp/of.c +5 -5
@@ -995,19 +995,19 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_get_sharing_cpus);* Returns the performance state of the OPP pointed out by the "required-opps" * property at @index in @np. * -* Return: Positive performance state on success, otherwise 0 on errors. +* Return: Zero or positive performance state on success, otherwise negative +* value on errors. */ -unsigned int of_get_required_opp_performance_state (struct device_node *np, -int index) +int of_get_required_opp_performance_state (struct device_node *np,int index) {struct dev_pm_opp *opp;struct device_node *required_np;struct opp_table *opp_table; -unsigned int pstate =0 ; +int pstate = -EINVAL ; required_np =of_parse_required_opp (np, index);if (!required_np) -return 0 ; +return -EINVAL ; opp_table =_find_table_of_opp_np (required_np);if (IS_ERR (opp_table)) {
include/linux/pm_opp.h +3 -3
@@ -321,7 +321,7 @@ void dev_pm_opp_of_cpumask_remove_table(const struct cpumask *cpumask);int dev_pm_opp_of_get_sharing_cpus (struct device *cpu_dev,struct cpumask *cpumask);struct device_node *dev_pm_opp_of_get_opp_desc_node (struct device *dev);struct device_node *dev_pm_opp_get_of_node (struct dev_pm_opp *opp); -unsigned int of_get_required_opp_performance_state (struct device_node *np,int index); +int of_get_required_opp_performance_state (struct device_node *np,int index);#else static inline int dev_pm_opp_of_add_table (struct device *dev) {
@@ -360,9 +360,9 @@ static inline struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp) {return NULL ; } -static inline unsigned int of_get_required_opp_performance_state (struct device_node *np,int index) +static inline int of_get_required_opp_performance_state (struct device_node *np,int index) { -return 0 ; +return -ENOTSUPP ; }#endif
Signature · 1 line
2.19.1.568.g152ad8e3369a
Re: [PATCH V4 4/7] OPP: Don't return 0 on error from of_get_required_opp_performance_state()
Quoted 11 lines · Viresh Kumar
of_get_required_opp_performance_state()returns 0 on errors currently and a positive performance state otherwise. Since 0 is a valid performance state (representing off), it would be better if this routine returns negative values on error.That will also make it behave similar to
dev_pm_opp_xlate_performance_state(), which also returns performance states and returns negative values on error. Change the return type of the function to "int" in order to return negative values.
Quoted 4 lines
This doesn't have any users for now and so no other part of the kernel will be impacted with this change.
Signed-off-by Viresh Kumar <[email protected]>
Kind regards
Uffe
Quoted 62 lines
--- drivers/opp/of.c | 10 +++++----- include/linux/pm_opp.h | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-)drivers/opp/of.c +5
-5@@ -995,19 +995,19 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_get_sharing_cpus);* Returns the performance state of the OPP pointed out by the "required-opps" * property at @index in @np. * -* Return: Positive performance state on success, otherwise 0 on errors. +* Return: Zero or positive performance state on success, otherwise negative +* value on errors. */ -unsigned int of_get_required_opp_performance_state (struct device_node *np, -int index) +int of_get_required_opp_performance_state (struct device_node *np,int index) {struct dev_pm_opp *opp;struct device_node *required_np;struct opp_table *opp_table; -unsigned int pstate =0 ; +int pstate = -EINVAL ; required_np =of_parse_required_opp (np, index);if (!required_np) -return 0 ; +return -EINVAL ; opp_table =_find_table_of_opp_np (required_np);if (IS_ERR (opp_table)) {include/linux/pm_opp.h +3
-4@@ -321,7 +321,7 @@ void dev_pm_opp_of_cpumask_remove_table(const struct cpumask *cpumask);int dev_pm_opp_of_get_sharing_cpus (struct device *cpu_dev,struct cpumask *cpumask);struct device_node *dev_pm_opp_of_get_opp_desc_node (struct device *dev);struct device_node *dev_pm_opp_get_of_node (struct dev_pm_opp *opp); -unsigned int of_get_required_opp_performance_state (struct device_node *np,int index); +int of_get_required_opp_performance_state (struct device_node *np,int index);#else static inline int dev_pm_opp_of_add_table (struct device *dev) {@@ -360,9 +360,9 @@ static inline struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp) {return NULL ; } -static inline unsigned int of_get_required_opp_performance_state (struct device_node *np,int index) +static inline int of_get_required_opp_performance_state (struct device_node *np,int index) { -return 0 ; +return -ENOTSUPP ; }#endif --2.19.1.568.g152ad8e3369a
PATCH v4 5/7 PM / Domains: Save OPP table pointer in genpd
In reply to Viresh Kumardev_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.
--- drivers/base/power/domain.c | 23 +++++++++++++++++++++-- include/linux/pm_domain.h | 2 ++ 2 files changed, 23 insertions(+), 2 deletions(-)
drivers/base/power/domain.c +21 -2
@@ -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); } }
include/linux/pm_domain.h +2 -0
@@ -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,
Signature · 1 line
2.19.1.568.g152ad8e3369a
Re: [PATCH V4 5/7] PM / Domains: Save OPP table pointer in genpd
Quoted 7 lines · Viresh Kumar
dev_pm_genpd_set_performance_state()will be required to calldev_pm_opp_xlate_performance_state()going forward to translate from performance state of a sub-domain to performance state of its master. Anddev_pm_opp_xlate_performance_state()needs pointers to the OPP tables of both genpd and its master.
Quoted 6 lines
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 <[email protected]>Signed-off-by Viresh Kumar <[email protected]>
Kind regards
Uffe
Quoted 89 lines
--- drivers/base/power/domain.c | 23 +++++++++++++++++++++-- include/linux/pm_domain.h | 2 ++ 2 files changed, 23 insertions(+), 2 deletions(-)drivers/base/power/domain.c +21
-2@@ -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); } }include/linux/pm_domain.h +2
-1@@ -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 v4 6/7 PM / Domains: Factorize dev_pm_genpd_set_performance_state()
In reply to Viresh KumarSeparate 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.
--- drivers/base/power/domain.c | 95 +++++++++++++++++++++---------------- 1 file changed, 55 insertions(+), 40 deletions(-)
drivers/base/power/domain.c +55 -40
@@ -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;
Signature · 1 line
2.19.1.568.g152ad8e3369a
Re: [PATCH V4 6/7] PM / Domains: Factorize dev_pm_genpd_set_performance_state()
Quoted 8 lines · Viresh Kumar
Separate out
_genpd_set_performance_state()and_genpd_reeval_performance_state()fromdev_pm_genpd_set_performance_state()to handle performance state update related stuff. This will be used by a later commit.Tested-by Rajendra Nayak <[email protected]>Signed-off-by Viresh Kumar <[email protected]>
Nitpick: Would be nice to remove the beginning "_" from both _genpd_reeval_performance_state() and _genpd_set_performance_state(), as to be consistent with existing function naming in genpd. However, it's not a big deal and can be done on top.
Quoted 130 lines
--- drivers/base/power/domain.c | 95 +++++++++++++++++++++---------------- 1 file changed, 55 insertions(+), 40 deletions(-)drivers/base/power/domain.c +55
-41@@ -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; --2.19.1.568.g152ad8e3369a
Re: [PATCH V4 6/7] PM / Domains: Factorize dev_pm_genpd_set_performance_state()
Quoted 4 lines · Ulf Hansson
Nitpick: Would be nice to remove the beginning "_" from both
_genpd_reeval_performance_state()and_genpd_set_performance_state(), as to be consistent with existing function naming in genpd. However, it's not a big deal and can be done on top.
Actually it is used at some places and so I did the same for the internal routines:
_genpd_power_on_genpd_power_off__genpd_runtime_suspend__genpd_runtime_resume__genpd_dev_pm_attach
Signature · 1 line
viresh
PATCH v4 7/7 PM / Domains: Propagate performance state updates
In reply to Viresh KumarCurrently 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.
--- drivers/base/power/domain.c | 93 ++++++++++++++++++++++++++++++++----- include/linux/pm_domain.h | 4 ++ 2 files changed, 86 insertions(+), 11 deletions(-)
drivers/base/power/domain.c +82 -11
@@ -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;
include/linux/pm_domain.h +4 -0
@@ -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 {
Signature · 1 line
2.19.1.568.g152ad8e3369a
Re: [PATCH V4 7/7] PM / Domains: Propagate performance state updates
Quoted 11 lines · Viresh Kumar
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 structgpd_linkalso got another fieldprev_performance_state, which is used by genpd core as a temporary variable during transitions.
Quoted 8 lines
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 ofdev_pm_opp_xlate_performance_state(), which looks at the OPP tables of both the domains to translate the state.
Perfect! Thanks for clarifying these things in the changelog!
Quoted 2 lines
Tested-by Rajendra Nayak <[email protected]>Signed-off-by Viresh Kumar <[email protected]>
Kind regards
Uffe
Quoted 156 lines
--- drivers/base/power/domain.c | 93 ++++++++++++++++++++++++++++++++----- include/linux/pm_domain.h | 4 ++ 2 files changed, 86 insertions(+), 11 deletions(-)drivers/base/power/domain.c +82
-11@@ -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;include/linux/pm_domain.h +4
-1@@ -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
Rafael J. Wysocki
maintainerQuoted 13 lines · 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).
Quoted 5 lines
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'm assuming that this set will go in via the OPP tree.
Cheers,
Rafael
Quoted 14 lines · Rafael J. Wysocki
Quoted 13 lines · 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).
Quoted 7 lines
Quoted 5 lines
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'm assuming that this set will go in via the OPP tree.
Yes.
Signature · 1 line
viresh
Quoting Viresh Kumar (2018-12-14 02:15:26)
Quoted 6 lines
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.
Quoted 9 lines
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).
For the series