Re: [PATCH 10/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain

[Ulf Hansson <ulf.hansson@linaro.org>]

On Thu, 24 Oct 2019 at 18:31, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 10, 2019 at 01:39:34PM +0200, Ulf Hansson wrote:
> > Introduce a PSCI DT helper function, psci_dt_attach_cpu(), which takes a
> > CPU number as an in-parameter and tries to attach the CPU's struct device
> > to its corresponding PM domain.
> >
> > Let's makes use of dev_pm_domain_attach_by_name(), as it allows us to
> > specify "psci" as the "name" of the PM domain to attach to. Additionally,
> > let's also prepare the attached device to be power managed via runtime PM.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/cpuidle/cpuidle-psci-domain.c | 21 +++++++++++++++++++++
> >  drivers/cpuidle/cpuidle-psci.h        |  6 ++++++
> >  2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > index 3f5143ccc3e0..7429fd7626a1 100644
> > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > @@ -9,9 +9,11 @@
> >
> >  #define pr_fmt(fmt) "CPUidle PSCI: " fmt
> >
> > +#include <linux/cpu.h>
> >  #include <linux/device.h>
> >  #include <linux/kernel.h>
> >  #include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/psci.h>
> >  #include <linux/slab.h>
> >  #include <linux/string.h>
> > @@ -279,3 +281,22 @@ static int __init psci_idle_init_domains(void)
> >       return ret;
> >  }
> >  subsys_initcall(psci_idle_init_domains);
> > +
> > +struct device *psci_dt_attach_cpu(int cpu)
> > +{
> > +     struct device *dev;
> > +
> > +     /* Currently limit the hierarchical topology to be used in OSI mode. */
> > +     if (!psci_has_osi_support())
> > +             return NULL;
> > +
> > +     dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
>
> This clarifies the need for the fixed name. But why not just go by index 0
> as the consumer of these psci power-domains will have only one power domain
> entry. Why do we need this name compulsory ?

The idea is to be future proof. If I recall correctly, the CPU node on
some QCOM SoCs may also have "CPR" PM domain specified, thus
"multiple" power-domains could be specified.

In any case, using "psci" doesn't really hurt, right?

> Further, it's specified as
> optional in the generic binding, do we make it "required" for this psci
> idle states binding anywhere that I missed ?

Good point! Unless you tell me differently, I will update the DT doc
to clarify this is "required".

Kind regards
Uffe