| Message ID | 20221024144304.198689-1-abel.vesa@linaro.org |
|---|---|
| State | Accepted |
| Commit | ae8ac19655e0cd7ca5ac2f45837d386745aaed7f |
| Headers | show |
| Series | PM: domains: Reverse the order of performance and enabling ops | expand |
On Mon, Oct 24, 2022 at 5:52 PM Abel Vesa <abel.vesa@linaro.org> wrote: > > The ->set_performance_state() needs to be called before ->power_on() > when a genpd is powered on, and after ->power_off() when a genpd is > powered off. Do this in order to let the provider know to which > performance state to power on the genpd, on the power on sequence, and > also to maintain the performance for that genpd until after powering off, > on power off sequence. > > There is no scenario where a consumer would need its genpd enabled and > then its performance state increased. Instead, in every scenario, the > consumer needs the genpd to be enabled from the start at a specific > performance state. > > And same logic applies to the powering down. No consumer would need its > genpd performance state dropped right before powering down. > > Now, there are currently two vendors which use ->set_performance_state() > in their genpd providers. One of them is Tegra, but the only genpd provider > (PMC) that makes use of ->set_performance_state() doesn't implement the > ->power_on() or ->power_off(), and so it will not be affected by the ops > reversal. > > The other vendor that uses it is Qualcomm, in multiple genpd providers > actually (RPM, RPMh and CPR). But all Qualcomm genpd providers that make > use of ->set_performance_state() need the order between enabling ops and > the performance setting op to be reversed. And the reason for that is that > it currently translates into two different voltages in order to power on > a genpd to a specific performance state. Basically, ->power_on() switches > to the minimum (enabling) voltage for that genpd, and then > ->set_performance_state() sets it to the voltage level required by the > consumer. > > By reversing the call order, we rely on the provider to know what to do > on each call, but most popular usecase is to cache the performance state > and postpone the voltage setting until the ->power_on() gets called. > > As for the reason of still needing the ->power_on() and ->power_off() for a > provider which could get away with just having ->set_performance_state() > implemented, there are consumers that do not (nor should) provide an > opp-table. For those consumers, ->set_performance_state() will not be > called, and so they will enable the genpd to its minimum performance state > by a ->power_on() call. Same logic goes for the disabling. Ulf, any comments? > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/base/power/domain.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index ead135c7044c..e66a711fec88 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -939,8 +939,8 @@ static int genpd_runtime_suspend(struct device *dev) > return 0; > > genpd_lock(genpd); > - gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > genpd_power_off(genpd, true, 0); > + gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > genpd_unlock(genpd); > > return 0; > @@ -978,9 +978,8 @@ static int genpd_runtime_resume(struct device *dev) > goto out; > > genpd_lock(genpd); > + genpd_restore_performance_state(dev, gpd_data->rpm_pstate); > ret = genpd_power_on(genpd, 0); > - if (!ret) > - genpd_restore_performance_state(dev, gpd_data->rpm_pstate); > genpd_unlock(genpd); > > if (ret) > @@ -1018,8 +1017,8 @@ static int genpd_runtime_resume(struct device *dev) > err_poweroff: > if (!pm_runtime_is_irq_safe(dev) || genpd_is_irq_safe(genpd)) { > genpd_lock(genpd); > - gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > genpd_power_off(genpd, true, 0); > + gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > genpd_unlock(genpd); > } > > @@ -2749,17 +2748,6 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, > dev->pm_domain->detach = genpd_dev_pm_detach; > dev->pm_domain->sync = genpd_dev_pm_sync; > > - if (power_on) { > - genpd_lock(pd); > - ret = genpd_power_on(pd, 0); > - genpd_unlock(pd); > - } > - > - if (ret) { > - genpd_remove_device(pd, dev); > - return -EPROBE_DEFER; > - } > - > /* Set the default performance state */ > pstate = of_get_required_opp_performance_state(dev->of_node, index); > if (pstate < 0 && pstate != -ENODEV && pstate != -EOPNOTSUPP) { > @@ -2771,6 +2759,18 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, > goto err; > dev_gpd_data(dev)->default_pstate = pstate; > } > + > + if (power_on) { > + genpd_lock(pd); > + ret = genpd_power_on(pd, 0); > + genpd_unlock(pd); > + } > + > + if (ret) { > + genpd_remove_device(pd, dev); > + return -EPROBE_DEFER; > + } > + > return 1; > > err: > -- > 2.34.1 >
On Mon, 24 Oct 2022 at 16:44, Abel Vesa <abel.vesa@linaro.org> wrote: > > The ->set_performance_state() needs to be called before ->power_on() > when a genpd is powered on, and after ->power_off() when a genpd is > powered off. Do this in order to let the provider know to which > performance state to power on the genpd, on the power on sequence, and > also to maintain the performance for that genpd until after powering off, > on power off sequence. > > There is no scenario where a consumer would need its genpd enabled and > then its performance state increased. Instead, in every scenario, the > consumer needs the genpd to be enabled from the start at a specific > performance state. > > And same logic applies to the powering down. No consumer would need its > genpd performance state dropped right before powering down. > > Now, there are currently two vendors which use ->set_performance_state() > in their genpd providers. One of them is Tegra, but the only genpd provider > (PMC) that makes use of ->set_performance_state() doesn't implement the > ->power_on() or ->power_off(), and so it will not be affected by the ops > reversal. > > The other vendor that uses it is Qualcomm, in multiple genpd providers > actually (RPM, RPMh and CPR). But all Qualcomm genpd providers that make > use of ->set_performance_state() need the order between enabling ops and > the performance setting op to be reversed. And the reason for that is that > it currently translates into two different voltages in order to power on > a genpd to a specific performance state. Basically, ->power_on() switches > to the minimum (enabling) voltage for that genpd, and then > ->set_performance_state() sets it to the voltage level required by the > consumer. > > By reversing the call order, we rely on the provider to know what to do > on each call, but most popular usecase is to cache the performance state > and postpone the voltage setting until the ->power_on() gets called. > > As for the reason of still needing the ->power_on() and ->power_off() for a > provider which could get away with just having ->set_performance_state() > implemented, there are consumers that do not (nor should) provide an > opp-table. For those consumers, ->set_performance_state() will not be > called, and so they will enable the genpd to its minimum performance state > by a ->power_on() call. Same logic goes for the disabling. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/base/power/domain.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index ead135c7044c..e66a711fec88 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -939,8 +939,8 @@ static int genpd_runtime_suspend(struct device *dev) > return 0; > > genpd_lock(genpd); > - gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > genpd_power_off(genpd, true, 0); > + gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > genpd_unlock(genpd); > > return 0; > @@ -978,9 +978,8 @@ static int genpd_runtime_resume(struct device *dev) > goto out; > > genpd_lock(genpd); > + genpd_restore_performance_state(dev, gpd_data->rpm_pstate); > ret = genpd_power_on(genpd, 0); > - if (!ret) > - genpd_restore_performance_state(dev, gpd_data->rpm_pstate); > genpd_unlock(genpd); > > if (ret) > @@ -1018,8 +1017,8 @@ static int genpd_runtime_resume(struct device *dev) > err_poweroff: > if (!pm_runtime_is_irq_safe(dev) || genpd_is_irq_safe(genpd)) { > genpd_lock(genpd); > - gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > genpd_power_off(genpd, true, 0); > + gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > genpd_unlock(genpd); > } > > @@ -2749,17 +2748,6 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, > dev->pm_domain->detach = genpd_dev_pm_detach; > dev->pm_domain->sync = genpd_dev_pm_sync; > > - if (power_on) { > - genpd_lock(pd); > - ret = genpd_power_on(pd, 0); > - genpd_unlock(pd); > - } > - > - if (ret) { > - genpd_remove_device(pd, dev); > - return -EPROBE_DEFER; > - } > - > /* Set the default performance state */ > pstate = of_get_required_opp_performance_state(dev->of_node, index); > if (pstate < 0 && pstate != -ENODEV && pstate != -EOPNOTSUPP) { > @@ -2771,6 +2759,18 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, > goto err; > dev_gpd_data(dev)->default_pstate = pstate; > } > + > + if (power_on) { > + genpd_lock(pd); > + ret = genpd_power_on(pd, 0); > + genpd_unlock(pd); > + } > + > + if (ret) { > + genpd_remove_device(pd, dev); > + return -EPROBE_DEFER; If we manage to request a default performance state (dev_gpd_data(dev)->default_pstate != 0) a few lines above, we should probably drop that request before returning here. Similar to what we do in genpd_dev_pm_detach(). > + } > + > return 1; > > err: Other than the rather small thing above, this looks good to me! Kind regards Uffe
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index ead135c7044c..e66a711fec88 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -939,8 +939,8 @@ static int genpd_runtime_suspend(struct device *dev) return 0; genpd_lock(genpd); - gpd_data->rpm_pstate = genpd_drop_performance_state(dev); genpd_power_off(genpd, true, 0); + gpd_data->rpm_pstate = genpd_drop_performance_state(dev); genpd_unlock(genpd); return 0; @@ -978,9 +978,8 @@ static int genpd_runtime_resume(struct device *dev) goto out; genpd_lock(genpd); + genpd_restore_performance_state(dev, gpd_data->rpm_pstate); ret = genpd_power_on(genpd, 0); - if (!ret) - genpd_restore_performance_state(dev, gpd_data->rpm_pstate); genpd_unlock(genpd); if (ret) @@ -1018,8 +1017,8 @@ static int genpd_runtime_resume(struct device *dev) err_poweroff: if (!pm_runtime_is_irq_safe(dev) || genpd_is_irq_safe(genpd)) { genpd_lock(genpd); - gpd_data->rpm_pstate = genpd_drop_performance_state(dev); genpd_power_off(genpd, true, 0); + gpd_data->rpm_pstate = genpd_drop_performance_state(dev); genpd_unlock(genpd); } @@ -2749,17 +2748,6 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, dev->pm_domain->detach = genpd_dev_pm_detach; dev->pm_domain->sync = genpd_dev_pm_sync; - if (power_on) { - genpd_lock(pd); - ret = genpd_power_on(pd, 0); - genpd_unlock(pd); - } - - if (ret) { - genpd_remove_device(pd, dev); - return -EPROBE_DEFER; - } - /* Set the default performance state */ pstate = of_get_required_opp_performance_state(dev->of_node, index); if (pstate < 0 && pstate != -ENODEV && pstate != -EOPNOTSUPP) { @@ -2771,6 +2759,18 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, goto err; dev_gpd_data(dev)->default_pstate = pstate; } + + if (power_on) { + genpd_lock(pd); + ret = genpd_power_on(pd, 0); + genpd_unlock(pd); + } + + if (ret) { + genpd_remove_device(pd, dev); + return -EPROBE_DEFER; + } + return 1; err:
The ->set_performance_state() needs to be called before ->power_on() when a genpd is powered on, and after ->power_off() when a genpd is powered off. Do this in order to let the provider know to which performance state to power on the genpd, on the power on sequence, and also to maintain the performance for that genpd until after powering off, on power off sequence. There is no scenario where a consumer would need its genpd enabled and then its performance state increased. Instead, in every scenario, the consumer needs the genpd to be enabled from the start at a specific performance state. And same logic applies to the powering down. No consumer would need its genpd performance state dropped right before powering down. Now, there are currently two vendors which use ->set_performance_state() in their genpd providers. One of them is Tegra, but the only genpd provider (PMC) that makes use of ->set_performance_state() doesn't implement the ->power_on() or ->power_off(), and so it will not be affected by the ops reversal. The other vendor that uses it is Qualcomm, in multiple genpd providers actually (RPM, RPMh and CPR). But all Qualcomm genpd providers that make use of ->set_performance_state() need the order between enabling ops and the performance setting op to be reversed. And the reason for that is that it currently translates into two different voltages in order to power on a genpd to a specific performance state. Basically, ->power_on() switches to the minimum (enabling) voltage for that genpd, and then ->set_performance_state() sets it to the voltage level required by the consumer. By reversing the call order, we rely on the provider to know what to do on each call, but most popular usecase is to cache the performance state and postpone the voltage setting until the ->power_on() gets called. As for the reason of still needing the ->power_on() and ->power_off() for a provider which could get away with just having ->set_performance_state() implemented, there are consumers that do not (nor should) provide an opp-table. For those consumers, ->set_performance_state() will not be called, and so they will enable the genpd to its minimum performance state by a ->power_on() call. Same logic goes for the disabling. Signed-off-by: Abel Vesa <abel.vesa@linaro.org> --- drivers/base/power/domain.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)