On 22-07-21 18:48:10, Ulf Hansson wrote:
> On Wed, 20 Jul 2022 at 13:03, Abel Vesa <abel.vesa@xxxxxxxxxx> wrote:
> >
> > Rather than enabling and then setting the performance state, which usually
> > translates into two different levels (voltages) in order to get to the
> > one required by the consumer, we could give a chance to the providers to
> > cache the performance state needed by the consumer and then, when powering
> > on the power domain, the provider could use the cached level instead.
>
> I don't think it's really clear what you want to do here. Let's see
> what the discussion below brings us to, but for the next version
> please elaborate a bit more in the commit message.
Sorry about that. Will give more details in the next version.
>
> Although, if I understand correctly (also from our offlist
> discussions), you want to make it possible to move from two calls,
> into one call into the FW from the genpd provider. So it's basically
> an optimization, which to me, certainly sounds worth doing.
>
> Furthermore, to get the complete picture, in the Qcom case, we set a
> "default" low performance level from the genpd's ->power_on()
> callback, which is needed to enable basic functionality for some
> consumers.
>
> The second call that I refer to is made when genpd calls the
> ->set_performance() callback (from genpd_runtime_suspend()), which is
> done by genpd to potentially set a new value for an aggregated
> performance state of the PM domain. In case when there actually is a
> new performance state set in this path, we end up calling the FW twice
> for the Qcom case, where this first one is unnecessary.
>
> Did I get that right?
Actually, for every ->power_on, there is a ->set_performance right after.
For example, on genpd_runtime_suspend, this is done:
genpd_lock(genpd);
ret = genpd_power_on(genpd, 0);
if (!ret)
genpd_restore_performance_state(dev, gpd_data->rpm_pstate);
genpd_unlock(genpd);
And same thing on __genpd_dev_pm_attach.
Now, TBH, I can't think of any scenario where a consumer would want its PD powered,
(which implies a non-zero voltage level) and then changed to a higher performance
level (higher voltage).
In most scenarios, though, the consumer needs the PD powered on to a specific voltage
level.
Based on the two statements above, we need ->set_performance to actually act as
a way to tell the provider to which voltage level to power on the power domain
when the ->power_on will be called.
So my suggestion with this patch is to reverse the order, do ->set_performance first
and then ->power_on, this way the provider receives the voltage level required by
a consumer before the request to power on the PD. Then a provider might use that
info when powering on/off that PD.
>
> > Also the drop_performance and power_off have to be reversed so that
> > when the last active consumer suspends, the level doesn't actually drop
> > until the pd is disabled.
>
> I don't quite get what this part helps with, is it really needed to
> improve the behaviour?
Again, why would a consumer need its PD voltage dropped before being powered off?
I think it makes more sense for the ->set_performance in this case to act as a
way to tell the provider that a specific device has yeilded its voltage level
request. That way the provider can drop the voltage to the minimum requested by
the active consumers of that PD.
>
> >
> > For the power domains that do not provide the set_performance, things
> > remain unchanged, as does for the power domains that only provide the
> > set_performance but do not provide the power_on/off.
>
> Right, good points!
>
> I get back to review the code soon, just wanted to make sure I have
> the complete picture first.
>
> Kind regards
> Uffe
>
> >
> > Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> > ---
> > 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 5a2e0232862e..38647c304b73 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);
> > }
> >
> > @@ -2747,17 +2746,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) {
> > @@ -2769,6 +2757,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.3
> >
>
