Re: [PATCH v3 1/3] PM: domains: Make set_performance_state() callback optional

From: Ulf Hansson <ulf.hansson@linaro.org>
To: Viresh Kumar <viresh.kumar@linaro.org>,
	Dmitry Osipenko <digetx@gmail.com>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Peter Geis <pgwipeout@gmail.com>,
	Nicolas Chauvet <kwizart@gmail.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Kevin Hilman <khilman@kernel.org>,
	Peter De Schrijver <pdeschrijver@nvidia.com>,
	Viresh Kumar <vireshk@kernel.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Matt Merhar <mattmerhar@protonmail.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-tegra <linux-tegra@vger.kernel.org>,
	Linux PM <linux-pm@vger.kernel.org>
Subject: Re: [PATCH v3 1/3] PM: domains: Make set_performance_state() callback optional
Date: Mon, 18 Jan 2021 11:59:13 +0100	[thread overview]
Message-ID: <CAPDyKFquCGUSTvcCpmN0vm1eGEz9B_hYSNm7wojhgwuXT=jkEQ@mail.gmail.com> (raw)
In-Reply-To: <20210118072855.anncyl6z3e5uznvd@vireshk-i7>

On Mon, 18 Jan 2021 at 08:28, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 18-01-21, 04:13, Dmitry Osipenko wrote:
> > Make set_performance_state() callback optional in order to remove the
> > need from power domain drivers to implement a dummy callback. If callback
> > isn't implemented by a GENPD driver, then the performance state is passed
> > to the parent domain.
> >
> > Tested-by: Peter Geis <pgwipeout@gmail.com>
> > Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> > Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > ---
> >  drivers/base/power/domain.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 9a14eedacb92..a3e1bfc233d4 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -339,9 +339,11 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
> >                       goto err;
> >       }
> >
> > -     ret = genpd->set_performance_state(genpd, state);
> > -     if (ret)
> > -             goto err;
> > +     if (genpd->set_performance_state) {
> > +             ret = genpd->set_performance_state(genpd, state);
> > +             if (ret)
> > +                     goto err;
> > +     }
>
> Earlier in this routine we also have this:
>
> if (!parent->set_performance_state)
>         continue;
>
> Should we change that too ?

Good point! I certainly overlooked that when reviewing. We need to
reevaluate the new state when propagating to the parent(s).

To me, it looks like when doing the propagation we must check if the
parent has the ->set_performance_state() callback assigned. If so, we
should call dev_pm_opp_xlate_performance_state(), but otherwise just
use the value of "state", when doing the reevaluation.

Does it make sense?

>
> >
> >       genpd->performance_state = state;
> >       return 0;
> > @@ -399,9 +401,6 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
> >       if (!genpd)
> >               return -ENODEV;
> >
> > -     if (unlikely(!genpd->set_performance_state))
> > -             return -EINVAL;
> > -
> >       if (WARN_ON(!dev->power.subsys_data ||
> >                    !dev->power.subsys_data->domain_data))
> >               return -EINVAL;
>
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> --
> viresh

Kind regards
Uffe