On Tue, 15 Nov 2022 at 22:25, Abel Vesa <abel.vesa@xxxxxxxxxx> 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@xxxxxxxxxx> Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx> Kind regards Uffe > --- > > Changes since v1: > - Added performance state drop on power on failure, like Ulf suggested > > drivers/base/power/domain.c | 36 +++++++++++++++++++++--------------- > 1 file changed, 21 insertions(+), 15 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index e5f4e5a2eb9e..967bcf9d415e 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -964,8 +964,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; > @@ -1003,9 +1003,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) > @@ -1043,8 +1042,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); > } > > @@ -2733,17 +2732,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) { > @@ -2755,6 +2743,24 @@ 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) { > + /* Drop the default performance state */ > + if (dev_gpd_data(dev)->default_pstate) { > + dev_pm_genpd_set_performance_state(dev, 0); > + dev_gpd_data(dev)->default_pstate = 0; > + } > + > + genpd_remove_device(pd, dev); > + return -EPROBE_DEFER; > + } > + > return 1; > > err: > -- > 2.34.1 >