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

[Ulf Hansson]

On Tue, 1 Sep 2020 at 11:01, Sudeep Holla <sudeep.holla at arm.com> wrote:
>
> On Wed, Aug 19, 2020 at 10:20:52AM +0200, Ulf Hansson wrote:
> > 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.
> >
>
> Then drop the check. I am confused as why we need that runtime check if
> we get the setup right.
>
> > 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.
>
> +1 for sure.
>
> > 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.
> >
>
> Just to clarify, you can use psci_osi_mode_enabled anytime during
> initialisation and get the setup right so that we can drop unnecessary
> runtime check every single poweroff call. I prefer to remove that.

Okay, I will respin the series then.

Although, just to be clear, this means that I will have to invoke
psci_set_osi_mode() prior to initializing the PM domains. Then if it
turns out that initializing the PM domain fails, I will have to switch
back to PC mode.

To do that, I will probably need to extend psci_set_osi_mode() to take
a bool as a parameter (to indicate enable or disable OSI mode). You
are okay with this, right?

Kind regards
Uffe