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