Re: [PATCH 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

From: Ulf Hansson <ulf.hansson@linaro.org>
To: Sudeep Holla <sudeep.holla@arm.com>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>,
	Linux PM <linux-pm@vger.kernel.org>
Cc: "Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Lina Iyer <ilina@codeaurora.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Benjamin Gaignard <benjamin.gaignard@st.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology
Date: Thu, 27 Feb 2020 15:59:25 +0100	[thread overview]
Message-ID: <CAPDyKFqeHLeMrOEPg1p6WaLOApRzJbzNL343d2d43bEQxOAz5w@mail.gmail.com> (raw)
In-Reply-To: <20200227124551.31860-5-ulf.hansson@linaro.org>

On Thu, 27 Feb 2020 at 13:46, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> It's possible that only the WFI state is supported for the CPU, while also
> a shared idle state exists for a group of CPUs.
>
> When the hierarchical topology is used, the shared idle state may not be
> compatible with arm,idle-state, rather with "domain-idle-state", which
> makes dt_init_idle_driver() to return zero. This leads to that the
> cpuidle-psci driver bails out during initialization, avoiding to register a
> cpuidle driver and instead relies on the default architectural
> back-end (called via cpu_do_idle()). In other words, the shared idle state
> becomes unused.
>
> Let's fix this behaviour, by allowing the dt_init_idle_driver() to return 0
> and then continue with the initialization. If it turns out that the
> hierarchical topology is used and we have some additional states to manage,
> then continue with the cpuidle driver registration, otherwise bail out, as
> we did before.
>
> Reported-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> Fixes: a65a397f2451 ("cpuidle: psci: Add support for PM domains by using genpd")
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/cpuidle/cpuidle-psci.c | 47 ++++++++++++++++++++++------------
>  1 file changed, 30 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 7b459f987c50..7699b2dab622 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -56,16 +56,19 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
>         u32 *states = data->psci_states;
>         struct device *pd_dev = data->dev;
>         u32 state;
> -       int ret;
> +       int ret = 0;
>
>         /* Do runtime PM to manage a hierarchical CPU toplogy. */
>         pm_runtime_put_sync_suspend(pd_dev);
>
>         state = psci_get_domain_state();
> -       if (!state)
> +       if (!state && states)
>                 state = states[idx];
>
> -       ret = psci_enter_state(idx, state);
> +       if (state)
> +               ret = psci_cpu_suspend_enter(state);

Looks like I should set ret to idx, if ret == 0 - as to instruct
cpuidle about what state we did enter. I will update that in the next
revision, but awaiting for some comments first.

> +       else
> +               cpu_do_idle();
>
>         pm_runtime_get_sync(pd_dev);
>

[...]

Kind regards
Uffe