cpuidle: psci: Align psci_power_state count with idle state count

Instead of allocating 'n-1' states in psci_power_state to manage 'n'
idle states which include "ARM WFI" state, it would be simpler to have
1:1 mapping between psci_power_state and cpuidle driver states.

ARM WFI state(i.e. idx == 0) is handled specially in the generic macro
CPU_PM_CPU_IDLE_ENTER_PARAM and hence state[-1] is not possible. However
for sake of code readability, it is better to have 1:1 mapping and not
use [idx - 1] to access psci_power_state corresponding to driver cpuidle
state for idx.

psci_power_state[0] is default initialised to 0 and is never accessed
while entering WFI state.

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

---
 drivers/cpuidle/cpuidle-psci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Hi Ulf, Lorenzo,

Just to avoid confusion, I thought I will just write this patch as I was
about to make reference to this in my review.

Regards,
Sudeep

--
2.17.1

On Thu, 24 Oct 2019 at 17:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
>

> Instead of allocating 'n-1' states in psci_power_state to manage 'n'

> idle states which include "ARM WFI" state, it would be simpler to have

> 1:1 mapping between psci_power_state and cpuidle driver states.

>

> ARM WFI state(i.e. idx == 0) is handled specially in the generic macro

> CPU_PM_CPU_IDLE_ENTER_PARAM and hence state[-1] is not possible. However

> for sake of code readability, it is better to have 1:1 mapping and not

> use [idx - 1] to access psci_power_state corresponding to driver cpuidle

> state for idx.

>

> psci_power_state[0] is default initialised to 0 and is never accessed

> while entering WFI state.

>

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


Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>


> ---

>  drivers/cpuidle/cpuidle-psci.c | 8 +++++---

>  1 file changed, 5 insertions(+), 3 deletions(-)

>

> Hi Ulf, Lorenzo,

>

> Just to avoid confusion, I thought I will just write this patch as I was

> about to make reference to this in my review.


As discussed with Lorenzo, I said I was going to adopt his review
comments, which means I already have a patch for this locally.

Nevermind this time, but I would appreciate if this kind of
bikeshedding can been avoided future wise.

Kind regards
Uffe

>

> Regards,

> Sudeep

>

> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c

> index f3c1a2396f98..361985f52ddd 100644

> --- a/drivers/cpuidle/cpuidle-psci.c

> +++ b/drivers/cpuidle/cpuidle-psci.c

> @@ -30,7 +30,7 @@ static int psci_enter_idle_state(struct cpuidle_device *dev,

>         u32 *state = __this_cpu_read(psci_power_state);

>

>         return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,

> -                                          idx, state[idx - 1]);

> +                                          idx, state[idx]);

>  }

>

>  static struct cpuidle_driver psci_idle_driver __initdata = {

> @@ -89,12 +89,14 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)

>         if (!count)

>                 return -ENODEV;

>

> +       count++; /* Add WFI state too */

>         psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);

>         if (!psci_states)

>                 return -ENOMEM;

>

> -       for (i = 0; i < count; i++) {

> -               state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);

> +       for (i = 1; i < count; i++) {

> +               state_node = of_parse_phandle(cpu_node, "cpu-idle-states",

> +                                             i - 1);

>                 ret = psci_dt_parse_state_node(state_node, &psci_states[i]);

>                 of_node_put(state_node);

>

> --

> 2.17.1

>

On Thu, Oct 24, 2019 at 06:10:09PM +0200, Ulf Hansson wrote:
> On Thu, 24 Oct 2019 at 17:18, Sudeep Holla <sudeep.holla@arm.com> wrote:

> >

> > Instead of allocating 'n-1' states in psci_power_state to manage 'n'

> > idle states which include "ARM WFI" state, it would be simpler to have

> > 1:1 mapping between psci_power_state and cpuidle driver states.

> >

> > ARM WFI state(i.e. idx == 0) is handled specially in the generic macro

> > CPU_PM_CPU_IDLE_ENTER_PARAM and hence state[-1] is not possible. However

> > for sake of code readability, it is better to have 1:1 mapping and not

> > use [idx - 1] to access psci_power_state corresponding to driver cpuidle

> > state for idx.

> >

> > psci_power_state[0] is default initialised to 0 and is never accessed

> > while entering WFI state.

> >

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

>

> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>

> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

>

> > ---

> >  drivers/cpuidle/cpuidle-psci.c | 8 +++++---

> >  1 file changed, 5 insertions(+), 3 deletions(-)

> >

> > Hi Ulf, Lorenzo,

> >

> > Just to avoid confusion, I thought I will just write this patch as I was

> > about to make reference to this in my review.

>

> As discussed with Lorenzo, I said I was going to adopt his review

> comments, which means I already have a patch for this locally.

>

> Nevermind this time, but I would appreciate if this kind of

> bikeshedding can been avoided future wise.

>


That's one of the reason I just wrote the patch as I felt describing it
it words was difficult compared to patch :). Sorry if you felt this was
bikeshedding.

--
Regards,
Sudeep