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

[Sudeep Holla <sudeep.holla@arm.com>]

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@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@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.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel