[RFC v11 0/3] Multiple intermediate states for genpd

[RFC v11 0/3] Multiple intermediate states for genpd

[ahaslam]

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

Some architectures may have intermediate power levels between on and off.

This patch set 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 v10
* Several reworks after Ulf Hansson's comments:
 - merge the governor changes and core changes into a single patch
 - remove the "name" of the state.
 - add a default state allocation function
 - convert governor power done ok function to while loop.
 - reword commit message

Changes since v9
*rebased on linux-next

Changes since v8
* rebased to linux-pm next

Changes since v7:
* rebase to 4.3-rc5

* add genpd_init_simple (Lina's suggestion) for platforms that don't have
multiple states and don't declare initial latencies. A default OFF
state with initial 0 latencies will be used in this case.

* Append Mark's patch to add "states" and "timings" to the genpd
debugfs

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.

* rename target_state to state_idx and remove init_state.

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.

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.

* rename default_power_down_ok_state to power_down_ok_for_state
Axel Haslam (3):
  PM / Domains: Support for multiple states
  ARM: imx6: pm: declare pm domain latency on power_state struct.
  PM / Domains: remove old power on/off latencies.

 arch/arm/mach-imx/gpc.c              | 11 ++++--
 drivers/base/power/domain.c          | 53 ++++++++++++++++++++++++---
 drivers/base/power/domain_governor.c | 70 +++++++++++++++++++++++-------------
 include/linux/pm_domain.h            | 11 ++++--
 4 files changed, 112 insertions(+), 33 deletions(-)

-- 
2.6.3

[RFC v11 1/3] PM / Domains: Support for multiple states

[ahaslam]

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

Some hardware (eg. OMAP), has the ability to enter different low power
modes for a given power domain. This allows for more fine grained control
over the power state of the platform. As a typical example, some registers
of the hardware may be implemented with retention flip-flops and be able to
retain their state at lower voltages allowing for faster on/off latencies
and an increased window of opportunity to enter an intermediate low power
state other than "off"

When trying to set a power domain to off, the genpd governor will
choose the deepest state that will respect the qos constraints of all the
devices and sub-domains on the power domain. The state chosen by the
governor is saved in the "state_idx" field of the generic_pm_domain structure
and shall be used by the power_off and power_on callbacks to perform the
necessary actions to set the power domain into (and out of) the state
indicated by state_idx.

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

For plaforms that are declaring power on and off latencies, on platorm
data, a single state is allocated which uses those values. Once all
platforms are converted to use the state array, the legacy on/off latencies
will be removed.

Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>
[Lina: Modified genpd state initialization and remove use of
save_state_latency_ns in genpd timing data]
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/base/power/domain.c          | 59 +++++++++++++++++++++++++++---
 drivers/base/power/domain_governor.c | 70 +++++++++++++++++++++++-------------
 include/linux/pm_domain.h            |  9 +++++
 3 files changed, 109 insertions(+), 29 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 301b785..7099eb3 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -104,6 +104,7 @@ static void genpd_sd_counter_inc(struct generic_pm_domain *genpd)
 
 static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 {
+	unsigned int state_idx = genpd->state_idx;
 	ktime_t time_start;
 	s64 elapsed_ns;
 	int ret;
@@ -120,10 +121,10 @@ static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 		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;
 	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
 		 genpd->name, "on", elapsed_ns);
@@ -133,6 +134,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 
 static int genpd_power_off(struct generic_pm_domain *genpd, bool timed)
 {
+	unsigned int state_idx = genpd->state_idx;
 	ktime_t time_start;
 	s64 elapsed_ns;
 	int ret;
@@ -149,10 +151,10 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool timed)
 		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);
@@ -585,6 +587,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, timed);
 
 	genpd->status = GPD_STATE_POWER_OFF;
@@ -1210,6 +1214,36 @@ static void genpd_free_dev_data(struct device *dev,
 	dev_pm_put_subsys_data(dev);
 }
 
+static int genpd_alloc_default_state(struct generic_pm_domain *genpd)
+{
+	int ret;
+
+	if (IS_ERR_OR_NULL(genpd)) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	genpd->states = kcalloc(1, sizeof(struct genpd_power_state),
+				GFP_KERNEL);
+	if (!genpd->states) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	genpd->states[0].power_on_latency_ns =
+				genpd->power_on_latency_ns;
+
+	genpd->states[0].power_off_latency_ns =
+				genpd->power_off_latency_ns;
+
+	genpd->state_count = 1;
+	genpd->state_idx = 0;
+
+	return 0;
+err:
+	return ret;
+}
+
 /**
  * __pm_genpd_add_device - Add a device to an I/O PM domain.
  * @genpd: PM domain to add the device to.
@@ -1464,9 +1498,19 @@ static int pm_genpd_default_restore_state(struct device *dev)
 void pm_genpd_init(struct generic_pm_domain *genpd,
 		   struct dev_power_governor *gov, bool is_off)
 {
+	int ret;
+
 	if (IS_ERR_OR_NULL(genpd))
 		return;
 
+	if (genpd->state_count == 0) {
+		ret = genpd_alloc_default_state(genpd);
+		if (ret) {
+			pr_err("unable to allocate default state.");
+			return;
+		}
+	}
+
 	INIT_LIST_HEAD(&genpd->master_links);
 	INIT_LIST_HEAD(&genpd->slave_links);
 	INIT_LIST_HEAD(&genpd->dev_list);
@@ -1872,7 +1916,12 @@ 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  %s", genpd->name, status_lookup[genpd->status]);
+
+	if (genpd->status == GPD_STATE_POWER_OFF)
+		seq_printf(s, " %-13d ", genpd->state_idx);
+	else
+		seq_printf(s, " %-15s ", "");
 
 	/*
 	 * 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 1e937ac..c4fe0f1 100644
--- a/drivers/base/power/domain_governor.c
+++ b/drivers/base/power/domain_governor.c
@@ -98,7 +98,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 __default_power_down_ok(struct dev_pm_domain *pd,
+				     unsigned int state)
 {
 	struct generic_pm_domain *genpd = pd_to_genpd(pd);
 	struct gpd_link *link;
@@ -106,27 +107,9 @@ 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;
-	}
+	off_on_time_ns = genpd->states[state].power_off_latency_ns +
+		genpd->states[state].power_on_latency_ns;
 
-	off_on_time_ns = genpd->power_off_latency_ns +
-				genpd->power_on_latency_ns;
 
 	min_off_time_ns = -1;
 	/*
@@ -186,8 +169,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
@@ -201,10 +182,51 @@ 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;
+	genpd->max_off_time_ns = min_off_time_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);
+	struct gpd_link *link;
+
+	/* There should always be at least one state. */
+	if (genpd->state_count < 1) {
+		pr_err("Invalid state count.");
+		return false;
+	}
+
+	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;
+	genpd->cached_power_down_ok = true;
+	genpd->state_idx = genpd->state_count - 1;
+
+	/* Find a state to power down to, starting from the deepest. */
+	while (!__default_power_down_ok(pd, genpd->state_idx)) {
+		if (genpd->state_idx == 0) {
+			genpd->cached_power_down_ok = false;
+			break;
+		}
+		genpd->state_idx--;
+	}
+
+	return genpd->cached_power_down_ok;
+}
+
 static bool always_on_power_down_ok(struct dev_pm_domain *domain)
 {
 	return false;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index db21d39..a5c3b68 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -37,6 +37,11 @@ struct gpd_dev_ops {
 	bool (*active_wakeup)(struct device *dev);
 };
 
+struct genpd_power_state {
+	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 */
@@ -66,6 +71,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)
-- 
2.6.3

[RFC v11 2/3] 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>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
[Lina: pm_genpd_init() argument change]
---
 arch/arm/mach-imx/gpc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index cfc696b..2630d94 100644
--- a/arch/arm/mach-imx/gpc.c
+++ b/arch/arm/mach-imx/gpc.c
@@ -369,13 +369,20 @@ static struct generic_pm_domain imx6q_arm_domain = {
 	.name = "ARM",
 };
 
+static struct genpd_power_state imx6q_arm_domain_states[] = {
+	{
+		.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,
+		.state_count = ARRAY_SIZE(imx6q_arm_domain_states),
 	},
 };
 
-- 
2.6.3

[RFC v11 3/3] 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 | 6 ------
 include/linux/pm_domain.h   | 2 --
 2 files changed, 8 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 7099eb3..c4b3e36 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1230,12 +1230,6 @@ static int genpd_alloc_default_state(struct generic_pm_domain *genpd)
 		goto err;
 	}
 
-	genpd->states[0].power_on_latency_ns =
-				genpd->power_on_latency_ns;
-
-	genpd->states[0].power_off_latency_ns =
-				genpd->power_off_latency_ns;
-
 	genpd->state_count = 1;
 	genpd->state_idx = 0;
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index a5c3b68..2cc75d8 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -59,9 +59,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;
-- 
2.6.3

Re: [RFC v11 1/3] PM / Domains: Support for multiple states

[Ulf Hansson]

On 9 February 2016 at 14:15,  <ahaslam@baylibre.com> wrote:
> From: Axel Haslam <ahaslam+renesas@baylibre.com>
>
> Some hardware (eg. OMAP), has the ability to enter different low power
> modes for a given power domain. This allows for more fine grained control
> over the power state of the platform. As a typical example, some registers
> of the hardware may be implemented with retention flip-flops and be able to
> retain their state at lower voltages allowing for faster on/off latencies
> and an increased window of opportunity to enter an intermediate low power
> state other than "off"
>
> When trying to set a power domain to off, the genpd governor will
> choose the deepest state that will respect the qos constraints of all the
> devices and sub-domains on the power domain. The state chosen by the
> governor is saved in the "state_idx" field of the generic_pm_domain structure
> and shall be used by the power_off and power_on callbacks to perform the
> necessary actions to set the power domain into (and out of) the state
> indicated by state_idx.
>
> States shall be declared in ascending order from shallowest to deepest,
> deepest meaning the state which takes longer to enter and exit.
>
> For plaforms that are declaring power on and off latencies, on platorm

/s/platorm/platform

> data, a single state is allocated which uses those values. Once all
> platforms are converted to use the state array, the legacy on/off latencies
> will be removed.
>
> Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com>
> [Lina: Modified genpd state initialization and remove use of
> save_state_latency_ns in genpd timing data]
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  drivers/base/power/domain.c          | 59 +++++++++++++++++++++++++++---
>  drivers/base/power/domain_governor.c | 70 +++++++++++++++++++++++-------------
>  include/linux/pm_domain.h            |  9 +++++
>  3 files changed, 109 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 301b785..7099eb3 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -104,6 +104,7 @@ static void genpd_sd_counter_inc(struct generic_pm_domain *genpd)
>
>  static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>  {
> +       unsigned int state_idx = genpd->state_idx;
>         ktime_t time_start;
>         s64 elapsed_ns;
>         int ret;
> @@ -120,10 +121,10 @@ static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>                 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;
>         pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
>                  genpd->name, "on", elapsed_ns);
> @@ -133,6 +134,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>
>  static int genpd_power_off(struct generic_pm_domain *genpd, bool timed)
>  {
> +       unsigned int state_idx = genpd->state_idx;
>         ktime_t time_start;
>         s64 elapsed_ns;
>         int ret;
> @@ -149,10 +151,10 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool timed)
>                 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);
> @@ -585,6 +587,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, timed);
>
>         genpd->status = GPD_STATE_POWER_OFF;
> @@ -1210,6 +1214,36 @@ static void genpd_free_dev_data(struct device *dev,
>         dev_pm_put_subsys_data(dev);
>  }
>
> +static int genpd_alloc_default_state(struct generic_pm_domain *genpd)
> +{
> +       int ret;
> +
> +       if (IS_ERR_OR_NULL(genpd)) {
> +               ret = -EINVAL;
> +               goto err;
> +       }

No need for this check. It's a static function called by
pm_genpd_init(), which already done this.

> +
> +       genpd->states = kcalloc(1, sizeof(struct genpd_power_state),
> +                               GFP_KERNEL);
> +       if (!genpd->states) {
> +               ret = -ENOMEM;
> +               goto err;

I just realized that this won't play well, mainly because
pm_genpd_init() don't return any error codes.

I suggest to not allocate the data for "states" on the heap, but
instead use a static struct.

That means the function should be "static void" and perhaps renamed to
genpd_set_default_state() or similar.

> +       }
> +
> +       genpd->states[0].power_on_latency_ns =
> +                               genpd->power_on_latency_ns;
> +
> +       genpd->states[0].power_off_latency_ns =
> +                               genpd->power_off_latency_ns;
> +
> +       genpd->state_count = 1;
> +       genpd->state_idx = 0;
> +
> +       return 0;
> +err:
> +       return ret;
> +}
> +
>  /**
>   * __pm_genpd_add_device - Add a device to an I/O PM domain.
>   * @genpd: PM domain to add the device to.
> @@ -1464,9 +1498,19 @@ static int pm_genpd_default_restore_state(struct device *dev)
>  void pm_genpd_init(struct generic_pm_domain *genpd,
>                    struct dev_power_governor *gov, bool is_off)
>  {
> +       int ret;
> +
>         if (IS_ERR_OR_NULL(genpd))
>                 return;
>
> +       if (genpd->state_count == 0) {
> +               ret = genpd_alloc_default_state(genpd);
> +               if (ret) {
> +                       pr_err("unable to allocate default state.");
> +                       return;

According to my upper comments, we shouldn't accept errors here...

> +               }
> +       }
> +
>         INIT_LIST_HEAD(&genpd->master_links);
>         INIT_LIST_HEAD(&genpd->slave_links);
>         INIT_LIST_HEAD(&genpd->dev_list);
> @@ -1872,7 +1916,12 @@ 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  %s", genpd->name, status_lookup[genpd->status]);
> +
> +       if (genpd->status == GPD_STATE_POWER_OFF)
> +               seq_printf(s, " %-13d ", genpd->state_idx);
> +       else
> +               seq_printf(s, " %-15s ", "");
>
>         /*
>          * 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 1e937ac..c4fe0f1 100644
> --- a/drivers/base/power/domain_governor.c
> +++ b/drivers/base/power/domain_governor.c
> @@ -98,7 +98,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 __default_power_down_ok(struct dev_pm_domain *pd,
> +                                    unsigned int state)
>  {
>         struct generic_pm_domain *genpd = pd_to_genpd(pd);
>         struct gpd_link *link;
> @@ -106,27 +107,9 @@ 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;
> -       }
> +       off_on_time_ns = genpd->states[state].power_off_latency_ns +
> +               genpd->states[state].power_on_latency_ns;
>
> -       off_on_time_ns = genpd->power_off_latency_ns +
> -                               genpd->power_on_latency_ns;
>
>         min_off_time_ns = -1;
>         /*
> @@ -186,8 +169,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
> @@ -201,10 +182,51 @@ 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;
> +       genpd->max_off_time_ns = min_off_time_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);
> +       struct gpd_link *link;
> +
> +       /* There should always be at least one state. */
> +       if (genpd->state_count < 1) {
> +               pr_err("Invalid state count.");

Not sure this is needed, but if you insists perhaps a WARN_ONCE is better!?

> +               return false;
> +       }
> +
> +       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;
> +       genpd->cached_power_down_ok = true;
> +       genpd->state_idx = genpd->state_count - 1;
> +
> +       /* Find a state to power down to, starting from the deepest. */
> +       while (!__default_power_down_ok(pd, genpd->state_idx)) {
> +               if (genpd->state_idx == 0) {
> +                       genpd->cached_power_down_ok = false;
> +                       break;
> +               }
> +               genpd->state_idx--;
> +       }
> +
> +       return genpd->cached_power_down_ok;
> +}
> +
>  static bool always_on_power_down_ok(struct dev_pm_domain *domain)
>  {
>         return false;
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index db21d39..a5c3b68 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -37,6 +37,11 @@ struct gpd_dev_ops {
>         bool (*active_wakeup)(struct device *dev);
>  };
>
> +struct genpd_power_state {
> +       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 */
> @@ -66,6 +71,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)
> --
> 2.6.3
>

Overall this looks really good to me. I suggest you send this as PATCH
instead of an RFC in the next respin.

Kind regards
Uffe

Re: [RFC v11 2/3] ARM: imx6: pm: declare pm domain latency on power_state struct.

[Ulf Hansson]

On 9 February 2016 at 14:15,  <ahaslam@baylibre.com> wrote:
> 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>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> [Lina: pm_genpd_init() argument change]

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

Kind regards
Uffe

> ---
>  arch/arm/mach-imx/gpc.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
> index cfc696b..2630d94 100644
> --- a/arch/arm/mach-imx/gpc.c
> +++ b/arch/arm/mach-imx/gpc.c
> @@ -369,13 +369,20 @@ static struct generic_pm_domain imx6q_arm_domain = {
>         .name = "ARM",
>  };
>
> +static struct genpd_power_state imx6q_arm_domain_states[] = {
> +       {
> +               .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,
> +               .state_count = ARRAY_SIZE(imx6q_arm_domain_states),
>         },
>  };
>
> --
> 2.6.3
>

Re: [RFC v11 3/3] PM / Domains: remove old power on/off latencies.

[Ulf Hansson]

On 9 February 2016 at 14:15,  <ahaslam@baylibre.com> wrote:
> 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>

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

Kind regards
Uffe

> ---
>  drivers/base/power/domain.c | 6 ------
>  include/linux/pm_domain.h   | 2 --
>  2 files changed, 8 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 7099eb3..c4b3e36 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1230,12 +1230,6 @@ static int genpd_alloc_default_state(struct generic_pm_domain *genpd)
>                 goto err;
>         }
>
> -       genpd->states[0].power_on_latency_ns =
> -                               genpd->power_on_latency_ns;
> -
> -       genpd->states[0].power_off_latency_ns =
> -                               genpd->power_off_latency_ns;
> -
>         genpd->state_count = 1;
>         genpd->state_idx = 0;
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index a5c3b68..2cc75d8 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -59,9 +59,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;
> --
> 2.6.3
>

Re: [RFC v11 1/3] PM / Domains: Support for multiple states

[Ulf Hansson]

[...]

Hi Axel,

Please make sure to send emails in plain text.

>> > +static int genpd_alloc_default_state(struct generic_pm_domain *genpd)
>> > +{
>> > +       int ret;
>> > +
>> > +       if (IS_ERR_OR_NULL(genpd)) {
>> > +               ret = -EINVAL;
>> > +               goto err;
>> > +       }
>>
>> No need for this check. It's a static function called by
>> pm_genpd_init(), which already done this.
>>
>> > +
>> > +       genpd->states = kcalloc(1, sizeof(struct genpd_power_state),
>> > +                               GFP_KERNEL);
>> > +       if (!genpd->states) {
>> > +               ret = -ENOMEM;
>> > +               goto err;
>>
>> I just realized that this won't play well, mainly because
>> pm_genpd_init() don't return any error codes.
>>
>> I suggest to not allocate the data for "states" on the heap, but
>> instead use a static struct.
>>
>> That means the function should be "static void" and perhaps renamed to
>> genpd_set_default_state() or similar.
>
>
>
>
> I have one doubt here, if i use static struct instead of allocating it,
> would it not mean that all genpd's that use this function would share
> the same state struct?
>
> the latency values are potentially updated when using the power on/off
> functions,
> so i think they should have they own separate "per-genpd" state struct.

Right! Apologize for my vague answer.

See more below.

[...]

>
>> >
>> > +struct genpd_power_state {
>> > +       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 */
>> > @@ -66,6 +71,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;

I suggest we change this to:

struct genpd_power_state states[MAX_GENPD_STATES];

I don't know what value MAX_GENPD_STATES should have but perhaps 8 is
good enough. In that way the number of supported states will be
"static". It's not the nicest thing to do, but I can't figure out a
better option.

Another option would be to change the pm_genpd_init() API to return an
int as it needs to be able to propagate an error code, if it allocates
data dynamically.

>> > +       unsigned int state_count; /* number of states */
>> > +       unsigned int state_idx; /* state that genpd will go to when off
>> > */
>> > +

[...]

Kind regards
Uffe

Re: [RFC v11 1/3] PM / Domains: Support for multiple states

[Axel Haslam]

On Wed, Feb 10, 2016 at 9:08 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> [...]
>
> Hi Axel,
>
> Please make sure to send emails in plain text.

Yes, Sorry about that.

>
>>> > +static int genpd_alloc_default_state(struct generic_pm_domain *genpd)
>>> > +{
>>> > +       int ret;
>>> > +
>>> > +       if (IS_ERR_OR_NULL(genpd)) {
>>> > +               ret = -EINVAL;
>>> > +               goto err;
>>> > +       }
>>>
>>> No need for this check. It's a static function called by
>>> pm_genpd_init(), which already done this.
>>>
>>> > +
>>> > +       genpd->states = kcalloc(1, sizeof(struct genpd_power_state),
>>> > +                               GFP_KERNEL);
>>> > +       if (!genpd->states) {
>>> > +               ret = -ENOMEM;
>>> > +               goto err;
>>>
>>> I just realized that this won't play well, mainly because
>>> pm_genpd_init() don't return any error codes.
>>>
>>> I suggest to not allocate the data for "states" on the heap, but
>>> instead use a static struct.
>>>
>>> That means the function should be "static void" and perhaps renamed to
>>> genpd_set_default_state() or similar.
>>
>>
>>
>>
>> I have one doubt here, if i use static struct instead of allocating it,
>> would it not mean that all genpd's that use this function would share
>> the same state struct?
>>
>> the latency values are potentially updated when using the power on/off
>> functions,
>> so i think they should have they own separate "per-genpd" state struct.
>
> Right! Apologize for my vague answer.
>
> See more below.
>
> [...]
>
>>
>>> >
>>> > +struct genpd_power_state {
>>> > +       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 */
>>> > @@ -66,6 +71,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;
>
> I suggest we change this to:
>
> struct genpd_power_state states[MAX_GENPD_STATES];
>
> I don't know what value MAX_GENPD_STATES should have but perhaps 8 is
> good enough. In that way the number of supported states will be
> "static". It's not the nicest thing to do, but I can't figure out a
> better option.

Ok understood, i had a doubt because in my first couple of series i was doing
that, but there was a suggestion form Geert to rather allocate the
states to save the unused memory.
Is it ok if i implement it with  static struct for now to keep this
series small?

Otherwise, i will implement the error code return and checking from
pm_genpd_init.

>
> Another option would be to change the pm_genpd_init() API to return an
> int as it needs to be able to propagate an error code, if it allocates
> data dynamically.
>
>>> > +       unsigned int state_count; /* number of states */
>>> > +       unsigned int state_idx; /* state that genpd will go to when off
>>> > */
>>> > +
>
> [...]
>
> Kind regards
> Uffe

Re: [RFC v11 1/3] PM / Domains: Support for multiple states

[Ulf Hansson]

+ Jon

>>>> >  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 */
>>>> > @@ -66,6 +71,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;
>>
>> I suggest we change this to:
>>
>> struct genpd_power_state states[MAX_GENPD_STATES];
>>
>> I don't know what value MAX_GENPD_STATES should have but perhaps 8 is
>> good enough. In that way the number of supported states will be
>> "static". It's not the nicest thing to do, but I can't figure out a
>> better option.
>
> Ok understood, i had a doubt because in my first couple of series i was doing
> that, but there was a suggestion form Geert to rather allocate the
> states to save the unused memory.
> Is it ok if i implement it with  static struct for now to keep this
> series small?

>From my point of view, yes.

>
> Otherwise, i will implement the error code return and checking from
> pm_genpd_init.
>

You may try that, but it will for sure involve a lot more changes.

Perhaps we should postpone that until the pm_genpd_remove() API has
been merged, as it seems somewhat related.
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/404504.html

Kind regards
Uffe

Re: [RFC v11 1/3] PM / Domains: Support for multiple states

[Axel Haslam]

On Wed, Feb 10, 2016 at 11:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> + Jon
>
>>>>> >  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 */
>>>>> > @@ -66,6 +71,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;
>>>
>>> I suggest we change this to:
>>>
>>> struct genpd_power_state states[MAX_GENPD_STATES];
>>>
>>> I don't know what value MAX_GENPD_STATES should have but perhaps 8 is
>>> good enough. In that way the number of supported states will be
>>> "static". It's not the nicest thing to do, but I can't figure out a
>>> better option.
>>
>> Ok understood, i had a doubt because in my first couple of series i was doing
>> that, but there was a suggestion form Geert to rather allocate the
>> states to save the unused memory.
>> Is it ok if i implement it with  static struct for now to keep this
>> series small?
>
> From my point of view, yes.


Ok,, i will go for static state table in v12

Thanks,
Axel

>
>>
>> Otherwise, i will implement the error code return and checking from
>> pm_genpd_init.
>>
>
> You may try that, but it will for sure involve a lot more changes.
>
> Perhaps we should postpone that until the pm_genpd_remove() API has
> been merged, as it seems somewhat related.
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/404504.html
>
> Kind regards
> Uffe