[PATCH v7 0/5] genpd multiple states v7

[PATCH v7 0/5] genpd multiple states v7

[ahaslam]

From: Axel Haslam <ahaslam+renesas@baylibre.com>

Some architectures may have intermediate power
levels between on and off. each state in between
may have its own set of procedures to put the
power domain into that state.

This patch adds the ability to declare multiple
states for a given generic power domain, the idea
is that the deepest state will be entered which
does not violate any of the device or sub-domain
latency constraints.

Changes since v6:
* change int to unsigned int were appropriate.

* spelling mistakes, and fix commit message for
removal of latencies.

Changes since v5:
* rebase to 4.1-rc1

* Pass state array as an init argument on pm_genpd_init

* declare a default OFF state with no latencies, that
will be used if a null state argument is given.

* set the deepest state when using sync_poweroff.

* create and use name allocation function in the 
debug area instead of inline.

Changes since v4:
* move to power_on/off callbacks out of the state array
Platforms can check the state_idx to know what
state the power on/off corresponds to.

* convert states to pointer,
Dynamically allocate the states array to save memory
on platforms with several power domains.

* fix bisect-ability,
by allowing the old latencies values to be used if the
state_count is 0. (meaning no states defined by the platform)

* rename target_state to state_idx and remove init_state,
platforms can directly set state_idx on registering
a domain that is off.

Changes since v3:
* remove old power on/off function at the end of the
series so that compilation will not break in between.

Changes since v2:
* remove state argument and macros from save/restore
callbacks.

* added init_state for platforms to pass the
initial state when the genpd is initially off.

* convert current genpd users for the structure changes.

* compile tested for shmobile_defconfig, exynos_defconfig,
s3c6400_defconfig u8500_defconfig.

Changes since v1:
* split the changes so that the actual logic that
selects the target state is a separate patch.

* move the cached logic out of the state function and add
it back to default_power_down_ok.

* use static arrays instead of dynamic for state declaration
since its not expected to have lots of intermediate states.
That way we don't have to mess with allocations.

* rename default_power_down_ok_state to power_down_ok_for_state

Axel Haslam (5):
  PM / Domains: prepare for multiple states
  PM / Domains: core changes for multiple states
  PM / Domains: make governor select deepest state
  ARM: imx6: pm: declare pm domain latency on power_state struct.
  PM / Domains: remove old power on/off latencies.

 arch/arm/mach-exynos/pm_domains.c    |   2 +-
 arch/arm/mach-imx/gpc.c              |  18 ++++-
 arch/arm/mach-s3c64xx/pm.c           |   5 +-
 arch/arm/mach-shmobile/pm-r8a7779.c  |   2 +-
 arch/arm/mach-shmobile/pm-rmobile.c  |   2 +-
 arch/arm/mach-ux500/pm_domains.c     |   2 +-
 drivers/base/power/domain.c          | 138 +++++++++++++++++++++++++++++++++--
 drivers/base/power/domain_governor.c |  71 +++++++++++-------
 include/linux/pm_domain.h            |  20 ++++-
 9 files changed, 214 insertions(+), 46 deletions(-)

-- 
1.9.1

[PATCH v7 1/5] PM / Domains: prepare for multiple states

[ahaslam]

From: Axel Haslam <ahaslam+renesas@baylibre.com>

prepare generic power domain init function parameters
to accept a pointer to the states structure that describes
the possible states that a power domain can enter.

There is no functional change, as support for multiple
domains will be added in subsequent patches.

Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>
---
 arch/arm/mach-exynos/pm_domains.c   |  2 +-
 arch/arm/mach-imx/gpc.c             |  2 +-
 arch/arm/mach-s3c64xx/pm.c          |  5 +++--
 arch/arm/mach-shmobile/pm-r8a7779.c |  2 +-
 arch/arm/mach-shmobile/pm-rmobile.c |  2 +-
 arch/arm/mach-ux500/pm_domains.c    |  2 +-
 drivers/base/power/domain.c         |  6 +++++-
 include/linux/pm_domain.h           | 14 ++++++++++++--
 8 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
index cbe56b3..3c68239 100644
--- a/arch/arm/mach-exynos/pm_domains.c
+++ b/arch/arm/mach-exynos/pm_domains.c
@@ -176,7 +176,7 @@ static __init int exynos4_pm_init_power_domain(void)
 no_clk:
 		on = __raw_readl(pd->base + 0x4) & INT_LOCAL_PWR_EN;
 
-		pm_genpd_init(&pd->pd, NULL, !on);
+		pm_genpd_init(&pd->pd, NULL, NULL, 0, !on);
 		of_genpd_add_provider_simple(np, &pd->pd);
 	}
 
diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index 4d60005..092b4ae 100644
--- a/arch/arm/mach-imx/gpc.c
+++ b/arch/arm/mach-imx/gpc.c
@@ -421,7 +421,7 @@ static int imx_gpc_genpd_init(struct device *dev, struct regulator *pu_reg)
 		imx6q_pm_pu_power_on(&imx6q_pu_domain.base);
 	}
 
-	pm_genpd_init(&imx6q_pu_domain.base, NULL, is_off);
+	pm_genpd_init(&imx6q_pu_domain.base, NULL, NULL, 0, is_off);
 	return of_genpd_add_provider_onecell(dev->of_node,
 					     &imx_gpc_onecell_data);
 
diff --git a/arch/arm/mach-s3c64xx/pm.c b/arch/arm/mach-s3c64xx/pm.c
index 75b14e7..0e79375 100644
--- a/arch/arm/mach-s3c64xx/pm.c
+++ b/arch/arm/mach-s3c64xx/pm.c
@@ -317,10 +317,11 @@ int __init s3c64xx_pm_init(void)
 
 	for (i = 0; i < ARRAY_SIZE(s3c64xx_always_on_pm_domains); i++)
 		pm_genpd_init(&s3c64xx_always_on_pm_domains[i]->pd,
-			      &pm_domain_always_on_gov, false);
+			      &pm_domain_always_on_gov, NULL, 0, false);
 
 	for (i = 0; i < ARRAY_SIZE(s3c64xx_pm_domains); i++)
-		pm_genpd_init(&s3c64xx_pm_domains[i]->pd, NULL, false);
+		pm_genpd_init(&s3c64xx_pm_domains[i]->pd,
+			      NULL, NULL, 0, false);
 
 #ifdef CONFIG_S3C_DEV_FB
 	if (dev_get_platdata(&s3c_device_fb.dev))
diff --git a/arch/arm/mach-shmobile/pm-r8a7779.c b/arch/arm/mach-shmobile/pm-r8a7779.c
index 44a74c4..a89a94f 100644
--- a/arch/arm/mach-shmobile/pm-r8a7779.c
+++ b/arch/arm/mach-shmobile/pm-r8a7779.c
@@ -84,7 +84,7 @@ static void r8a7779_init_pm_domain(struct r8a7779_pm_domain *r8a7779_pd)
 	struct generic_pm_domain *genpd = &r8a7779_pd->genpd;
 
 	genpd->flags = GENPD_FLAG_PM_CLK;
-	pm_genpd_init(genpd, NULL, false);
+	pm_genpd_init(genpd, NULL, NULL, 0, false);
 	genpd->dev_ops.active_wakeup = pd_active_wakeup;
 	genpd->power_off = pd_power_down;
 	genpd->power_on = pd_power_up;
diff --git a/arch/arm/mach-shmobile/pm-rmobile.c b/arch/arm/mach-shmobile/pm-rmobile.c
index 9501820..18f87c5 100644
--- a/arch/arm/mach-shmobile/pm-rmobile.c
+++ b/arch/arm/mach-shmobile/pm-rmobile.c
@@ -154,7 +154,7 @@ static void rmobile_init_pm_domain(struct rmobile_pm_domain *rmobile_pd)
 	struct dev_power_governor *gov = rmobile_pd->gov;
 
 	genpd->flags = GENPD_FLAG_PM_CLK;
-	pm_genpd_init(genpd, gov ? : &simple_qos_governor, false);
+	pm_genpd_init(genpd, gov ? : &simple_qos_governor, NULL, 0, false);
 	genpd->dev_ops.active_wakeup	= rmobile_pd_active_wakeup;
 	genpd->power_off		= rmobile_pd_power_down;
 	genpd->power_on			= rmobile_pd_power_up;
diff --git a/arch/arm/mach-ux500/pm_domains.c b/arch/arm/mach-ux500/pm_domains.c
index 4d71c90..7bd4d98 100644
--- a/arch/arm/mach-ux500/pm_domains.c
+++ b/arch/arm/mach-ux500/pm_domains.c
@@ -72,7 +72,7 @@ int __init ux500_pm_domains_init(void)
 	genpd_data->num_domains = ARRAY_SIZE(ux500_pm_domains);
 
 	for (i = 0; i < ARRAY_SIZE(ux500_pm_domains); ++i)
-		pm_genpd_init(ux500_pm_domains[i], NULL, false);
+		pm_genpd_init(ux500_pm_domains[i], NULL, NULL, 0, false);
 
 	of_genpd_add_provider_onecell(np, genpd_data);
 	return 0;
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 2327613..b30a42f 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1882,10 +1882,14 @@ static int pm_genpd_default_restore_state(struct device *dev)
  * pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.
  * @gov: PM domain governor to associate with the domain (may be NULL).
+ * @states: Array of possible low power states when powering off.
+ * @state_count: Number of low power states.
  * @is_off: Initial value of the domain's power_is_off field.
  */
 void pm_genpd_init(struct generic_pm_domain *genpd,
-		   struct dev_power_governor *gov, bool is_off)
+		   struct dev_power_governor *gov,
+		   const struct genpd_power_state *states,
+		   unsigned int state_count, bool is_off)
 {
 	if (IS_ERR_OR_NULL(genpd))
 		return;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 681ccb0..6b4802e 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -46,6 +46,12 @@ struct gpd_cpuidle_data {
 	struct cpuidle_state *idle_state;
 };
 
+struct genpd_power_state {
+	char *name;
+	s64 power_off_latency_ns;
+	s64 power_on_latency_ns;
+};
+
 struct generic_pm_domain {
 	struct dev_pm_domain domain;	/* PM domain operations */
 	struct list_head gpd_list_node;	/* Node in the global PM domains list */
@@ -149,7 +155,9 @@ extern int pm_genpd_name_attach_cpuidle(const char *name, int state);
 extern int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd);
 extern int pm_genpd_name_detach_cpuidle(const char *name);
 extern void pm_genpd_init(struct generic_pm_domain *genpd,
-			  struct dev_power_governor *gov, bool is_off);
+			  struct dev_power_governor *gov,
+			  const struct genpd_power_state *states,
+			  unsigned int state_count, bool is_off);
 
 extern int pm_genpd_poweron(struct generic_pm_domain *genpd);
 extern int pm_genpd_name_poweron(const char *domain_name);
@@ -216,7 +224,9 @@ static inline int pm_genpd_name_detach_cpuidle(const char *name)
 	return -ENOSYS;
 }
 static inline void pm_genpd_init(struct generic_pm_domain *genpd,
-				 struct dev_power_governor *gov, bool is_off)
+				 struct dev_power_governor *gov,
+				 const struct genpd_power_state *states,
+				 unsigned int state_count, bool is_off)
 {
 }
 static inline int pm_genpd_poweron(struct generic_pm_domain *genpd)
-- 
1.9.1

[PATCH v7 2/5] PM / Domains: core changes for multiple states

[ahaslam]

From: Axel Haslam <ahaslam+renesas@baylibre.com>

Add the core changes to be able to declare multiple states.
When trying to set a power domain to off, genpd will be able
to choose from an array of states declared by the platform.

The power on and off latencies are now tied to a state.

States should be declared in ascending order from shallowest
to deepest, deepest meaning the state which takes longer to
enter and exit.

the power_off and power_on function can use the 'state_idx'
field of the generic_pm_domain structure, to distinguish between
the different states and act accordingly.

 - if the genpd is initially off, the user should set the state_idx
   field when registering the genpd.

 - if no states are passed to pm_genpd_init, a single OFF
   state with no latency is assumed.

Example:

static int pd1_power_on(struct generic_pm_domain *domain)
{
	/* domain->state_idx = state the domain is coming from */
}

static int pd1_power_off(struct generic_pm_domain *domain)
{
	/* domain->state_idx = desired powered off state */
}

const struct genpd_power_state pd_states[] = {
	{
		.name = "RET",
		.power_on_latency_ns = ON_LATENCY_FAST,
		.power_off_latency_ns = OFF_LATENCY_FAST,
	},
	{
		.name = "DEEP_RET",
		.power_on_latency_ns = ON_LATENCY_MED,
		.power_off_latency_ns = OFF_LATENCY_MED,
	},
	{
		.name = "OFF",
		.power_on_latency_ns = ON_LATENCY_SLOW,
		.power_off_latency_ns = OFF_LATENCY_SLOW,
	}
};

struct generic_pm_domain pd1 = {
	.name = "PD1",
	.state_idx = 2, /* needed if domain is not on at init. */
	.power_on = pd1_power_on,
	.power_off = pd1_power_off,
	...
};

int xxx_init_pm_domain(){

	pm_genpd_init(struct generic_pm_domain,
			pd1, pd_states, ARRAY_SIZE(pd_states), true);

}

Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>
---
 drivers/base/power/domain.c          | 145 +++++++++++++++++++++++++++++++++--
 drivers/base/power/domain_governor.c |  13 +++-
 include/linux/pm_domain.h            |   4 +
 3 files changed, 153 insertions(+), 9 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index b30a42f..86f0c46 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -47,6 +47,12 @@
 	__retval;								\
 })
 
+#define GENPD_MAX_NAME_SIZE 20
+
+static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,
+				       const struct genpd_power_state *st,
+				       unsigned int st_count);
+
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 
@@ -170,12 +176,13 @@ static void genpd_set_active(struct generic_pm_domain *genpd)
 
 static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
 {
+	unsigned int state_idx = genpd->state_idx;
 	s64 usecs64;
 
 	if (!genpd->cpuidle_data)
 		return;
 
-	usecs64 = genpd->power_on_latency_ns;
+	usecs64 = genpd->states[state_idx].power_on_latency_ns;
 	do_div(usecs64, NSEC_PER_USEC);
 	usecs64 += genpd->cpuidle_data->saved_exit_latency;
 	genpd->cpuidle_data->idle_state->exit_latency = usecs64;
@@ -183,6 +190,7 @@ static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
 
 static int genpd_power_on(struct generic_pm_domain *genpd)
 {
+	unsigned int state_idx = genpd->state_idx;
 	ktime_t time_start;
 	s64 elapsed_ns;
 	int ret;
@@ -196,10 +204,10 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
 		return ret;
 
 	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
-	if (elapsed_ns <= genpd->power_on_latency_ns)
+	if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
 		return ret;
 
-	genpd->power_on_latency_ns = elapsed_ns;
+	genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
 	genpd->max_off_time_changed = true;
 	genpd_recalc_cpu_exit_latency(genpd);
 	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
@@ -210,6 +218,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
 
 static int genpd_power_off(struct generic_pm_domain *genpd)
 {
+	unsigned int state_idx = genpd->state_idx;
 	ktime_t time_start;
 	s64 elapsed_ns;
 	int ret;
@@ -223,10 +232,10 @@ static int genpd_power_off(struct generic_pm_domain *genpd)
 		return ret;
 
 	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
-	if (elapsed_ns <= genpd->power_off_latency_ns)
+	if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns)
 		return ret;
 
-	genpd->power_off_latency_ns = elapsed_ns;
+	genpd->states[state_idx].power_off_latency_ns = elapsed_ns;
 	genpd->max_off_time_changed = true;
 	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
 		 genpd->name, "off", elapsed_ns);
@@ -847,6 +856,8 @@ static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd)
 	    || atomic_read(&genpd->sd_count) > 0)
 		return;
 
+	/* Choose the deepest state when suspending */
+	genpd->state_idx = genpd->state_count - 1;
 	genpd_power_off(genpd);
 
 	genpd->status = GPD_STATE_POWER_OFF;
@@ -1468,6 +1479,61 @@ static void genpd_free_dev_data(struct device *dev,
 	dev_pm_put_subsys_data(dev);
 }
 
+static int genpd_alloc_states_data(struct generic_pm_domain *genpd,
+				   const struct genpd_power_state *st,
+				   unsigned int st_count)
+{
+	int ret = 0;
+	unsigned int i;
+
+	if (IS_ERR_OR_NULL(genpd)) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	if (!st || (st_count < 1)) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	/* Allocate the local memory to keep the states for this genpd */
+	genpd->states = kcalloc(st_count, sizeof(*st), GFP_KERNEL);
+	if (!genpd->states) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	for (i = 0; i < st_count; i++) {
+		genpd->states[i].power_on_latency_ns =
+			st[i].power_on_latency_ns;
+		genpd->states[i].power_off_latency_ns =
+			st[i].power_off_latency_ns;
+	}
+
+	/*
+	 * Copy the latency values To keep compatibility with
+	 * platforms that are not converted to use the multiple states.
+	 * This will be removed once all platforms are converted to use
+	 * multiple states. note that non converted platforms will use the
+	 * default single off state.
+	 */
+	if (genpd->power_on_latency_ns != 0)
+		genpd->states[0].power_on_latency_ns =
+				genpd->power_on_latency_ns;
+
+	if (genpd->power_off_latency_ns != 0)
+		genpd->states[0].power_off_latency_ns =
+				genpd->power_off_latency_ns;
+
+	genpd->state_count = st_count;
+
+	/* to save memory, Name allocation will happen if debug is enabled */
+	pm_genpd_alloc_states_names(genpd, st, st_count);
+
+err:
+	return ret;
+}
+
 /**
  * __pm_genpd_add_device - Add a device to an I/O PM domain.
  * @genpd: PM domain to add the device to.
@@ -1891,9 +1957,37 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 		   const struct genpd_power_state *states,
 		   unsigned int state_count, bool is_off)
 {
+	static const struct genpd_power_state genpd_default_state[] = {
+		{
+			.name = "OFF",
+			.power_off_latency_ns = 0,
+			.power_on_latency_ns = 0,
+		},
+	};
+	int ret;
+
 	if (IS_ERR_OR_NULL(genpd))
 		return;
 
+	/* If no states defined, use the default OFF state */
+	if (!states || (state_count < 1))
+		ret = genpd_alloc_states_data(genpd, genpd_default_state,
+					      ARRAY_SIZE(genpd_default_state));
+	else
+		ret = genpd_alloc_states_data(genpd, states, state_count);
+
+	if (ret) {
+		pr_err("Fail to allocate states for %s\n", genpd->name);
+		return;
+	}
+
+	/* Sanity check for initial state */
+	if (genpd->state_idx >= genpd->state_count) {
+		pr_warn("pm domain %s Invalid initial state.\n",
+			genpd->name);
+		genpd->state_idx = genpd->state_count - 1;
+	}
+
 	INIT_LIST_HEAD(&genpd->master_links);
 	INIT_LIST_HEAD(&genpd->slave_links);
 	INIT_LIST_HEAD(&genpd->dev_list);
@@ -2247,6 +2341,33 @@ EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
 #include <linux/kobject.h>
 static struct dentry *pm_genpd_debugfs_dir;
 
+static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,
+				       const struct genpd_power_state *st,
+				       unsigned int st_count)
+{
+	unsigned int i;
+
+	if (IS_ERR_OR_NULL(genpd))
+		return -EINVAL;
+
+	if (genpd->state_count != st_count) {
+		pr_err("Invalid allocated state count\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < st_count; i++) {
+		genpd->states[i].name = kstrndup(st[i].name,
+				GENPD_MAX_NAME_SIZE, GFP_KERNEL);
+		if (!genpd->states[i].name) {
+			pr_err("%s Failed to allocate state %d name.\n",
+				genpd->name, i);
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
 /*
  * TODO: This function is a slightly modified version of rtpm_status_show
  * from sysfs.c, so generalize it.
@@ -2283,6 +2404,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
 		[GPD_STATE_REPEAT] = "off-in-progress",
 		[GPD_STATE_POWER_OFF] = "off"
 	};
+	unsigned int state_idx = genpd->state_idx;
 	struct pm_domain_data *pm_data;
 	const char *kobj_path;
 	struct gpd_link *link;
@@ -2294,7 +2416,11 @@ static int pm_genpd_summary_one(struct seq_file *s,
 
 	if (WARN_ON(genpd->status >= ARRAY_SIZE(status_lookup)))
 		goto exit;
-	seq_printf(s, "%-30s  %-15s  ", genpd->name, status_lookup[genpd->status]);
+
+	seq_printf(s, "%-30s  %-15s  ", genpd->name,
+		   (genpd->status == GPD_STATE_POWER_OFF) ?
+		   genpd->states[state_idx].name :
+		   status_lookup[genpd->status]);
 
 	/*
 	 * Modifications on the list require holding locks on both
@@ -2382,4 +2508,11 @@ static void __exit pm_genpd_debug_exit(void)
 	debugfs_remove_recursive(pm_genpd_debugfs_dir);
 }
 __exitcall(pm_genpd_debug_exit);
+#else
+static inline int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,
+					const struct genpd_power_state *st,
+					unsigned int st_count)
+{
+	return 0;
+}
 #endif /* CONFIG_PM_ADVANCED_DEBUG */
diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index 2a4154a..5e63a84 100644
--- a/drivers/base/power/domain_governor.c
+++ b/drivers/base/power/domain_governor.c
@@ -124,8 +124,12 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
 		return genpd->cached_power_down_ok;
 	}
 
-	off_on_time_ns = genpd->power_off_latency_ns +
-				genpd->power_on_latency_ns;
+	/*
+	 * Use the only available state, until multiple state support is added
+	 * to the governor.
+	 */
+	off_on_time_ns = genpd->states[0].power_off_latency_ns +
+				genpd->states[0].power_on_latency_ns;
 	/*
 	 * It doesn't make sense to remove power from the domain if saving
 	 * the state of all devices in it and the power off/power on operations
@@ -215,8 +219,11 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
 	 * The difference between the computed minimum subdomain or device off
 	 * time and the time needed to turn the domain on is the maximum
 	 * theoretical time this domain can spend in the "off" state.
+	 * Use the only available state, until multiple state support is added
+	 * to the governor.
 	 */
-	genpd->max_off_time_ns = min_off_time_ns - genpd->power_on_latency_ns;
+	genpd->max_off_time_ns = min_off_time_ns -
+		genpd->states[0].power_on_latency_ns;
 	return true;
 }
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 6b4802e..581ae08 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -86,6 +86,10 @@ struct generic_pm_domain {
 	void (*detach_dev)(struct generic_pm_domain *domain,
 			   struct device *dev);
 	unsigned int flags;		/* Bit field of configs for genpd */
+	struct genpd_power_state *states;
+	unsigned int state_count; /* number of states */
+	unsigned int state_idx; /* state that genpd will go to when off */
+
 };
 
 static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
-- 
1.9.1

[PATCH v7 3/5] PM / Domains: make governor select deepest state

[ahaslam]

From: Axel Haslam <ahaslam+renesas@baylibre.com>

now that the structures of genpd can support
multiple state definitions, add the logic in
the governor to select the deepest possible
state when powering down.

For this, create the new function power_down_ok_for_state
which will test if a particular state will not violate
the devices and sub-domains constraints.

default_power_down_ok is modified to try each
state starting from the deepest until a valid
state is found or there are no more states to test.

the resulting state will be valid until
there are latency or constraint changes,
thus, we can avoid looping every power_down,
and use the cached results instead.

Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>
---
 drivers/base/power/domain_governor.c | 76 +++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 31 deletions(-)

diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index 5e63a84..8139c01 100644
--- a/drivers/base/power/domain_governor.c
+++ b/drivers/base/power/domain_governor.c
@@ -97,7 +97,8 @@ static bool default_stop_ok(struct device *dev)
  *
  * This routine must be executed under the PM domain's lock.
  */
-static bool default_power_down_ok(struct dev_pm_domain *pd)
+static bool power_down_ok_for_state(struct dev_pm_domain *pd,
+				     unsigned int state)
 {
 	struct generic_pm_domain *genpd = pd_to_genpd(pd);
 	struct gpd_link *link;
@@ -105,31 +106,8 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
 	s64 min_off_time_ns;
 	s64 off_on_time_ns;
 
-	if (genpd->max_off_time_changed) {
-		struct gpd_link *link;
-
-		/*
-		 * We have to invalidate the cached results for the masters, so
-		 * use the observation that default_power_down_ok() is not
-		 * going to be called for any master until this instance
-		 * returns.
-		 */
-		list_for_each_entry(link, &genpd->slave_links, slave_node)
-			link->master->max_off_time_changed = true;
-
-		genpd->max_off_time_changed = false;
-		genpd->cached_power_down_ok = false;
-		genpd->max_off_time_ns = -1;
-	} else {
-		return genpd->cached_power_down_ok;
-	}
-
-	/*
-	 * Use the only available state, until multiple state support is added
-	 * to the governor.
-	 */
-	off_on_time_ns = genpd->states[0].power_off_latency_ns +
-				genpd->states[0].power_on_latency_ns;
+	off_on_time_ns = genpd->states[state].power_off_latency_ns +
+		genpd->states[state].power_on_latency_ns;
 	/*
 	 * It doesn't make sense to remove power from the domain if saving
 	 * the state of all devices in it and the power off/power on operations
@@ -205,8 +183,6 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
 			min_off_time_ns = constraint_ns;
 	}
 
-	genpd->cached_power_down_ok = true;
-
 	/*
 	 * If the computed minimum device off time is negative, there are no
 	 * latency constraints, so the domain can spend arbitrary time in the
@@ -219,14 +195,52 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
 	 * The difference between the computed minimum subdomain or device off
 	 * time and the time needed to turn the domain on is the maximum
 	 * theoretical time this domain can spend in the "off" state.
-	 * Use the only available state, until multiple state support is added
-	 * to the governor.
 	 */
 	genpd->max_off_time_ns = min_off_time_ns -
-		genpd->states[0].power_on_latency_ns;
+			genpd->states[state].power_on_latency_ns;
 	return true;
 }
 
+static bool default_power_down_ok(struct dev_pm_domain *pd)
+{
+	struct generic_pm_domain *genpd = pd_to_genpd(pd);
+	unsigned int last_state_idx = genpd->state_count - 1;
+	struct gpd_link *link;
+	bool retval = false;
+	unsigned int i;
+
+	/*
+	 * if there was no change on max_off_time, we can return the
+	 * cached value and we dont need to find a new target_state
+	 */
+	if (!genpd->max_off_time_changed)
+		return genpd->cached_power_down_ok;
+
+	/*
+	 * We have to invalidate the cached results for the masters, so
+	 * use the observation that default_power_down_ok() is not
+	 * going to be called for any master until this instance
+	 * returns.
+	 */
+	list_for_each_entry(link, &genpd->slave_links, slave_node)
+		link->master->max_off_time_changed = true;
+
+	genpd->max_off_time_ns = -1;
+	genpd->max_off_time_changed = false;
+
+	/* find a state to power down to, starting from the deepest */
+	for (i = 0; i < genpd->state_count; i++) {
+		if (power_down_ok_for_state(pd, last_state_idx - i)) {
+			genpd->state_idx = last_state_idx - i;
+			retval = true;
+			break;
+		}
+	}
+
+	genpd->cached_power_down_ok = retval;
+	return retval;
+}
+
 static bool always_on_power_down_ok(struct dev_pm_domain *domain)
 {
 	return false;
-- 
1.9.1

[PATCH v7 4/5] ARM: imx6: pm: declare pm domain latency on power_state struct.

[ahaslam]

From: Axel Haslam <ahaslam+renesas@baylibre.com>

The generic_pm_domain structure uses an array of latencies
to be able to declare multiple intermediate states.

Declare a single "OFF" state with the default latencies
So that the  power_off_latency_ns and power_on_latency_ns
fields of generic_pm_domain structure can be eventualy removed.

Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>
---
 arch/arm/mach-imx/gpc.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index 092b4ae..9329ed2 100644
--- a/arch/arm/mach-imx/gpc.c
+++ b/arch/arm/mach-imx/gpc.c
@@ -45,9 +45,11 @@
 
 struct pu_domain {
 	struct generic_pm_domain base;
+	struct genpd_power_state *states;
 	struct regulator *reg;
 	struct clk *clk[GPC_CLK_MAX];
 	int num_clks;
+	unsigned int num_states;
 };
 
 static void __iomem *gpc_base;
@@ -365,14 +367,22 @@ static struct generic_pm_domain imx6q_arm_domain = {
 	.name = "ARM",
 };
 
+static struct genpd_power_state imx6q_arm_domain_states[] = {
+	{
+		.name = "OFF",
+		.power_off_latency_ns = 25000,
+		.power_on_latency_ns = 2000000,
+	},
+};
+
 static struct pu_domain imx6q_pu_domain = {
 	.base = {
 		.name = "PU",
 		.power_off = imx6q_pm_pu_power_off,
 		.power_on = imx6q_pm_pu_power_on,
-		.power_off_latency_ns = 25000,
-		.power_on_latency_ns = 2000000,
 	},
+	.states = imx6q_arm_domain_states,
+	.num_states = ARRAY_SIZE(imx6q_arm_domain_states),
 };
 
 static struct generic_pm_domain imx6sl_display_domain = {
@@ -421,7 +431,9 @@ static int imx_gpc_genpd_init(struct device *dev, struct regulator *pu_reg)
 		imx6q_pm_pu_power_on(&imx6q_pu_domain.base);
 	}
 
-	pm_genpd_init(&imx6q_pu_domain.base, NULL, NULL, 0, is_off);
+	pm_genpd_init(&imx6q_pu_domain.base, NULL, imx6q_pu_domain.states,
+		      imx6q_pu_domain.num_states, is_off);
+
 	return of_genpd_add_provider_onecell(dev->of_node,
 					     &imx_gpc_onecell_data);
 
-- 
1.9.1

[PATCH v7 5/5] PM / Domains: remove old power on/off latencies.

[ahaslam]

From: Axel Haslam <ahaslam+renesas@baylibre.com>

Now that all known users have been converted to use state
latencies, we can remove the latency field in the generic_pm_domain
structure.

Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>
---
 drivers/base/power/domain.c | 15 ---------------
 include/linux/pm_domain.h   |  2 --
 2 files changed, 17 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 86f0c46..fa863c7 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1510,21 +1510,6 @@ static int genpd_alloc_states_data(struct generic_pm_domain *genpd,
 			st[i].power_off_latency_ns;
 	}
 
-	/*
-	 * Copy the latency values To keep compatibility with
-	 * platforms that are not converted to use the multiple states.
-	 * This will be removed once all platforms are converted to use
-	 * multiple states. note that non converted platforms will use the
-	 * default single off state.
-	 */
-	if (genpd->power_on_latency_ns != 0)
-		genpd->states[0].power_on_latency_ns =
-				genpd->power_on_latency_ns;
-
-	if (genpd->power_off_latency_ns != 0)
-		genpd->states[0].power_off_latency_ns =
-				genpd->power_off_latency_ns;
-
 	genpd->state_count = st_count;
 
 	/* to save memory, Name allocation will happen if debug is enabled */
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 581ae08..0248207 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -73,9 +73,7 @@ struct generic_pm_domain {
 	unsigned int prepared_count;	/* Suspend counter of prepared devices */
 	bool suspend_power_off;	/* Power status before system suspend */
 	int (*power_off)(struct generic_pm_domain *domain);
-	s64 power_off_latency_ns;
 	int (*power_on)(struct generic_pm_domain *domain);
-	s64 power_on_latency_ns;
 	struct gpd_dev_ops dev_ops;
 	s64 max_off_time_ns;	/* Maximum allowed "suspended" time. */
 	bool max_off_time_changed;
-- 
1.9.1

Re: [PATCH v7 1/5] PM / Domains: prepare for multiple states

[Lina Iyer]

On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote:
>From: Axel Haslam <ahaslam+renesas@baylibre.com>
>
>prepare generic power domain init function parameters
>to accept a pointer to the states structure that describes
>the possible states that a power domain can enter.
>
>There is no functional change, as support for multiple
>domains will be added in subsequent patches.
>
>Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>

<...>

> extern void pm_genpd_init(struct generic_pm_domain *genpd,
>-			  struct dev_power_governor *gov, bool is_off);
>+			  struct dev_power_governor *gov,
>+			  const struct genpd_power_state *states,
>+			  unsigned int state_count, bool is_off);
>

Wouldnt it be better to setup another function pm_genpd_init_simple()
that calls into pm_genpd_init() with no arguments?

static inline void pm_genpd_init_simple(struct generic_pm_domain *genpd,
                               struct dev_power_governor *gov, bool is_off)
{
	return pm_genpd_init(genpd, gov, NULL, 0, is_off);
}

It would be explicit that way to indicate the new genpd feature that would let
domains support multiple states.

Thanks,
Lina

Re: [PATCH v7 1/5] PM / Domains: prepare for multiple states

[Axel Haslam]

Hi Lina,

On 30/04/2015 17:29, Lina Iyer wrote:
> On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote:
>> From: Axel Haslam <ahaslam+renesas@baylibre.com>
>>
>> prepare generic power domain init function parameters
>> to accept a pointer to the states structure that describes
>> the possible states that a power domain can enter.
>>
>> There is no functional change, as support for multiple
>> domains will be added in subsequent patches.
>>
>> Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>
>
> <...>
>
>> extern void pm_genpd_init(struct generic_pm_domain *genpd,
>> -              struct dev_power_governor *gov, bool is_off);
>> +              struct dev_power_governor *gov,
>> +              const struct genpd_power_state *states,
>> +              unsigned int state_count, bool is_off);
>>
>
> Wouldnt it be better to setup another function pm_genpd_init_simple()
> that calls into pm_genpd_init() with no arguments?
>
> static inline void pm_genpd_init_simple(struct generic_pm_domain *genpd,
>                                struct dev_power_governor *gov, bool is_off)
> {
>      return pm_genpd_init(genpd, gov, NULL, 0, is_off);
> }

Im not against adding the wrapper if it simplifies things. But, in 
general, i think all latencies should be set, otherwise genpd may 
violate device constraints by turning a power domain off when it should 
not. So maybe, by leaving the arguments, it kind of sends the message to 
the developer that something important is needed.

Regards,
Axel

>
> It would be explicit that way to indicate the new genpd feature that
> would let
> domains support multiple states.
>
> Thanks,
> Lina

Re: [PATCH v7 1/5] PM / Domains: prepare for multiple states

[Lina Iyer]

On Thu, Apr 30 2015 at 10:06 -0600, Axel Haslam wrote:
>Hi Lina,
>
>On 30/04/2015 17:29, Lina Iyer wrote:
>>On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote:
>>>From: Axel Haslam <ahaslam+renesas@baylibre.com>
>>>
>>>prepare generic power domain init function parameters
>>>to accept a pointer to the states structure that describes
>>>the possible states that a power domain can enter.
>>>
>>>There is no functional change, as support for multiple
>>>domains will be added in subsequent patches.
>>>
>>>Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>
>>
>><...>
>>
>>>extern void pm_genpd_init(struct generic_pm_domain *genpd,
>>>-              struct dev_power_governor *gov, bool is_off);
>>>+              struct dev_power_governor *gov,
>>>+              const struct genpd_power_state *states,
>>>+              unsigned int state_count, bool is_off);
>>>
>>
>>Wouldnt it be better to setup another function pm_genpd_init_simple()
>>that calls into pm_genpd_init() with no arguments?
>>
>>static inline void pm_genpd_init_simple(struct generic_pm_domain *genpd,
>>                               struct dev_power_governor *gov, bool is_off)
>>{
>>     return pm_genpd_init(genpd, gov, NULL, 0, is_off);
>>}
>
>Im not against adding the wrapper if it simplifies things. But, in 
>general, i think all latencies should be set, otherwise genpd may 
>violate device constraints by turning a power domain off when it 
>should not. So maybe, by leaving the arguments, it kind of sends the 
>message to the developer that something important is needed.

hmm. Alright.

>
>Regards,
>Axel
>
>>
>>It would be explicit that way to indicate the new genpd feature that
>>would let
>>domains support multiple states.
>>
>>Thanks,
>>Lina

Re: [PATCH v7 1/5] PM / Domains: prepare for multiple states

[Geert Uytterhoeven]

On Thu, Apr 30, 2015 at 6:06 PM, Axel Haslam <ahaslam@baylibre.com> wrote:
> Im not against adding the wrapper if it simplifies things. But, in general,
> i think all latencies should be set, otherwise genpd may violate device
> constraints by turning a power domain off when it should not. So maybe, by
> leaving the arguments, it kind of sends the message to the developer that
> something important is needed.

Note that in the absence of latencies, device constraints may indeed be
violated once. After that the genpd core will have measured the real
latency value, and will start using that.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v7 2/5] PM / Domains: core changes for multiple states

[Lina Iyer]

On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote:
>From: Axel Haslam <ahaslam+renesas@baylibre.com>
>
>Add the core changes to be able to declare multiple states.
>When trying to set a power domain to off, genpd will be able
>to choose from an array of states declared by the platform.
>
>The power on and off latencies are now tied to a state.
>
>States should be declared in ascending order from shallowest
>to deepest, deepest meaning the state which takes longer to
>enter and exit.
>
>the power_off and power_on function can use the 'state_idx'
>field of the generic_pm_domain structure, to distinguish between
>the different states and act accordingly.
>
> - if the genpd is initially off, the user should set the state_idx
>   field when registering the genpd.
>
> - if no states are passed to pm_genpd_init, a single OFF
>   state with no latency is assumed.
>
>Example:
>
>static int pd1_power_on(struct generic_pm_domain *domain)
>{
>	/* domain->state_idx = state the domain is coming from */
>}
>
>static int pd1_power_off(struct generic_pm_domain *domain)
>{
>	/* domain->state_idx = desired powered off state */
>}
>
>const struct genpd_power_state pd_states[] = {
>	{
>		.name = "RET",
>		.power_on_latency_ns = ON_LATENCY_FAST,
>		.power_off_latency_ns = OFF_LATENCY_FAST,
>	},
>	{
>		.name = "DEEP_RET",
>		.power_on_latency_ns = ON_LATENCY_MED,
>		.power_off_latency_ns = OFF_LATENCY_MED,
>	},
>	{
>		.name = "OFF",
>		.power_on_latency_ns = ON_LATENCY_SLOW,
>		.power_off_latency_ns = OFF_LATENCY_SLOW,
>	}
>};
>

I am trying to use your patches in fashioning a genpd for the cpu
domain.

The cpus are part of a power domain that can be powered off when the
cpus are powered off as part of the cpuidle. One of the biggest power
savings comes from powering off the domain completely. However, powering
off the domain involves flushing of caches (possibly) and turning off
some regulators and peripheral hardware. The power benefit is only
realized when the domain remains powered off for a certain period of
time, otherwise the overhead of powering on/off would add up to the
ineffeciency in the system.

So a governor that determines the idle state of the domain has two
things that needs to match, the time available to power off the domain
(which we can get from timer wheel) and the other residency as dictated
by the platform.

I was wondering if we could match the idle state definition of the
domain with that of the cpu, which has a precedence in the kernel. The
idle state of the cpu [1] is a superset of the idle state definition you
have above.  That way a shim layer could pick up domain idle states from
DT and pass the states to pm_genpd_init(). The use of these values could
depend on the genpd governor.

Thanks,
Lina

>struct generic_pm_domain pd1 = {
>	.name = "PD1",
>	.state_idx = 2, /* needed if domain is not on at init. */
>	.power_on = pd1_power_on,
>	.power_off = pd1_power_off,
>	...
>};
>
>int xxx_init_pm_domain(){
>
>	pm_genpd_init(struct generic_pm_domain,
>			pd1, pd_states, ARRAY_SIZE(pd_states), true);
>
>}
>
>Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>
>---

[1]. Documentation/devicetree/bindings/arm/idle-states.txt

Re: [PATCH v7 1/5] PM / Domains: prepare for multiple states

[Krzysztof Kozlowski]

2015-04-30 19:55 GMT+09:00  <ahaslam@baylibre.com>:
> From: Axel Haslam <ahaslam+renesas@baylibre.com>
>
> prepare generic power domain init function parameters
> to accept a pointer to the states structure that describes
> the possible states that a power domain can enter.
>
> There is no functional change, as support for multiple
> domains will be added in subsequent patches.
>
> Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>
> ---
>  arch/arm/mach-exynos/pm_domains.c   |  2 +-

For Exynos:
Acked-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

However could you fix up your GIT or email app? It is wrapping lines
in commit message around ~55 character making the message unreadable.
Just use default value of 75 characters (gui.commitmsgwidth?).

Best regards,
Krzysztof

Re: [PATCH v7 2/5] PM / Domains: core changes for multiple states

[Kevin Hilman]

Lina Iyer <lina.iyer@linaro.org> writes:

> On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote:
>>From: Axel Haslam <ahaslam+renesas@baylibre.com>
>>
>>Add the core changes to be able to declare multiple states.
>>When trying to set a power domain to off, genpd will be able
>>to choose from an array of states declared by the platform.
>>
>>The power on and off latencies are now tied to a state.
>>
>>States should be declared in ascending order from shallowest
>>to deepest, deepest meaning the state which takes longer to
>>enter and exit.
>>
>>the power_off and power_on function can use the 'state_idx'
>>field of the generic_pm_domain structure, to distinguish between
>>the different states and act accordingly.
>>
>> - if the genpd is initially off, the user should set the state_idx
>>   field when registering the genpd.
>>
>> - if no states are passed to pm_genpd_init, a single OFF
>>   state with no latency is assumed.
>>
>>Example:
>>
>>static int pd1_power_on(struct generic_pm_domain *domain)
>>{
>>	/* domain->state_idx = state the domain is coming from */
>>}
>>
>>static int pd1_power_off(struct generic_pm_domain *domain)
>>{
>>	/* domain->state_idx = desired powered off state */
>>}
>>
>>const struct genpd_power_state pd_states[] = {
>>	{
>>		.name = "RET",
>>		.power_on_latency_ns = ON_LATENCY_FAST,
>>		.power_off_latency_ns = OFF_LATENCY_FAST,
>>	},
>>	{
>>		.name = "DEEP_RET",
>>		.power_on_latency_ns = ON_LATENCY_MED,
>>		.power_off_latency_ns = OFF_LATENCY_MED,
>>	},
>>	{
>>		.name = "OFF",
>>		.power_on_latency_ns = ON_LATENCY_SLOW,
>>		.power_off_latency_ns = OFF_LATENCY_SLOW,
>>	}
>>};
>>
>
> I am trying to use your patches in fashioning a genpd for the cpu
> domain.
>
> The cpus are part of a power domain that can be powered off when the
> cpus are powered off as part of the cpuidle. One of the biggest power
> savings comes from powering off the domain completely. However, powering
> off the domain involves flushing of caches (possibly) and turning off
> some regulators and peripheral hardware. The power benefit is only
> realized when the domain remains powered off for a certain period of
> time, otherwise the overhead of powering on/off would add up to the
> ineffeciency in the system.
>
> So a governor that determines the idle state of the domain has two
> things that needs to match, the time available to power off the domain
> (which we can get from timer wheel) and the other residency as dictated
> by the platform.
>
> I was wondering if we could match the idle state definition of the
> domain with that of the cpu, which has a precedence in the kernel. The
> idle state of the cpu [1] is a superset of the idle state definition you
> have above.  That way a shim layer could pick up domain idle states from
> DT and pass the states to pm_genpd_init(). The use of these values could
> depend on the genpd governor.

IMO, we need to keep the domain latency descriptions separate from the
the devices within those domains, which could be CPUs or any other form
of device.

It should be the job of the governor then to look at the domain latency
along with latencies of the other devices in the domain to make its
decision.

Kevin

Re: [PATCH v7 2/5] PM / Domains: core changes for multiple states

[Lina Iyer]

On Fri, May 08 2015 at 14:53 -0600, Kevin Hilman wrote:
>Lina Iyer <lina.iyer@linaro.org> writes:
>
>> On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote:
>>>From: Axel Haslam <ahaslam+renesas@baylibre.com>
>>>
>>>Add the core changes to be able to declare multiple states.
>>>When trying to set a power domain to off, genpd will be able
>>>to choose from an array of states declared by the platform.
>>>
>>>The power on and off latencies are now tied to a state.
>>>
>>>States should be declared in ascending order from shallowest
>>>to deepest, deepest meaning the state which takes longer to
>>>enter and exit.
>>>
>>>the power_off and power_on function can use the 'state_idx'
>>>field of the generic_pm_domain structure, to distinguish between
>>>the different states and act accordingly.
>>>
>>> - if the genpd is initially off, the user should set the state_idx
>>>   field when registering the genpd.
>>>
>>> - if no states are passed to pm_genpd_init, a single OFF
>>>   state with no latency is assumed.
>>>
>>>Example:
>>>
>>>static int pd1_power_on(struct generic_pm_domain *domain)
>>>{
>>>	/* domain->state_idx = state the domain is coming from */
>>>}
>>>
>>>static int pd1_power_off(struct generic_pm_domain *domain)
>>>{
>>>	/* domain->state_idx = desired powered off state */
>>>}
>>>
>>>const struct genpd_power_state pd_states[] = {
>>>	{
>>>		.name = "RET",
>>>		.power_on_latency_ns = ON_LATENCY_FAST,
>>>		.power_off_latency_ns = OFF_LATENCY_FAST,
>>>	},
>>>	{
>>>		.name = "DEEP_RET",
>>>		.power_on_latency_ns = ON_LATENCY_MED,
>>>		.power_off_latency_ns = OFF_LATENCY_MED,
>>>	},
>>>	{
>>>		.name = "OFF",
>>>		.power_on_latency_ns = ON_LATENCY_SLOW,
>>>		.power_off_latency_ns = OFF_LATENCY_SLOW,
>>>	}
>>>};
>>>
>>
>> I am trying to use your patches in fashioning a genpd for the cpu
>> domain.
>>
>> The cpus are part of a power domain that can be powered off when the
>> cpus are powered off as part of the cpuidle. One of the biggest power
>> savings comes from powering off the domain completely. However, powering
>> off the domain involves flushing of caches (possibly) and turning off
>> some regulators and peripheral hardware. The power benefit is only
>> realized when the domain remains powered off for a certain period of
>> time, otherwise the overhead of powering on/off would add up to the
>> ineffeciency in the system.
>>
>> So a governor that determines the idle state of the domain has two
>> things that needs to match, the time available to power off the domain
>> (which we can get from timer wheel) and the other residency as dictated
>> by the platform.
>>
>> I was wondering if we could match the idle state definition of the
>> domain with that of the cpu, which has a precedence in the kernel. The
>> idle state of the cpu [1] is a superset of the idle state definition you
>> have above.  That way a shim layer could pick up domain idle states from
>> DT and pass the states to pm_genpd_init(). The use of these values could
>> depend on the genpd governor.
>
>IMO, we need to keep the domain latency descriptions separate from the
>the devices within those domains, which could be CPUs or any other form
>of device.

Sure, the devices could be any. All I am saying is the format of idle
state definitions could be the same (same as cpu). There is a good
value in specifying the residency of an idle state in addition to the
the enter and exit latency.

>It should be the job of the governor then to look at the domain latency
>along with latencies of the other devices in the domain to make its
>decision.

Agreed and thats how it should be. In all probablility, a cpu-domain
governor would need a residency value of the domain idle state to best
determine the idle state of the domain.

If we have not stored the residency in struct genpd_power_state, then
the governor would not have a good way to make that decision.

I am not ready yet with my patches, but feel free to browse through my
working tree at [1] to see how I plan to use this patchset.

Thanks,
Lina

[1].
https://git.linaro.org/people/lina.iyer/linux-next.git/shortlog/refs/heads/genpd-8916-3

Re: [PATCH v7 1/5] PM / Domains: prepare for multiple states

[Axel Haslam]

Hi Krzysztof,

On Fri, May 8, 2015 at 2:36 AM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> 2015-04-30 19:55 GMT+09:00  <ahaslam@baylibre.com>:
>> From: Axel Haslam <ahaslam+renesas@baylibre.com>
>>
>> prepare generic power domain init function parameters
>> to accept a pointer to the states structure that describes
>> the possible states that a power domain can enter.
>>
>> There is no functional change, as support for multiple
>> domains will be added in subsequent patches.
>>
>> Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>
>> ---
>>  arch/arm/mach-exynos/pm_domains.c   |  2 +-
>
> For Exynos:
> Acked-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>
> However could you fix up your GIT or email app? It is wrapping lines
> in commit message around ~55 character making the message unreadable.
> Just use default value of 75 characters (gui.commitmsgwidth?).

Ok, ill change the message width in the next series,
ill wait for acks or comments to the rest of the patches
before sending it.

Regards,
Axel.

>
> Best regards,
> Krzysztof

Re: [PATCH v7 2/5] PM / Domains: core changes for multiple states

[Axel Haslam]

Hi Lina,

+ Bartosz who was interested in the patches too.

On Sat, May 9, 2015 at 12:31 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Fri, May 08 2015 at 14:53 -0600, Kevin Hilman wrote:
>>
>> Lina Iyer <lina.iyer@linaro.org> writes:
>>
>>> On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote:
>>>>
>>>> From: Axel Haslam <ahaslam+renesas@baylibre.com>
>>>>
>>>> Add the core changes to be able to declare multiple states.
>>>> When trying to set a power domain to off, genpd will be able
>>>> to choose from an array of states declared by the platform.
>>>>
>>>> The power on and off latencies are now tied to a state.
>>>>
>>>> States should be declared in ascending order from shallowest
>>>> to deepest, deepest meaning the state which takes longer to
>>>> enter and exit.
>>>>
>>>> the power_off and power_on function can use the 'state_idx'
>>>> field of the generic_pm_domain structure, to distinguish between
>>>> the different states and act accordingly.
>>>>
>>>> - if the genpd is initially off, the user should set the state_idx
>>>>   field when registering the genpd.
>>>>
>>>> - if no states are passed to pm_genpd_init, a single OFF
>>>>   state with no latency is assumed.
>>>>
>>>> Example:
>>>>
>>>> static int pd1_power_on(struct generic_pm_domain *domain)
>>>> {
>>>>         /* domain->state_idx = state the domain is coming from */
>>>> }
>>>>
>>>> static int pd1_power_off(struct generic_pm_domain *domain)
>>>> {
>>>>         /* domain->state_idx = desired powered off state */
>>>> }
>>>>
>>>> const struct genpd_power_state pd_states[] = {
>>>>         {
>>>>                 .name = "RET",
>>>>                 .power_on_latency_ns = ON_LATENCY_FAST,
>>>>                 .power_off_latency_ns = OFF_LATENCY_FAST,
>>>>         },
>>>>         {
>>>>                 .name = "DEEP_RET",
>>>>                 .power_on_latency_ns = ON_LATENCY_MED,
>>>>                 .power_off_latency_ns = OFF_LATENCY_MED,
>>>>         },
>>>>         {
>>>>                 .name = "OFF",
>>>>                 .power_on_latency_ns = ON_LATENCY_SLOW,
>>>>                 .power_off_latency_ns = OFF_LATENCY_SLOW,
>>>>         }
>>>> };
>>>>
>>>
>>> I am trying to use your patches in fashioning a genpd for the cpu
>>> domain.
>>>
>>> The cpus are part of a power domain that can be powered off when the
>>> cpus are powered off as part of the cpuidle. One of the biggest power
>>> savings comes from powering off the domain completely. However, powering
>>> off the domain involves flushing of caches (possibly) and turning off
>>> some regulators and peripheral hardware. The power benefit is only
>>> realized when the domain remains powered off for a certain period of
>>> time, otherwise the overhead of powering on/off would add up to the
>>> ineffeciency in the system.
>>>
>>> So a governor that determines the idle state of the domain has two
>>> things that needs to match, the time available to power off the domain
>>> (which we can get from timer wheel) and the other residency as dictated
>>> by the platform.
>>>
>>> I was wondering if we could match the idle state definition of the
>>> domain with that of the cpu, which has a precedence in the kernel. The
>>> idle state of the cpu [1] is a superset of the idle state definition you
>>> have above.  That way a shim layer could pick up domain idle states from
>>> DT and pass the states to pm_genpd_init(). The use of these values could
>>> depend on the genpd governor.
>>
>>
>> IMO, we need to keep the domain latency descriptions separate from the
>> the devices within those domains, which could be CPUs or any other form
>> of device.
>
>
> Sure, the devices could be any. All I am saying is the format of idle
> state definitions could be the same (same as cpu). There is a good
> value in specifying the residency of an idle state in addition to the
> the enter and exit latency.
>
>> It should be the job of the governor then to look at the domain latency
>> along with latencies of the other devices in the domain to make its
>> decision.
>
>
> Agreed and thats how it should be. In all probablility, a cpu-domain
> governor would need a residency value of the domain idle state to best
> determine the idle state of the domain.
>
> If we have not stored the residency in struct genpd_power_state, then
> the governor would not have a good way to make that decision.
>
> I am not ready yet with my patches, but feel free to browse through my
> working tree at [1] to see how I plan to use this patchset.


I took a quick look at the tree you mentioned and i have a couple of
doubts about
the concepts. i will list what i understood so maybe you can comment
if im wrong:

- genpd  cpu-domain governor is not meant to replace the cpuidle governor.
right?

- the idea is to tie the cpuidle states to the cpu-domain states. there
would be a c-state for doing a cluster off, with the off/on latencies
and min residency
values that would represent the cost of turning the cluster off.

- the cpuidle governor (menu governor)has some extra logic to decide the idle
state aside from the exit latencies and target residency (eg, buckets,
performance multiplier).
Even if the next wakeup, latencies and target residencies justify going to a
deep c-state, the governor can decide to demote that state based on the
recent history. im not sure were the line is between the governor taking a
c state decision and genpd taking another one, I feel the cpu-domain
governor will not always respect the same state decision as that the cpuidle
governor may have taken, and i guess there should not be two places
were the decision
is taken. (but again,maybe i got  the idea wrong)

- the cpu_pm_enter call in cpuidle, which will notify of the intention to
suspend, happens before the actual suspend. when the genpd gets the
notifications
you will call pm_runtime_put_sync(dev) for the cpu which will potentially
turn off the cluster using the genpd-off callback (regulators..etc...),
but the cpu is not yet be suspended right?

-maybe it was discussed before, But i think that if the cpuidle governor
would save the time a cpu entered in a particular state, we could add
a .validate callback
in the menu governor that would take the a cpumask of the cpus in the cluster
and would compare the time each cpu entered a c-state against the predicted
time to get how much "remaining time" is "probably" left. That way we could
know if a cluster off i worth it or not, and it would use the inteligence
allready available in the cpuidle governor.


Regards
Axel


>
> Thanks,
> Lina
>
> [1].
> https://git.linaro.org/people/lina.iyer/linux-next.git/shortlog/refs/heads/genpd-8916-3

Re: [PATCH v7 2/5] PM / Domains: core changes for multiple states

[Lina Iyer]

On Mon, May 11 2015 at 09:53 -0600, Axel Haslam wrote:
>Hi Lina,
>
>+ Bartosz who was interested in the patches too.
>
>On Sat, May 9, 2015 at 12:31 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Fri, May 08 2015 at 14:53 -0600, Kevin Hilman wrote:
>>>
>>> Lina Iyer <lina.iyer@linaro.org> writes:
>>>
>>>> On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote:
>>>>>
>>>>> From: Axel Haslam <ahaslam+renesas@baylibre.com>
>>>>>
>>>>> Add the core changes to be able to declare multiple states.
>>>>> When trying to set a power domain to off, genpd will be able
>>>>> to choose from an array of states declared by the platform.
>>>>>
>>>>> The power on and off latencies are now tied to a state.
>>>>>
>>>>> States should be declared in ascending order from shallowest
>>>>> to deepest, deepest meaning the state which takes longer to
>>>>> enter and exit.
>>>>>
>>>>> the power_off and power_on function can use the 'state_idx'
>>>>> field of the generic_pm_domain structure, to distinguish between
>>>>> the different states and act accordingly.
>>>>>
>>>>> - if the genpd is initially off, the user should set the state_idx
>>>>>   field when registering the genpd.
>>>>>
>>>>> - if no states are passed to pm_genpd_init, a single OFF
>>>>>   state with no latency is assumed.
>>>>>
>>>>> Example:
>>>>>
>>>>> static int pd1_power_on(struct generic_pm_domain *domain)
>>>>> {
>>>>>         /* domain->state_idx = state the domain is coming from */
>>>>> }
>>>>>
>>>>> static int pd1_power_off(struct generic_pm_domain *domain)
>>>>> {
>>>>>         /* domain->state_idx = desired powered off state */
>>>>> }
>>>>>
>>>>> const struct genpd_power_state pd_states[] = {
>>>>>         {
>>>>>                 .name = "RET",
>>>>>                 .power_on_latency_ns = ON_LATENCY_FAST,
>>>>>                 .power_off_latency_ns = OFF_LATENCY_FAST,
>>>>>         },
>>>>>         {
>>>>>                 .name = "DEEP_RET",
>>>>>                 .power_on_latency_ns = ON_LATENCY_MED,
>>>>>                 .power_off_latency_ns = OFF_LATENCY_MED,
>>>>>         },
>>>>>         {
>>>>>                 .name = "OFF",
>>>>>                 .power_on_latency_ns = ON_LATENCY_SLOW,
>>>>>                 .power_off_latency_ns = OFF_LATENCY_SLOW,
>>>>>         }
>>>>> };
>>>>>
>>>>
>>>> I am trying to use your patches in fashioning a genpd for the cpu
>>>> domain.
>>>>
>>>> The cpus are part of a power domain that can be powered off when the
>>>> cpus are powered off as part of the cpuidle. One of the biggest power
>>>> savings comes from powering off the domain completely. However, powering
>>>> off the domain involves flushing of caches (possibly) and turning off
>>>> some regulators and peripheral hardware. The power benefit is only
>>>> realized when the domain remains powered off for a certain period of
>>>> time, otherwise the overhead of powering on/off would add up to the
>>>> ineffeciency in the system.
>>>>
>>>> So a governor that determines the idle state of the domain has two
>>>> things that needs to match, the time available to power off the domain
>>>> (which we can get from timer wheel) and the other residency as dictated
>>>> by the platform.
>>>>
>>>> I was wondering if we could match the idle state definition of the
>>>> domain with that of the cpu, which has a precedence in the kernel. The
>>>> idle state of the cpu [1] is a superset of the idle state definition you
>>>> have above.  That way a shim layer could pick up domain idle states from
>>>> DT and pass the states to pm_genpd_init(). The use of these values could
>>>> depend on the genpd governor.
>>>
>>>
>>> IMO, we need to keep the domain latency descriptions separate from the
>>> the devices within those domains, which could be CPUs or any other form
>>> of device.
>>
>>
>> Sure, the devices could be any. All I am saying is the format of idle
>> state definitions could be the same (same as cpu). There is a good
>> value in specifying the residency of an idle state in addition to the
>> the enter and exit latency.
>>
>>> It should be the job of the governor then to look at the domain latency
>>> along with latencies of the other devices in the domain to make its
>>> decision.
>>
>>
>> Agreed and thats how it should be. In all probablility, a cpu-domain
>> governor would need a residency value of the domain idle state to best
>> determine the idle state of the domain.
>>
>> If we have not stored the residency in struct genpd_power_state, then
>> the governor would not have a good way to make that decision.
>>
>> I am not ready yet with my patches, but feel free to browse through my
>> working tree at [1] to see how I plan to use this patchset.
>
>
>I took a quick look at the tree you mentioned and i have a couple of
>doubts about
>the concepts. i will list what i understood so maybe you can comment
>if im wrong:

Hi Axel,

Thanks for looking at my series.

>
>- genpd  cpu-domain governor is not meant to replace the cpuidle governor.
>right?
>
That is correct. I am not replacing the cpuidle governor. What I am
working on is independent of the cpuidle governor.

>- the idea is to tie the cpuidle states to the cpu-domain states. there
>would be a c-state for doing a cluster off, with the off/on latencies
>and min residency
>values that would represent the cost of turning the cluster off.

Not necessarily. The map is not to individual C-States of the cpu, but
only to the power down state of the cpu (the ones which send out CPU_PM
notifications), at this time. The residencies of the individual cpu
c-states are for the cpu only. A similar but independent one could exist
for the cpu-domain is the proposal here. 

>
>- the cpuidle governor (menu governor)has some extra logic to decide the idle
>state aside from the exit latencies and target residency (eg, buckets,
>performance multiplier).
>Even if the next wakeup, latencies and target residencies justify going to a
>deep c-state, the governor can decide to demote that state based on the
>recent history. im not sure were the line is between the governor taking a
>c state decision and genpd taking another one, I feel the cpu-domain
>governor will not always respect the same state decision as that the cpuidle
>governor may have taken, and i guess there should not be two places
>were the decision
>is taken. (but again,maybe i got  the idea wrong)o

Absolutely. The cpus will obey the idle state suggested by the menu or
any other governor. Once a state is chosen, the cpu is expected to enter
the state or bail out if there was a pending interrupt.

CPU-Domain decisions will be based on the cpuidle governor decisions.
For states that CPU_PM notifications are not sent out, we can be sure
that the cpu is not powered off and so the domain would not be powered
off as well. Hence the tie up of the pm_runtime_put_sync() with CPU_PM
notifications.

But take the case where the menu governor decides for each cpu, that it
can power down the cpu. If there is enough time where all cpus are
powered off in the domain, then the domain can safely be powered down,
until the first of the cpus in that domain wakes up. The time between
the last cpu down and the first cpu up is generally a good opportunity
for power saving. Power savings could come from turning off peripheral
hardware, flushing cashes, switching regulators to supply a low load
mode. There is usually a cost benefit table associated with the sleep
time available for the domain vs turning off these components and that
cost benefict analysis in my case is the residency - the sleep time that
the domain needs to sleep in order to reap the benefits of the overhead
in powering off and on the domain while the cpus are sleeping. Its
fairly common for the mobile cpus to sleep tens of milliseconds between
active operations and it is beneficial to flush caches and power the
domain in many such cases.

>
>- the cpu_pm_enter call in cpuidle, which will notify of the intention to
>suspend, happens before the actual suspend. when the genpd gets the
>notifications
>you will call pm_runtime_put_sync(dev) for the cpu which will potentially
>turn off the cluster using the genpd-off callback (regulators..etc...),
>but the cpu is not yet be suspended right?
>

Also correct. Most architectures (ARM especially)that allow powering off
the cpu domain, usually would do it through a peripheral power-controller
that is capable of being triggered when the last cpu in the domain is
powered off. In QCOM SoCs that I am most familiar with, ARM WFI signal
(which means that the processor is clock gated, therefore not running)
from a processor is used to trigger an external power controller for the
cpu. This ensures that there is no race with the cpu actually running
when the power-controller is powering down the cpu on its behalf. The
same WFI signal is also sent to the domain power-controller that can
be configured to power down the caches and turn off regulators etc. On
the way up, the interrupt to the core is trapped to wake up the domain
first (if powered off), before the cpu is resumed.

The CPU_PM notifications do get sent before the cpu is actually powered
off, but is sent at a point, when the decision has been made to power
off the cpu. So the domain decision can be made decisively knowing what
state the cpus are going to be in.

In all likelihood, there might be a pending interrupt for the cpu, when
this domain decision happens, in which case, both the cpu and the domain
would bail out of their idle states.

>-maybe it was discussed before, But i think that if the cpuidle governor
>would save the time a cpu entered in a particular state, we could add
>a .validate callback
>in the menu governor that would take the a cpumask of the cpus in the cluster
>and would compare the time each cpu entered a c-state against the predicted
>time to get how much "remaining time" is "probably" left. That way we could
>know if a cluster off i worth it or not, and it would use the inteligence
>allready available in the cpuidle governor.

Fair point. And that could be ideal. The governor algorithm need not
exist with GenPD and could be handled by the menu governor. I can work 
on that, but since it is external to menu governor decision, I had
chosen to put as a genpd governor. This can be discussed more.

Thanks,
Lina

>> [1].
>> https://git.linaro.org/people/lina.iyer/linux-next.git/shortlog/refs/heads/genpd-8916-3

Re: [PATCH v7 2/5] PM / Domains: core changes for multiple states

[Axel Haslam]

Hi Lina

On Mon, May 11, 2015 at 6:50 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Mon, May 11 2015 at 09:53 -0600, Axel Haslam wrote:
>>
>> Hi Lina,
>>
>> + Bartosz who was interested in the patches too.
>>
>> On Sat, May 9, 2015 at 12:31 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>
>>> On Fri, May 08 2015 at 14:53 -0600, Kevin Hilman wrote:
>>>>
>>>>
>>>> Lina Iyer <lina.iyer@linaro.org> writes:
>>>>
>>>>> On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote:
>>>>>>
>>>>>>
>>>>>> From: Axel Haslam <ahaslam+renesas@baylibre.com>
>>>>>>
>>>>>> Add the core changes to be able to declare multiple states.
>>>>>> When trying to set a power domain to off, genpd will be able
>>>>>> to choose from an array of states declared by the platform.
>>>>>>
>>>>>> The power on and off latencies are now tied to a state.
>>>>>>
>>>>>> States should be declared in ascending order from shallowest
>>>>>> to deepest, deepest meaning the state which takes longer to
>>>>>> enter and exit.
>>>>>>
>>>>>> the power_off and power_on function can use the 'state_idx'
>>>>>> field of the generic_pm_domain structure, to distinguish between
>>>>>> the different states and act accordingly.
>>>>>>
>>>>>> - if the genpd is initially off, the user should set the state_idx
>>>>>>   field when registering the genpd.
>>>>>>
>>>>>> - if no states are passed to pm_genpd_init, a single OFF
>>>>>>   state with no latency is assumed.
>>>>>>
>>>>>> Example:
>>>>>>
>>>>>> static int pd1_power_on(struct generic_pm_domain *domain)
>>>>>> {
>>>>>>         /* domain->state_idx = state the domain is coming from */
>>>>>> }
>>>>>>
>>>>>> static int pd1_power_off(struct generic_pm_domain *domain)
>>>>>> {
>>>>>>         /* domain->state_idx = desired powered off state */
>>>>>> }
>>>>>>
>>>>>> const struct genpd_power_state pd_states[] = {
>>>>>>         {
>>>>>>                 .name = "RET",
>>>>>>                 .power_on_latency_ns = ON_LATENCY_FAST,
>>>>>>                 .power_off_latency_ns = OFF_LATENCY_FAST,
>>>>>>         },
>>>>>>         {
>>>>>>                 .name = "DEEP_RET",
>>>>>>                 .power_on_latency_ns = ON_LATENCY_MED,
>>>>>>                 .power_off_latency_ns = OFF_LATENCY_MED,
>>>>>>         },
>>>>>>         {
>>>>>>                 .name = "OFF",
>>>>>>                 .power_on_latency_ns = ON_LATENCY_SLOW,
>>>>>>                 .power_off_latency_ns = OFF_LATENCY_SLOW,
>>>>>>         }
>>>>>> };
>>>>>>
>>>>>
>>>>> I am trying to use your patches in fashioning a genpd for the cpu
>>>>> domain.
>>>>>
>>>>> The cpus are part of a power domain that can be powered off when the
>>>>> cpus are powered off as part of the cpuidle. One of the biggest power
>>>>> savings comes from powering off the domain completely. However,
>>>>> powering
>>>>> off the domain involves flushing of caches (possibly) and turning off
>>>>> some regulators and peripheral hardware. The power benefit is only
>>>>> realized when the domain remains powered off for a certain period of
>>>>> time, otherwise the overhead of powering on/off would add up to the
>>>>> ineffeciency in the system.
>>>>>
>>>>> So a governor that determines the idle state of the domain has two
>>>>> things that needs to match, the time available to power off the domain
>>>>> (which we can get from timer wheel) and the other residency as dictated
>>>>> by the platform.
>>>>>
>>>>> I was wondering if we could match the idle state definition of the
>>>>> domain with that of the cpu, which has a precedence in the kernel. The
>>>>> idle state of the cpu [1] is a superset of the idle state definition
>>>>> you
>>>>> have above.  That way a shim layer could pick up domain idle states
>>>>> from
>>>>> DT and pass the states to pm_genpd_init(). The use of these values
>>>>> could
>>>>> depend on the genpd governor.
>>>>
>>>>
>>>>
>>>> IMO, we need to keep the domain latency descriptions separate from the
>>>> the devices within those domains, which could be CPUs or any other form
>>>> of device.
>>>
>>>
>>>
>>> Sure, the devices could be any. All I am saying is the format of idle
>>> state definitions could be the same (same as cpu). There is a good
>>> value in specifying the residency of an idle state in addition to the
>>> the enter and exit latency.
>>>
>>>> It should be the job of the governor then to look at the domain latency
>>>> along with latencies of the other devices in the domain to make its
>>>> decision.
>>>
>>>
>>>
>>> Agreed and thats how it should be. In all probablility, a cpu-domain
>>> governor would need a residency value of the domain idle state to best
>>> determine the idle state of the domain.
>>>
>>> If we have not stored the residency in struct genpd_power_state, then
>>> the governor would not have a good way to make that decision.
>>>
>>> I am not ready yet with my patches, but feel free to browse through my
>>> working tree at [1] to see how I plan to use this patchset.
>>
>>
>>
>> I took a quick look at the tree you mentioned and i have a couple of
>> doubts about
>> the concepts. i will list what i understood so maybe you can comment
>> if im wrong:
>
>
> Hi Axel,
>
> Thanks for looking at my series.
>
>>
>> - genpd  cpu-domain governor is not meant to replace the cpuidle governor.
>> right?
>>
> That is correct. I am not replacing the cpuidle governor. What I am
> working on is independent of the cpuidle governor.
>
>> - the idea is to tie the cpuidle states to the cpu-domain states. there
>> would be a c-state for doing a cluster off, with the off/on latencies
>> and min residency
>> values that would represent the cost of turning the cluster off.
>
>
> Not necessarily. The map is not to individual C-States of the cpu, but
> only to the power down state of the cpu (the ones which send out CPU_PM
> notifications), at this time. The residencies of the individual cpu
> c-states are for the cpu only. A similar but independent one could exist
> for the cpu-domain is the proposal here.


Thanks for the clarification, i think most of my confusion is that
i had imagined doing a cluster off as simply a more expensive c-state.

>>
>>
>> - the cpuidle governor (menu governor)has some extra logic to decide the
>> idle
>> state aside from the exit latencies and target residency (eg, buckets,
>> performance multiplier).
>> Even if the next wakeup, latencies and target residencies justify going to
>> a
>> deep c-state, the governor can decide to demote that state based on the
>> recent history. im not sure were the line is between the governor taking a
>> c state decision and genpd taking another one, I feel the cpu-domain
>> governor will not always respect the same state decision as that the
>> cpuidle
>> governor may have taken, and i guess there should not be two places
>> were the decision
>> is taken. (but again,maybe i got  the idea wrong)o
>
>
> Absolutely. The cpus will obey the idle state suggested by the menu or
> any other governor. Once a state is chosen, the cpu is expected to enter
> the state or bail out if there was a pending interrupt.
>
> CPU-Domain decisions will be based on the cpuidle governor decisions.
> For states that CPU_PM notifications are not sent out, we can be sure
> that the cpu is not powered off and so the domain would not be powered
> off as well. Hence the tie up of the pm_runtime_put_sync() with CPU_PM
> notifications.
>
> But take the case where the menu governor decides for each cpu, that it
> can power down the cpu. If there is enough time where all cpus are
> powered off in the domain, then the domain can safely be powered down,
> until the first of the cpus in that domain wakes up. The time between
> the last cpu down and the first cpu up is generally a good opportunity
> for power saving. Power savings could come from turning off peripheral
> hardware, flushing cashes, switching regulators to supply a low load
> mode. There is usually a cost benefit table associated with the sleep
> time available for the domain vs turning off these components and that
> cost benefict analysis in my case is the residency - the sleep time that
> the domain needs to sleep in order to reap the benefits of the overhead
> in powering off and on the domain while the cpus are sleeping. Its
> fairly common for the mobile cpus to sleep tens of milliseconds between
> active operations and it is beneficial to flush caches and power the
> domain in many such cases.
>
>>
>> - the cpu_pm_enter call in cpuidle, which will notify of the intention to
>> suspend, happens before the actual suspend. when the genpd gets the
>> notifications
>> you will call pm_runtime_put_sync(dev) for the cpu which will potentially
>> turn off the cluster using the genpd-off callback (regulators..etc...),
>> but the cpu is not yet be suspended right?
>>
>
> Also correct. Most architectures (ARM especially)that allow powering off
> the cpu domain, usually would do it through a peripheral power-controller
> that is capable of being triggered when the last cpu in the domain is
> powered off. In QCOM SoCs that I am most familiar with, ARM WFI signal
> (which means that the processor is clock gated, therefore not running)
> from a processor is used to trigger an external power controller for the
> cpu. This ensures that there is no race with the cpu actually running
> when the power-controller is powering down the cpu on its behalf. The
> same WFI signal is also sent to the domain power-controller that can
> be configured to power down the caches and turn off regulators etc. On
> the way up, the interrupt to the core is trapped to wake up the domain
> first (if powered off), before the cpu is resumed.
>
> The CPU_PM notifications do get sent before the cpu is actually powered
> off, but is sent at a point, when the decision has been made to power
> off the cpu. So the domain decision can be made decisively knowing what
> state the cpus are going to be in.
>
> In all likelihood, there might be a pending interrupt for the cpu, when
> this domain decision happens, in which case, both the cpu and the domain
> would bail out of their idle states.
>
>> -maybe it was discussed before, But i think that if the cpuidle governor
>> would save the time a cpu entered in a particular state, we could add
>> a .validate callback
>> in the menu governor that would take the a cpumask of the cpus in the
>> cluster
>> and would compare the time each cpu entered a c-state against the
>> predicted
>> time to get how much "remaining time" is "probably" left. That way we
>> could
>> know if a cluster off i worth it or not, and it would use the inteligence
>> allready available in the cpuidle governor.
>
>
> Fair point. And that could be ideal. The governor algorithm need not
> exist with GenPD and could be handled by the menu governor. I can work on
> that, but since it is external to menu governor decision, I had
> chosen to put as a genpd governor. This can be discussed more.

 maybe it can be usefull to start a separate thread about this discussion.
i can post a RFC to start the thread..

Regards
Axel



>
> Thanks,
> Lina
>
>>> [1].
>>>
>>> https://git.linaro.org/people/lina.iyer/linux-next.git/shortlog/refs/heads/genpd-8916-3

Re: [PATCH v7 2/5] PM / Domains: core changes for multiple states

[Lina Iyer]

On Tue, May 12 2015 at 05:58 -0600, Axel Haslam wrote:
>On Mon, May 11, 2015 at 6:50 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Mon, May 11 2015 at 09:53 -0600, Axel Haslam wrote:
>>> + Bartosz who was interested in the patches too.
>>> On Sat, May 9, 2015 at 12:31 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>> On Fri, May 08 2015 at 14:53 -0600, Kevin Hilman wrote:
>>>>> Lina Iyer <lina.iyer@linaro.org> writes:
>>>>>> On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote:

[...]

> maybe it can be usefull to start a separate thread about this discussion.
>i can post a RFC to start the thread..
>

I was planning to post my patches soon. I am sure plenty of
conversations would follow :)

I think what started off this discussion (residency in idle states)
could probably be submitted separately.


Thanks,
Lina

Re: [PATCH v7 1/5] PM / Domains: prepare for multiple states

[Lina Iyer]

On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote:
>From: Axel Haslam <ahaslam+renesas@baylibre.com>
>
>prepare generic power domain init function parameters
>to accept a pointer to the states structure that describes
>the possible states that a power domain can enter.
>
>There is no functional change, as support for multiple
>domains will be added in subsequent patches.
>
>Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>

[...]

>diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>index 681ccb0..6b4802e 100644
>--- a/include/linux/pm_domain.h
>+++ b/include/linux/pm_domain.h
>@@ -46,6 +46,12 @@ struct gpd_cpuidle_data {
> 	struct cpuidle_state *idle_state;
> };
>
>+struct genpd_power_state {
>+	char *name;
>+	s64 power_off_latency_ns;
>+	s64 power_on_latency_ns;
>+};
>+

I know you carried this over from the existing code, but any reason why
this should be signed?

-- Lina

Re: [PATCH v7 1/5] PM / Domains: prepare for multiple states

[Axel Haslam]

Hi Lina,

On Tue, May 12, 2015 at 5:37 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote:
>>
>> From: Axel Haslam <ahaslam+renesas@baylibre.com>
>>
>> prepare generic power domain init function parameters
>> to accept a pointer to the states structure that describes
>> the possible states that a power domain can enter.
>>
>> There is no functional change, as support for multiple
>> domains will be added in subsequent patches.
>>
>> Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>
>
>
> [...]
>
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 681ccb0..6b4802e 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -46,6 +46,12 @@ struct gpd_cpuidle_data {
>>         struct cpuidle_state *idle_state;
>> };
>>
>> +struct genpd_power_state {
>> +       char *name;
>> +       s64 power_off_latency_ns;
>> +       s64 power_on_latency_ns;
>> +};
>> +
>
>
> I know you carried this over from the existing code, but any reason why
> this should be signed?
>

correct, i just carried this values over as they were originally.
i suppose they can be changed to u64.

maybe a separate cleanup patch,  as it would make sense to change
some other s64's in domain.c, like "s64 elapsed_ns;"




> -- Lina