[PATCH v2 0/3] PM / Domains: Add power on/off notifiers for genpd

[PATCH v2 0/3] PM / Domains: Add power on/off notifiers for genpd

[Ulf Hansson]

Changes in v2:
	- Improved error handling in patch3.

A device may have specific HW constraints that must be obeyed to, before its
corresponding PM domain (genpd) can be powered off - and vice verse at power
on. These constraints can't be managed through the regular runtime PM based
deployment for a device, because the access pattern for it, isn't always
request based. In other words, using the runtime PM callbacks to deal with the
constraints doesn't work for these cases.

For these reasons, this series introduces a power on/off notification mechanism
to genpd. To add/remove a notifier for a device, the device must already have
been attached to the genpd, which also means that it needs to be a part of the
PM domain topology.

The intent is to allow these genpd power on/off notifiers to replace the need
for the existing CPU_CLUSTER_PM_ENTER|EXIT notifiers. For example, those would
otherwise be needed in psci_pd_power_off() in cpuidle-psci-domain.c, when
powering off the CPU cluster.

Another series that enables drivers/soc/qcom/rpmh-rsc.c to make use of the new
genpd on/off notifiers, are soon to be posted. However, I would appreciate any
feedback on the approach taken, even before that series hits LKML.

Kind regards
Ulf Hansson


Ulf Hansson (3):
  PM / Domains: Rename power state enums for genpd
  PM / Domains: Allow to abort power off when no ->power_off() callback
  PM / Domains: Add support for PM domain on/off notifiers for genpd

 drivers/base/power/domain.c | 187 +++++++++++++++++++++++++++++-------
 include/linux/pm_domain.h   |  19 +++-
 2 files changed, 171 insertions(+), 35 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] PM / Domains: Rename power state enums for genpd

[Ulf Hansson]

To clarify the code a bit, let's rename GPD_STATE_ACTIVE into
GENPD_STATE_ON and GPD_STATE_POWER_OFF to GENPD_STATE_OFF.

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

Changes in v2:
	- None.

---
 drivers/base/power/domain.c | 33 ++++++++++++++++-----------------
 include/linux/pm_domain.h   |  4 ++--
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 2cb5e04cf86c..23aa2feced77 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -123,7 +123,7 @@ static const struct genpd_lock_ops genpd_spin_ops = {
 #define genpd_lock_interruptible(p)	p->lock_ops->lock_interruptible(p)
 #define genpd_unlock(p)			p->lock_ops->unlock(p)
 
-#define genpd_status_on(genpd)		(genpd->status == GPD_STATE_ACTIVE)
+#define genpd_status_on(genpd)		(genpd->status == GENPD_STATE_ON)
 #define genpd_is_irq_safe(genpd)	(genpd->flags & GENPD_FLAG_IRQ_SAFE)
 #define genpd_is_always_on(genpd)	(genpd->flags & GENPD_FLAG_ALWAYS_ON)
 #define genpd_is_active_wakeup(genpd)	(genpd->flags & GENPD_FLAG_ACTIVE_WAKEUP)
@@ -222,7 +222,7 @@ static void genpd_update_accounting(struct generic_pm_domain *genpd)
 	 * out of off and so update the idle time and vice
 	 * versa.
 	 */
-	if (genpd->status == GPD_STATE_ACTIVE) {
+	if (genpd->status == GENPD_STATE_ON) {
 		int state_idx = genpd->state_idx;
 
 		genpd->states[state_idx].idle_time =
@@ -563,7 +563,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 			return ret;
 	}
 
-	genpd->status = GPD_STATE_POWER_OFF;
+	genpd->status = GENPD_STATE_OFF;
 	genpd_update_accounting(genpd);
 
 	list_for_each_entry(link, &genpd->child_links, child_node) {
@@ -616,7 +616,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth)
 	if (ret)
 		goto err;
 
-	genpd->status = GPD_STATE_ACTIVE;
+	genpd->status = GENPD_STATE_ON;
 	genpd_update_accounting(genpd);
 
 	return 0;
@@ -961,7 +961,7 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
 	if (_genpd_power_off(genpd, false))
 		return;
 
-	genpd->status = GPD_STATE_POWER_OFF;
+	genpd->status = GENPD_STATE_OFF;
 
 	list_for_each_entry(link, &genpd->child_links, child_node) {
 		genpd_sd_counter_dec(link->parent);
@@ -1007,8 +1007,7 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock,
 	}
 
 	_genpd_power_on(genpd, false);
-
-	genpd->status = GPD_STATE_ACTIVE;
+	genpd->status = GENPD_STATE_ON;
 }
 
 /**
@@ -1287,7 +1286,7 @@ static int genpd_restore_noirq(struct device *dev)
 		 * so make it appear as powered off to genpd_sync_power_on(),
 		 * so that it tries to power it on in case it was really off.
 		 */
-		genpd->status = GPD_STATE_POWER_OFF;
+		genpd->status = GENPD_STATE_OFF;
 
 	genpd_sync_power_on(genpd, true, 0);
 	genpd_unlock(genpd);
@@ -1777,7 +1776,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	atomic_set(&genpd->sd_count, 0);
-	genpd->status = is_off ? GPD_STATE_POWER_OFF : GPD_STATE_ACTIVE;
+	genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
 	genpd->device_count = 0;
 	genpd->max_off_time_ns = -1;
 	genpd->max_off_time_changed = true;
@@ -2802,8 +2801,8 @@ static int genpd_summary_one(struct seq_file *s,
 			struct generic_pm_domain *genpd)
 {
 	static const char * const status_lookup[] = {
-		[GPD_STATE_ACTIVE] = "on",
-		[GPD_STATE_POWER_OFF] = "off"
+		[GENPD_STATE_ON] = "on",
+		[GENPD_STATE_OFF] = "off"
 	};
 	struct pm_domain_data *pm_data;
 	const char *kobj_path;
@@ -2881,8 +2880,8 @@ static int summary_show(struct seq_file *s, void *data)
 static int status_show(struct seq_file *s, void *data)
 {
 	static const char * const status_lookup[] = {
-		[GPD_STATE_ACTIVE] = "on",
-		[GPD_STATE_POWER_OFF] = "off"
+		[GENPD_STATE_ON] = "on",
+		[GENPD_STATE_OFF] = "off"
 	};
 
 	struct generic_pm_domain *genpd = s->private;
@@ -2895,7 +2894,7 @@ static int status_show(struct seq_file *s, void *data)
 	if (WARN_ON_ONCE(genpd->status >= ARRAY_SIZE(status_lookup)))
 		goto exit;
 
-	if (genpd->status == GPD_STATE_POWER_OFF)
+	if (genpd->status == GENPD_STATE_OFF)
 		seq_printf(s, "%s-%u\n", status_lookup[genpd->status],
 			genpd->state_idx);
 	else
@@ -2938,7 +2937,7 @@ static int idle_states_show(struct seq_file *s, void *data)
 		ktime_t delta = 0;
 		s64 msecs;
 
-		if ((genpd->status == GPD_STATE_POWER_OFF) &&
+		if ((genpd->status == GENPD_STATE_OFF) &&
 				(genpd->state_idx == i))
 			delta = ktime_sub(ktime_get(), genpd->accounting_time);
 
@@ -2961,7 +2960,7 @@ static int active_time_show(struct seq_file *s, void *data)
 	if (ret)
 		return -ERESTARTSYS;
 
-	if (genpd->status == GPD_STATE_ACTIVE)
+	if (genpd->status == GENPD_STATE_ON)
 		delta = ktime_sub(ktime_get(), genpd->accounting_time);
 
 	seq_printf(s, "%lld ms\n", ktime_to_ms(
@@ -2984,7 +2983,7 @@ static int total_idle_time_show(struct seq_file *s, void *data)
 
 	for (i = 0; i < genpd->state_count; i++) {
 
-		if ((genpd->status == GPD_STATE_POWER_OFF) &&
+		if ((genpd->status == GENPD_STATE_OFF) &&
 				(genpd->state_idx == i))
 			delta = ktime_sub(ktime_get(), genpd->accounting_time);
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index ee11502a575b..66f3c5d64d81 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -64,8 +64,8 @@
 #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
 
 enum gpd_status {
-	GPD_STATE_ACTIVE = 0,	/* PM domain is active */
-	GPD_STATE_POWER_OFF,	/* PM domain is off */
+	GENPD_STATE_ON = 0,	/* PM domain is on */
+	GENPD_STATE_OFF,	/* PM domain is off */
 };
 
 struct dev_power_governor {
-- 
2.25.1

[PATCH v2 2/3] PM / Domains: Allow to abort power off when no ->power_off() callback

[Ulf Hansson]

In genpd_power_off() we may decide to abort the power off of the PM domain,
even beyond the point when the governor would accept it. The abort is done
if it turns out that a child domain has been requested to be powered on,
which means it's waiting for the lock of the parent to be released.

However, the abort is currently only considered if the genpd in question
has a ->power_off() callback assigned. This is unnecessary limiting,
especially if the genpd would have a parent of its own. Let's remove the
limitation and make the behaviour consistent.

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

Changes in v2:
	- None.

---
 drivers/base/power/domain.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 23aa2feced77..0198af358503 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -497,6 +497,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 	struct pm_domain_data *pdd;
 	struct gpd_link *link;
 	unsigned int not_suspended = 0;
+	int ret;
 
 	/*
 	 * Do not try to power off the domain in the following situations:
@@ -544,24 +545,13 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 	if (!genpd->gov)
 		genpd->state_idx = 0;
 
-	if (genpd->power_off) {
-		int ret;
-
-		if (atomic_read(&genpd->sd_count) > 0)
-			return -EBUSY;
+	/* Don't power off, if a child domain is waiting to power on. */
+	if (atomic_read(&genpd->sd_count) > 0)
+		return -EBUSY;
 
-		/*
-		 * If sd_count > 0 at this point, one of the subdomains hasn't
-		 * managed to call genpd_power_on() for the parent yet after
-		 * incrementing it.  In that case genpd_power_on() will wait
-		 * for us to drop the lock, so we can call .power_off() and let
-		 * the genpd_power_on() restore power for us (this shouldn't
-		 * happen very often).
-		 */
-		ret = _genpd_power_off(genpd, true);
-		if (ret)
-			return ret;
-	}
+	ret = _genpd_power_off(genpd, true);
+	if (ret)
+		return ret;
 
 	genpd->status = GENPD_STATE_OFF;
 	genpd_update_accounting(genpd);
-- 
2.25.1

[PATCH v2 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd

[Ulf Hansson]

A device may have specific HW constraints that must be obeyed to, before
its corresponding PM domain (genpd) can be powered off - and vice verse at
power on. These constraints can't be managed through the regular runtime PM
based deployment for a device, because the access pattern for it, isn't
always request based. In other words, using the runtime PM callbacks to
deal with the constraints doesn't work for these cases.

For these reasons, let's instead add a PM domain power on/off notification
mechanism to genpd. To add/remove a notifier for a device, the device must
already have been attached to the genpd, which also means that it needs to
be a part of the PM domain topology.

To add/remove a notifier, let's introduce two genpd specific functions:
 - dev_pm_genpd_add|remove_notifier()

Note that, to further clarify when genpd power on/off notifiers may be
used, one can compare with the existing CPU_CLUSTER_PM_ENTER|EXIT
notifiers. In the long run, the genpd power on/off notifiers should be able
to replace them, but that requires additional genpd based platform support
for the current users.

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

Changes in v2:
	- Improved error handling from fired notifications, according to
	suggestions by Lina Iyer.

---
 drivers/base/power/domain.c | 142 ++++++++++++++++++++++++++++++++++--
 include/linux/pm_domain.h   |  15 ++++
 2 files changed, 152 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0198af358503..f001ac6326fb 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -497,7 +497,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 	struct pm_domain_data *pdd;
 	struct gpd_link *link;
 	unsigned int not_suspended = 0;
-	int ret;
+	int ret, nr_calls = 0;
 
 	/*
 	 * Do not try to power off the domain in the following situations:
@@ -545,13 +545,22 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 	if (!genpd->gov)
 		genpd->state_idx = 0;
 
+	/* Notify consumers that we are about to power off. */
+	ret = __raw_notifier_call_chain(&genpd->power_notifiers,
+					GENPD_STATE_OFF, NULL, -1, &nr_calls);
+	ret = notifier_to_errno(ret);
+	if (ret)
+		goto busy;
+
 	/* Don't power off, if a child domain is waiting to power on. */
-	if (atomic_read(&genpd->sd_count) > 0)
-		return -EBUSY;
+	if (atomic_read(&genpd->sd_count) > 0) {
+		ret = -EBUSY;
+		goto busy;
+	}
 
 	ret = _genpd_power_off(genpd, true);
 	if (ret)
-		return ret;
+		goto busy;
 
 	genpd->status = GENPD_STATE_OFF;
 	genpd_update_accounting(genpd);
@@ -564,6 +573,12 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 	}
 
 	return 0;
+busy:
+	if (nr_calls)
+		__raw_notifier_call_chain(&genpd->power_notifiers,
+					  GENPD_STATE_ON, NULL,
+					  nr_calls - 1, NULL);
+	return ret;
 }
 
 /**
@@ -609,6 +624,9 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth)
 	genpd->status = GENPD_STATE_ON;
 	genpd_update_accounting(genpd);
 
+	/* Inform consumers that we have powered on. */
+	raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, NULL);
+
 	return 0;
 
  err:
@@ -938,6 +956,7 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
 				 unsigned int depth)
 {
 	struct gpd_link *link;
+	int err, nr_calls = 0;
 
 	if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
 		return;
@@ -948,8 +967,15 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
 
 	/* Choose the deepest state when suspending */
 	genpd->state_idx = genpd->state_count - 1;
+
+	/* Notify consumers that we are about to power off. */
+	err = __raw_notifier_call_chain(&genpd->power_notifiers,
+					GENPD_STATE_OFF, NULL, -1, &nr_calls);
+	if (notifier_to_errno(err))
+		goto err;
+
 	if (_genpd_power_off(genpd, false))
-		return;
+		goto err;
 
 	genpd->status = GENPD_STATE_OFF;
 
@@ -964,6 +990,13 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
 		if (use_lock)
 			genpd_unlock(link->parent);
 	}
+
+	return;
+err:
+	if (nr_calls)
+		__raw_notifier_call_chain(&genpd->power_notifiers,
+					  GENPD_STATE_ON, NULL,
+					  nr_calls - 1, NULL);
 }
 
 /**
@@ -998,6 +1031,9 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock,
 
 	_genpd_power_on(genpd, false);
 	genpd->status = GENPD_STATE_ON;
+
+	/* Inform consumers that we have powered on. */
+	raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON, NULL);
 }
 
 /**
@@ -1592,6 +1628,101 @@ int pm_genpd_remove_device(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(pm_genpd_remove_device);
 
+/**
+ * dev_pm_genpd_add_notifier - Add a genpd power on/off notifier for @dev
+ *
+ * @dev: Device that should be associated with the notifier
+ * @nb: The notifier block to register
+ *
+ * Users may call this function to add a genpd power on/off notifier for an
+ * attached @dev. Only one notifier per device is allowed. The notifier is
+ * sent when genpd is powering on/off the PM domain.
+ *
+ * It is assumed that the user guarantee that the genpd wouldn't be detached
+ * while this routine is getting called.
+ *
+ * Returns 0 on success and negative error values on failures.
+ */
+int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb)
+{
+	struct generic_pm_domain *genpd;
+	struct generic_pm_domain_data *gpd_data;
+	int ret;
+
+	genpd = dev_to_genpd_safe(dev);
+	if (!genpd)
+		return -ENODEV;
+
+	if (WARN_ON(!dev->power.subsys_data ||
+		     !dev->power.subsys_data->domain_data))
+		return -EINVAL;
+
+	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+	if (gpd_data->power_nb)
+		return -EEXIST;
+
+	genpd_lock(genpd);
+	ret = raw_notifier_chain_register(&genpd->power_notifiers, nb);
+	genpd_unlock(genpd);
+
+	if (ret) {
+		dev_warn(dev, "failed to add notifier for PM domain %s\n",
+			 genpd->name);
+		return ret;
+	}
+
+	gpd_data->power_nb = nb;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_add_notifier);
+
+/**
+ * dev_pm_genpd_remove_notifier - Remove a genpd power on/off notifier for @dev
+ *
+ * @dev: Device that is associated with the notifier
+ *
+ * Users may call this function to remove a genpd power on/off notifier for an
+ * attached @dev.
+ *
+ * It is assumed that the user guarantee that the genpd wouldn't be detached
+ * while this routine is getting called.
+ *
+ * Returns 0 on success and negative error values on failures.
+ */
+int dev_pm_genpd_remove_notifier(struct device *dev)
+{
+	struct generic_pm_domain *genpd;
+	struct generic_pm_domain_data *gpd_data;
+	int ret;
+
+	genpd = dev_to_genpd_safe(dev);
+	if (!genpd)
+		return -ENODEV;
+
+	if (WARN_ON(!dev->power.subsys_data ||
+		     !dev->power.subsys_data->domain_data))
+		return -EINVAL;
+
+	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+	if (!gpd_data->power_nb)
+		return -ENODEV;
+
+	genpd_lock(genpd);
+	ret = raw_notifier_chain_unregister(&genpd->power_notifiers,
+					    gpd_data->power_nb);
+	genpd_unlock(genpd);
+
+	if (ret) {
+		dev_warn(dev, "failed to remove notifier for PM domain %s\n",
+			 genpd->name);
+		return ret;
+	}
+
+	gpd_data->power_nb = NULL;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_remove_notifier);
+
 static int genpd_add_subdomain(struct generic_pm_domain *genpd,
 			       struct generic_pm_domain *subdomain)
 {
@@ -1762,6 +1893,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 	INIT_LIST_HEAD(&genpd->parent_links);
 	INIT_LIST_HEAD(&genpd->child_links);
 	INIT_LIST_HEAD(&genpd->dev_list);
+	RAW_INIT_NOTIFIER_HEAD(&genpd->power_notifiers);
 	genpd_lock_init(genpd);
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 66f3c5d64d81..3b2b561ce846 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -112,6 +112,7 @@ struct generic_pm_domain {
 	cpumask_var_t cpus;		/* A cpumask of the attached CPUs */
 	int (*power_off)(struct generic_pm_domain *domain);
 	int (*power_on)(struct generic_pm_domain *domain);
+	struct raw_notifier_head power_notifiers; /* Power on/off notifiers */
 	struct opp_table *opp_table;	/* OPP table of the genpd */
 	unsigned int (*opp_to_performance_state)(struct generic_pm_domain *genpd,
 						 struct dev_pm_opp *opp);
@@ -178,6 +179,7 @@ struct generic_pm_domain_data {
 	struct pm_domain_data base;
 	struct gpd_timing_data td;
 	struct notifier_block nb;
+	struct notifier_block *power_nb;
 	int cpu;
 	unsigned int performance_state;
 	void *data;
@@ -204,6 +206,8 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 		  struct dev_power_governor *gov, bool is_off);
 int pm_genpd_remove(struct generic_pm_domain *genpd);
 int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
+int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb);
+int dev_pm_genpd_remove_notifier(struct device *dev);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -251,6 +255,17 @@ static inline int dev_pm_genpd_set_performance_state(struct device *dev,
 	return -ENOTSUPP;
 }
 
+static inline int dev_pm_genpd_add_notifier(struct device *dev,
+					    struct notifier_block *nb)
+{
+	return -ENOTSUPP;
+}
+
+static inline int dev_pm_genpd_remove_notifier(struct device *dev)
+{
+	return -ENOTSUPP;
+}
+
 #define simple_qos_governor		(*(struct dev_power_governor *)(NULL))
 #define pm_domain_always_on_gov		(*(struct dev_power_governor *)(NULL))
 #endif
-- 
2.25.1

RE: [PATCH v2 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd

[Peng Fan]

Hi Ulf,

> Subject: [PATCH v2 3/3] PM / Domains: Add support for PM domain on/off
> notifiers for genpd
> 
> A device may have specific HW constraints that must be obeyed to, before its
> corresponding PM domain (genpd) can be powered off - and vice verse at
> power on. These constraints can't be managed through the regular runtime
> PM based deployment for a device, because the access pattern for it, isn't
> always request based. In other words, using the runtime PM callbacks to deal
> with the constraints doesn't work for these cases.

Could the notification be added before/after power on, and before/after power
off? not just after power on and before power off?

Our SoC has a requirement that before power on/off the specific module,
the corresponding clk needs to be on to make sure the hardware async
bridge could finish handshake.

So we need clk_prepare_on/off to guard power on and power off as below:

clk_prepare_on
power on
clk_prepare_off

clk_prepare_on
power off
clk_prepare_off

Thanks,
Peng.

> 
> For these reasons, let's instead add a PM domain power on/off notification
> mechanism to genpd. To add/remove a notifier for a device, the device must
> already have been attached to the genpd, which also means that it needs to
> be a part of the PM domain topology.
> 
> To add/remove a notifier, let's introduce two genpd specific functions:
>  - dev_pm_genpd_add|remove_notifier()
> 
> Note that, to further clarify when genpd power on/off notifiers may be used,
> one can compare with the existing CPU_CLUSTER_PM_ENTER|EXIT notifiers.
> In the long run, the genpd power on/off notifiers should be able to replace
> them, but that requires additional genpd based platform support for the
> current users.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Changes in v2:
> 	- Improved error handling from fired notifications, according to
> 	suggestions by Lina Iyer.
> 
> ---
>  drivers/base/power/domain.c | 142
> ++++++++++++++++++++++++++++++++++--
>  include/linux/pm_domain.h   |  15 ++++
>  2 files changed, 152 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0198af358503..f001ac6326fb 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -497,7 +497,7 @@ static int genpd_power_off(struct
> generic_pm_domain *genpd, bool one_dev_on,
>  	struct pm_domain_data *pdd;
>  	struct gpd_link *link;
>  	unsigned int not_suspended = 0;
> -	int ret;
> +	int ret, nr_calls = 0;
> 
>  	/*
>  	 * Do not try to power off the domain in the following situations:
> @@ -545,13 +545,22 @@ static int genpd_power_off(struct
> generic_pm_domain *genpd, bool one_dev_on,
>  	if (!genpd->gov)
>  		genpd->state_idx = 0;
> 
> +	/* Notify consumers that we are about to power off. */
> +	ret = __raw_notifier_call_chain(&genpd->power_notifiers,
> +					GENPD_STATE_OFF, NULL, -1, &nr_calls);
> +	ret = notifier_to_errno(ret);
> +	if (ret)
> +		goto busy;
> +
>  	/* Don't power off, if a child domain is waiting to power on. */
> -	if (atomic_read(&genpd->sd_count) > 0)
> -		return -EBUSY;
> +	if (atomic_read(&genpd->sd_count) > 0) {
> +		ret = -EBUSY;
> +		goto busy;
> +	}
> 
>  	ret = _genpd_power_off(genpd, true);
>  	if (ret)
> -		return ret;
> +		goto busy;
> 
>  	genpd->status = GENPD_STATE_OFF;
>  	genpd_update_accounting(genpd);
> @@ -564,6 +573,12 @@ static int genpd_power_off(struct
> generic_pm_domain *genpd, bool one_dev_on,
>  	}
> 
>  	return 0;
> +busy:
> +	if (nr_calls)
> +		__raw_notifier_call_chain(&genpd->power_notifiers,
> +					  GENPD_STATE_ON, NULL,
> +					  nr_calls - 1, NULL);
> +	return ret;
>  }
> 
>  /**
> @@ -609,6 +624,9 @@ static int genpd_power_on(struct
> generic_pm_domain *genpd, unsigned int depth)
>  	genpd->status = GENPD_STATE_ON;
>  	genpd_update_accounting(genpd);
> 
> +	/* Inform consumers that we have powered on. */
> +	raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON,
> +NULL);
> +
>  	return 0;
> 
>   err:
> @@ -938,6 +956,7 @@ static void genpd_sync_power_off(struct
> generic_pm_domain *genpd, bool use_lock,
>  				 unsigned int depth)
>  {
>  	struct gpd_link *link;
> +	int err, nr_calls = 0;
> 
>  	if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
>  		return;
> @@ -948,8 +967,15 @@ static void genpd_sync_power_off(struct
> generic_pm_domain *genpd, bool use_lock,
> 
>  	/* Choose the deepest state when suspending */
>  	genpd->state_idx = genpd->state_count - 1;
> +
> +	/* Notify consumers that we are about to power off. */
> +	err = __raw_notifier_call_chain(&genpd->power_notifiers,
> +					GENPD_STATE_OFF, NULL, -1, &nr_calls);
> +	if (notifier_to_errno(err))
> +		goto err;
> +
>  	if (_genpd_power_off(genpd, false))
> -		return;
> +		goto err;
> 
>  	genpd->status = GENPD_STATE_OFF;
> 
> @@ -964,6 +990,13 @@ static void genpd_sync_power_off(struct
> generic_pm_domain *genpd, bool use_lock,
>  		if (use_lock)
>  			genpd_unlock(link->parent);
>  	}
> +
> +	return;
> +err:
> +	if (nr_calls)
> +		__raw_notifier_call_chain(&genpd->power_notifiers,
> +					  GENPD_STATE_ON, NULL,
> +					  nr_calls - 1, NULL);
>  }
> 
>  /**
> @@ -998,6 +1031,9 @@ static void genpd_sync_power_on(struct
> generic_pm_domain *genpd, bool use_lock,
> 
>  	_genpd_power_on(genpd, false);
>  	genpd->status = GENPD_STATE_ON;
> +
> +	/* Inform consumers that we have powered on. */
> +	raw_notifier_call_chain(&genpd->power_notifiers, GENPD_STATE_ON,
> +NULL);
>  }
> 
>  /**
> @@ -1592,6 +1628,101 @@ int pm_genpd_remove_device(struct device
> *dev)  }  EXPORT_SYMBOL_GPL(pm_genpd_remove_device);
> 
> +/**
> + * dev_pm_genpd_add_notifier - Add a genpd power on/off notifier for
> +@dev
> + *
> + * @dev: Device that should be associated with the notifier
> + * @nb: The notifier block to register
> + *
> + * Users may call this function to add a genpd power on/off notifier
> +for an
> + * attached @dev. Only one notifier per device is allowed. The notifier
> +is
> + * sent when genpd is powering on/off the PM domain.
> + *
> + * It is assumed that the user guarantee that the genpd wouldn't be
> +detached
> + * while this routine is getting called.
> + *
> + * Returns 0 on success and negative error values on failures.
> + */
> +int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block
> +*nb) {
> +	struct generic_pm_domain *genpd;
> +	struct generic_pm_domain_data *gpd_data;
> +	int ret;
> +
> +	genpd = dev_to_genpd_safe(dev);
> +	if (!genpd)
> +		return -ENODEV;
> +
> +	if (WARN_ON(!dev->power.subsys_data ||
> +		     !dev->power.subsys_data->domain_data))
> +		return -EINVAL;
> +
> +	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
> +	if (gpd_data->power_nb)
> +		return -EEXIST;
> +
> +	genpd_lock(genpd);
> +	ret = raw_notifier_chain_register(&genpd->power_notifiers, nb);
> +	genpd_unlock(genpd);
> +
> +	if (ret) {
> +		dev_warn(dev, "failed to add notifier for PM domain %s\n",
> +			 genpd->name);
> +		return ret;
> +	}
> +
> +	gpd_data->power_nb = nb;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_add_notifier);
> +
> +/**
> + * dev_pm_genpd_remove_notifier - Remove a genpd power on/off notifier
> +for @dev
> + *
> + * @dev: Device that is associated with the notifier
> + *
> + * Users may call this function to remove a genpd power on/off notifier
> +for an
> + * attached @dev.
> + *
> + * It is assumed that the user guarantee that the genpd wouldn't be
> +detached
> + * while this routine is getting called.
> + *
> + * Returns 0 on success and negative error values on failures.
> + */
> +int dev_pm_genpd_remove_notifier(struct device *dev) {
> +	struct generic_pm_domain *genpd;
> +	struct generic_pm_domain_data *gpd_data;
> +	int ret;
> +
> +	genpd = dev_to_genpd_safe(dev);
> +	if (!genpd)
> +		return -ENODEV;
> +
> +	if (WARN_ON(!dev->power.subsys_data ||
> +		     !dev->power.subsys_data->domain_data))
> +		return -EINVAL;
> +
> +	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
> +	if (!gpd_data->power_nb)
> +		return -ENODEV;
> +
> +	genpd_lock(genpd);
> +	ret = raw_notifier_chain_unregister(&genpd->power_notifiers,
> +					    gpd_data->power_nb);
> +	genpd_unlock(genpd);
> +
> +	if (ret) {
> +		dev_warn(dev, "failed to remove notifier for PM domain %s\n",
> +			 genpd->name);
> +		return ret;
> +	}
> +
> +	gpd_data->power_nb = NULL;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_remove_notifier);
> +
>  static int genpd_add_subdomain(struct generic_pm_domain *genpd,
>  			       struct generic_pm_domain *subdomain)  { @@
> -1762,6 +1893,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>  	INIT_LIST_HEAD(&genpd->parent_links);
>  	INIT_LIST_HEAD(&genpd->child_links);
>  	INIT_LIST_HEAD(&genpd->dev_list);
> +	RAW_INIT_NOTIFIER_HEAD(&genpd->power_notifiers);
>  	genpd_lock_init(genpd);
>  	genpd->gov = gov;
>  	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn); diff
> --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index
> 66f3c5d64d81..3b2b561ce846 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -112,6 +112,7 @@ struct generic_pm_domain {
>  	cpumask_var_t cpus;		/* A cpumask of the attached CPUs */
>  	int (*power_off)(struct generic_pm_domain *domain);
>  	int (*power_on)(struct generic_pm_domain *domain);
> +	struct raw_notifier_head power_notifiers; /* Power on/off notifiers */
>  	struct opp_table *opp_table;	/* OPP table of the genpd */
>  	unsigned int (*opp_to_performance_state)(struct generic_pm_domain
> *genpd,
>  						 struct dev_pm_opp *opp);
> @@ -178,6 +179,7 @@ struct generic_pm_domain_data {
>  	struct pm_domain_data base;
>  	struct gpd_timing_data td;
>  	struct notifier_block nb;
> +	struct notifier_block *power_nb;
>  	int cpu;
>  	unsigned int performance_state;
>  	void *data;
> @@ -204,6 +206,8 @@ int pm_genpd_init(struct generic_pm_domain
> *genpd,
>  		  struct dev_power_governor *gov, bool is_off);  int
> pm_genpd_remove(struct generic_pm_domain *genpd);  int
> dev_pm_genpd_set_performance_state(struct device *dev, unsigned int
> state);
> +int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block
> +*nb); int dev_pm_genpd_remove_notifier(struct device *dev);
> 
>  extern struct dev_power_governor simple_qos_governor;  extern struct
> dev_power_governor pm_domain_always_on_gov; @@ -251,6 +255,17 @@
> static inline int dev_pm_genpd_set_performance_state(struct device *dev,
>  	return -ENOTSUPP;
>  }
> 
> +static inline int dev_pm_genpd_add_notifier(struct device *dev,
> +					    struct notifier_block *nb)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static inline int dev_pm_genpd_remove_notifier(struct device *dev) {
> +	return -ENOTSUPP;
> +}
> +
>  #define simple_qos_governor		(*(struct dev_power_governor
> *)(NULL))
>  #define pm_domain_always_on_gov		(*(struct dev_power_governor
> *)(NULL))
>  #endif
> --
> 2.25.1

Re: [PATCH v2 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd

[Ulf Hansson]

On Fri, 25 Sep 2020 at 08:08, Peng Fan <peng.fan@nxp.com> wrote:
>
> Hi Ulf,
>
> > Subject: [PATCH v2 3/3] PM / Domains: Add support for PM domain on/off
> > notifiers for genpd
> >
> > A device may have specific HW constraints that must be obeyed to, before its
> > corresponding PM domain (genpd) can be powered off - and vice verse at
> > power on. These constraints can't be managed through the regular runtime
> > PM based deployment for a device, because the access pattern for it, isn't
> > always request based. In other words, using the runtime PM callbacks to deal
> > with the constraints doesn't work for these cases.
>
> Could the notification be added before/after power on, and before/after power
> off? not just after power on and before power off?
>
> Our SoC has a requirement that before power on/off the specific module,
> the corresponding clk needs to be on to make sure the hardware async
> bridge could finish handshake.

Thanks for your comments!

May I ask, to be sure - does the clock correspond to the genpd
provider or is it a clock for the genpd consumer device?

If the former, couldn't the clock be managed from the ->power_on|off()
callbacks for the genpd provider?

>
> So we need clk_prepare_on/off to guard power on and power off as below:
>
> clk_prepare_on
> power on
> clk_prepare_off
>
> clk_prepare_on
> power off
> clk_prepare_off
>
> Thanks,
> Peng.
>

[...]

Kind regards
Uffe

RE: [PATCH v2 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd

[Peng Fan]

> Subject: Re: [PATCH v2 3/3] PM / Domains: Add support for PM domain on/off
> notifiers for genpd
> 
> On Fri, 25 Sep 2020 at 08:08, Peng Fan <peng.fan@nxp.com> wrote:
> >
> > Hi Ulf,
> >
> > > Subject: [PATCH v2 3/3] PM / Domains: Add support for PM domain
> > > on/off notifiers for genpd
> > >
> > > A device may have specific HW constraints that must be obeyed to,
> > > before its corresponding PM domain (genpd) can be powered off - and
> > > vice verse at power on. These constraints can't be managed through
> > > the regular runtime PM based deployment for a device, because the
> > > access pattern for it, isn't always request based. In other words,
> > > using the runtime PM callbacks to deal with the constraints doesn't work
> for these cases.
> >
> > Could the notification be added before/after power on, and
> > before/after power off? not just after power on and before power off?
> >
> > Our SoC has a requirement that before power on/off the specific
> > module, the corresponding clk needs to be on to make sure the hardware
> > async bridge could finish handshake.
> 
> Thanks for your comments!
> 
> May I ask, to be sure - does the clock correspond to the genpd provider or is it
> a clock for the genpd consumer device?

It is the clock for the genpd consumer device.

> 
> If the former, couldn't the clock be managed from the ->power_on|off()
> callbacks for the genpd provider?

Sadly not former.

Our current solution is to add a clock property to the power domain node(NXP ARM SIP
based power domain driver), and when power_on/off, we enable/disable the clocks.

But we are moving to use SCMI power domain, and leave the clock in Linux,
Scmi power domain driver is a generic driver, and we are not able to mix clock
parts in the driver which is bad.

Your patch gives me a light that if we could use notification to let consumer
device driver do the clock enable/disable for each power on and power off.
That would be great.


Thanks,
Peng.
> 
> >
> > So we need clk_prepare_on/off to guard power on and power off as below:
> >
> > clk_prepare_on
> > power on
> > clk_prepare_off
> >
> > clk_prepare_on
> > power off
> > clk_prepare_off
> >
> > Thanks,
> > Peng.
> >
> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH v2 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd

[Ulf Hansson]

On Fri, 25 Sep 2020 at 16:30, Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: Re: [PATCH v2 3/3] PM / Domains: Add support for PM domain on/off
> > notifiers for genpd
> >
> > On Fri, 25 Sep 2020 at 08:08, Peng Fan <peng.fan@nxp.com> wrote:
> > >
> > > Hi Ulf,
> > >
> > > > Subject: [PATCH v2 3/3] PM / Domains: Add support for PM domain
> > > > on/off notifiers for genpd
> > > >
> > > > A device may have specific HW constraints that must be obeyed to,
> > > > before its corresponding PM domain (genpd) can be powered off - and
> > > > vice verse at power on. These constraints can't be managed through
> > > > the regular runtime PM based deployment for a device, because the
> > > > access pattern for it, isn't always request based. In other words,
> > > > using the runtime PM callbacks to deal with the constraints doesn't work
> > for these cases.
> > >
> > > Could the notification be added before/after power on, and
> > > before/after power off? not just after power on and before power off?
> > >
> > > Our SoC has a requirement that before power on/off the specific
> > > module, the corresponding clk needs to be on to make sure the hardware
> > > async bridge could finish handshake.
> >
> > Thanks for your comments!
> >
> > May I ask, to be sure - does the clock correspond to the genpd provider or is it
> > a clock for the genpd consumer device?
>
> It is the clock for the genpd consumer device.
>
> >
> > If the former, couldn't the clock be managed from the ->power_on|off()
> > callbacks for the genpd provider?
>
> Sadly not former.
>
> Our current solution is to add a clock property to the power domain node(NXP ARM SIP
> based power domain driver), and when power_on/off, we enable/disable the clocks.
>
> But we are moving to use SCMI power domain, and leave the clock in Linux,
> Scmi power domain driver is a generic driver, and we are not able to mix clock
> parts in the driver which is bad.
>
> Your patch gives me a light that if we could use notification to let consumer
> device driver do the clock enable/disable for each power on and power off.
> That would be great.

Thanks for clarifying. Let me re-spin the patch to address your points.

[...]

Kind regards
Uffe

Re: [PATCH v2 0/3] PM / Domains: Add power on/off notifiers for genpd

[Ulf Hansson]

Rafael,

On Thu, 24 Sep 2020 at 13:06, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Changes in v2:
>         - Improved error handling in patch3.
>
> A device may have specific HW constraints that must be obeyed to, before its
> corresponding PM domain (genpd) can be powered off - and vice verse at power
> on. These constraints can't be managed through the regular runtime PM based
> deployment for a device, because the access pattern for it, isn't always
> request based. In other words, using the runtime PM callbacks to deal with the
> constraints doesn't work for these cases.
>
> For these reasons, this series introduces a power on/off notification mechanism
> to genpd. To add/remove a notifier for a device, the device must already have
> been attached to the genpd, which also means that it needs to be a part of the
> PM domain topology.
>
> The intent is to allow these genpd power on/off notifiers to replace the need
> for the existing CPU_CLUSTER_PM_ENTER|EXIT notifiers. For example, those would
> otherwise be needed in psci_pd_power_off() in cpuidle-psci-domain.c, when
> powering off the CPU cluster.
>
> Another series that enables drivers/soc/qcom/rpmh-rsc.c to make use of the new
> genpd on/off notifiers, are soon to be posted. However, I would appreciate any
> feedback on the approach taken, even before that series hits LKML.
>
> Kind regards
> Ulf Hansson
>
>
> Ulf Hansson (3):
>   PM / Domains: Rename power state enums for genpd
>   PM / Domains: Allow to abort power off when no ->power_off() callback
>   PM / Domains: Add support for PM domain on/off notifiers for genpd
>
>  drivers/base/power/domain.c | 187 +++++++++++++++++++++++++++++-------
>  include/linux/pm_domain.h   |  19 +++-
>  2 files changed, 171 insertions(+), 35 deletions(-)
>
> --
> 2.25.1
>

I will need to iterate patch 3, potentially even a couple of more times.

As I expect patch 1 and patch2 to not get changed, may I suggest that
you pick up those so we can move focus to patch3?

Kind regards
Uffe

Re: [PATCH v2 0/3] PM / Domains: Add power on/off notifiers for genpd

[Rafael J. Wysocki]

On Mon, Sep 28, 2020 at 1:57 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Rafael,
>
> On Thu, 24 Sep 2020 at 13:06, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > Changes in v2:
> >         - Improved error handling in patch3.
> >
> > A device may have specific HW constraints that must be obeyed to, before its
> > corresponding PM domain (genpd) can be powered off - and vice verse at power
> > on. These constraints can't be managed through the regular runtime PM based
> > deployment for a device, because the access pattern for it, isn't always
> > request based. In other words, using the runtime PM callbacks to deal with the
> > constraints doesn't work for these cases.
> >
> > For these reasons, this series introduces a power on/off notification mechanism
> > to genpd. To add/remove a notifier for a device, the device must already have
> > been attached to the genpd, which also means that it needs to be a part of the
> > PM domain topology.
> >
> > The intent is to allow these genpd power on/off notifiers to replace the need
> > for the existing CPU_CLUSTER_PM_ENTER|EXIT notifiers. For example, those would
> > otherwise be needed in psci_pd_power_off() in cpuidle-psci-domain.c, when
> > powering off the CPU cluster.
> >
> > Another series that enables drivers/soc/qcom/rpmh-rsc.c to make use of the new
> > genpd on/off notifiers, are soon to be posted. However, I would appreciate any
> > feedback on the approach taken, even before that series hits LKML.
> >
> > Kind regards
> > Ulf Hansson
> >
> >
> > Ulf Hansson (3):
> >   PM / Domains: Rename power state enums for genpd
> >   PM / Domains: Allow to abort power off when no ->power_off() callback
> >   PM / Domains: Add support for PM domain on/off notifiers for genpd
> >
> >  drivers/base/power/domain.c | 187 +++++++++++++++++++++++++++++-------
> >  include/linux/pm_domain.h   |  19 +++-
> >  2 files changed, 171 insertions(+), 35 deletions(-)
> >
> > --
> > 2.25.1
> >
>
> I will need to iterate patch 3, potentially even a couple of more times.
>
> As I expect patch 1 and patch2 to not get changed, may I suggest that
> you pick up those so we can move focus to patch3?

OK, [1-2/3] applied as 5.10 material with minor subject edits, thanks!