Re: [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling

[Sudeep Holla]

On Thu, Aug 08, 2019 at 05:29:24PM +0200, Ulf Hansson wrote:
> On Thu, 8 Aug 2019 at 14:55, Sudeep Holla  wrote:
> >
> > On Mon, Jul 22, 2019 at 04:37:45PM +0100, Lorenzo Pieralisi wrote:
> > > Current PSCI code handles idle state entry through the
> > > psci_cpu_suspend_enter() API, that takes an idle state index as a
> > > parameter and convert the index into a previously initialized
> > > power_state parameter before calling the PSCI.CPU_SUSPEND() with it.
> > >
> > > This is unwieldly, since it forces the PSCI firmware layer to keep track
> > > of power_state parameter for every idle state so that the
> > > index->power_state conversion can be made in the PSCI firmware layer
> > > instead of the CPUidle driver implementations.
> > >
> > > Move the power_state handling out of drivers/firmware/psci
> > > into the respective ACPI/DT PSCI CPUidle backends and convert
> > > the psci_cpu_suspend_enter() API to get the power_state
> > > parameter as input, which makes it closer to its firmware
> > > interface PSCI.CPU_SUSPEND() API.
> > >
> > > A notable side effect is that the PSCI ACPI/DT CPUidle backends
> > > now can directly handle (and if needed update) power_state
> > > parameters before handing them over to the PSCI firmware
> > > interface to trigger PSCI.CPU_SUSPEND() calls.
> > >
> > > Signed-off-by: Lorenzo Pieralisi 
> > > Cc: Will Deacon 
> > > Cc: Ulf Hansson 
> > > Cc: Sudeep Holla 
> >
> > Reviewed-by: Sudeep Holla 
> >
> > > +static __init int psci_cpu_init_idle(unsigned int cpu)
> > > +{
> > > + struct device_node *cpu_node;
> > > + int ret;
> > > +
> > > + /*
> > > +  * If the PSCI cpu_suspend function hook has not been initialized
> > > +  * idle states must not be enabled, so bail out
> > > +  */
> > > + if (!psci_ops.cpu_suspend)
> > > + return -EOPNOTSUPP;
> > > +
> > > + cpu_node = of_get_cpu_node(cpu, NULL);
> >
> > [nit] You could use of_cpu_device_node_get in linux/of_device.h as
> > it may avoid parsing if used later during the boot(i.e. after
> > cpu->of_node is populated). I think there's another instance in
> > psci_idle_init_cpu
>
> Good idea!
>
> However, as $subject patch more or less just moves code from the
> current psci firmware directory into cpuidle, perhaps it's better to
> defer improvements to be made on top?
>

I am fine either way. But since cpuidle-psci.c is new file introduced
in this series, we can have it as part of the original patch. I leave it
to Lorenzo to decide :)

--
Regards,
Sudeep

Re: [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling

[Ulf Hansson]

On Thu, 8 Aug 2019 at 14:55, Sudeep Holla  wrote:
>
> On Mon, Jul 22, 2019 at 04:37:45PM +0100, Lorenzo Pieralisi wrote:
> > Current PSCI code handles idle state entry through the
> > psci_cpu_suspend_enter() API, that takes an idle state index as a
> > parameter and convert the index into a previously initialized
> > power_state parameter before calling the PSCI.CPU_SUSPEND() with it.
> >
> > This is unwieldly, since it forces the PSCI firmware layer to keep track
> > of power_state parameter for every idle state so that the
> > index->power_state conversion can be made in the PSCI firmware layer
> > instead of the CPUidle driver implementations.
> >
> > Move the power_state handling out of drivers/firmware/psci
> > into the respective ACPI/DT PSCI CPUidle backends and convert
> > the psci_cpu_suspend_enter() API to get the power_state
> > parameter as input, which makes it closer to its firmware
> > interface PSCI.CPU_SUSPEND() API.
> >
> > A notable side effect is that the PSCI ACPI/DT CPUidle backends
> > now can directly handle (and if needed update) power_state
> > parameters before handing them over to the PSCI firmware
> > interface to trigger PSCI.CPU_SUSPEND() calls.
> >
> > Signed-off-by: Lorenzo Pieralisi 
> > Cc: Will Deacon 
> > Cc: Ulf Hansson 
> > Cc: Sudeep Holla 
>
> Reviewed-by: Sudeep Holla 
>
> > +static __init int psci_cpu_init_idle(unsigned int cpu)
> > +{
> > + struct device_node *cpu_node;
> > + int ret;
> > +
> > + /*
> > +  * If the PSCI cpu_suspend function hook has not been initialized
> > +  * idle states must not be enabled, so bail out
> > +  */
> > + if (!psci_ops.cpu_suspend)
> > + return -EOPNOTSUPP;
> > +
> > + cpu_node = of_get_cpu_node(cpu, NULL);
>
> [nit] You could use of_cpu_device_node_get in linux/of_device.h as
> it may avoid parsing if used later during the boot(i.e. after
> cpu->of_node is populated). I think there's another instance in
> psci_idle_init_cpu

Good idea!

However, as $subject patch more or less just moves code from the
current psci firmware directory into cpuidle, perhaps it's better to
defer improvements to be made on top?

Kind regards
Uffe

Re: [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling

[Sudeep Holla]

On Mon, Jul 22, 2019 at 04:37:45PM +0100, Lorenzo Pieralisi wrote:
> Current PSCI code handles idle state entry through the
> psci_cpu_suspend_enter() API, that takes an idle state index as a
> parameter and convert the index into a previously initialized
> power_state parameter before calling the PSCI.CPU_SUSPEND() with it.
>
> This is unwieldly, since it forces the PSCI firmware layer to keep track
> of power_state parameter for every idle state so that the
> index->power_state conversion can be made in the PSCI firmware layer
> instead of the CPUidle driver implementations.
>
> Move the power_state handling out of drivers/firmware/psci
> into the respective ACPI/DT PSCI CPUidle backends and convert
> the psci_cpu_suspend_enter() API to get the power_state
> parameter as input, which makes it closer to its firmware
> interface PSCI.CPU_SUSPEND() API.
>
> A notable side effect is that the PSCI ACPI/DT CPUidle backends
> now can directly handle (and if needed update) power_state
> parameters before handing them over to the PSCI firmware
> interface to trigger PSCI.CPU_SUSPEND() calls.
>
> Signed-off-by: Lorenzo Pieralisi 
> Cc: Will Deacon 
> Cc: Ulf Hansson 
> Cc: Sudeep Holla 

Reviewed-by: Sudeep Holla 

> +static __init int psci_cpu_init_idle(unsigned int cpu)
> +{
> + struct device_node *cpu_node;
> + int ret;
> +
> + /*
> +  * If the PSCI cpu_suspend function hook has not been initialized
> +  * idle states must not be enabled, so bail out
> +  */
> + if (!psci_ops.cpu_suspend)
> + return -EOPNOTSUPP;
> +
> + cpu_node = of_get_cpu_node(cpu, NULL);

[nit] You could use of_cpu_device_node_get in linux/of_device.h as
it may avoid parsing if used later during the boot(i.e. after
cpu->of_node is populated). I think there's another instance in
psci_idle_init_cpu

--
Regards,
Sudeep

Re: [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling

[Daniel Lezcano]

On 22/07/2019 17:37, Lorenzo Pieralisi wrote:
> Current PSCI code handles idle state entry through the
> psci_cpu_suspend_enter() API, that takes an idle state index as a
> parameter and convert the index into a previously initialized
> power_state parameter before calling the PSCI.CPU_SUSPEND() with it.
> 
> This is unwieldly, since it forces the PSCI firmware layer to keep track
> of power_state parameter for every idle state so that the
> index->power_state conversion can be made in the PSCI firmware layer
> instead of the CPUidle driver implementations.
> 
> Move the power_state handling out of drivers/firmware/psci
> into the respective ACPI/DT PSCI CPUidle backends and convert
> the psci_cpu_suspend_enter() API to get the power_state
> parameter as input, which makes it closer to its firmware
> interface PSCI.CPU_SUSPEND() API.
> 
> A notable side effect is that the PSCI ACPI/DT CPUidle backends
> now can directly handle (and if needed update) power_state
> parameters before handing them over to the PSCI firmware
> interface to trigger PSCI.CPU_SUSPEND() calls.
> 
> Signed-off-by: Lorenzo Pieralisi 
> Cc: Will Deacon 
> Cc: Ulf Hansson 
> Cc: Sudeep Holla 
> Cc: Daniel Lezcano 
> Cc: Catalin Marinas 
> Cc: Mark Rutland 
> Cc: "Rafael J. Wysocki" 
> ---

AFAICT,

Acked-by: Daniel Lezcano 



-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling

[Ulf Hansson]

On Mon, 22 Jul 2019 at 17:38, Lorenzo Pieralisi
 wrote:
>
> Current PSCI code handles idle state entry through the
> psci_cpu_suspend_enter() API, that takes an idle state index as a
> parameter and convert the index into a previously initialized
> power_state parameter before calling the PSCI.CPU_SUSPEND() with it.
>
> This is unwieldly, since it forces the PSCI firmware layer to keep track
> of power_state parameter for every idle state so that the
> index->power_state conversion can be made in the PSCI firmware layer
> instead of the CPUidle driver implementations.
>
> Move the power_state handling out of drivers/firmware/psci
> into the respective ACPI/DT PSCI CPUidle backends and convert
> the psci_cpu_suspend_enter() API to get the power_state
> parameter as input, which makes it closer to its firmware
> interface PSCI.CPU_SUSPEND() API.
>
> A notable side effect is that the PSCI ACPI/DT CPUidle backends
> now can directly handle (and if needed update) power_state
> parameters before handing them over to the PSCI firmware
> interface to trigger PSCI.CPU_SUSPEND() calls.
>
> Signed-off-by: Lorenzo Pieralisi 
> Cc: Will Deacon 
> Cc: Ulf Hansson 
> Cc: Sudeep Holla 
> Cc: Daniel Lezcano 
> Cc: Catalin Marinas 
> Cc: Mark Rutland 
> Cc: "Rafael J. Wysocki" 
> ---
>  arch/arm64/kernel/cpuidle.c|  47 +-
>  drivers/cpuidle/cpuidle-psci.c |  87 +-
>  drivers/firmware/psci/psci.c   | 158 ++---
>  include/linux/cpuidle.h|  17 +++-
>  include/linux/psci.h   |   4 +-
>  5 files changed, 153 insertions(+), 160 deletions(-)
>
> diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
> index 4bcd1bca0dfc..e4d6af2fdec7 100644
> --- a/arch/arm64/kernel/cpuidle.c
> +++ b/arch/arm64/kernel/cpuidle.c
> @@ -47,6 +47,44 @@ int arm_cpuidle_suspend(int index)
>
>  #define ARM64_LPI_IS_RETENTION_STATE(arch_flags) (!(arch_flags))
>
> +static int psci_acpi_cpu_init_idle(unsigned int cpu)
> +{
> +   int i, count;
> +   struct acpi_lpi_state *lpi;
> +   struct acpi_processor *pr = per_cpu(processors, cpu);
> +
> +   /*
> +* If the PSCI cpu_suspend function hook has not been initialized
> +* idle states must not be enabled, so bail out
> +*/
> +   if (!psci_ops.cpu_suspend)
> +   return -EOPNOTSUPP;
> +
> +   if (unlikely(!pr || !pr->flags.has_lpi))
> +   return -EINVAL;
> +
> +   count = pr->power.count - 1;
> +   if (count <= 0)
> +   return -ENODEV;
> +
> +   for (i = 0; i < count; i++) {
> +   u32 state;
> +
> +   lpi = &pr->power.lpi_states[i + 1];
> +   /*
> +* Only bits[31:0] represent a PSCI power_state while
> +* bits[63:32] must be 0x0 as per ARM ACPI FFH Specification
> +*/
> +   state = lpi->address;
> +   if (!psci_power_state_is_valid(state)) {
> +   pr_warn("Invalid PSCI power state %#x\n", state);
> +   return -EINVAL;
> +   }
> +   }
> +
> +   return 0;
> +}
> +
>  int acpi_processor_ffh_lpi_probe(unsigned int cpu)
>  {
> return psci_acpi_cpu_init_idle(cpu);
> @@ -54,10 +92,13 @@ int acpi_processor_ffh_lpi_probe(unsigned int cpu)
>
>  int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
>  {
> +   u32 state = lpi->address;
> +
> if (ARM64_LPI_IS_RETENTION_STATE(lpi->arch_flags))
> -   return CPU_PM_CPU_IDLE_ENTER_RETENTION(psci_cpu_suspend_enter,
> -   lpi->index);
> +   return 
> CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(psci_cpu_suspend_enter,
> +   lpi->index, state);
> else
> -   return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, 
> lpi->index);
> +   return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
> +lpi->index, state);
>  }

I am not sure where the acpi+psci cpuidle code really belongs. Perhaps
some code should be moved into separate acpi+psci cpuidle driver?

In any case and whatever makes sense, it can be done on top of the
current series.

[...]

Kind regards
Uffe

[PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling

[Lorenzo Pieralisi]

Current PSCI code handles idle state entry through the
psci_cpu_suspend_enter() API, that takes an idle state index as a
parameter and convert the index into a previously initialized
power_state parameter before calling the PSCI.CPU_SUSPEND() with it.

This is unwieldly, since it forces the PSCI firmware layer to keep track
of power_state parameter for every idle state so that the
index->power_state conversion can be made in the PSCI firmware layer
instead of the CPUidle driver implementations.

Move the power_state handling out of drivers/firmware/psci
into the respective ACPI/DT PSCI CPUidle backends and convert
the psci_cpu_suspend_enter() API to get the power_state
parameter as input, which makes it closer to its firmware
interface PSCI.CPU_SUSPEND() API.

A notable side effect is that the PSCI ACPI/DT CPUidle backends
now can directly handle (and if needed update) power_state
parameters before handing them over to the PSCI firmware
interface to trigger PSCI.CPU_SUSPEND() calls.

Signed-off-by: Lorenzo Pieralisi 
Cc: Will Deacon 
Cc: Ulf Hansson 
Cc: Sudeep Holla 
Cc: Daniel Lezcano 
Cc: Catalin Marinas 
Cc: Mark Rutland 
Cc: "Rafael J. Wysocki" 
---
 arch/arm64/kernel/cpuidle.c|  47 +-
 drivers/cpuidle/cpuidle-psci.c |  87 +-
 drivers/firmware/psci/psci.c   | 158 ++---
 include/linux/cpuidle.h|  17 +++-
 include/linux/psci.h   |   4 +-
 5 files changed, 153 insertions(+), 160 deletions(-)

diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index 4bcd1bca0dfc..e4d6af2fdec7 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -47,6 +47,44 @@ int arm_cpuidle_suspend(int index)
 
 #define ARM64_LPI_IS_RETENTION_STATE(arch_flags) (!(arch_flags))
 
+static int psci_acpi_cpu_init_idle(unsigned int cpu)
+{
+   int i, count;
+   struct acpi_lpi_state *lpi;
+   struct acpi_processor *pr = per_cpu(processors, cpu);
+
+   /*
+* If the PSCI cpu_suspend function hook has not been initialized
+* idle states must not be enabled, so bail out
+*/
+   if (!psci_ops.cpu_suspend)
+   return -EOPNOTSUPP;
+
+   if (unlikely(!pr || !pr->flags.has_lpi))
+   return -EINVAL;
+
+   count = pr->power.count - 1;
+   if (count <= 0)
+   return -ENODEV;
+
+   for (i = 0; i < count; i++) {
+   u32 state;
+
+   lpi = &pr->power.lpi_states[i + 1];
+   /*
+* Only bits[31:0] represent a PSCI power_state while
+* bits[63:32] must be 0x0 as per ARM ACPI FFH Specification
+*/
+   state = lpi->address;
+   if (!psci_power_state_is_valid(state)) {
+   pr_warn("Invalid PSCI power state %#x\n", state);
+   return -EINVAL;
+   }
+   }
+
+   return 0;
+}
+
 int acpi_processor_ffh_lpi_probe(unsigned int cpu)
 {
return psci_acpi_cpu_init_idle(cpu);
@@ -54,10 +92,13 @@ int acpi_processor_ffh_lpi_probe(unsigned int cpu)
 
 int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
 {
+   u32 state = lpi->address;
+
if (ARM64_LPI_IS_RETENTION_STATE(lpi->arch_flags))
-   return CPU_PM_CPU_IDLE_ENTER_RETENTION(psci_cpu_suspend_enter,
-   lpi->index);
+   return 
CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(psci_cpu_suspend_enter,
+   lpi->index, state);
else
-   return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, 
lpi->index);
+   return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
+lpi->index, state);
 }
 #endif
diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index bdf02600e4e2..7485b3abe372 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -21,10 +21,15 @@
 
 #include "dt_idle_states.h"
 
+static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
+
 static int psci_enter_idle_state(struct cpuidle_device *dev,
struct cpuidle_driver *drv, int idx)
 {
-   return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, idx);
+   u32 *state = __this_cpu_read(psci_power_state);
+
+   return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
+  idx, state[idx - 1]);
 }
 
 static struct cpuidle_driver psci_idle_driver __initdata = {
@@ -50,6 +55,86 @@ 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 err = of_property_read_u32(np, "arm,psci-suspend-param", state);
+
+   if (err) {
+   pr_warn("%pOF missing arm,psci-suspend-param property\n", np);
+   return err;
+   }
+
+