linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework
@ 2019-07-22 15:37 Lorenzo Pieralisi
  2019-07-22 15:37 ` [PATCH 1/6] ARM: cpuidle: Remove useless header include Lorenzo Pieralisi
                   ` (7 more replies)
  0 siblings, 8 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-07-22 15:37 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Will Deacon, Ulf Hansson, Sudeep Holla,
	Daniel Lezcano, Catalin Marinas, Mark Rutland, Rafael J. Wysocki,
	LKML, LAKML

Current PSCI CPUidle driver is built on top of the generic ARM
CPUidle infrastructure that relies on the architectural back-end
idle operations to initialize and enter idle states.

On ARM64 systems, PSCI is the only interface the kernel ever uses
to enter idle states, so, having to rely on a generic ARM CPUidle
driver when there is and there will always be only one method
for entering idle states proved to be overkill, more so given
that on ARM 32-bit systems (that can also enable the generic
ARM CPUidle driver) only one additional idle back-end was
ever added:

drivers/soc/qcom/spm.c

and it can be easily converted to a full-fledged CPUidle driver
without requiring the generic ARM CPUidle framework.

Furthermore, the generic ARM CPUidle infrastructure forces the
PSCI firmware layer to keep CPUidle specific information in it,
which does not really fit its purpose that should be kernel
control/data structure agnostic.

Lastly, the interface between the generic ARM CPUidle driver and
the arch back-end requires an idle state index to be passed to
suspend operations, with idle states back-end internals (such
as idle state parameters) hidden in architectural back-ends and
not available to the generic ARM CPUidle driver.

To improve the above mentioned shortcomings, implement a stand
alone PSCI CPUidle driver; this improves the current kernel
code from several perspective:

- Move CPUidle internal knowledge into CPUidle driver out of
  the PSCI firmware interface
- Give the PSCI CPUidle driver control over power state parameters,
  in particular in preparation for PSCI OSI support
- Remove generic CPUidle operations infrastructure from the kernel

This patchset does not go as far as removing the generic ARM CPUidle
infrastructure in order to collect feedback on the new approach
before completing the removal from the kernel, the generic and PSCI
CPUidle driver are left to co-exist.

Tested on Juno platform with both DT and ACPI boot firmwares.

Cc: Will Deacon <will@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

Lorenzo Pieralisi (6):
  ARM: cpuidle: Remove useless header include
  ARM: cpuidle: Remove overzealous error logging
  drivers: firmware: psci: Decouple checker from generic ARM CPUidle
  ARM: psci: cpuidle: Introduce PSCI CPUidle driver
  ARM: psci: cpuidle: Enable PSCI CPUidle driver
  PSCI: cpuidle: Refactor CPU suspend power_state parameter handling

 MAINTAINERS                          |   8 +
 arch/arm/configs/imx_v6_v7_defconfig |   1 +
 arch/arm64/configs/defconfig         |   1 +
 arch/arm64/kernel/cpuidle.c          |  50 +++++-
 arch/arm64/kernel/psci.c             |   4 -
 drivers/cpuidle/Kconfig.arm          |   7 +
 drivers/cpuidle/Makefile             |   1 +
 drivers/cpuidle/cpuidle-arm.c        |  13 +-
 drivers/cpuidle/cpuidle-psci.c       | 235 +++++++++++++++++++++++++++
 drivers/firmware/psci/psci.c         | 167 +------------------
 drivers/firmware/psci/psci_checker.c |  16 +-
 include/linux/cpuidle.h              |  17 +-
 include/linux/psci.h                 |   4 +-
 13 files changed, 338 insertions(+), 186 deletions(-)
 create mode 100644 drivers/cpuidle/cpuidle-psci.c

-- 
2.21.0


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH 1/6] ARM: cpuidle: Remove useless header include
  2019-07-22 15:37 [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework Lorenzo Pieralisi
@ 2019-07-22 15:37 ` Lorenzo Pieralisi
  2019-08-06 15:51   ` Sudeep Holla
  2019-08-07  8:19   ` Daniel Lezcano
  2019-07-22 15:37 ` [PATCH 2/6] ARM: cpuidle: Remove overzealous error logging Lorenzo Pieralisi
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-07-22 15:37 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Ulf Hansson, Sudeep Holla, Daniel Lezcano,
	Rafael J. Wysocki, Catalin Marinas, Will Deacon, Mark Rutland,
	LKML, LAKML

The generic ARM CPUidle driver includes <linux/topology.h> by mistake.

Remove the topology header include.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 drivers/cpuidle/cpuidle-arm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 5bcd82c35dcf..dc33b3d2954f 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -15,7 +15,6 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/slab.h>
-#include <linux/topology.h>
 
 #include <asm/cpuidle.h>
 
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 2/6] ARM: cpuidle: Remove overzealous error logging
  2019-07-22 15:37 [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework Lorenzo Pieralisi
  2019-07-22 15:37 ` [PATCH 1/6] ARM: cpuidle: Remove useless header include Lorenzo Pieralisi
@ 2019-07-22 15:37 ` Lorenzo Pieralisi
  2019-08-06 15:51   ` Sudeep Holla
  2019-08-07  8:20   ` Daniel Lezcano
  2019-07-22 15:37 ` [PATCH 3/6] drivers: firmware: psci: Decouple checker from generic ARM CPUidle Lorenzo Pieralisi
                   ` (5 subsequent siblings)
  7 siblings, 2 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-07-22 15:37 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Ulf Hansson, Sudeep Holla, Daniel Lezcano,
	Rafael J. Wysocki, Catalin Marinas, Will Deacon, Mark Rutland,
	LKML, LAKML

CPUidle back-end operations are not implemented in some platforms
but this should not be considered an error serious enough to be
logged. Check the arm_cpuidle_init() return value to detect whether
the failure must be reported or not in the kernel log and do
not log it if the platform does not support CPUidle operations.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 drivers/cpuidle/cpuidle-arm.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index dc33b3d2954f..9e5156d39627 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -105,11 +105,17 @@ static int __init arm_idle_init_cpu(int cpu)
 	ret = arm_cpuidle_init(cpu);
 
 	/*
-	 * Allow the initialization to continue for other CPUs, if the reported
-	 * failure is a HW misconfiguration/breakage (-ENXIO).
+	 * Allow the initialization to continue for other CPUs, if the
+	 * reported failure is a HW misconfiguration/breakage (-ENXIO).
+	 *
+	 * Some platforms do not support idle operations
+	 * (arm_cpuidle_init() returning -EOPNOTSUPP), we should
+	 * not flag this case as an error, it is a valid
+	 * configuration.
 	 */
 	if (ret) {
-		pr_err("CPU %d failed to init idle CPU ops\n", cpu);
+		if (ret != -EOPNOTSUPP)
+			pr_err("CPU %d failed to init idle CPU ops\n", cpu);
 		ret = ret == -ENXIO ? 0 : ret;
 		goto out_kfree_drv;
 	}
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 3/6] drivers: firmware: psci: Decouple checker from generic ARM CPUidle
  2019-07-22 15:37 [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework Lorenzo Pieralisi
  2019-07-22 15:37 ` [PATCH 1/6] ARM: cpuidle: Remove useless header include Lorenzo Pieralisi
  2019-07-22 15:37 ` [PATCH 2/6] ARM: cpuidle: Remove overzealous error logging Lorenzo Pieralisi
@ 2019-07-22 15:37 ` Lorenzo Pieralisi
  2019-08-06 15:54   ` Sudeep Holla
  2019-08-07 14:09   ` Daniel Lezcano
  2019-07-22 15:37 ` [PATCH 4/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver Lorenzo Pieralisi
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-07-22 15:37 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Sudeep Holla, Mark Rutland, Catalin Marinas,
	Will Deacon, Ulf Hansson, Daniel Lezcano, Rafael J. Wysocki,
	LKML, LAKML

The PSCI checker currently relies on the generic ARM CPUidle
infrastructure to enter an idle state, which in turn creates
a dependency that is not really needed.

The PSCI checker code to test PSCI CPU suspend is built on
top of the CPUidle framework and can easily reuse the
struct cpuidle_state.enter() function (previously initialized
by an idle driver, with a PSCI back-end) to trigger an entry
into an idle state, decoupling the PSCI checker from the
generic ARM CPUidle infrastructure and simplyfing the code
in the process.

Convert the PSCI checker suspend entry function to use
the struct cpuidle_state.enter() function callback.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 drivers/firmware/psci/psci_checker.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/psci/psci_checker.c b/drivers/firmware/psci/psci_checker.c
index f3659443f8c2..6a445397771c 100644
--- a/drivers/firmware/psci/psci_checker.c
+++ b/drivers/firmware/psci/psci_checker.c
@@ -228,8 +228,11 @@ static int hotplug_tests(void)
 
 static void dummy_callback(struct timer_list *unused) {}
 
-static int suspend_cpu(int index, bool broadcast)
+static int suspend_cpu(struct cpuidle_device *dev,
+		       struct cpuidle_driver *drv, int index)
 {
+	struct cpuidle_state *state = &drv->states[index];
+	bool broadcast = state->flags & CPUIDLE_FLAG_TIMER_STOP;
 	int ret;
 
 	arch_cpu_idle_enter();
@@ -254,11 +257,7 @@ static int suspend_cpu(int index, bool broadcast)
 		}
 	}
 
-	/*
-	 * Replicate the common ARM cpuidle enter function
-	 * (arm_enter_idle_state).
-	 */
-	ret = CPU_PM_CPU_IDLE_ENTER(arm_cpuidle_suspend, index);
+	ret = state->enter(dev, drv, index);
 
 	if (broadcast)
 		tick_broadcast_exit();
@@ -301,9 +300,8 @@ static int suspend_test_thread(void *arg)
 		 * doesn't use PSCI).
 		 */
 		for (index = 1; index < drv->state_count; ++index) {
-			struct cpuidle_state *state = &drv->states[index];
-			bool broadcast = state->flags & CPUIDLE_FLAG_TIMER_STOP;
 			int ret;
+			struct cpuidle_state *state = &drv->states[index];
 
 			/*
 			 * Set the timer to wake this CPU up in some time (which
@@ -318,7 +316,7 @@ static int suspend_test_thread(void *arg)
 			/* IRQs must be disabled during suspend operations. */
 			local_irq_disable();
 
-			ret = suspend_cpu(index, broadcast);
+			ret = suspend_cpu(dev, drv, index);
 
 			/*
 			 * We have woken up. Re-enable IRQs to handle any
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 4/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver
  2019-07-22 15:37 [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework Lorenzo Pieralisi
                   ` (2 preceding siblings ...)
  2019-07-22 15:37 ` [PATCH 3/6] drivers: firmware: psci: Decouple checker from generic ARM CPUidle Lorenzo Pieralisi
@ 2019-07-22 15:37 ` Lorenzo Pieralisi
  2019-07-23 11:46   ` Ulf Hansson
                     ` (2 more replies)
  2019-07-22 15:37 ` [PATCH 5/6] ARM: psci: cpuidle: Enable " Lorenzo Pieralisi
                   ` (3 subsequent siblings)
  7 siblings, 3 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-07-22 15:37 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Ulf Hansson, Sudeep Holla, Daniel Lezcano,
	Mark Rutland, Rafael J. Wysocki, Catalin Marinas, Will Deacon,
	LKML, LAKML

PSCI firmware is the standard power management control for
all ARM64 based platforms and it is also deployed on some
ARM 32 bit platforms to date.

Idle state entry in PSCI is currently achieved by calling
arm_cpuidle_init() and arm_cpuidle_suspend() in a generic
idle driver, which in turn relies on ARM/ARM64 CPUidle back-end
to relay the call into PSCI firmware if PSCI is the boot method.

Given that PSCI is the standard idle entry method on ARM64 systems
(which means that no other CPUidle driver are expected on ARM64
platforms - so PSCI is already a generic idle driver), in order to
simplify idle entry and code maintenance, it makes sense to have a PSCI
specific idle driver so that idle code that it is currently living in
drivers/firmware directory can be hoisted out of it and moved
where it belongs, into a full-fledged PSCI driver, leaving PSCI code
in drivers/firmware as a pure firmware interface, as it should be.

Implement a PSCI CPUidle driver. By default it is a silent Kconfig entry
which is left unselected, since it selection would clash with the
generic ARM CPUidle driver that provides a PSCI based idle driver
through the arm/arm64 arches back-ends CPU operations.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 MAINTAINERS                    |   8 ++
 drivers/cpuidle/Kconfig.arm    |   3 +
 drivers/cpuidle/Makefile       |   1 +
 drivers/cpuidle/cpuidle-psci.c | 150 +++++++++++++++++++++++++++++++++
 4 files changed, 162 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-psci.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 783569e3c4b4..c2bf8ce65e83 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4286,6 +4286,14 @@ S:	Supported
 F:	drivers/cpuidle/cpuidle-exynos.c
 F:	arch/arm/mach-exynos/pm.c
 
+CPUIDLE DRIVER - ARM PSCI
+M:	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
+M:	Sudeep Holla <sudeep.holla@arm.com>
+L:	linux-pm@vger.kernel.org
+L:	linux-arm-kernel@lists.infradead.org
+S:	Supported
+F:	drivers/cpuidle/cpuidle-psci.c
+
 CPU IDLE TIME MANAGEMENT FRAMEWORK
 M:	"Rafael J. Wysocki" <rjw@rjwysocki.net>
 M:	Daniel Lezcano <daniel.lezcano@linaro.org>
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 48cb3d4bb7d1..929b57424ea4 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -13,6 +13,9 @@ config ARM_CPUIDLE
           initialized by calling the CPU operations init idle hook
           provided by architecture code.
 
+config ARM_PSCI_CPUIDLE
+	bool
+
 config ARM_BIG_LITTLE_CPUIDLE
 	bool "Support for ARM big.LITTLE processors"
 	depends on ARCH_VEXPRESS_TC2_PM || ARCH_EXYNOS
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 9d7176cee3d3..40d016339b29 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
 obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
 obj-$(CONFIG_ARM_EXYNOS_CPUIDLE)        += cpuidle-exynos.o
 obj-$(CONFIG_ARM_CPUIDLE)		+= cpuidle-arm.o
+obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle-psci.o
 
 ###############################################################################
 # MIPS drivers
diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
new file mode 100644
index 000000000000..bdf02600e4e2
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * PSCI CPU idle driver.
+ *
+ * Copyright (C) 2019 ARM Ltd.
+ * Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
+ */
+
+#define pr_fmt(fmt) "CPUidle PSCI: " fmt
+
+#include <linux/cpuidle.h>
+#include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/psci.h>
+#include <linux/slab.h>
+
+#include <asm/cpuidle.h>
+
+#include "dt_idle_states.h"
+
+static int psci_enter_idle_state(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int idx)
+{
+	return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, idx);
+}
+
+static struct cpuidle_driver psci_idle_driver __initdata = {
+	.name = "psci_idle",
+	.owner = THIS_MODULE,
+	/*
+	 * PSCI idle states relies on architectural WFI to
+	 * be represented as state index 0.
+	 */
+	.states[0] = {
+		.enter                  = psci_enter_idle_state,
+		.exit_latency           = 1,
+		.target_residency       = 1,
+		.power_usage		= UINT_MAX,
+		.name                   = "WFI",
+		.desc                   = "ARM WFI",
+	}
+};
+
+static const struct of_device_id psci_idle_state_match[] __initconst = {
+	{ .compatible = "arm,idle-state",
+	  .data = psci_enter_idle_state },
+	{ },
+};
+
+static int __init psci_idle_init_cpu(int cpu)
+{
+	struct cpuidle_driver *drv;
+	struct device_node *cpu_node;
+	const char *enable_method;
+	int ret = 0;
+
+	drv = kmemdup(&psci_idle_driver, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	drv->cpumask = (struct cpumask *)cpumask_of(cpu);
+
+	cpu_node = of_get_cpu_node(cpu, NULL);
+	if (!cpu_node)
+		return -ENODEV;
+
+	/*
+	 * Check whether the enable-method for the cpu is PSCI, fail
+	 * if it is not.
+	 */
+	enable_method = of_get_property(cpu_node, "enable-method", NULL);
+	if (!enable_method || (strcmp(enable_method, "psci")))
+		ret = -ENODEV;
+
+	of_node_put(cpu_node);
+	if (ret)
+		return ret;
+
+	/*
+	 * Initialize idle states data, starting at index 1, since
+	 * by default idle state 0 is the quiescent state reached
+	 * by the cpu by executing the wfi instruction.
+	 *
+	 * If no DT idle states are detected (ret == 0) let the driver
+	 * initialization fail accordingly since there is no reason to
+	 * initialize the idle driver if only wfi is supported, the
+	 * default archictectural back-end already executes wfi
+	 * on idle entry.
+	 */
+	ret = dt_init_idle_driver(drv, psci_idle_state_match, 1);
+	if (ret <= 0) {
+		ret = ret ? : -ENODEV;
+		goto out_kfree_drv;
+	}
+
+	/*
+	 * Initialize PSCI idle states.
+	 */
+	ret = psci_cpu_init_idle(cpu);
+	if (ret) {
+		pr_err("CPU %d failed to PSCI idle\n", cpu);
+		goto out_kfree_drv;
+	}
+
+	ret = cpuidle_register(drv, NULL);
+	if (ret)
+		goto out_kfree_drv;
+
+	return 0;
+
+out_kfree_drv:
+	kfree(drv);
+	return ret;
+}
+
+/*
+ * psci_idle_init - Initializes PSCI cpuidle driver
+ *
+ * Initializes PSCI cpuidle driver for all CPUs, if any CPU fails
+ * to register cpuidle driver then rollback to cancel all CPUs
+ * registration.
+ */
+static int __init psci_idle_init(void)
+{
+	int cpu, ret;
+	struct cpuidle_driver *drv;
+	struct cpuidle_device *dev;
+
+	for_each_possible_cpu(cpu) {
+		ret = psci_idle_init_cpu(cpu);
+		if (ret)
+			goto out_fail;
+	}
+
+	return 0;
+
+out_fail:
+	while (--cpu >= 0) {
+		dev = per_cpu(cpuidle_devices, cpu);
+		drv = cpuidle_get_cpu_driver(dev);
+		cpuidle_unregister(drv);
+		kfree(drv);
+	}
+
+	return ret;
+}
+device_initcall(psci_idle_init);
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 5/6] ARM: psci: cpuidle: Enable PSCI CPUidle driver
  2019-07-22 15:37 [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework Lorenzo Pieralisi
                   ` (3 preceding siblings ...)
  2019-07-22 15:37 ` [PATCH 4/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver Lorenzo Pieralisi
@ 2019-07-22 15:37 ` Lorenzo Pieralisi
  2019-08-06 16:16   ` Sudeep Holla
  2019-07-22 15:37 ` [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling Lorenzo Pieralisi
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-07-22 15:37 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Will Deacon, Ulf Hansson, Sudeep Holla,
	Daniel Lezcano, Catalin Marinas, Mark Rutland, Rafael J. Wysocki,
	LKML, LAKML

Allow selection of the PSCI CPUidle in the kernel by adding
the required Kconfig options.

Remove PSCI callbacks from ARM/ARM64 generic CPU ops
to prevent the PSCI idle driver from clashing with the generic
ARM CPUidle driver initialization, that relies on CPU ops
to initialize and enter idle states.

Update the affected defconfig files to guarantee seamingless
transition from the generic ARM CPUidle to the PSCI CPUidle
driver on arch/platforms using it.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 arch/arm/configs/imx_v6_v7_defconfig | 1 +
 arch/arm64/configs/defconfig         | 1 +
 arch/arm64/kernel/cpuidle.c          | 7 ++++---
 arch/arm64/kernel/psci.c             | 4 ----
 drivers/cpuidle/Kconfig.arm          | 8 ++++++--
 drivers/firmware/psci/psci.c         | 9 ---------
 6 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index a53b29251ed4..4174fd1b79e7 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -60,6 +60,7 @@ CONFIG_ARM_IMX6Q_CPUFREQ=y
 CONFIG_ARM_IMX_CPUFREQ_DT=y
 CONFIG_CPU_IDLE=y
 CONFIG_ARM_CPUIDLE=y
+CONFIG_ARM_PSCI_CPUIDLE=y
 CONFIG_VFP=y
 CONFIG_NEON=y
 CONFIG_PM_DEBUG=y
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 0e58ef02880c..c0a7cfe3aebd 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -72,6 +72,7 @@ CONFIG_RANDOMIZE_BASE=y
 CONFIG_HIBERNATION=y
 CONFIG_WQ_POWER_EFFICIENT_DEFAULT=y
 CONFIG_ARM_CPUIDLE=y
+CONFIG_ARM_PSCI_CPUIDLE=y
 CONFIG_CPU_FREQ=y
 CONFIG_CPU_FREQ_STAT=y
 CONFIG_CPU_FREQ_GOV_POWERSAVE=m
diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index d1048173fd8a..4bcd1bca0dfc 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -11,6 +11,7 @@
 #include <linux/cpu_pm.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/psci.h>
 
 #include <asm/cpuidle.h>
 #include <asm/cpu_ops.h>
@@ -48,15 +49,15 @@ int arm_cpuidle_suspend(int index)
 
 int acpi_processor_ffh_lpi_probe(unsigned int cpu)
 {
-	return arm_cpuidle_init(cpu);
+	return psci_acpi_cpu_init_idle(cpu);
 }
 
 int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
 {
 	if (ARM64_LPI_IS_RETENTION_STATE(lpi->arch_flags))
-		return CPU_PM_CPU_IDLE_ENTER_RETENTION(arm_cpuidle_suspend,
+		return CPU_PM_CPU_IDLE_ENTER_RETENTION(psci_cpu_suspend_enter,
 						lpi->index);
 	else
-		return CPU_PM_CPU_IDLE_ENTER(arm_cpuidle_suspend, lpi->index);
+		return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, lpi->index);
 }
 #endif
diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
index 85ee7d07889e..a543ab7e007c 100644
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -105,10 +105,6 @@ static int cpu_psci_cpu_kill(unsigned int cpu)
 
 const struct cpu_operations cpu_psci_ops = {
 	.name		= "psci",
-#ifdef CONFIG_CPU_IDLE
-	.cpu_init_idle	= psci_cpu_init_idle,
-	.cpu_suspend	= psci_cpu_suspend_enter,
-#endif
 	.cpu_init	= cpu_psci_cpu_init,
 	.cpu_prepare	= cpu_psci_cpu_prepare,
 	.cpu_boot	= cpu_psci_cpu_boot,
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 929b57424ea4..b9c56c60ab98 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -14,8 +14,12 @@ config ARM_CPUIDLE
           provided by architecture code.
 
 config ARM_PSCI_CPUIDLE
-	bool
-
+	bool "PSCI CPU idle Driver"
+	depends on ARM_PSCI_FW
+	select DT_IDLE_STATES
+	select CPU_IDLE_MULTIPLE_DRIVERS
+	help
+	  Select this to enable PSCI firmware based CPUidle driver for ARM.
 config ARM_BIG_LITTLE_CPUIDLE
 	bool "Support for ARM big.LITTLE processors"
 	depends on ARCH_VEXPRESS_TC2_PM || ARCH_EXYNOS
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index f82ccd39a913..bae734d13a52 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -436,15 +436,6 @@ int psci_cpu_suspend_enter(unsigned long index)
 
 	return ret;
 }
-
-/* ARM specific CPU idle operations */
-#ifdef CONFIG_ARM
-static const struct cpuidle_ops psci_cpuidle_ops __initconst = {
-	.suspend = psci_cpu_suspend_enter,
-	.init = psci_dt_cpu_init_idle,
-};
-
-CPUIDLE_METHOD_OF_DECLARE(psci, "psci", &psci_cpuidle_ops);
 #endif
 #endif
 
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling
  2019-07-22 15:37 [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework Lorenzo Pieralisi
                   ` (4 preceding siblings ...)
  2019-07-22 15:37 ` [PATCH 5/6] ARM: psci: cpuidle: Enable " Lorenzo Pieralisi
@ 2019-07-22 15:37 ` Lorenzo Pieralisi
  2019-07-23 11:47   ` Ulf Hansson
                     ` (2 more replies)
  2019-07-23 11:49 ` [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework Ulf Hansson
  2019-08-09 11:03 ` [PATCH v2 0/8] " Lorenzo Pieralisi
  7 siblings, 3 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-07-22 15:37 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Will Deacon, Ulf Hansson, Sudeep Holla,
	Daniel Lezcano, Catalin Marinas, Mark Rutland, Rafael J. Wysocki,
	LKML, LAKML

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

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

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

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

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 arch/arm64/kernel/cpuidle.c    |  47 +++++++++-
 drivers/cpuidle/cpuidle-psci.c |  87 +++++++++++++++++-
 drivers/firmware/psci/psci.c   | 158 ++-------------------------------
 include/linux/cpuidle.h        |  17 +++-
 include/linux/psci.h           |   4 +-
 5 files changed, 153 insertions(+), 160 deletions(-)

diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index 4bcd1bca0dfc..e4d6af2fdec7 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -47,6 +47,44 @@ int arm_cpuidle_suspend(int index)
 
 #define ARM64_LPI_IS_RETENTION_STATE(arch_flags) (!(arch_flags))
 
+static int psci_acpi_cpu_init_idle(unsigned int cpu)
+{
+	int i, count;
+	struct acpi_lpi_state *lpi;
+	struct acpi_processor *pr = per_cpu(processors, cpu);
+
+	/*
+	 * If the PSCI cpu_suspend function hook has not been initialized
+	 * idle states must not be enabled, so bail out
+	 */
+	if (!psci_ops.cpu_suspend)
+		return -EOPNOTSUPP;
+
+	if (unlikely(!pr || !pr->flags.has_lpi))
+		return -EINVAL;
+
+	count = pr->power.count - 1;
+	if (count <= 0)
+		return -ENODEV;
+
+	for (i = 0; i < count; i++) {
+		u32 state;
+
+		lpi = &pr->power.lpi_states[i + 1];
+		/*
+		 * Only bits[31:0] represent a PSCI power_state while
+		 * bits[63:32] must be 0x0 as per ARM ACPI FFH Specification
+		 */
+		state = lpi->address;
+		if (!psci_power_state_is_valid(state)) {
+			pr_warn("Invalid PSCI power state %#x\n", state);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 int acpi_processor_ffh_lpi_probe(unsigned int cpu)
 {
 	return psci_acpi_cpu_init_idle(cpu);
@@ -54,10 +92,13 @@ int acpi_processor_ffh_lpi_probe(unsigned int cpu)
 
 int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
 {
+	u32 state = lpi->address;
+
 	if (ARM64_LPI_IS_RETENTION_STATE(lpi->arch_flags))
-		return CPU_PM_CPU_IDLE_ENTER_RETENTION(psci_cpu_suspend_enter,
-						lpi->index);
+		return CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(psci_cpu_suspend_enter,
+						lpi->index, state);
 	else
-		return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, lpi->index);
+		return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
+					     lpi->index, state);
 }
 #endif
diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index bdf02600e4e2..7485b3abe372 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -21,10 +21,15 @@
 
 #include "dt_idle_states.h"
 
+static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
+
 static int psci_enter_idle_state(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int idx)
 {
-	return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, idx);
+	u32 *state = __this_cpu_read(psci_power_state);
+
+	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
+					   idx, state[idx - 1]);
 }
 
 static struct cpuidle_driver psci_idle_driver __initdata = {
@@ -50,6 +55,86 @@ static const struct of_device_id psci_idle_state_match[] __initconst = {
 	{ },
 };
 
+static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
+{
+	int err = of_property_read_u32(np, "arm,psci-suspend-param", state);
+
+	if (err) {
+		pr_warn("%pOF missing arm,psci-suspend-param property\n", np);
+		return err;
+	}
+
+	if (!psci_power_state_is_valid(*state)) {
+		pr_warn("Invalid PSCI power state %#x\n", *state);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
+{
+	int i, ret = 0, count = 0;
+	u32 *psci_states;
+	struct device_node *state_node;
+
+	/* Count idle states */
+	while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
+					      count))) {
+		count++;
+		of_node_put(state_node);
+	}
+
+	if (!count)
+		return -ENODEV;
+
+	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
+	if (!psci_states)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		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);
+
+		if (ret)
+			goto free_mem;
+
+		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;
+
+free_mem:
+	kfree(psci_states);
+	return ret;
+}
+
+static __init int psci_cpu_init_idle(unsigned int cpu)
+{
+	struct device_node *cpu_node;
+	int ret;
+
+	/*
+	 * If the PSCI cpu_suspend function hook has not been initialized
+	 * idle states must not be enabled, so bail out
+	 */
+	if (!psci_ops.cpu_suspend)
+		return -EOPNOTSUPP;
+
+	cpu_node = of_get_cpu_node(cpu, NULL);
+	if (!cpu_node)
+		return -ENODEV;
+
+	ret = psci_dt_cpu_init_idle(cpu_node, cpu);
+
+	of_node_put(cpu_node);
+
+	return ret;
+}
+
 static int __init psci_idle_init_cpu(int cpu)
 {
 	struct cpuidle_driver *drv;
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index bae734d13a52..84f4ff351c62 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -103,7 +103,7 @@ static inline bool psci_power_state_loses_context(u32 state)
 	return state & mask;
 }
 
-static inline bool psci_power_state_is_valid(u32 state)
+bool psci_power_state_is_valid(u32 state)
 {
 	const u32 valid_mask = psci_has_ext_power_state() ?
 			       PSCI_1_0_EXT_POWER_STATE_MASK :
@@ -277,167 +277,25 @@ 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)
+static int psci_suspend_finisher(unsigned long state)
 {
-	int err = of_property_read_u32(np, "arm,psci-suspend-param", state);
+	u32 power_state = 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;
+	return psci_ops.cpu_suspend(power_state, __pa_symbol(cpu_resume));
 }
 
-static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
-{
-	int i, ret = 0, count = 0;
-	u32 *psci_states;
-	struct device_node *state_node;
-
-	/* Count idle states */
-	while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
-					      count))) {
-		count++;
-		of_node_put(state_node);
-	}
-
-	if (!count)
-		return -ENODEV;
-
-	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
-	if (!psci_states)
-		return -ENOMEM;
-
-	for (i = 0; i < count; i++) {
-		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);
-
-		if (ret)
-			goto free_mem;
-
-		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;
-
-free_mem:
-	kfree(psci_states);
-	return ret;
-}
-
-#ifdef CONFIG_ACPI
-#include <acpi/processor.h>
-
-static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
-{
-	int i, count;
-	u32 *psci_states;
-	struct acpi_lpi_state *lpi;
-	struct acpi_processor *pr = per_cpu(processors, cpu);
-
-	if (unlikely(!pr || !pr->flags.has_lpi))
-		return -EINVAL;
-
-	count = pr->power.count - 1;
-	if (count <= 0)
-		return -ENODEV;
-
-	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
-	if (!psci_states)
-		return -ENOMEM;
-
-	for (i = 0; i < count; i++) {
-		u32 state;
-
-		lpi = &pr->power.lpi_states[i + 1];
-		/*
-		 * Only bits[31:0] represent a PSCI power_state while
-		 * bits[63:32] must be 0x0 as per ARM ACPI FFH Specification
-		 */
-		state = lpi->address;
-		if (!psci_power_state_is_valid(state)) {
-			pr_warn("Invalid PSCI power state %#x\n", state);
-			kfree(psci_states);
-			return -EINVAL;
-		}
-		psci_states[i] = state;
-	}
-	/* Idle states parsed correctly, initialize per-cpu pointer */
-	per_cpu(psci_power_state, cpu) = psci_states;
-	return 0;
-}
-#else
-static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
-{
-	return -EINVAL;
-}
-#endif
-
-int psci_cpu_init_idle(unsigned int cpu)
-{
-	struct device_node *cpu_node;
-	int ret;
-
-	/*
-	 * If the PSCI cpu_suspend function hook has not been initialized
-	 * idle states must not be enabled, so bail out
-	 */
-	if (!psci_ops.cpu_suspend)
-		return -EOPNOTSUPP;
-
-	if (!acpi_disabled)
-		return psci_acpi_cpu_init_idle(cpu);
-
-	cpu_node = of_get_cpu_node(cpu, NULL);
-	if (!cpu_node)
-		return -ENODEV;
-
-	ret = psci_dt_cpu_init_idle(cpu_node, cpu);
-
-	of_node_put(cpu_node);
-
-	return ret;
-}
-
-static int psci_suspend_finisher(unsigned long index)
-{
-	u32 *state = __this_cpu_read(psci_power_state);
-
-	return psci_ops.cpu_suspend(state[index - 1],
-				    __pa_symbol(cpu_resume));
-}
-
-int psci_cpu_suspend_enter(unsigned long index)
+int psci_cpu_suspend_enter(u32 state)
 {
 	int ret;
-	u32 *state = __this_cpu_read(psci_power_state);
-	/*
-	 * idle state index 0 corresponds to wfi, should never be called
-	 * from the cpu_suspend operations
-	 */
-	if (WARN_ON_ONCE(!index))
-		return -EINVAL;
 
-	if (!psci_power_state_loses_context(state[index - 1]))
-		ret = psci_ops.cpu_suspend(state[index - 1], 0);
+	if (!psci_power_state_loses_context(state))
+		ret = psci_ops.cpu_suspend(state, 0);
 	else
-		ret = cpu_suspend(index, psci_suspend_finisher);
+		ret = cpu_suspend(state, psci_suspend_finisher);
 
 	return ret;
 }
 #endif
-#endif
 
 static int psci_system_suspend(unsigned long unused)
 {
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index bb9a0db89f1a..12ae4b87494e 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -256,7 +256,10 @@ static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 {return 0;}
 #endif
 
-#define __CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, is_retention) \
+#define __CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter,			\
+				idx,					\
+				state,					\
+				is_retention)				\
 ({									\
 	int __ret = 0;							\
 									\
@@ -268,7 +271,7 @@ static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 	if (!is_retention)						\
 		__ret =  cpu_pm_enter();				\
 	if (!__ret) {							\
-		__ret = low_level_idle_enter(idx);			\
+		__ret = low_level_idle_enter(state);			\
 		if (!is_retention)					\
 			cpu_pm_exit();					\
 	}								\
@@ -277,9 +280,15 @@ static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 })
 
 #define CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx)	\
-	__CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, 0)
+	__CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, idx, 0)
 
 #define CPU_PM_CPU_IDLE_ENTER_RETENTION(low_level_idle_enter, idx)	\
-	__CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, 1)
+	__CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, idx, 1)
+
+#define CPU_PM_CPU_IDLE_ENTER_PARAM(low_level_idle_enter, idx, state)	\
+	__CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, state, 0)
+
+#define CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(low_level_idle_enter, idx, state)	\
+	__CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, state, 1)
 
 #endif /* _LINUX_CPUIDLE_H */
diff --git a/include/linux/psci.h b/include/linux/psci.h
index a8a15613c157..e2bacc6fd2f2 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -15,8 +15,8 @@
 
 bool psci_tos_resident_on(int cpu);
 
-int psci_cpu_init_idle(unsigned int cpu);
-int psci_cpu_suspend_enter(unsigned long index);
+int psci_cpu_suspend_enter(u32 state);
+bool psci_power_state_is_valid(u32 state);
 
 enum psci_conduit {
 	PSCI_CONDUIT_NONE,
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* Re: [PATCH 4/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver
  2019-07-22 15:37 ` [PATCH 4/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver Lorenzo Pieralisi
@ 2019-07-23 11:46   ` Ulf Hansson
  2019-07-23 14:15     ` Lorenzo Pieralisi
  2019-08-06 16:10   ` Sudeep Holla
  2019-08-07 16:30   ` Daniel Lezcano
  2 siblings, 1 reply; 37+ messages in thread
From: Ulf Hansson @ 2019-07-23 11:46 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Linux PM, Sudeep Holla, Daniel Lezcano, Mark Rutland,
	Rafael J. Wysocki, Catalin Marinas, Will Deacon, LKML, LAKML

[...]

> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * PSCI CPU idle driver.
> + *
> + * Copyright (C) 2019 ARM Ltd.
> + * Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> + */
> +
> +#define pr_fmt(fmt) "CPUidle PSCI: " fmt
> +
> +#include <linux/cpuidle.h>
> +#include <linux/cpumask.h>
> +#include <linux/cpu_pm.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/psci.h>
> +#include <linux/slab.h>
> +
> +#include <asm/cpuidle.h>

This should go away, right?

> +
> +#include "dt_idle_states.h"
> +
> +static int psci_enter_idle_state(struct cpuidle_device *dev,
> +                               struct cpuidle_driver *drv, int idx)
> +{
> +       return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, idx);
> +}
> +
> +static struct cpuidle_driver psci_idle_driver __initdata = {
> +       .name = "psci_idle",
> +       .owner = THIS_MODULE,
> +       /*
> +        * PSCI idle states relies on architectural WFI to
> +        * be represented as state index 0.
> +        */
> +       .states[0] = {
> +               .enter                  = psci_enter_idle_state,
> +               .exit_latency           = 1,
> +               .target_residency       = 1,
> +               .power_usage            = UINT_MAX,
> +               .name                   = "WFI",
> +               .desc                   = "ARM WFI",
> +       }
> +};
> +
> +static const struct of_device_id psci_idle_state_match[] __initconst = {
> +       { .compatible = "arm,idle-state",
> +         .data = psci_enter_idle_state },
> +       { },
> +};
> +
> +static int __init psci_idle_init_cpu(int cpu)
> +{
> +       struct cpuidle_driver *drv;
> +       struct device_node *cpu_node;
> +       const char *enable_method;
> +       int ret = 0;
> +
> +       drv = kmemdup(&psci_idle_driver, sizeof(*drv), GFP_KERNEL);
> +       if (!drv)
> +               return -ENOMEM;
> +
> +       drv->cpumask = (struct cpumask *)cpumask_of(cpu);
> +
> +       cpu_node = of_get_cpu_node(cpu, NULL);
> +       if (!cpu_node)
> +               return -ENODEV;

You should free drv in case of error here (goto out_kfree_drv; etc).

> +
> +       /*
> +        * Check whether the enable-method for the cpu is PSCI, fail
> +        * if it is not.
> +        */
> +       enable_method = of_get_property(cpu_node, "enable-method", NULL);
> +       if (!enable_method || (strcmp(enable_method, "psci")))
> +               ret = -ENODEV;
> +
> +       of_node_put(cpu_node);
> +       if (ret)
> +               return ret;

You should free drv in case of error here (goto out_kfree_drv;).

> +
> +       /*
> +        * Initialize idle states data, starting at index 1, since
> +        * by default idle state 0 is the quiescent state reached
> +        * by the cpu by executing the wfi instruction.
> +        *
> +        * If no DT idle states are detected (ret == 0) let the driver
> +        * initialization fail accordingly since there is no reason to
> +        * initialize the idle driver if only wfi is supported, the
> +        * default archictectural back-end already executes wfi
> +        * on idle entry.
> +        */
> +       ret = dt_init_idle_driver(drv, psci_idle_state_match, 1);
> +       if (ret <= 0) {
> +               ret = ret ? : -ENODEV;
> +               goto out_kfree_drv;
> +       }
> +
> +       /*
> +        * Initialize PSCI idle states.
> +        */
> +       ret = psci_cpu_init_idle(cpu);
> +       if (ret) {
> +               pr_err("CPU %d failed to PSCI idle\n", cpu);
> +               goto out_kfree_drv;
> +       }
> +
> +       ret = cpuidle_register(drv, NULL);
> +       if (ret)
> +               goto out_kfree_drv;
> +
> +       return 0;
> +
> +out_kfree_drv:
> +       kfree(drv);
> +       return ret;
> +}
> +

[...]

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling
  2019-07-22 15:37 ` [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling Lorenzo Pieralisi
@ 2019-07-23 11:47   ` Ulf Hansson
  2019-08-07 18:09   ` Daniel Lezcano
  2019-08-08 12:55   ` Sudeep Holla
  2 siblings, 0 replies; 37+ messages in thread
From: Ulf Hansson @ 2019-07-23 11:47 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Linux PM, Will Deacon, Sudeep Holla, Daniel Lezcano,
	Catalin Marinas, Mark Rutland, Rafael J. Wysocki, LKML, LAKML

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

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

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

[...]

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework
  2019-07-22 15:37 [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework Lorenzo Pieralisi
                   ` (5 preceding siblings ...)
  2019-07-22 15:37 ` [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling Lorenzo Pieralisi
@ 2019-07-23 11:49 ` Ulf Hansson
  2019-07-23 14:19   ` Lorenzo Pieralisi
  2019-08-09 11:03 ` [PATCH v2 0/8] " Lorenzo Pieralisi
  7 siblings, 1 reply; 37+ messages in thread
From: Ulf Hansson @ 2019-07-23 11:49 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Linux PM, Will Deacon, Sudeep Holla, Daniel Lezcano,
	Catalin Marinas, Mark Rutland, Rafael J. Wysocki, LKML, LAKML

On Mon, 22 Jul 2019 at 17:37, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> Current PSCI CPUidle driver is built on top of the generic ARM
> CPUidle infrastructure that relies on the architectural back-end
> idle operations to initialize and enter idle states.
>
> On ARM64 systems, PSCI is the only interface the kernel ever uses
> to enter idle states, so, having to rely on a generic ARM CPUidle
> driver when there is and there will always be only one method
> for entering idle states proved to be overkill, more so given
> that on ARM 32-bit systems (that can also enable the generic
> ARM CPUidle driver) only one additional idle back-end was
> ever added:
>
> drivers/soc/qcom/spm.c
>
> and it can be easily converted to a full-fledged CPUidle driver
> without requiring the generic ARM CPUidle framework.
>
> Furthermore, the generic ARM CPUidle infrastructure forces the
> PSCI firmware layer to keep CPUidle specific information in it,
> which does not really fit its purpose that should be kernel
> control/data structure agnostic.
>
> Lastly, the interface between the generic ARM CPUidle driver and
> the arch back-end requires an idle state index to be passed to
> suspend operations, with idle states back-end internals (such
> as idle state parameters) hidden in architectural back-ends and
> not available to the generic ARM CPUidle driver.
>
> To improve the above mentioned shortcomings, implement a stand
> alone PSCI CPUidle driver; this improves the current kernel
> code from several perspective:
>
> - Move CPUidle internal knowledge into CPUidle driver out of
>   the PSCI firmware interface
> - Give the PSCI CPUidle driver control over power state parameters,
>   in particular in preparation for PSCI OSI support
> - Remove generic CPUidle operations infrastructure from the kernel
>
> This patchset does not go as far as removing the generic ARM CPUidle
> infrastructure in order to collect feedback on the new approach
> before completing the removal from the kernel, the generic and PSCI
> CPUidle driver are left to co-exist.

I like the approach and I think this series definitely moves things in
the right direction.

Of course, some additional cleanups/re-works on top are needed to show
its full benefit, but step by step we reach that point.

>
> Tested on Juno platform with both DT and ACPI boot firmwares.
>
> Cc: Will Deacon <will@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>
> Lorenzo Pieralisi (6):
>   ARM: cpuidle: Remove useless header include
>   ARM: cpuidle: Remove overzealous error logging
>   drivers: firmware: psci: Decouple checker from generic ARM CPUidle
>   ARM: psci: cpuidle: Introduce PSCI CPUidle driver
>   ARM: psci: cpuidle: Enable PSCI CPUidle driver
>   PSCI: cpuidle: Refactor CPU suspend power_state parameter handling
>
>  MAINTAINERS                          |   8 +
>  arch/arm/configs/imx_v6_v7_defconfig |   1 +
>  arch/arm64/configs/defconfig         |   1 +
>  arch/arm64/kernel/cpuidle.c          |  50 +++++-
>  arch/arm64/kernel/psci.c             |   4 -
>  drivers/cpuidle/Kconfig.arm          |   7 +
>  drivers/cpuidle/Makefile             |   1 +
>  drivers/cpuidle/cpuidle-arm.c        |  13 +-
>  drivers/cpuidle/cpuidle-psci.c       | 235 +++++++++++++++++++++++++++
>  drivers/firmware/psci/psci.c         | 167 +------------------
>  drivers/firmware/psci/psci_checker.c |  16 +-
>  include/linux/cpuidle.h              |  17 +-
>  include/linux/psci.h                 |   4 +-
>  13 files changed, 338 insertions(+), 186 deletions(-)
>  create mode 100644 drivers/cpuidle/cpuidle-psci.c
>
> --
> 2.21.0
>

For the series, besides the minor comments I had on patch 4, feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 4/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver
  2019-07-23 11:46   ` Ulf Hansson
@ 2019-07-23 14:15     ` Lorenzo Pieralisi
  0 siblings, 0 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-07-23 14:15 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Linux PM, Sudeep Holla, Daniel Lezcano, Mark Rutland,
	Rafael J. Wysocki, Catalin Marinas, Will Deacon, LKML, LAKML

On Tue, Jul 23, 2019 at 01:46:56PM +0200, Ulf Hansson wrote:
> [...]
> 
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -0,0 +1,150 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * PSCI CPU idle driver.
> > + *
> > + * Copyright (C) 2019 ARM Ltd.
> > + * Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > + */
> > +
> > +#define pr_fmt(fmt) "CPUidle PSCI: " fmt
> > +
> > +#include <linux/cpuidle.h>
> > +#include <linux/cpumask.h>
> > +#include <linux/cpu_pm.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/psci.h>
> > +#include <linux/slab.h>
> > +
> > +#include <asm/cpuidle.h>
> 
> This should go away, right?

We need to pull in cpu_do_idle() so it will have to stay there.

> > +#include "dt_idle_states.h"
> > +
> > +static int psci_enter_idle_state(struct cpuidle_device *dev,
> > +                               struct cpuidle_driver *drv, int idx)
> > +{
> > +       return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, idx);
> > +}
> > +
> > +static struct cpuidle_driver psci_idle_driver __initdata = {
> > +       .name = "psci_idle",
> > +       .owner = THIS_MODULE,
> > +       /*
> > +        * PSCI idle states relies on architectural WFI to
> > +        * be represented as state index 0.
> > +        */
> > +       .states[0] = {
> > +               .enter                  = psci_enter_idle_state,
> > +               .exit_latency           = 1,
> > +               .target_residency       = 1,
> > +               .power_usage            = UINT_MAX,
> > +               .name                   = "WFI",
> > +               .desc                   = "ARM WFI",
> > +       }
> > +};
> > +
> > +static const struct of_device_id psci_idle_state_match[] __initconst = {
> > +       { .compatible = "arm,idle-state",
> > +         .data = psci_enter_idle_state },
> > +       { },
> > +};
> > +
> > +static int __init psci_idle_init_cpu(int cpu)
> > +{
> > +       struct cpuidle_driver *drv;
> > +       struct device_node *cpu_node;
> > +       const char *enable_method;
> > +       int ret = 0;
> > +
> > +       drv = kmemdup(&psci_idle_driver, sizeof(*drv), GFP_KERNEL);
> > +       if (!drv)
> > +               return -ENOMEM;
> > +
> > +       drv->cpumask = (struct cpumask *)cpumask_of(cpu);
> > +
> > +       cpu_node = of_get_cpu_node(cpu, NULL);
> > +       if (!cpu_node)
> > +               return -ENODEV;
> 
> You should free drv in case of error here (goto out_kfree_drv; etc).
> 
> > +
> > +       /*
> > +        * Check whether the enable-method for the cpu is PSCI, fail
> > +        * if it is not.
> > +        */
> > +       enable_method = of_get_property(cpu_node, "enable-method", NULL);
> > +       if (!enable_method || (strcmp(enable_method, "psci")))
> > +               ret = -ENODEV;
> > +
> > +       of_node_put(cpu_node);
> > +       if (ret)
> > +               return ret;
> 
> You should free drv in case of error here (goto out_kfree_drv;).

True on both cases, I missed that, thanks.

Lorenzo

> > +
> > +       /*
> > +        * Initialize idle states data, starting at index 1, since
> > +        * by default idle state 0 is the quiescent state reached
> > +        * by the cpu by executing the wfi instruction.
> > +        *
> > +        * If no DT idle states are detected (ret == 0) let the driver
> > +        * initialization fail accordingly since there is no reason to
> > +        * initialize the idle driver if only wfi is supported, the
> > +        * default archictectural back-end already executes wfi
> > +        * on idle entry.
> > +        */
> > +       ret = dt_init_idle_driver(drv, psci_idle_state_match, 1);
> > +       if (ret <= 0) {
> > +               ret = ret ? : -ENODEV;
> > +               goto out_kfree_drv;
> > +       }
> > +
> > +       /*
> > +        * Initialize PSCI idle states.
> > +        */
> > +       ret = psci_cpu_init_idle(cpu);
> > +       if (ret) {
> > +               pr_err("CPU %d failed to PSCI idle\n", cpu);
> > +               goto out_kfree_drv;
> > +       }
> > +
> > +       ret = cpuidle_register(drv, NULL);
> > +       if (ret)
> > +               goto out_kfree_drv;
> > +
> > +       return 0;
> > +
> > +out_kfree_drv:
> > +       kfree(drv);
> > +       return ret;
> > +}
> > +
> 
> [...]
> 
> Kind regards
> Uffe

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework
  2019-07-23 11:49 ` [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework Ulf Hansson
@ 2019-07-23 14:19   ` Lorenzo Pieralisi
  0 siblings, 0 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-07-23 14:19 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Linux PM, Will Deacon, Sudeep Holla, Daniel Lezcano,
	Catalin Marinas, Mark Rutland, Rafael J. Wysocki, LKML, LAKML

On Tue, Jul 23, 2019 at 01:49:15PM +0200, Ulf Hansson wrote:
> On Mon, 22 Jul 2019 at 17:37, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > Current PSCI CPUidle driver is built on top of the generic ARM
> > CPUidle infrastructure that relies on the architectural back-end
> > idle operations to initialize and enter idle states.
> >
> > On ARM64 systems, PSCI is the only interface the kernel ever uses
> > to enter idle states, so, having to rely on a generic ARM CPUidle
> > driver when there is and there will always be only one method
> > for entering idle states proved to be overkill, more so given
> > that on ARM 32-bit systems (that can also enable the generic
> > ARM CPUidle driver) only one additional idle back-end was
> > ever added:
> >
> > drivers/soc/qcom/spm.c
> >
> > and it can be easily converted to a full-fledged CPUidle driver
> > without requiring the generic ARM CPUidle framework.
> >
> > Furthermore, the generic ARM CPUidle infrastructure forces the
> > PSCI firmware layer to keep CPUidle specific information in it,
> > which does not really fit its purpose that should be kernel
> > control/data structure agnostic.
> >
> > Lastly, the interface between the generic ARM CPUidle driver and
> > the arch back-end requires an idle state index to be passed to
> > suspend operations, with idle states back-end internals (such
> > as idle state parameters) hidden in architectural back-ends and
> > not available to the generic ARM CPUidle driver.
> >
> > To improve the above mentioned shortcomings, implement a stand
> > alone PSCI CPUidle driver; this improves the current kernel
> > code from several perspective:
> >
> > - Move CPUidle internal knowledge into CPUidle driver out of
> >   the PSCI firmware interface
> > - Give the PSCI CPUidle driver control over power state parameters,
> >   in particular in preparation for PSCI OSI support
> > - Remove generic CPUidle operations infrastructure from the kernel
> >
> > This patchset does not go as far as removing the generic ARM CPUidle
> > infrastructure in order to collect feedback on the new approach
> > before completing the removal from the kernel, the generic and PSCI
> > CPUidle driver are left to co-exist.
> 
> I like the approach and I think this series definitely moves things in
> the right direction.
> 
> Of course, some additional cleanups/re-works on top are needed to show
> its full benefit, but step by step we reach that point.

I pushed code out as we agreed.

git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git

branch: cpuidle/psci-driver

I will version the code as I update the patches, I will leave
them on the list for this week before sending a v2.

> > Tested on Juno platform with both DT and ACPI boot firmwares.
> >
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >
> > Lorenzo Pieralisi (6):
> >   ARM: cpuidle: Remove useless header include
> >   ARM: cpuidle: Remove overzealous error logging
> >   drivers: firmware: psci: Decouple checker from generic ARM CPUidle
> >   ARM: psci: cpuidle: Introduce PSCI CPUidle driver
> >   ARM: psci: cpuidle: Enable PSCI CPUidle driver
> >   PSCI: cpuidle: Refactor CPU suspend power_state parameter handling
> >
> >  MAINTAINERS                          |   8 +
> >  arch/arm/configs/imx_v6_v7_defconfig |   1 +
> >  arch/arm64/configs/defconfig         |   1 +
> >  arch/arm64/kernel/cpuidle.c          |  50 +++++-
> >  arch/arm64/kernel/psci.c             |   4 -
> >  drivers/cpuidle/Kconfig.arm          |   7 +
> >  drivers/cpuidle/Makefile             |   1 +
> >  drivers/cpuidle/cpuidle-arm.c        |  13 +-
> >  drivers/cpuidle/cpuidle-psci.c       | 235 +++++++++++++++++++++++++++
> >  drivers/firmware/psci/psci.c         | 167 +------------------
> >  drivers/firmware/psci/psci_checker.c |  16 +-
> >  include/linux/cpuidle.h              |  17 +-
> >  include/linux/psci.h                 |   4 +-
> >  13 files changed, 338 insertions(+), 186 deletions(-)
> >  create mode 100644 drivers/cpuidle/cpuidle-psci.c
> >
> > --
> > 2.21.0
> >
> 
> For the series, besides the minor comments I had on patch 4, feel free to add:
> 
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Thanks !
Lorenzo

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 1/6] ARM: cpuidle: Remove useless header include
  2019-07-22 15:37 ` [PATCH 1/6] ARM: cpuidle: Remove useless header include Lorenzo Pieralisi
@ 2019-08-06 15:51   ` Sudeep Holla
  2019-08-07  8:19   ` Daniel Lezcano
  1 sibling, 0 replies; 37+ messages in thread
From: Sudeep Holla @ 2019-08-06 15:51 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pm, Ulf Hansson, Daniel Lezcano, Rafael J. Wysocki,
	Catalin Marinas, Will Deacon, Mark Rutland, LKML, LAKML

On Mon, Jul 22, 2019 at 04:37:40PM +0100, Lorenzo Pieralisi wrote:
> The generic ARM CPUidle driver includes <linux/topology.h> by mistake.
> 
> Remove the topology header include.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 2/6] ARM: cpuidle: Remove overzealous error logging
  2019-07-22 15:37 ` [PATCH 2/6] ARM: cpuidle: Remove overzealous error logging Lorenzo Pieralisi
@ 2019-08-06 15:51   ` Sudeep Holla
  2019-08-07  8:20   ` Daniel Lezcano
  1 sibling, 0 replies; 37+ messages in thread
From: Sudeep Holla @ 2019-08-06 15:51 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pm, Ulf Hansson, Daniel Lezcano, Rafael J. Wysocki,
	Catalin Marinas, Will Deacon, Mark Rutland, LKML, LAKML

On Mon, Jul 22, 2019 at 04:37:41PM +0100, Lorenzo Pieralisi wrote:
> CPUidle back-end operations are not implemented in some platforms
> but this should not be considered an error serious enough to be
> logged. Check the arm_cpuidle_init() return value to detect whether
> the failure must be reported or not in the kernel log and do
> not log it if the platform does not support CPUidle operations.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/6] drivers: firmware: psci: Decouple checker from generic ARM CPUidle
  2019-07-22 15:37 ` [PATCH 3/6] drivers: firmware: psci: Decouple checker from generic ARM CPUidle Lorenzo Pieralisi
@ 2019-08-06 15:54   ` Sudeep Holla
  2019-08-07 14:09   ` Daniel Lezcano
  1 sibling, 0 replies; 37+ messages in thread
From: Sudeep Holla @ 2019-08-06 15:54 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pm, Mark Rutland, Catalin Marinas, Will Deacon,
	Ulf Hansson, Daniel Lezcano, Rafael J. Wysocki, LKML, LAKML

On Mon, Jul 22, 2019 at 04:37:42PM +0100, Lorenzo Pieralisi wrote:
> The PSCI checker currently relies on the generic ARM CPUidle
> infrastructure to enter an idle state, which in turn creates
> a dependency that is not really needed.
> 
> The PSCI checker code to test PSCI CPU suspend is built on
> top of the CPUidle framework and can easily reuse the
> struct cpuidle_state.enter() function (previously initialized
> by an idle driver, with a PSCI back-end) to trigger an entry
> into an idle state, decoupling the PSCI checker from the
> generic ARM CPUidle infrastructure and simplyfing the code
> in the process.
> 
> Convert the PSCI checker suspend entry function to use
> the struct cpuidle_state.enter() function callback.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>

Not sure why we didn't take this path from the beginning.
Anyways make sense and much needed for later patches in the series.

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 4/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver
  2019-07-22 15:37 ` [PATCH 4/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver Lorenzo Pieralisi
  2019-07-23 11:46   ` Ulf Hansson
@ 2019-08-06 16:10   ` Sudeep Holla
  2019-08-06 16:34     ` Lorenzo Pieralisi
  2019-08-07 16:30   ` Daniel Lezcano
  2 siblings, 1 reply; 37+ messages in thread
From: Sudeep Holla @ 2019-08-06 16:10 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pm, Ulf Hansson, Daniel Lezcano, Mark Rutland,
	Rafael J. Wysocki, Catalin Marinas, Will Deacon, LKML, LAKML

On Mon, Jul 22, 2019 at 04:37:43PM +0100, Lorenzo Pieralisi wrote:
> PSCI firmware is the standard power management control for
> all ARM64 based platforms and it is also deployed on some
> ARM 32 bit platforms to date.
>
> Idle state entry in PSCI is currently achieved by calling
> arm_cpuidle_init() and arm_cpuidle_suspend() in a generic
> idle driver, which in turn relies on ARM/ARM64 CPUidle back-end
> to relay the call into PSCI firmware if PSCI is the boot method.
>
> Given that PSCI is the standard idle entry method on ARM64 systems
> (which means that no other CPUidle driver are expected on ARM64
> platforms - so PSCI is already a generic idle driver), in order to
> simplify idle entry and code maintenance, it makes sense to have a PSCI
> specific idle driver so that idle code that it is currently living in
> drivers/firmware directory can be hoisted out of it and moved
> where it belongs, into a full-fledged PSCI driver, leaving PSCI code
> in drivers/firmware as a pure firmware interface, as it should be.
>
> Implement a PSCI CPUidle driver. By default it is a silent Kconfig entry
> which is left unselected, since it selection would clash with the
> generic ARM CPUidle driver that provides a PSCI based idle driver
> through the arm/arm64 arches back-ends CPU operations.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>

Once the error path issues pointed by Ulf are resolved,

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> ---
>  MAINTAINERS                    |   8 ++
>  drivers/cpuidle/Kconfig.arm    |   3 +
>  drivers/cpuidle/Makefile       |   1 +
>  drivers/cpuidle/cpuidle-psci.c | 150 +++++++++++++++++++++++++++++++++
>  4 files changed, 162 insertions(+)
>  create mode 100644 drivers/cpuidle/cpuidle-psci.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 783569e3c4b4..c2bf8ce65e83 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4286,6 +4286,14 @@ S:	Supported
>  F:	drivers/cpuidle/cpuidle-exynos.c
>  F:	arch/arm/mach-exynos/pm.c
>
> +CPUIDLE DRIVER - ARM PSCI
> +M:	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> +M:	Sudeep Holla <sudeep.holla@arm.com>
> +L:	linux-pm@vger.kernel.org
> +L:	linux-arm-kernel@lists.infradead.org
> +S:	Supported
> +F:	drivers/cpuidle/cpuidle-psci.c
> +
>  CPU IDLE TIME MANAGEMENT FRAMEWORK
>  M:	"Rafael J. Wysocki" <rjw@rjwysocki.net>
>  M:	Daniel Lezcano <daniel.lezcano@linaro.org>
> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> index 48cb3d4bb7d1..929b57424ea4 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -13,6 +13,9 @@ config ARM_CPUIDLE
>            initialized by calling the CPU operations init idle hook
>            provided by architecture code.
>
> +config ARM_PSCI_CPUIDLE
> +	bool
> +

[nit] I understand the intention to keep it hidden, but can't we have
the dependency and selection of other config as part of this patch to
make it more complete ?

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/6] ARM: psci: cpuidle: Enable PSCI CPUidle driver
  2019-07-22 15:37 ` [PATCH 5/6] ARM: psci: cpuidle: Enable " Lorenzo Pieralisi
@ 2019-08-06 16:16   ` Sudeep Holla
  2019-08-06 16:40     ` Lorenzo Pieralisi
  0 siblings, 1 reply; 37+ messages in thread
From: Sudeep Holla @ 2019-08-06 16:16 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pm, Will Deacon, Ulf Hansson, Daniel Lezcano,
	Catalin Marinas, Mark Rutland, Rafael J. Wysocki, LKML, LAKML

On Mon, Jul 22, 2019 at 04:37:44PM +0100, Lorenzo Pieralisi wrote:
> Allow selection of the PSCI CPUidle in the kernel by adding
> the required Kconfig options.
> 
> Remove PSCI callbacks from ARM/ARM64 generic CPU ops
> to prevent the PSCI idle driver from clashing with the generic
> ARM CPUidle driver initialization, that relies on CPU ops
> to initialize and enter idle states.
> 
> Update the affected defconfig files to guarantee seamingless
> transition from the generic ARM CPUidle to the PSCI CPUidle
> driver on arch/platforms using it.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> ---
>  arch/arm/configs/imx_v6_v7_defconfig | 1 +
>  arch/arm64/configs/defconfig         | 1 +

Better to keep above you as separate patch, though it may cause
minor issues from bisectibility. It may be needed anyway for merging.

>  arch/arm64/kernel/cpuidle.c          | 7 ++++---
>  arch/arm64/kernel/psci.c             | 4 ----
>  drivers/cpuidle/Kconfig.arm          | 8 ++++++--
>  drivers/firmware/psci/psci.c         | 9 ---------
>  6 files changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
> index a53b29251ed4..4174fd1b79e7 100644
> --- a/arch/arm/configs/imx_v6_v7_defconfig
> +++ b/arch/arm/configs/imx_v6_v7_defconfig
> @@ -60,6 +60,7 @@ CONFIG_ARM_IMX6Q_CPUFREQ=y
>  CONFIG_ARM_IMX_CPUFREQ_DT=y
>  CONFIG_CPU_IDLE=y
>  CONFIG_ARM_CPUIDLE=y
> +CONFIG_ARM_PSCI_CPUIDLE=y
>  CONFIG_VFP=y
>  CONFIG_NEON=y
>  CONFIG_PM_DEBUG=y
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 0e58ef02880c..c0a7cfe3aebd 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -72,6 +72,7 @@ CONFIG_RANDOMIZE_BASE=y
>  CONFIG_HIBERNATION=y
>  CONFIG_WQ_POWER_EFFICIENT_DEFAULT=y
>  CONFIG_ARM_CPUIDLE=y
> +CONFIG_ARM_PSCI_CPUIDLE=y
>  CONFIG_CPU_FREQ=y
>  CONFIG_CPU_FREQ_STAT=y
>  CONFIG_CPU_FREQ_GOV_POWERSAVE=m
> diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
> index d1048173fd8a..4bcd1bca0dfc 100644
> --- a/arch/arm64/kernel/cpuidle.c
> +++ b/arch/arm64/kernel/cpuidle.c
> @@ -11,6 +11,7 @@
>  #include <linux/cpu_pm.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/psci.h>
>  
>  #include <asm/cpuidle.h>
>  #include <asm/cpu_ops.h>
> @@ -48,15 +49,15 @@ int arm_cpuidle_suspend(int index)
>  
>  int acpi_processor_ffh_lpi_probe(unsigned int cpu)
>  {
> -	return arm_cpuidle_init(cpu);
> +	return psci_acpi_cpu_init_idle(cpu);

This will break build as psci_acpi_cpu_init_idle is introduced in next patch.
You can simply move it to next patch I assume.

>  }
>  
>  int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
>  {
>  	if (ARM64_LPI_IS_RETENTION_STATE(lpi->arch_flags))
> -		return CPU_PM_CPU_IDLE_ENTER_RETENTION(arm_cpuidle_suspend,
> +		return CPU_PM_CPU_IDLE_ENTER_RETENTION(psci_cpu_suspend_enter,
>  						lpi->index);
>  	else
> -		return CPU_PM_CPU_IDLE_ENTER(arm_cpuidle_suspend, lpi->index);
> +		return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, lpi->index);
>  }
>  #endif
> diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
> index 85ee7d07889e..a543ab7e007c 100644
> --- a/arch/arm64/kernel/psci.c
> +++ b/arch/arm64/kernel/psci.c
> @@ -105,10 +105,6 @@ static int cpu_psci_cpu_kill(unsigned int cpu)
>  
>  const struct cpu_operations cpu_psci_ops = {
>  	.name		= "psci",
> -#ifdef CONFIG_CPU_IDLE
> -	.cpu_init_idle	= psci_cpu_init_idle,
> -	.cpu_suspend	= psci_cpu_suspend_enter,
> -#endif
>  	.cpu_init	= cpu_psci_cpu_init,
>  	.cpu_prepare	= cpu_psci_cpu_prepare,
>  	.cpu_boot	= cpu_psci_cpu_boot,
> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> index 929b57424ea4..b9c56c60ab98 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -14,8 +14,12 @@ config ARM_CPUIDLE
>            provided by architecture code.
>  
>  config ARM_PSCI_CPUIDLE
> -	bool
> -
> +	bool "PSCI CPU idle Driver"

As mentioned in previous patch, do you see issues having just above
change in this patch and the below ones moved to the previous.

> +	depends on ARM_PSCI_FW
> +	select DT_IDLE_STATES
> +	select CPU_IDLE_MULTIPLE_DRIVERS
> +	help
> +	  Select this to enable PSCI firmware based CPUidle driver for ARM.

You need extra blank line here.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 4/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver
  2019-08-06 16:10   ` Sudeep Holla
@ 2019-08-06 16:34     ` Lorenzo Pieralisi
  0 siblings, 0 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-08-06 16:34 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-pm, Ulf Hansson, Daniel Lezcano, Mark Rutland,
	Rafael J. Wysocki, Catalin Marinas, Will Deacon, LKML, LAKML

On Tue, Aug 06, 2019 at 05:10:33PM +0100, Sudeep Holla wrote:
> On Mon, Jul 22, 2019 at 04:37:43PM +0100, Lorenzo Pieralisi wrote:
> > PSCI firmware is the standard power management control for
> > all ARM64 based platforms and it is also deployed on some
> > ARM 32 bit platforms to date.
> >
> > Idle state entry in PSCI is currently achieved by calling
> > arm_cpuidle_init() and arm_cpuidle_suspend() in a generic
> > idle driver, which in turn relies on ARM/ARM64 CPUidle back-end
> > to relay the call into PSCI firmware if PSCI is the boot method.
> >
> > Given that PSCI is the standard idle entry method on ARM64 systems
> > (which means that no other CPUidle driver are expected on ARM64
> > platforms - so PSCI is already a generic idle driver), in order to
> > simplify idle entry and code maintenance, it makes sense to have a PSCI
> > specific idle driver so that idle code that it is currently living in
> > drivers/firmware directory can be hoisted out of it and moved
> > where it belongs, into a full-fledged PSCI driver, leaving PSCI code
> > in drivers/firmware as a pure firmware interface, as it should be.
> >
> > Implement a PSCI CPUidle driver. By default it is a silent Kconfig entry
> > which is left unselected, since it selection would clash with the
> > generic ARM CPUidle driver that provides a PSCI based idle driver
> > through the arm/arm64 arches back-ends CPU operations.
> >
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> 
> Once the error path issues pointed by Ulf are resolved,
> 
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > ---
> >  MAINTAINERS                    |   8 ++
> >  drivers/cpuidle/Kconfig.arm    |   3 +
> >  drivers/cpuidle/Makefile       |   1 +
> >  drivers/cpuidle/cpuidle-psci.c | 150 +++++++++++++++++++++++++++++++++
> >  4 files changed, 162 insertions(+)
> >  create mode 100644 drivers/cpuidle/cpuidle-psci.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 783569e3c4b4..c2bf8ce65e83 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4286,6 +4286,14 @@ S:	Supported
> >  F:	drivers/cpuidle/cpuidle-exynos.c
> >  F:	arch/arm/mach-exynos/pm.c
> >
> > +CPUIDLE DRIVER - ARM PSCI
> > +M:	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > +M:	Sudeep Holla <sudeep.holla@arm.com>
> > +L:	linux-pm@vger.kernel.org
> > +L:	linux-arm-kernel@lists.infradead.org
> > +S:	Supported
> > +F:	drivers/cpuidle/cpuidle-psci.c
> > +
> >  CPU IDLE TIME MANAGEMENT FRAMEWORK
> >  M:	"Rafael J. Wysocki" <rjw@rjwysocki.net>
> >  M:	Daniel Lezcano <daniel.lezcano@linaro.org>
> > diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> > index 48cb3d4bb7d1..929b57424ea4 100644
> > --- a/drivers/cpuidle/Kconfig.arm
> > +++ b/drivers/cpuidle/Kconfig.arm
> > @@ -13,6 +13,9 @@ config ARM_CPUIDLE
> >            initialized by calling the CPU operations init idle hook
> >            provided by architecture code.
> >
> > +config ARM_PSCI_CPUIDLE
> > +	bool
> > +
> 
> [nit] I understand the intention to keep it hidden, but can't we have
> the dependency and selection of other config as part of this patch to
> make it more complete ?

Yes we can, it makes sense.

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/6] ARM: psci: cpuidle: Enable PSCI CPUidle driver
  2019-08-06 16:16   ` Sudeep Holla
@ 2019-08-06 16:40     ` Lorenzo Pieralisi
  0 siblings, 0 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-08-06 16:40 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-pm, Will Deacon, Ulf Hansson, Daniel Lezcano,
	Catalin Marinas, Mark Rutland, Rafael J. Wysocki, LKML, LAKML

On Tue, Aug 06, 2019 at 05:16:24PM +0100, Sudeep Holla wrote:
> On Mon, Jul 22, 2019 at 04:37:44PM +0100, Lorenzo Pieralisi wrote:
> > Allow selection of the PSCI CPUidle in the kernel by adding
> > the required Kconfig options.
> > 
> > Remove PSCI callbacks from ARM/ARM64 generic CPU ops
> > to prevent the PSCI idle driver from clashing with the generic
> > ARM CPUidle driver initialization, that relies on CPU ops
> > to initialize and enter idle states.
> > 
> > Update the affected defconfig files to guarantee seamingless
> > transition from the generic ARM CPUidle to the PSCI CPUidle
> > driver on arch/platforms using it.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > ---
> >  arch/arm/configs/imx_v6_v7_defconfig | 1 +
> >  arch/arm64/configs/defconfig         | 1 +
> 
> Better to keep above you as separate patch, though it may cause
> minor issues from bisectibility. It may be needed anyway for merging.

That's a good point, I will split these bits in a separate patch.

> >  arch/arm64/kernel/cpuidle.c          | 7 ++++---
> >  arch/arm64/kernel/psci.c             | 4 ----
> >  drivers/cpuidle/Kconfig.arm          | 8 ++++++--
> >  drivers/firmware/psci/psci.c         | 9 ---------
> >  6 files changed, 12 insertions(+), 18 deletions(-)
> > 
> > diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
> > index a53b29251ed4..4174fd1b79e7 100644
> > --- a/arch/arm/configs/imx_v6_v7_defconfig
> > +++ b/arch/arm/configs/imx_v6_v7_defconfig
> > @@ -60,6 +60,7 @@ CONFIG_ARM_IMX6Q_CPUFREQ=y
> >  CONFIG_ARM_IMX_CPUFREQ_DT=y
> >  CONFIG_CPU_IDLE=y
> >  CONFIG_ARM_CPUIDLE=y
> > +CONFIG_ARM_PSCI_CPUIDLE=y
> >  CONFIG_VFP=y
> >  CONFIG_NEON=y
> >  CONFIG_PM_DEBUG=y
> > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> > index 0e58ef02880c..c0a7cfe3aebd 100644
> > --- a/arch/arm64/configs/defconfig
> > +++ b/arch/arm64/configs/defconfig
> > @@ -72,6 +72,7 @@ CONFIG_RANDOMIZE_BASE=y
> >  CONFIG_HIBERNATION=y
> >  CONFIG_WQ_POWER_EFFICIENT_DEFAULT=y
> >  CONFIG_ARM_CPUIDLE=y
> > +CONFIG_ARM_PSCI_CPUIDLE=y
> >  CONFIG_CPU_FREQ=y
> >  CONFIG_CPU_FREQ_STAT=y
> >  CONFIG_CPU_FREQ_GOV_POWERSAVE=m
> > diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
> > index d1048173fd8a..4bcd1bca0dfc 100644
> > --- a/arch/arm64/kernel/cpuidle.c
> > +++ b/arch/arm64/kernel/cpuidle.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/cpu_pm.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > +#include <linux/psci.h>
> >  
> >  #include <asm/cpuidle.h>
> >  #include <asm/cpu_ops.h>
> > @@ -48,15 +49,15 @@ int arm_cpuidle_suspend(int index)
> >  
> >  int acpi_processor_ffh_lpi_probe(unsigned int cpu)
> >  {
> > -	return arm_cpuidle_init(cpu);
> > +	return psci_acpi_cpu_init_idle(cpu);
> 
> This will break build as psci_acpi_cpu_init_idle is introduced in next patch.
> You can simply move it to next patch I assume.

Yes, it is a bisectability issue, I fixed it already but thanks
for spotting it anyway.

> >  }
> >  
> >  int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
> >  {
> >  	if (ARM64_LPI_IS_RETENTION_STATE(lpi->arch_flags))
> > -		return CPU_PM_CPU_IDLE_ENTER_RETENTION(arm_cpuidle_suspend,
> > +		return CPU_PM_CPU_IDLE_ENTER_RETENTION(psci_cpu_suspend_enter,
> >  						lpi->index);
> >  	else
> > -		return CPU_PM_CPU_IDLE_ENTER(arm_cpuidle_suspend, lpi->index);
> > +		return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, lpi->index);
> >  }
> >  #endif
> > diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
> > index 85ee7d07889e..a543ab7e007c 100644
> > --- a/arch/arm64/kernel/psci.c
> > +++ b/arch/arm64/kernel/psci.c
> > @@ -105,10 +105,6 @@ static int cpu_psci_cpu_kill(unsigned int cpu)
> >  
> >  const struct cpu_operations cpu_psci_ops = {
> >  	.name		= "psci",
> > -#ifdef CONFIG_CPU_IDLE
> > -	.cpu_init_idle	= psci_cpu_init_idle,
> > -	.cpu_suspend	= psci_cpu_suspend_enter,
> > -#endif
> >  	.cpu_init	= cpu_psci_cpu_init,
> >  	.cpu_prepare	= cpu_psci_cpu_prepare,
> >  	.cpu_boot	= cpu_psci_cpu_boot,
> > diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> > index 929b57424ea4..b9c56c60ab98 100644
> > --- a/drivers/cpuidle/Kconfig.arm
> > +++ b/drivers/cpuidle/Kconfig.arm
> > @@ -14,8 +14,12 @@ config ARM_CPUIDLE
> >            provided by architecture code.
> >  
> >  config ARM_PSCI_CPUIDLE
> > -	bool
> > -
> > +	bool "PSCI CPU idle Driver"
> 
> As mentioned in previous patch, do you see issues having just above
> change in this patch and the below ones moved to the previous.

No you are right, I will make this change.

> > +	depends on ARM_PSCI_FW
> > +	select DT_IDLE_STATES
> > +	select CPU_IDLE_MULTIPLE_DRIVERS
> > +	help
> > +	  Select this to enable PSCI firmware based CPUidle driver for ARM.
> 
> You need extra blank line here.

OK.

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 1/6] ARM: cpuidle: Remove useless header include
  2019-07-22 15:37 ` [PATCH 1/6] ARM: cpuidle: Remove useless header include Lorenzo Pieralisi
  2019-08-06 15:51   ` Sudeep Holla
@ 2019-08-07  8:19   ` Daniel Lezcano
  1 sibling, 0 replies; 37+ messages in thread
From: Daniel Lezcano @ 2019-08-07  8:19 UTC (permalink / raw)
  To: Lorenzo Pieralisi, linux-pm
  Cc: Ulf Hansson, Sudeep Holla, Rafael J. Wysocki, Catalin Marinas,
	Will Deacon, Mark Rutland, LKML, LAKML

On 22/07/2019 17:37, Lorenzo Pieralisi wrote:
> The generic ARM CPUidle driver includes <linux/topology.h> by mistake.
> 
> Remove the topology header include.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 2/6] ARM: cpuidle: Remove overzealous error logging
  2019-07-22 15:37 ` [PATCH 2/6] ARM: cpuidle: Remove overzealous error logging Lorenzo Pieralisi
  2019-08-06 15:51   ` Sudeep Holla
@ 2019-08-07  8:20   ` Daniel Lezcano
  1 sibling, 0 replies; 37+ messages in thread
From: Daniel Lezcano @ 2019-08-07  8:20 UTC (permalink / raw)
  To: Lorenzo Pieralisi, linux-pm
  Cc: Ulf Hansson, Sudeep Holla, Rafael J. Wysocki, Catalin Marinas,
	Will Deacon, Mark Rutland, LKML, LAKML

On 22/07/2019 17:37, Lorenzo Pieralisi wrote:
> CPUidle back-end operations are not implemented in some platforms
> but this should not be considered an error serious enough to be
> logged. Check the arm_cpuidle_init() return value to detect whether
> the failure must be reported or not in the kernel log and do
> not log it if the platform does not support CPUidle operations.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/6] drivers: firmware: psci: Decouple checker from generic ARM CPUidle
  2019-07-22 15:37 ` [PATCH 3/6] drivers: firmware: psci: Decouple checker from generic ARM CPUidle Lorenzo Pieralisi
  2019-08-06 15:54   ` Sudeep Holla
@ 2019-08-07 14:09   ` Daniel Lezcano
  1 sibling, 0 replies; 37+ messages in thread
From: Daniel Lezcano @ 2019-08-07 14:09 UTC (permalink / raw)
  To: Lorenzo Pieralisi, linux-pm
  Cc: Sudeep Holla, Mark Rutland, Catalin Marinas, Will Deacon,
	Ulf Hansson, Rafael J. Wysocki, LKML, LAKML

On 22/07/2019 17:37, Lorenzo Pieralisi wrote:
> The PSCI checker currently relies on the generic ARM CPUidle
> infrastructure to enter an idle state, which in turn creates
> a dependency that is not really needed.
> 
> The PSCI checker code to test PSCI CPU suspend is built on
> top of the CPUidle framework and can easily reuse the
> struct cpuidle_state.enter() function (previously initialized
> by an idle driver, with a PSCI back-end) to trigger an entry
> into an idle state, decoupling the PSCI checker from the
> generic ARM CPUidle infrastructure and simplyfing the code
> in the process.
> 
> Convert the PSCI checker suspend entry function to use
> the struct cpuidle_state.enter() function callback.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>

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

> ---
>  drivers/firmware/psci/psci_checker.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/firmware/psci/psci_checker.c b/drivers/firmware/psci/psci_checker.c
> index f3659443f8c2..6a445397771c 100644
> --- a/drivers/firmware/psci/psci_checker.c
> +++ b/drivers/firmware/psci/psci_checker.c
> @@ -228,8 +228,11 @@ static int hotplug_tests(void)
>  
>  static void dummy_callback(struct timer_list *unused) {}
>  
> -static int suspend_cpu(int index, bool broadcast)
> +static int suspend_cpu(struct cpuidle_device *dev,
> +		       struct cpuidle_driver *drv, int index)
>  {
> +	struct cpuidle_state *state = &drv->states[index];
> +	bool broadcast = state->flags & CPUIDLE_FLAG_TIMER_STOP;
>  	int ret;
>  
>  	arch_cpu_idle_enter();
> @@ -254,11 +257,7 @@ static int suspend_cpu(int index, bool broadcast)
>  		}
>  	}
>  
> -	/*
> -	 * Replicate the common ARM cpuidle enter function
> -	 * (arm_enter_idle_state).
> -	 */
> -	ret = CPU_PM_CPU_IDLE_ENTER(arm_cpuidle_suspend, index);
> +	ret = state->enter(dev, drv, index);
>  
>  	if (broadcast)
>  		tick_broadcast_exit();
> @@ -301,9 +300,8 @@ static int suspend_test_thread(void *arg)
>  		 * doesn't use PSCI).
>  		 */
>  		for (index = 1; index < drv->state_count; ++index) {
> -			struct cpuidle_state *state = &drv->states[index];
> -			bool broadcast = state->flags & CPUIDLE_FLAG_TIMER_STOP;
>  			int ret;
> +			struct cpuidle_state *state = &drv->states[index];
>  
>  			/*
>  			 * Set the timer to wake this CPU up in some time (which
> @@ -318,7 +316,7 @@ static int suspend_test_thread(void *arg)
>  			/* IRQs must be disabled during suspend operations. */
>  			local_irq_disable();
>  
> -			ret = suspend_cpu(index, broadcast);
> +			ret = suspend_cpu(dev, drv, index);
>  
>  			/*
>  			 * We have woken up. Re-enable IRQs to handle any
> 


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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 4/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver
  2019-07-22 15:37 ` [PATCH 4/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver Lorenzo Pieralisi
  2019-07-23 11:46   ` Ulf Hansson
  2019-08-06 16:10   ` Sudeep Holla
@ 2019-08-07 16:30   ` Daniel Lezcano
  2 siblings, 0 replies; 37+ messages in thread
From: Daniel Lezcano @ 2019-08-07 16:30 UTC (permalink / raw)
  To: Lorenzo Pieralisi, linux-pm
  Cc: Ulf Hansson, Sudeep Holla, Mark Rutland, Rafael J. Wysocki,
	Catalin Marinas, Will Deacon, LKML, LAKML

On 22/07/2019 17:37, Lorenzo Pieralisi wrote:
> PSCI firmware is the standard power management control for
> all ARM64 based platforms and it is also deployed on some
> ARM 32 bit platforms to date.
> 
> Idle state entry in PSCI is currently achieved by calling
> arm_cpuidle_init() and arm_cpuidle_suspend() in a generic
> idle driver, which in turn relies on ARM/ARM64 CPUidle back-end
> to relay the call into PSCI firmware if PSCI is the boot method.
> 
> Given that PSCI is the standard idle entry method on ARM64 systems
> (which means that no other CPUidle driver are expected on ARM64
> platforms - so PSCI is already a generic idle driver), in order to
> simplify idle entry and code maintenance, it makes sense to have a PSCI
> specific idle driver so that idle code that it is currently living in
> drivers/firmware directory can be hoisted out of it and moved
> where it belongs, into a full-fledged PSCI driver, leaving PSCI code
> in drivers/firmware as a pure firmware interface, as it should be.
> 
> Implement a PSCI CPUidle driver. By default it is a silent Kconfig entry
> which is left unselected, since it selection would clash with the
> generic ARM CPUidle driver that provides a PSCI based idle driver
> through the arm/arm64 arches back-ends CPU operations.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> ---

Modulo Ulf and Sudeep comments,

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling
  2019-07-22 15:37 ` [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling Lorenzo Pieralisi
  2019-07-23 11:47   ` Ulf Hansson
@ 2019-08-07 18:09   ` Daniel Lezcano
  2019-08-08 12:55   ` Sudeep Holla
  2 siblings, 0 replies; 37+ messages in thread
From: Daniel Lezcano @ 2019-08-07 18:09 UTC (permalink / raw)
  To: Lorenzo Pieralisi, linux-pm
  Cc: Will Deacon, Ulf Hansson, Sudeep Holla, Catalin Marinas,
	Mark Rutland, Rafael J. Wysocki, LKML, LAKML

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

AFAICT,

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling
  2019-07-22 15:37 ` [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling Lorenzo Pieralisi
  2019-07-23 11:47   ` Ulf Hansson
  2019-08-07 18:09   ` Daniel Lezcano
@ 2019-08-08 12:55   ` Sudeep Holla
  2019-08-08 15:29     ` Ulf Hansson
  2 siblings, 1 reply; 37+ messages in thread
From: Sudeep Holla @ 2019-08-08 12:55 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pm, Will Deacon, Ulf Hansson, Daniel Lezcano,
	Catalin Marinas, Mark Rutland, Rafael J. Wysocki, LKML, LAKML

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

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

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

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

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling
  2019-08-08 12:55   ` Sudeep Holla
@ 2019-08-08 15:29     ` Ulf Hansson
  2019-08-08 16:04       ` Sudeep Holla
  0 siblings, 1 reply; 37+ messages in thread
From: Ulf Hansson @ 2019-08-08 15:29 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Lorenzo Pieralisi, Linux PM, Will Deacon, Daniel Lezcano,
	Catalin Marinas, Mark Rutland, Rafael J. Wysocki, LKML, LAKML

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

Good idea!

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

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling
  2019-08-08 15:29     ` Ulf Hansson
@ 2019-08-08 16:04       ` Sudeep Holla
  0 siblings, 0 replies; 37+ messages in thread
From: Sudeep Holla @ 2019-08-08 16:04 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Lorenzo Pieralisi, Linux PM, Will Deacon, Daniel Lezcano,
	Catalin Marinas, Mark Rutland, Rafael J. Wysocki, LKML, LAKML

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

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

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v2 0/8] ARM: psci: cpuidle: PSCI CPUidle rework
  2019-07-22 15:37 [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework Lorenzo Pieralisi
                   ` (6 preceding siblings ...)
  2019-07-23 11:49 ` [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework Ulf Hansson
@ 2019-08-09 11:03 ` Lorenzo Pieralisi
  2019-08-09 11:03   ` [PATCH v2 1/8] ARM: cpuidle: Remove useless header include Lorenzo Pieralisi
                     ` (7 more replies)
  7 siblings, 8 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-08-09 11:03 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Will Deacon, Shawn Guo, Ulf Hansson,
	Sudeep Holla, Daniel Lezcano, Catalin Marinas, Mark Rutland,
	Rafael J. Wysocki, LKML, LAKML

v2 of a previous posting:

v1: https://lore.kernel.org/linux-pm/20190722153745.32446-1-lorenzo.pieralisi@arm.com/

v1->v2:

- Split config files updates into separate patches
- Fixed minor memory leaks/bisectability issues

Original cover letter
---

Current PSCI CPUidle driver is built on top of the generic ARM
CPUidle infrastructure that relies on the architectural back-end
idle operations to initialize and enter idle states.

On ARM64 systems, PSCI is the only interface the kernel ever uses
to enter idle states, so, having to rely on a generic ARM CPUidle
driver when there is and there will always be only one method
for entering idle states proved to be overkill, more so given
that on ARM 32-bit systems (that can also enable the generic
ARM CPUidle driver) only one additional idle back-end was
ever added:

drivers/soc/qcom/spm.c

and it can be easily converted to a full-fledged CPUidle driver
without requiring the generic ARM CPUidle framework.

Furthermore, the generic ARM CPUidle infrastructure forces the
PSCI firmware layer to keep CPUidle specific information in it,
which does not really fit its purpose that should be kernel
control/data structure agnostic.

Lastly, the interface between the generic ARM CPUidle driver and
the arch back-end requires an idle state index to be passed to
suspend operations, with idle states back-end internals (such
as idle state parameters) hidden in architectural back-ends and
not available to the generic ARM CPUidle driver.

To improve the above mentioned shortcomings, implement a stand
alone PSCI CPUidle driver; this improves the current kernel
code from several perspective:

- Move CPUidle internal knowledge into CPUidle driver out of
  the PSCI firmware interface
- Give the PSCI CPUidle driver control over power state parameters,
  in particular in preparation for PSCI OSI support
- Remove generic CPUidle operations infrastructure from the kernel

This patchset does not go as far as removing the generic ARM CPUidle
infrastructure in order to collect feedback on the new approach
before completing the removal from the kernel, the generic and PSCI
CPUidle driver are left to co-exist.

Tested on Juno platform with both DT and ACPI boot firmwares.

Cc: Will Deacon <will@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

Lorenzo Pieralisi (8):
  ARM: cpuidle: Remove useless header include
  ARM: cpuidle: Remove overzealous error logging
  drivers: firmware: psci: Decouple checker from generic ARM CPUidle
  ARM: psci: cpuidle: Introduce PSCI CPUidle driver
  ARM: psci: cpuidle: Enable PSCI CPUidle driver
  PSCI: cpuidle: Refactor CPU suspend power_state parameter handling
  arm64: defconfig: Enable the PSCI CPUidle driver
  ARM: imx_v6_v7_defconfig: Enable the PSCI CPUidle driver

 MAINTAINERS                          |   8 +
 arch/arm/configs/imx_v6_v7_defconfig |   1 +
 arch/arm64/configs/defconfig         |   1 +
 arch/arm64/kernel/cpuidle.c          |  50 +++++-
 arch/arm64/kernel/psci.c             |   4 -
 drivers/cpuidle/Kconfig.arm          |  10 ++
 drivers/cpuidle/Makefile             |   1 +
 drivers/cpuidle/cpuidle-arm.c        |  13 +-
 drivers/cpuidle/cpuidle-psci.c       | 236 +++++++++++++++++++++++++++
 drivers/firmware/psci/psci.c         | 167 +------------------
 drivers/firmware/psci/psci_checker.c |  16 +-
 include/linux/cpuidle.h              |  17 +-
 include/linux/psci.h                 |   4 +-
 13 files changed, 342 insertions(+), 186 deletions(-)
 create mode 100644 drivers/cpuidle/cpuidle-psci.c

-- 
2.21.0


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v2 1/8] ARM: cpuidle: Remove useless header include
  2019-08-09 11:03 ` [PATCH v2 0/8] " Lorenzo Pieralisi
@ 2019-08-09 11:03   ` Lorenzo Pieralisi
  2019-08-09 11:03   ` [PATCH v2 2/8] ARM: cpuidle: Remove overzealous error logging Lorenzo Pieralisi
                     ` (6 subsequent siblings)
  7 siblings, 0 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-08-09 11:03 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Daniel Lezcano, Ulf Hansson, Sudeep Holla,
	Rafael J. Wysocki, Catalin Marinas, Will Deacon, Mark Rutland,
	Shawn Guo, LKML, LAKML

The generic ARM CPUidle driver includes <linux/topology.h> by mistake.

Remove the topology header include.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 drivers/cpuidle/cpuidle-arm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 5bcd82c35dcf..dc33b3d2954f 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -15,7 +15,6 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/slab.h>
-#include <linux/topology.h>
 
 #include <asm/cpuidle.h>
 
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v2 2/8] ARM: cpuidle: Remove overzealous error logging
  2019-08-09 11:03 ` [PATCH v2 0/8] " Lorenzo Pieralisi
  2019-08-09 11:03   ` [PATCH v2 1/8] ARM: cpuidle: Remove useless header include Lorenzo Pieralisi
@ 2019-08-09 11:03   ` Lorenzo Pieralisi
  2019-08-09 11:03   ` [PATCH v2 3/8] drivers: firmware: psci: Decouple checker from generic ARM CPUidle Lorenzo Pieralisi
                     ` (5 subsequent siblings)
  7 siblings, 0 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-08-09 11:03 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Daniel Lezcano, Ulf Hansson, Sudeep Holla,
	Rafael J. Wysocki, Catalin Marinas, Will Deacon, Mark Rutland,
	Shawn Guo, LKML, LAKML

CPUidle back-end operations are not implemented in some platforms
but this should not be considered an error serious enough to be
logged. Check the arm_cpuidle_init() return value to detect whether
the failure must be reported or not in the kernel log and do
not log it if the platform does not support CPUidle operations.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 drivers/cpuidle/cpuidle-arm.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index dc33b3d2954f..9e5156d39627 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -105,11 +105,17 @@ static int __init arm_idle_init_cpu(int cpu)
 	ret = arm_cpuidle_init(cpu);
 
 	/*
-	 * Allow the initialization to continue for other CPUs, if the reported
-	 * failure is a HW misconfiguration/breakage (-ENXIO).
+	 * Allow the initialization to continue for other CPUs, if the
+	 * reported failure is a HW misconfiguration/breakage (-ENXIO).
+	 *
+	 * Some platforms do not support idle operations
+	 * (arm_cpuidle_init() returning -EOPNOTSUPP), we should
+	 * not flag this case as an error, it is a valid
+	 * configuration.
 	 */
 	if (ret) {
-		pr_err("CPU %d failed to init idle CPU ops\n", cpu);
+		if (ret != -EOPNOTSUPP)
+			pr_err("CPU %d failed to init idle CPU ops\n", cpu);
 		ret = ret == -ENXIO ? 0 : ret;
 		goto out_kfree_drv;
 	}
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v2 3/8] drivers: firmware: psci: Decouple checker from generic ARM CPUidle
  2019-08-09 11:03 ` [PATCH v2 0/8] " Lorenzo Pieralisi
  2019-08-09 11:03   ` [PATCH v2 1/8] ARM: cpuidle: Remove useless header include Lorenzo Pieralisi
  2019-08-09 11:03   ` [PATCH v2 2/8] ARM: cpuidle: Remove overzealous error logging Lorenzo Pieralisi
@ 2019-08-09 11:03   ` Lorenzo Pieralisi
  2019-08-09 11:03   ` [PATCH v2 4/8] ARM: psci: cpuidle: Introduce PSCI CPUidle driver Lorenzo Pieralisi
                     ` (4 subsequent siblings)
  7 siblings, 0 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-08-09 11:03 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Daniel Lezcano, Ulf Hansson, Sudeep Holla,
	Mark Rutland, Catalin Marinas, Will Deacon, Rafael J. Wysocki,
	Shawn Guo, LKML, LAKML

The PSCI checker currently relies on the generic ARM CPUidle
infrastructure to enter an idle state, which in turn creates
a dependency that is not really needed.

The PSCI checker code to test PSCI CPU suspend is built on
top of the CPUidle framework and can easily reuse the
struct cpuidle_state.enter() function (previously initialized
by an idle driver, with a PSCI back-end) to trigger an entry
into an idle state, decoupling the PSCI checker from the
generic ARM CPUidle infrastructure and simplyfing the code
in the process.

Convert the PSCI checker suspend entry function to use
the struct cpuidle_state.enter() function callback.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 drivers/firmware/psci/psci_checker.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/psci/psci_checker.c b/drivers/firmware/psci/psci_checker.c
index f3659443f8c2..6a445397771c 100644
--- a/drivers/firmware/psci/psci_checker.c
+++ b/drivers/firmware/psci/psci_checker.c
@@ -228,8 +228,11 @@ static int hotplug_tests(void)
 
 static void dummy_callback(struct timer_list *unused) {}
 
-static int suspend_cpu(int index, bool broadcast)
+static int suspend_cpu(struct cpuidle_device *dev,
+		       struct cpuidle_driver *drv, int index)
 {
+	struct cpuidle_state *state = &drv->states[index];
+	bool broadcast = state->flags & CPUIDLE_FLAG_TIMER_STOP;
 	int ret;
 
 	arch_cpu_idle_enter();
@@ -254,11 +257,7 @@ static int suspend_cpu(int index, bool broadcast)
 		}
 	}
 
-	/*
-	 * Replicate the common ARM cpuidle enter function
-	 * (arm_enter_idle_state).
-	 */
-	ret = CPU_PM_CPU_IDLE_ENTER(arm_cpuidle_suspend, index);
+	ret = state->enter(dev, drv, index);
 
 	if (broadcast)
 		tick_broadcast_exit();
@@ -301,9 +300,8 @@ static int suspend_test_thread(void *arg)
 		 * doesn't use PSCI).
 		 */
 		for (index = 1; index < drv->state_count; ++index) {
-			struct cpuidle_state *state = &drv->states[index];
-			bool broadcast = state->flags & CPUIDLE_FLAG_TIMER_STOP;
 			int ret;
+			struct cpuidle_state *state = &drv->states[index];
 
 			/*
 			 * Set the timer to wake this CPU up in some time (which
@@ -318,7 +316,7 @@ static int suspend_test_thread(void *arg)
 			/* IRQs must be disabled during suspend operations. */
 			local_irq_disable();
 
-			ret = suspend_cpu(index, broadcast);
+			ret = suspend_cpu(dev, drv, index);
 
 			/*
 			 * We have woken up. Re-enable IRQs to handle any
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v2 4/8] ARM: psci: cpuidle: Introduce PSCI CPUidle driver
  2019-08-09 11:03 ` [PATCH v2 0/8] " Lorenzo Pieralisi
                     ` (2 preceding siblings ...)
  2019-08-09 11:03   ` [PATCH v2 3/8] drivers: firmware: psci: Decouple checker from generic ARM CPUidle Lorenzo Pieralisi
@ 2019-08-09 11:03   ` Lorenzo Pieralisi
  2019-08-09 11:03   ` [PATCH v2 5/8] ARM: psci: cpuidle: Enable " Lorenzo Pieralisi
                     ` (3 subsequent siblings)
  7 siblings, 0 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-08-09 11:03 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Daniel Lezcano, Ulf Hansson, Sudeep Holla,
	Mark Rutland, Rafael J. Wysocki, Catalin Marinas, Will Deacon,
	Shawn Guo, LKML, LAKML

PSCI firmware is the standard power management control for
all ARM64 based platforms and it is also deployed on some
ARM 32 bit platforms to date.

Idle state entry in PSCI is currently achieved by calling
arm_cpuidle_init() and arm_cpuidle_suspend() in a generic
idle driver, which in turn relies on ARM/ARM64 CPUidle back-end
to relay the call into PSCI firmware if PSCI is the boot method.

Given that PSCI is the standard idle entry method on ARM64 systems
(which means that no other CPUidle driver are expected on ARM64
platforms - so PSCI is already a generic idle driver), in order to
simplify idle entry and code maintenance, it makes sense to have a PSCI
specific idle driver so that idle code that it is currently living in
drivers/firmware directory can be hoisted out of it and moved
where it belongs, into a full-fledged PSCI driver, leaving PSCI code
in drivers/firmware as a pure firmware interface, as it should be.

Implement a PSCI CPUidle driver. By default it is a silent Kconfig entry
which is left unselected, since it selection would clash with the
generic ARM CPUidle driver that provides a PSCI based idle driver
through the arm/arm64 arches back-ends CPU operations.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 MAINTAINERS                    |   8 ++
 drivers/cpuidle/Kconfig.arm    |  10 +++
 drivers/cpuidle/Makefile       |   1 +
 drivers/cpuidle/cpuidle-psci.c | 151 +++++++++++++++++++++++++++++++++
 4 files changed, 170 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-psci.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 783569e3c4b4..c2bf8ce65e83 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4286,6 +4286,14 @@ S:	Supported
 F:	drivers/cpuidle/cpuidle-exynos.c
 F:	arch/arm/mach-exynos/pm.c
 
+CPUIDLE DRIVER - ARM PSCI
+M:	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
+M:	Sudeep Holla <sudeep.holla@arm.com>
+L:	linux-pm@vger.kernel.org
+L:	linux-arm-kernel@lists.infradead.org
+S:	Supported
+F:	drivers/cpuidle/cpuidle-psci.c
+
 CPU IDLE TIME MANAGEMENT FRAMEWORK
 M:	"Rafael J. Wysocki" <rjw@rjwysocki.net>
 M:	Daniel Lezcano <daniel.lezcano@linaro.org>
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 48cb3d4bb7d1..eb014aa5ce6b 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -13,6 +13,16 @@ config ARM_CPUIDLE
           initialized by calling the CPU operations init idle hook
           provided by architecture code.
 
+config ARM_PSCI_CPUIDLE
+	bool
+	depends on ARM_PSCI_FW
+	select DT_IDLE_STATES
+	select CPU_IDLE_MULTIPLE_DRIVERS
+	help
+	  Select this to enable PSCI firmware based CPUidle driver for ARM.
+	  It provides an idle driver that is capable of detecting and
+	  managing idle states through the PSCI firmware interface.
+
 config ARM_BIG_LITTLE_CPUIDLE
 	bool "Support for ARM big.LITTLE processors"
 	depends on ARCH_VEXPRESS_TC2_PM || ARCH_EXYNOS
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 9d7176cee3d3..40d016339b29 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
 obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
 obj-$(CONFIG_ARM_EXYNOS_CPUIDLE)        += cpuidle-exynos.o
 obj-$(CONFIG_ARM_CPUIDLE)		+= cpuidle-arm.o
+obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle-psci.o
 
 ###############################################################################
 # MIPS drivers
diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
new file mode 100644
index 000000000000..ab1dea918ea3
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * PSCI CPU idle driver.
+ *
+ * Copyright (C) 2019 ARM Ltd.
+ * Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
+ */
+
+#define pr_fmt(fmt) "CPUidle PSCI: " fmt
+
+#include <linux/cpuidle.h>
+#include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/psci.h>
+#include <linux/slab.h>
+
+#include <asm/cpuidle.h>
+
+#include "dt_idle_states.h"
+
+static int psci_enter_idle_state(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int idx)
+{
+	return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, idx);
+}
+
+static struct cpuidle_driver psci_idle_driver __initdata = {
+	.name = "psci_idle",
+	.owner = THIS_MODULE,
+	/*
+	 * PSCI idle states relies on architectural WFI to
+	 * be represented as state index 0.
+	 */
+	.states[0] = {
+		.enter                  = psci_enter_idle_state,
+		.exit_latency           = 1,
+		.target_residency       = 1,
+		.power_usage		= UINT_MAX,
+		.name                   = "WFI",
+		.desc                   = "ARM WFI",
+	}
+};
+
+static const struct of_device_id psci_idle_state_match[] __initconst = {
+	{ .compatible = "arm,idle-state",
+	  .data = psci_enter_idle_state },
+	{ },
+};
+
+static int __init psci_idle_init_cpu(int cpu)
+{
+	struct cpuidle_driver *drv;
+	struct device_node *cpu_node;
+	const char *enable_method;
+	int ret = 0;
+
+	cpu_node = of_cpu_device_node_get(cpu);
+	if (!cpu_node)
+		return -ENODEV;
+
+	/*
+	 * Check whether the enable-method for the cpu is PSCI, fail
+	 * if it is not.
+	 */
+	enable_method = of_get_property(cpu_node, "enable-method", NULL);
+	if (!enable_method || (strcmp(enable_method, "psci")))
+		ret = -ENODEV;
+
+	of_node_put(cpu_node);
+	if (ret)
+		return ret;
+
+	drv = kmemdup(&psci_idle_driver, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	drv->cpumask = (struct cpumask *)cpumask_of(cpu);
+
+	/*
+	 * Initialize idle states data, starting at index 1, since
+	 * by default idle state 0 is the quiescent state reached
+	 * by the cpu by executing the wfi instruction.
+	 *
+	 * If no DT idle states are detected (ret == 0) let the driver
+	 * initialization fail accordingly since there is no reason to
+	 * initialize the idle driver if only wfi is supported, the
+	 * default archictectural back-end already executes wfi
+	 * on idle entry.
+	 */
+	ret = dt_init_idle_driver(drv, psci_idle_state_match, 1);
+	if (ret <= 0) {
+		ret = ret ? : -ENODEV;
+		goto out_kfree_drv;
+	}
+
+	/*
+	 * Initialize PSCI idle states.
+	 */
+	ret = psci_cpu_init_idle(cpu);
+	if (ret) {
+		pr_err("CPU %d failed to PSCI idle\n", cpu);
+		goto out_kfree_drv;
+	}
+
+	ret = cpuidle_register(drv, NULL);
+	if (ret)
+		goto out_kfree_drv;
+
+	return 0;
+
+out_kfree_drv:
+	kfree(drv);
+	return ret;
+}
+
+/*
+ * psci_idle_init - Initializes PSCI cpuidle driver
+ *
+ * Initializes PSCI cpuidle driver for all CPUs, if any CPU fails
+ * to register cpuidle driver then rollback to cancel all CPUs
+ * registration.
+ */
+static int __init psci_idle_init(void)
+{
+	int cpu, ret;
+	struct cpuidle_driver *drv;
+	struct cpuidle_device *dev;
+
+	for_each_possible_cpu(cpu) {
+		ret = psci_idle_init_cpu(cpu);
+		if (ret)
+			goto out_fail;
+	}
+
+	return 0;
+
+out_fail:
+	while (--cpu >= 0) {
+		dev = per_cpu(cpuidle_devices, cpu);
+		drv = cpuidle_get_cpu_driver(dev);
+		cpuidle_unregister(drv);
+		kfree(drv);
+	}
+
+	return ret;
+}
+device_initcall(psci_idle_init);
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v2 5/8] ARM: psci: cpuidle: Enable PSCI CPUidle driver
  2019-08-09 11:03 ` [PATCH v2 0/8] " Lorenzo Pieralisi
                     ` (3 preceding siblings ...)
  2019-08-09 11:03   ` [PATCH v2 4/8] ARM: psci: cpuidle: Introduce PSCI CPUidle driver Lorenzo Pieralisi
@ 2019-08-09 11:03   ` Lorenzo Pieralisi
  2019-08-09 11:03   ` [PATCH v2 6/8] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling Lorenzo Pieralisi
                     ` (2 subsequent siblings)
  7 siblings, 0 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-08-09 11:03 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Ulf Hansson, Will Deacon, Sudeep Holla,
	Daniel Lezcano, Catalin Marinas, Mark Rutland, Rafael J. Wysocki,
	Shawn Guo, LKML, LAKML

Allow selection of the PSCI CPUidle in the kernel by updating
the respective Kconfig entry.

Remove PSCI callbacks from ARM/ARM64 generic CPU ops
to prevent the PSCI idle driver from clashing with the generic
ARM CPUidle driver initialization, that relies on CPU ops
to initialize and enter idle states.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Will Deacon <will@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 arch/arm64/kernel/cpuidle.c  |  7 ++++---
 arch/arm64/kernel/psci.c     |  4 ----
 drivers/cpuidle/Kconfig.arm  |  2 +-
 drivers/firmware/psci/psci.c | 10 ----------
 4 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index d1048173fd8a..619e0ebb8399 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -11,6 +11,7 @@
 #include <linux/cpu_pm.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/psci.h>
 
 #include <asm/cpuidle.h>
 #include <asm/cpu_ops.h>
@@ -48,15 +49,15 @@ int arm_cpuidle_suspend(int index)
 
 int acpi_processor_ffh_lpi_probe(unsigned int cpu)
 {
-	return arm_cpuidle_init(cpu);
+	return psci_cpu_init_idle(cpu);
 }
 
 int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
 {
 	if (ARM64_LPI_IS_RETENTION_STATE(lpi->arch_flags))
-		return CPU_PM_CPU_IDLE_ENTER_RETENTION(arm_cpuidle_suspend,
+		return CPU_PM_CPU_IDLE_ENTER_RETENTION(psci_cpu_suspend_enter,
 						lpi->index);
 	else
-		return CPU_PM_CPU_IDLE_ENTER(arm_cpuidle_suspend, lpi->index);
+		return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, lpi->index);
 }
 #endif
diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
index 85ee7d07889e..a543ab7e007c 100644
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -105,10 +105,6 @@ static int cpu_psci_cpu_kill(unsigned int cpu)
 
 const struct cpu_operations cpu_psci_ops = {
 	.name		= "psci",
-#ifdef CONFIG_CPU_IDLE
-	.cpu_init_idle	= psci_cpu_init_idle,
-	.cpu_suspend	= psci_cpu_suspend_enter,
-#endif
 	.cpu_init	= cpu_psci_cpu_init,
 	.cpu_prepare	= cpu_psci_cpu_prepare,
 	.cpu_boot	= cpu_psci_cpu_boot,
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index eb014aa5ce6b..d8530475493c 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -14,7 +14,7 @@ config ARM_CPUIDLE
           provided by architecture code.
 
 config ARM_PSCI_CPUIDLE
-	bool
+	bool "PSCI CPU idle Driver"
 	depends on ARM_PSCI_FW
 	select DT_IDLE_STATES
 	select CPU_IDLE_MULTIPLE_DRIVERS
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index f82ccd39a913..b343f8a34c6a 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -436,16 +436,6 @@ int psci_cpu_suspend_enter(unsigned long index)
 
 	return ret;
 }
-
-/* ARM specific CPU idle operations */
-#ifdef CONFIG_ARM
-static const struct cpuidle_ops psci_cpuidle_ops __initconst = {
-	.suspend = psci_cpu_suspend_enter,
-	.init = psci_dt_cpu_init_idle,
-};
-
-CPUIDLE_METHOD_OF_DECLARE(psci, "psci", &psci_cpuidle_ops);
-#endif
 #endif
 
 static int psci_system_suspend(unsigned long unused)
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v2 6/8] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling
  2019-08-09 11:03 ` [PATCH v2 0/8] " Lorenzo Pieralisi
                     ` (4 preceding siblings ...)
  2019-08-09 11:03   ` [PATCH v2 5/8] ARM: psci: cpuidle: Enable " Lorenzo Pieralisi
@ 2019-08-09 11:03   ` Lorenzo Pieralisi
  2019-08-09 11:03   ` [PATCH v2 7/8] arm64: defconfig: Enable the PSCI CPUidle driver Lorenzo Pieralisi
  2019-08-09 11:03   ` [PATCH v2 8/8] ARM: imx_v6_v7_defconfig: " Lorenzo Pieralisi
  7 siblings, 0 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-08-09 11:03 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Daniel Lezcano, Ulf Hansson, Sudeep Holla,
	Will Deacon, Catalin Marinas, Mark Rutland, Rafael J. Wysocki,
	Shawn Guo, LKML, LAKML

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

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

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

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

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 arch/arm64/kernel/cpuidle.c    |  49 +++++++++-
 drivers/cpuidle/cpuidle-psci.c |  87 +++++++++++++++++-
 drivers/firmware/psci/psci.c   | 157 ++-------------------------------
 include/linux/cpuidle.h        |  17 +++-
 include/linux/psci.h           |   4 +-
 5 files changed, 154 insertions(+), 160 deletions(-)

diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index 619e0ebb8399..e4d6af2fdec7 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -47,17 +47,58 @@ int arm_cpuidle_suspend(int index)
 
 #define ARM64_LPI_IS_RETENTION_STATE(arch_flags) (!(arch_flags))
 
+static int psci_acpi_cpu_init_idle(unsigned int cpu)
+{
+	int i, count;
+	struct acpi_lpi_state *lpi;
+	struct acpi_processor *pr = per_cpu(processors, cpu);
+
+	/*
+	 * If the PSCI cpu_suspend function hook has not been initialized
+	 * idle states must not be enabled, so bail out
+	 */
+	if (!psci_ops.cpu_suspend)
+		return -EOPNOTSUPP;
+
+	if (unlikely(!pr || !pr->flags.has_lpi))
+		return -EINVAL;
+
+	count = pr->power.count - 1;
+	if (count <= 0)
+		return -ENODEV;
+
+	for (i = 0; i < count; i++) {
+		u32 state;
+
+		lpi = &pr->power.lpi_states[i + 1];
+		/*
+		 * Only bits[31:0] represent a PSCI power_state while
+		 * bits[63:32] must be 0x0 as per ARM ACPI FFH Specification
+		 */
+		state = lpi->address;
+		if (!psci_power_state_is_valid(state)) {
+			pr_warn("Invalid PSCI power state %#x\n", state);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 int acpi_processor_ffh_lpi_probe(unsigned int cpu)
 {
-	return psci_cpu_init_idle(cpu);
+	return psci_acpi_cpu_init_idle(cpu);
 }
 
 int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
 {
+	u32 state = lpi->address;
+
 	if (ARM64_LPI_IS_RETENTION_STATE(lpi->arch_flags))
-		return CPU_PM_CPU_IDLE_ENTER_RETENTION(psci_cpu_suspend_enter,
-						lpi->index);
+		return CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(psci_cpu_suspend_enter,
+						lpi->index, state);
 	else
-		return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, lpi->index);
+		return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
+					     lpi->index, state);
 }
 #endif
diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index ab1dea918ea3..f3c1a2396f98 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -22,10 +22,15 @@
 
 #include "dt_idle_states.h"
 
+static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
+
 static int psci_enter_idle_state(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int idx)
 {
-	return CPU_PM_CPU_IDLE_ENTER(psci_cpu_suspend_enter, idx);
+	u32 *state = __this_cpu_read(psci_power_state);
+
+	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
+					   idx, state[idx - 1]);
 }
 
 static struct cpuidle_driver psci_idle_driver __initdata = {
@@ -51,6 +56,86 @@ static const struct of_device_id psci_idle_state_match[] __initconst = {
 	{ },
 };
 
+static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
+{
+	int err = of_property_read_u32(np, "arm,psci-suspend-param", state);
+
+	if (err) {
+		pr_warn("%pOF missing arm,psci-suspend-param property\n", np);
+		return err;
+	}
+
+	if (!psci_power_state_is_valid(*state)) {
+		pr_warn("Invalid PSCI power state %#x\n", *state);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
+{
+	int i, ret = 0, count = 0;
+	u32 *psci_states;
+	struct device_node *state_node;
+
+	/* Count idle states */
+	while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
+					      count))) {
+		count++;
+		of_node_put(state_node);
+	}
+
+	if (!count)
+		return -ENODEV;
+
+	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
+	if (!psci_states)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		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);
+
+		if (ret)
+			goto free_mem;
+
+		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;
+
+free_mem:
+	kfree(psci_states);
+	return ret;
+}
+
+static __init int psci_cpu_init_idle(unsigned int cpu)
+{
+	struct device_node *cpu_node;
+	int ret;
+
+	/*
+	 * If the PSCI cpu_suspend function hook has not been initialized
+	 * idle states must not be enabled, so bail out
+	 */
+	if (!psci_ops.cpu_suspend)
+		return -EOPNOTSUPP;
+
+	cpu_node = of_cpu_device_node_get(cpu);
+	if (!cpu_node)
+		return -ENODEV;
+
+	ret = psci_dt_cpu_init_idle(cpu_node, cpu);
+
+	of_node_put(cpu_node);
+
+	return ret;
+}
+
 static int __init psci_idle_init_cpu(int cpu)
 {
 	struct cpuidle_driver *drv;
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index b343f8a34c6a..84f4ff351c62 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -103,7 +103,7 @@ static inline bool psci_power_state_loses_context(u32 state)
 	return state & mask;
 }
 
-static inline bool psci_power_state_is_valid(u32 state)
+bool psci_power_state_is_valid(u32 state)
 {
 	const u32 valid_mask = psci_has_ext_power_state() ?
 			       PSCI_1_0_EXT_POWER_STATE_MASK :
@@ -277,162 +277,21 @@ 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 = 0, count = 0;
-	u32 *psci_states;
-	struct device_node *state_node;
-
-	/* Count idle states */
-	while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
-					      count))) {
-		count++;
-		of_node_put(state_node);
-	}
-
-	if (!count)
-		return -ENODEV;
-
-	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
-	if (!psci_states)
-		return -ENOMEM;
-
-	for (i = 0; i < count; i++) {
-		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);
-
-		if (ret)
-			goto free_mem;
-
-		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;
-
-free_mem:
-	kfree(psci_states);
-	return ret;
-}
-
-#ifdef CONFIG_ACPI
-#include <acpi/processor.h>
-
-static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
-{
-	int i, count;
-	u32 *psci_states;
-	struct acpi_lpi_state *lpi;
-	struct acpi_processor *pr = per_cpu(processors, cpu);
-
-	if (unlikely(!pr || !pr->flags.has_lpi))
-		return -EINVAL;
-
-	count = pr->power.count - 1;
-	if (count <= 0)
-		return -ENODEV;
-
-	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
-	if (!psci_states)
-		return -ENOMEM;
-
-	for (i = 0; i < count; i++) {
-		u32 state;
-
-		lpi = &pr->power.lpi_states[i + 1];
-		/*
-		 * Only bits[31:0] represent a PSCI power_state while
-		 * bits[63:32] must be 0x0 as per ARM ACPI FFH Specification
-		 */
-		state = lpi->address;
-		if (!psci_power_state_is_valid(state)) {
-			pr_warn("Invalid PSCI power state %#x\n", state);
-			kfree(psci_states);
-			return -EINVAL;
-		}
-		psci_states[i] = state;
-	}
-	/* Idle states parsed correctly, initialize per-cpu pointer */
-	per_cpu(psci_power_state, cpu) = psci_states;
-	return 0;
-}
-#else
-static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
-{
-	return -EINVAL;
-}
-#endif
-
-int psci_cpu_init_idle(unsigned int cpu)
-{
-	struct device_node *cpu_node;
-	int ret;
-
-	/*
-	 * If the PSCI cpu_suspend function hook has not been initialized
-	 * idle states must not be enabled, so bail out
-	 */
-	if (!psci_ops.cpu_suspend)
-		return -EOPNOTSUPP;
-
-	if (!acpi_disabled)
-		return psci_acpi_cpu_init_idle(cpu);
-
-	cpu_node = of_get_cpu_node(cpu, NULL);
-	if (!cpu_node)
-		return -ENODEV;
-
-	ret = psci_dt_cpu_init_idle(cpu_node, cpu);
-
-	of_node_put(cpu_node);
-
-	return ret;
-}
-
-static int psci_suspend_finisher(unsigned long index)
+static int psci_suspend_finisher(unsigned long state)
 {
-	u32 *state = __this_cpu_read(psci_power_state);
+	u32 power_state = state;
 
-	return psci_ops.cpu_suspend(state[index - 1],
-				    __pa_symbol(cpu_resume));
+	return psci_ops.cpu_suspend(power_state, __pa_symbol(cpu_resume));
 }
 
-int psci_cpu_suspend_enter(unsigned long index)
+int psci_cpu_suspend_enter(u32 state)
 {
 	int ret;
-	u32 *state = __this_cpu_read(psci_power_state);
-	/*
-	 * idle state index 0 corresponds to wfi, should never be called
-	 * from the cpu_suspend operations
-	 */
-	if (WARN_ON_ONCE(!index))
-		return -EINVAL;
 
-	if (!psci_power_state_loses_context(state[index - 1]))
-		ret = psci_ops.cpu_suspend(state[index - 1], 0);
+	if (!psci_power_state_loses_context(state))
+		ret = psci_ops.cpu_suspend(state, 0);
 	else
-		ret = cpu_suspend(index, psci_suspend_finisher);
+		ret = cpu_suspend(state, psci_suspend_finisher);
 
 	return ret;
 }
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index bb9a0db89f1a..12ae4b87494e 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -256,7 +256,10 @@ static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 {return 0;}
 #endif
 
-#define __CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, is_retention) \
+#define __CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter,			\
+				idx,					\
+				state,					\
+				is_retention)				\
 ({									\
 	int __ret = 0;							\
 									\
@@ -268,7 +271,7 @@ static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 	if (!is_retention)						\
 		__ret =  cpu_pm_enter();				\
 	if (!__ret) {							\
-		__ret = low_level_idle_enter(idx);			\
+		__ret = low_level_idle_enter(state);			\
 		if (!is_retention)					\
 			cpu_pm_exit();					\
 	}								\
@@ -277,9 +280,15 @@ static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 })
 
 #define CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx)	\
-	__CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, 0)
+	__CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, idx, 0)
 
 #define CPU_PM_CPU_IDLE_ENTER_RETENTION(low_level_idle_enter, idx)	\
-	__CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, 1)
+	__CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, idx, 1)
+
+#define CPU_PM_CPU_IDLE_ENTER_PARAM(low_level_idle_enter, idx, state)	\
+	__CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, state, 0)
+
+#define CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(low_level_idle_enter, idx, state)	\
+	__CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter, idx, state, 1)
 
 #endif /* _LINUX_CPUIDLE_H */
diff --git a/include/linux/psci.h b/include/linux/psci.h
index a8a15613c157..e2bacc6fd2f2 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -15,8 +15,8 @@
 
 bool psci_tos_resident_on(int cpu);
 
-int psci_cpu_init_idle(unsigned int cpu);
-int psci_cpu_suspend_enter(unsigned long index);
+int psci_cpu_suspend_enter(u32 state);
+bool psci_power_state_is_valid(u32 state);
 
 enum psci_conduit {
 	PSCI_CONDUIT_NONE,
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v2 7/8] arm64: defconfig: Enable the PSCI CPUidle driver
  2019-08-09 11:03 ` [PATCH v2 0/8] " Lorenzo Pieralisi
                     ` (5 preceding siblings ...)
  2019-08-09 11:03   ` [PATCH v2 6/8] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling Lorenzo Pieralisi
@ 2019-08-09 11:03   ` Lorenzo Pieralisi
  2019-08-09 16:53     ` Will Deacon
  2019-08-09 11:03   ` [PATCH v2 8/8] ARM: imx_v6_v7_defconfig: " Lorenzo Pieralisi
  7 siblings, 1 reply; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-08-09 11:03 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Will Deacon, Sudeep Holla, Catalin Marinas,
	Ulf Hansson, Daniel Lezcano, Mark Rutland, Rafael J. Wysocki,
	Shawn Guo, LKML, LAKML

Enable the PSCI CPUidle driver to replace the functionality
previously provided by the generic ARM CPUidle driver through
CPU operations.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 0e58ef02880c..c0a7cfe3aebd 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -72,6 +72,7 @@ CONFIG_RANDOMIZE_BASE=y
 CONFIG_HIBERNATION=y
 CONFIG_WQ_POWER_EFFICIENT_DEFAULT=y
 CONFIG_ARM_CPUIDLE=y
+CONFIG_ARM_PSCI_CPUIDLE=y
 CONFIG_CPU_FREQ=y
 CONFIG_CPU_FREQ_STAT=y
 CONFIG_CPU_FREQ_GOV_POWERSAVE=m
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v2 8/8] ARM: imx_v6_v7_defconfig: Enable the PSCI CPUidle driver
  2019-08-09 11:03 ` [PATCH v2 0/8] " Lorenzo Pieralisi
                     ` (6 preceding siblings ...)
  2019-08-09 11:03   ` [PATCH v2 7/8] arm64: defconfig: Enable the PSCI CPUidle driver Lorenzo Pieralisi
@ 2019-08-09 11:03   ` Lorenzo Pieralisi
  7 siblings, 0 replies; 37+ messages in thread
From: Lorenzo Pieralisi @ 2019-08-09 11:03 UTC (permalink / raw)
  To: linux-pm
  Cc: Lorenzo Pieralisi, Shawn Guo, Catalin Marinas, Will Deacon,
	Ulf Hansson, Sudeep Holla, Daniel Lezcano, Mark Rutland,
	Rafael J. Wysocki, LKML, LAKML

Enable the PSCI CPUidle driver to replace the functionality
previously provided by the generic ARM CPUidle driver.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Shawn Guo <shawnguo@kernel.org>
---
 arch/arm/configs/imx_v6_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index a53b29251ed4..4174fd1b79e7 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -60,6 +60,7 @@ CONFIG_ARM_IMX6Q_CPUFREQ=y
 CONFIG_ARM_IMX_CPUFREQ_DT=y
 CONFIG_CPU_IDLE=y
 CONFIG_ARM_CPUIDLE=y
+CONFIG_ARM_PSCI_CPUIDLE=y
 CONFIG_VFP=y
 CONFIG_NEON=y
 CONFIG_PM_DEBUG=y
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* Re: [PATCH v2 7/8] arm64: defconfig: Enable the PSCI CPUidle driver
  2019-08-09 11:03   ` [PATCH v2 7/8] arm64: defconfig: Enable the PSCI CPUidle driver Lorenzo Pieralisi
@ 2019-08-09 16:53     ` Will Deacon
  0 siblings, 0 replies; 37+ messages in thread
From: Will Deacon @ 2019-08-09 16:53 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pm, Sudeep Holla, Catalin Marinas, Ulf Hansson,
	Daniel Lezcano, Mark Rutland, Rafael J. Wysocki, Shawn Guo, LKML,
	LAKML

On Fri, Aug 09, 2019 at 12:03:13PM +0100, Lorenzo Pieralisi wrote:
> Enable the PSCI CPUidle driver to replace the functionality
> previously provided by the generic ARM CPUidle driver through
> CPU operations.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/arm64/configs/defconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 0e58ef02880c..c0a7cfe3aebd 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -72,6 +72,7 @@ CONFIG_RANDOMIZE_BASE=y
>  CONFIG_HIBERNATION=y
>  CONFIG_WQ_POWER_EFFICIENT_DEFAULT=y
>  CONFIG_ARM_CPUIDLE=y
> +CONFIG_ARM_PSCI_CPUIDLE=y
>  CONFIG_CPU_FREQ=y
>  CONFIG_CPU_FREQ_STAT=y
>  CONFIG_CPU_FREQ_GOV_POWERSAVE=m

I'll queue the first 6 patches in this series, but please route this one
via arm-soc to avoid conflicts:

Acked-by: Will Deacon <will@kernel.org>

Failing that, I'm happy to take it at -rc1.

Will

^ permalink raw reply	[flat|nested] 37+ messages in thread

end of thread, other threads:[~2019-08-09 16:53 UTC | newest]

Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-22 15:37 [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework Lorenzo Pieralisi
2019-07-22 15:37 ` [PATCH 1/6] ARM: cpuidle: Remove useless header include Lorenzo Pieralisi
2019-08-06 15:51   ` Sudeep Holla
2019-08-07  8:19   ` Daniel Lezcano
2019-07-22 15:37 ` [PATCH 2/6] ARM: cpuidle: Remove overzealous error logging Lorenzo Pieralisi
2019-08-06 15:51   ` Sudeep Holla
2019-08-07  8:20   ` Daniel Lezcano
2019-07-22 15:37 ` [PATCH 3/6] drivers: firmware: psci: Decouple checker from generic ARM CPUidle Lorenzo Pieralisi
2019-08-06 15:54   ` Sudeep Holla
2019-08-07 14:09   ` Daniel Lezcano
2019-07-22 15:37 ` [PATCH 4/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver Lorenzo Pieralisi
2019-07-23 11:46   ` Ulf Hansson
2019-07-23 14:15     ` Lorenzo Pieralisi
2019-08-06 16:10   ` Sudeep Holla
2019-08-06 16:34     ` Lorenzo Pieralisi
2019-08-07 16:30   ` Daniel Lezcano
2019-07-22 15:37 ` [PATCH 5/6] ARM: psci: cpuidle: Enable " Lorenzo Pieralisi
2019-08-06 16:16   ` Sudeep Holla
2019-08-06 16:40     ` Lorenzo Pieralisi
2019-07-22 15:37 ` [PATCH 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling Lorenzo Pieralisi
2019-07-23 11:47   ` Ulf Hansson
2019-08-07 18:09   ` Daniel Lezcano
2019-08-08 12:55   ` Sudeep Holla
2019-08-08 15:29     ` Ulf Hansson
2019-08-08 16:04       ` Sudeep Holla
2019-07-23 11:49 ` [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework Ulf Hansson
2019-07-23 14:19   ` Lorenzo Pieralisi
2019-08-09 11:03 ` [PATCH v2 0/8] " Lorenzo Pieralisi
2019-08-09 11:03   ` [PATCH v2 1/8] ARM: cpuidle: Remove useless header include Lorenzo Pieralisi
2019-08-09 11:03   ` [PATCH v2 2/8] ARM: cpuidle: Remove overzealous error logging Lorenzo Pieralisi
2019-08-09 11:03   ` [PATCH v2 3/8] drivers: firmware: psci: Decouple checker from generic ARM CPUidle Lorenzo Pieralisi
2019-08-09 11:03   ` [PATCH v2 4/8] ARM: psci: cpuidle: Introduce PSCI CPUidle driver Lorenzo Pieralisi
2019-08-09 11:03   ` [PATCH v2 5/8] ARM: psci: cpuidle: Enable " Lorenzo Pieralisi
2019-08-09 11:03   ` [PATCH v2 6/8] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling Lorenzo Pieralisi
2019-08-09 11:03   ` [PATCH v2 7/8] arm64: defconfig: Enable the PSCI CPUidle driver Lorenzo Pieralisi
2019-08-09 16:53     ` Will Deacon
2019-08-09 11:03   ` [PATCH v2 8/8] ARM: imx_v6_v7_defconfig: " Lorenzo Pieralisi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).