OPP: Simplify required-opp handling
Subsystems (2)
v2 cover OPP: Simplify required-opp handling
Hello,
I wasn't able to test this locally (despite trying to hack it around) and need help from someone who is virt_devs field of struct dev_pm_opp_config.
Pushed here:
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/required-opps
-------------------------8<-------------------------
Configuring the required OPP was never properly implemented, we just took an exception for genpds and configured them directly, while leaving out all other required OPP types.
Now that a standard call to dev_pm_opp_set_opp() takes care of configuring the opp->level too, the special handling for genpds can be avoided by simply calling dev_pm_opp_set_opp() for the required OPPs, which shall eventually configure the corresponding level for genpds.
This also makes it possible for us to configure other type of required OPPs (no concrete users yet though), via the same path. This is how other frameworks take care of parent nodes, like clock, regulators, etc, where we recursively call the same helper.
V1->V2:
- Support opp-level 0, drop vote i.e..
- Fix OPP pointer while calling dev_pm_opp_set_opp() recursively.
- Minor checks and fixes.
- Add Reviewed-by from Ulf.
--
Viresh
Viresh Kumar (3):
OPP: Level zero is valid
OPP: Use _set_opp_level() for single genpd case
OPP: Call dev_pm_opp_set_opp() for required OPPs
drivers/opp/core.c | 176 ++++++++++++++++++++++------------------- drivers/opp/of.c | 48 ++++++++--- drivers/opp/opp.h | 8 +- include/linux/pm_opp.h | 12 ++- 4 files changed, 145 insertions(+), 99 deletions(-)
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 1/3 OPP: Level zero is valid
The level zero can be used by some OPPs to drop performance state vote for the device. It is perfectly fine to allow the same.
_set_opp_level() considers it as an invalid value currently and returns early.
In order to support this properly, initialize the level field with U32_MAX, which denotes unused level field.
--- drivers/opp/core.c | 24 ++++++++++++++++++++---- drivers/opp/of.c | 8 +++++++- include/linux/pm_opp.h | 5 ++++- 3 files changed, 31 insertions(+), 6 deletions(-)
drivers/opp/core.c +20 -4
@@ -201,7 +201,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq_indexed);* @opp: opp for which level value has to be returned for * * Return: level read from device tree corresponding to the opp, else -* return 0. +* return U32_MAX. */ unsigned int dev_pm_opp_get_level (struct dev_pm_opp *opp) {
@@ -221,7 +221,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_level);* @index: index of the required opp * * Return: performance state read from device tree corresponding to the -* required opp, else return 0. +* required opp, else return U32_MAX. */ unsigned int dev_pm_opp_get_required_pstate (struct dev_pm_opp *opp,unsigned int index)
@@ -808,6 +808,14 @@ struct dev_pm_opp *dev_pm_opp_find_level_ceil(struct device *dev,struct dev_pm_opp *opp; opp =_find_key_ceil (dev, &temp,0 , true, _read_level,NULL ); + +/* False match */ +if (temp ==OPP_LEVEL_UNSET ) { +dev_err (dev,"%s: OPP levels aren't available\n" , __func__); +dev_pm_opp_put (opp); +return ERR_PTR (-ENODEV ); + } + *level = temp;return opp; }
@@ -1049,12 +1057,18 @@ static int _set_opp_bw(const struct opp_table *opp_table,static int _set_performance_state (struct device *dev,struct device *pd_dev,struct dev_pm_opp *opp,int i) { -unsigned int pstate =likely (opp) ? opp->required_opps[i]->level:0 ; +unsigned int pstate =0 ;int ret;if (!pd_dev)return 0 ; +if (likely (opp)) { + pstate = opp->required_opps[i]->level; +if (pstate ==OPP_LEVEL_UNSET ) +return 0 ; + } + ret =dev_pm_domain_set_performance_state (pd_dev, pstate);if (ret) { dev_err(dev, "Failed to set performance state of %s: %d (%d)\n",
@@ -1135,7 +1149,7 @@ static int _set_opp_level(struct device *dev, struct opp_table *opp_table,int ret =0 ;if (opp) { -if (!opp->level) +if (opp->level ==OPP_LEVEL_UNSET )return 0 ; level = opp->level;
@@ -1867,6 +1881,8 @@ struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table)INIT_LIST_HEAD (&opp->node); + opp->level =OPP_LEVEL_UNSET ; +return opp; }
drivers/opp/of.c +7 -1
@@ -1393,8 +1393,14 @@ int of_get_required_opp_performance_state(struct device_node *np, int index) opp =_find_opp_of_np (opp_table, required_np);if (opp) { - pstate = opp->level; +if (opp->level ==OPP_LEVEL_UNSET ) { +pr_err ("%s: OPP levels aren't available for %pOF\n" , + __func__, np); + }else { + pstate = opp->level; + }dev_pm_opp_put (opp); + }dev_pm_opp_put_opp_table (opp_table);
include/linux/pm_opp.h +4 -1
@@ -92,9 +92,12 @@ struct dev_pm_opp_config {struct device ***virt_devs; }; +#define OPP_LEVEL_UNSET U32_MAX +/** * struct dev_pm_opp_data - The data to use to initialize an OPP. -* @level: The performance level for the OPP. +* @level: The performance level for the OPP. Set level to OPP_LEVEL_UNSET if +* level field isn't used. * @freq: The clock rate in Hz for the OPP. * @u_volt: The voltage in uV for the OPP. */
Signature · 1 line
2.31.1.272.g89b43f80a514
Re: [PATCH V2 1/3] OPP: Level zero is valid
Quoted 5 lines · Viresh Kumar
The level zero can be used by some OPPs to drop performance state vote for the device. It is perfectly fine to allow the same.
_set_opp_level()considers it as an invalid value currently and returns early.
So, currently if the device is PM-enabled, but OPP requirements are lifted, is the API currently internally stuck at the last non-zero level?
Just trying to understand if this could fix some silent issues
Konrad
Re: [PATCH V2 1/3] OPP: Level zero is valid
Quoted 8 lines · Konrad Dybcio
Quoted 5 lines · Viresh Kumar
The level zero can be used by some OPPs to drop performance state vote for the device. It is perfectly fine to allow the same.
_set_opp_level()considers it as an invalid value currently and returns early.So, currently if the device is PM-enabled, but OPP requirements are lifted,
How exactly are the OPP requirements lifted ?
By calling dev_pm_opp_set_opp(dev, NULL) ?
This will work fine even without this commit.
Quoted 3 lines
is the API currently internally stuck at the last non-zero level?
Just trying to understand if this could fix some silent issues
Also the issue I am trying to solve here came in existence only during the 6.7 merge window and doesn't affect the genpds linked via required-opps. And this commit will soon be merged.
Signature · 1 line
viresh
Re: [PATCH V2 1/3] OPP: Level zero is valid
Quoted 8 lines · Viresh Kumar
Quoted 6 lines · Konrad Dybcio
Quoted 5 lines · Viresh Kumar
The level zero can be used by some OPPs to drop performance state vote for the device. It is perfectly fine to allow the same.
_set_opp_level()considers it as an invalid value currently and returns early.
Quoted 7 lines
Quoted 1 line
So, currently if the device is PM-enabled, but OPP requirements are lifted,
How exactly are the OPP requirements lifted ?
By calling
dev_pm_opp_set_opp(dev, NULL)?This will work fine even without this commit.
Ok!
Quoted 8 lines
Quoted 4 lines
is the API currently internally stuck at the last non-zero level?
Just trying to understand if this could fix some silent issues
Also the issue I am trying to solve here came in existence only during the 6.7 merge window and doesn't affect the genpds linked via required-opps. And this commit will soon be merged.
Ack, thanks
Konrad
Re: [PATCH V2 1/3] OPP: Level zero is valid
In reply to Viresh KumarQuoted 4 lines · Viresh Kumar
The level zero can be used by some OPPs to drop performance state vote for the device. It is perfectly fine to allow the same.
Quoted 8 lines
_set_opp_level()considers it as an invalid value currently and returns early.In order to support this properly, initialize the level field with
U32_MAX, which denotes unused level field.Reported-by Stephan Gerhold <[email protected]>Signed-off-by Viresh Kumar <[email protected]>
Kind regards
Uffe
Quoted 122 lines
--- drivers/opp/core.c | 24 ++++++++++++++++++++---- drivers/opp/of.c | 8 +++++++- include/linux/pm_opp.h | 5 ++++- 3 files changed, 31 insertions(+), 6 deletions(-)drivers/opp/core.c +20
-4@@ -201,7 +201,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq_indexed);* @opp: opp for which level value has to be returned for * * Return: level read from device tree corresponding to the opp, else -* return 0. +* return U32_MAX. */ unsigned int dev_pm_opp_get_level (struct dev_pm_opp *opp) {@@ -221,7 +221,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_level);* @index: index of the required opp * * Return: performance state read from device tree corresponding to the -* required opp, else return 0. +* required opp, else return U32_MAX. */ unsigned int dev_pm_opp_get_required_pstate (struct dev_pm_opp *opp,unsigned int index)@@ -808,6 +808,14 @@ struct dev_pm_opp *dev_pm_opp_find_level_ceil(struct device *dev,struct dev_pm_opp *opp; opp =_find_key_ceil (dev, &temp,0 , true, _read_level,NULL ); + +/* False match */ +if (temp ==OPP_LEVEL_UNSET ) { +dev_err (dev,"%s: OPP levels aren't available\n" , __func__); +dev_pm_opp_put (opp); +return ERR_PTR (-ENODEV ); + } + *level = temp;return opp; }@@ -1049,12 +1057,18 @@ static int _set_opp_bw(const struct opp_table *opp_table,static int _set_performance_state (struct device *dev,struct device *pd_dev,struct dev_pm_opp *opp,int i) { -unsigned int pstate =likely (opp) ? opp->required_opps[i]->level:0 ; +unsigned int pstate =0 ;int ret;if (!pd_dev)return 0 ; +if (likely (opp)) { + pstate = opp->required_opps[i]->level; +if (pstate ==OPP_LEVEL_UNSET ) +return 0 ; + } + ret =dev_pm_domain_set_performance_state (pd_dev, pstate);if (ret) { dev_err(dev, "Failed to set performance state of %s: %d (%d)\n",@@ -1135,7 +1149,7 @@ static int _set_opp_level(struct device *dev, struct opp_table *opp_table,int ret =0 ;if (opp) { -if (!opp->level) +if (opp->level ==OPP_LEVEL_UNSET )return 0 ; level = opp->level;@@ -1867,6 +1881,8 @@ struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table)INIT_LIST_HEAD (&opp->node); + opp->level =OPP_LEVEL_UNSET ; +return opp; }drivers/opp/of.c +7
-1@@ -1393,8 +1393,14 @@ int of_get_required_opp_performance_state(struct device_node *np, int index) opp =_find_opp_of_np (opp_table, required_np);if (opp) { - pstate = opp->level; +if (opp->level ==OPP_LEVEL_UNSET ) { +pr_err ("%s: OPP levels aren't available for %pOF\n" , + __func__, np); + }else { + pstate = opp->level; + }dev_pm_opp_put (opp); + }dev_pm_opp_put_opp_table (opp_table);include/linux/pm_opp.h +4
-2@@ -92,9 +92,12 @@ struct dev_pm_opp_config {struct device ***virt_devs; }; +#define OPP_LEVEL_UNSET U32_MAX +/** * struct dev_pm_opp_data - The data to use to initialize an OPP. -* @level: The performance level for the OPP. +* @level: The performance level for the OPP. Set level to OPP_LEVEL_UNSET if +* level field isn't used. * @freq: The clock rate in Hz for the OPP. * @u_volt: The voltage in uV for the OPP. */ --2.31.1.272.g89b43f80a514
PATCH v2 2/3 OPP: Use _set_opp_level() for single genpd case
In reply to Viresh KumarThere are two genpd (as required-opp) cases that we need to handle, devices with a single genpd and ones with multiple genpds.
The multiple genpds case is clear, where the OPP core calls dev_pm_domain_attach_by_name() for them and uses the virtual devices returned by this helper to call dev_pm_domain_set_performance_state() later to change the performance state.
The single genpd case however requires special handling as we need to use the same dev structure (instead of a virtual one provided by genpd core) for setting the performance state via dev_pm_domain_set_performance_state().
As we move towards more generic code to take care of the required OPPs, where we will recursively call dev_pm_opp_set_opp() for all the required OPPs, the above special case becomes a problem.
Eventually we want to handle all performance state changes via _set_opp_level(), so lets move the single genpd case to that right away.
--- drivers/opp/core.c | 6 ++++-- drivers/opp/of.c | 25 ++++++++++++++++++++++--- 2 files changed, 26 insertions(+), 5 deletions(-)
drivers/opp/core.c +4 -2
@@ -1088,10 +1088,12 @@ static int _opp_set_required_opps_generic(struct device *dev,static int _opp_set_required_opps_genpd (struct device *dev,struct opp_table *opp_table,struct dev_pm_opp *opp,bool scaling_down) { -struct device **genpd_virt_devs = - opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev; +struct device **genpd_virt_devs = opp_table->genpd_virt_devs;int index, target, delta, ret; +if (!genpd_virt_devs) +return 0 ; +/* Scaling up? Set required OPPs in normal order, else reverse */ if (!scaling_down) { index =0 ;
drivers/opp/of.c +22 -3
@@ -296,7 +296,7 @@ void _of_clear_opp(struct opp_table *opp_table, struct dev_pm_opp *opp)of_node_put (opp->np); } -static int _link_required_opps (struct dev_pm_opp *opp, +static int _link_required_opps (struct dev_pm_opp *opp,struct opp_table *opp_table,struct opp_table *required_table,int index) {struct device_node *np;
@@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp,return -ENODEV ; } +/* +* There are two genpd (as required-opp) cases that we need to handle, +* devices with a single genpd and ones with multiple genpds. +* +* The single genpd case requires special handling as we need to use the +* same `dev` structure (instead of a virtual one provided by genpd +* core) for setting the performance state. Lets treat this as a case +* where the OPP's level is directly available without required genpd +* link in the DT. +* +* Just update the `level` with the right value, which +* dev_pm_opp_set_opp() will take care of in the normal path itself. +*/ +if (required_table->is_genpd && opp_table->required_opp_count ==1 && + !opp_table->genpd_virt_devs) { +if (!WARN_ON (opp->level !=OPP_LEVEL_UNSET )) + opp->level = opp->required_opps[0 ]->level; + } +return 0 ; }
@@ -338,7 +357,7 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,if (IS_ERR_OR_NULL (required_table))continue ; - ret =_link_required_opps (opp, required_table, i); + ret =_link_required_opps (opp, opp_table, required_table, i);if (ret) goto free_required_opps; }
@@ -359,7 +378,7 @@ static int lazy_link_required_opps(struct opp_table *opp_table,int ret;list_for_each_entry (opp, &opp_table->opp_list, node) { - ret =_link_required_opps (opp, new_table, index); + ret =_link_required_opps (opp, opp_table, new_table, index);if (ret)return ret; }
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 3/3 OPP: Call dev_pm_opp_set_opp() for required OPPs
In reply to Viresh KumarConfiguring the required OPP was never properly implemented, we just took an exception for genpds and configured them directly, while leaving out all other required OPP types.
Now that a standard call to dev_pm_opp_set_opp() takes care of configuring the opp->level too, the special handling for genpds can be avoided by simply calling dev_pm_opp_set_opp() for the required OPPs, which shall eventually configure the corresponding level for genpds.
This also makes it possible for us to configure other type of required OPPs (no concrete users yet though), via the same path. This is how other frameworks take care of parent nodes, like clock, regulators, etc, where we recursively call the same helper.
In order to call dev_pm_opp_set_opp() for the virtual genpd devices, they must share the OPP table of the genpd. Call _add_opp_dev() for them to get that done.
This commit also extends the struct dev_pm_opp_config to pass required devices, for non-genpd cases, which can be used to call dev_pm_opp_set_opp() for the non-genpd required devices.
--- drivers/opp/core.c | 168 ++++++++++++++++++++--------------------- drivers/opp/of.c | 17 +++-- drivers/opp/opp.h | 8 +- include/linux/pm_opp.h | 7 +- 4 files changed, 100 insertions(+), 100 deletions(-)
drivers/opp/core.c +82 -86
@@ -1054,48 +1054,22 @@ static int _set_opp_bw(const struct opp_table *opp_table,return 0 ; } -static int _set_performance_state (struct device *dev,struct device *pd_dev, -struct dev_pm_opp *opp,int i) -{ -unsigned int pstate =0 ; -int ret; - -if (!pd_dev) -return 0 ; - -if (likely (opp)) { - pstate = opp->required_opps[i]->level; -if (pstate ==OPP_LEVEL_UNSET ) -return 0 ; - } - - ret =dev_pm_domain_set_performance_state (pd_dev, pstate); -if (ret) { -dev_err (dev,"Failed to set performance state of %s: %d (%d)\n" , -dev_name (pd_dev), pstate, ret); - } - -return ret; -} - -static int _opp_set_required_opps_generic (struct device *dev, -struct opp_table *opp_table,struct dev_pm_opp *opp,bool scaling_down) -{ -dev_err (dev,"setting required-opps isn't supported for non-genpd devices\n" ); -return -ENOENT ; -} - -static int _opp_set_required_opps_genpd (struct device *dev, -struct opp_table *opp_table,struct dev_pm_opp *opp,bool scaling_down) +/* This is only called for PM domain for now */ +static int _set_required_opps (struct device *dev,struct opp_table *opp_table, +struct dev_pm_opp *opp,bool up) { -struct device **genpd_virt_devs = opp_table->genpd_virt_devs; +struct device **devs = opp_table->required_devs;int index, target, delta, ret; -if (!genpd_virt_devs) +if (!devs)return 0 ; +/* required-opps not fully initialized yet */ +if (lazy_linking_pending (opp_table)) +return -EBUSY ; +/* Scaling up? Set required OPPs in normal order, else reverse */ -if (!scaling_down) { +if (up) { index =0 ; target = opp_table->required_opp_count; delta =1 ;
@@ -1106,9 +1080,11 @@ static int _opp_set_required_opps_genpd(struct device *dev, }while (index != target) { - ret =_set_performance_state (dev, genpd_virt_devs[index], opp, index); -if (ret) -return ret; +if (devs[index]) { + ret =dev_pm_opp_set_opp (devs[index], opp->required_opps[index]); +if (ret) +return ret; + } index += delta; }
@@ -1116,34 +1092,6 @@ static int _opp_set_required_opps_genpd(struct device *dev,return 0 ; } -/* This is only called for PM domain for now */ -static int _set_required_opps (struct device *dev,struct opp_table *opp_table, -struct dev_pm_opp *opp,bool up) -{ -/* required-opps not fully initialized yet */ -if (lazy_linking_pending (opp_table)) -return -EBUSY ; - -if (opp_table->set_required_opps) -return opp_table->set_required_opps (dev, opp_table, opp, up); - -return 0 ; -} - -/* Update set_required_opps handler */ -void _update_set_required_opps (struct opp_table *opp_table) -{ -/* Already set */ -if (opp_table->set_required_opps) -return ; - -/* All required OPPs will belong to genpd or none */ -if (opp_table->required_opp_tables[0 ]->is_genpd) - opp_table->set_required_opps = _opp_set_required_opps_genpd; -else - opp_table->set_required_opps = _opp_set_required_opps_generic; -} -static int _set_opp_level (struct device *dev,struct opp_table *opp_table,struct dev_pm_opp *opp) {
@@ -2406,19 +2354,13 @@ static void _opp_detach_genpd(struct opp_table *opp_table) {int index; -if (!opp_table->genpd_virt_devs) -return ; -for (index =0 ; index < opp_table->required_opp_count; index++) { -if (!opp_table->genpd_virt_devs[index]) +if (!opp_table->required_devs[index])continue ; -dev_pm_domain_detach (opp_table->genpd_virt_devs[index], false); - opp_table->genpd_virt_devs[index] =NULL ; +dev_pm_domain_detach (opp_table->required_devs[index], false); + opp_table->required_devs[index] =NULL ; } - -kfree (opp_table->genpd_virt_devs); - opp_table->genpd_virt_devs =NULL ; }/*
@@ -2445,14 +2387,14 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,int index =0 , ret = -EINVAL ;const char *const *name = names; -if (opp_table->genpd_virt_devs) -return 0 ; +if (!opp_table->required_devs) { +dev_err (dev,"Required OPPs not available, can't attach genpd\n" ); +return -EINVAL ; + } - opp_table->genpd_virt_devs =kcalloc (opp_table->required_opp_count, -sizeof (*opp_table->genpd_virt_devs), -GFP_KERNEL ); -if (!opp_table->genpd_virt_devs) -return -ENOMEM ; +/* Checking only the first one is enough ? */ +if (opp_table->required_devs[0 ]) +return 0 ;while (*name) {if (index >= opp_table->required_opp_count) {
@@ -2468,13 +2410,25 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, goto err; } - opp_table->genpd_virt_devs[index] = virt_dev; +/* +* Add the virtual genpd device as a user of the OPP table, so +* we can call dev_pm_opp_set_opp() on it directly. +* +* This will be automatically removed when the OPP table is +* removed, don't need to handle that here. +*/ +if (!_add_opp_dev (virt_dev, opp_table->required_opp_tables[index])) { + ret = -ENOMEM ; + goto err; + } + + opp_table->required_devs[index] = virt_dev; index++; name++; }if (virt_devs) - *virt_devs = opp_table->genpd_virt_devs; + *virt_devs = opp_table->required_devs;return 0 ;
@@ -2484,10 +2438,42 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, } +static int _opp_set_required_devs (struct opp_table *opp_table, +struct device *dev, +struct device **required_devs) +{ +int i; + +if (!opp_table->required_devs) { +dev_err (dev,"Required OPPs not available, can't set required devs\n" ); +return -EINVAL ; + } + +/* Another device that shares the OPP table has set the required devs ? */ +if (opp_table->required_devs[0 ]) +return 0 ; + +for (i =0 ; i < opp_table->required_opp_count; i++) + opp_table->required_devs[i] = required_devs[i]; + +return 0 ; +} + +static void _opp_put_required_devs (struct opp_table *opp_table) +{ +int i; + +for (i =0 ; i < opp_table->required_opp_count; i++) + opp_table->required_devs[i] =NULL ; +} +static void _opp_clear_config (struct opp_config_data *data) { -if (data->flags &OPP_CONFIG_GENPD ) +if (data->flags &OPP_CONFIG_REQUIRED_DEVS ) +_opp_put_required_devs (data->opp_table); +else if (data->flags &OPP_CONFIG_GENPD )_opp_detach_genpd (data->opp_table); +if (data->flags &OPP_CONFIG_REGULATOR )_opp_put_regulators (data->opp_table);if (data->flags &OPP_CONFIG_SUPPORTED_HW )
@@ -2601,12 +2587,22 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)/* Attach genpds */ if (config->genpd_names) { +if (config->required_devs) + goto err; + ret =_opp_attach_genpd (opp_table, dev, config->genpd_names, config->virt_devs);if (ret) goto err; data->flags |=OPP_CONFIG_GENPD ; + }else if (config->required_devs) { + ret =_opp_set_required_devs (opp_table, dev, + config->required_devs); +if (ret) + goto err; + + data->flags |=OPP_CONFIG_REQUIRED_DEVS ; } ret = xa_alloc(&opp_configs, &id, data, XA_LIMIT(1, INT_MAX),
drivers/opp/of.c +10 -7
@@ -165,7 +165,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,struct opp_table **required_opp_tables;struct device_node *required_np, *np;bool lazy = false; -int count, i; +int count, i, size;/* Traversing the first OPP node is all we need */ np =of_get_next_available_child (opp_np,NULL );
@@ -179,12 +179,13 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,if (count <=0 ) goto put_np; - required_opp_tables =kcalloc (count,sizeof (*required_opp_tables), -GFP_KERNEL ); + size =sizeof (*required_opp_tables) +sizeof (*opp_table->required_devs); + required_opp_tables =kcalloc (count, size,GFP_KERNEL );if (!required_opp_tables) goto put_np; opp_table->required_opp_tables = required_opp_tables; + opp_table->required_devs = (void *)(required_opp_tables + count); opp_table->required_opp_count = count;for (i =0 ; i < count; i++) {
@@ -208,8 +209,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,mutex_lock (&opp_table_lock);list_add (&opp_table->lazy, &lazy_opp_tables);mutex_unlock (&opp_table_lock); - }else { -_update_set_required_opps (opp_table); } goto put_np;
@@ -326,9 +325,14 @@ static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_tab* * Just update the `level` with the right value, which * dev_pm_opp_set_opp() will take care of in the normal path itself. +* +* There is another case though, where a genpd's OPP table has +* required-opps set to a parent genpd. The OPP core expects the user to +* set the respective required `struct device` pointer via +* dev_pm_opp_set_config(). */ if (required_table->is_genpd && opp_table->required_opp_count ==1 && - !opp_table->genpd_virt_devs) { + !opp_table->required_devs[0 ]) {if (!WARN_ON (opp->level !=OPP_LEVEL_UNSET )) opp->level = opp->required_opps[0 ]->level; }
@@ -441,7 +445,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)/* All required opp-tables found, remove from lazy list */ if (!lazy) { -_update_set_required_opps (opp_table);list_del_init (&opp_table->lazy);list_for_each_entry (opp, &opp_table->opp_list, node)
drivers/opp/opp.h +3 -5
@@ -35,6 +35,7 @@ extern struct list_head opp_tables;#define OPP_CONFIG_PROP_NAME BIT(3)#define OPP_CONFIG_SUPPORTED_HW BIT(4)#define OPP_CONFIG_GENPD BIT(5) +#define OPP_CONFIG_REQUIRED_DEVS BIT(6)/** * struct opp_config_data - data for set config operations
@@ -160,9 +161,9 @@ enum opp_table_access {* @rate_clk_single: Currently configured frequency for single clk. * @current_opp: Currently configured OPP for the table. * @suspend_opp: Pointer to OPP to be used during device suspend. -* @genpd_virt_devs: List of virtual devices for multiple genpd support. * @required_opp_tables: List of device OPP tables that are required by OPPs in * this table. +* @required_devs: List of devices for required OPP tables. * @required_opp_count: Number of required devices. * @supported_hw: Array of version number to support. * @supported_hw_count: Number of elements in supported_hw array.
@@ -180,7 +181,6 @@ enum opp_table_access {* @path_count: Number of interconnect paths * @enabled: Set to true if the device's resources are enabled/configured. * @is_genpd: Marks if the OPP table belongs to a genpd. -* @set_required_opps: Helper responsible to set required OPPs. * @dentry: debugfs dentry pointer of the real device directory (not links). * @dentry_name: Name of the real dentry. *
@@ -211,8 +211,8 @@ struct opp_table {struct dev_pm_opp *current_opp;struct dev_pm_opp *suspend_opp; -struct device **genpd_virt_devs;struct opp_table **required_opp_tables; +struct device **required_devs;unsigned int required_opp_count;unsigned int *supported_hw;
@@ -229,8 +229,6 @@ struct opp_table {unsigned int path_count;bool enabled;bool is_genpd; -int (*set_required_opps)(struct device *dev, -struct opp_table *opp_table,struct dev_pm_opp *opp,bool scaling_down);#ifdef CONFIG_DEBUG_FS struct dentry *dentry;
include/linux/pm_opp.h +5 -2
@@ -74,8 +74,10 @@ typedef int (*config_clks_t)(struct device *dev, struct opp_table *opp_table,* @supported_hw_count: Number of elements in the array. * @regulator_names: Array of pointers to the names of the regulator, NULL terminated. * @genpd_names: Null terminated array of pointers containing names of genpd to -* attach. -* @virt_devs: Pointer to return the array of virtual devices. +* attach. Mutually exclusive with required_devs. +* @virt_devs: Pointer to return the array of genpd virtual devices. Mutually +* exclusive with required_devs. +* @required_devs: Required OPP devices. Mutually exclusive with genpd_names/virt_devs. * * This structure contains platform specific OPP configurations for the device. */
@@ -90,6 +92,7 @@ struct dev_pm_opp_config {const char *const *regulator_names;const char *const *genpd_names;struct device ***virt_devs; +struct device **required_devs; };#define OPP_LEVEL_UNSET U32_MAX
Signature · 1 line
2.31.1.272.g89b43f80a514
Quoted 7 lines · Viresh Kumar
Hello,
I wasn't able to test this locally (despite trying to hack it around) and need help from someone who is
virt_devsfield ofstruct dev_pm_opp_config.Pushed here:
Quoted 7 lines
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.gitopp/required-opps
V1->V2:
- Support opp-level 0, drop vote i.e..
- Fix OPP pointer while callingdev_pm_opp_set_opp()recursively.
- Minor checks and fixes.
- Add Reviewed-by from Ulf.
Stephan, Ulf,
Any feedback on this before I merge it ?
Signature · 1 line
viresh
Quoted 17 lines · Viresh Kumar
Quoted 7 lines · Viresh Kumar
Hello,
I wasn't able to test this locally (despite trying to hack it around) and need help from someone who is
virt_devsfield ofstruct dev_pm_opp_config.Pushed here:
Quoted 7 lines
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.gitopp/required-opps
V1->V2:
- Support opp-level 0, drop vote i.e..
- Fix OPP pointer while callingdev_pm_opp_set_opp()recursively.
- Minor checks and fixes.
- Add Reviewed-by from Ulf.
Quoted 3 lines
Stephan, Ulf,
Any feedback on this before I merge it ?
I intend to review it within the next couple of days - or at least before the merge window gets closed.
Kind regards
Uffe
Quoted 14 lines · Viresh Kumar
Quoted 5 lines · Viresh Kumar
I wasn't able to test this locally (despite trying to hack it around) and need help from someone who is
virt_devsfield ofstruct dev_pm_opp_config.Pushed here:
Quoted 7 lines
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.gitopp/required-opps
V1->V2:
- Support opp-level 0, drop vote i.e..
- Fix OPP pointer while callingdev_pm_opp_set_opp()recursively.
- Minor checks and fixes.
- Add Reviewed-by from Ulf.
Quoted 4 lines
Stephan, Ulf,
Any feedback on this before I merge it ?
Sorry for the delay. I tested this successfully on the MSM8909 board on Wednesday (with the single genpd, and without opp-level 0 there), but until now didn't find time to test it on the MSM8916 board with the multiple genpds and the opp-level 0.
The opp-level 0 works fine now, thanks for fixing that!
The warning in _link_required_opps() when using the parent genpd setup
[1] is still present though. Given that this setup is an existing feature in the genpd core I would appreciate if we try to find a solution before merging this patch set. It's kind of a regression otherwise since the warning isn't present without this patch set. Maybe someone else is already actively using such a setup.
Thanks!
Stephan
Quoted 4 lines · Stephan Gerhold
Sorry for the delay. I tested this successfully on the MSM8909 board on Wednesday (with the single genpd, and without opp-level 0 there), but until now didn't find time to test it on the MSM8916 board with the multiple genpds and the opp-level 0.
Thanks.
Quoted 8 lines
The opp-level 0 works fine now, thanks for fixing that!
The warning in
_link_required_opps()when using the parent genpd setup
[1] is still present though. Given that this setup is an existing feature in the genpd core I would appreciate if we try to find a solution before merging this patch set. It's kind of a regression otherwise since the warning isn't present without this patch set. Maybe someone else is already actively using such a setup.
I did mention the solution that I seem fit for this case [1]. That's what I have in mind.
Signature · 3 lines
viresh [1] https://lore.kernel.org/all/20231030102944.nrw4bta467zxes5c@vireshk-i7/
Quoted 6 lines · Stephan Gerhold
The warning in
_link_required_opps()when using the parent genpd setup
[1] is still present though. Given that this setup is an existing feature in the genpd core I would appreciate if we try to find a solution before merging this patch set. It's kind of a regression otherwise since the warning isn't present without this patch set. Maybe someone else is already actively using such a setup.
Can you please try V3. The warning should be gone now.
Signature · 1 line
viresh