Re: [PATCH 08/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains

[Ulf Hansson <ulf.hansson@linaro.org>]

On Thu, 24 Oct 2019 at 17:42, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> 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@linaro.org>
> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  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 ?

Feel free to suggest something. I couldn't find any better. :-)

>
> > +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 ?

Sure, just wanted to keep the all the preparations for the PM domains
support together.

But you have a point, let me move this part to the patch that needs it.

Kind regards
Uffe