[PATCH RFC v3 0/6] ARM generic idle states

[PATCH RFC v3 0/6] ARM generic idle states

[Lorenzo Pieralisi]

This patch is v3 of a previous posting:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/248084.html

Changes in v3:

- Streamlined the idle states bindings and added them to the series
  http://www.spinics.net/lists/arm-kernel/msg316299.html
- Sorting states through min-residency+exit-latency
- Added debug strings formatting
- Reworded min-residency-us idle state property
- Removed power-domain properties from idle states waiting for code
  examples requiring them to be defined

Changes in v2:

- Moved OF parsing code to drivers/cpuidle
- Improved states detection and sorting through linked list
- Split code in generic and ARM64 specific bits
- Moved idle enter function into ARM64 idle driver
- Refactored PSCI idle states register function
- Renamed suspend operations and moved detection to ARM64 idle driver
- Changed the way CPUIDLE_FLAG_TIMER_STOP is handled
- Simplified idle state nodes parsing since according to the latest
  bindings idle state nodes are a flat list, not hierarchical anymore
- Used min-residency-us to sort the states, to be further discussed

Idle states on most ARM platforms can be characterized by a set of
parameters that are platform agnostic and describe the HW idle states
features. So far, CPU idle drivers for ARM platforms required the definition
of parameters through static tables, duplicating control data for different
platforms. Moreover, the lack of standardization on firmware interfaces
hampered any standardization effort, resulting in CPU idle drivers for ARM
platforms containing duplicated code and platform specific power down routines.

The introduction of the PSCI firmware interface, and more in general, well
defined suspend back-ends, allows the definition of generic idle states and
the respective kernel infrastructure to support them.

Building on top of DT idle states bindings, that standardize idle states
parameters and corresponding suspend back-ends, this patchset provides code
that parses DT idle states nodes and builds at run-time the control data
infrastructure required by the ARM CPU idle drivers.

Idle states define an entry method (eg PSCI), that requires the respective
ARM64 kernel back-end to be invoked to initialize idle states parameters, so
that when the idle driver executes the back-end specific entry method a table
look-up can be carried out to retrieve the corresponding idle state parameter.

On legacy ARM platforms, the OF idle states are just used to initialize
states data.

The idle states bindings can be extended with new back-ends; the ARM64 CPUidle
driver must be updated accordingly so that the corresponding back
end initializer can be invoked at boot time for parameters initialization.

Patchset has been tested on AEM v8 models, on top of bootwrapper PSCI CPU
SUSPEND implementation which provides simulated core power gating.

Lorenzo Pieralisi (6):
  Documentation: arm: define DT idle states bindings
  Documentation: devicetree: psci: define CPU suspend parameter
  drivers: cpuidle: implement OF based idle states infrastructure
  arm64: add PSCI CPU_SUSPEND based cpu_suspend support
  drivers: cpuidle: CPU idle ARM64 driver
  arm64: boot: dts: update rtsm aemv8 dts with PSCI and idle states

 Documentation/devicetree/bindings/arm/cpus.txt     |   8 +
 .../devicetree/bindings/arm/idle-states.txt        | 508 +++++++++++++++++++++
 Documentation/devicetree/bindings/arm/psci.txt     |  11 +
 arch/arm64/boot/dts/rtsm_ve-aemv8a.dts             |  49 +-
 arch/arm64/include/asm/psci.h                      |   4 +
 arch/arm64/kernel/psci.c                           | 102 +++++
 drivers/cpuidle/Kconfig                            |  14 +
 drivers/cpuidle/Kconfig.arm64                      |  13 +
 drivers/cpuidle/Makefile                           |   5 +
 drivers/cpuidle/cpuidle-arm64.c                    | 161 +++++++
 drivers/cpuidle/of_idle_states.c                   | 293 ++++++++++++
 drivers/cpuidle/of_idle_states.h                   |   8 +
 12 files changed, 1168 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/idle-states.txt
 create mode 100644 drivers/cpuidle/Kconfig.arm64
 create mode 100644 drivers/cpuidle/cpuidle-arm64.c
 create mode 100644 drivers/cpuidle/of_idle_states.c
 create mode 100644 drivers/cpuidle/of_idle_states.h

-- 
1.8.4

[PATCH RFC v3 2/6] Documentation: devicetree: psci: define CPU suspend parameter

[Lorenzo Pieralisi]

OS layers built on top of PSCI to enter low-power states require the
power_state parameter to be passed to the PSCI CPU suspend method.

This parameter is specific to a power state and platform specific,
therefore must be provided by firmware to the OS in order to enable
proper call sequence.

This patch adds a property in the PSCI bindings that describes how
the CPU suspend power_state parameter should be defined in DT in
all device nodes that rely on PSCI CPU suspend method usage.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 Documentation/devicetree/bindings/arm/psci.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
index 433afe9..797c48f 100644
--- a/Documentation/devicetree/bindings/arm/psci.txt
+++ b/Documentation/devicetree/bindings/arm/psci.txt
@@ -42,6 +42,14 @@ Main node optional properties:
 
  - migrate       : Function ID for MIGRATE operation
 
+Device tree nodes that require usage of PSCI CPU_SUSPEND function (ie idle
+states bindings[1]) must specify the following properties:
+
+- entry-method-param
+		Usage: Required for idle states bindings [1].
+		Value type: <u32>
+		Definition: power_state parameter to pass to the PSCI
+			    suspend call.
 
 Example:
 
@@ -53,3 +61,6 @@ Example:
 		cpu_on		= <0x95c10002>;
 		migrate		= <0x95c10003>;
 	};
+
+[1] Kernel documentation - ARM idle states bindings
+    Documentation/devicetree/bindings/arm/idle-states.txt
-- 
1.8.4

Re: [PATCH RFC v3 2/6] Documentation: devicetree: psci: define CPU suspend parameter

[Sebastian Capella]

Reviewed-by: Sebastian Capella <sebastian.capella@linaro.org>

Thanks!

Sebastian

[PATCH RFC v3 3/6] drivers: cpuidle: implement OF based idle states infrastructure

[Lorenzo Pieralisi]

On most common ARM systems, the low-power states a CPU can be put into are
not discoverable in HW and require device tree bindings to describe
power down suspend operations and idle states parameters.

In order to enable DT based idle states and configure idle drivers, this
patch implements the bulk infrastructure required to parse the device tree
idle states bindings and initialize the corresponding CPUidle driver states
data.

Code that initializes idle states checks the CPU idle driver cpumask so
that multiple CPU idle drivers can be initialized through it in the
kernel. The CPU idle driver cpumask defines which idle states should be
considered valid for the driver, ie idle states that are valid on a set
of cpus the idle driver manages.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/cpuidle/Kconfig          |   9 ++
 drivers/cpuidle/Makefile         |   1 +
 drivers/cpuidle/of_idle_states.c | 293 +++++++++++++++++++++++++++++++++++++++
 drivers/cpuidle/of_idle_states.h |   8 ++
 4 files changed, 311 insertions(+)
 create mode 100644 drivers/cpuidle/of_idle_states.c
 create mode 100644 drivers/cpuidle/of_idle_states.h

diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index f04e25f..995654e 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -30,6 +30,15 @@ config CPU_IDLE_GOV_MENU
 	bool "Menu governor (for tickless system)"
 	default y
 
+config OF_IDLE_STATES
+        bool "Idle states DT support"
+	depends on ARM || ARM64
+	default n
+	help
+	 Allows the CPU idle framework to initialize CPU idle drivers
+	 state data by using DT provided nodes compliant with idle states
+	 device tree bindings.
+
 menu "ARM CPU Idle Drivers"
 depends on ARM
 source "drivers/cpuidle/Kconfig.arm"
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index f71ae1b..2fc5139 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -4,6 +4,7 @@
 
 obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
 obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
+obj-$(CONFIG_OF_IDLE_STATES)		  += of_idle_states.o
 
 ##################################################################################
 # ARM SoC drivers
diff --git a/drivers/cpuidle/of_idle_states.c b/drivers/cpuidle/of_idle_states.c
new file mode 100644
index 0000000..360b7ad
--- /dev/null
+++ b/drivers/cpuidle/of_idle_states.c
@@ -0,0 +1,293 @@
+/*
+ * OF idle states parsing code.
+ *
+ * Copyright (C) 2014 ARM Ltd.
+ * Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) "OF idle-states: " fmt
+
+#include <linux/cpuidle.h>
+#include <linux/cpumask.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/list_sort.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+
+#include "of_idle_states.h"
+
+struct state_elem {
+	struct list_head list;
+	struct device_node *node;
+	u32 val;
+};
+
+static struct list_head head __initdata = LIST_HEAD_INIT(head);
+
+static bool __init state_cpu_valid(struct device_node *state_node,
+				   struct device_node *cpu_node)
+{
+	int i = 0;
+	struct device_node *cpu_state;
+
+	while ((cpu_state = of_parse_phandle(cpu_node,
+					     "cpu-idle-states", i++))) {
+		if (cpu_state && state_node == cpu_state) {
+			of_node_put(cpu_state);
+			return true;
+		}
+		of_node_put(cpu_state);
+	}
+	return false;
+}
+
+static bool __init state_cpus_valid(const cpumask_t *cpus,
+				    struct device_node *state_node)
+{
+	int cpu;
+	struct device_node *cpu_node;
+
+	/*
+	 * Check if state is valid on driver cpumask cpus
+	 */
+	for_each_cpu(cpu, cpus) {
+		cpu_node = of_get_cpu_node(cpu, NULL);
+
+		if (!cpu_node) {
+			pr_err("Missing device node for CPU %d\n", cpu);
+			return false;
+		}
+
+		if (!state_cpu_valid(state_node, cpu_node))
+			return false;
+	}
+
+	return true;
+}
+
+static int __init state_cmp(void *priv, struct list_head *a,
+			    struct list_head *b)
+{
+	struct state_elem *ela, *elb;
+
+	ela = container_of(a, struct state_elem, list);
+	elb = container_of(b, struct state_elem, list);
+
+	return ela->val - elb->val;
+}
+
+static int __init add_state_node(cpumask_t *cpumask,
+				 struct device_node *state_node)
+{
+	struct state_elem *el;
+	u32 tmp, val = 0;
+
+	pr_debug(" * %s...\n", state_node->full_name);
+
+	if (!state_cpus_valid(cpumask, state_node))
+		return -EINVAL;
+	/*
+	 * Parse just the properties required to sort the states.
+	 * Since we are missing a value defining the energy
+	 * efficiency of a state, for now the sorting code uses
+	 *
+	 * min-residency-us+exit-latency-us
+	 *
+	 * as sorting rank.
+	 */
+	if (of_property_read_u32(state_node, "min-residency-us",
+				 &tmp)) {
+		pr_debug(" * %s missing min-residency-us property\n",
+			 state_node->full_name);
+		return -EINVAL;
+	}
+
+	val += tmp;
+
+	if (of_property_read_u32(state_node, "exit-latency-us",
+				 &tmp)) {
+		pr_debug(" * %s missing exit-latency-us property\n",
+			     state_node->full_name);
+		return -EINVAL;
+	}
+
+	val += tmp;
+
+	el = kmalloc(sizeof(*el), GFP_KERNEL);
+	if (!el) {
+		pr_err("%s failed to allocate memory\n", __func__);
+		return -ENOMEM;
+	}
+
+	el->node = state_node;
+	el->val = val;
+	list_add_tail(&el->list, &head);
+
+	return 0;
+}
+
+static void __init init_state_node(struct cpuidle_driver *drv,
+				   struct device_node *state_node,
+				   int *cnt)
+{
+	struct cpuidle_state *idle_state;
+
+	pr_debug(" * %s...\n", state_node->full_name);
+
+	idle_state = &drv->states[*cnt];
+
+	if (of_property_read_u32(state_node, "exit-latency-us",
+				 &idle_state->exit_latency)) {
+		pr_debug(" * %s missing exit-latency-us property\n",
+			     state_node->full_name);
+		return;
+	}
+
+	if (of_property_read_u32(state_node, "min-residency-us",
+				 &idle_state->target_residency)) {
+		pr_debug(" * %s missing min-residency-us property\n",
+			     state_node->full_name);
+		return;
+	}
+	/*
+	 * It is unknown to the idle driver if and when the tick_device
+	 * loses context when the CPU enters the idle states. To solve
+	 * this issue the tick device must be linked to a power domain
+	 * so that the idle driver can check on which states the device
+	 * loses its context. Current code takes the conservative choice
+	 * of defining the idle state as one where the tick device always
+	 * loses its context. On platforms where tick device never loses
+	 * its context (ie it is not a C3STOP device) this turns into
+	 * a nop. On platforms where the tick device does lose context in some
+	 * states, this code can be optimized, when power domain specifications
+	 * for ARM CPUs are finalized.
+	 */
+	idle_state->flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TIMER_STOP;
+
+	strncpy(idle_state->name, state_node->name, CPUIDLE_NAME_LEN);
+	strncpy(idle_state->desc, state_node->name, CPUIDLE_NAME_LEN);
+
+	(*cnt)++;
+}
+
+static int __init init_idle_states(struct cpuidle_driver *drv,
+				   struct device_node *state_nodes[],
+				   unsigned int start_idx, bool init_nodes)
+{
+	struct state_elem *el;
+	struct list_head *curr, *tmp;
+	unsigned int cnt = start_idx;
+
+	list_for_each_entry(el, &head, list) {
+		/*
+		 * Check if the init function has to fill the
+		 * state_nodes array on behalf of the CPUidle driver.
+		 */
+		if (init_nodes)
+			state_nodes[cnt] = el->node;
+		/*
+		 * cnt is updated on return if a state was added.
+		 */
+		init_state_node(drv, el->node, &cnt);
+
+		if (cnt == CPUIDLE_STATE_MAX) {
+			pr_warn("State index reached static CPU idle state limit\n");
+			break;
+		}
+	}
+
+	drv->state_count = cnt;
+
+	list_for_each_safe(curr, tmp, &head) {
+		list_del(curr);
+		kfree(container_of(curr, struct state_elem, list));
+	}
+
+	/*
+	 * If no idle states are detected, return an error and let the idle
+	 * driver initialization fail accordingly.
+	 */
+	return (cnt > start_idx) ? 0 : -ENODATA;
+}
+
+static void __init add_idle_states(struct cpuidle_driver *drv,
+				   struct device_node *idle_states)
+{
+	struct device_node *state_node;
+
+	for_each_child_of_node(idle_states, state_node) {
+		if ((!of_device_is_compatible(state_node, "arm,idle-state"))) {
+			pr_warn(" * %s: children of /cpus/idle-states must be \"arm,idle-state\" compatible\n",
+				     state_node->full_name);
+			continue;
+		}
+		/*
+		 * If memory allocation fails, better bail out.
+		 * Initialized nodes are freed at initialization
+		 * completion in of_init_idle_driver().
+		 */
+		if ((add_state_node(drv->cpumask, state_node) == -ENOMEM))
+			break;
+	}
+	/*
+	 * Sort the states list before initializing the CPUidle driver
+	 * states array.
+	 */
+	list_sort(NULL, &head, state_cmp);
+}
+
+/*
+ * of_init_idle_driver - Parse the DT idle states and initialize the
+ *			 idle driver states array
+ *
+ * @drv:	  Pointer to CPU idle driver to be initialized
+ * @state_nodes:  Array of struct device_nodes to be initialized if
+ *		  init_nodes == true. Must be sized CPUIDLE_STATE_MAX
+ * @start_idx:    First idle state index to be initialized
+ * @init_nodes:   Boolean to request device nodes initialization
+ *
+ * Returns:
+ *	0 on success
+ *	<0 on failure
+ *
+ *	On success the states array in the cpuidle driver contains
+ *	initialized entries in the states array, starting from index start_idx.
+ *	If init_nodes == true, on success the state_nodes array is initialized
+ *	with idle state DT node pointers, starting from index start_idx,
+ *	in a 1:1 relation with the idle driver states array.
+ */
+int __init of_init_idle_driver(struct cpuidle_driver *drv,
+			       struct device_node *state_nodes[],
+			       unsigned int start_idx, bool init_nodes)
+{
+	struct device_node *idle_states_node;
+	int ret;
+
+	if (start_idx >= CPUIDLE_STATE_MAX) {
+		pr_warn("State index exceeds static CPU idle driver states array size\n");
+		return -EINVAL;
+	}
+
+	if (WARN(init_nodes && !state_nodes,
+		"Requested nodes stashing in an invalid nodes container\n"))
+		return -EINVAL;
+
+	idle_states_node = of_find_node_by_path("/cpus/idle-states");
+	if (!idle_states_node)
+		return -ENOENT;
+
+	add_idle_states(drv, idle_states_node);
+
+	ret = init_idle_states(drv, state_nodes, start_idx, init_nodes);
+
+	of_node_put(idle_states_node);
+
+	return ret;
+}
diff --git a/drivers/cpuidle/of_idle_states.h b/drivers/cpuidle/of_idle_states.h
new file mode 100644
index 0000000..049f94f
--- /dev/null
+++ b/drivers/cpuidle/of_idle_states.h
@@ -0,0 +1,8 @@
+#ifndef __OF_IDLE_STATES
+#define __OF_IDLE_STATES
+
+int __init of_init_idle_driver(struct cpuidle_driver *drv,
+			       struct device_node *state_nodes[],
+			       unsigned int start_idx,
+			       bool init_nodes);
+#endif
-- 
1.8.4

Re: [PATCH RFC v3 3/6] drivers: cpuidle: implement OF based idle states infrastructure

[Sebastian Capella]

Quoting Lorenzo Pieralisi (2014-05-06 11:04:40)

> diff --git a/drivers/cpuidle/of_idle_states.c b/drivers/cpuidle/of_idle_states.c
> new file mode 100644
> index 0000000..360b7ad
> --- /dev/null
> +++ b/drivers/cpuidle/of_idle_states.c
> @@ -0,0 +1,293 @@
...
> +static int __init add_state_node(cpumask_t *cpumask,
> +                                struct device_node *state_node)
> +{
> +       struct state_elem *el;
> +       u32 tmp, val = 0;
> +
> +       pr_debug(" * %s...\n", state_node->full_name);
> +
> +       if (!state_cpus_valid(cpumask, state_node))
> +               return -EINVAL;
> +       /*
> +        * Parse just the properties required to sort the states.
> +        * Since we are missing a value defining the energy
> +        * efficiency of a state, for now the sorting code uses
> +        *
> +        * min-residency-us+exit-latency-us
> +        *
> +        * as sorting rank.
> +        */
> +       if (of_property_read_u32(state_node, "min-residency-us",
> +                                &tmp)) {
> +               pr_debug(" * %s missing min-residency-us property\n",
> +                        state_node->full_name);
> +               return -EINVAL;
> +       }
> +
> +       val += tmp;
> +
> +       if (of_property_read_u32(state_node, "exit-latency-us",
> +                                &tmp)) {
> +               pr_debug(" * %s missing exit-latency-us property\n",
> +                            state_node->full_name);
> +               return -EINVAL;
> +       }
> +
> +       val += tmp;

Sorry if i'm rehashing old stuff, but I prefer not to use the
min-residency + exit-latency to sort.  I saw Rob's comment suggesting it
and your reply.  I'm not sure when it was decided.

Would it be possible to sort instead based on the order in the
cpus->cpu-idle-states?  If not, my preference would be to either use
index like you had before, or specify another sort order / rank value.

I think there's potential for us to create lower power states that
have lower min-residencies (reduced power consumption in the state,
allowing us to more quickly recover the higher entrance cost)
with higher exit latencies in such a way that the formula would not
sort as we expect.  Having a separate value would allow us to control
the sorting in those cases.

> +
> +/*

For kernel-doc, I think you need a /** here, and a () after the
of_init_idle_driver below.  Also maybe Return: instead of Returns:
and I think the return paragraph goes at the end, but am not positive.

kernel-doc-nano-HOWTO.txt

> + * of_init_idle_driver - Parse the DT idle states and initialize the
> + *                      idle driver states array
> + *
> + * @drv:         Pointer to CPU idle driver to be initialized
> + * @state_nodes:  Array of struct device_nodes to be initialized if
> + *               init_nodes == true. Must be sized CPUIDLE_STATE_MAX
> + * @start_idx:    First idle state index to be initialized
> + * @init_nodes:   Boolean to request device nodes initialization
> + *
> + * Returns:
> + *     0 on success
> + *     <0 on failure
> + *
> + *     On success the states array in the cpuidle driver contains
> + *     initialized entries in the states array, starting from index start_idx.
> + *     If init_nodes == true, on success the state_nodes array is initialized
> + *     with idle state DT node pointers, starting from index start_idx,
> + *     in a 1:1 relation with the idle driver states array.
> + */
> +int __init of_init_idle_driver(struct cpuidle_driver *drv,
> +                              struct device_node *state_nodes[],
> +                              unsigned int start_idx, bool init_nodes)
> +{

Thanks!

Sebastian

Re: [PATCH RFC v3 3/6] drivers: cpuidle: implement OF based idle states infrastructure

[Lorenzo Pieralisi]

On Fri, May 09, 2014 at 12:12:19AM +0100, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-05-06 11:04:40)
> 
> > diff --git a/drivers/cpuidle/of_idle_states.c b/drivers/cpuidle/of_idle_states.c
> > new file mode 100644
> > index 0000000..360b7ad
> > --- /dev/null
> > +++ b/drivers/cpuidle/of_idle_states.c
> > @@ -0,0 +1,293 @@
> ...
> > +static int __init add_state_node(cpumask_t *cpumask,
> > +                                struct device_node *state_node)
> > +{
> > +       struct state_elem *el;
> > +       u32 tmp, val = 0;
> > +
> > +       pr_debug(" * %s...\n", state_node->full_name);
> > +
> > +       if (!state_cpus_valid(cpumask, state_node))
> > +               return -EINVAL;
> > +       /*
> > +        * Parse just the properties required to sort the states.
> > +        * Since we are missing a value defining the energy
> > +        * efficiency of a state, for now the sorting code uses
> > +        *
> > +        * min-residency-us+exit-latency-us
> > +        *
> > +        * as sorting rank.
> > +        */
> > +       if (of_property_read_u32(state_node, "min-residency-us",
> > +                                &tmp)) {
> > +               pr_debug(" * %s missing min-residency-us property\n",
> > +                        state_node->full_name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       val += tmp;
> > +
> > +       if (of_property_read_u32(state_node, "exit-latency-us",
> > +                                &tmp)) {
> > +               pr_debug(" * %s missing exit-latency-us property\n",
> > +                            state_node->full_name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       val += tmp;
> 
> Sorry if i'm rehashing old stuff, but I prefer not to use the
> min-residency + exit-latency to sort.  I saw Rob's comment suggesting it
> and your reply.  I'm not sure when it was decided.
> 
> Would it be possible to sort instead based on the order in the
> cpus->cpu-idle-states?  If not, my preference would be to either use
> index like you had before, or specify another sort order / rank value.

"power-rank" property ? We can't rely on the DT state nodes ordering.

I am ok with adding a power-rank property, as long as it is not frowned
upon by DT maintainers, I am running short of ideas for states sorting if
I can't use idle state properties to pull that off.

> I think there's potential for us to create lower power states that
> have lower min-residencies (reduced power consumption in the state,
> allowing us to more quickly recover the higher entrance cost)
> with higher exit latencies in such a way that the formula would not
> sort as we expect.  Having a separate value would allow us to control
> the sorting in those cases.
> 
> > +
> > +/*
> 
> For kernel-doc, I think you need a /** here, and a () after the
> of_init_idle_driver below.  Also maybe Return: instead of Returns:
> and I think the return paragraph goes at the end, but am not positive.
> 
> kernel-doc-nano-HOWTO.txt

Ok changes done.

Thanks,
Lorenzo

> > + * of_init_idle_driver - Parse the DT idle states and initialize the
> > + *                      idle driver states array
> > + *
> > + * @drv:         Pointer to CPU idle driver to be initialized
> > + * @state_nodes:  Array of struct device_nodes to be initialized if
> > + *               init_nodes == true. Must be sized CPUIDLE_STATE_MAX
> > + * @start_idx:    First idle state index to be initialized
> > + * @init_nodes:   Boolean to request device nodes initialization
> > + *
> > + * Returns:
> > + *     0 on success
> > + *     <0 on failure
> > + *
> > + *     On success the states array in the cpuidle driver contains
> > + *     initialized entries in the states array, starting from index start_idx.
> > + *     If init_nodes == true, on success the state_nodes array is initialized
> > + *     with idle state DT node pointers, starting from index start_idx,
> > + *     in a 1:1 relation with the idle driver states array.
> > + */
> > +int __init of_init_idle_driver(struct cpuidle_driver *drv,
> > +                              struct device_node *state_nodes[],
> > +                              unsigned int start_idx, bool init_nodes)
> > +{
> 
> Thanks!
> 
> Sebastian
> 

Re: [PATCH RFC v3 3/6] drivers: cpuidle: implement OF based idle states infrastructure

[Lorenzo Pieralisi]

On Fri, May 09, 2014 at 12:12:19AM +0100, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-05-06 11:04:40)
> 
> > diff --git a/drivers/cpuidle/of_idle_states.c b/drivers/cpuidle/of_idle_states.c
> > new file mode 100644
> > index 0000000..360b7ad
> > --- /dev/null
> > +++ b/drivers/cpuidle/of_idle_states.c
> > @@ -0,0 +1,293 @@
> ...
> > +static int __init add_state_node(cpumask_t *cpumask,
> > +                                struct device_node *state_node)
> > +{
> > +       struct state_elem *el;
> > +       u32 tmp, val = 0;
> > +
> > +       pr_debug(" * %s...\n", state_node->full_name);
> > +
> > +       if (!state_cpus_valid(cpumask, state_node))
> > +               return -EINVAL;
> > +       /*
> > +        * Parse just the properties required to sort the states.
> > +        * Since we are missing a value defining the energy
> > +        * efficiency of a state, for now the sorting code uses
> > +        *
> > +        * min-residency-us+exit-latency-us
> > +        *
> > +        * as sorting rank.
> > +        */
> > +       if (of_property_read_u32(state_node, "min-residency-us",
> > +                                &tmp)) {
> > +               pr_debug(" * %s missing min-residency-us property\n",
> > +                        state_node->full_name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       val += tmp;
> > +
> > +       if (of_property_read_u32(state_node, "exit-latency-us",
> > +                                &tmp)) {
> > +               pr_debug(" * %s missing exit-latency-us property\n",
> > +                            state_node->full_name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       val += tmp;
> 
> Sorry if i'm rehashing old stuff, but I prefer not to use the
> min-residency + exit-latency to sort.  I saw Rob's comment suggesting it
> and your reply.  I'm not sure when it was decided.
> 
> Would it be possible to sort instead based on the order in the
> cpus->cpu-idle-states?  If not, my preference would be to either use
> index like you had before, or specify another sort order / rank value.
> 
> I think there's potential for us to create lower power states that
> have lower min-residencies (reduced power consumption in the state,
> allowing us to more quickly recover the higher entrance cost)
> with higher exit latencies in such a way that the formula would not
> sort as we expect.  Having a separate value would allow us to control
> the sorting in those cases.

Ok, so adding to my previous comment, would exit_latency by itself be
enough ? Can we think of a system where ranking by exit_latency is wrong ?

If yes, we need a power rank, and this patch is wrong too:

http://marc.info/?l=linux-kernel&m=139924292401056&w=2

If answer is not, I can just rely on exit_latency to sort the states.

Lorenzo

[PATCH RFC v3 1/6] Documentation: arm: define DT idle states bindings

[Lorenzo Pieralisi]

ARM based platforms implement a variety of power management schemes that
allow processors to enter idle states at run-time.
The parameters defining these idle states vary on a per-platform basis forcing
the OS to hardcode the state parameters in platform specific static tables
whose size grows as the number of platforms supported in the kernel increases
and hampers device drivers standardization.

Therefore, this patch aims at standardizing idle state device tree bindings for
ARM platforms. Bindings define idle state parameters inclusive of entry methods
and state latencies, to allow operating systems to retrieve the configuration
entries from the device tree and initialize the related power management
drivers, paving the way for common code in the kernel to deal with idle
states and removing the need for static data in current and previous kernel
versions.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/cpus.txt     |   8 +
 .../devicetree/bindings/arm/idle-states.txt        | 508 +++++++++++++++++++++
 2 files changed, 516 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/idle-states.txt

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index 333f4ae..d19e8ae 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -209,6 +209,12 @@ nodes to be present and contain the properties described below.
 		Value type: <phandle>
 		Definition: Specifies the ACC[2] node associated with this CPU.
 
+	- cpu-idle-states
+		Usage: Optional
+		Value type: <prop-encoded-array>
+		Definition:
+			# List of phandles to idle state nodes supported
+			  by this cpu [3].
 
 Example 1 (dual-cluster big.LITTLE system 32-bit):
 
@@ -405,3 +411,5 @@ cpus {
 --
 [1] arm/msm/qcom,saw2.txt
 [2] arm/msm/qcom,kpss-acc.txt
+[3] ARM Linux kernel documentation - idle states bindings
+    Documentation/devicetree/bindings/arm/idle-states.txt
diff --git a/Documentation/devicetree/bindings/arm/idle-states.txt b/Documentation/devicetree/bindings/arm/idle-states.txt
new file mode 100644
index 0000000..196ef52
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/idle-states.txt
@@ -0,0 +1,508 @@
+==========================================
+ARM idle states binding description
+==========================================
+
+==========================================
+1 - Introduction
+==========================================
+
+ARM systems contain HW capable of managing power consumption dynamically,
+where cores can be put in different low-power states (ranging from simple
+wfi to power gating) according to OSPM policies. The CPU states representing
+the range of dynamic idle states that a processor can enter at run-time, can be
+specified through device tree bindings representing the parameters required
+to enter/exit specific idle states on a given processor.
+
+According to the Server Base System Architecture document (SBSA, [3]), the
+power states an ARM CPU can be put into are identified by the following list:
+
+- Running
+- Idle_standby
+- Idle_retention
+- Sleep
+- Off
+
+The power states described in the SBSA document define the basic CPU states on
+top of which ARM platforms implement power management schemes that allow an OS
+PM implementation to put the processor in different idle states (which include
+states listed above; "off" state is not an idle state since it does not have
+wake-up capabilities, hence it is not considered in this document).
+
+Idle state parameters (eg entry latency) are platform specific and need to be
+characterized with bindings that provide the required information to OSPM
+code so that it can build the required tables and use them at runtime.
+
+The device tree binding definition for ARM idle states is the subject of this
+document.
+
+===========================================
+2 - idle-states node
+===========================================
+
+ARM processor idle states are defined within the idle-states node, which is
+a direct child of the cpus node [1] and provides a container where the
+processor idle states, defined as device tree nodes, are listed.
+
+- idle-states node
+
+	Usage: Optional - On ARM systems, is a container of processor idle
+			  states nodes. If the system does not provide CPU
+			  power management capabilities or the processor just
+			  supports idle_standby an idle-states node is not
+			  required.
+
+	Description: idle-states node is a container node, where its
+		     subnodes describe the CPU idle states.
+
+	Node name must be "idle-states".
+
+	The idle-states node's parent node must be the cpus node.
+
+	The idle-states node's child nodes can be:
+
+	- one or more state nodes
+
+	Any other configuration is considered invalid.
+
+	An idle-states node defines the following properties:
+
+	- entry-method
+		Usage: Required
+		Value type: <stringlist>
+		Definition: Describes the method by which a CPU enters the
+			    idle states. This property is required and must be
+			    one of:
+
+			    - "arm,psci"
+			      ARM PSCI firmware interface [2].
+
+			    - "[vendor],[method]"
+			      An implementation dependent string with
+			      format "vendor,method", where vendor is a string
+			      denoting the name of the manufacturer and
+			      method is a string specifying the mechanism
+			      used to enter the idle state.
+
+The nodes describing the idle states (state) can only be defined within the
+idle-states node.
+
+Any other configuration is consider invalid and therefore must be ignored.
+
+===========================================
+3 - state node
+===========================================
+
+A state node represents an idle state description and must be defined as
+follows:
+
+- state node
+
+	Description: must be child of the idle-states node
+
+	The state node name shall follow standard device tree naming
+	rules ([5], 2.2.1 "Node names"), in particular state nodes which
+	are siblings within a single common parent must be given a unique name.
+
+	The idle state entered by executing the wfi instruction (idle_standby
+	SBSA,[3][4]) is considered standard on all ARM platforms and therefore
+	must not be listed.
+
+	A state node defines the following properties:
+
+	- compatible
+		Usage: Required
+		Value type: <stringlist>
+		Definition: Must be "arm,idle-state".
+
+	- logic-state-retained
+		Usage: See definition
+		Value type: <none>
+		Definition: if present logic is retained on state entry,
+			    otherwise it is lost.
+
+	- cache-state-retained
+		Usage: See definition
+		Value type: <none>
+		Definition: if present cache memory is retained on state entry,
+			    otherwise it is lost.
+
+	- entry-method-param
+		Usage: See definition.
+		Value type: <u32>
+		Definition: Depends on the idle-states node entry-method
+			    property value. Refer to the entry-method bindings
+			    for this property value definition.
+
+	- entry-latency-us
+		Usage: Required
+		Value type: <prop-encoded-array>
+		Definition: u32 value representing worst case latency
+			    in microseconds required to enter the idle state.
+
+	- exit-latency-us
+		Usage: Required
+		Value type: <prop-encoded-array>
+		Definition: u32 value representing worst case latency
+			    in microseconds required to exit the idle state.
+
+	- min-residency-us
+		Usage: Required
+		Value type: <prop-encoded-array>
+		Definition: u32 value representing duration in microseconds
+			    after which this state becomes more energy
+			    efficient than any shallower states.
+
+===========================================
+4 - Examples
+===========================================
+
+Example 1 (ARM 64-bit, 16-cpu system):
+
+cpus {
+	#size-cells = <0>;
+	#address-cells = <2>;
+
+	idle-states {
+		entry-method = "arm,psci";
+
+		CPU_RETENTION_0_0: cpu-retention-0-0 {
+			compatible = "arm,idle-state";
+			cache-state-retained;
+			entry-method-param = <0x0010000>;
+			entry-latency-us = <20>;
+			exit-latency-us = <40>;
+			min-residency-us = <30>;
+		};
+
+		CLUSTER_RETENTION_0: cluster-retention-0 {
+			compatible = "arm,idle-state";
+			logic-state-retained;
+			cache-state-retained;
+			entry-method-param = <0x1010000>;
+			entry-latency-us = <50>;
+			exit-latency-us = <100>;
+			min-residency-us = <250>;
+		};
+
+		CPU_SLEEP_0_0: cpu-sleep-0-0 {
+			compatible = "arm,idle-state";
+			entry-method-param = <0x0010000>;
+			entry-latency-us = <250>;
+			exit-latency-us = <500>;
+			min-residency-us = <350>;
+		};
+
+		CLUSTER_SLEEP_0: cluster-sleep-0 {
+			compatible = "arm,idle-state";
+			entry-method-param = <0x1010000>;
+			entry-latency-us = <600>;
+			exit-latency-us = <1100>;
+			min-residency-us = <2700>;
+		};
+
+		CPU_RETENTION_1_0: cpu-retention-1-0 {
+			compatible = "arm,idle-state";
+			cache-state-retained;
+			entry-method-param = <0x0010000>;
+			entry-latency-us = <20>;
+			exit-latency-us = <40>;
+			min-residency-us = <30>;
+		};
+
+		CLUSTER_RETENTION_1: cluster-retention-1 {
+			compatible = "arm,idle-state";
+			logic-state-retained;
+			cache-state-retained;
+			entry-method-param = <0x1010000>;
+			entry-latency-us = <50>;
+			exit-latency-us = <100>;
+			min-residency-us = <270>;
+		};
+
+		CPU_SLEEP_1_0: cpu-sleep-1-0 {
+			compatible = "arm,idle-state";
+			entry-method-param = <0x0010000>;
+			entry-latency-us = <70>;
+			exit-latency-us = <100>;
+			min-residency-us = <100>;
+		};
+
+		CLUSTER_SLEEP_1: cluster-sleep-1 {
+			compatible = "arm,idle-state";
+			entry-method-param = <0x1010000>;
+			entry-latency-us = <500>;
+			exit-latency-us = <1200>;
+			min-residency-us = <3500>;
+		};
+	};
+
+	CPU0: cpu@0 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x0>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_0_0 &CPU_SLEEP_0_0
+				   &CLUSTER_RETENTION_0 &CLUSTER_SLEEP_0>;
+	};
+
+	CPU1: cpu@1 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x1>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_0_0 &CPU_SLEEP_0_0
+				   &CLUSTER_RETENTION_0 &CLUSTER_SLEEP_0>;
+	};
+
+	CPU2: cpu@100 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x100>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_0_0 &CPU_SLEEP_0_0
+				   &CLUSTER_RETENTION_0 &CLUSTER_SLEEP_0>;
+	};
+
+	CPU3: cpu@101 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x101>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_0_0 &CPU_SLEEP_0_0
+				   &CLUSTER_RETENTION_0 &CLUSTER_SLEEP_0>;
+	};
+
+	CPU4: cpu@10000 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x10000>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_0_0 &CPU_SLEEP_0_0
+				   &CLUSTER_RETENTION_0 &CLUSTER_SLEEP_0>;
+	};
+
+	CPU5: cpu@10001 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x10001>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_0_0 &CPU_SLEEP_0_0
+				   &CLUSTER_RETENTION_0 &CLUSTER_SLEEP_0>;
+	};
+
+	CPU6: cpu@10100 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x10100>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_0_0 &CPU_SLEEP_0_0
+				   &CLUSTER_RETENTION_0 &CLUSTER_SLEEP_0>;
+	};
+
+	CPU7: cpu@10101 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x10101>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_0_0 &CPU_SLEEP_0_0
+				   &CLUSTER_RETENTION_0 &CLUSTER_SLEEP_0>;
+	};
+
+	CPU8: cpu@100000000 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x0>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_1_0 &CPU_SLEEP_1_0
+				   &CLUSTER_RETENTION_1 &CLUSTER_SLEEP_1>;
+	};
+
+	CPU9: cpu@100000001 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x1>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_1_0 &CPU_SLEEP_1_0
+				   &CLUSTER_RETENTION_1 &CLUSTER_SLEEP_1>;
+	};
+
+	CPU10: cpu@100000100 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x100>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_1_0 &CPU_SLEEP_1_0
+				   &CLUSTER_RETENTION_1 &CLUSTER_SLEEP_1>;
+	};
+
+	CPU11: cpu@100000101 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x101>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_1_0 &CPU_SLEEP_1_0
+				   &CLUSTER_RETENTION_1 &CLUSTER_SLEEP_1>;
+	};
+
+	CPU12: cpu@100010000 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x10000>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_1_0 &CPU_SLEEP_1_0
+				   &CLUSTER_RETENTION_1 &CLUSTER_SLEEP_1>;
+	};
+
+	CPU13: cpu@100010001 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x10001>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_1_0 &CPU_SLEEP_1_0
+				   &CLUSTER_RETENTION_1 &CLUSTER_SLEEP_1>;
+	};
+
+	CPU14: cpu@100010100 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x10100>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_1_0 &CPU_SLEEP_1_0
+				   &CLUSTER_RETENTION_1 &CLUSTER_SLEEP_1>;
+	};
+
+	CPU15: cpu@100010101 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x10101>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_RETENTION_1_0 &CPU_SLEEP_1_0
+				   &CLUSTER_RETENTION_1 &CLUSTER_SLEEP_1>;
+	};
+};
+
+Example 2 (ARM 32-bit, 8-cpu system, two clusters):
+
+cpus {
+	#size-cells = <0>;
+	#address-cells = <1>;
+
+	idle-states {
+		entry-method = "arm,psci";
+
+		CPU_SLEEP_0_0: cpu-sleep-0-0 {
+			compatible = "arm,idle-state";
+			entry-method-param = <0x0010000>;
+			entry-latency-us = <400>;
+			exit-latency-us = <500>;
+			min-residency-us = <300>;
+		};
+
+		CLUSTER_SLEEP_0: cluster-sleep-0 {
+			compatible = "arm,idle-state";
+			entry-method-param = <0x1010000>;
+			entry-latency-us = <1000>;
+			exit-latency-us = <1500>;
+			min-residency-us = <1500>;
+		};
+
+		CPU_SLEEP_1_0: cpu-sleep-1-0 {
+			compatible = "arm,idle-state";
+			entry-method-param = <0x0010000>;
+			entry-latency-us = <300>;
+			exit-latency-us = <500>;
+			min-residency-us = <500>;
+		};
+
+		CLUSTER_SLEEP_1: cluster-sleep-1 {
+			compatible = "arm,idle-state";
+			entry-method-param = <0x1010000>;
+			entry-latency-us = <800>;
+			exit-latency-us = <2000>;
+			min-residency-us = <6500>;
+		};
+	};
+
+	CPU0: cpu@0 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a15";
+		reg = <0x0>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_SLEEP_0_0 &CLUSTER_SLEEP_0>;
+	};
+
+	CPU1: cpu@1 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a15";
+		reg = <0x1>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_SLEEP_0_0 &CLUSTER_SLEEP_0>;
+	};
+
+	CPU2: cpu@2 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a15";
+		reg = <0x2>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_SLEEP_0_0 &CLUSTER_SLEEP_0>;
+	};
+
+	CPU3: cpu@3 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a15";
+		reg = <0x3>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_SLEEP_0_0 &CLUSTER_SLEEP_0>;
+	};
+
+	CPU4: cpu@100 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a7";
+		reg = <0x100>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_SLEEP_1_0 &CLUSTER_SLEEP_1>;
+	};
+
+	CPU5: cpu@101 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a7";
+		reg = <0x101>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_SLEEP_1_0 &CLUSTER_SLEEP_1>;
+	};
+
+	CPU6: cpu@102 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a7";
+		reg = <0x102>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_SLEEP_1_0 &CLUSTER_SLEEP_1>;
+	};
+
+	CPU7: cpu@103 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a7";
+		reg = <0x103>;
+		enable-method = "psci";
+		cpu-idle-states = <&CPU_SLEEP_1_0 &CLUSTER_SLEEP_1>;
+	};
+};
+
+===========================================
+4 - References
+===========================================
+
+[1] ARM Linux Kernel documentation - CPUs bindings
+    Documentation/devicetree/bindings/arm/cpus.txt
+
+[2] ARM Linux Kernel documentation - PSCI bindings
+    Documentation/devicetree/bindings/arm/psci.txt
+
+[3] ARM Server Base System Architecture (SBSA)
+    http://infocenter.arm.com/help/index.jsp
+
+[4] ARM Architecture Reference Manuals
+    http://infocenter.arm.com/help/index.jsp
+
+[5] ePAPR standard
+    https://www.power.org/documentation/epapr-version-1-1/
-- 
1.8.4


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v3 1/6] Documentation: arm: define DT idle states bindings

[Sebastian Capella]

Quoting Lorenzo Pieralisi (2014-05-06 11:04:38)
> diff --git a/Documentation/devicetree/bindings/arm/idle-states.txt b/Documentation/devicetree/bindings/arm/idle-states.txt
> new file mode 100644
> index 0000000..196ef52
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/idle-states.txt
> @@ -0,0 +1,508 @@
...
> +===========================================
> +2 - idle-states node
> +===========================================
> +
> +ARM processor idle states are defined within the idle-states node, which is
> +a direct child of the cpus node [1] and provides a container where the
> +processor idle states, defined as device tree nodes, are listed.
> +
> +- idle-states node
> +
> +       Usage: Optional - On ARM systems, is a container of processor idle
> +                         states nodes. If the system does not provide CPU
> +                         power management capabilities or the processor just
> +                         supports idle_standby an idle-states node is not
> +                         required.
> +
> +       Description: idle-states node is a container node, where its
> +                    subnodes describe the CPU idle states.
> +
> +       Node name must be "idle-states".
> +
> +       The idle-states node's parent node must be the cpus node.
> +
> +       The idle-states node's child nodes can be:
> +
> +       - one or more state nodes
> +
> +       Any other configuration is considered invalid.
> +
> +       An idle-states node defines the following properties:
> +
> +       - entry-method
> +               Usage: Required
> +               Value type: <stringlist>
> +               Definition: Describes the method by which a CPU enters the
> +                           idle states. This property is required and must be
> +                           one of:
> +
> +                           - "arm,psci"
> +                             ARM PSCI firmware interface [2].
> +
> +                           - "[vendor],[method]"
> +                             An implementation dependent string with
> +                             format "vendor,method", where vendor is a string
> +                             denoting the name of the manufacturer and
> +                             method is a string specifying the mechanism
> +                             used to enter the idle state.
> +
> +The nodes describing the idle states (state) can only be defined within the
> +idle-states node.
> +
> +Any other configuration is consider invalid and therefore must be ignored.

consider -> considered
must be -> shall?

Is the reference to "any other configuration" referring to state nodes
not in the idle states node?  If so, maybe this sentence should go
together with that one.

> +===========================================
> +4 - Examples
> +===========================================
> +
> +Example 1 (ARM 64-bit, 16-cpu system):
> +
> +cpus {
> +       #size-cells = <0>;
> +       #address-cells = <2>;
> +
> +       idle-states {
> +               entry-method = "arm,psci";
> +
> +               CPU_RETENTION_0_0: cpu-retention-0-0 {
> +                       compatible = "arm,idle-state";
> +                       cache-state-retained;
> +                       entry-method-param = <0x0010000>;
> +                       entry-latency-us = <20>;
> +                       exit-latency-us = <40>;
> +                       min-residency-us = <30>;
> +               };
> +
> +               CLUSTER_RETENTION_0: cluster-retention-0 {
> +                       compatible = "arm,idle-state";
> +                       logic-state-retained;
> +                       cache-state-retained;
> +                       entry-method-param = <0x1010000>;
> +                       entry-latency-us = <50>;
> +                       exit-latency-us = <100>;
> +                       min-residency-us = <250>;
> +               };
                  ...
> +       };
> +
> +       CPU0: cpu@0 {
> +               device_type = "cpu";
> +               compatible = "arm,cortex-a57";
> +               reg = <0x0 0x0>;
> +               enable-method = "psci";
> +               cpu-idle-states = <&CPU_RETENTION_0_0 &CPU_SLEEP_0_0
> +                                  &CLUSTER_RETENTION_0 &CLUSTER_SLEEP_0>;
> +       };
> +
> +       CPU1: cpu@1 {
> +               device_type = "cpu";
> +               compatible = "arm,cortex-a57";
> +               reg = <0x0 0x1>;
> +               enable-method = "psci";
> +               cpu-idle-states = <&CPU_RETENTION_0_0 &CPU_SLEEP_0_0
> +                                  &CLUSTER_RETENTION_0 &CLUSTER_SLEEP_0>;
> +       };

How can we specify what states are entered together vs. those that
are independent (since they also share the entry-method-param)?
CPU_SLEEP_0_0 is used for both CPU0 and CPU1, yet each cpu can enter
it without waiting for the other.  Whereas CLUSTER_RETENTION_0 should
not be entered until all CPUs sharing it enter.

Is the idea to define separate CPU_SLEEP/CPU_RETENTION nodes for each cpu?

Thanks!

Sebastian

Re: [PATCH RFC v3 1/6] Documentation: arm: define DT idle states bindings

[Lorenzo Pieralisi]

On Thu, May 08, 2014 at 12:43:04AM +0100, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-05-06 11:04:38)
> > diff --git a/Documentation/devicetree/bindings/arm/idle-states.txt b/Documentation/devicetree/bindings/arm/idle-states.txt
> > new file mode 100644
> > index 0000000..196ef52
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/idle-states.txt
> > @@ -0,0 +1,508 @@
> ...
> > +===========================================
> > +2 - idle-states node
> > +===========================================
> > +
> > +ARM processor idle states are defined within the idle-states node, which is
> > +a direct child of the cpus node [1] and provides a container where the
> > +processor idle states, defined as device tree nodes, are listed.
> > +
> > +- idle-states node
> > +
> > +       Usage: Optional - On ARM systems, is a container of processor idle
> > +                         states nodes. If the system does not provide CPU
> > +                         power management capabilities or the processor just
> > +                         supports idle_standby an idle-states node is not
> > +                         required.
> > +
> > +       Description: idle-states node is a container node, where its
> > +                    subnodes describe the CPU idle states.
> > +
> > +       Node name must be "idle-states".
> > +
> > +       The idle-states node's parent node must be the cpus node.
> > +
> > +       The idle-states node's child nodes can be:
> > +
> > +       - one or more state nodes
> > +
> > +       Any other configuration is considered invalid.
> > +
> > +       An idle-states node defines the following properties:
> > +
> > +       - entry-method
> > +               Usage: Required
> > +               Value type: <stringlist>
> > +               Definition: Describes the method by which a CPU enters the
> > +                           idle states. This property is required and must be
> > +                           one of:
> > +
> > +                           - "arm,psci"
> > +                             ARM PSCI firmware interface [2].
> > +
> > +                           - "[vendor],[method]"
> > +                             An implementation dependent string with
> > +                             format "vendor,method", where vendor is a string
> > +                             denoting the name of the manufacturer and
> > +                             method is a string specifying the mechanism
> > +                             used to enter the idle state.
> > +
> > +The nodes describing the idle states (state) can only be defined within the
> > +idle-states node.
> > +
> > +Any other configuration is consider invalid and therefore must be ignored.
> 
> consider -> considered
> must be -> shall?
> 
> Is the reference to "any other configuration" referring to state nodes
> not in the idle states node?  If so, maybe this sentence should go
> together with that one.

Yes, it makes sense.

> > +===========================================
> > +4 - Examples
> > +===========================================
> > +
> > +Example 1 (ARM 64-bit, 16-cpu system):
> > +
> > +cpus {
> > +       #size-cells = <0>;
> > +       #address-cells = <2>;
> > +
> > +       idle-states {
> > +               entry-method = "arm,psci";
> > +
> > +               CPU_RETENTION_0_0: cpu-retention-0-0 {
> > +                       compatible = "arm,idle-state";
> > +                       cache-state-retained;
> > +                       entry-method-param = <0x0010000>;
> > +                       entry-latency-us = <20>;
> > +                       exit-latency-us = <40>;
> > +                       min-residency-us = <30>;
> > +               };
> > +
> > +               CLUSTER_RETENTION_0: cluster-retention-0 {
> > +                       compatible = "arm,idle-state";
> > +                       logic-state-retained;
> > +                       cache-state-retained;
> > +                       entry-method-param = <0x1010000>;
> > +                       entry-latency-us = <50>;
> > +                       exit-latency-us = <100>;
> > +                       min-residency-us = <250>;
> > +               };
>                   ...
> > +       };
> > +
> > +       CPU0: cpu@0 {
> > +               device_type = "cpu";
> > +               compatible = "arm,cortex-a57";
> > +               reg = <0x0 0x0>;
> > +               enable-method = "psci";
> > +               cpu-idle-states = <&CPU_RETENTION_0_0 &CPU_SLEEP_0_0
> > +                                  &CLUSTER_RETENTION_0 &CLUSTER_SLEEP_0>;
> > +       };
> > +
> > +       CPU1: cpu@1 {
> > +               device_type = "cpu";
> > +               compatible = "arm,cortex-a57";
> > +               reg = <0x0 0x1>;
> > +               enable-method = "psci";
> > +               cpu-idle-states = <&CPU_RETENTION_0_0 &CPU_SLEEP_0_0
> > +                                  &CLUSTER_RETENTION_0 &CLUSTER_SLEEP_0>;
> > +       };
> 
> How can we specify what states are entered together vs. those that
> are independent (since they also share the entry-method-param)?
> CPU_SLEEP_0_0 is used for both CPU0 and CPU1, yet each cpu can enter
> it without waiting for the other.  Whereas CLUSTER_RETENTION_0 should
> not be entered until all CPUs sharing it enter.

We do not specify that. That can be solved with power-domains but I
removed them from the bindings since I want to merge bindings that are
required by current code, and PSCI does not need this information, since
the power state parameter to PSCI suspend call (and the related SMC
implementation) caters for it.

When I see implementations requiring these bits of info, we will add
them to the bindings.

> Is the idea to define separate CPU_SLEEP/CPU_RETENTION nodes for each cpu?

On big.LITTLE systems you might want to have different states (eg CPU
core gatng) for big and little CPUs, and that's doable with the current
bindings since each CPU can point at idle states in its cpu node.

Thanks for having a look,
Lorenzo

Re: [PATCH RFC v3 1/6] Documentation: arm: define DT idle states bindings

[Sebastian Capella]

Quoting Lorenzo Pieralisi (2014-05-08 01:57:43)
> On Thu, May 08, 2014 at 12:43:04AM +0100, Sebastian Capella wrote:
> > Quoting Lorenzo Pieralisi (2014-05-06 11:04:38)
> > > diff --git a/Documentation/devicetree/bindings/arm/idle-states.txt b/Documentation/devicetree/bindings/arm/idle-states.txt
> > > new file mode 100644
> > > index 0000000..196ef52
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/arm/idle-states.txt
> > > @@ -0,0 +1,508 @@
...
> > > +
> > > +The nodes describing the idle states (state) can only be defined within the
> > > +idle-states node.
> > > +
> > > +Any other configuration is consider invalid and therefore must be ignored.
> > 
> > consider -> considered
> > must be -> shall?
> > 
> > Is the reference to "any other configuration" referring to state nodes
> > not in the idle states node?  If so, maybe this sentence should go
> > together with that one.
> 
> Yes, it makes sense.
> 

With this small change you can add my Reviewed-by

Thanks!

Sebastian

[PATCH RFC v3 4/6] arm64: add PSCI CPU_SUSPEND based cpu_suspend support

[Lorenzo Pieralisi]

This patch implements the cpu_suspend cpu operations method through
the PSCI CPU_SUSPEND API. The PSCI implementation translates the idle state
index passed by the cpu_suspend core call into a valid PSCI state according to
the PSCI states initialized at boot by the PSCI suspend backend.

Entry point is set to cpu_resume physical address, that represents the
default kernel execution address following a CPU reset.

Idle state indices missing a DT node description are initialized to power
state standby WFI so that if called by the idle driver they provide the
default behaviour.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
---
 arch/arm64/include/asm/psci.h |   4 ++
 arch/arm64/kernel/psci.c      | 102 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+)

diff --git a/arch/arm64/include/asm/psci.h b/arch/arm64/include/asm/psci.h
index d15ab8b4..9a4b663 100644
--- a/arch/arm64/include/asm/psci.h
+++ b/arch/arm64/include/asm/psci.h
@@ -14,6 +14,10 @@
 #ifndef __ASM_PSCI_H
 #define __ASM_PSCI_H
 
+struct cpuidle_driver;
 void psci_init(void);
 
+int __init psci_dt_register_idle_states(struct cpuidle_driver *,
+					struct device_node *[]);
+
 #endif /* __ASM_PSCI_H */
diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
index ea4828a..0e32ab4 100644
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -15,15 +15,18 @@
 
 #define pr_fmt(fmt) "psci: " fmt
 
+#include <linux/cpuidle.h>
 #include <linux/init.h>
 #include <linux/of.h>
 #include <linux/smp.h>
+#include <linux/slab.h>
 
 #include <asm/compiler.h>
 #include <asm/cpu_ops.h>
 #include <asm/errno.h>
 #include <asm/psci.h>
 #include <asm/smp_plat.h>
+#include <asm/suspend.h>
 
 #define PSCI_POWER_STATE_TYPE_STANDBY		0
 #define PSCI_POWER_STATE_TYPE_POWER_DOWN	1
@@ -54,6 +57,8 @@ enum psci_function {
 	PSCI_FN_MAX,
 };
 
+static DEFINE_PER_CPU_READ_MOSTLY(struct psci_power_state *, psci_power_state);
+
 static u32 psci_function_id[PSCI_FN_MAX];
 
 #define PSCI_RET_SUCCESS		0
@@ -94,6 +99,17 @@ static u32 psci_power_state_pack(struct psci_power_state state)
 			<< PSCI_POWER_STATE_AFFL_SHIFT);
 }
 
+static void psci_power_state_unpack(u32 power_state,
+				    struct psci_power_state *state)
+{
+	state->id = (power_state >> PSCI_POWER_STATE_ID_SHIFT)
+			& PSCI_POWER_STATE_ID_MASK;
+	state->type = (power_state >> PSCI_POWER_STATE_TYPE_SHIFT)
+			& PSCI_POWER_STATE_TYPE_MASK;
+	state->affinity_level = (power_state >> PSCI_POWER_STATE_AFFL_SHIFT)
+			& PSCI_POWER_STATE_AFFL_MASK;
+}
+
 /*
  * The following two functions are invoked via the invoke_psci_fn pointer
  * and will not be inlined, allowing us to piggyback on the AAPCS.
@@ -176,6 +192,77 @@ static const struct of_device_id psci_of_match[] __initconst = {
 	{},
 };
 
+int __init psci_dt_register_idle_states(struct cpuidle_driver *drv,
+					struct device_node *state_nodes[])
+{
+	int cpu, i;
+	struct psci_power_state *psci_states;
+	const struct cpu_operations *cpu_ops_ptr;
+
+	if (!state_nodes)
+		return -EINVAL;
+	/*
+	 * This is belt-and-braces: make sure that if the idle
+	 * specified protocol is psci, the cpu_ops have been
+	 * initialized to psci operations. Anything else is
+	 * a recipe for mayhem.
+	 */
+	for_each_cpu(cpu, drv->cpumask) {
+		cpu_ops_ptr = cpu_ops[cpu];
+		if (WARN_ON(!cpu_ops_ptr || strcmp(cpu_ops_ptr->name, "psci")))
+			return -EOPNOTSUPP;
+	}
+
+	psci_states = kcalloc(drv->state_count, sizeof(*psci_states),
+			      GFP_KERNEL);
+
+	if (!psci_states) {
+		pr_warn("psci idle state allocation failed\n");
+		return -ENOMEM;
+	}
+
+	for_each_cpu(cpu, drv->cpumask) {
+		if (per_cpu(psci_power_state, cpu)) {
+			pr_warn("idle states already initialized on cpu %u\n",
+				cpu);
+			continue;
+		}
+		per_cpu(psci_power_state, cpu) = psci_states;
+	}
+
+
+	for (i = 0; i < drv->state_count; i++) {
+		u32 psci_power_state;
+
+		if (!state_nodes[i]) {
+			/*
+			 * An index with a missing node pointer falls back to
+			 * simple STANDBYWFI
+			 */
+			psci_states[i].type = PSCI_POWER_STATE_TYPE_STANDBY;
+			continue;
+		}
+
+		if (of_property_read_u32(state_nodes[i], "entry-method-param",
+					 &psci_power_state)) {
+			pr_warn(" * %s missing entry-method-param property\n",
+				state_nodes[i]->full_name);
+			/*
+			 * If entry-method-param property is missing, fall
+			 * back to STANDBYWFI state
+			 */
+			psci_states[i].type = PSCI_POWER_STATE_TYPE_STANDBY;
+			continue;
+		}
+
+		pr_debug("psci-power-state %#x index %u\n",
+			 psci_power_state, i);
+		psci_power_state_unpack(psci_power_state, &psci_states[i]);
+	}
+
+	return 0;
+}
+
 void __init psci_init(void)
 {
 	struct device_node *np;
@@ -279,6 +366,18 @@ static void cpu_psci_cpu_die(unsigned int cpu)
 }
 #endif
 
+#ifdef CONFIG_ARM64_CPU_SUSPEND
+static int cpu_psci_cpu_suspend(unsigned long index)
+{
+	struct psci_power_state *state = __get_cpu_var(psci_power_state);
+
+	if (!state)
+		return -EOPNOTSUPP;
+
+	return psci_ops.cpu_suspend(state[index], virt_to_phys(cpu_resume));
+}
+#endif
+
 const struct cpu_operations cpu_psci_ops = {
 	.name		= "psci",
 	.cpu_init	= cpu_psci_cpu_init,
@@ -288,6 +387,9 @@ const struct cpu_operations cpu_psci_ops = {
 	.cpu_disable	= cpu_psci_cpu_disable,
 	.cpu_die	= cpu_psci_cpu_die,
 #endif
+#ifdef CONFIG_ARM64_CPU_SUSPEND
+	.cpu_suspend	= cpu_psci_cpu_suspend,
+#endif
 };
 
 #endif
-- 
1.8.4


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v3 4/6] arm64: add PSCI CPU_SUSPEND based cpu_suspend support

[Sebastian Capella]

Quoting Lorenzo Pieralisi (2014-05-06 11:04:41)
> This patch implements the cpu_suspend cpu operations method through
> the PSCI CPU_SUSPEND API. The PSCI implementation translates the idle state
> index passed by the cpu_suspend core call into a valid PSCI state according to
> the PSCI states initialized at boot by the PSCI suspend backend.
> 
> Entry point is set to cpu_resume physical address, that represents the
> default kernel execution address following a CPU reset.
> 
> Idle state indices missing a DT node description are initialized to power
> state standby WFI so that if called by the idle driver they provide the
> default behaviour.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Reviewed-by: Sebastian Capella <sebastian.capella@linaro.org>

Thanks!

Sebastian

[PATCH RFC v3 5/6] drivers: cpuidle: CPU idle ARM64 driver

[Lorenzo Pieralisi]

This patch implements a generic CPU idle driver for ARM64 machines.

It relies on the DT idle states infrastructure to initialize idle
states count and respective parameters. Current code assumes the driver
is managing idle states on all possible CPUs but can be easily
generalized to support heterogenous systems and build cpumasks at
runtime using MIDRs or DT cpu nodes compatible properties.

Suspend back-ends (eg PSCI) must register a suspend initializer with
the CPU idle driver so that the suspend backend call can be detected,
and the driver code can call the back-end infrastructure to complete the
suspend backend initialization.

Idle state index 0 is always initialized as a simple wfi state, ie always
considered present and functional on all ARM64 platforms.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
---
 drivers/cpuidle/Kconfig         |   5 ++
 drivers/cpuidle/Kconfig.arm64   |  13 ++++
 drivers/cpuidle/Makefile        |   4 +
 drivers/cpuidle/cpuidle-arm64.c | 161 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 183 insertions(+)
 create mode 100644 drivers/cpuidle/Kconfig.arm64
 create mode 100644 drivers/cpuidle/cpuidle-arm64.c

diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index 995654e..0654eb5 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -44,6 +44,11 @@ depends on ARM
 source "drivers/cpuidle/Kconfig.arm"
 endmenu
 
+menu "ARM64 CPU Idle Drivers"
+depends on ARM64
+source "drivers/cpuidle/Kconfig.arm64"
+endmenu
+
 menu "POWERPC CPU Idle Drivers"
 depends on PPC
 source "drivers/cpuidle/Kconfig.powerpc"
diff --git a/drivers/cpuidle/Kconfig.arm64 b/drivers/cpuidle/Kconfig.arm64
new file mode 100644
index 0000000..b83612c
--- /dev/null
+++ b/drivers/cpuidle/Kconfig.arm64
@@ -0,0 +1,13 @@
+#
+# ARM64 CPU Idle drivers
+#
+
+config ARM64_CPUIDLE
+	bool "Generic ARM64 CPU idle Driver"
+	select OF_IDLE_STATES
+	help
+	  Select this to enable generic cpuidle driver for ARM v8.
+	  It provides a generic idle driver whose idle states are configured
+	  at run-time through DT nodes. The CPUidle suspend backend is
+	  initialized by the device tree parsing code on matching the entry
+	  method to the respective CPU operations.
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 2fc5139..161a5f2 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -16,6 +16,10 @@ obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
 obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
 
 ###############################################################################
+# ARM64 drivers
+obj-$(CONFIG_ARM64_CPUIDLE)		+= cpuidle-arm64.o
+
+###############################################################################
 # POWERPC drivers
 obj-$(CONFIG_PSERIES_CPUIDLE)		+= cpuidle-pseries.o
 obj-$(CONFIG_POWERNV_CPUIDLE)		+= cpuidle-powernv.o
diff --git a/drivers/cpuidle/cpuidle-arm64.c b/drivers/cpuidle/cpuidle-arm64.c
new file mode 100644
index 0000000..fef1fad
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-arm64.c
@@ -0,0 +1,161 @@
+/*
+ * ARM64 generic CPU idle driver.
+ *
+ * Copyright (C) 2014 ARM Ltd.
+ * Author: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) "CPUidle arm64: " 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 <asm/psci.h>
+#include <asm/suspend.h>
+
+#include "of_idle_states.h"
+
+typedef int (*suspend_init_fn)(struct cpuidle_driver *,
+			       struct device_node *[]);
+
+struct cpu_suspend_ops {
+	const char *id;
+	suspend_init_fn init_fn;
+};
+
+static const struct cpu_suspend_ops suspend_operations[] __initconst = {
+	{"arm,psci", psci_dt_register_idle_states},
+	{}
+};
+
+static __init const struct cpu_suspend_ops *get_suspend_ops(const char *str)
+{
+	int i;
+
+	if (!str)
+		return NULL;
+
+	for (i = 0; suspend_operations[i].id; i++)
+		if (!strcmp(suspend_operations[i].id, str))
+			return &suspend_operations[i];
+
+	return NULL;
+}
+
+/*
+ * arm_enter_idle_state - Programs CPU to enter the specified state
+ *
+ * @dev: cpuidle device
+ * @drv: cpuidle driver
+ * @idx: state index
+ *
+ * Called from the CPUidle framework to program the device to the
+ * specified target state selected by the governor.
+ */
+static int arm_enter_idle_state(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int idx)
+{
+	int ret;
+
+	if (!idx) {
+		cpu_do_idle();
+		return idx;
+	}
+
+	cpu_pm_enter();
+	/*
+	 * Pass idle state index to cpu_suspend which in turn will call
+	 * the CPU ops suspend protocol with idle index as a parameter.
+	 *
+	 * Some states would not require context to be saved and flushed
+	 * to DRAM, so calling cpu_suspend would not be stricly necessary.
+	 * When power domains specifications for ARM CPUs are finalized then
+	 * this code can be optimized to prevent saving registers if not
+	 * needed.
+	 */
+	ret = cpu_suspend(idx);
+
+	cpu_pm_exit();
+
+	return ret ? -1 : idx;
+}
+
+struct cpuidle_driver arm64_idle_driver = {
+	.name = "arm64_idle",
+	.owner = THIS_MODULE,
+};
+
+static struct device_node *state_nodes[CPUIDLE_STATE_MAX] __initdata;
+
+/*
+ * arm64_idle_init
+ *
+ * Registers the arm64 specific cpuidle driver with the cpuidle
+ * framework. It relies on core code to parse the idle states
+ * and initialize them using driver data structures accordingly.
+ */
+static int __init arm64_idle_init(void)
+{
+	int i, ret;
+	const char *entry_method;
+	struct device_node *idle_states_node;
+	const struct cpu_suspend_ops *suspend_init;
+	struct cpuidle_driver *drv = &arm64_idle_driver;
+
+	idle_states_node = of_find_node_by_path("/cpus/idle-states");
+	if (!idle_states_node)
+		return -ENOENT;
+
+	if (of_property_read_string(idle_states_node, "entry-method",
+				    &entry_method)) {
+		pr_warn(" * %s missing entry-method property\n",
+			    idle_states_node->full_name);
+		of_node_put(idle_states_node);
+		return -EOPNOTSUPP;
+	}
+
+	suspend_init = get_suspend_ops(entry_method);
+	if (!suspend_init) {
+		pr_warn("Missing suspend initializer\n");
+		of_node_put(idle_states_node);
+		return -EOPNOTSUPP;
+	}
+
+	/*
+	 * State at index 0 is standby wfi and considered standard
+	 * on all ARM platforms. If in some platforms simple wfi
+	 * can't be used as "state 0", DT bindings must be implemented
+	 * to work around this issue and allow installing a special
+	 * handler for idle state index 0.
+	 */
+	drv->states[0].exit_latency = 1;
+	drv->states[0].target_residency = 1;
+	drv->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
+	strncpy(drv->states[0].name, "ARM WFI", CPUIDLE_NAME_LEN);
+	strncpy(drv->states[0].desc, "ARM WFI", CPUIDLE_DESC_LEN);
+
+	drv->cpumask = (struct cpumask *) cpu_possible_mask;
+	/*
+	 * Start at index 1, request idle state nodes to be filled
+	 */
+	ret = of_init_idle_driver(drv, state_nodes, 1, true);
+	if (ret)
+		return ret;
+
+	if (suspend_init->init_fn(drv, state_nodes))
+		return -EOPNOTSUPP;
+
+	for (i = 0; i < drv->state_count; i++)
+		drv->states[i].enter = arm_enter_idle_state;
+
+	return cpuidle_register(drv, NULL);
+}
+device_initcall(arm64_idle_init);
-- 
1.8.4


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v3 5/6] drivers: cpuidle: CPU idle ARM64 driver

[Sebastian Capella]

Quoting Lorenzo Pieralisi (2014-05-06 11:04:42)
> diff --git a/drivers/cpuidle/cpuidle-arm64.c b/drivers/cpuidle/cpuidle-arm64.c
> new file mode 100644
> index 0000000..fef1fad
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-arm64.c
> @@ -0,0 +1,161 @@
...
> +/*

Is this intended to be kernel-docs?  If so, should maybe use the /** and
() after the function below.  Otherwise, should you remove the @ in the
argument list below?

> + * arm_enter_idle_state - Programs CPU to enter the specified state
> + *
> + * @dev: cpuidle device
> + * @drv: cpuidle driver
> + * @idx: state index
> + *
> + * Called from the CPUidle framework to program the device to the
> + * specified target state selected by the governor.
> + */
> +static int arm_enter_idle_state(struct cpuidle_device *dev,
> +                               struct cpuidle_driver *drv, int idx)

...

> +static int __init arm64_idle_init(void)
> +{
> +       int i, ret;
> +       const char *entry_method;
> +       struct device_node *idle_states_node;
> +       const struct cpu_suspend_ops *suspend_init;
> +       struct cpuidle_driver *drv = &arm64_idle_driver;
> +
> +       idle_states_node = of_find_node_by_path("/cpus/idle-states");
> +       if (!idle_states_node)
> +               return -ENOENT;
> +
> +       if (of_property_read_string(idle_states_node, "entry-method",
> +                                   &entry_method)) {
> +               pr_warn(" * %s missing entry-method property\n",
> +                           idle_states_node->full_name);
> +               of_node_put(idle_states_node);
> +               return -EOPNOTSUPP;
> +       }
> +
> +       suspend_init = get_suspend_ops(entry_method);
> +       if (!suspend_init) {
> +               pr_warn("Missing suspend initializer\n");
> +               of_node_put(idle_states_node);
> +               return -EOPNOTSUPP;
> +       }
> +
> +       /*
> +        * State at index 0 is standby wfi and considered standard
> +        * on all ARM platforms. If in some platforms simple wfi
> +        * can't be used as "state 0", DT bindings must be implemented
> +        * to work around this issue and allow installing a special
> +        * handler for idle state index 0.
> +        */
> +       drv->states[0].exit_latency = 1;
> +       drv->states[0].target_residency = 1;
> +       drv->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
> +       strncpy(drv->states[0].name, "ARM WFI", CPUIDLE_NAME_LEN);
> +       strncpy(drv->states[0].desc, "ARM WFI", CPUIDLE_DESC_LEN);
> +
> +       drv->cpumask = (struct cpumask *) cpu_possible_mask;
> +       /*
> +        * Start at index 1, request idle state nodes to be filled
> +        */
> +       ret = of_init_idle_driver(drv, state_nodes, 1, true);
> +       if (ret)
> +               return ret;
> +
> +       if (suspend_init->init_fn(drv, state_nodes))
> +               return -EOPNOTSUPP;
> +
> +       for (i = 0; i < drv->state_count; i++)
> +               drv->states[i].enter = arm_enter_idle_state;

Is this missing an of_node_put(idle_states_node)?

Thanks!

Sebastian

Re: [PATCH RFC v3 5/6] drivers: cpuidle: CPU idle ARM64 driver

[Lorenzo Pieralisi]

On Fri, May 09, 2014 at 01:48:38AM +0100, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-05-06 11:04:42)
> > diff --git a/drivers/cpuidle/cpuidle-arm64.c b/drivers/cpuidle/cpuidle-arm64.c
> > new file mode 100644
> > index 0000000..fef1fad
> > --- /dev/null
> > +++ b/drivers/cpuidle/cpuidle-arm64.c
> > @@ -0,0 +1,161 @@
> ...
> > +/*
> 
> Is this intended to be kernel-docs?  If so, should maybe use the /** and
> () after the function below.  Otherwise, should you remove the @ in the
> argument list below?

Changes done according to kernel-docs.

> > + * arm_enter_idle_state - Programs CPU to enter the specified state
> > + *
> > + * @dev: cpuidle device
> > + * @drv: cpuidle driver
> > + * @idx: state index
> > + *
> > + * Called from the CPUidle framework to program the device to the
> > + * specified target state selected by the governor.
> > + */
> > +static int arm_enter_idle_state(struct cpuidle_device *dev,
> > +                               struct cpuidle_driver *drv, int idx)
> 
> ...
> 
> > +static int __init arm64_idle_init(void)
> > +{
> > +       int i, ret;
> > +       const char *entry_method;
> > +       struct device_node *idle_states_node;
> > +       const struct cpu_suspend_ops *suspend_init;
> > +       struct cpuidle_driver *drv = &arm64_idle_driver;
> > +
> > +       idle_states_node = of_find_node_by_path("/cpus/idle-states");
> > +       if (!idle_states_node)
> > +               return -ENOENT;
> > +
> > +       if (of_property_read_string(idle_states_node, "entry-method",
> > +                                   &entry_method)) {
> > +               pr_warn(" * %s missing entry-method property\n",
> > +                           idle_states_node->full_name);
> > +               of_node_put(idle_states_node);
> > +               return -EOPNOTSUPP;
> > +       }
> > +
> > +       suspend_init = get_suspend_ops(entry_method);
> > +       if (!suspend_init) {
> > +               pr_warn("Missing suspend initializer\n");
> > +               of_node_put(idle_states_node);
> > +               return -EOPNOTSUPP;
> > +       }
> > +
> > +       /*
> > +        * State at index 0 is standby wfi and considered standard
> > +        * on all ARM platforms. If in some platforms simple wfi
> > +        * can't be used as "state 0", DT bindings must be implemented
> > +        * to work around this issue and allow installing a special
> > +        * handler for idle state index 0.
> > +        */
> > +       drv->states[0].exit_latency = 1;
> > +       drv->states[0].target_residency = 1;
> > +       drv->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
> > +       strncpy(drv->states[0].name, "ARM WFI", CPUIDLE_NAME_LEN);
> > +       strncpy(drv->states[0].desc, "ARM WFI", CPUIDLE_DESC_LEN);
> > +
> > +       drv->cpumask = (struct cpumask *) cpu_possible_mask;
> > +       /*
> > +        * Start at index 1, request idle state nodes to be filled
> > +        */
> > +       ret = of_init_idle_driver(drv, state_nodes, 1, true);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (suspend_init->init_fn(drv, state_nodes))
> > +               return -EOPNOTSUPP;
> > +
> > +       for (i = 0; i < drv->state_count; i++)
> > +               drv->states[i].enter = arm_enter_idle_state;
> 
> Is this missing an of_node_put(idle_states_node)?

Yes, good catch, patched.

Thanks !
Lorenzo

[PATCH RFC v3 6/6] arm64: boot: dts: update rtsm aemv8 dts with PSCI and idle states

[Lorenzo Pieralisi]

This patch updates the RTSM dts file with PSCI bindings and nodes
describing the AEMv8 model idle states parameters.

PSCI function IDs compliancy with PSCI v0.2 is still under development
so this patch provides PSCI function IDs for demonstration purposes only.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 arch/arm64/boot/dts/rtsm_ve-aemv8a.dts | 49 ++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/boot/dts/rtsm_ve-aemv8a.dts b/arch/arm64/boot/dts/rtsm_ve-aemv8a.dts
index d79de9c..fd23737 100644
--- a/arch/arm64/boot/dts/rtsm_ve-aemv8a.dts
+++ b/arch/arm64/boot/dts/rtsm_ve-aemv8a.dts
@@ -27,37 +27,70 @@
 		serial3 = &v2m_serial3;
 	};
 
+	psci {
+		compatible = "arm,psci";
+		method = "smc";
+		/*
+		 * Function IDs usage and compliancy with PSCI v0.2 still
+		 * under discussion.  Current IDs should be considered
+		 * temporary for demonstration purposes.
+		 */
+		cpu_suspend = <0x84000001>;
+		cpu_off = <0x84000002>;
+		cpu_on = <0x84000003>;
+	};
+
 	cpus {
 		#address-cells = <2>;
 		#size-cells = <0>;
 
+		idle-states {
+			entry-method = "arm,psci";
+
+			CPU_SLEEP_0: cpu-sleep-0 {
+				compatible = "arm,idle-state";
+				entry-method-param = <0x0010000>;
+				entry-latency-us = <40>;
+				exit-latency-us = <100>;
+				min-residency-us = <150>;
+			};
+
+			CLUSTER_SLEEP_0: cluster-sleep-0 {
+				compatible = "arm,idle-state";
+				entry-method-param = <0x1010000>;
+				entry-latency-us = <500>;
+				exit-latency-us = <1000>;
+				min-residency-us = <2500>;
+			};
+		};
+
 		cpu@0 {
 			device_type = "cpu";
 			compatible = "arm,armv8";
 			reg = <0x0 0x0>;
-			enable-method = "spin-table";
-			cpu-release-addr = <0x0 0x8000fff8>;
+			enable-method = "psci";
+			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
 		};
 		cpu@1 {
 			device_type = "cpu";
 			compatible = "arm,armv8";
 			reg = <0x0 0x1>;
-			enable-method = "spin-table";
-			cpu-release-addr = <0x0 0x8000fff8>;
+			enable-method = "psci";
+			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
 		};
 		cpu@2 {
 			device_type = "cpu";
 			compatible = "arm,armv8";
 			reg = <0x0 0x2>;
-			enable-method = "spin-table";
-			cpu-release-addr = <0x0 0x8000fff8>;
+			enable-method = "psci";
+			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
 		};
 		cpu@3 {
 			device_type = "cpu";
 			compatible = "arm,armv8";
 			reg = <0x0 0x3>;
-			enable-method = "spin-table";
-			cpu-release-addr = <0x0 0x8000fff8>;
+			enable-method = "psci";
+			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
 		};
 	};
 
-- 
1.8.4

Re: [PATCH RFC v3 6/6] arm64: boot: dts: update rtsm aemv8 dts with PSCI and idle states

[Sebastian Capella]

Quoting Lorenzo Pieralisi (2014-05-06 11:04:43)
> This patch updates the RTSM dts file with PSCI bindings and nodes
> describing the AEMv8 model idle states parameters.
> 
> PSCI function IDs compliancy with PSCI v0.2 is still under development
> so this patch provides PSCI function IDs for demonstration purposes only.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Reviewed-by: Sebastian Capella <sebastian.capella@linaro.org>

Thanks!

Sebastian