Re: [PATCH v2 2/2] 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: Mark Rutland <mark.rutland@arm.com>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Benjamin Gaignard <benjamin.gaignard@st.com>,
	linux-pm@vger.kernel.org, Stephen Boyd <sboyd@kernel.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Lina Iyer <ilina@codeaurora.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	linux-arm-kernel@lists.infradead.org,
	Sudeep Holla <sudeep.holla@arm.com>,
	Lukasz Luba <lukasz.luba@arm.com>
Subject: Re: [PATCH v2 2/2] cpuidle: psci: Allow PM domain to be initialized even if no OSI mode
Date: Tue, 1 Sep 2020 14:32:18 +0100	[thread overview]
Message-ID: <20200901133218.GB27090@bogus> (raw)
In-Reply-To: <20200901121226.217568-3-ulf.hansson@linaro.org>

On Tue, Sep 01, 2020 at 02:12:26PM +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.
>
> 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.
>
> 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.
>
> 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>
> ---
>
> Changes in v2:
> 	- Assign the genpd ->power_off() callback, only when the PSCI OSI mode
> 	has been successfully enabled.
>
> ---
>  drivers/cpuidle/cpuidle-psci-domain.c | 57 ++++++++++++++-------------
>  1 file changed, 29 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> index b6ab0415f450..256e7e35b5af 100644
> --- a/drivers/cpuidle/cpuidle-psci-domain.c
> +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> @@ -105,7 +105,7 @@ static void psci_pd_free_states(struct genpd_power_state *states,
>  	kfree(states);
>  }
>
> -static int psci_pd_init(struct device_node *np)
> +static int psci_pd_init(struct device_node *np, bool use_osi)
>  {
>  	struct generic_pm_domain *pd;
>  	struct psci_pd_provider *pd_provider;
> @@ -135,11 +135,14 @@ static int psci_pd_init(struct device_node *np)
>
>  	pd->free_states = psci_pd_free_states;
>  	pd->name = kbasename(pd->name);
> -	pd->power_off = psci_pd_power_off;
>  	pd->states = states;
>  	pd->state_count = state_count;
>  	pd->flags |= GENPD_FLAG_IRQ_SAFE | GENPD_FLAG_CPU_DOMAIN;
>
> +	/* Use the ->power_off() callback when OSI is enabled. */

IIUC, you did mention that we need to attach PD even when the OSI fails
as this could be shared domain. With the below conditional assignment
of power_off, I understand that if OSI fails and we have domains attached,
then it will be always ON ?

Other than that, this patch looks good.

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

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