From 598a7acb19af3763aa17926168e24d8131f3a57d Mon Sep 17 00:00:00 2001
From: Brendan Jackman <brendan.jackman@arm.com>
Date: Mon, 25 Jul 2016 14:26:08 +0100
Subject: [PATCH] cpuidle: dt: Parse states from power domain nodes
This commit allows bindings similar to those in [1] to be used without
requiring CPU PM domains. The cpuidle DT parsing code will first read
idle states from the "cpu-idle-states" property, then walk up the
power-domains tree (assuming that it is indeed a tree) for the CPU and
read idle states from the "power-states" property.
This is a temporary hack to enable a prototype EAS energy model in the
device tree, for which we want to use CPU power-domains to describe
energy costs in a topologically-aware way.
[1] https://patchwork.kernel.org/patch/9193651/
---
drivers/cpuidle/dt_idle_states.c | 85 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 82 insertions(+), 3 deletions(-)
@@ -21,6 +21,8 @@
#include "dt_idle_states.h"
+#define MAX_PD_DEPTH 8
+
static int init_state_node(struct cpuidle_state *idle_state,
const struct of_device_id *matches,
struct device_node *state_node)
@@ -92,6 +94,84 @@ static int init_state_node(struct cpuidle_state *idle_state,
}
/*
+ * First look in cpu-idle-states, then walk up the power-domains tree, returning
+ * the idx'th idle state node encountered, or NULL if it can't be found.
+ *
+ * Returns the device_node pointer with refcount incremented. Use
+ * of_node_put() on it when done.
+ */
+static struct device_node *get_idle_state_node(struct device_node *cpu_node,
+ int idx)
+{
+ struct device_node *state_node, *consumer_node;
+ int num_cpu_idle_states, num_pd_idle_states;
+ int idx_pd;
+ unsigned int i;
+
+ state_node = of_parse_phandle(cpu_node, "cpu-idle-states", idx);
+ if (state_node)
+ return state_node;
+
+ num_cpu_idle_states = of_property_count_elems_of_size(cpu_node,
+ "cpu-idle-states",
+ 4);
+ if (num_cpu_idle_states < 1) {
+ pr_devel("%s: no cpu-idle-states for %s\n", __func__,
+ cpu_node->name);
+ num_cpu_idle_states = 0;
+ }
+
+ pr_devel("%s: Looking in %s's power domains tree for state %d\n",
+ __func__, cpu_node->full_name, idx);
+
+ BUG_ON(idx < num_cpu_idle_states);
+ idx_pd = idx - num_cpu_idle_states;
+
+ /* Use a loop counter so we don't infiniloop if there's a cycle in the
+ * power-domains graph. */
+ consumer_node = cpu_node;
+ for (i = 0; i < MAX_PD_DEPTH; i++) {
+ int num_pd_providers;
+ struct device_node *pd_node = of_parse_phandle(
+ consumer_node, "power-domains", 0);
+ if (!pd_node) {
+ return NULL;
+ }
+
+ /* We're assuming the power-domains graph is a tree, complain if
+ * not. */
+ num_pd_providers = of_property_count_elems_of_size(
+ consumer_node, "power-domains", 4);
+ BUG_ON(num_pd_providers < 1);
+ if (num_pd_providers > 1)
+ pr_warn("power-domains graph for %s not a tree "
+ "(%s has multiple power-domains)\n",
+ cpu_node->full_name, consumer_node->full_name);
+
+
+ state_node = of_parse_phandle(pd_node, "power-states", idx_pd);
+ if (state_node) {
+ of_node_put(pd_node);
+ return state_node;
+ }
+
+ num_pd_idle_states = of_property_count_elems_of_size(
+ pd_node, "power-states", 4);
+ if (num_pd_idle_states < 1)
+ num_pd_idle_states = 0;
+
+ BUG_ON(idx_pd < num_pd_idle_states);
+ idx_pd -= num_pd_idle_states;
+
+ of_node_put(pd_node);
+ consumer_node = pd_node;
+ }
+
+ WARN(1, "DT CPU power domain graph too deep. Probably has a cycle.");
+ return NULL;
+}
+
+/*
* Check that the idle state is uniform across all CPUs in the CPUidle driver
* cpumask
*/
@@ -112,8 +192,7 @@ static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
for (cpu = cpumask_next(cpumask_first(cpumask), cpumask);
cpu < nr_cpu_ids; cpu = cpumask_next(cpu, cpumask)) {
cpu_node = of_cpu_device_node_get(cpu);
- curr_state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
- idx);
+ curr_state_node = get_idle_state_node(cpu_node, idx);
if (state_node != curr_state_node)
valid = false;
@@ -170,7 +249,7 @@ int dt_init_idle_driver(struct cpuidle_driver *drv,
cpu_node = of_cpu_device_node_get(cpumask_first(cpumask));
for (i = 0; ; i++) {
- state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+ state_node = get_idle_state_node(cpu_node,i);
if (!state_node)
break;
--
1.9.1
Hi folks, I'm trying to start a conversation with Lina about using the power-domains DT bindings for describing the EAS power model. Partly because I'm still waiting for fossmail approval so I can't send this email to anyone without an @arm.com address, I wanted to circulate a draft internally before I make a fool of myself pubicly; does this mail make sense to you guys (also please let me know if I'm egregiously breaking linux-pm etiquette)? Cheers, Brendan ====================================================================== Hi Lina, Mark, I'm currently investigating a generic way of expressing in DT the power costs of CPU idle states, for use by the energy model for Energy Aware Scheduling (EAS) [1][2]. Essentially what I want to do is probably add a "power-usage" property to idle state nodes. I have heard that you and/or Kevin Hilman have actually experimented with something similar. However, the model is required to be topologically aware: for each idle state, we want to know the power usage of the CPU, as well as of the cluster-level resources it shares. This has led me to this patchset, since the CPU power domain bindings allow you to tell which topology level "owns" an idle state. I guess my point here is that these bindings have another use-case besides OS-coordinated cluster idle. I've hacked up a patch (attached) for my experimentation which allows the bindings as described in this mail to be used without any of the actual CPU Runtime PM implementation I share Mark's concern about the duplication between cpu-idle-states in CPU nodes and power-states in power domain nodes. As I understand it, with this patchset the boundary between cpu-idle-states and power-states nodes translates to the boundary between states managed by cpuidle and states managed by Runtime PM. Mark hinted at a solution where idle state nodes refer to the power domains they affect. I guess this solution would look something like: CPU_0: cpu@0 { compatible = "example-cpu"; reg = <0x0 0x0>; device_type = "cpu"; enable-method = "psci"; cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>; power-domains = <&CLUSTER_PD>; }; /* ... elsewhere ... */ idle-states { CPU_SLEEP: cpu-sleep { compatible = "arm,idle-state"; arm,psci-suspend-param = <0x0010000>; entry-latency-us = <100>; exit-latency-us = <250>; min-residency-us = <150>; }; CLUSTER_SLEEP: cluster-sleep { compatible = "arm,idle-state"; arm,psci-suspend-param = <0x1010000>; entry-latency-us = <800>; exit-latency-us = <700>; min-residency-us = <2500>; affects-power-domains = <&CLUSTER_PD> }; }; Then if desired/necessary, CLUSTER_SLEEP can be taken care of by Runtime PM (i.e. become a genpd_power_state) rather than cpuidle, the deciding factor being that it has an affects-power-domain property. (In this model, idle state nodes don't have a power-states property at all.) Mark, have I understood you correctly, is that what you were imagining? On 23/06/16 19:39, Lina Iyer wrote: > On Thu, Jun 23 2016 at 12:19 -0600, Mark Rutland wrote: >> On Thu, Jun 23, 2016 at 12:04:51PM -0600, Lina Iyer wrote: >>> Hi Mark, >>> >>> On Thu, Jun 23 2016 at 11:35 -0600, Mark Rutland wrote: >>> >Hi, >>> > >>> >On Wed, Jun 22, 2016 at 01:36:37PM -0600, Lina Iyer wrote: >>> >>From: Axel Haslam <ahaslam+renesas@baylibre.com> >>> >> >>> >>Update DT bindings to describe idle states of PM domains. >>> >> >>> >>Cc: <devicetree@vger.kernel.org> >>> >>Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com> >>> >>Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >>> >>[Lina: Added state properties, removed state names, wakeup-latency, >>> >>added of_pm_genpd_init() API, pruned commit text] >>> >>Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>> >>[Ulf: Moved around code to make it compile properly, rebased on top >>> of multiple state support] >>> >>--- >>> >> .../devicetree/bindings/power/power_domain.txt | 70 >>> ++++++++++++++++++++++ >>> >> 1 file changed, 70 insertions(+) >>> >> >>> >>diff --git >>> a/Documentation/devicetree/bindings/power/power_domain.txt >>> b/Documentation/devicetree/bindings/power/power_domain.txt >>> >>index 025b5e7..41e8dda 100644 >>> >>--- a/Documentation/devicetree/bindings/power/power_domain.txt >>> >>+++ b/Documentation/devicetree/bindings/power/power_domain.txt >>> >>@@ -29,6 +29,43 @@ Optional properties: >>> >> specified by this binding. More details about power domain >>> specifier are >>> >> available in the next section. >>> >> >>> >>+- power-states : A phandle of an idle-state that shall be soaked >>> into a >>> >>+ generic domain power state. >>> > >>> >It's somewhat unfortunate that this gives us two possible locations for >>> >idle state lists (under the /cpus node and in a pm-domains node), >>> >especially as it's not clear what would happen were a DT to have both. >>> > >>> >I would prefer that we extend the existing bindings such that states >>> can >>> >refer to the power domains which they affect. >>> > >>> I agree. The CPU idle states have become defined to be specific to CPUs. >>> PM Domain idle states are generic for any type of domain. I am hoping at >>> some point, we could converge and use the same idle state, but that >>> would mean changing the CPU idle states to make it generic. >> >> Outside of CPU idling, I don't fully understand how this will be used, >> so it's not clear to me what would need to be made generic. Apologies >> for my ignorance there. >> > There may be non-PSCI ARM v7 CPU domains that may have domain controller > drivers in the kernel. They would not hook into the ARM PSCI frameworks. > It is still cpuidle though. > Apologies also for my ignorance, but I don't think I understand you here. It sounds to me like the drivers you're describing are currently unable to get their idle states from DT - is that correct? If so, perhaps we can implement support using the CPU idle state nodes at first, then later make them generic so that these non-PSCI domains can use DT. >>> At some point, during my development, I did use the arm,idle-state for >>> domains as well, but the binding definitions were too restrictive for >>> a generic PM domain. >>> >>> I would be willing to make the change to CPU idle states to make it >>> generic and then we could just reference domain and CPU idle states >>> using the same bindings. Are we okay with that, specifically, >>> arm,psci-suspend-param? This binding is very restrictive in its >>> description. What we pass to the platform driver upon choosing a domain >>> state is very platform specific and therefore has to be generic in its >>> description. >> >> I was suggesting that for PSCI we should consistently us >> arm,psci-suspend-param, not that this should be used for all power >> domain state data. >> >> I imagine that mechanisms for powering down power domains will have >> varied requirements on data they require (and may require more than can >> be encoded in a u32), and I don't think it's best to try to force a >> single representation in the DT for that. It would be better to allow >> them to define the properties which they require. >> > The only way to do that is to push the DT parsing to the platform > drivers. In the case of CPU domains controlled by PSCI, we could use the > arm,idle-states but any other generic domain, may need to define their > own bindings and fill up the domain states before initiailizing the domain. > > While this approach pushes the onus on to the platform code, I am fine > with it. Is that what you were thinking too? [1] http://www.linaro.org/blog/core-dump/energy-aware-scheduling-eas-progress-update/ [2] https://lkml.org/lkml/2015/7/7/754 IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.