On Thu, Oct 10, 2019 at 01:39:32PM +0200, Ulf Hansson wrote:
> The per CPU variable psci_power_state, contains an array of fixed values,
> which reflects the corresponding arm,psci-suspend-param parsed from DT, for
> each of the available CPU idle states.
>
> This isn't sufficient when using the hierarchical CPU topology in DT, in
> combination with having PSCI OS initiated (OSI) mode enabled. More
> precisely, in OSI mode, Linux is responsible of telling the PSCI FW what
> idle state the cluster (a group of CPUs) should enter, while in PSCI
> Platform Coordinated (PC) mode, each CPU independently votes for an idle
> state of the cluster.
>
> For this reason, introduce a per CPU variable called domain_state and
> implement two helper functions to read/write its value. Then let the
> domain_state take precedence over the regular selected state, when idling
> the CPU in psci_enter_idle_state().
>
> This allows subsequent patches that implements support for PM domains for
> cpuidle-psci, to write the selected idle state parameter for the cluster
> into the domain_state variable. Furthermore, let's share the needed
> functions in a header file, to enable the support for PM domains to be
> implemented in a separate c-file.
>
> Co-developed-by: Lina Iyer <lina.iyer@xxxxxxxxxx>
> Signed-off-by: Lina Iyer <lina.iyer@xxxxxxxxxx>
> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> ---
> drivers/cpuidle/cpuidle-psci.c | 31 ++++++++++++++++++++++++++++---
> drivers/cpuidle/cpuidle-psci.h | 11 +++++++++++
> 2 files changed, 39 insertions(+), 3 deletions(-)
> create mode 100644 drivers/cpuidle/cpuidle-psci.h
>
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 5c30f23a8a7b..a16467daf99d 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -20,17 +20,42 @@
>
> #include <asm/cpuidle.h>
>
> +#include "cpuidle-psci.h"
> #include "dt_idle_states.h"
>
> static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> +static DEFINE_PER_CPU(u32, domain_state);
> +
[nit] for me name 'domain_state' is per domain and for one each in the
hierarchical topology. But here it's per cpu, just wondering if we can
rename it to provide implicit meaning ?
> +void psci_set_domain_state(u32 state)
> +{
> + __this_cpu_write(domain_state, state);
> +}
> +
> +static inline u32 psci_get_domain_state(void)
> +{
> + return __this_cpu_read(domain_state);
> +}
> +
> +static int __psci_enter_idle_state(int idx, u32 state)
> +{
> + return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> +}
>
> static int psci_enter_idle_state(struct cpuidle_device *dev,
> struct cpuidle_driver *drv, int idx)
> {
> + int ret;
> u32 *states = __this_cpu_read(psci_power_state);
> - u32 state = idx ? states[idx - 1] : 0;
> + u32 state = psci_get_domain_state();
>
> - return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> + if (!state && idx)
> + state = states[idx - 1];
This can go as mentioned earlier once we have entry for WFI also.
> +
> + ret = __psci_enter_idle_state(idx, state);
> +
> + /* Clear the domain state to start fresh when back from idle. */
> + psci_set_domain_state(0);
> + return ret;
> }
>
> static struct cpuidle_driver psci_idle_driver __initdata = {
> @@ -56,7 +81,7 @@ static const struct of_device_id psci_idle_state_match[] __initconst = {
> { },
> };
>
> -static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
> +int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
Can this me made part of patch that uses it instead of here ?
--
Regards,
Sudeep
