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

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

On Tue, Sep 01, 2020 at 11:43:34AM +0200, Ulf Hansson wrote:
> On Tue, 1 Sep 2020 at 11:01, Sudeep Holla <sudeep.holla@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@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.
>
> 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?
>

I am fine with that. I assume by disable OSI mode, you are referring to
switch back to PC mode.

--
Regards,
Sudeep

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