Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 26 Jul 2022 21:38:14 +0300
From:   Abel Vesa <abel.vesa@linaro.org>
To:     Ulf Hansson <ulf.hansson@linaro.org>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Kevin Hilman <khilman@kernel.org>,
        Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-pm@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arm-msm@vger.kernel.org
Subject: Re: [RFC] PM: domains: Reverse the order of performance and enabling
 ops
Message-ID: <YuA0luCtQ1J+ExBi@linaro.org>
References: <20220720110246.762939-1-abel.vesa@linaro.org>
 <CAPDyKFoh8UV=QC6RhOkc=FSvoeqF_UiWp97h0Qp8dniB=sS+8A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPDyKFoh8UV=QC6RhOkc=FSvoeqF_UiWp97h0Qp8dniB=sS+8A@mail.gmail.com>
Precedence: bulk

On 22-07-21 18:48:10, Ulf Hansson wrote:
> On Wed, 20 Jul 2022 at 13:03, Abel Vesa <abel.vesa@linaro.org> 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@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 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
> >
>