lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0id+D4wCZCU1RCJbLLcQPWRZ0iVA3mqXZg4iEAkG78eJw@mail.gmail.com> Date: Fri, 25 Nov 2022 19:33:20 +0100 From: "Rafael J. Wysocki" <rafael@...nel.org> To: Ulf Hansson <ulf.hansson@...aro.org>, Abel Vesa <abel.vesa@...aro.org> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Kevin Hilman <khilman@...nel.org>, Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Bjorn Andersson <andersson@...nel.org>, linux-pm@...r.kernel.org, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, linux-arm-msm@...r.kernel.org Subject: Re: [PATCH v2] PM: domains: Reverse the order of performance and enabling ops On Wed, Nov 16, 2022 at 1:48 PM Ulf Hansson <ulf.hansson@...aro.org> wrote: > > On Tue, 15 Nov 2022 at 22:25, Abel Vesa <abel.vesa@...aro.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@...aro.org> > > Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org> Applied as 6.2 material, thanks! > > --- > > > > 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 > >
Powered by blists - more mailing lists