[PATCH 0/4] PM / Domains: Some simplifications/clean-ups for genpd

[PATCH 0/4] PM / Domains: Some simplifications/clean-ups for genpd

[Ulf Hansson]

Ulf Hansson (4):
  PM / Domains: Simplify detaching a device from its genpd
  PM / Domains: Remove redundant system PM callbacks
  PM / Domains: Don't measure latency of ->power_on|off() during system
    PM
  PM / Domains: Rename pm_genpd_sync_poweron|poweroff()

 drivers/base/power/domain.c | 77 +++++++++++++++++++--------------------------
 1 file changed, 33 insertions(+), 44 deletions(-)

-- 
1.9.1

[PATCH 1/4] PM / Domains: Simplify detaching a device from its genpd

[Ulf Hansson]

There's no need to validate the PM domain by using genpd_lookup_dev() when
removing the device via genpd's genpd_dev_pm_detach() function. That's
because this function can't be called, unless there is a valid PM domain
for the device.

To simplify the behaviour, let's move code from pm_genpd_remove_device()
into a new internal function, genpd_remove_device(), which is called from
pm_genpd_remove_device() and genpd_dev_pm_detach().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 83ae3d7..63f0f8b 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1121,13 +1121,8 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 }
 EXPORT_SYMBOL_GPL(__pm_genpd_add_device);
 
-/**
- * pm_genpd_remove_device - Remove a device from an I/O PM domain.
- * @genpd: PM domain to remove the device from.
- * @dev: Device to be removed.
- */
-int pm_genpd_remove_device(struct generic_pm_domain *genpd,
-			   struct device *dev)
+static int genpd_remove_device(struct generic_pm_domain *genpd,
+			       struct device *dev)
 {
 	struct generic_pm_domain_data *gpd_data;
 	struct pm_domain_data *pdd;
@@ -1135,10 +1130,6 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	dev_dbg(dev, "%s()\n", __func__);
 
-	if (!genpd || genpd != genpd_lookup_dev(dev))
-		return -EINVAL;
-
-	/* The above validation also means we have existing domain_data. */
 	pdd = dev->power.subsys_data->domain_data;
 	gpd_data = to_gpd_data(pdd);
 	dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
@@ -1170,6 +1161,20 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	return ret;
 }
+
+/**
+ * pm_genpd_remove_device - Remove a device from an I/O PM domain.
+ * @genpd: PM domain to remove the device from.
+ * @dev: Device to be removed.
+ */
+int pm_genpd_remove_device(struct generic_pm_domain *genpd,
+			   struct device *dev)
+{
+	if (!genpd || genpd != genpd_lookup_dev(dev))
+		return -EINVAL;
+
+	return genpd_remove_device(genpd, dev);
+}
 EXPORT_SYMBOL_GPL(pm_genpd_remove_device);
 
 static int genpd_add_subdomain(struct generic_pm_domain *genpd,
@@ -1797,14 +1802,14 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
 	unsigned int i;
 	int ret = 0;
 
-	pd = genpd_lookup_dev(dev);
-	if (!pd)
+	pd = dev_to_genpd(dev);
+	if (IS_ERR(pd))
 		return;
 
 	dev_dbg(dev, "removing from PM domain %s\n", pd->name);
 
 	for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) {
-		ret = pm_genpd_remove_device(pd, dev);
+		ret = genpd_remove_device(pd, dev);
 		if (ret != -EAGAIN)
 			break;
 
-- 
1.9.1

Re: [PATCH 1/4] PM / Domains: Simplify detaching a device from its genpd

[Lina Iyer]

On Wed, Sep 21 2016 at 07:39 -0600, Ulf Hansson wrote:
>There's no need to validate the PM domain by using genpd_lookup_dev() when
>removing the device via genpd's genpd_dev_pm_detach() function. That's
>because this function can't be called, unless there is a valid PM domain
>for the device.
>
>To simplify the behaviour, let's move code from pm_genpd_remove_device()
>into a new internal function, genpd_remove_device(), which is called from
>pm_genpd_remove_device() and genpd_dev_pm_detach().
>
>Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Lina Iyer <lina.iyer@linaro.org>

>---
> drivers/base/power/domain.c | 33 +++++++++++++++++++--------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>index 83ae3d7..63f0f8b 100644
>--- a/drivers/base/power/domain.c
>+++ b/drivers/base/power/domain.c
>@@ -1121,13 +1121,8 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> }
> EXPORT_SYMBOL_GPL(__pm_genpd_add_device);
>
>-/**
>- * pm_genpd_remove_device - Remove a device from an I/O PM domain.
>- * @genpd: PM domain to remove the device from.
>- * @dev: Device to be removed.
>- */
>-int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>-			   struct device *dev)
>+static int genpd_remove_device(struct generic_pm_domain *genpd,
>+			       struct device *dev)
> {
> 	struct generic_pm_domain_data *gpd_data;
> 	struct pm_domain_data *pdd;
>@@ -1135,10 +1130,6 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>
> 	dev_dbg(dev, "%s()\n", __func__);
>
>-	if (!genpd || genpd != genpd_lookup_dev(dev))
>-		return -EINVAL;
>-
>-	/* The above validation also means we have existing domain_data. */
> 	pdd = dev->power.subsys_data->domain_data;
> 	gpd_data = to_gpd_data(pdd);
> 	dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
>@@ -1170,6 +1161,20 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>
> 	return ret;
> }
>+
>+/**
>+ * pm_genpd_remove_device - Remove a device from an I/O PM domain.
>+ * @genpd: PM domain to remove the device from.
>+ * @dev: Device to be removed.
>+ */
>+int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>+			   struct device *dev)
>+{
>+	if (!genpd || genpd != genpd_lookup_dev(dev))
>+		return -EINVAL;
>+
>+	return genpd_remove_device(genpd, dev);
>+}
> EXPORT_SYMBOL_GPL(pm_genpd_remove_device);
>
> static int genpd_add_subdomain(struct generic_pm_domain *genpd,
>@@ -1797,14 +1802,14 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
> 	unsigned int i;
> 	int ret = 0;
>
>-	pd = genpd_lookup_dev(dev);
>-	if (!pd)
>+	pd = dev_to_genpd(dev);
>+	if (IS_ERR(pd))
> 		return;
>
> 	dev_dbg(dev, "removing from PM domain %s\n", pd->name);
>
> 	for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) {
>-		ret = pm_genpd_remove_device(pd, dev);
>+		ret = genpd_remove_device(pd, dev);
> 		if (ret != -EAGAIN)
> 			break;
>
>-- 
>1.9.1
>

[PATCH 2/4] PM / Domains: Remove redundant system PM callbacks

[Ulf Hansson]

In cases when the PM domain haven't assigned a system PM callback, the PM
core fall-backs to check for the callback at the driver level instead.
This makes it redundant to assign a pm_generic_* helper function to a
corresponding system PM callback at a PM domain level.

Therefore, let's remove these assignments in pm_genpd_init().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 63f0f8b..661ac3a 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1316,24 +1316,12 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 	genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
 	genpd->domain.ops.runtime_resume = genpd_runtime_resume;
 	genpd->domain.ops.prepare = pm_genpd_prepare;
-	genpd->domain.ops.suspend = pm_generic_suspend;
-	genpd->domain.ops.suspend_late = pm_generic_suspend_late;
 	genpd->domain.ops.suspend_noirq = pm_genpd_suspend_noirq;
 	genpd->domain.ops.resume_noirq = pm_genpd_resume_noirq;
-	genpd->domain.ops.resume_early = pm_generic_resume_early;
-	genpd->domain.ops.resume = pm_generic_resume;
-	genpd->domain.ops.freeze = pm_generic_freeze;
-	genpd->domain.ops.freeze_late = pm_generic_freeze_late;
 	genpd->domain.ops.freeze_noirq = pm_genpd_freeze_noirq;
 	genpd->domain.ops.thaw_noirq = pm_genpd_thaw_noirq;
-	genpd->domain.ops.thaw_early = pm_generic_thaw_early;
-	genpd->domain.ops.thaw = pm_generic_thaw;
-	genpd->domain.ops.poweroff = pm_generic_poweroff;
-	genpd->domain.ops.poweroff_late = pm_generic_poweroff_late;
 	genpd->domain.ops.poweroff_noirq = pm_genpd_suspend_noirq;
 	genpd->domain.ops.restore_noirq = pm_genpd_restore_noirq;
-	genpd->domain.ops.restore_early = pm_generic_restore_early;
-	genpd->domain.ops.restore = pm_generic_restore;
 	genpd->domain.ops.complete = pm_genpd_complete;
 
 	if (genpd->flags & GENPD_FLAG_PM_CLK) {
-- 
1.9.1

[PATCH 3/4] PM / Domains: Don't measure latency of ->power_on|off() during system PM

[Ulf Hansson]

Measure latency does by itself contribute to an increased latency, thus we
should avoid it when it isn't needed.

Currently genpd measures latencies in the system PM phase for the
->power_on|off() callbacks, except in the syscore case when it's not
allowed to use ktime_get() as timekeeping may be suspended.

Since there should be plenty of occasions during runtime PM to perform
these measurements, let's rely on that and drop them from system PM. This
will also make it consistent for how measurements are done of the runtime
PM callbacks (as those may be invoked during system PM).

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 661ac3a..d52709e 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -619,7 +619,6 @@ static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd,
 /**
  * pm_genpd_sync_poweroff - Synchronously power off a PM domain and its masters.
  * @genpd: PM domain to power off, if possible.
- * @timed: True if latency measurements are allowed.
  *
  * Check if the given PM domain can be powered off (during system suspend or
  * hibernation) and do that if so.  Also, in that case propagate to its masters.
@@ -629,8 +628,7 @@ static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd,
  * executed sequentially, so it is guaranteed that it will never run twice in
  * parallel).
  */
-static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd,
-				   bool timed)
+static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd)
 {
 	struct gpd_link *link;
 
@@ -643,28 +641,26 @@ static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd,
 
 	/* Choose the deepest state when suspending */
 	genpd->state_idx = genpd->state_count - 1;
-	genpd_power_off(genpd, timed);
+	genpd_power_off(genpd, false);
 
 	genpd->status = GPD_STATE_POWER_OFF;
 
 	list_for_each_entry(link, &genpd->slave_links, slave_node) {
 		genpd_sd_counter_dec(link->master);
-		pm_genpd_sync_poweroff(link->master, timed);
+		pm_genpd_sync_poweroff(link->master);
 	}
 }
 
 /**
  * pm_genpd_sync_poweron - Synchronously power on a PM domain and its masters.
  * @genpd: PM domain to power on.
- * @timed: True if latency measurements are allowed.
  *
  * This function is only called in "noirq" and "syscore" stages of system power
  * transitions, so it need not acquire locks (all of the "noirq" callbacks are
  * executed sequentially, so it is guaranteed that it will never run twice in
  * parallel).
  */
-static void pm_genpd_sync_poweron(struct generic_pm_domain *genpd,
-				  bool timed)
+static void pm_genpd_sync_poweron(struct generic_pm_domain *genpd)
 {
 	struct gpd_link *link;
 
@@ -672,11 +668,11 @@ static void pm_genpd_sync_poweron(struct generic_pm_domain *genpd,
 		return;
 
 	list_for_each_entry(link, &genpd->slave_links, slave_node) {
-		pm_genpd_sync_poweron(link->master, timed);
+		pm_genpd_sync_poweron(link->master);
 		genpd_sd_counter_inc(link->master);
 	}
 
-	genpd_power_on(genpd, timed);
+	genpd_power_on(genpd, false);
 
 	genpd->status = GPD_STATE_ACTIVE;
 }
@@ -788,7 +784,7 @@ static int pm_genpd_suspend_noirq(struct device *dev)
 	 * the same PM domain, so it is not necessary to use locking here.
 	 */
 	genpd->suspended_count++;
-	pm_genpd_sync_poweroff(genpd, true);
+	pm_genpd_sync_poweroff(genpd);
 
 	return 0;
 }
@@ -818,7 +814,7 @@ static int pm_genpd_resume_noirq(struct device *dev)
 	 * guaranteed that this function will never run twice in parallel for
 	 * the same PM domain, so it is not necessary to use locking here.
 	 */
-	pm_genpd_sync_poweron(genpd, true);
+	pm_genpd_sync_poweron(genpd);
 	genpd->suspended_count--;
 
 	if (genpd->dev_ops.stop && genpd->dev_ops.start)
@@ -911,7 +907,7 @@ static int pm_genpd_restore_noirq(struct device *dev)
 		 */
 		genpd->status = GPD_STATE_POWER_OFF;
 
-	pm_genpd_sync_poweron(genpd, true);
+	pm_genpd_sync_poweron(genpd);
 
 	if (genpd->dev_ops.stop && genpd->dev_ops.start)
 		ret = pm_runtime_force_resume(dev);
@@ -966,9 +962,9 @@ static void genpd_syscore_switch(struct device *dev, bool suspend)
 
 	if (suspend) {
 		genpd->suspended_count++;
-		pm_genpd_sync_poweroff(genpd, false);
+		pm_genpd_sync_poweroff(genpd);
 	} else {
-		pm_genpd_sync_poweron(genpd, false);
+		pm_genpd_sync_poweron(genpd);
 		genpd->suspended_count--;
 	}
 }
-- 
1.9.1

[PATCH 4/4] PM / Domains: Rename pm_genpd_sync_poweron|poweroff()

[Ulf Hansson]

These are internal static functions to genpd. Let's conform to the naming
rules, by dropping the "pm_" prefix from these.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index d52709e..e023066 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -617,7 +617,7 @@ static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd,
 }
 
 /**
- * pm_genpd_sync_poweroff - Synchronously power off a PM domain and its masters.
+ * genpd_sync_poweroff - Synchronously power off a PM domain and its masters.
  * @genpd: PM domain to power off, if possible.
  *
  * Check if the given PM domain can be powered off (during system suspend or
@@ -628,7 +628,7 @@ static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd,
  * executed sequentially, so it is guaranteed that it will never run twice in
  * parallel).
  */
-static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd)
+static void genpd_sync_poweroff(struct generic_pm_domain *genpd)
 {
 	struct gpd_link *link;
 
@@ -647,12 +647,12 @@ static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd)
 
 	list_for_each_entry(link, &genpd->slave_links, slave_node) {
 		genpd_sd_counter_dec(link->master);
-		pm_genpd_sync_poweroff(link->master);
+		genpd_sync_poweroff(link->master);
 	}
 }
 
 /**
- * pm_genpd_sync_poweron - Synchronously power on a PM domain and its masters.
+ * genpd_sync_poweron - Synchronously power on a PM domain and its masters.
  * @genpd: PM domain to power on.
  *
  * This function is only called in "noirq" and "syscore" stages of system power
@@ -660,7 +660,7 @@ static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd)
  * executed sequentially, so it is guaranteed that it will never run twice in
  * parallel).
  */
-static void pm_genpd_sync_poweron(struct generic_pm_domain *genpd)
+static void genpd_sync_poweron(struct generic_pm_domain *genpd)
 {
 	struct gpd_link *link;
 
@@ -668,7 +668,7 @@ static void pm_genpd_sync_poweron(struct generic_pm_domain *genpd)
 		return;
 
 	list_for_each_entry(link, &genpd->slave_links, slave_node) {
-		pm_genpd_sync_poweron(link->master);
+		genpd_sync_poweron(link->master);
 		genpd_sd_counter_inc(link->master);
 	}
 
@@ -784,7 +784,7 @@ static int pm_genpd_suspend_noirq(struct device *dev)
 	 * the same PM domain, so it is not necessary to use locking here.
 	 */
 	genpd->suspended_count++;
-	pm_genpd_sync_poweroff(genpd);
+	genpd_sync_poweroff(genpd);
 
 	return 0;
 }
@@ -814,7 +814,7 @@ static int pm_genpd_resume_noirq(struct device *dev)
 	 * guaranteed that this function will never run twice in parallel for
 	 * the same PM domain, so it is not necessary to use locking here.
 	 */
-	pm_genpd_sync_poweron(genpd);
+	genpd_sync_poweron(genpd);
 	genpd->suspended_count--;
 
 	if (genpd->dev_ops.stop && genpd->dev_ops.start)
@@ -902,12 +902,12 @@ static int pm_genpd_restore_noirq(struct device *dev)
 	if (genpd->suspended_count++ == 0)
 		/*
 		 * The boot kernel might put the domain into arbitrary state,
-		 * so make it appear as powered off to pm_genpd_sync_poweron(),
+		 * so make it appear as powered off to genpd_sync_poweron(),
 		 * so that it tries to power it on in case it was really off.
 		 */
 		genpd->status = GPD_STATE_POWER_OFF;
 
-	pm_genpd_sync_poweron(genpd);
+	genpd_sync_poweron(genpd);
 
 	if (genpd->dev_ops.stop && genpd->dev_ops.start)
 		ret = pm_runtime_force_resume(dev);
@@ -962,9 +962,9 @@ static void genpd_syscore_switch(struct device *dev, bool suspend)
 
 	if (suspend) {
 		genpd->suspended_count++;
-		pm_genpd_sync_poweroff(genpd);
+		genpd_sync_poweroff(genpd);
 	} else {
-		pm_genpd_sync_poweron(genpd);
+		genpd_sync_poweron(genpd);
 		genpd->suspended_count--;
 	}
 }
-- 
1.9.1

Re: [PATCH 0/4] PM / Domains: Some simplifications/clean-ups for genpd

[Rafael J. Wysocki]

On Wednesday, September 21, 2016 03:38:49 PM Ulf Hansson wrote:
> Ulf Hansson (4):
>   PM / Domains: Simplify detaching a device from its genpd
>   PM / Domains: Remove redundant system PM callbacks
>   PM / Domains: Don't measure latency of ->power_on|off() during system
>     PM
>   PM / Domains: Rename pm_genpd_sync_poweron|poweroff()
> 
>  drivers/base/power/domain.c | 77 +++++++++++++++++++--------------------------
>  1 file changed, 33 insertions(+), 44 deletions(-)

This series is fine by me.

If there are no objections from Kevin or others, I'll queue it up later today.

Thanks,
Rafael

Re: [PATCH 0/4] PM / Domains: Some simplifications/clean-ups for genpd

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@rjwysocki.net> writes:

> On Wednesday, September 21, 2016 03:38:49 PM Ulf Hansson wrote:
>> Ulf Hansson (4):
>>   PM / Domains: Simplify detaching a device from its genpd
>>   PM / Domains: Remove redundant system PM callbacks
>>   PM / Domains: Don't measure latency of ->power_on|off() during system
>>     PM
>>   PM / Domains: Rename pm_genpd_sync_poweron|poweroff()
>> 
>>  drivers/base/power/domain.c | 77 +++++++++++++++++++--------------------------
>>  1 file changed, 33 insertions(+), 44 deletions(-)
>
> This series is fine by me.
>
> If there are no objections from Kevin or others, I'll queue it up later today.
>

No objections.

Acked-by: Kevin Hilman <khilman@baylibre.com>