[PATCH] cpuidle: psci: Allow PM domain to be initialized even if no OSI mode

[Ulf Hansson]

On Tue, 18 Aug 2020 at 14:35, Sudeep Holla <sudeep.holla at arm.com> wrote:
>
> On Fri, Aug 14, 2020 at 02:34:36PM +0200, Ulf Hansson wrote:
> > If the PSCI OSI mode isn't supported or fails to be enabled, the PM domain
> > topology with the genpd providers isn't initialized. This is perfectly fine
> > from cpuidle-psci point of view.
> >
>
> Indeed.
>
> > However, since the PM domain topology in the DTS files is a description of
> > the HW, no matter of whether the PSCI OSI mode is supported or not, other
> > consumers besides the CPUs may rely on it.
> >
>
> And why are they even registered as part of cpuidle-psci-domain ?
> If they have to be, can be decouple it completely from cpuidle then ?

These devices can't be decoupled as they are a part of the CPU cluster
PM domain.

This is for example the case RPMH (rsc) device for Qcom platforms, but
there are other platforms that need this too.

>
> > Therefore, let's always allow the initialization of the PM domain topology
> > to succeed, independently of whether the PSCI OSI mode is supported.
> > Consequentially we need to track if we succeed to enable the OSI mode, as
> > to know when a domain idlestate can be selected.
> >
>
> I thought we had discussed this in past, why are we back to the same
> discussion ? I may need to read those again to get the context.

That discussion was according to my understanding about whether we
should allow CPU devices to be managed by runtime PM and the CPU PM
domains, if OSI was *not* supported.

We concluded that we didn't want to allow that, which makes sense -
and I am not changing that in $subject patch.

>
> > Note that, CPU devices are still not being attached to the PM domain
> > topology, unless the PSCI OSI mode is supported.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson at linaro.org>
> > ---
> >  drivers/cpuidle/cpuidle-psci-domain.c | 49 +++++++++++++--------------
> >  1 file changed, 24 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > index b6e9649ab0da..55653c110e3a 100644
> > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > @@ -28,6 +28,7 @@ struct psci_pd_provider {
> >
> >  static LIST_HEAD(psci_pd_providers);
> >  static bool psci_pd_allow_domain_state;
> > +static bool psci_osi_mode_enabled;
> >
> >  static int psci_pd_power_off(struct generic_pm_domain *pd)
> >  {
> > @@ -37,7 +38,7 @@ static int psci_pd_power_off(struct generic_pm_domain *pd)
> >       if (!state->data)
> >               return 0;
> >
> > -     if (!psci_pd_allow_domain_state)
> > +     if (!psci_pd_allow_domain_state || !psci_osi_mode_enabled)
>
> I really don't like this check. Why do we have to keep checking
> psci_osi_mode_enabled every single time and that is the reason IIRC
> I was against this and just don't add the domains.

You have a point about the check, it's not very nice - but from an
execution point of view, I don't think it's the end of the world.

Note that, when not using OSI, then the ->power_off() callback will
not be invoked in the cpuidle path.

Anyway, if you like, I can try to rework the code, so that the
->power_off() callback doesn't get assigned, if we are not using OSI.
Although, I am not sure the trouble is worth it, as I probably need to
try to enable OSI before initializing the genpd data structures. Then,
if failing with genpd initializations, I need to revert back to PC
mode.

What do you think?

Kind regards
Uffe