All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC v5 0/8]  genpd multiple states v5
@ 2015-04-24 17:35 ahaslam
  2015-04-24 17:35 ` [RFC v5 1/8] PM / Domains: structure changes for multiple states ahaslam
                   ` (7 more replies)
  0 siblings, 8 replies; 15+ messages in thread
From: ahaslam @ 2015-04-24 17:35 UTC (permalink / raw)
  To: ulf.hansson, khilman, k.kozlowski.k, geert, rjw
  Cc: bcousson, linux-pm, Axel Haslam

From: Axel Haslam <ahaslam@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 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 (8):
  PM / Domains: structure changes for multiple states
  PM / Domains: select deepest state
  ARM: s3c64xx: pm: Convert to multiple states
  ARM: exynos: pm: Convert to multiple states
  ARM: r8a7779: pm: Convert to multiple states
  ARM: rmobile: pm: Convert to multiple states
  ARM: ux500: pm: Convert to multiple states
  PM / Domains: remove old power on/off callbacks.

 arch/arm/mach-exynos/pm_domains.c    |  8 ++++
 arch/arm/mach-s3c64xx/pm.c           | 22 +++++++++++
 arch/arm/mach-shmobile/pm-r8a7779.c  |  8 ++++
 arch/arm/mach-shmobile/pm-rmobile.c  |  8 ++++
 arch/arm/mach-ux500/pm_domains.c     |  8 ++++
 drivers/base/power/domain.c          | 75 +++++++++++++++++++++++++++++++++---
 drivers/base/power/domain_governor.c | 71 +++++++++++++++++++++-------------
 include/linux/pm_domain.h            | 12 +++++-
 8 files changed, 178 insertions(+), 34 deletions(-)

-- 
1.9.1


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

* [RFC v5 1/8] PM / Domains: structure changes for multiple states
  2015-04-24 17:35 [RFC v5 0/8] genpd multiple states v5 ahaslam
@ 2015-04-24 17:35 ` ahaslam
  2015-04-26  8:42   ` Geert Uytterhoeven
  2015-04-24 17:35 ` [RFC v5 2/8] PM / Domains: select deepest state ahaslam
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 15+ messages in thread
From: ahaslam @ 2015-04-24 17:35 UTC (permalink / raw)
  To: ulf.hansson, khilman, k.kozlowski.k, geert, rjw
  Cc: bcousson, linux-pm, Axel Haslam

From: Axel Haslam <ahaslam@baylibre.com>

Add the structure 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 particular 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.

An genpd with multiple states would look something like:

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

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

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",
	.states = pd_states,
	.state_count = ARRAY_SIZE(pd_states),
	.state_idx = 2, /* needed if domain is off at init. */
	.power_on = pd1_power_on,
	.power_off = pd1_power_off,
	...
};

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 drivers/base/power/domain.c          | 102 +++++++++++++++++++++++++++++++----
 drivers/base/power/domain_governor.c |  15 ++++--
 include/linux/pm_domain.h            |  10 ++++
 3 files changed, 115 insertions(+), 12 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 45937f8..b519926 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -141,12 +141,17 @@ static void genpd_set_active(struct generic_pm_domain *genpd)
 
 static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
 {
+	int state_idx = genpd->state_idx;
 	s64 usecs64;
 
 	if (!genpd->cpuidle_data)
 		return;
 
-	usecs64 = genpd->power_on_latency_ns;
+	if (genpd->state_count == 0)
+		usecs64 = genpd->power_on_latency_ns;
+	else
+		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;
@@ -154,6 +159,7 @@ static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
 
 static int genpd_power_on(struct generic_pm_domain *genpd)
 {
+	int state_idx = genpd->state_idx;
 	ktime_t time_start;
 	s64 elapsed_ns;
 	int ret;
@@ -167,10 +173,16 @@ 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)
-		return ret;
+	if (genpd->state_count == 0) {
+		if (elapsed_ns <= genpd->power_on_latency_ns)
+			return ret;
+		genpd->power_on_latency_ns = elapsed_ns;
+	} else {
+		if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
+			return ret;
+		genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
+	}
 
-	genpd->power_on_latency_ns = elapsed_ns;
 	genpd->max_off_time_changed = true;
 	genpd_recalc_cpu_exit_latency(genpd);
 	pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
@@ -181,6 +193,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
 
 static int genpd_power_off(struct generic_pm_domain *genpd)
 {
+	int state_idx = genpd->state_idx;
 	ktime_t time_start;
 	s64 elapsed_ns;
 	int ret;
@@ -194,10 +207,15 @@ 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)
-		return ret;
-
-	genpd->power_off_latency_ns = elapsed_ns;
+	if (genpd->state_count == 0) {
+		if (elapsed_ns <= genpd->power_off_latency_ns)
+			return ret;
+		genpd->power_off_latency_ns = elapsed_ns;
+	} else {
+		if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns)
+			return ret;
+		genpd->states[state_idx].power_off_latency_ns = elapsed_ns;
+	}
 	genpd->max_off_time_changed = true;
 	pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
 		genpd->name, "off", elapsed_ns);
@@ -486,6 +504,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 	struct pm_domain_data *pdd;
 	struct gpd_link *link;
 	unsigned int not_suspended;
+	int state_idx;
 	int ret = 0;
 
  start:
@@ -538,6 +557,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 
 	genpd->status = GPD_STATE_BUSY;
 	genpd->poweroff_task = current;
+	state_idx = genpd->state_idx;
 
 	list_for_each_entry_reverse(pdd, &genpd->dev_list, list_node) {
 		ret = atomic_read(&genpd->sd_count) == 0 ?
@@ -1438,6 +1458,54 @@ static void genpd_free_dev_data(struct device *dev,
 	kfree(gpd_data);
 	dev_pm_put_subsys_data(dev);
 }
+/**
+ * genpd_alloc_states_data - Allocate and copy state parameters
+ * @genpd: PM domain use to copy and allocate data
+ */
+static struct genpd_power_state *genpd_alloc_states_data(
+			struct generic_pm_domain *genpd)
+{
+	struct genpd_power_state *states;
+	size_t states_size;
+	int i;
+
+	if (IS_ERR_OR_NULL(genpd))
+		return NULL;
+
+	if (!genpd->states) {
+		pr_err("%s Null state pointer\n", genpd->name);
+		return NULL;
+	}
+
+	if (genpd->state_count < 1) {
+		pr_err("%s Invalid number of states %d\n",
+			genpd->name, genpd->state_count);
+		return NULL;
+	}
+
+	/* Allocate the local memory to keep the states for this genpd */
+	states_size = sizeof(*states) * genpd->state_count;
+	states = kzalloc(states_size, GFP_KERNEL);
+	if (!states) {
+		pr_err("%s states Allocation error\n", genpd->name);
+		return NULL;
+	}
+
+	/* Copy the parameters passed to the allocated structure */
+	for (i = 0; i < genpd->state_count; i++) {
+		states[i].power_off_latency_ns =
+			genpd->states[i].power_off_latency_ns;
+		states[i].power_on_latency_ns =
+			genpd->states[i].power_on_latency_ns;
+	}
+
+#ifdef CONFIG_PM_ADVANCED_DEBUG
+	/* State name is used for debug. Avoid unnecessary allocations */
+	for (i = 0; i < genpd->state_count; i++)
+		states[i].name = kstrdup(genpd->states[i].name, GFP_KERNEL);
+#endif
+	return states;
+}
 
 /**
  * __pm_genpd_add_device - Add a device to an I/O PM domain.
@@ -1863,6 +1931,13 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 	if (IS_ERR_OR_NULL(genpd))
 		return;
 
+	if (genpd->state_count > 0) {
+		/* Copy the state data to allocated memory */
+		genpd->states = genpd_alloc_states_data(genpd);
+		if (!genpd->states)
+			return;
+	}
+
 	INIT_LIST_HEAD(&genpd->master_links);
 	INIT_LIST_HEAD(&genpd->slave_links);
 	INIT_LIST_HEAD(&genpd->dev_list);
@@ -2251,6 +2326,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
 		[GPD_STATE_REPEAT] = "off-in-progress",
 		[GPD_STATE_POWER_OFF] = "off"
 	};
+	int state_idx = genpd->state_idx;
 	struct pm_domain_data *pm_data;
 	const char *kobj_path;
 	struct gpd_link *link;
@@ -2262,7 +2338,15 @@ 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]);
+
+	if (genpd->state_count == 0)
+		seq_printf(s, "%-30s  %-15s  ",
+			genpd->name, status_lookup[genpd->status]);
+	else
+		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
diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index 2a4154a..9d74885 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;
+	if (genpd->state_count == 0)
+		off_on_time_ns = genpd->power_off_latency_ns +
+			genpd->power_on_latency_ns;
+	else
+		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
@@ -216,7 +220,12 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
 	 * time and the time needed to turn the domain on is the maximum
 	 * theoretical time this domain can spend in the "off" state.
 	 */
-	genpd->max_off_time_ns = min_off_time_ns - genpd->power_on_latency_ns;
+	if (genpd->state_count == 0)
+		genpd->max_off_time_ns =
+			min_off_time_ns - genpd->power_on_latency_ns;
+	else
+		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 080e778..25082f6 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 */
@@ -80,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;
+	int state_count; /* number of states */
+	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


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

* [RFC v5 2/8] PM / Domains: select deepest state
  2015-04-24 17:35 [RFC v5 0/8] genpd multiple states v5 ahaslam
  2015-04-24 17:35 ` [RFC v5 1/8] PM / Domains: structure changes for multiple states ahaslam
@ 2015-04-24 17:35 ` ahaslam
  2015-04-24 17:35 ` [RFC v5 3/8] ARM: s3c64xx: pm: Convert to multiple states ahaslam
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: ahaslam @ 2015-04-24 17:35 UTC (permalink / raw)
  To: ulf.hansson, khilman, k.kozlowski.k, geert, rjw
  Cc: bcousson, linux-pm, Axel Haslam

From: Axel Haslam <ahaslam@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@baylibre.com>
---
 drivers/base/power/domain_governor.c | 77 ++++++++++++++++++++++++------------
 1 file changed, 52 insertions(+), 25 deletions(-)

diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index 9d74885..061c711 100644
--- a/drivers/base/power/domain_governor.c
+++ b/drivers/base/power/domain_governor.c
@@ -97,7 +97,7 @@ 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, int state)
 {
 	struct generic_pm_domain *genpd = pd_to_genpd(pd);
 	struct gpd_link *link;
@@ -105,31 +105,12 @@ 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;
-	}
-
 	if (genpd->state_count == 0)
 		off_on_time_ns = genpd->power_off_latency_ns +
 			genpd->power_on_latency_ns;
 	else
-		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 +186,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
@@ -225,10 +204,58 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
 			min_off_time_ns - genpd->power_on_latency_ns;
 	else
 		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);
+	int last_state_idx = genpd->state_count - 1;
+	struct gpd_link *link;
+	bool retval = false;
+	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 */
+	if (genpd->state_count == 0) {
+		/*
+		 * there are no states. power_down_ok_for_state will use
+		 * the stateless values, and ignore the state argument.
+		 */
+		retval = power_down_ok_for_state(pd, 0);
+	} else {
+		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


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

* [RFC v5 3/8] ARM: s3c64xx: pm: Convert to multiple states
  2015-04-24 17:35 [RFC v5 0/8] genpd multiple states v5 ahaslam
  2015-04-24 17:35 ` [RFC v5 1/8] PM / Domains: structure changes for multiple states ahaslam
  2015-04-24 17:35 ` [RFC v5 2/8] PM / Domains: select deepest state ahaslam
@ 2015-04-24 17:35 ` ahaslam
  2015-04-24 17:35 ` [RFC v5 4/8] ARM: exynos: " ahaslam
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: ahaslam @ 2015-04-24 17:35 UTC (permalink / raw)
  To: ulf.hansson, khilman, k.kozlowski.k, geert, rjw
  Cc: bcousson, linux-pm, Axel Haslam

From: Axel Haslam <ahaslam@baylibre.com>

The generic power domain framework added structure changes
to support multiple intermediate states when powering off a domain.

These changes are needed to prevent compilation breaks with
the new structures.

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 arch/arm/mach-s3c64xx/pm.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm/mach-s3c64xx/pm.c b/arch/arm/mach-s3c64xx/pm.c
index aaf7bea..446c5bd 100644
--- a/arch/arm/mach-s3c64xx/pm.c
+++ b/arch/arm/mach-s3c64xx/pm.c
@@ -85,12 +85,20 @@ static int s3c64xx_pd_on(struct generic_pm_domain *domain)
 	return 0;
 }
 
+struct genpd_power_state s3c64xx_pm_states[] = {
+	{
+		.name = "OFF",
+	}
+};
+
 static struct s3c64xx_pm_domain s3c64xx_pm_irom = {
 	.name = "IROM",
 	.ena = S3C64XX_NORMALCFG_IROM_ON,
 	.pd = {
 		.power_off = s3c64xx_pd_off,
 		.power_on = s3c64xx_pd_on,
+		.states = s3c64xx_pm_states,
+		.state_count = ARRAY_SIZE(s3c64xx_pm_states),
 	},
 };
 
@@ -101,6 +109,8 @@ static struct s3c64xx_pm_domain s3c64xx_pm_etm = {
 	.pd = {
 		.power_off = s3c64xx_pd_off,
 		.power_on = s3c64xx_pd_on,
+		.states = s3c64xx_pm_states,
+		.state_count = ARRAY_SIZE(s3c64xx_pm_states),
 	},
 };
 
@@ -111,6 +121,8 @@ static struct s3c64xx_pm_domain s3c64xx_pm_s = {
 	.pd = {
 		.power_off = s3c64xx_pd_off,
 		.power_on = s3c64xx_pd_on,
+		.states = s3c64xx_pm_states,
+		.state_count = ARRAY_SIZE(s3c64xx_pm_states),
 	},
 };
 
@@ -121,6 +133,8 @@ static struct s3c64xx_pm_domain s3c64xx_pm_f = {
 	.pd = {
 		.power_off = s3c64xx_pd_off,
 		.power_on = s3c64xx_pd_on,
+		.states = s3c64xx_pm_states,
+		.state_count = ARRAY_SIZE(s3c64xx_pm_states),
 	},
 };
 
@@ -131,6 +145,8 @@ static struct s3c64xx_pm_domain s3c64xx_pm_p = {
 	.pd = {
 		.power_off = s3c64xx_pd_off,
 		.power_on = s3c64xx_pd_on,
+		.states = s3c64xx_pm_states,
+		.state_count = ARRAY_SIZE(s3c64xx_pm_states),
 	},
 };
 
@@ -141,6 +157,8 @@ static struct s3c64xx_pm_domain s3c64xx_pm_i = {
 	.pd = {
 		.power_off = s3c64xx_pd_off,
 		.power_on = s3c64xx_pd_on,
+		.states = s3c64xx_pm_states,
+		.state_count = ARRAY_SIZE(s3c64xx_pm_states),
 	},
 };
 
@@ -150,6 +168,8 @@ static struct s3c64xx_pm_domain s3c64xx_pm_g = {
 	.pd = {
 		.power_off = s3c64xx_pd_off,
 		.power_on = s3c64xx_pd_on,
+		.states = s3c64xx_pm_states,
+		.state_count = ARRAY_SIZE(s3c64xx_pm_states),
 	},
 };
 
@@ -160,6 +180,8 @@ static struct s3c64xx_pm_domain s3c64xx_pm_v = {
 	.pd = {
 		.power_off = s3c64xx_pd_off,
 		.power_on = s3c64xx_pd_on,
+		.states = s3c64xx_pm_states,
+		.state_count = ARRAY_SIZE(s3c64xx_pm_states),
 	},
 };
 
-- 
1.9.1


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

* [RFC v5 4/8] ARM: exynos: pm: Convert to multiple states
  2015-04-24 17:35 [RFC v5 0/8] genpd multiple states v5 ahaslam
                   ` (2 preceding siblings ...)
  2015-04-24 17:35 ` [RFC v5 3/8] ARM: s3c64xx: pm: Convert to multiple states ahaslam
@ 2015-04-24 17:35 ` ahaslam
  2015-04-24 17:35 ` [RFC v5 5/8] ARM: r8a7779: " ahaslam
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: ahaslam @ 2015-04-24 17:35 UTC (permalink / raw)
  To: ulf.hansson, khilman, k.kozlowski.k, geert, rjw
  Cc: bcousson, linux-pm, Axel Haslam

From: Axel Haslam <ahaslam@baylibre.com>

The generic power domain framework added structure changes
to support multiple intermediate states when powering off a domain.

These changes are needed to prevent compilation breaks with
the new structures.

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 arch/arm/mach-exynos/pm_domains.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
index 37266a8..87e67df 100644
--- a/arch/arm/mach-exynos/pm_domains.c
+++ b/arch/arm/mach-exynos/pm_domains.c
@@ -105,6 +105,12 @@ static int exynos_pd_power_off(struct generic_pm_domain *domain)
 	return exynos_pd_power(domain, false);
 }
 
+struct genpd_power_state exynos_pm_states[] = {
+	{
+		.name = "OFF",
+	}
+};
+
 static __init int exynos4_pm_init_power_domain(void)
 {
 	struct platform_device *pdev;
@@ -130,6 +136,8 @@ static __init int exynos4_pm_init_power_domain(void)
 		pd->base = of_iomap(np, 0);
 		pd->pd.power_off = exynos_pd_power_off;
 		pd->pd.power_on = exynos_pd_power_on;
+		pd->pd.states = exynos_pm_states;
+		pd->pd.state_count = ARRAY_SIZE(exynos_pm_states);
 
 		pd->oscclk = clk_get(dev, "oscclk");
 		if (IS_ERR(pd->oscclk))
-- 
1.9.1


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

* [RFC v5 5/8] ARM: r8a7779: pm: Convert to multiple states
  2015-04-24 17:35 [RFC v5 0/8] genpd multiple states v5 ahaslam
                   ` (3 preceding siblings ...)
  2015-04-24 17:35 ` [RFC v5 4/8] ARM: exynos: " ahaslam
@ 2015-04-24 17:35 ` ahaslam
  2015-04-26  8:43   ` Geert Uytterhoeven
  2015-04-24 17:35 ` [RFC v5 6/8] ARM: rmobile: " ahaslam
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 15+ messages in thread
From: ahaslam @ 2015-04-24 17:35 UTC (permalink / raw)
  To: ulf.hansson, khilman, k.kozlowski.k, geert, rjw
  Cc: bcousson, linux-pm, Axel Haslam

From: Axel Haslam <ahaslam@baylibre.com>

The generic power domain framework added structure changes
to support multiple intermediate states when powering off a domain.

These changes are needed to prevent compilation breaks with
the new structures.

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 arch/arm/mach-shmobile/pm-r8a7779.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/mach-shmobile/pm-r8a7779.c b/arch/arm/mach-shmobile/pm-r8a7779.c
index 44a74c4..2ecca87 100644
--- a/arch/arm/mach-shmobile/pm-r8a7779.c
+++ b/arch/arm/mach-shmobile/pm-r8a7779.c
@@ -79,6 +79,12 @@ static bool pd_active_wakeup(struct device *dev)
 	return true;
 }
 
+struct genpd_power_state r8a7779_genpd_states[] = {
+	{
+		.name = "OFF",
+	}
+};
+
 static void r8a7779_init_pm_domain(struct r8a7779_pm_domain *r8a7779_pd)
 {
 	struct generic_pm_domain *genpd = &r8a7779_pd->genpd;
@@ -88,6 +94,8 @@ static void r8a7779_init_pm_domain(struct r8a7779_pm_domain *r8a7779_pd)
 	genpd->dev_ops.active_wakeup = pd_active_wakeup;
 	genpd->power_off = pd_power_down;
 	genpd->power_on = pd_power_up;
+	genpd->states = r8a7779_genpd_states;
+	genpd->state_count = ARRAY_SIZE(r8a7779_genpd_states);
 
 	if (pd_is_off(&r8a7779_pd->genpd))
 		pd_power_up(&r8a7779_pd->genpd);
-- 
1.9.1


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

* [RFC v5 6/8] ARM: rmobile: pm: Convert to multiple states
  2015-04-24 17:35 [RFC v5 0/8] genpd multiple states v5 ahaslam
                   ` (4 preceding siblings ...)
  2015-04-24 17:35 ` [RFC v5 5/8] ARM: r8a7779: " ahaslam
@ 2015-04-24 17:35 ` ahaslam
  2015-04-26  8:44   ` Geert Uytterhoeven
  2015-04-24 17:35 ` [RFC v5 7/8] ARM: ux500: " ahaslam
  2015-04-24 17:35 ` [RFC v5 8/8] PM / Domains: remove old power on/off callbacks ahaslam
  7 siblings, 1 reply; 15+ messages in thread
From: ahaslam @ 2015-04-24 17:35 UTC (permalink / raw)
  To: ulf.hansson, khilman, k.kozlowski.k, geert, rjw
  Cc: bcousson, linux-pm, Axel Haslam

From: Axel Haslam <ahaslam@baylibre.com>

The generic power domain framework added structure changes
to support multiple intermediate states when powering off a domain.

These changes are needed to prevent compilation breaks with
the new structures.

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 arch/arm/mach-shmobile/pm-rmobile.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/mach-shmobile/pm-rmobile.c b/arch/arm/mach-shmobile/pm-rmobile.c
index 9501820..038376c 100644
--- a/arch/arm/mach-shmobile/pm-rmobile.c
+++ b/arch/arm/mach-shmobile/pm-rmobile.c
@@ -148,6 +148,12 @@ static void rmobile_pd_detach_dev(struct generic_pm_domain *domain,
 	pm_clk_destroy(dev);
 }
 
+struct genpd_power_state rmobile_genpd_states[] = {
+	{
+		.name = "OFF",
+	}
+};
+
 static void rmobile_init_pm_domain(struct rmobile_pm_domain *rmobile_pd)
 {
 	struct generic_pm_domain *genpd = &rmobile_pd->genpd;
@@ -158,6 +164,8 @@ static void rmobile_init_pm_domain(struct rmobile_pm_domain *rmobile_pd)
 	genpd->dev_ops.active_wakeup	= rmobile_pd_active_wakeup;
 	genpd->power_off		= rmobile_pd_power_down;
 	genpd->power_on			= rmobile_pd_power_up;
+	genpd->states = rmobile_genpd_states;
+	genpd->state_count = ARRAY_SIZE(rmobile_genpd_states);
 	genpd->attach_dev		= rmobile_pd_attach_dev;
 	genpd->detach_dev		= rmobile_pd_detach_dev;
 	__rmobile_pd_power_up(rmobile_pd, false);
-- 
1.9.1


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

* [RFC v5 7/8] ARM: ux500: pm: Convert to multiple states
  2015-04-24 17:35 [RFC v5 0/8] genpd multiple states v5 ahaslam
                   ` (5 preceding siblings ...)
  2015-04-24 17:35 ` [RFC v5 6/8] ARM: rmobile: " ahaslam
@ 2015-04-24 17:35 ` ahaslam
  2015-04-24 17:35 ` [RFC v5 8/8] PM / Domains: remove old power on/off callbacks ahaslam
  7 siblings, 0 replies; 15+ messages in thread
From: ahaslam @ 2015-04-24 17:35 UTC (permalink / raw)
  To: ulf.hansson, khilman, k.kozlowski.k, geert, rjw
  Cc: bcousson, linux-pm, Axel Haslam

From: Axel Haslam <ahaslam@baylibre.com>

The generic power domain framework added structure changes
to support multiple intermediate states when powering off a domain.

These changes are needed to prevent compilation breaks with
the new structures.

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 arch/arm/mach-ux500/pm_domains.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/mach-ux500/pm_domains.c b/arch/arm/mach-ux500/pm_domains.c
index 4d71c90..c155eaa 100644
--- a/arch/arm/mach-ux500/pm_domains.c
+++ b/arch/arm/mach-ux500/pm_domains.c
@@ -39,10 +39,18 @@ static int pd_power_on(struct generic_pm_domain *domain)
 	return 0;
 }
 
+struct genpd_power_state ux500_genpd_states[] = {
+	{
+		.name = "OFF",
+	}
+};
+
 static struct generic_pm_domain ux500_pm_domain_vape = {
 	.name = "VAPE",
 	.power_off = pd_power_off,
 	.power_on = pd_power_on,
+	.states = ux500_genpd_states,
+	.state_count = ARRAY_SIZE(ux500_genpd_states),
 };
 
 static struct generic_pm_domain *ux500_pm_domains[NR_DOMAINS] = {
-- 
1.9.1


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

* [RFC v5 8/8] PM / Domains: remove old power on/off callbacks.
  2015-04-24 17:35 [RFC v5 0/8] genpd multiple states v5 ahaslam
                   ` (6 preceding siblings ...)
  2015-04-24 17:35 ` [RFC v5 7/8] ARM: ux500: " ahaslam
@ 2015-04-24 17:35 ` ahaslam
  7 siblings, 0 replies; 15+ messages in thread
From: ahaslam @ 2015-04-24 17:35 UTC (permalink / raw)
  To: ulf.hansson, khilman, k.kozlowski.k, geert, rjw
  Cc: bcousson, linux-pm, Axel Haslam

From: Axel Haslam <ahaslam@baylibre.com>

Now that all known users have been converted to use the state
version of the callbacks, we can remove the default callbacks
without breaking compilation.

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 drivers/base/power/domain.c          | 55 +++++++++++-------------------------
 drivers/base/power/domain_governor.c | 35 ++++++-----------------
 include/linux/pm_domain.h            |  2 --
 3 files changed, 26 insertions(+), 66 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index b519926..2927763 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -147,11 +147,7 @@ static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
 	if (!genpd->cpuidle_data)
 		return;
 
-	if (genpd->state_count == 0)
-		usecs64 = genpd->power_on_latency_ns;
-	else
-		usecs64 = genpd->states[state_idx].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;
@@ -173,15 +169,9 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
 		return ret;
 
 	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
-	if (genpd->state_count == 0) {
-		if (elapsed_ns <= genpd->power_on_latency_ns)
-			return ret;
-		genpd->power_on_latency_ns = elapsed_ns;
-	} else {
-		if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
-			return ret;
-		genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
-	}
+	if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
+		return ret;
+	genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
 
 	genpd->max_off_time_changed = true;
 	genpd_recalc_cpu_exit_latency(genpd);
@@ -207,15 +197,11 @@ static int genpd_power_off(struct generic_pm_domain *genpd)
 		return ret;
 
 	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
-	if (genpd->state_count == 0) {
-		if (elapsed_ns <= genpd->power_off_latency_ns)
-			return ret;
-		genpd->power_off_latency_ns = elapsed_ns;
-	} else {
-		if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns)
-			return ret;
-		genpd->states[state_idx].power_off_latency_ns = elapsed_ns;
-	}
+	if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns)
+		return ret;
+
+	genpd->states[state_idx].power_off_latency_ns = elapsed_ns;
+
 	genpd->max_off_time_changed = true;
 	pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
 		genpd->name, "off", elapsed_ns);
@@ -1931,12 +1917,10 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 	if (IS_ERR_OR_NULL(genpd))
 		return;
 
-	if (genpd->state_count > 0) {
-		/* Copy the state data to allocated memory */
-		genpd->states = genpd_alloc_states_data(genpd);
-		if (!genpd->states)
-			return;
-	}
+	/* Point the state data to allocated memory */
+	genpd->states = genpd_alloc_states_data(genpd);
+	if (!genpd->states)
+		return;
 
 	INIT_LIST_HEAD(&genpd->master_links);
 	INIT_LIST_HEAD(&genpd->slave_links);
@@ -2338,15 +2322,10 @@ static int pm_genpd_summary_one(struct seq_file *s,
 
 	if (WARN_ON(genpd->status >= ARRAY_SIZE(status_lookup)))
 		goto exit;
-
-	if (genpd->state_count == 0)
-		seq_printf(s, "%-30s  %-15s  ",
-			genpd->name, status_lookup[genpd->status]);
-	else
-		seq_printf(s, "%-30s  %-15s  ", genpd->name,
-			(genpd->status == GPD_STATE_POWER_OFF) ?
-				genpd->states[state_idx].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
diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index 061c711..b77f7c9 100644
--- a/drivers/base/power/domain_governor.c
+++ b/drivers/base/power/domain_governor.c
@@ -104,13 +104,8 @@ static bool power_down_ok_for_state(struct dev_pm_domain *pd, int state)
 	struct pm_domain_data *pdd;
 	s64 min_off_time_ns;
 	s64 off_on_time_ns;
-
-	if (genpd->state_count == 0)
-		off_on_time_ns = genpd->power_off_latency_ns +
-			genpd->power_on_latency_ns;
-	else
-		off_on_time_ns = genpd->states[state].power_off_latency_ns +
-			genpd->states[state].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
@@ -199,12 +194,8 @@ static bool power_down_ok_for_state(struct dev_pm_domain *pd, int state)
 	 * time and the time needed to turn the domain on is the maximum
 	 * theoretical time this domain can spend in the "off" state.
 	 */
-	if (genpd->state_count == 0)
-		genpd->max_off_time_ns =
-			min_off_time_ns - genpd->power_on_latency_ns;
-	else
-		genpd->max_off_time_ns = min_off_time_ns -
-			genpd->states[state].power_on_latency_ns;
+	genpd->max_off_time_ns = min_off_time_ns -
+		genpd->states[state].power_on_latency_ns;
 	return true;
 }
 
@@ -236,19 +227,11 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
 	genpd->max_off_time_changed = false;
 
 	/* find a state to power down to, starting from the deepest */
-	if (genpd->state_count == 0) {
-		/*
-		 * there are no states. power_down_ok_for_state will use
-		 * the stateless values, and ignore the state argument.
-		 */
-		retval = power_down_ok_for_state(pd, 0);
-	} else {
-		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;
-			}
+	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;
 		}
 	}
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 25082f6..38339f4 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


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

* Re: [RFC v5 1/8] PM / Domains: structure changes for multiple states
  2015-04-24 17:35 ` [RFC v5 1/8] PM / Domains: structure changes for multiple states ahaslam
@ 2015-04-26  8:42   ` Geert Uytterhoeven
  2015-04-27  8:26     ` Axel Haslam
  0 siblings, 1 reply; 15+ messages in thread
From: Geert Uytterhoeven @ 2015-04-26  8:42 UTC (permalink / raw)
  To: Axel Haslam
  Cc: Ulf Hansson, Kevin Hilman, Krzysztof Kozłowski,
	Rafael J. Wysocki, Benoit Cousson, Linux PM list

On Fri, Apr 24, 2015 at 7:35 PM,  <ahaslam@baylibre.com> wrote:
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c

> @@ -1438,6 +1458,54 @@ static void genpd_free_dev_data(struct device *dev,
>         kfree(gpd_data);
>         dev_pm_put_subsys_data(dev);
>  }
> +/**
> + * genpd_alloc_states_data - Allocate and copy state parameters
> + * @genpd: PM domain use to copy and allocate data
> + */
> +static struct genpd_power_state *genpd_alloc_states_data(
> +                       struct generic_pm_domain *genpd)
> +{
> +       struct genpd_power_state *states;
> +       size_t states_size;
> +       int i;
> +
> +       if (IS_ERR_OR_NULL(genpd))
> +               return NULL;
> +
> +       if (!genpd->states) {
> +               pr_err("%s Null state pointer\n", genpd->name);
> +               return NULL;
> +       }
> +
> +       if (genpd->state_count < 1) {
> +               pr_err("%s Invalid number of states %d\n",
> +                       genpd->name, genpd->state_count);
> +               return NULL;
> +       }
> +
> +       /* Allocate the local memory to keep the states for this genpd */
> +       states_size = sizeof(*states) * genpd->state_count;
> +       states = kzalloc(states_size, GFP_KERNEL);
> +       if (!states) {
> +               pr_err("%s states Allocation error\n", genpd->name);
> +               return NULL;
> +       }
> +
> +       /* Copy the parameters passed to the allocated structure */
> +       for (i = 0; i < genpd->state_count; i++) {
> +               states[i].power_off_latency_ns =
> +                       genpd->states[i].power_off_latency_ns;
> +               states[i].power_on_latency_ns =
> +                       genpd->states[i].power_on_latency_ns;
> +       }
> +
> +#ifdef CONFIG_PM_ADVANCED_DEBUG
> +       /* State name is used for debug. Avoid unnecessary allocations */
> +       for (i = 0; i < genpd->state_count; i++)
> +               states[i].name = kstrdup(genpd->states[i].name, GFP_KERNEL);
> +#endif
> +       return states;
> +}
>
>  /**
>   * __pm_genpd_add_device - Add a device to an I/O PM domain.
> @@ -1863,6 +1931,13 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>         if (IS_ERR_OR_NULL(genpd))
>                 return;
>
> +       if (genpd->state_count > 0) {
> +               /* Copy the state data to allocated memory */
> +               genpd->states = genpd_alloc_states_data(genpd);
> +               if (!genpd->states)
> +                       return;
> +       }
> +

So the above replaces genpd->states (which is usually pointing to a an
array of static data) by an allocated copy?
Nice trick, but I'm wondering whether it may bite us one day?
This also means the static data can't be const.

Both could be solved by e.g. passing the states array to pm_genpd_init().

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

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

* Re: [RFC v5 5/8] ARM: r8a7779: pm: Convert to multiple states
  2015-04-24 17:35 ` [RFC v5 5/8] ARM: r8a7779: " ahaslam
@ 2015-04-26  8:43   ` Geert Uytterhoeven
  2015-04-27  9:32     ` Axel Haslam
  0 siblings, 1 reply; 15+ messages in thread
From: Geert Uytterhoeven @ 2015-04-26  8:43 UTC (permalink / raw)
  To: Axel Haslam
  Cc: Ulf Hansson, Kevin Hilman, Krzysztof Kozłowski,
	Rafael J. Wysocki, Benoit Cousson, Linux PM list

On Fri, Apr 24, 2015 at 7:35 PM,  <ahaslam@baylibre.com> wrote:
> --- a/arch/arm/mach-shmobile/pm-r8a7779.c
> +++ b/arch/arm/mach-shmobile/pm-r8a7779.c
> @@ -79,6 +79,12 @@ static bool pd_active_wakeup(struct device *dev)
>         return true;
>  }
>
> +struct genpd_power_state r8a7779_genpd_states[] = {
> +       {
> +               .name = "OFF",
> +       }
> +};
> +
>  static void r8a7779_init_pm_domain(struct r8a7779_pm_domain *r8a7779_pd)
>  {
>         struct generic_pm_domain *genpd = &r8a7779_pd->genpd;
> @@ -88,6 +94,8 @@ static void r8a7779_init_pm_domain(struct r8a7779_pm_domain *r8a7779_pd)
>         genpd->dev_ops.active_wakeup = pd_active_wakeup;
>         genpd->power_off = pd_power_down;
>         genpd->power_on = pd_power_up;
> +       genpd->states = r8a7779_genpd_states;
> +       genpd->state_count = ARRAY_SIZE(r8a7779_genpd_states);

The states are set _after_ pm_genpd_init(), so this won't work.

BTW, no idea why all these genpd fields are set after that call. Perhaps
originally they had to override defaults set by pm_genpd_init()?

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

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

* Re: [RFC v5 6/8] ARM: rmobile: pm: Convert to multiple states
  2015-04-24 17:35 ` [RFC v5 6/8] ARM: rmobile: " ahaslam
@ 2015-04-26  8:44   ` Geert Uytterhoeven
  0 siblings, 0 replies; 15+ messages in thread
From: Geert Uytterhoeven @ 2015-04-26  8:44 UTC (permalink / raw)
  To: Axel Haslam
  Cc: Ulf Hansson, Kevin Hilman, Krzysztof Kozłowski,
	Rafael J. Wysocki, Benoit Cousson, Linux PM list

On Fri, Apr 24, 2015 at 7:35 PM,  <ahaslam@baylibre.com> wrote:
> --- a/arch/arm/mach-shmobile/pm-rmobile.c
> +++ b/arch/arm/mach-shmobile/pm-rmobile.c
> @@ -158,6 +164,8 @@ static void rmobile_init_pm_domain(struct rmobile_pm_domain *rmobile_pd)
>         genpd->dev_ops.active_wakeup    = rmobile_pd_active_wakeup;
>         genpd->power_off                = rmobile_pd_power_down;
>         genpd->power_on                 = rmobile_pd_power_up;
> +       genpd->states = rmobile_genpd_states;
> +       genpd->state_count = ARRAY_SIZE(rmobile_genpd_states);

The states are set _after_ pm_genpd_init(), so this won't work.

BTW, no idea why all these genpd fields are set after that call. Perhaps
originally they had to override defaults set by pm_genpd_init()?

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

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

* Re: [RFC v5 1/8] PM / Domains: structure changes for multiple states
  2015-04-26  8:42   ` Geert Uytterhoeven
@ 2015-04-27  8:26     ` Axel Haslam
  2015-04-27  8:30       ` Geert Uytterhoeven
  0 siblings, 1 reply; 15+ messages in thread
From: Axel Haslam @ 2015-04-27  8:26 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Ulf Hansson, Kevin Hilman, Krzysztof Kozłowski,
	Rafael J. Wysocki, Benoit Cousson, Linux PM list

Hi Geert,

Thanks for the review!

On 26/04/2015 10:42, Geert Uytterhoeven wrote:

>>                  return;
>>
>> +       if (genpd->state_count > 0) {
>> +               /* Copy the state data to allocated memory */
>> +               genpd->states = genpd_alloc_states_data(genpd);
>> +               if (!genpd->states)
>> +                       return;
>> +       }
>> +
>
> So the above replaces genpd->states (which is usually pointing to a an
> array of static data) by an allocated copy?
> Nice trick, but I'm wondering whether it may bite us one day?
> This also means the static data can't be const.
>
> Both could be solved by e.g. passing the states array to pm_genpd_init().

i thought i should avoid adding an argument to init. But you are right, 
i guess its better that way. ill do it on the on the next spin.

BTW, im thinking i should add a default OFF state in case the 
state_count is 0, that way, platforms could not worry about states
at all in case they dont define latencies.


Thanks again,
Axel.

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

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

* Re: [RFC v5 1/8] PM / Domains: structure changes for multiple states
  2015-04-27  8:26     ` Axel Haslam
@ 2015-04-27  8:30       ` Geert Uytterhoeven
  0 siblings, 0 replies; 15+ messages in thread
From: Geert Uytterhoeven @ 2015-04-27  8:30 UTC (permalink / raw)
  To: Axel Haslam
  Cc: Ulf Hansson, Kevin Hilman, Krzysztof Kozłowski,
	Rafael J. Wysocki, Benoit Cousson, Linux PM list

On Mon, Apr 27, 2015 at 10:26 AM, Axel Haslam <ahaslam@baylibre.com> wrote:
> BTW, im thinking i should add a default OFF state in case the state_count is
> 0, that way, platforms could not worry about states
> at all in case they dont define latencies.

Ideally, the latencies should come from DT, not from C platform code.

Now v4.1-rc1 is out, I'll resurrect my patches for that, as Kevin asked back
in March.
They'll be RFC again, as they're gonna need changes to support multiple
states.

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

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

* Re: [RFC v5 5/8] ARM: r8a7779: pm: Convert to multiple states
  2015-04-26  8:43   ` Geert Uytterhoeven
@ 2015-04-27  9:32     ` Axel Haslam
  0 siblings, 0 replies; 15+ messages in thread
From: Axel Haslam @ 2015-04-27  9:32 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Ulf Hansson, Kevin Hilman, Krzysztof Kozłowski,
	Rafael J. Wysocki, Benoit Cousson, Linux PM list

Hi Geert,

On 26/04/2015 10:43, Geert Uytterhoeven wrote:
> On Fri, Apr 24, 2015 at 7:35 PM,  <ahaslam@baylibre.com> wrote:
>> --- a/arch/arm/mach-shmobile/pm-r8a7779.c
>> +++ b/arch/arm/mach-shmobile/pm-r8a7779.c
>> @@ -79,6 +79,12 @@ static bool pd_active_wakeup(struct device *dev)
>>          return true;
>>   }
>>
>> +struct genpd_power_state r8a7779_genpd_states[] = {
>> +       {
>> +               .name = "OFF",
>> +       }
>> +};
>> +
>>   static void r8a7779_init_pm_domain(struct r8a7779_pm_domain *r8a7779_pd)
>>   {
>>          struct generic_pm_domain *genpd = &r8a7779_pd->genpd;
>> @@ -88,6 +94,8 @@ static void r8a7779_init_pm_domain(struct r8a7779_pm_domain *r8a7779_pd)
>>          genpd->dev_ops.active_wakeup = pd_active_wakeup;
>>          genpd->power_off = pd_power_down;
>>          genpd->power_on = pd_power_up;
>> +       genpd->states = r8a7779_genpd_states;
>> +       genpd->state_count = ARRAY_SIZE(r8a7779_genpd_states);
>
> The states are set _after_ pm_genpd_init(), so this won't work.
>
> BTW, no idea why all these genpd fields are set after that call. Perhaps
> originally they had to override defaults set by pm_genpd_init()?

good catch!

im not sure why they are set after either. i guess they should be moved. 
non of those fields seem to be set by the init today.

Anyways, the issue with the states array will be fixed once they are 
passed as paramers to the init, on the next spin.


Thanks,
Axel


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

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

end of thread, other threads:[~2015-04-27  9:33 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-24 17:35 [RFC v5 0/8] genpd multiple states v5 ahaslam
2015-04-24 17:35 ` [RFC v5 1/8] PM / Domains: structure changes for multiple states ahaslam
2015-04-26  8:42   ` Geert Uytterhoeven
2015-04-27  8:26     ` Axel Haslam
2015-04-27  8:30       ` Geert Uytterhoeven
2015-04-24 17:35 ` [RFC v5 2/8] PM / Domains: select deepest state ahaslam
2015-04-24 17:35 ` [RFC v5 3/8] ARM: s3c64xx: pm: Convert to multiple states ahaslam
2015-04-24 17:35 ` [RFC v5 4/8] ARM: exynos: " ahaslam
2015-04-24 17:35 ` [RFC v5 5/8] ARM: r8a7779: " ahaslam
2015-04-26  8:43   ` Geert Uytterhoeven
2015-04-27  9:32     ` Axel Haslam
2015-04-24 17:35 ` [RFC v5 6/8] ARM: rmobile: " ahaslam
2015-04-26  8:44   ` Geert Uytterhoeven
2015-04-24 17:35 ` [RFC v5 7/8] ARM: ux500: " ahaslam
2015-04-24 17:35 ` [RFC v5 8/8] PM / Domains: remove old power on/off callbacks ahaslam

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.