diff mbox series

cpuidle: psci: Align psci_power_state count with idle state count

Message ID 20191024151834.17036-1-sudeep.holla@arm.com (mailing list archive)
State New, archived
Headers show
Series cpuidle: psci: Align psci_power_state count with idle state count | expand

Commit Message

Sudeep Holla Oct. 24, 2019, 3:18 p.m. UTC
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

Comments

Ulf Hansson Oct. 24, 2019, 4:10 p.m. UTC | #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
>
Sudeep Holla Oct. 27, 2019, 2:20 a.m. UTC | #2
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
diff mbox series

Patch

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);