diff mbox series

[06/13] cpuidle: psci: Simplify OF parsing of CPU idle state nodes

Message ID 20191010113937.15962-7-ulf.hansson@linaro.org (mailing list archive)
State Not Applicable, archived
Headers show
Series cpuidle: psci: Support hierarchical CPU arrangement | expand

Commit Message

Ulf Hansson Oct. 10, 2019, 11:39 a.m. UTC
Iterating through the idle state nodes in DT, to find out the number of
states that needs to be allocated is unnecessary, as it has already been
done from dt_init_idle_driver(). Therefore, drop the iteration and use the
number we already have at hand.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpuidle/cpuidle-psci.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

Comments

Sudeep Holla Oct. 24, 2019, 3:36 p.m. UTC | #1
On Thu, Oct 10, 2019 at 01:39:30PM +0200, Ulf Hansson wrote:
> Iterating through the idle state nodes in DT, to find out the number of
> states that needs to be allocated is unnecessary, as it has already been
> done from dt_init_idle_driver(). Therefore, drop the iteration and use the
> number we already have at hand.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/cpuidle/cpuidle-psci.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 2e91c8d6c211..1195a1056139 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -73,28 +73,22 @@ static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
>  	return 0;
>  }
>
> -static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> +static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> +				unsigned int state_nodes, int cpu)

[super nit] Too much in the beginning of the patch to not notice this ;)
May need some '(' alignment here and other places in general.

>  {
> -	int i, ret = 0, count = 0;
> +	int i, ret = 0;
>  	u32 *psci_states;
>  	struct device_node *state_node;
>
> -	/* Count idle states */
> -	while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
> -					      count))) {
> -		count++;
> -		of_node_put(state_node);
> -	}
> -
> -	if (!count)
> -		return -ENODEV;
> -
> -	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
> +	psci_states = kcalloc(state_nodes, sizeof(*psci_states), GFP_KERNEL);
>  	if (!psci_states)
>  		return -ENOMEM;
>
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < state_nodes; i++) {
>  		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);

Can we move above to use of_get_cpu_state_node ? Since it also handles
domain-idle-states.

> +		if (!state_node)
> +			break;
> +
>  		ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
>  		of_node_put(state_node);
>
> @@ -104,6 +98,11 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>  		pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
>  	}
>
> +	if (i != state_nodes) {
> +		ret = -ENODEV;
> +		goto free_mem;
> +	}
> +
>  	/* Idle states parsed correctly, initialize per-cpu pointer */
>  	per_cpu(psci_power_state, cpu) = psci_states;
>  	return 0;
> @@ -113,7 +112,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>  	return ret;
>  }
>
> -static __init int psci_cpu_init_idle(unsigned int cpu)
> +static __init int psci_cpu_init_idle(unsigned int cpu, unsigned int state_nodes)

Does it make sense to rename it as state_count or something similar ?
And it may need + 1 once we add wfi also as entry as suggested by
Lorenzo.

--
Regards,
Sudeep
Ulf Hansson Oct. 24, 2019, 4:33 p.m. UTC | #2
On Thu, 24 Oct 2019 at 17:36, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 10, 2019 at 01:39:30PM +0200, Ulf Hansson wrote:
> > Iterating through the idle state nodes in DT, to find out the number of
> > states that needs to be allocated is unnecessary, as it has already been
> > done from dt_init_idle_driver(). Therefore, drop the iteration and use the
> > number we already have at hand.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/cpuidle/cpuidle-psci.c | 33 ++++++++++++++++-----------------
> >  1 file changed, 16 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 2e91c8d6c211..1195a1056139 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -73,28 +73,22 @@ static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
> >       return 0;
> >  }
> >
> > -static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> > +static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> > +                             unsigned int state_nodes, int cpu)
>
> [super nit] Too much in the beginning of the patch to not notice this ;)
> May need some '(' alignment here and other places in general.

I was trying to find a consistent way of doing it, according to the
existing code, but I failed. :-)

Two cases exist where calls/functions crosses one line, one use solely
tabs and the other uses tab+whitespace to align "exactly". You are
saying that you prefer the latter? If so, I can adopt to that.

>
> >  {
> > -     int i, ret = 0, count = 0;
> > +     int i, ret = 0;
> >       u32 *psci_states;
> >       struct device_node *state_node;
> >
> > -     /* Count idle states */
> > -     while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
> > -                                           count))) {
> > -             count++;
> > -             of_node_put(state_node);
> > -     }
> > -
> > -     if (!count)
> > -             return -ENODEV;
> > -
> > -     psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
> > +     psci_states = kcalloc(state_nodes, sizeof(*psci_states), GFP_KERNEL);
> >       if (!psci_states)
> >               return -ENOMEM;
> >
> > -     for (i = 0; i < count; i++) {
> > +     for (i = 0; i < state_nodes; i++) {
> >               state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
>
> Can we move above to use of_get_cpu_state_node ? Since it also handles
> domain-idle-states.
>
> > +             if (!state_node)
> > +                     break;
> > +
> >               ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
> >               of_node_put(state_node);
> >
> > @@ -104,6 +98,11 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> >               pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
> >       }
> >
> > +     if (i != state_nodes) {
> > +             ret = -ENODEV;
> > +             goto free_mem;
> > +     }
> > +
> >       /* Idle states parsed correctly, initialize per-cpu pointer */
> >       per_cpu(psci_power_state, cpu) = psci_states;
> >       return 0;
> > @@ -113,7 +112,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> >       return ret;
> >  }
> >
> > -static __init int psci_cpu_init_idle(unsigned int cpu)
> > +static __init int psci_cpu_init_idle(unsigned int cpu, unsigned int state_nodes)
>
> Does it make sense to rename it as state_count or something similar ?

Let me check to see if it makes sense to change it. Rebasing on top of
your recently submitted patch, might tell better.

> And it may need + 1 once we add wfi also as entry as suggested by
> Lorenzo.

Yep.

Kind regards
Uffe
Sudeep Holla Oct. 27, 2019, 2:24 a.m. UTC | #3
On Thu, Oct 24, 2019 at 06:33:00PM +0200, Ulf Hansson wrote:
> On Thu, 24 Oct 2019 at 17:36, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 10, 2019 at 01:39:30PM +0200, Ulf Hansson wrote:
> > > Iterating through the idle state nodes in DT, to find out the number of
> > > states that needs to be allocated is unnecessary, as it has already been
> > > done from dt_init_idle_driver(). Therefore, drop the iteration and use the
> > > number we already have at hand.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/cpuidle/cpuidle-psci.c | 33 ++++++++++++++++-----------------
> > >  1 file changed, 16 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > > index 2e91c8d6c211..1195a1056139 100644
> > > --- a/drivers/cpuidle/cpuidle-psci.c
> > > +++ b/drivers/cpuidle/cpuidle-psci.c
> > > @@ -73,28 +73,22 @@ static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
> > >       return 0;
> > >  }
> > >
> > > -static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> > > +static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> > > +                             unsigned int state_nodes, int cpu)
> >
> > [super nit] Too much in the beginning of the patch to not notice this ;)
> > May need some '(' alignment here and other places in general.
>
> I was trying to find a consistent way of doing it, according to the
> existing code, but I failed. :-)
>
> Two cases exist where calls/functions crosses one line, one use solely
> tabs and the other uses tab+whitespace to align "exactly". You are
> saying that you prefer the latter? If so, I can adopt to that.
>

I am not too picky on these, just found it in the beginning of the patch
and hence mentioned it. If it was in the middle, I am sure I wouldn't have
noticed.

> >
> > >  {
> > > -     int i, ret = 0, count = 0;
> > > +     int i, ret = 0;
> > >       u32 *psci_states;
> > >       struct device_node *state_node;
> > >
> > > -     /* Count idle states */
> > > -     while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
> > > -                                           count))) {
> > > -             count++;
> > > -             of_node_put(state_node);
> > > -     }
> > > -
> > > -     if (!count)
> > > -             return -ENODEV;
> > > -
> > > -     psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
> > > +     psci_states = kcalloc(state_nodes, sizeof(*psci_states), GFP_KERNEL);
> > >       if (!psci_states)
> > >               return -ENOMEM;
> > >
> > > -     for (i = 0; i < count; i++) {
> > > +     for (i = 0; i < state_nodes; i++) {
> > >               state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
> >
> > Can we move above to use of_get_cpu_state_node ? Since it also handles
> > domain-idle-states.
> >
> > > +             if (!state_node)
> > > +                     break;
> > > +
> > >               ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
> > >               of_node_put(state_node);
> > >
> > > @@ -104,6 +98,11 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> > >               pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
> > >       }
> > >
> > > +     if (i != state_nodes) {
> > > +             ret = -ENODEV;
> > > +             goto free_mem;
> > > +     }
> > > +
> > >       /* Idle states parsed correctly, initialize per-cpu pointer */
> > >       per_cpu(psci_power_state, cpu) = psci_states;
> > >       return 0;
> > > @@ -113,7 +112,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> > >       return ret;
> > >  }
> > >
> > > -static __init int psci_cpu_init_idle(unsigned int cpu)
> > > +static __init int psci_cpu_init_idle(unsigned int cpu, unsigned int state_nodes)
> >
> > Does it make sense to rename it as state_count or something similar ?
>
> Let me check to see if it makes sense to change it. Rebasing on top of
> your recently submitted patch, might tell better.
>

Sure.

--
Regards,
Sudeep
diff mbox series

Patch

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 2e91c8d6c211..1195a1056139 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -73,28 +73,22 @@  static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
 	return 0;
 }
 
-static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
+static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
+				unsigned int state_nodes, int cpu)
 {
-	int i, ret = 0, count = 0;
+	int i, ret = 0;
 	u32 *psci_states;
 	struct device_node *state_node;
 
-	/* Count idle states */
-	while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
-					      count))) {
-		count++;
-		of_node_put(state_node);
-	}
-
-	if (!count)
-		return -ENODEV;
-
-	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
+	psci_states = kcalloc(state_nodes, sizeof(*psci_states), GFP_KERNEL);
 	if (!psci_states)
 		return -ENOMEM;
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < state_nodes; i++) {
 		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+		if (!state_node)
+			break;
+
 		ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
 		of_node_put(state_node);
 
@@ -104,6 +98,11 @@  static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 		pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
 	}
 
+	if (i != state_nodes) {
+		ret = -ENODEV;
+		goto free_mem;
+	}
+
 	/* Idle states parsed correctly, initialize per-cpu pointer */
 	per_cpu(psci_power_state, cpu) = psci_states;
 	return 0;
@@ -113,7 +112,7 @@  static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 	return ret;
 }
 
-static __init int psci_cpu_init_idle(unsigned int cpu)
+static __init int psci_cpu_init_idle(unsigned int cpu, unsigned int state_nodes)
 {
 	struct device_node *cpu_node;
 	int ret;
@@ -129,7 +128,7 @@  static __init int psci_cpu_init_idle(unsigned int cpu)
 	if (!cpu_node)
 		return -ENODEV;
 
-	ret = psci_dt_cpu_init_idle(cpu_node, cpu);
+	ret = psci_dt_cpu_init_idle(cpu_node, state_nodes, cpu);
 
 	of_node_put(cpu_node);
 
@@ -185,7 +184,7 @@  static int __init psci_idle_init_cpu(int cpu)
 	/*
 	 * Initialize PSCI idle states.
 	 */
-	ret = psci_cpu_init_idle(cpu);
+	ret = psci_cpu_init_idle(cpu, ret);
 	if (ret) {
 		pr_err("CPU %d failed to PSCI idle\n", cpu);
 		goto out_kfree_drv;