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

From: Sudeep Holla <sudeep.holla@arm.com>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>,
	linux-pm@vger.kernel.org,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Lina Iyer <ilina@codeaurora.org>,
	Lukasz Luba <lukasz.luba@arm.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Benjamin Gaignard <benjamin.gaignard@st.com>,
	Sudeep Holla <sudeep.holla@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] cpuidle: psci: Allow PM domain to be initialized even if no OSI mode
Date: Tue, 18 Aug 2020 13:35:07 +0100	[thread overview]
Message-ID: <20200818123507.GD6873@bogus> (raw)
In-Reply-To: <20200814123436.61851-1-ulf.hansson@linaro.org>

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 ?

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

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

--
Regards,
Sudeep