Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Rajendra Nayak]

On 8/6/2021 3:02 PM, Ulf Hansson wrote:
> On Wed, 4 Aug 2021 at 12:58, Rajendra Nayak <rnayak@xxxxxxxxxxxxxx> wrote:
> > Some devices within power domains with performance states do not
> > support DVFS, but still need to vote on a default/static state
> > while they are active. They can express this using the 'required-opps'
> > property in device tree, which points to the phandle of the OPP
> > supported by the corresponding power-domains.
> >
> > Add support to parse this information from DT and then set the
> > specified performance state during attach and drop it on detach.
> > runtime suspend/resume callbacks already have logic to drop/set
> > the vote as needed and should take care of dropping the default
> > perf state vote on runtime suspend and restore it back on runtime
> > resume.
> >
> > Signed-off-by: Rajendra Nayak <rnayak@xxxxxxxxxxxxxx>
> > ---
> >   drivers/base/power/domain.c | 28 ++++++++++++++++++++++++++--
> >   include/linux/pm_domain.h   |  1 +
> >   2 files changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index a934c67..b9b5a9b 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -2598,6 +2598,12 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
> >
> >          dev_dbg(dev, "removing from PM domain %s\n", pd->name);
> >
> > +       /* 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;
> > +       }
> > +
> >          for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) {
> >                  ret = genpd_remove_device(pd, dev);
> >                  if (ret != -EAGAIN)
> > @@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
> >   {
> >          struct of_phandle_args pd_args;
> >          struct generic_pm_domain *pd;
> > +       struct device_node *np;
> > +       int pstate;
> >          int ret;
> >
> >          ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
> > @@ -2675,10 +2683,26 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
> >                  genpd_unlock(pd);
> >          }
> >
> > -       if (ret)
> > +       if (ret) {
> >                  genpd_remove_device(pd, dev);
> > +               return -EPROBE_DEFER;
> > +       }
> > +
> > +       /* Set the default performance state */
> > +       np = dev->of_node;
> > +       if (of_parse_phandle(np, "required-opps", index)) {
>
> Looks like Viresh thinks it's a good idea to drop the error print in
> of_get_required_opp_performance_state() when there is no
> "required-opps" specifier.
>
> Would you mind folding in a patch for that in the series, so this code
> can be simplified according to our earlier discussions?

Sure, I can do that, apart from the error print, the function currently also
returns a -EINVAL in case of the missing 'required-opps', are we suggesting
we change that to not return an error also?

Since this is completely optional in the device node, we would want the function to
ideally not return error and only do so in case 'required-opps' exists and the
translation to performance state fails.
I am not sure that's the behavior we expect in case of 'required-opps' in the OPP
tables also, Viresh?

> > +               pstate = of_get_required_opp_performance_state(np, index);
> > +               if (pstate < 0) {
> > +                       ret = pstate;
> > +                       dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
> > +                               pd->name, ret);
> > +               } else {
> > +                       dev_pm_genpd_set_performance_state(dev, pstate);
> > +                       dev_gpd_data(dev)->default_pstate = pstate;
> > +               }
> > +       }
> >
> > -       return ret ? -EPROBE_DEFER : 1;
> > +       return ret ? ret : 1;
> >   }
> >
> >   /**
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index 21a0577..67017c9 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -198,6 +198,7 @@ struct generic_pm_domain_data {
> >          struct notifier_block *power_nb;
> >          int cpu;
> >          unsigned int performance_state;
> > +       unsigned int default_pstate;
> >          unsigned int rpm_pstate;
> >          ktime_t next_wakeup;
> >          void *data;
>
> Other than the above, this looks good to me!
>
> Kind regards
> Uffe

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation