| Message ID | 20240618155013.323322-1-ulf.hansson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | OPP: Fix support for required OPPs for multiple PM domains | expand |
On 18-06-24, 17:50, Ulf Hansson wrote: > In _set_opp() we are normally bailing out when trying to set an OPP that is > the current one. This make perfect sense, but becomes a problem when > _set_required_opps() calls it recursively. > > More precisely, when a required OPP is being shared by multiple PM domains, > we end up skipping to request the corresponding performance-state for all > of the PM domains, but the first one. Let's fix the problem, by calling > _set_opp_level() from _set_required_opps() instead. > > Fixes: e37440e7e2c2 ("OPP: Call dev_pm_opp_set_opp() for required OPPs") > Cc: stable@vger.kernel.org > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/opp/core.c | 47 +++++++++++++++++++++++----------------------- > 1 file changed, 24 insertions(+), 23 deletions(-) > /* 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) > @@ -1091,7 +1113,8 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table, > if (devs[index]) { > required_opp = opp ? opp->required_opps[index] : NULL; > > - ret = dev_pm_opp_set_opp(devs[index], required_opp); > + ret = _set_opp_level(devs[index], opp_table, > + required_opp); No, we won't be doing this I guess. Its like going back instead of moving forward :) The required OPPs is not just a performance domain thing, but specially with devs[] here, it can be used to set OPP for any device type and so dev_pm_opp_set_opp() is the right call here. Coming back to the problem you are pointing to, I am not very clear of the whole picture here. Can you please help me get some details on that ?
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index cb4611fe1b5b..45eca65f27f9 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1061,6 +1061,28 @@ static int _set_opp_bw(const struct opp_table *opp_table, return 0; } +static int _set_opp_level(struct device *dev, struct opp_table *opp_table, + struct dev_pm_opp *opp) +{ + unsigned int level = 0; + int ret = 0; + + if (opp) { + if (opp->level == OPP_LEVEL_UNSET) + return 0; + + level = opp->level; + } + + /* Request a new performance state through the device's PM domain. */ + ret = dev_pm_domain_set_performance_state(dev, level); + if (ret) + dev_err(dev, "Failed to set performance state %u (%d)\n", level, + ret); + + return ret; +} + /* 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) @@ -1091,7 +1113,8 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table, if (devs[index]) { required_opp = opp ? opp->required_opps[index] : NULL; - ret = dev_pm_opp_set_opp(devs[index], required_opp); + ret = _set_opp_level(devs[index], opp_table, + required_opp); if (ret) return ret; } @@ -1102,28 +1125,6 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table, return 0; } -static int _set_opp_level(struct device *dev, struct opp_table *opp_table, - struct dev_pm_opp *opp) -{ - unsigned int level = 0; - int ret = 0; - - if (opp) { - if (opp->level == OPP_LEVEL_UNSET) - return 0; - - level = opp->level; - } - - /* Request a new performance state through the device's PM domain. */ - ret = dev_pm_domain_set_performance_state(dev, level); - if (ret) - dev_err(dev, "Failed to set performance state %u (%d)\n", level, - ret); - - return ret; -} - static void _find_current_opp(struct device *dev, struct opp_table *opp_table) { struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
In _set_opp() we are normally bailing out when trying to set an OPP that is the current one. This make perfect sense, but becomes a problem when _set_required_opps() calls it recursively. More precisely, when a required OPP is being shared by multiple PM domains, we end up skipping to request the corresponding performance-state for all of the PM domains, but the first one. Let's fix the problem, by calling _set_opp_level() from _set_required_opps() instead. Fixes: e37440e7e2c2 ("OPP: Call dev_pm_opp_set_opp() for required OPPs") Cc: stable@vger.kernel.org Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/opp/core.c | 47 +++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-)