[PATCH 0/7] drivers: firmware: psci: Some cleanup and refactoring

[PATCH 0/7] drivers: firmware: psci: Some cleanup and refactoring

[Ulf Hansson]

All of these patches have been sent earlier, but part of a bigger series [1].
Instead of waiting for that series to get reviewed and accepted, I have split
it up, such that these trivial independent cleanups can go in as a first step.

The only intentional function change, should be the last patch, where we start
checking if PSCI OSI mode is supported in the PSCI FW and print a message about
it, as that is quite useful information for the user/developer.

As there is a ARM/ARM64 "tree wide" change included, it would be nice if
this could go in already for v5.1-rc1, to set the foundation for the next cycle.
I know it's *really* late for that, but maybe Rafael can help take it via his
pm-tree, if all acks are received of course?

Kind regards
Ulf Hansson

[1]
https://lkml.org/lkml/2018/11/29/1801

Ulf Hansson (7):
  drivers: firmware: psci: Move psci to separate directory
  MAINTAINERS: Update files for PSCI
  drivers: firmware: psci: Split psci_dt_cpu_init_idle()
  ARM/ARM64: cpuidle: Let back-end init ops take the driver as input
  drivers: firmware: psci: Simplify state node parsing
  drivers: firmware: psci: Simplify error path of psci_dt_init()
  drivers: firmware: psci: Announce support for OS initiated suspend
    mode

 MAINTAINERS                                |   2 +-
 arch/arm/include/asm/cpuidle.h             |   4 +-
 arch/arm/kernel/cpuidle.c                  |   5 +-
 arch/arm64/include/asm/cpu_ops.h           |   4 +-
 arch/arm64/include/asm/cpuidle.h           |   6 +-
 arch/arm64/kernel/cpuidle.c                |   6 +-
 drivers/cpuidle/cpuidle-arm.c              |   2 +-
 drivers/firmware/Kconfig                   |  15 +--
 drivers/firmware/Makefile                  |   3 +-
 drivers/firmware/psci/Kconfig              |  13 +++
 drivers/firmware/psci/Makefile             |   4 +
 drivers/firmware/{ => psci}/psci.c         | 114 ++++++++++++---------
 drivers/firmware/{ => psci}/psci_checker.c |   0
 drivers/soc/qcom/spm.c                     |   3 +-
 include/linux/psci.h                       |   4 +-
 include/uapi/linux/psci.h                  |   5 +
 16 files changed, 113 insertions(+), 77 deletions(-)
 create mode 100644 drivers/firmware/psci/Kconfig
 create mode 100644 drivers/firmware/psci/Makefile
 rename drivers/firmware/{ => psci}/psci.c (91%)
 rename drivers/firmware/{ => psci}/psci_checker.c (100%)

-- 
2.17.1

[PATCH 1/7] drivers: firmware: psci: Move psci to separate directory

[Ulf Hansson]

Some following changes extends the PSCI driver with some additional new
files.  Let's avoid to continue cluttering the toplevel firmware directory
and first move the PSCI files into a PSCI sub-directory.

Suggested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/Kconfig                   | 15 +--------------
 drivers/firmware/Makefile                  |  3 +--
 drivers/firmware/psci/Kconfig              | 13 +++++++++++++
 drivers/firmware/psci/Makefile             |  4 ++++
 drivers/firmware/{ => psci}/psci.c         |  0
 drivers/firmware/{ => psci}/psci_checker.c |  0
 6 files changed, 19 insertions(+), 16 deletions(-)
 create mode 100644 drivers/firmware/psci/Kconfig
 create mode 100644 drivers/firmware/psci/Makefile
 rename drivers/firmware/{ => psci}/psci.c (100%)
 rename drivers/firmware/{ => psci}/psci_checker.c (100%)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index f754578414f0..e1385f47d4ac 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -5,20 +5,6 @@
 
 menu "Firmware Drivers"
 
-config ARM_PSCI_FW
-	bool
-
-config ARM_PSCI_CHECKER
-	bool "ARM PSCI checker"
-	depends on ARM_PSCI_FW && HOTPLUG_CPU && CPU_IDLE && !TORTURE_TEST
-	help
-	  Run the PSCI checker during startup. This checks that hotplug and
-	  suspend operations work correctly when using PSCI.
-
-	  The torture tests may interfere with the PSCI checker by turning CPUs
-	  on and off through hotplug, so for now torture tests and PSCI checker
-	  are mutually exclusive.
-
 config ARM_SCMI_PROTOCOL
 	bool "ARM System Control and Management Interface (SCMI) Message Protocol"
 	depends on ARM || ARM64 || COMPILE_TEST
@@ -270,6 +256,7 @@ config TI_SCI_PROTOCOL
 config HAVE_ARM_SMCCC
 	bool
 
+source "drivers/firmware/psci/Kconfig"
 source "drivers/firmware/broadcom/Kconfig"
 source "drivers/firmware/google/Kconfig"
 source "drivers/firmware/efi/Kconfig"
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 80feb635120f..9a3909a22682 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -2,8 +2,6 @@
 #
 # Makefile for the linux kernel.
 #
-obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
-obj-$(CONFIG_ARM_PSCI_CHECKER)	+= psci_checker.o
 obj-$(CONFIG_ARM_SCPI_PROTOCOL)	+= arm_scpi.o
 obj-$(CONFIG_ARM_SCPI_POWER_DOMAIN) += scpi_pm_domain.o
 obj-$(CONFIG_ARM_SDE_INTERFACE)	+= arm_sdei.o
@@ -25,6 +23,7 @@ CFLAGS_qcom_scm-32.o :=$(call as-instr,.arch armv7-a\n.arch_extension sec,-DREQU
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
 
 obj-$(CONFIG_ARM_SCMI_PROTOCOL)	+= arm_scmi/
+obj-y				+= psci/
 obj-y				+= broadcom/
 obj-y				+= meson/
 obj-$(CONFIG_GOOGLE_FIRMWARE)	+= google/
diff --git a/drivers/firmware/psci/Kconfig b/drivers/firmware/psci/Kconfig
new file mode 100644
index 000000000000..26a3b32bf7ab
--- /dev/null
+++ b/drivers/firmware/psci/Kconfig
@@ -0,0 +1,13 @@
+config ARM_PSCI_FW
+	bool
+
+config ARM_PSCI_CHECKER
+	bool "ARM PSCI checker"
+	depends on ARM_PSCI_FW && HOTPLUG_CPU && CPU_IDLE && !TORTURE_TEST
+	help
+	  Run the PSCI checker during startup. This checks that hotplug and
+	  suspend operations work correctly when using PSCI.
+
+	  The torture tests may interfere with the PSCI checker by turning CPUs
+	  on and off through hotplug, so for now torture tests and PSCI checker
+	  are mutually exclusive.
diff --git a/drivers/firmware/psci/Makefile b/drivers/firmware/psci/Makefile
new file mode 100644
index 000000000000..1956b882470f
--- /dev/null
+++ b/drivers/firmware/psci/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
+obj-$(CONFIG_ARM_PSCI_CHECKER)	+= psci_checker.o
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci/psci.c
similarity index 100%
rename from drivers/firmware/psci.c
rename to drivers/firmware/psci/psci.c
diff --git a/drivers/firmware/psci_checker.c b/drivers/firmware/psci/psci_checker.c
similarity index 100%
rename from drivers/firmware/psci_checker.c
rename to drivers/firmware/psci/psci_checker.c
-- 
2.17.1

Re: [PATCH 1/7] drivers: firmware: psci: Move psci to separate directory

[Daniel Lezcano]

On 28/02/2019 14:59, Ulf Hansson wrote:
> Some following changes extends the PSCI driver with some additional new
> files.  Let's avoid to continue cluttering the toplevel firmware directory
> and first move the PSCI files into a PSCI sub-directory.
> 
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


> ---
>  drivers/firmware/Kconfig                   | 15 +--------------
>  drivers/firmware/Makefile                  |  3 +--
>  drivers/firmware/psci/Kconfig              | 13 +++++++++++++
>  drivers/firmware/psci/Makefile             |  4 ++++
>  drivers/firmware/{ => psci}/psci.c         |  0
>  drivers/firmware/{ => psci}/psci_checker.c |  0
>  6 files changed, 19 insertions(+), 16 deletions(-)
>  create mode 100644 drivers/firmware/psci/Kconfig
>  create mode 100644 drivers/firmware/psci/Makefile
>  rename drivers/firmware/{ => psci}/psci.c (100%)
>  rename drivers/firmware/{ => psci}/psci_checker.c (100%)
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index f754578414f0..e1385f47d4ac 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -5,20 +5,6 @@
>  
>  menu "Firmware Drivers"
>  
> -config ARM_PSCI_FW
> -	bool
> -
> -config ARM_PSCI_CHECKER
> -	bool "ARM PSCI checker"
> -	depends on ARM_PSCI_FW && HOTPLUG_CPU && CPU_IDLE && !TORTURE_TEST
> -	help
> -	  Run the PSCI checker during startup. This checks that hotplug and
> -	  suspend operations work correctly when using PSCI.
> -
> -	  The torture tests may interfere with the PSCI checker by turning CPUs
> -	  on and off through hotplug, so for now torture tests and PSCI checker
> -	  are mutually exclusive.
> -
>  config ARM_SCMI_PROTOCOL
>  	bool "ARM System Control and Management Interface (SCMI) Message Protocol"
>  	depends on ARM || ARM64 || COMPILE_TEST
> @@ -270,6 +256,7 @@ config TI_SCI_PROTOCOL
>  config HAVE_ARM_SMCCC
>  	bool
>  
> +source "drivers/firmware/psci/Kconfig"
>  source "drivers/firmware/broadcom/Kconfig"
>  source "drivers/firmware/google/Kconfig"
>  source "drivers/firmware/efi/Kconfig"
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 80feb635120f..9a3909a22682 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -2,8 +2,6 @@
>  #
>  # Makefile for the linux kernel.
>  #
> -obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
> -obj-$(CONFIG_ARM_PSCI_CHECKER)	+= psci_checker.o
>  obj-$(CONFIG_ARM_SCPI_PROTOCOL)	+= arm_scpi.o
>  obj-$(CONFIG_ARM_SCPI_POWER_DOMAIN) += scpi_pm_domain.o
>  obj-$(CONFIG_ARM_SDE_INTERFACE)	+= arm_sdei.o
> @@ -25,6 +23,7 @@ CFLAGS_qcom_scm-32.o :=$(call as-instr,.arch armv7-a\n.arch_extension sec,-DREQU
>  obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
>  
>  obj-$(CONFIG_ARM_SCMI_PROTOCOL)	+= arm_scmi/
> +obj-y				+= psci/
>  obj-y				+= broadcom/
>  obj-y				+= meson/
>  obj-$(CONFIG_GOOGLE_FIRMWARE)	+= google/
> diff --git a/drivers/firmware/psci/Kconfig b/drivers/firmware/psci/Kconfig
> new file mode 100644
> index 000000000000..26a3b32bf7ab
> --- /dev/null
> +++ b/drivers/firmware/psci/Kconfig
> @@ -0,0 +1,13 @@
> +config ARM_PSCI_FW
> +	bool
> +
> +config ARM_PSCI_CHECKER
> +	bool "ARM PSCI checker"
> +	depends on ARM_PSCI_FW && HOTPLUG_CPU && CPU_IDLE && !TORTURE_TEST
> +	help
> +	  Run the PSCI checker during startup. This checks that hotplug and
> +	  suspend operations work correctly when using PSCI.
> +
> +	  The torture tests may interfere with the PSCI checker by turning CPUs
> +	  on and off through hotplug, so for now torture tests and PSCI checker
> +	  are mutually exclusive.
> diff --git a/drivers/firmware/psci/Makefile b/drivers/firmware/psci/Makefile
> new file mode 100644
> index 000000000000..1956b882470f
> --- /dev/null
> +++ b/drivers/firmware/psci/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
> +obj-$(CONFIG_ARM_PSCI_CHECKER)	+= psci_checker.o
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci/psci.c
> similarity index 100%
> rename from drivers/firmware/psci.c
> rename to drivers/firmware/psci/psci.c
> diff --git a/drivers/firmware/psci_checker.c b/drivers/firmware/psci/psci_checker.c
> similarity index 100%
> rename from drivers/firmware/psci_checker.c
> rename to drivers/firmware/psci/psci_checker.c
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/7] drivers: firmware: psci: Move psci to separate directory

[Mark Rutland]

On Thu, Feb 28, 2019 at 02:59:13PM +0100, Ulf Hansson wrote:
> Some following changes extends the PSCI driver with some additional new
> files.  Let's avoid to continue cluttering the toplevel firmware directory
> and first move the PSCI files into a PSCI sub-directory.
> 
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  drivers/firmware/Kconfig                   | 15 +--------------
>  drivers/firmware/Makefile                  |  3 +--
>  drivers/firmware/psci/Kconfig              | 13 +++++++++++++
>  drivers/firmware/psci/Makefile             |  4 ++++
>  drivers/firmware/{ => psci}/psci.c         |  0
>  drivers/firmware/{ => psci}/psci_checker.c |  0
>  6 files changed, 19 insertions(+), 16 deletions(-)
>  create mode 100644 drivers/firmware/psci/Kconfig
>  create mode 100644 drivers/firmware/psci/Makefile
>  rename drivers/firmware/{ => psci}/psci.c (100%)
>  rename drivers/firmware/{ => psci}/psci_checker.c (100%)
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index f754578414f0..e1385f47d4ac 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -5,20 +5,6 @@
>  
>  menu "Firmware Drivers"
>  
> -config ARM_PSCI_FW
> -	bool
> -
> -config ARM_PSCI_CHECKER
> -	bool "ARM PSCI checker"
> -	depends on ARM_PSCI_FW && HOTPLUG_CPU && CPU_IDLE && !TORTURE_TEST
> -	help
> -	  Run the PSCI checker during startup. This checks that hotplug and
> -	  suspend operations work correctly when using PSCI.
> -
> -	  The torture tests may interfere with the PSCI checker by turning CPUs
> -	  on and off through hotplug, so for now torture tests and PSCI checker
> -	  are mutually exclusive.
> -
>  config ARM_SCMI_PROTOCOL
>  	bool "ARM System Control and Management Interface (SCMI) Message Protocol"
>  	depends on ARM || ARM64 || COMPILE_TEST
> @@ -270,6 +256,7 @@ config TI_SCI_PROTOCOL
>  config HAVE_ARM_SMCCC
>  	bool
>  
> +source "drivers/firmware/psci/Kconfig"
>  source "drivers/firmware/broadcom/Kconfig"
>  source "drivers/firmware/google/Kconfig"
>  source "drivers/firmware/efi/Kconfig"
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 80feb635120f..9a3909a22682 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -2,8 +2,6 @@
>  #
>  # Makefile for the linux kernel.
>  #
> -obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
> -obj-$(CONFIG_ARM_PSCI_CHECKER)	+= psci_checker.o
>  obj-$(CONFIG_ARM_SCPI_PROTOCOL)	+= arm_scpi.o
>  obj-$(CONFIG_ARM_SCPI_POWER_DOMAIN) += scpi_pm_domain.o
>  obj-$(CONFIG_ARM_SDE_INTERFACE)	+= arm_sdei.o
> @@ -25,6 +23,7 @@ CFLAGS_qcom_scm-32.o :=$(call as-instr,.arch armv7-a\n.arch_extension sec,-DREQU
>  obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
>  
>  obj-$(CONFIG_ARM_SCMI_PROTOCOL)	+= arm_scmi/
> +obj-y				+= psci/
>  obj-y				+= broadcom/
>  obj-y				+= meson/
>  obj-$(CONFIG_GOOGLE_FIRMWARE)	+= google/
> diff --git a/drivers/firmware/psci/Kconfig b/drivers/firmware/psci/Kconfig
> new file mode 100644
> index 000000000000..26a3b32bf7ab
> --- /dev/null
> +++ b/drivers/firmware/psci/Kconfig
> @@ -0,0 +1,13 @@
> +config ARM_PSCI_FW
> +	bool
> +
> +config ARM_PSCI_CHECKER
> +	bool "ARM PSCI checker"
> +	depends on ARM_PSCI_FW && HOTPLUG_CPU && CPU_IDLE && !TORTURE_TEST
> +	help
> +	  Run the PSCI checker during startup. This checks that hotplug and
> +	  suspend operations work correctly when using PSCI.
> +
> +	  The torture tests may interfere with the PSCI checker by turning CPUs
> +	  on and off through hotplug, so for now torture tests and PSCI checker
> +	  are mutually exclusive.
> diff --git a/drivers/firmware/psci/Makefile b/drivers/firmware/psci/Makefile
> new file mode 100644
> index 000000000000..1956b882470f
> --- /dev/null
> +++ b/drivers/firmware/psci/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
> +obj-$(CONFIG_ARM_PSCI_CHECKER)	+= psci_checker.o
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci/psci.c
> similarity index 100%
> rename from drivers/firmware/psci.c
> rename to drivers/firmware/psci/psci.c
> diff --git a/drivers/firmware/psci_checker.c b/drivers/firmware/psci/psci_checker.c
> similarity index 100%
> rename from drivers/firmware/psci_checker.c
> rename to drivers/firmware/psci/psci_checker.c
> -- 
> 2.17.1
> 

[PATCH 2/7] MAINTAINERS: Update files for PSCI

[Ulf Hansson]

The files for the PSCI firmware driver were moved to a sub-directory, let's
update MAINTAINERS to reflect that.

Suggested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 32b76cb95f57..ff998605b773 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12182,7 +12182,7 @@ M:	Mark Rutland <mark.rutland@arm.com>
 M:	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
 L:	linux-arm-kernel@lists.infradead.org
 S:	Maintained
-F:	drivers/firmware/psci*.c
+F:	drivers/firmware/psci/
 F:	include/linux/psci.h
 F:	include/uapi/linux/psci.h
 
-- 
2.17.1

Re: [PATCH 2/7] MAINTAINERS: Update files for PSCI

[Daniel Lezcano]

On 28/02/2019 14:59, Ulf Hansson wrote:
> The files for the PSCI firmware driver were moved to a sub-directory, let's
> update MAINTAINERS to reflect that.
> 
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---
>  MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 32b76cb95f57..ff998605b773 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12182,7 +12182,7 @@ M:	Mark Rutland <mark.rutland@arm.com>
>  M:	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>  L:	linux-arm-kernel@lists.infradead.org
>  S:	Maintained
> -F:	drivers/firmware/psci*.c
> +F:	drivers/firmware/psci/
>  F:	include/linux/psci.h
>  F:	include/uapi/linux/psci.h
>  
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/7] MAINTAINERS: Update files for PSCI

[Mark Rutland]

On Thu, Feb 28, 2019 at 02:59:14PM +0100, Ulf Hansson wrote:
> The files for the PSCI firmware driver were moved to a sub-directory, let's
> update MAINTAINERS to reflect that.
> 
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 32b76cb95f57..ff998605b773 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12182,7 +12182,7 @@ M:	Mark Rutland <mark.rutland@arm.com>
>  M:	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>  L:	linux-arm-kernel@lists.infradead.org
>  S:	Maintained
> -F:	drivers/firmware/psci*.c
> +F:	drivers/firmware/psci/
>  F:	include/linux/psci.h
>  F:	include/uapi/linux/psci.h
>  
> -- 
> 2.17.1
> 

[PATCH 3/7] drivers: firmware: psci: Split psci_dt_cpu_init_idle()

[Ulf Hansson]

Let's split the psci_dt_cpu_init_idle() function into two functions. This
makes the code clearer and provides better re-usability.

Cc: Lina Iyer <ilina@codeaurora.org>
Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/psci/psci.c | 42 ++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index c80ec1d03274..9788bfc1cf8b 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -270,9 +270,26 @@ static int __init psci_features(u32 psci_func_id)
 #ifdef CONFIG_CPU_IDLE
 static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
 
+static int 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;
+	}
+
+	if (!psci_power_state_is_valid(*state)) {
+		pr_warn("Invalid PSCI power state %#x\n", *state);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 {
-	int i, ret, count = 0;
+	int i, ret = 0, count = 0;
 	u32 *psci_states;
 	struct device_node *state_node;
 
@@ -291,29 +308,16 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 		return -ENOMEM;
 
 	for (i = 0; i < count; i++) {
-		u32 state;
-
 		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+		ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
+		of_node_put(state_node);
 
-		ret = of_property_read_u32(state_node,
-					   "arm,psci-suspend-param",
-					   &state);
-		if (ret) {
-			pr_warn(" * %pOF missing arm,psci-suspend-param property\n",
-				state_node);
-			of_node_put(state_node);
+		if (ret)
 			goto free_mem;
-		}
 
-		of_node_put(state_node);
-		pr_debug("psci-power-state %#x index %d\n", state, i);
-		if (!psci_power_state_is_valid(state)) {
-			pr_warn("Invalid PSCI power state %#x\n", state);
-			ret = -EINVAL;
-			goto free_mem;
-		}
-		psci_states[i] = state;
+		pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
 	}
+
 	/* Idle states parsed correctly, initialize per-cpu pointer */
 	per_cpu(psci_power_state, cpu) = psci_states;
 	return 0;
-- 
2.17.1

Re: [PATCH 3/7] drivers: firmware: psci: Split psci_dt_cpu_init_idle()

[Daniel Lezcano]

On 28/02/2019 14:59, Ulf Hansson wrote:
> Let's split the psci_dt_cpu_init_idle() function into two functions. This
> makes the code clearer and provides better re-usability.
> 
> Cc: Lina Iyer <ilina@codeaurora.org>
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

but one question below.


> ---
>  drivers/firmware/psci/psci.c | 42 ++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index c80ec1d03274..9788bfc1cf8b 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -270,9 +270,26 @@ static int __init psci_features(u32 psci_func_id)
>  #ifdef CONFIG_CPU_IDLE
>  static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
>  
> +static int 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;
> +	}
> +
> +	if (!psci_power_state_is_valid(*state)) {
> +		pr_warn("Invalid PSCI power state %#x\n", *state);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>  {
> -	int i, ret, count = 0;
> +	int i, ret = 0, count = 0;

Why do you need to intialize the ret variable? If the count is zero we
go directly to return 0, otherwise we enter in the loop and ret is
affected with the new function call.

>  	u32 *psci_states;
>  	struct device_node *state_node;
>  
> @@ -291,29 +308,16 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>  		return -ENOMEM;
>  
>  	for (i = 0; i < count; i++) {
> -		u32 state;
> -
>  		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
> +		ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
> +		of_node_put(state_node);
>  
> -		ret = of_property_read_u32(state_node,
> -					   "arm,psci-suspend-param",
> -					   &state);
> -		if (ret) {
> -			pr_warn(" * %pOF missing arm,psci-suspend-param property\n",
> -				state_node);
> -			of_node_put(state_node);
> +		if (ret)
>  			goto free_mem;
> -		}
>  
> -		of_node_put(state_node);
> -		pr_debug("psci-power-state %#x index %d\n", state, i);
> -		if (!psci_power_state_is_valid(state)) {
> -			pr_warn("Invalid PSCI power state %#x\n", state);
> -			ret = -EINVAL;
> -			goto free_mem;
> -		}
> -		psci_states[i] = state;
> +		pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
>  	}
> +
>  	/* Idle states parsed correctly, initialize per-cpu pointer */
>  	per_cpu(psci_power_state, cpu) = psci_states;
>  	return 0;
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 3/7] drivers: firmware: psci: Split psci_dt_cpu_init_idle()

[Ulf Hansson]

On Thu, 28 Feb 2019 at 15:42, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 28/02/2019 14:59, Ulf Hansson wrote:
> > Let's split the psci_dt_cpu_init_idle() function into two functions. This
> > makes the code clearer and provides better re-usability.
> >
> > Cc: Lina Iyer <ilina@codeaurora.org>
> > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>
> but one question below.
>
>
> > ---
> >  drivers/firmware/psci/psci.c | 42 ++++++++++++++++++++----------------
> >  1 file changed, 23 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > index c80ec1d03274..9788bfc1cf8b 100644
> > --- a/drivers/firmware/psci/psci.c
> > +++ b/drivers/firmware/psci/psci.c
> > @@ -270,9 +270,26 @@ static int __init psci_features(u32 psci_func_id)
> >  #ifdef CONFIG_CPU_IDLE
> >  static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> >
> > +static int 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;
> > +     }
> > +
> > +     if (!psci_power_state_is_valid(*state)) {
> > +             pr_warn("Invalid PSCI power state %#x\n", *state);
> > +             return -EINVAL;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> >  {
> > -     int i, ret, count = 0;
> > +     int i, ret = 0, count = 0;
>
> Why do you need to intialize the ret variable? If the count is zero we
> go directly to return 0, otherwise we enter in the loop and ret is
> affected with the new function call.

Depending on the compiler and the compiler flags, one could otherwise
potentially get a warning about using an uninitialized variable at the
free_mem label (not shown in the patch).

So, I just wanted to play safe.

Thanks a lot for reviewing!

Kind regards
Uffe

Re: [PATCH 3/7] drivers: firmware: psci: Split psci_dt_cpu_init_idle()

[Mark Rutland]

On Thu, Feb 28, 2019 at 02:59:15PM +0100, Ulf Hansson wrote:
> Let's split the psci_dt_cpu_init_idle() function into two functions. This
> makes the code clearer and provides better re-usability.
> 
> Cc: Lina Iyer <ilina@codeaurora.org>
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  drivers/firmware/psci/psci.c | 42 ++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index c80ec1d03274..9788bfc1cf8b 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -270,9 +270,26 @@ static int __init psci_features(u32 psci_func_id)
>  #ifdef CONFIG_CPU_IDLE
>  static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
>  
> +static int 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;
> +	}
> +
> +	if (!psci_power_state_is_valid(*state)) {
> +		pr_warn("Invalid PSCI power state %#x\n", *state);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>  {
> -	int i, ret, count = 0;
> +	int i, ret = 0, count = 0;
>  	u32 *psci_states;
>  	struct device_node *state_node;
>  
> @@ -291,29 +308,16 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>  		return -ENOMEM;
>  
>  	for (i = 0; i < count; i++) {
> -		u32 state;
> -
>  		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
> +		ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
> +		of_node_put(state_node);
>  
> -		ret = of_property_read_u32(state_node,
> -					   "arm,psci-suspend-param",
> -					   &state);
> -		if (ret) {
> -			pr_warn(" * %pOF missing arm,psci-suspend-param property\n",
> -				state_node);
> -			of_node_put(state_node);
> +		if (ret)
>  			goto free_mem;
> -		}
>  
> -		of_node_put(state_node);
> -		pr_debug("psci-power-state %#x index %d\n", state, i);
> -		if (!psci_power_state_is_valid(state)) {
> -			pr_warn("Invalid PSCI power state %#x\n", state);
> -			ret = -EINVAL;
> -			goto free_mem;
> -		}
> -		psci_states[i] = state;
> +		pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
>  	}
> +
>  	/* Idle states parsed correctly, initialize per-cpu pointer */
>  	per_cpu(psci_power_state, cpu) = psci_states;
>  	return 0;
> -- 
> 2.17.1
> 

[PATCH 4/7] ARM/ARM64: cpuidle: Let back-end init ops take the driver as input

[Ulf Hansson]

To allow arch back-end init ops to operate on the cpuidle driver for the
corresponding CPU, let's pass along a pointer to the struct cpuidle_driver*
and forward it the relevant layers of callbacks for ARM/ARM64.

Following changes for the PSCI firmware driver starts making use of this.

Cc: Lina Iyer <ilina@codeaurora.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Andy Gross <andy.gross@linaro.org>
Cc: David Brown <david.brown@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 arch/arm/include/asm/cpuidle.h   | 4 ++--
 arch/arm/kernel/cpuidle.c        | 5 +++--
 arch/arm64/include/asm/cpu_ops.h | 4 +++-
 arch/arm64/include/asm/cpuidle.h | 6 ++++--
 arch/arm64/kernel/cpuidle.c      | 6 +++---
 drivers/cpuidle/cpuidle-arm.c    | 2 +-
 drivers/firmware/psci/psci.c     | 7 ++++---
 drivers/soc/qcom/spm.c           | 3 ++-
 include/linux/psci.h             | 4 +++-
 9 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
index 6b2ff7243b4b..bee0a7847733 100644
--- a/arch/arm/include/asm/cpuidle.h
+++ b/arch/arm/include/asm/cpuidle.h
@@ -32,7 +32,7 @@ struct device_node;
 
 struct cpuidle_ops {
 	int (*suspend)(unsigned long arg);
-	int (*init)(struct device_node *, int cpu);
+	int (*init)(struct cpuidle_driver *, struct device_node *, int cpu);
 };
 
 struct of_cpuidle_method {
@@ -47,6 +47,6 @@ struct of_cpuidle_method {
 
 extern int arm_cpuidle_suspend(int index);
 
-extern int arm_cpuidle_init(int cpu);
+extern int arm_cpuidle_init(struct cpuidle_driver *drv, int cpu);
 
 #endif
diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
index fda5579123a8..43778c9b373d 100644
--- a/arch/arm/kernel/cpuidle.c
+++ b/arch/arm/kernel/cpuidle.c
@@ -122,6 +122,7 @@ static int __init arm_cpuidle_read_ops(struct device_node *dn, int cpu)
 
 /**
  * arm_cpuidle_init() - Initialize cpuidle_ops for a specific cpu
+ * @drv: the drv to be initialized
  * @cpu: the cpu to be initialized
  *
  * Initialize the cpuidle ops with the device for the cpu and then call
@@ -137,7 +138,7 @@ static int __init arm_cpuidle_read_ops(struct device_node *dn, int cpu)
  *  -ENXIO if the HW reports a failure or a misconfiguration,
  *  -ENOMEM if the HW report an memory allocation failure 
  */
-int __init arm_cpuidle_init(int cpu)
+int __init arm_cpuidle_init(struct cpuidle_driver *drv, int cpu)
 {
 	struct device_node *cpu_node = of_cpu_device_node_get(cpu);
 	int ret;
@@ -147,7 +148,7 @@ int __init arm_cpuidle_init(int cpu)
 
 	ret = arm_cpuidle_read_ops(cpu_node, cpu);
 	if (!ret)
-		ret = cpuidle_ops[cpu].init(cpu_node, cpu);
+		ret = cpuidle_ops[cpu].init(drv, cpu_node, cpu);
 
 	of_node_put(cpu_node);
 
diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
index 8f03446cf89f..8db870c29f1b 100644
--- a/arch/arm64/include/asm/cpu_ops.h
+++ b/arch/arm64/include/asm/cpu_ops.h
@@ -19,6 +19,8 @@
 #include <linux/init.h>
 #include <linux/threads.h>
 
+struct cpuidle_driver;
+
 /**
  * struct cpu_operations - Callback operations for hotplugging CPUs.
  *
@@ -58,7 +60,7 @@ struct cpu_operations {
 	int		(*cpu_kill)(unsigned int cpu);
 #endif
 #ifdef CONFIG_CPU_IDLE
-	int		(*cpu_init_idle)(unsigned int);
+	int		(*cpu_init_idle)(struct cpuidle_driver *, unsigned int);
 	int		(*cpu_suspend)(unsigned long);
 #endif
 };
diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
index 3c5ddb429ea2..3fd3efb61649 100644
--- a/arch/arm64/include/asm/cpuidle.h
+++ b/arch/arm64/include/asm/cpuidle.h
@@ -4,11 +4,13 @@
 
 #include <asm/proc-fns.h>
 
+struct cpuidle_driver;
+
 #ifdef CONFIG_CPU_IDLE
-extern int arm_cpuidle_init(unsigned int cpu);
+extern int arm_cpuidle_init(struct cpuidle_driver *drv, unsigned int cpu);
 extern int arm_cpuidle_suspend(int index);
 #else
-static inline int arm_cpuidle_init(unsigned int cpu)
+static inline int arm_cpuidle_init(struct cpuidle_driver *drv, unsigned int cpu)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index f2d13810daa8..aaf9dc5cb87a 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -18,13 +18,13 @@
 #include <asm/cpuidle.h>
 #include <asm/cpu_ops.h>
 
-int arm_cpuidle_init(unsigned int cpu)
+int arm_cpuidle_init(struct cpuidle_driver *drv, unsigned int cpu)
 {
 	int ret = -EOPNOTSUPP;
 
 	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_suspend &&
 			cpu_ops[cpu]->cpu_init_idle)
-		ret = cpu_ops[cpu]->cpu_init_idle(cpu);
+		ret = cpu_ops[cpu]->cpu_init_idle(drv, cpu);
 
 	return ret;
 }
@@ -51,7 +51,7 @@ int arm_cpuidle_suspend(int index)
 
 int acpi_processor_ffh_lpi_probe(unsigned int cpu)
 {
-	return arm_cpuidle_init(cpu);
+	return arm_cpuidle_init(NULL, cpu);
 }
 
 int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 3a407a3ef22b..39413973b21d 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -106,7 +106,7 @@ static int __init arm_idle_init_cpu(int cpu)
 	 * Call arch CPU operations in order to initialize
 	 * idle states suspend back-end specific data
 	 */
-	ret = arm_cpuidle_init(cpu);
+	ret = arm_cpuidle_init(drv, cpu);
 
 	/*
 	 * Allow the initialization to continue for other CPUs, if the reported
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index 9788bfc1cf8b..d50b46a0528f 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -287,7 +287,8 @@ static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
 	return 0;
 }
 
-static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
+static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
+			struct device_node *cpu_node, int cpu)
 {
 	int i, ret = 0, count = 0;
 	u32 *psci_states;
@@ -375,7 +376,7 @@ static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
 }
 #endif
 
-int psci_cpu_init_idle(unsigned int cpu)
+int psci_cpu_init_idle(struct cpuidle_driver *drv, unsigned int cpu)
 {
 	struct device_node *cpu_node;
 	int ret;
@@ -394,7 +395,7 @@ 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(drv, cpu_node, cpu);
 
 	of_node_put(cpu_node);
 
diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
index 53807e839664..6e967f0a8608 100644
--- a/drivers/soc/qcom/spm.c
+++ b/drivers/soc/qcom/spm.c
@@ -208,7 +208,8 @@ static const struct of_device_id qcom_idle_state_match[] __initconst = {
 	{ },
 };
 
-static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
+static int __init qcom_cpuidle_init(struct cpuidle_driver *drv,
+			struct device_node *cpu_node, int cpu)
 {
 	const struct of_device_id *match_id;
 	struct device_node *state_node;
diff --git a/include/linux/psci.h b/include/linux/psci.h
index 8b1b3b5935ab..4f29a3bff379 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -20,9 +20,11 @@
 #define PSCI_POWER_STATE_TYPE_STANDBY		0
 #define PSCI_POWER_STATE_TYPE_POWER_DOWN	1
 
+struct cpuidle_driver;
+
 bool psci_tos_resident_on(int cpu);
 
-int psci_cpu_init_idle(unsigned int cpu);
+int psci_cpu_init_idle(struct cpuidle_driver *drv, unsigned int cpu);
 int psci_cpu_suspend_enter(unsigned long index);
 
 enum psci_conduit {
-- 
2.17.1

Re: [PATCH 4/7] ARM/ARM64: cpuidle: Let back-end init ops take the driver as input

[Daniel Lezcano]

On 28/02/2019 14:59, Ulf Hansson wrote:
> To allow arch back-end init ops to operate on the cpuidle driver for the
> corresponding CPU, let's pass along a pointer to the struct cpuidle_driver*
> and forward it the relevant layers of callbacks for ARM/ARM64.
> 
> Following changes for the PSCI firmware driver starts making use of this.
> 
> Cc: Lina Iyer <ilina@codeaurora.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Andy Gross <andy.gross@linaro.org>
> Cc: David Brown <david.brown@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 4/7] ARM/ARM64: cpuidle: Let back-end init ops take the driver as input

[Mark Rutland]

On Thu, Feb 28, 2019 at 02:59:16PM +0100, Ulf Hansson wrote:
> To allow arch back-end init ops to operate on the cpuidle driver for the
> corresponding CPU, let's pass along a pointer to the struct cpuidle_driver*
> and forward it the relevant layers of callbacks for ARM/ARM64.
> 
> Following changes for the PSCI firmware driver starts making use of this.
> 
> Cc: Lina Iyer <ilina@codeaurora.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Andy Gross <andy.gross@linaro.org>
> Cc: David Brown <david.brown@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

In general, I'd prefer to avoid tightening the coupling between the
backend and the cpuidle driver, so if this is only necessary for the
next patch (to get at the driver's state count), I'd prefer to avoid it
for now.

I assume that for OSI there are other reasons we'll want to get at the
driver in the backend init ops?

Thanks,
Mark.

> ---
>  arch/arm/include/asm/cpuidle.h   | 4 ++--
>  arch/arm/kernel/cpuidle.c        | 5 +++--
>  arch/arm64/include/asm/cpu_ops.h | 4 +++-
>  arch/arm64/include/asm/cpuidle.h | 6 ++++--
>  arch/arm64/kernel/cpuidle.c      | 6 +++---
>  drivers/cpuidle/cpuidle-arm.c    | 2 +-
>  drivers/firmware/psci/psci.c     | 7 ++++---
>  drivers/soc/qcom/spm.c           | 3 ++-
>  include/linux/psci.h             | 4 +++-
>  9 files changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
> index 6b2ff7243b4b..bee0a7847733 100644
> --- a/arch/arm/include/asm/cpuidle.h
> +++ b/arch/arm/include/asm/cpuidle.h
> @@ -32,7 +32,7 @@ struct device_node;
>  
>  struct cpuidle_ops {
>  	int (*suspend)(unsigned long arg);
> -	int (*init)(struct device_node *, int cpu);
> +	int (*init)(struct cpuidle_driver *, struct device_node *, int cpu);
>  };
>  
>  struct of_cpuidle_method {
> @@ -47,6 +47,6 @@ struct of_cpuidle_method {
>  
>  extern int arm_cpuidle_suspend(int index);
>  
> -extern int arm_cpuidle_init(int cpu);
> +extern int arm_cpuidle_init(struct cpuidle_driver *drv, int cpu);
>  
>  #endif
> diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
> index fda5579123a8..43778c9b373d 100644
> --- a/arch/arm/kernel/cpuidle.c
> +++ b/arch/arm/kernel/cpuidle.c
> @@ -122,6 +122,7 @@ static int __init arm_cpuidle_read_ops(struct device_node *dn, int cpu)
>  
>  /**
>   * arm_cpuidle_init() - Initialize cpuidle_ops for a specific cpu
> + * @drv: the drv to be initialized
>   * @cpu: the cpu to be initialized
>   *
>   * Initialize the cpuidle ops with the device for the cpu and then call
> @@ -137,7 +138,7 @@ static int __init arm_cpuidle_read_ops(struct device_node *dn, int cpu)
>   *  -ENXIO if the HW reports a failure or a misconfiguration,
>   *  -ENOMEM if the HW report an memory allocation failure 
>   */
> -int __init arm_cpuidle_init(int cpu)
> +int __init arm_cpuidle_init(struct cpuidle_driver *drv, int cpu)
>  {
>  	struct device_node *cpu_node = of_cpu_device_node_get(cpu);
>  	int ret;
> @@ -147,7 +148,7 @@ int __init arm_cpuidle_init(int cpu)
>  
>  	ret = arm_cpuidle_read_ops(cpu_node, cpu);
>  	if (!ret)
> -		ret = cpuidle_ops[cpu].init(cpu_node, cpu);
> +		ret = cpuidle_ops[cpu].init(drv, cpu_node, cpu);
>  
>  	of_node_put(cpu_node);
>  
> diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
> index 8f03446cf89f..8db870c29f1b 100644
> --- a/arch/arm64/include/asm/cpu_ops.h
> +++ b/arch/arm64/include/asm/cpu_ops.h
> @@ -19,6 +19,8 @@
>  #include <linux/init.h>
>  #include <linux/threads.h>
>  
> +struct cpuidle_driver;
> +
>  /**
>   * struct cpu_operations - Callback operations for hotplugging CPUs.
>   *
> @@ -58,7 +60,7 @@ struct cpu_operations {
>  	int		(*cpu_kill)(unsigned int cpu);
>  #endif
>  #ifdef CONFIG_CPU_IDLE
> -	int		(*cpu_init_idle)(unsigned int);
> +	int		(*cpu_init_idle)(struct cpuidle_driver *, unsigned int);
>  	int		(*cpu_suspend)(unsigned long);
>  #endif
>  };
> diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
> index 3c5ddb429ea2..3fd3efb61649 100644
> --- a/arch/arm64/include/asm/cpuidle.h
> +++ b/arch/arm64/include/asm/cpuidle.h
> @@ -4,11 +4,13 @@
>  
>  #include <asm/proc-fns.h>
>  
> +struct cpuidle_driver;
> +
>  #ifdef CONFIG_CPU_IDLE
> -extern int arm_cpuidle_init(unsigned int cpu);
> +extern int arm_cpuidle_init(struct cpuidle_driver *drv, unsigned int cpu);
>  extern int arm_cpuidle_suspend(int index);
>  #else
> -static inline int arm_cpuidle_init(unsigned int cpu)
> +static inline int arm_cpuidle_init(struct cpuidle_driver *drv, unsigned int cpu)
>  {
>  	return -EOPNOTSUPP;
>  }
> diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
> index f2d13810daa8..aaf9dc5cb87a 100644
> --- a/arch/arm64/kernel/cpuidle.c
> +++ b/arch/arm64/kernel/cpuidle.c
> @@ -18,13 +18,13 @@
>  #include <asm/cpuidle.h>
>  #include <asm/cpu_ops.h>
>  
> -int arm_cpuidle_init(unsigned int cpu)
> +int arm_cpuidle_init(struct cpuidle_driver *drv, unsigned int cpu)
>  {
>  	int ret = -EOPNOTSUPP;
>  
>  	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_suspend &&
>  			cpu_ops[cpu]->cpu_init_idle)
> -		ret = cpu_ops[cpu]->cpu_init_idle(cpu);
> +		ret = cpu_ops[cpu]->cpu_init_idle(drv, cpu);
>  
>  	return ret;
>  }
> @@ -51,7 +51,7 @@ int arm_cpuidle_suspend(int index)
>  
>  int acpi_processor_ffh_lpi_probe(unsigned int cpu)
>  {
> -	return arm_cpuidle_init(cpu);
> +	return arm_cpuidle_init(NULL, cpu);
>  }
>  
>  int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index 3a407a3ef22b..39413973b21d 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -106,7 +106,7 @@ static int __init arm_idle_init_cpu(int cpu)
>  	 * Call arch CPU operations in order to initialize
>  	 * idle states suspend back-end specific data
>  	 */
> -	ret = arm_cpuidle_init(cpu);
> +	ret = arm_cpuidle_init(drv, cpu);
>  
>  	/*
>  	 * Allow the initialization to continue for other CPUs, if the reported
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index 9788bfc1cf8b..d50b46a0528f 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -287,7 +287,8 @@ static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
>  	return 0;
>  }
>  
> -static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> +static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> +			struct device_node *cpu_node, int cpu)
>  {
>  	int i, ret = 0, count = 0;
>  	u32 *psci_states;
> @@ -375,7 +376,7 @@ static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
>  }
>  #endif
>  
> -int psci_cpu_init_idle(unsigned int cpu)
> +int psci_cpu_init_idle(struct cpuidle_driver *drv, unsigned int cpu)
>  {
>  	struct device_node *cpu_node;
>  	int ret;
> @@ -394,7 +395,7 @@ 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(drv, cpu_node, cpu);
>  
>  	of_node_put(cpu_node);
>  
> diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
> index 53807e839664..6e967f0a8608 100644
> --- a/drivers/soc/qcom/spm.c
> +++ b/drivers/soc/qcom/spm.c
> @@ -208,7 +208,8 @@ static const struct of_device_id qcom_idle_state_match[] __initconst = {
>  	{ },
>  };
>  
> -static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
> +static int __init qcom_cpuidle_init(struct cpuidle_driver *drv,
> +			struct device_node *cpu_node, int cpu)
>  {
>  	const struct of_device_id *match_id;
>  	struct device_node *state_node;
> diff --git a/include/linux/psci.h b/include/linux/psci.h
> index 8b1b3b5935ab..4f29a3bff379 100644
> --- a/include/linux/psci.h
> +++ b/include/linux/psci.h
> @@ -20,9 +20,11 @@
>  #define PSCI_POWER_STATE_TYPE_STANDBY		0
>  #define PSCI_POWER_STATE_TYPE_POWER_DOWN	1
>  
> +struct cpuidle_driver;
> +
>  bool psci_tos_resident_on(int cpu);
>  
> -int psci_cpu_init_idle(unsigned int cpu);
> +int psci_cpu_init_idle(struct cpuidle_driver *drv, unsigned int cpu);
>  int psci_cpu_suspend_enter(unsigned long index);
>  
>  enum psci_conduit {
> -- 
> 2.17.1
> 

[PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Ulf Hansson]

Instead of iterating through all the state nodes in DT, to find out how
many states that needs to be allocated, let's use the number already known
by the cpuidle driver. In this way we can drop the iteration altogether.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/psci/psci.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index d50b46a0528f..cbfc936d251c 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -290,26 +290,20 @@ static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
 static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
 			struct device_node *cpu_node, int cpu)
 {
-	int i, ret = 0, count = 0;
+	int i, ret = 0, num_state_nodes = drv->state_count - 1;
 	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(num_state_nodes, sizeof(*psci_states),
+			GFP_KERNEL);
 	if (!psci_states)
 		return -ENOMEM;
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < num_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);
 
@@ -319,6 +313,11 @@ static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
 		pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
 	}
 
+	if (i != num_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;
-- 
2.17.1

Re: [PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Daniel Lezcano]

On 28/02/2019 14:59, Ulf Hansson wrote:
> Instead of iterating through all the state nodes in DT, to find out how
> many states that needs to be allocated, let's use the number already known
> by the cpuidle driver. In this way we can drop the iteration altogether.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/firmware/psci/psci.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index d50b46a0528f..cbfc936d251c 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -290,26 +290,20 @@ static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
>  static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
>  			struct device_node *cpu_node, int cpu)
>  {
> -	int i, ret = 0, count = 0;
> +	int i, ret = 0, num_state_nodes = drv->state_count - 1;

why -1 ?

>  	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(num_state_nodes, sizeof(*psci_states),
> +			GFP_KERNEL);
>  	if (!psci_states)
>  		return -ENOMEM;
>  
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < num_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);
>  
> @@ -319,6 +313,11 @@ static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
>  		pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
>  	}
>  
> +	if (i != num_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;
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Ulf Hansson]

On Thu, 28 Feb 2019 at 16:40, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 28/02/2019 14:59, Ulf Hansson wrote:
> > Instead of iterating through all the state nodes in DT, to find out how
> > many states that needs to be allocated, let's use the number already known
> > by the cpuidle driver. In this way we can drop the iteration altogether.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/firmware/psci/psci.c | 25 ++++++++++++-------------
> >  1 file changed, 12 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > index d50b46a0528f..cbfc936d251c 100644
> > --- a/drivers/firmware/psci/psci.c
> > +++ b/drivers/firmware/psci/psci.c
> > @@ -290,26 +290,20 @@ static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
> >  static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> >                       struct device_node *cpu_node, int cpu)
> >  {
> > -     int i, ret = 0, count = 0;
> > +     int i, ret = 0, num_state_nodes = drv->state_count - 1;
>
> why -1 ?

Because of the WFI state. The cpuidle-arm driver always carries this
state at index 0, which also is never used in
psci_cpu_suspend_enter(), where the similar is taken into account.

It's a bit of magic, so perhaps someone should post a patch that
documents this a bit better in the code, wherever it makes sense.

>
> >       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(num_state_nodes, sizeof(*psci_states),
> > +                     GFP_KERNEL);
> >       if (!psci_states)
> >               return -ENOMEM;
> >
> > -     for (i = 0; i < count; i++) {
> > +     for (i = 0; i < num_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);
> >
> > @@ -319,6 +313,11 @@ static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> >               pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
> >       }
> >
> > +     if (i != num_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;
> >

Again, thanks a lot for reviewing!

Kind regards
Uffe

Re: [PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Daniel Lezcano]

On 28/02/2019 23:26, Ulf Hansson wrote:
> On Thu, 28 Feb 2019 at 16:40, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 28/02/2019 14:59, Ulf Hansson wrote:
>>> Instead of iterating through all the state nodes in DT, to find out how
>>> many states that needs to be allocated, let's use the number already known
>>> by the cpuidle driver. In this way we can drop the iteration altogether.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/firmware/psci/psci.c | 25 ++++++++++++-------------
>>>  1 file changed, 12 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
>>> index d50b46a0528f..cbfc936d251c 100644
>>> --- a/drivers/firmware/psci/psci.c
>>> +++ b/drivers/firmware/psci/psci.c
>>> @@ -290,26 +290,20 @@ static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
>>>  static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
>>>                       struct device_node *cpu_node, int cpu)
>>>  {
>>> -     int i, ret = 0, count = 0;
>>> +     int i, ret = 0, num_state_nodes = drv->state_count - 1;
>>
>> why -1 ?
> 
> Because of the WFI state. The cpuidle-arm driver always carries this
> state at index 0, which also is never used in
> psci_cpu_suspend_enter(), where the similar is taken into account.
> 
> It's a bit of magic, so perhaps someone should post a patch that
> documents this a bit better in the code, wherever it makes sense.

Ah yes, right. The first state is already filled with the WFI state in
the cpuidle-arm.c driver and we must infer the index passed to
dt_init_idle_driver is 1 because of that.

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


[ ... ]


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Mark Rutland]

On Thu, Feb 28, 2019 at 02:59:17PM +0100, Ulf Hansson wrote:
> Instead of iterating through all the state nodes in DT, to find out how
> many states that needs to be allocated, let's use the number already known
> by the cpuidle driver. In this way we can drop the iteration altogether.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/firmware/psci/psci.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index d50b46a0528f..cbfc936d251c 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -290,26 +290,20 @@ static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
>  static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
>  			struct device_node *cpu_node, int cpu)
>  {
> -	int i, ret = 0, count = 0;
> +	int i, ret = 0, num_state_nodes = drv->state_count - 1;
>  	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);
> -	}
> -

To be honest, I'd rather not tighten the coupling with the cpuidle
driver here. For example, I'm not that happy with the PSCI backend
having to know the driver has a specific WFI state.

IIUC we could get rid of the explicit counting with something like:

	count = of_parse_phandle_with_args(cpu_node, "cpu-idle-states", NULL);

... but I'm not sure that the overall change is a simplification.

Does this change make it easier to plumb in something in future?

Thanks,
Mark.

> -	if (!count)
> -		return -ENODEV;
> -
> -	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
> +	psci_states = kcalloc(num_state_nodes, sizeof(*psci_states),
> +			GFP_KERNEL);
>  	if (!psci_states)
>  		return -ENOMEM;
>  
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < num_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);
>  
> @@ -319,6 +313,11 @@ static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
>  		pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
>  	}
>  
> +	if (i != num_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;
> -- 
> 2.17.1
> 

Re: [PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Ulf Hansson]

On Fri, 1 Mar 2019 at 18:28, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Thu, Feb 28, 2019 at 02:59:17PM +0100, Ulf Hansson wrote:
> > Instead of iterating through all the state nodes in DT, to find out how
> > many states that needs to be allocated, let's use the number already known
> > by the cpuidle driver. In this way we can drop the iteration altogether.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/firmware/psci/psci.c | 25 ++++++++++++-------------
> >  1 file changed, 12 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > index d50b46a0528f..cbfc936d251c 100644
> > --- a/drivers/firmware/psci/psci.c
> > +++ b/drivers/firmware/psci/psci.c
> > @@ -290,26 +290,20 @@ static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
> >  static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> >                       struct device_node *cpu_node, int cpu)
> >  {
> > -     int i, ret = 0, count = 0;
> > +     int i, ret = 0, num_state_nodes = drv->state_count - 1;
> >       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);
> > -     }
> > -
>
> To be honest, I'd rather not tighten the coupling with the cpuidle
> driver here. For example, I'm not that happy with the PSCI backend
> having to know the driver has a specific WFI state.

If you ask me, the coupling is already there, only that it's hidden by
taking assumptions about the WFI state in the code.

However, I certainly agree with you, that this isn't very nice.

>
> IIUC we could get rid of the explicit counting with something like:
>
>         count = of_parse_phandle_with_args(cpu_node, "cpu-idle-states", NULL);
>
> ... but I'm not sure that the overall change is a simplification.

In my opinion, no it doesn't.

To me, it seems a kind of silly (and in-efficient) to do an OF parsing
that has already been done and given the information we need.

>
> Does this change make it easier to plumb in something in future?

Yes, I need this for additional changes on top.

Note that, patch4 also provides the opportunity to do a similar
cleanup of the initialization code in drivers/soc/qcom/spm.c. I
haven't made that part of this series though.

I guess in the end, we need to accept that part of the psci driver is
really a cpuidle driver. Trying to keep them entirely separate,
doesn't come without complexity/churns.

While working on psci changes in the recent series I have posted, I
was even considering adding a completely new cpuidle driver for psci
(in drivers/cpuidle/*) and instead define a number of psci interface
functions, which that driver could call. That would probably be a
better split, but requires quite some changes.

So, what do you want me to do with this?

>
> Thanks,
> Mark.

Thanks a lot for reviewing!

Kind regards
Uffe

>
> > -     if (!count)
> > -             return -ENODEV;
> > -
> > -     psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
> > +     psci_states = kcalloc(num_state_nodes, sizeof(*psci_states),
> > +                     GFP_KERNEL);
> >       if (!psci_states)
> >               return -ENOMEM;
> >
> > -     for (i = 0; i < count; i++) {
> > +     for (i = 0; i < num_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);
> >
> > @@ -319,6 +313,11 @@ static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> >               pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
> >       }
> >
> > +     if (i != num_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;
> > --
> > 2.17.1
> >

Re: [PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Lorenzo Pieralisi]

On Mon, Mar 04, 2019 at 11:14:18AM +0100, Ulf Hansson wrote:
> On Fri, 1 Mar 2019 at 18:28, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Thu, Feb 28, 2019 at 02:59:17PM +0100, Ulf Hansson wrote:
> > > Instead of iterating through all the state nodes in DT, to find out how
> > > many states that needs to be allocated, let's use the number already known
> > > by the cpuidle driver. In this way we can drop the iteration altogether.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/firmware/psci/psci.c | 25 ++++++++++++-------------
> > >  1 file changed, 12 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > > index d50b46a0528f..cbfc936d251c 100644
> > > --- a/drivers/firmware/psci/psci.c
> > > +++ b/drivers/firmware/psci/psci.c
> > > @@ -290,26 +290,20 @@ static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
> > >  static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> > >                       struct device_node *cpu_node, int cpu)
> > >  {
> > > -     int i, ret = 0, count = 0;
> > > +     int i, ret = 0, num_state_nodes = drv->state_count - 1;
> > >       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);
> > > -     }
> > > -
> >
> > To be honest, I'd rather not tighten the coupling with the cpuidle
> > driver here. For example, I'm not that happy with the PSCI backend
> > having to know the driver has a specific WFI state.
> 
> If you ask me, the coupling is already there, only that it's hidden by
> taking assumptions about the WFI state in the code.

There is no assumption taken - I wrote it down here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/arm/idle-states.txt?h=v5.0


> However, I certainly agree with you, that this isn't very nice.

The idea behind the generic ARM CPU idle driver is as follows:

- A generic front-end in drivers/cpuidle/cpuidle-arm.c
- An arch back-end (that is defined by the enable-method), on ARM64
  it is PSCI

As usual with the ARM CPUidle mess, there must be logic connecting
the front-end and the back-end. An idle state index was used since
I saw no other generic way. If there are better ideas they are welcome.

Otherwise we must go back to having a PSCI specific CPUIdle driver
and, on arch/arm, platform specific CPUidle drivers.

The aim was to simplify but to do that we need a connection logic
between drivers<->arch code, that's the only way we can have a generic
idle driver and corresponding boilerplate.

> > IIUC we could get rid of the explicit counting with something like:
> >
> >         count = of_parse_phandle_with_args(cpu_node, "cpu-idle-states", NULL);
> >
> > ... but I'm not sure that the overall change is a simplification.
> 
> In my opinion, no it doesn't.
> 
> To me, it seems a kind of silly (and in-efficient) to do an OF parsing
> that has already been done and given the information we need.

Yeah. It is boot path with idle states in the order of (max) dozens,
silly and inefficient, maybe but that should be fine.

See above.

> > Does this change make it easier to plumb in something in future?
> 
> Yes, I need this for additional changes on top.
> 
> Note that, patch4 also provides the opportunity to do a similar
> cleanup of the initialization code in drivers/soc/qcom/spm.c. I
> haven't made that part of this series though.
> 
> I guess in the end, we need to accept that part of the psci driver is
> really a cpuidle driver. Trying to keep them entirely separate,
> doesn't come without complexity/churns.

PSCI driver is a kernel interface to firmware, it is not a CPUidle
driver per-se, we tried to decouple firmware interfaces from kernel
data structures as much as possible, again, see above.

> While working on psci changes in the recent series I have posted, I
> was even considering adding a completely new cpuidle driver for psci
> (in drivers/cpuidle/*) and instead define a number of psci interface
> functions, which that driver could call. That would probably be a
> better split, but requires quite some changes.

It can be done but with it the whole generic ARM CPUidle driver
infrastructure must go with it (and you will still have a standard wfi
state in the PSCI idle state array anyway).

The idea behind ARM64 cpu_ops clashes a bit with this approach so
there is a discussion to be had here.

> So, what do you want me to do with this?

We need to answer the question above.

Thanks,
Lorenzo

> > Thanks,
> > Mark.
> 
> Thanks a lot for reviewing!
> 
> Kind regards
> Uffe
> 
> >
> > > -     if (!count)
> > > -             return -ENODEV;
> > > -
> > > -     psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
> > > +     psci_states = kcalloc(num_state_nodes, sizeof(*psci_states),
> > > +                     GFP_KERNEL);
> > >       if (!psci_states)
> > >               return -ENOMEM;
> > >
> > > -     for (i = 0; i < count; i++) {
> > > +     for (i = 0; i < num_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);
> > >
> > > @@ -319,6 +313,11 @@ static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> > >               pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
> > >       }
> > >
> > > +     if (i != num_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;
> > > --
> > > 2.17.1
> > >

Re: [PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Ulf Hansson]

Lorenzo, Mark,

On Wed, 6 Mar 2019 at 19:15, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Mon, Mar 04, 2019 at 11:14:18AM +0100, Ulf Hansson wrote:
> > On Fri, 1 Mar 2019 at 18:28, Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > On Thu, Feb 28, 2019 at 02:59:17PM +0100, Ulf Hansson wrote:
> > > > Instead of iterating through all the state nodes in DT, to find out how
> > > > many states that needs to be allocated, let's use the number already known
> > > > by the cpuidle driver. In this way we can drop the iteration altogether.
> > > >
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >  drivers/firmware/psci/psci.c | 25 ++++++++++++-------------
> > > >  1 file changed, 12 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > > > index d50b46a0528f..cbfc936d251c 100644
> > > > --- a/drivers/firmware/psci/psci.c
> > > > +++ b/drivers/firmware/psci/psci.c
> > > > @@ -290,26 +290,20 @@ static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
> > > >  static int psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> > > >                       struct device_node *cpu_node, int cpu)
> > > >  {
> > > > -     int i, ret = 0, count = 0;
> > > > +     int i, ret = 0, num_state_nodes = drv->state_count - 1;
> > > >       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);
> > > > -     }
> > > > -
> > >
> > > To be honest, I'd rather not tighten the coupling with the cpuidle
> > > driver here. For example, I'm not that happy with the PSCI backend
> > > having to know the driver has a specific WFI state.
> >
> > If you ask me, the coupling is already there, only that it's hidden by
> > taking assumptions about the WFI state in the code.
>
> There is no assumption taken - I wrote it down here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/arm/idle-states.txt?h=v5.0
>
>
> > However, I certainly agree with you, that this isn't very nice.
>
> The idea behind the generic ARM CPU idle driver is as follows:
>
> - A generic front-end in drivers/cpuidle/cpuidle-arm.c
> - An arch back-end (that is defined by the enable-method), on ARM64
>   it is PSCI
>
> As usual with the ARM CPUidle mess, there must be logic connecting
> the front-end and the back-end. An idle state index was used since
> I saw no other generic way. If there are better ideas they are welcome.
>
> Otherwise we must go back to having a PSCI specific CPUIdle driver
> and, on arch/arm, platform specific CPUidle drivers.

To be clear, I am not proposing to change into this. But, since Mark
pointed out his concerns about the specifics around the WFI state, I
just wanted to share what has been on my mind in regards to this as
well.

There are positive/negative consequences with any design, it's not
more than that. If we want to re-design all this, there should be good
reasons and benefits for doing it. Maybe there is, I can't tell at
this point, without further exploring it.

More importantly, so far, I have been able fit my changes to the PSCI
code into the existing model - and with quite limited churns I think.
However, I do need the handle to the struct cpuidle_driver *, that we
use during initialization as patch4 and $subject patch suggests, for
future steps.

>
> The aim was to simplify but to do that we need a connection logic
> between drivers<->arch code, that's the only way we can have a generic
> idle driver and corresponding boilerplate.
>
> > > IIUC we could get rid of the explicit counting with something like:
> > >
> > >         count = of_parse_phandle_with_args(cpu_node, "cpu-idle-states", NULL);
> > >
> > > ... but I'm not sure that the overall change is a simplification.
> >
> > In my opinion, no it doesn't.
> >
> > To me, it seems a kind of silly (and in-efficient) to do an OF parsing
> > that has already been done and given the information we need.
>
> Yeah. It is boot path with idle states in the order of (max) dozens,
> silly and inefficient, maybe but that should be fine.

Yes, it certainly works as is today.

>
> See above.
>
> > > Does this change make it easier to plumb in something in future?
> >
> > Yes, I need this for additional changes on top.
> >
> > Note that, patch4 also provides the opportunity to do a similar
> > cleanup of the initialization code in drivers/soc/qcom/spm.c. I
> > haven't made that part of this series though.
> >
> > I guess in the end, we need to accept that part of the psci driver is
> > really a cpuidle driver. Trying to keep them entirely separate,
> > doesn't come without complexity/churns.
>
> PSCI driver is a kernel interface to firmware, it is not a CPUidle
> driver per-se, we tried to decouple firmware interfaces from kernel
> data structures as much as possible, again, see above.

I fully agree the PSCI firmware driver/code is not a CPUidle driver,
just wanted point out that *part* of that code, is in immediate
connection with CPUidle.

>
> > While working on psci changes in the recent series I have posted, I
> > was even considering adding a completely new cpuidle driver for psci
> > (in drivers/cpuidle/*) and instead define a number of psci interface
> > functions, which that driver could call. That would probably be a
> > better split, but requires quite some changes.
>
> It can be done but with it the whole generic ARM CPUidle driver
> infrastructure must go with it (and you will still have a standard wfi
> state in the PSCI idle state array anyway).
>
> The idea behind ARM64 cpu_ops clashes a bit with this approach so
> there is a discussion to be had here.

I am open to discuss whatever suggestion there is on the table. But is
there any? :-)

>
> > So, what do you want me to do with this?
>
> We need to answer the question above.

So, at this point, I am not suggesting to re-design the cpuidle-arm
driver into a psci cpuidle driver.

Instead, my suggestion is according to what I propose in patch 4 and
$subject patch, which means minor adjustments to be able to pass the
struct cpuidle_driver * to the init functions. This, I need it for
next steps, but already at this point it improves things as it avoids
some of the OF parsing, and that's good, isn't it?

>
> Thanks,
> Lorenzo
>

Thanks for you comments!

Kind regards
Uffe

Re: [PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Lorenzo Pieralisi]

On Fri, Mar 08, 2019 at 11:36:49AM +0100, Ulf Hansson wrote:

[...]

> Instead, my suggestion is according to what I propose in patch 4 and
> $subject patch, which means minor adjustments to be able to pass the
> struct cpuidle_driver * to the init functions. This, I need it for
> next steps, but already at this point it improves things as it avoids
> some of the OF parsing, and that's good, isn't it?

I will take the patches Mark ACKed and send them for v5.2 as
early as it gets in v5.1-rc* cycle.

For this one maybe you can post the changes on top and see what's
the best way forward ?

I agree that duplicating idle state parsing code across back-ends
is silly - we just want to keep PSCI and kernel data structure
decoupled.

Post the code on top and we will find a way forward, OK ?

Thanks,
Lorenzo

Re: [PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Ulf Hansson]

On Fri, 8 Mar 2019 at 12:49, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Fri, Mar 08, 2019 at 11:36:49AM +0100, Ulf Hansson wrote:
>
> [...]
>
> > Instead, my suggestion is according to what I propose in patch 4 and
> > $subject patch, which means minor adjustments to be able to pass the
> > struct cpuidle_driver * to the init functions. This, I need it for
> > next steps, but already at this point it improves things as it avoids
> > some of the OF parsing, and that's good, isn't it?
>
> I will take the patches Mark ACKed and send them for v5.2 as
> early as it gets in v5.1-rc* cycle.

Actually, may I suggest we funnel these through Rafael's tree, unless
you are expecting other PSCI changes for v.5.2, which could cause
conflicts?

The reason is, other PM core changes, to genpd for example, needs to
go via Rafael's tree. Those would then potentially block us for
applying any other changes to your tree (arm-soc?) for PSCI (as there
is dependency) until v5.3.

How about if you provides your explicit acks for those PSCI changes
your are happy with, then Rafael can pick them?

>
> For this one maybe you can post the changes on top and see what's
> the best way forward ?
>
> I agree that duplicating idle state parsing code across back-ends
> is silly - we just want to keep PSCI and kernel data structure
> decoupled.

Right. Let's continue this discussion later on and move forward here.
Make sense.

>
> Post the code on top and we will find a way forward, OK ?

Sure, let me do that.

Thanks and kind regards
Uffe

Re: [PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Lorenzo Pieralisi]

On Fri, Mar 08, 2019 at 02:07:51PM +0100, Ulf Hansson wrote:
> On Fri, 8 Mar 2019 at 12:49, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Fri, Mar 08, 2019 at 11:36:49AM +0100, Ulf Hansson wrote:
> >
> > [...]
> >
> > > Instead, my suggestion is according to what I propose in patch 4 and
> > > $subject patch, which means minor adjustments to be able to pass the
> > > struct cpuidle_driver * to the init functions. This, I need it for
> > > next steps, but already at this point it improves things as it avoids
> > > some of the OF parsing, and that's good, isn't it?
> >
> > I will take the patches Mark ACKed and send them for v5.2 as
> > early as it gets in v5.1-rc* cycle.
> 
> Actually, may I suggest we funnel these through Rafael's tree, unless
> you are expecting other PSCI changes for v.5.2, which could cause
> conflicts?
> 
> The reason is, other PM core changes, to genpd for example, needs to
> go via Rafael's tree. Those would then potentially block us for
> applying any other changes to your tree (arm-soc?) for PSCI (as there
> is dependency) until v5.3.
> 
> How about if you provides your explicit acks for those PSCI changes
> your are happy with, then Rafael can pick them?

It is fine we can do that, I would have not sent the patches Mark
has ACKed to arm-soc till -{rc2/rc3} anyway.

Thanks,
Lorenzo

> > For this one maybe you can post the changes on top and see what's
> > the best way forward ?
> >
> > I agree that duplicating idle state parsing code across back-ends
> > is silly - we just want to keep PSCI and kernel data structure
> > decoupled.
> 
> Right. Let's continue this discussion later on and move forward here.
> Make sense.
> 
> >
> > Post the code on top and we will find a way forward, OK ?
> 
> Sure, let me do that.
> 
> Thanks and kind regards
> Uffe

Re: [PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Ulf Hansson]

On Fri, 8 Mar 2019 at 14:17, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Fri, Mar 08, 2019 at 02:07:51PM +0100, Ulf Hansson wrote:
> > On Fri, 8 Mar 2019 at 12:49, Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> > >
> > > On Fri, Mar 08, 2019 at 11:36:49AM +0100, Ulf Hansson wrote:
> > >
> > > [...]
> > >
> > > > Instead, my suggestion is according to what I propose in patch 4 and
> > > > $subject patch, which means minor adjustments to be able to pass the
> > > > struct cpuidle_driver * to the init functions. This, I need it for
> > > > next steps, but already at this point it improves things as it avoids
> > > > some of the OF parsing, and that's good, isn't it?
> > >
> > > I will take the patches Mark ACKed and send them for v5.2 as
> > > early as it gets in v5.1-rc* cycle.
> >
> > Actually, may I suggest we funnel these through Rafael's tree, unless
> > you are expecting other PSCI changes for v.5.2, which could cause
> > conflicts?
> >
> > The reason is, other PM core changes, to genpd for example, needs to
> > go via Rafael's tree. Those would then potentially block us for
> > applying any other changes to your tree (arm-soc?) for PSCI (as there
> > is dependency) until v5.3.
> >
> > How about if you provides your explicit acks for those PSCI changes
> > your are happy with, then Rafael can pick them?
>
> It is fine we can do that, I would have not sent the patches Mark
> has ACKed to arm-soc till -{rc2/rc3} anyway.

Great!

May I suggest you just reply to the cover-letter and provide the acks
to the relevant patches, then I can then collect the received acks
tags and re-post them to Rafael once rc1 is out.

Kind regards
Uffe

>
> Thanks,
> Lorenzo
>
> > > For this one maybe you can post the changes on top and see what's
> > > the best way forward ?
> > >
> > > I agree that duplicating idle state parsing code across back-ends
> > > is silly - we just want to keep PSCI and kernel data structure
> > > decoupled.
> >
> > Right. Let's continue this discussion later on and move forward here.
> > Make sense.
> >
> > >
> > > Post the code on top and we will find a way forward, OK ?
> >
> > Sure, let me do that.
> >
> > Thanks and kind regards
> > Uffe

Re: [PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Lorenzo Pieralisi]

On Fri, Mar 08, 2019 at 02:23:26PM +0100, Ulf Hansson wrote:
> On Fri, 8 Mar 2019 at 14:17, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Fri, Mar 08, 2019 at 02:07:51PM +0100, Ulf Hansson wrote:
> > > On Fri, 8 Mar 2019 at 12:49, Lorenzo Pieralisi
> > > <lorenzo.pieralisi@arm.com> wrote:
> > > >
> > > > On Fri, Mar 08, 2019 at 11:36:49AM +0100, Ulf Hansson wrote:
> > > >
> > > > [...]
> > > >
> > > > > Instead, my suggestion is according to what I propose in patch 4 and
> > > > > $subject patch, which means minor adjustments to be able to pass the
> > > > > struct cpuidle_driver * to the init functions. This, I need it for
> > > > > next steps, but already at this point it improves things as it avoids
> > > > > some of the OF parsing, and that's good, isn't it?
> > > >
> > > > I will take the patches Mark ACKed and send them for v5.2 as
> > > > early as it gets in v5.1-rc* cycle.
> > >
> > > Actually, may I suggest we funnel these through Rafael's tree, unless
> > > you are expecting other PSCI changes for v.5.2, which could cause
> > > conflicts?
> > >
> > > The reason is, other PM core changes, to genpd for example, needs to
> > > go via Rafael's tree. Those would then potentially block us for
> > > applying any other changes to your tree (arm-soc?) for PSCI (as there
> > > is dependency) until v5.3.
> > >
> > > How about if you provides your explicit acks for those PSCI changes
> > > your are happy with, then Rafael can pick them?
> >
> > It is fine we can do that, I would have not sent the patches Mark
> > has ACKed to arm-soc till -{rc2/rc3} anyway.
> 
> Great!
> 
> May I suggest you just reply to the cover-letter and provide the acks
> to the relevant patches, then I can then collect the received acks
> tags and re-post them to Rafael once rc1 is out.

Mark ACKed the patches that we consider ready for upstream, tag them
and send them out at -rc1 there is nothing left to do on those.

Lorenzo

Re: [PATCH 5/7] drivers: firmware: psci: Simplify state node parsing

[Ulf Hansson]

On Fri, 8 Mar 2019 at 14:31, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Fri, Mar 08, 2019 at 02:23:26PM +0100, Ulf Hansson wrote:
> > On Fri, 8 Mar 2019 at 14:17, Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> > >
> > > On Fri, Mar 08, 2019 at 02:07:51PM +0100, Ulf Hansson wrote:
> > > > On Fri, 8 Mar 2019 at 12:49, Lorenzo Pieralisi
> > > > <lorenzo.pieralisi@arm.com> wrote:
> > > > >
> > > > > On Fri, Mar 08, 2019 at 11:36:49AM +0100, Ulf Hansson wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > > Instead, my suggestion is according to what I propose in patch 4 and
> > > > > > $subject patch, which means minor adjustments to be able to pass the
> > > > > > struct cpuidle_driver * to the init functions. This, I need it for
> > > > > > next steps, but already at this point it improves things as it avoids
> > > > > > some of the OF parsing, and that's good, isn't it?
> > > > >
> > > > > I will take the patches Mark ACKed and send them for v5.2 as
> > > > > early as it gets in v5.1-rc* cycle.
> > > >
> > > > Actually, may I suggest we funnel these through Rafael's tree, unless
> > > > you are expecting other PSCI changes for v.5.2, which could cause
> > > > conflicts?
> > > >
> > > > The reason is, other PM core changes, to genpd for example, needs to
> > > > go via Rafael's tree. Those would then potentially block us for
> > > > applying any other changes to your tree (arm-soc?) for PSCI (as there
> > > > is dependency) until v5.3.
> > > >
> > > > How about if you provides your explicit acks for those PSCI changes
> > > > your are happy with, then Rafael can pick them?
> > >
> > > It is fine we can do that, I would have not sent the patches Mark
> > > has ACKed to arm-soc till -{rc2/rc3} anyway.
> >
> > Great!
> >
> > May I suggest you just reply to the cover-letter and provide the acks
> > to the relevant patches, then I can then collect the received acks
> > tags and re-post them to Rafael once rc1 is out.
>
> Mark ACKed the patches that we consider ready for upstream, tag them
> and send them out at -rc1 there is nothing left to do on those.

Right, I add your acks to them as well then.

Thanks!

Kind regards
Uffe

[PATCH 6/7] drivers: firmware: psci: Simplify error path of psci_dt_init()

[Ulf Hansson]

Instead of having each psci init function taking care of the of_node_put(),
let's deal with that from psci_dt_init(), as this enables a bit simpler
error path for each psci init function.

Cc: Lina Iyer <ilina@codeaurora.org>
Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/psci/psci.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index cbfc936d251c..14d25b54b9f2 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -609,9 +609,9 @@ static int __init psci_0_2_init(struct device_node *np)
 	int err;
 
 	err = get_set_conduit_method(np);
-
 	if (err)
-		goto out_put_node;
+		return err;
+
 	/*
 	 * Starting with v0.2, the PSCI specification introduced a call
 	 * (PSCI_VERSION) that allows probing the firmware version, so
@@ -619,11 +619,7 @@ static int __init psci_0_2_init(struct device_node *np)
 	 * can be carried out according to the specific version reported
 	 * by firmware
 	 */
-	err = psci_probe();
-
-out_put_node:
-	of_node_put(np);
-	return err;
+	return psci_probe();
 }
 
 /*
@@ -635,9 +631,8 @@ static int __init psci_0_1_init(struct device_node *np)
 	int err;
 
 	err = get_set_conduit_method(np);
-
 	if (err)
-		goto out_put_node;
+		return err;
 
 	pr_info("Using PSCI v0.1 Function IDs from DT\n");
 
@@ -661,9 +656,7 @@ static int __init psci_0_1_init(struct device_node *np)
 		psci_ops.migrate = psci_migrate;
 	}
 
-out_put_node:
-	of_node_put(np);
-	return err;
+	return 0;
 }
 
 static const struct of_device_id psci_of_match[] __initconst = {
@@ -678,6 +671,7 @@ int __init psci_dt_init(void)
 	struct device_node *np;
 	const struct of_device_id *matched_np;
 	psci_initcall_t init_fn;
+	int ret;
 
 	np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
 
@@ -685,7 +679,10 @@ int __init psci_dt_init(void)
 		return -ENODEV;
 
 	init_fn = (psci_initcall_t)matched_np->data;
-	return init_fn(np);
+	ret = init_fn(np);
+
+	of_node_put(np);
+	return ret;
 }
 
 #ifdef CONFIG_ACPI
-- 
2.17.1

[PATCH 7/7] drivers: firmware: psci: Announce support for OS initiated suspend mode

[Ulf Hansson]

PSCI firmware v1.0+, supports two different modes for CPU_SUSPEND. The
Platform Coordinated mode, which is the default and mandatory mode, while
support for the OS initiated (OSI) mode is optional.

In some cases it's interesting for the user/developer to know if the OSI
mode is supported by the PSCI FW. Therefore, let's print a message to the
log, if that is the case.

Cc: Lina Iyer <ilina@codeaurora.org>
Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/firmware/psci/psci.c | 21 ++++++++++++++++++++-
 include/uapi/linux/psci.h    |  5 +++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index 14d25b54b9f2..417d43886948 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -95,6 +95,11 @@ static inline bool psci_has_ext_power_state(void)
 				PSCI_1_0_FEATURES_CPU_SUSPEND_PF_MASK;
 }
 
+static inline bool psci_has_osi_support(void)
+{
+	return psci_cpu_suspend_feature & PSCI_1_0_OS_INITIATED;
+}
+
 static inline bool psci_power_state_loses_context(u32 state)
 {
 	const u32 mask = psci_has_ext_power_state() ?
@@ -659,10 +664,24 @@ static int __init psci_0_1_init(struct device_node *np)
 	return 0;
 }
 
+static int __init psci_1_0_init(struct device_node *np)
+{
+	int err;
+
+	err = psci_0_2_init(np);
+	if (err)
+		return err;
+
+	if (psci_has_osi_support())
+		pr_info("OSI mode supported.\n");
+
+	return 0;
+}
+
 static const struct of_device_id psci_of_match[] __initconst = {
 	{ .compatible = "arm,psci",	.data = psci_0_1_init},
 	{ .compatible = "arm,psci-0.2",	.data = psci_0_2_init},
-	{ .compatible = "arm,psci-1.0",	.data = psci_0_2_init},
+	{ .compatible = "arm,psci-1.0",	.data = psci_1_0_init},
 	{},
 };
 
diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
index b3bcabe380da..581f72085c33 100644
--- a/include/uapi/linux/psci.h
+++ b/include/uapi/linux/psci.h
@@ -49,6 +49,7 @@
 
 #define PSCI_1_0_FN_PSCI_FEATURES		PSCI_0_2_FN(10)
 #define PSCI_1_0_FN_SYSTEM_SUSPEND		PSCI_0_2_FN(14)
+#define PSCI_1_0_FN_SET_SUSPEND_MODE		PSCI_0_2_FN(15)
 
 #define PSCI_1_0_FN64_SYSTEM_SUSPEND		PSCI_0_2_FN64(14)
 
@@ -97,6 +98,10 @@
 #define PSCI_1_0_FEATURES_CPU_SUSPEND_PF_MASK	\
 			(0x1 << PSCI_1_0_FEATURES_CPU_SUSPEND_PF_SHIFT)
 
+#define PSCI_1_0_OS_INITIATED			BIT(0)
+#define PSCI_1_0_SUSPEND_MODE_PC		0
+#define PSCI_1_0_SUSPEND_MODE_OSI		1
+
 /* PSCI return values (inclusive of all PSCI versions) */
 #define PSCI_RET_SUCCESS			0
 #define PSCI_RET_NOT_SUPPORTED			-1
-- 
2.17.1

Re: [PATCH 7/7] drivers: firmware: psci: Announce support for OS initiated suspend mode

[Stephen Boyd]

Quoting Ulf Hansson (2019-02-28 05:59:19)
> PSCI firmware v1.0+, supports two different modes for CPU_SUSPEND. The
> Platform Coordinated mode, which is the default and mandatory mode, while
> support for the OS initiated (OSI) mode is optional.
> 
> In some cases it's interesting for the user/developer to know if the OSI
> mode is supported by the PSCI FW. Therefore, let's print a message to the
> log, if that is the case.

I don't see anything wrong with the patch besides the potential for the
logs to wrap and the informational message to be lost, but would it be
possible to express the PSCI features that are supported in sysfs at
/sys/firmware/psci/ somehow? It may be useful for a user to know that
their firmware has PSCI support and that it has a certain set of
features available, like OSI vs. PC, or both. I don't know of any use
that userspace would have though, besides maybe the CPU state residency
counters being used by some sort of statistics collecting program or
just plain knowing that certain PSCI features are present, so perhaps it
could just be done in debugfs for now until a real user exists.

Re: [PATCH 7/7] drivers: firmware: psci: Announce support for OS initiated suspend mode

[Ulf Hansson]

On Fri, 1 Mar 2019 at 18:28, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Ulf Hansson (2019-02-28 05:59:19)
> > PSCI firmware v1.0+, supports two different modes for CPU_SUSPEND. The
> > Platform Coordinated mode, which is the default and mandatory mode, while
> > support for the OS initiated (OSI) mode is optional.
> >
> > In some cases it's interesting for the user/developer to know if the OSI
> > mode is supported by the PSCI FW. Therefore, let's print a message to the
> > log, if that is the case.
>
> I don't see anything wrong with the patch besides the potential for the
> logs to wrap and the informational message to be lost, but would it be
> possible to express the PSCI features that are supported in sysfs at
> /sys/firmware/psci/ somehow? It may be useful for a user to know that
> their firmware has PSCI support and that it has a certain set of
> features available, like OSI vs. PC, or both. I don't know of any use
> that userspace would have though, besides maybe the CPU state residency
> counters being used by some sort of statistics collecting program or
> just plain knowing that certain PSCI features are present, so perhaps it
> could just be done in debugfs for now until a real user exists.

Having a sysfs interface for PSCI, allowing users to know a little bit
more about what the FW supports/version/etc sound like a great idea to
me.

There is already quite some prints being made by psci during boot, so
perhaps having sysfs allows us to drop some of these.

Mark, what do you think? Is it something you think we should do? I can
offer my help to implement this, if you think it's a good idea.

Stephen, thanks for reviewing!

Kind regards
Uffe

Re: [PATCH 7/7] drivers: firmware: psci: Announce support for OS initiated suspend mode

[Mark Rutland]

On Thu, Feb 28, 2019 at 02:59:19PM +0100, Ulf Hansson wrote:
> PSCI firmware v1.0+, supports two different modes for CPU_SUSPEND. The
> Platform Coordinated mode, which is the default and mandatory mode, while
> support for the OS initiated (OSI) mode is optional.
> 
> In some cases it's interesting for the user/developer to know if the OSI
> mode is supported by the PSCI FW. Therefore, let's print a message to the
> log, if that is the case.
> 
> Cc: Lina Iyer <ilina@codeaurora.org>
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  drivers/firmware/psci/psci.c | 21 ++++++++++++++++++++-
>  include/uapi/linux/psci.h    |  5 +++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index 14d25b54b9f2..417d43886948 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -95,6 +95,11 @@ static inline bool psci_has_ext_power_state(void)
>  				PSCI_1_0_FEATURES_CPU_SUSPEND_PF_MASK;
>  }
>  
> +static inline bool psci_has_osi_support(void)
> +{
> +	return psci_cpu_suspend_feature & PSCI_1_0_OS_INITIATED;
> +}
> +
>  static inline bool psci_power_state_loses_context(u32 state)
>  {
>  	const u32 mask = psci_has_ext_power_state() ?
> @@ -659,10 +664,24 @@ static int __init psci_0_1_init(struct device_node *np)
>  	return 0;
>  }
>  
> +static int __init psci_1_0_init(struct device_node *np)
> +{
> +	int err;
> +
> +	err = psci_0_2_init(np);
> +	if (err)
> +		return err;
> +
> +	if (psci_has_osi_support())
> +		pr_info("OSI mode supported.\n");
> +
> +	return 0;
> +}
> +
>  static const struct of_device_id psci_of_match[] __initconst = {
>  	{ .compatible = "arm,psci",	.data = psci_0_1_init},
>  	{ .compatible = "arm,psci-0.2",	.data = psci_0_2_init},
> -	{ .compatible = "arm,psci-1.0",	.data = psci_0_2_init},
> +	{ .compatible = "arm,psci-1.0",	.data = psci_1_0_init},
>  	{},
>  };
>  
> diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
> index b3bcabe380da..581f72085c33 100644
> --- a/include/uapi/linux/psci.h
> +++ b/include/uapi/linux/psci.h
> @@ -49,6 +49,7 @@
>  
>  #define PSCI_1_0_FN_PSCI_FEATURES		PSCI_0_2_FN(10)
>  #define PSCI_1_0_FN_SYSTEM_SUSPEND		PSCI_0_2_FN(14)
> +#define PSCI_1_0_FN_SET_SUSPEND_MODE		PSCI_0_2_FN(15)
>  
>  #define PSCI_1_0_FN64_SYSTEM_SUSPEND		PSCI_0_2_FN64(14)
>  
> @@ -97,6 +98,10 @@
>  #define PSCI_1_0_FEATURES_CPU_SUSPEND_PF_MASK	\
>  			(0x1 << PSCI_1_0_FEATURES_CPU_SUSPEND_PF_SHIFT)
>  
> +#define PSCI_1_0_OS_INITIATED			BIT(0)
> +#define PSCI_1_0_SUSPEND_MODE_PC		0
> +#define PSCI_1_0_SUSPEND_MODE_OSI		1
> +
>  /* PSCI return values (inclusive of all PSCI versions) */
>  #define PSCI_RET_SUCCESS			0
>  #define PSCI_RET_NOT_SUPPORTED			-1
> -- 
> 2.17.1
>