[PATCH 00/11] pmdomain: Add generic ->sync_state() support to genpd

[PATCH 00/11] pmdomain: Add generic ->sync_state() support to genpd

[Ulf Hansson]

If a PM domain (genpd) is powered-on during boot, there is probably a good
reason for it. Therefore it's known to be a bad idea to allow such genpd to be
powered-off before all of its consumer devices have been probed. This series
intends to fix this problem.

We have been discussing these issues at LKML and at various Linux-conferences
in the past. I have therefore tried to include the people I can recall being
involved, but I may have forgotten some (my apologies), feel free to loop them
in.

A few notes:
*)
Even if this looks good, the last patch can't go in without some additional
changes to a couple of existing genpd provider drivers. Typically genpd provider
drivers that implements ->sync_state() need to call of_genpd_sync_state(), but I
will fix this asap, if we think the series makes sense.

*)
Patch 1 -> 3 are just preparatory cleanups. 

*)
I have tested this with QEMU with a bunch of local test-drivers and DT nodes.
Let me know if you want me to share this code too.


Please help review and test!
Finally, a big thanks to Saravana for all the support!

Kind regards
Ulf Hansson


Saravana Kannan (1):
  driver core: Add dev_set_drv_sync_state()

Ulf Hansson (10):
  pmdomain: core: Convert genpd_power_off() to void
  pmdomain: core: Simplify return statement in genpd_power_off()
  pmdomain: core: Use genpd->opp_table to simplify error/remove path
  pmdomain: core: Add a bus and a driver for genpd providers
  pmdomain: core: Use device_set_node() to assign the fwnode too
  pmdomain: core: Add the genpd->dev to the genpd provider bus
  pmdomain: core: Export a common ->sync_state() helper for genpd
    providers
  pmdomain: core: Add internal ->sync_state() support for genpd
    providers
  pmdomain: core: Default to use of_genpd_sync_state() for genpd
    providers
  pmdomain: core: Leave powered-on genpds on until ->sync_state()

 drivers/pmdomain/core.c   | 273 +++++++++++++++++++++++++++++++-------
 include/linux/device.h    |  12 ++
 include/linux/pm_domain.h |  11 ++
 3 files changed, 249 insertions(+), 47 deletions(-)

-- 
2.43.0

[PATCH 01/11] pmdomain: core: Convert genpd_power_off() to void

[Ulf Hansson]

At some point it made sense to have genpd_power_off() to return an error
code. That hasn't been the case for quite some time, so let's convert it
into a static void function and simplify some of the corresponding code.

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

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 3523d0331cec..574a0de1696a 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -908,13 +908,12 @@ static void genpd_queue_power_off_work(struct generic_pm_domain *genpd)
  * If all of the @genpd's devices have been suspended and all of its subdomains
  * have been powered down, remove power from @genpd.
  */
-static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
-			   unsigned int depth)
+static void genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
+			    unsigned int depth)
 {
 	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:
@@ -922,7 +921,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 	 * (2) System suspend is in progress.
 	 */
 	if (!genpd_status_on(genpd) || genpd->prepared_count > 0)
-		return 0;
+		return;
 
 	/*
 	 * Abort power off for the PM domain in the following situations:
@@ -932,7 +931,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 	if (genpd_is_always_on(genpd) ||
 			genpd_is_rpm_always_on(genpd) ||
 			atomic_read(&genpd->sd_count) > 0)
-		return -EBUSY;
+		return;
 
 	/*
 	 * The children must be in their deepest (powered-off) states to allow
@@ -943,7 +942,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 	list_for_each_entry(link, &genpd->parent_links, parent_node) {
 		struct generic_pm_domain *child = link->child;
 		if (child->state_idx < child->state_count - 1)
-			return -EBUSY;
+			return;
 	}
 
 	list_for_each_entry(pdd, &genpd->dev_list, list_node) {
@@ -957,15 +956,15 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 
 		/* The device may need its PM domain to stay powered on. */
 		if (to_gpd_data(pdd)->rpm_always_on)
-			return -EBUSY;
+			return;
 	}
 
 	if (not_suspended > 1 || (not_suspended == 1 && !one_dev_on))
-		return -EBUSY;
+		return;
 
 	if (genpd->gov && genpd->gov->power_down_ok) {
 		if (!genpd->gov->power_down_ok(&genpd->domain))
-			return -EAGAIN;
+			return;
 	}
 
 	/* Default to shallowest state. */
@@ -974,12 +973,11 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 
 	/* Don't power off, if a child domain is waiting to power on. */
 	if (atomic_read(&genpd->sd_count) > 0)
-		return -EBUSY;
+		return;
 
-	ret = _genpd_power_off(genpd, true);
-	if (ret) {
+	if (_genpd_power_off(genpd, true)) {
 		genpd->states[genpd->state_idx].rejected++;
-		return ret;
+		return;
 	}
 
 	genpd->status = GENPD_STATE_OFF;
@@ -992,8 +990,6 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 		genpd_power_off(link->parent, false, depth + 1);
 		genpd_unlock(link->parent);
 	}
-
-	return 0;
 }
 
 /**
-- 
2.43.0

Re: [PATCH 01/11] pmdomain: core: Convert genpd_power_off() to void

[Abel Vesa]

On 25-04-17 16:24:59, Ulf Hansson wrote:
> At some point it made sense to have genpd_power_off() to return an error
> code. That hasn't been the case for quite some time, so let's convert it
> into a static void function and simplify some of the corresponding code.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

> ---
>  drivers/pmdomain/core.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 3523d0331cec..574a0de1696a 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -908,13 +908,12 @@ static void genpd_queue_power_off_work(struct generic_pm_domain *genpd)
>   * If all of the @genpd's devices have been suspended and all of its subdomains
>   * have been powered down, remove power from @genpd.
>   */
> -static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
> -			   unsigned int depth)
> +static void genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
> +			    unsigned int depth)
>  {
>  	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:
> @@ -922,7 +921,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>  	 * (2) System suspend is in progress.
>  	 */
>  	if (!genpd_status_on(genpd) || genpd->prepared_count > 0)
> -		return 0;
> +		return;
>  
>  	/*
>  	 * Abort power off for the PM domain in the following situations:
> @@ -932,7 +931,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>  	if (genpd_is_always_on(genpd) ||
>  			genpd_is_rpm_always_on(genpd) ||
>  			atomic_read(&genpd->sd_count) > 0)
> -		return -EBUSY;
> +		return;
>  
>  	/*
>  	 * The children must be in their deepest (powered-off) states to allow
> @@ -943,7 +942,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>  	list_for_each_entry(link, &genpd->parent_links, parent_node) {
>  		struct generic_pm_domain *child = link->child;
>  		if (child->state_idx < child->state_count - 1)
> -			return -EBUSY;
> +			return;
>  	}
>  
>  	list_for_each_entry(pdd, &genpd->dev_list, list_node) {
> @@ -957,15 +956,15 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>  
>  		/* The device may need its PM domain to stay powered on. */
>  		if (to_gpd_data(pdd)->rpm_always_on)
> -			return -EBUSY;
> +			return;
>  	}
>  
>  	if (not_suspended > 1 || (not_suspended == 1 && !one_dev_on))
> -		return -EBUSY;
> +		return;
>  
>  	if (genpd->gov && genpd->gov->power_down_ok) {
>  		if (!genpd->gov->power_down_ok(&genpd->domain))
> -			return -EAGAIN;
> +			return;
>  	}
>  
>  	/* Default to shallowest state. */
> @@ -974,12 +973,11 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>  
>  	/* Don't power off, if a child domain is waiting to power on. */
>  	if (atomic_read(&genpd->sd_count) > 0)
> -		return -EBUSY;
> +		return;
>  
> -	ret = _genpd_power_off(genpd, true);
> -	if (ret) {
> +	if (_genpd_power_off(genpd, true)) {
>  		genpd->states[genpd->state_idx].rejected++;
> -		return ret;
> +		return;
>  	}
>  
>  	genpd->status = GENPD_STATE_OFF;
> @@ -992,8 +990,6 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>  		genpd_power_off(link->parent, false, depth + 1);
>  		genpd_unlock(link->parent);
>  	}
> -
> -	return 0;
>  }
>  
>  /**
> -- 
> 2.43.0
> 

[PATCH 02/11] pmdomain: core: Simplify return statement in genpd_power_off()

[Ulf Hansson]

Rather than using two if-clauses immediately after each to check for
similar reasons to prevent the power-off, let's combine them into one
if-clause to simplify the code.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/pmdomain/core.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 574a0de1696a..34a85bf347ad 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -917,20 +917,14 @@ static void genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 
 	/*
 	 * Do not try to power off the domain in the following situations:
-	 * (1) The domain is already in the "power off" state.
-	 * (2) System suspend is in progress.
+	 * The domain is already in the "power off" state.
+	 * System suspend is in progress.
+	 * The domain is configured as always on.
+	 * The domain has a subdomain being powered on.
 	 */
-	if (!genpd_status_on(genpd) || genpd->prepared_count > 0)
-		return;
-
-	/*
-	 * Abort power off for the PM domain in the following situations:
-	 * (1) The domain is configured as always on.
-	 * (2) When the domain has a subdomain being powered on.
-	 */
-	if (genpd_is_always_on(genpd) ||
-			genpd_is_rpm_always_on(genpd) ||
-			atomic_read(&genpd->sd_count) > 0)
+	if (!genpd_status_on(genpd) || genpd->prepared_count > 0 ||
+	    genpd_is_always_on(genpd) || genpd_is_rpm_always_on(genpd) ||
+	    atomic_read(&genpd->sd_count) > 0)
 		return;
 
 	/*
-- 
2.43.0

Re: [PATCH 02/11] pmdomain: core: Simplify return statement in genpd_power_off()

[Abel Vesa]

On 25-04-17 16:25:00, Ulf Hansson wrote:
> Rather than using two if-clauses immediately after each to check for
> similar reasons to prevent the power-off, let's combine them into one
> if-clause to simplify the code.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

> ---
>  drivers/pmdomain/core.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 574a0de1696a..34a85bf347ad 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -917,20 +917,14 @@ static void genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>  
>  	/*
>  	 * Do not try to power off the domain in the following situations:
> -	 * (1) The domain is already in the "power off" state.
> -	 * (2) System suspend is in progress.
> +	 * The domain is already in the "power off" state.
> +	 * System suspend is in progress.
> +	 * The domain is configured as always on.
> +	 * The domain has a subdomain being powered on.
>  	 */
> -	if (!genpd_status_on(genpd) || genpd->prepared_count > 0)
> -		return;
> -
> -	/*
> -	 * Abort power off for the PM domain in the following situations:
> -	 * (1) The domain is configured as always on.
> -	 * (2) When the domain has a subdomain being powered on.
> -	 */
> -	if (genpd_is_always_on(genpd) ||
> -			genpd_is_rpm_always_on(genpd) ||
> -			atomic_read(&genpd->sd_count) > 0)
> +	if (!genpd_status_on(genpd) || genpd->prepared_count > 0 ||
> +	    genpd_is_always_on(genpd) || genpd_is_rpm_always_on(genpd) ||
> +	    atomic_read(&genpd->sd_count) > 0)
>  		return;
>  
>  	/*
> -- 
> 2.43.0
> 

[PATCH 03/11] pmdomain: core: Use genpd->opp_table to simplify error/remove path

[Ulf Hansson]

While we add an OF-provider we may, based upon a specific condition, also
assign genpd->opp_table. Rather using the same specific condition in the
error/remove path, let's check genpd->opp_table instead as it makes the
code easier.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/pmdomain/core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 34a85bf347ad..035b65563947 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2343,6 +2343,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 	genpd->provider = NULL;
 	genpd->device_id = -ENXIO;
 	genpd->has_provider = false;
+	genpd->opp_table = NULL;
 	genpd->accounting_time = ktime_get_mono_fast_ns();
 	genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
 	genpd->domain.ops.runtime_resume = genpd_runtime_resume;
@@ -2617,7 +2618,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
 
 	ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
 	if (ret) {
-		if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
+		if (genpd->opp_table) {
 			dev_pm_opp_put_opp_table(genpd->opp_table);
 			dev_pm_opp_of_remove_table(&genpd->dev);
 		}
@@ -2697,7 +2698,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 		genpd->provider = NULL;
 		genpd->has_provider = false;
 
-		if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
+		if (genpd->opp_table) {
 			dev_pm_opp_put_opp_table(genpd->opp_table);
 			dev_pm_opp_of_remove_table(&genpd->dev);
 		}
@@ -2729,11 +2730,10 @@ void of_genpd_del_provider(struct device_node *np)
 				if (gpd->provider == &np->fwnode) {
 					gpd->has_provider = false;
 
-					if (genpd_is_opp_table_fw(gpd) || !gpd->set_performance_state)
-						continue;
-
-					dev_pm_opp_put_opp_table(gpd->opp_table);
-					dev_pm_opp_of_remove_table(&gpd->dev);
+					if (gpd->opp_table) {
+						dev_pm_opp_put_opp_table(gpd->opp_table);
+						dev_pm_opp_of_remove_table(&gpd->dev);
+					}
 				}
 			}
 
-- 
2.43.0

Re: [PATCH 03/11] pmdomain: core: Use genpd->opp_table to simplify error/remove path

[Abel Vesa]

On 25-04-17 16:25:01, Ulf Hansson wrote:
> While we add an OF-provider we may, based upon a specific condition, also
> assign genpd->opp_table. Rather using the same specific condition in the
> error/remove path, let's check genpd->opp_table instead as it makes the
> code easier.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

> ---
>  drivers/pmdomain/core.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 34a85bf347ad..035b65563947 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -2343,6 +2343,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>  	genpd->provider = NULL;
>  	genpd->device_id = -ENXIO;
>  	genpd->has_provider = false;
> +	genpd->opp_table = NULL;
>  	genpd->accounting_time = ktime_get_mono_fast_ns();
>  	genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
>  	genpd->domain.ops.runtime_resume = genpd_runtime_resume;
> @@ -2617,7 +2618,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
>  
>  	ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
>  	if (ret) {
> -		if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
> +		if (genpd->opp_table) {
>  			dev_pm_opp_put_opp_table(genpd->opp_table);
>  			dev_pm_opp_of_remove_table(&genpd->dev);
>  		}
> @@ -2697,7 +2698,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>  		genpd->provider = NULL;
>  		genpd->has_provider = false;
>  
> -		if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
> +		if (genpd->opp_table) {
>  			dev_pm_opp_put_opp_table(genpd->opp_table);
>  			dev_pm_opp_of_remove_table(&genpd->dev);
>  		}
> @@ -2729,11 +2730,10 @@ void of_genpd_del_provider(struct device_node *np)
>  				if (gpd->provider == &np->fwnode) {
>  					gpd->has_provider = false;
>  
> -					if (genpd_is_opp_table_fw(gpd) || !gpd->set_performance_state)
> -						continue;
> -
> -					dev_pm_opp_put_opp_table(gpd->opp_table);
> -					dev_pm_opp_of_remove_table(&gpd->dev);
> +					if (gpd->opp_table) {
> +						dev_pm_opp_put_opp_table(gpd->opp_table);
> +						dev_pm_opp_of_remove_table(&gpd->dev);
> +					}
>  				}
>  			}
>  
> -- 
> 2.43.0
> 

[PATCH 04/11] pmdomain: core: Add a bus and a driver for genpd providers

[Ulf Hansson]

When we create a genpd via pm_genpd_init() we are initializing a
corresponding struct device for it, but we don't add the device to any
bus_type. It has not really been needed as the device is used as cookie to
help us manage OPP tables.

However, to prepare to make better use of the device let's add a new genpd
provider bus_type and a corresponding genpd provider driver. Subsequent
changes will make use of this.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/pmdomain/core.c | 89 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 88 insertions(+), 1 deletion(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 035b65563947..da51a61a974c 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -27,6 +27,11 @@
 /* Provides a unique ID for each genpd device */
 static DEFINE_IDA(genpd_ida);
 
+/* The parent for genpd_provider devices. */
+static struct device genpd_provider_bus = {
+	.init_name = "genpd_provider",
+};
+
 #define GENPD_RETRY_MAX_MS	250		/* Approximate */
 
 #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)		\
@@ -44,6 +49,14 @@ static DEFINE_IDA(genpd_ida);
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 
+#define to_genpd_provider_drv(d) container_of(d, struct genpd_provider_drv, drv)
+
+struct genpd_provider_drv {
+	struct device_driver drv;
+	int (*probe)(struct device *dev);
+	void (*remove)(struct device *dev);
+};
+
 struct genpd_lock_ops {
 	void (*lock)(struct generic_pm_domain *genpd);
 	void (*lock_nested)(struct generic_pm_domain *genpd, int depth);
@@ -2225,6 +2238,26 @@ static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
 	return 0;
 }
 
+static int genpd_provider_bus_probe(struct device *dev)
+{
+	struct genpd_provider_drv *drv = to_genpd_provider_drv(dev->driver);
+
+	return drv->probe(dev);
+}
+
+static void genpd_provider_bus_remove(struct device *dev)
+{
+	struct genpd_provider_drv *drv = to_genpd_provider_drv(dev->driver);
+
+	drv->remove(dev);
+}
+
+static const struct bus_type genpd_provider_bus_type = {
+	.name		= "genpd_provider",
+	.probe		= genpd_provider_bus_probe,
+	.remove		= genpd_provider_bus_remove,
+};
+
 static void genpd_provider_release(struct device *dev)
 {
 	/* nothing to be done here */
@@ -2262,6 +2295,8 @@ static int genpd_alloc_data(struct generic_pm_domain *genpd)
 	genpd->gd = gd;
 	device_initialize(&genpd->dev);
 	genpd->dev.release = genpd_provider_release;
+	genpd->dev.bus = &genpd_provider_bus_type;
+	genpd->dev.parent = &genpd_provider_bus;
 
 	if (!genpd_is_dev_name_fw(genpd)) {
 		dev_set_name(&genpd->dev, "%s", genpd->name);
@@ -3355,9 +3390,61 @@ int of_genpd_parse_idle_states(struct device_node *dn,
 }
 EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
 
+static int genpd_provider_probe(struct device *dev)
+{
+	return 0;
+}
+
+static void genpd_provider_remove(struct device *dev)
+{
+}
+
+static void genpd_provider_sync_state(struct device *dev)
+{
+}
+
+static struct genpd_provider_drv genpd_provider_drv = {
+	.drv = {
+		.name = "genpd_provider",
+		.bus = &genpd_provider_bus_type,
+		.sync_state = genpd_provider_sync_state,
+		.suppress_bind_attrs = true,
+	},
+	.probe = genpd_provider_probe,
+	.remove = genpd_provider_remove,
+};
+
 static int __init genpd_bus_init(void)
 {
-	return bus_register(&genpd_bus_type);
+	int ret;
+
+	ret = device_register(&genpd_provider_bus);
+	if (ret) {
+		put_device(&genpd_provider_bus);
+		return ret;
+	}
+
+	ret = bus_register(&genpd_provider_bus_type);
+	if (ret)
+		goto err_dev;
+
+	ret = bus_register(&genpd_bus_type);
+	if (ret)
+		goto err_prov_bus;
+
+	ret = driver_register(&genpd_provider_drv.drv);
+	if (ret)
+		goto err_bus;
+
+	return 0;
+
+err_bus:
+	bus_unregister(&genpd_bus_type);
+err_prov_bus:
+	bus_unregister(&genpd_provider_bus_type);
+err_dev:
+	device_unregister(&genpd_provider_bus);
+	return ret;
 }
 core_initcall(genpd_bus_init);
 
-- 
2.43.0

Re: [PATCH 04/11] pmdomain: core: Add a bus and a driver for genpd providers

[Abel Vesa]

On 25-04-17 16:25:02, Ulf Hansson wrote:
> When we create a genpd via pm_genpd_init() we are initializing a
> corresponding struct device for it, but we don't add the device to any
> bus_type. It has not really been needed as the device is used as cookie to
> help us manage OPP tables.
> 
> However, to prepare to make better use of the device let's add a new genpd
> provider bus_type and a corresponding genpd provider driver. Subsequent
> changes will make use of this.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/pmdomain/core.c | 89 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 035b65563947..da51a61a974c 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -27,6 +27,11 @@
>  /* Provides a unique ID for each genpd device */
>  static DEFINE_IDA(genpd_ida);
>  
> +/* The parent for genpd_provider devices. */
> +static struct device genpd_provider_bus = {
> +	.init_name = "genpd_provider",
> +};
> +
>  #define GENPD_RETRY_MAX_MS	250		/* Approximate */
>  
>  #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)		\
> @@ -44,6 +49,14 @@ static DEFINE_IDA(genpd_ida);
>  static LIST_HEAD(gpd_list);
>  static DEFINE_MUTEX(gpd_list_lock);
>  
> +#define to_genpd_provider_drv(d) container_of(d, struct genpd_provider_drv, drv)
> +
> +struct genpd_provider_drv {

I'd replace "provider" substring and expand drv to driver everywhere.

I think that's more in line with all other subsystems.

> +	struct device_driver drv;
> +	int (*probe)(struct device *dev);
> +	void (*remove)(struct device *dev);
> +};
> +
>  struct genpd_lock_ops {
>  	void (*lock)(struct generic_pm_domain *genpd);
>  	void (*lock_nested)(struct generic_pm_domain *genpd, int depth);
> @@ -2225,6 +2238,26 @@ static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
>  	return 0;
>  }
>  
> +static int genpd_provider_bus_probe(struct device *dev)

... and then here drop the "provider" as well.

Other than that, LGTM:

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

Re: [PATCH 04/11] pmdomain: core: Add a bus and a driver for genpd providers

[Ulf Hansson]

On Tue, 22 Apr 2025 at 16:03, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 25-04-17 16:25:02, Ulf Hansson wrote:
> > When we create a genpd via pm_genpd_init() we are initializing a
> > corresponding struct device for it, but we don't add the device to any
> > bus_type. It has not really been needed as the device is used as cookie to
> > help us manage OPP tables.
> >
> > However, to prepare to make better use of the device let's add a new genpd
> > provider bus_type and a corresponding genpd provider driver. Subsequent
> > changes will make use of this.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/pmdomain/core.c | 89 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 88 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > index 035b65563947..da51a61a974c 100644
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -27,6 +27,11 @@
> >  /* Provides a unique ID for each genpd device */
> >  static DEFINE_IDA(genpd_ida);
> >
> > +/* The parent for genpd_provider devices. */
> > +static struct device genpd_provider_bus = {
> > +     .init_name = "genpd_provider",
> > +};
> > +
> >  #define GENPD_RETRY_MAX_MS   250             /* Approximate */
> >
> >  #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)               \
> > @@ -44,6 +49,14 @@ static DEFINE_IDA(genpd_ida);
> >  static LIST_HEAD(gpd_list);
> >  static DEFINE_MUTEX(gpd_list_lock);
> >
> > +#define to_genpd_provider_drv(d) container_of(d, struct genpd_provider_drv, drv)
> > +
> > +struct genpd_provider_drv {
>
> I'd replace "provider" substring and expand drv to driver everywhere.
>
> I think that's more in line with all other subsystems.

I understand your point, but it's not that straight-forward to find a
proper name this time.

We already have another bus_type for genpd consumer devices (virtual
devices created when attaching a device to one of its multiple PM
domains). That bus is already named "genpd".

>
> > +     struct device_driver drv;
> > +     int (*probe)(struct device *dev);
> > +     void (*remove)(struct device *dev);
> > +};
> > +
> >  struct genpd_lock_ops {
> >       void (*lock)(struct generic_pm_domain *genpd);
> >       void (*lock_nested)(struct generic_pm_domain *genpd, int depth);
> > @@ -2225,6 +2238,26 @@ static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
> >       return 0;
> >  }
> >
> > +static int genpd_provider_bus_probe(struct device *dev)
>
> ... and then here drop the "provider" as well.

For the reason I pointed out above, I decided to use "provider" in the
bus/driver's functions names to have a clear difference from the
"consumer" genpd bus.

I am worried that if we don't use "provider" we will mix up things
with the existing genpd bus. Maybe there is a better option?

>
> Other than that, LGTM:
>
> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

Thanks for reviewing!

Kind regards
Uffe

[PATCH 05/11] pmdomain: core: Use device_set_node() to assign the fwnode too

[Ulf Hansson]

Rather than just assigning the dev->of_node for the genpd's device, let's
use device_set_node() to make sure the fwnode gets assigned too. This is
needed to allow fw_devlink to work correctly, for example.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/pmdomain/core.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index da51a61a974c..3911d3e96626 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2627,6 +2627,7 @@ static bool genpd_present(const struct generic_pm_domain *genpd)
 int of_genpd_add_provider_simple(struct device_node *np,
 				 struct generic_pm_domain *genpd)
 {
+	struct fwnode_handle *fwnode;
 	int ret;
 
 	if (!np || !genpd)
@@ -2635,7 +2636,9 @@ int of_genpd_add_provider_simple(struct device_node *np,
 	if (!genpd_present(genpd))
 		return -EINVAL;
 
-	genpd->dev.of_node = np;
+	fwnode = &np->fwnode;
+
+	device_set_node(&genpd->dev, fwnode);
 
 	/* Parse genpd OPP table */
 	if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
@@ -2661,7 +2664,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
 		return ret;
 	}
 
-	genpd->provider = &np->fwnode;
+	genpd->provider = fwnode;
 	genpd->has_provider = true;
 
 	return 0;
@@ -2677,6 +2680,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 				  struct genpd_onecell_data *data)
 {
 	struct generic_pm_domain *genpd;
+	struct fwnode_handle *fwnode;
 	unsigned int i;
 	int ret = -EINVAL;
 
@@ -2686,6 +2690,8 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 	if (!data->xlate)
 		data->xlate = genpd_xlate_onecell;
 
+	fwnode = &np->fwnode;
+
 	for (i = 0; i < data->num_domains; i++) {
 		genpd = data->domains[i];
 
@@ -2694,7 +2700,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 		if (!genpd_present(genpd))
 			goto error;
 
-		genpd->dev.of_node = np;
+		device_set_node(&genpd->dev, fwnode);
 
 		/* Parse genpd OPP table */
 		if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
@@ -2713,7 +2719,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 			WARN_ON(IS_ERR(genpd->opp_table));
 		}
 
-		genpd->provider = &np->fwnode;
+		genpd->provider = fwnode;
 		genpd->has_provider = true;
 	}
 
-- 
2.43.0

Re: [PATCH 05/11] pmdomain: core: Use device_set_node() to assign the fwnode too

[Saravana Kannan]

On Thu, Apr 17, 2025 at 7:25 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Rather than just assigning the dev->of_node for the genpd's device, let's
> use device_set_node() to make sure the fwnode gets assigned too. This is
> needed to allow fw_devlink to work correctly, for example.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/pmdomain/core.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index da51a61a974c..3911d3e96626 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -2627,6 +2627,7 @@ static bool genpd_present(const struct generic_pm_domain *genpd)
>  int of_genpd_add_provider_simple(struct device_node *np,
>                                  struct generic_pm_domain *genpd)
>  {
> +       struct fwnode_handle *fwnode;
>         int ret;
>
>         if (!np || !genpd)
> @@ -2635,7 +2636,9 @@ int of_genpd_add_provider_simple(struct device_node *np,
>         if (!genpd_present(genpd))
>                 return -EINVAL;
>
> -       genpd->dev.of_node = np;
> +       fwnode = &np->fwnode;

Use of_fwnode_handle() please.

> +
> +       device_set_node(&genpd->dev, fwnode);
>
>         /* Parse genpd OPP table */
>         if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
> @@ -2661,7 +2664,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
>                 return ret;
>         }
>
> -       genpd->provider = &np->fwnode;
> +       genpd->provider = fwnode;
>         genpd->has_provider = true;
>
>         return 0;
> @@ -2677,6 +2680,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>                                   struct genpd_onecell_data *data)
>  {
>         struct generic_pm_domain *genpd;
> +       struct fwnode_handle *fwnode;
>         unsigned int i;
>         int ret = -EINVAL;
>
> @@ -2686,6 +2690,8 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>         if (!data->xlate)
>                 data->xlate = genpd_xlate_onecell;
>
> +       fwnode = &np->fwnode;
> +

Use of_fwnode_handle() please.

-Saravana

>         for (i = 0; i < data->num_domains; i++) {
>                 genpd = data->domains[i];
>
> @@ -2694,7 +2700,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>                 if (!genpd_present(genpd))
>                         goto error;
>
> -               genpd->dev.of_node = np;
> +               device_set_node(&genpd->dev, fwnode);
>
>                 /* Parse genpd OPP table */
>                 if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
> @@ -2713,7 +2719,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>                         WARN_ON(IS_ERR(genpd->opp_table));
>                 }
>
> -               genpd->provider = &np->fwnode;
> +               genpd->provider = fwnode;
>                 genpd->has_provider = true;
>         }
>
> --
> 2.43.0
>

Re: [PATCH 05/11] pmdomain: core: Use device_set_node() to assign the fwnode too

[Ulf Hansson]

On Thu, 17 Apr 2025 at 22:56, Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Apr 17, 2025 at 7:25 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > Rather than just assigning the dev->of_node for the genpd's device, let's
> > use device_set_node() to make sure the fwnode gets assigned too. This is
> > needed to allow fw_devlink to work correctly, for example.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/pmdomain/core.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > index da51a61a974c..3911d3e96626 100644
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -2627,6 +2627,7 @@ static bool genpd_present(const struct generic_pm_domain *genpd)
> >  int of_genpd_add_provider_simple(struct device_node *np,
> >                                  struct generic_pm_domain *genpd)
> >  {
> > +       struct fwnode_handle *fwnode;
> >         int ret;
> >
> >         if (!np || !genpd)
> > @@ -2635,7 +2636,9 @@ int of_genpd_add_provider_simple(struct device_node *np,
> >         if (!genpd_present(genpd))
> >                 return -EINVAL;
> >
> > -       genpd->dev.of_node = np;
> > +       fwnode = &np->fwnode;
>
> Use of_fwnode_handle() please.

Sure! Thanks for reviewing!

[...]

Kind regards
Uffe

Re: [PATCH 05/11] pmdomain: core: Use device_set_node() to assign the fwnode too

[Abel Vesa]

On 25-04-17 16:25:03, Ulf Hansson wrote:
> Rather than just assigning the dev->of_node for the genpd's device, let's
> use device_set_node() to make sure the fwnode gets assigned too. This is
> needed to allow fw_devlink to work correctly, for example.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

With Saravana's comments addressed, this LGTM:

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

[PATCH 06/11] pmdomain: core: Add the genpd->dev to the genpd provider bus

[Ulf Hansson]

To take the next step for a more common handling of the genpd providers,
let's add the genpd->dev to the genpd provider bus when registering a genpd
OF provider.

Beyond this, the corresponding genpd provider driver's ->probe(),
->remove() and ->sync_state() callbacks starts to be invoked. However,
let's leave those callbacks as empty functions for now. Instead, subsequent
changes will implement them.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/pmdomain/core.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 3911d3e96626..5aba66ac78f1 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2640,11 +2640,17 @@ int of_genpd_add_provider_simple(struct device_node *np,
 
 	device_set_node(&genpd->dev, fwnode);
 
+	ret = device_add(&genpd->dev);
+	if (ret)
+		return ret;
+
 	/* Parse genpd OPP table */
 	if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
 		ret = dev_pm_opp_of_add_table(&genpd->dev);
-		if (ret)
-			return dev_err_probe(&genpd->dev, ret, "Failed to add OPP table\n");
+		if (ret) {
+			dev_err_probe(&genpd->dev, ret, "Failed to add OPP table\n");
+			goto err_del;
+		}
 
 		/*
 		 * Save table for faster processing while setting performance
@@ -2655,19 +2661,22 @@ int of_genpd_add_provider_simple(struct device_node *np,
 	}
 
 	ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
-	if (ret) {
-		if (genpd->opp_table) {
-			dev_pm_opp_put_opp_table(genpd->opp_table);
-			dev_pm_opp_of_remove_table(&genpd->dev);
-		}
-
-		return ret;
-	}
+	if (ret)
+		goto err_opp;
 
 	genpd->provider = fwnode;
 	genpd->has_provider = true;
 
 	return 0;
+
+err_opp:
+	if (genpd->opp_table) {
+		dev_pm_opp_put_opp_table(genpd->opp_table);
+		dev_pm_opp_of_remove_table(&genpd->dev);
+	}
+err_del:
+	device_del(&genpd->dev);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
 
@@ -2702,12 +2711,17 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 
 		device_set_node(&genpd->dev, fwnode);
 
+		ret = device_add(&genpd->dev);
+		if (ret)
+			goto error;
+
 		/* Parse genpd OPP table */
 		if (!genpd_is_opp_table_fw(genpd) && genpd->set_performance_state) {
 			ret = dev_pm_opp_of_add_table_indexed(&genpd->dev, i);
 			if (ret) {
 				dev_err_probe(&genpd->dev, ret,
 					      "Failed to add OPP table for index %d\n", i);
+				device_del(&genpd->dev);
 				goto error;
 			}
 
@@ -2743,6 +2757,8 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 			dev_pm_opp_put_opp_table(genpd->opp_table);
 			dev_pm_opp_of_remove_table(&genpd->dev);
 		}
+
+		device_del(&genpd->dev);
 	}
 
 	return ret;
@@ -2775,6 +2791,8 @@ void of_genpd_del_provider(struct device_node *np)
 						dev_pm_opp_put_opp_table(gpd->opp_table);
 						dev_pm_opp_of_remove_table(&gpd->dev);
 					}
+
+					device_del(&gpd->dev);
 				}
 			}
 
-- 
2.43.0

Re: [PATCH 06/11] pmdomain: core: Add the genpd->dev to the genpd provider bus

[Abel Vesa]

On 25-04-17 16:25:04, Ulf Hansson wrote:
> To take the next step for a more common handling of the genpd providers,
> let's add the genpd->dev to the genpd provider bus when registering a genpd
> OF provider.
> 
> Beyond this, the corresponding genpd provider driver's ->probe(),
> ->remove() and ->sync_state() callbacks starts to be invoked. However,
> let's leave those callbacks as empty functions for now. Instead, subsequent
> changes will implement them.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

[PATCH 07/11] pmdomain: core: Export a common ->sync_state() helper for genpd providers

[Ulf Hansson]

In some cases the typical platform driver that act as genpd provider, may
need its own ->sync_state() callback to manage various things. In this
regards, the provider most likely wants to allow its corresponding genpds
to be powered-off.

For this reason, let's introduce a new genpd helper function,
of_genpd_sync_state() that helps genpd provider drivers to achieve this.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/pmdomain/core.c   | 28 ++++++++++++++++++++++++++++
 include/linux/pm_domain.h |  3 +++
 2 files changed, 31 insertions(+)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 5aba66ac78f1..512f89e6d302 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2619,6 +2619,34 @@ static bool genpd_present(const struct generic_pm_domain *genpd)
 	return ret;
 }
 
+/**
+ * of_genpd_sync_state() - A common sync_state function for genpd providers
+ * @dev: The device the genpd provider is associated with.
+ *
+ * The @dev that corresponds to a genpd provider may provide one or multiple
+ * genpds. This function makes use of the device node for @dev to find the
+ * genpds that belongs to the provider. For each genpd we try a power-off.
+ */
+void of_genpd_sync_state(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct generic_pm_domain *genpd;
+
+	if (!np)
+		return;
+
+	mutex_lock(&gpd_list_lock);
+	list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
+		if (genpd->provider == &np->fwnode) {
+			genpd_lock(genpd);
+			genpd_power_off(genpd, false, 0);
+			genpd_unlock(genpd);
+		}
+	}
+	mutex_unlock(&gpd_list_lock);
+}
+EXPORT_SYMBOL_GPL(of_genpd_sync_state);
+
 /**
  * of_genpd_add_provider_simple() - Register a simple PM domain provider
  * @np: Device node pointer associated with the PM domain provider.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 0b18160901a2..e9a1f8975c4f 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -418,6 +418,7 @@ struct genpd_onecell_data {
 };
 
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
+void of_genpd_sync_state(struct device *dev);
 int of_genpd_add_provider_simple(struct device_node *np,
 				 struct generic_pm_domain *genpd);
 int of_genpd_add_provider_onecell(struct device_node *np,
@@ -438,6 +439,8 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
 struct device *genpd_dev_pm_attach_by_name(struct device *dev,
 					   const char *name);
 #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
+static inline void of_genpd_sync_state(struct device *dev) {}
+
 static inline int of_genpd_add_provider_simple(struct device_node *np,
 					struct generic_pm_domain *genpd)
 {
-- 
2.43.0

Re: [PATCH 07/11] pmdomain: core: Export a common ->sync_state() helper for genpd providers

[Abel Vesa]

On 25-04-17 16:25:05, Ulf Hansson wrote:
> In some cases the typical platform driver that act as genpd provider, may
> need its own ->sync_state() callback to manage various things. In this
> regards, the provider most likely wants to allow its corresponding genpds
> to be powered-off.
> 
> For this reason, let's introduce a new genpd helper function,
> of_genpd_sync_state() that helps genpd provider drivers to achieve this.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

Re: [PATCH 07/11] pmdomain: core: Export a common ->sync_state() helper for genpd providers

[Dan Carpenter]

On Thu, Apr 17, 2025 at 04:25:05PM +0200, Ulf Hansson wrote:
> +void of_genpd_sync_state(struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct generic_pm_domain *genpd;
> +
> +	if (!np)
> +		return;
> +
> +	mutex_lock(&gpd_list_lock);
> +	list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> +		if (genpd->provider == &np->fwnode) {

Presumably this would be "== of_fwnode_handle(np)) {" as well...

regards,
dan carpenter

> +			genpd_lock(genpd);
> +			genpd_power_off(genpd, false, 0);
> +			genpd_unlock(genpd);
> +		}
> +	}
> +	mutex_unlock(&gpd_list_lock);
> +}

Re: [PATCH 07/11] pmdomain: core: Export a common ->sync_state() helper for genpd providers

[Ulf Hansson]

On Wed, 7 May 2025 at 18:23, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Thu, Apr 17, 2025 at 04:25:05PM +0200, Ulf Hansson wrote:
> > +void of_genpd_sync_state(struct device *dev)
> > +{
> > +     struct device_node *np = dev->of_node;
> > +     struct generic_pm_domain *genpd;
> > +
> > +     if (!np)
> > +             return;
> > +
> > +     mutex_lock(&gpd_list_lock);
> > +     list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> > +             if (genpd->provider == &np->fwnode) {
>
> Presumably this would be "== of_fwnode_handle(np)) {" as well...

Yes, in fact there are a whole bunch of them in genpd. I am making
preparatory patch to change all of them.

>
> regards,
> dan carpenter

Thanks for reviewing!

Kind regards
Uffe

[PATCH 08/11] pmdomain: core: Add internal ->sync_state() support for genpd providers

[Ulf Hansson]

If the genpd provider's fwnode doesn't have an associated struct device
with it, we can make use of the generic genpd->dev and it corresponding
driver internally in genpd to manage ->sync_state().

More precisely, while adding a genpd OF provider let's check if the fwnode
has a device and if not, make the preparation to handle ->sync_state()
internally through the genpd_provider_driver and the genpd_provider_bus.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/pmdomain/core.c   | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h |  7 +++++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 512f89e6d302..9c5a77bf59d2 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2374,6 +2374,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	atomic_set(&genpd->sd_count, 0);
 	genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
+	genpd->sync_state = GENPD_SYNC_STATE_OFF;
 	genpd->device_count = 0;
 	genpd->provider = NULL;
 	genpd->device_id = -ENXIO;
@@ -2656,6 +2657,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
 				 struct generic_pm_domain *genpd)
 {
 	struct fwnode_handle *fwnode;
+	struct device *dev;
 	int ret;
 
 	if (!np || !genpd)
@@ -2665,6 +2667,10 @@ int of_genpd_add_provider_simple(struct device_node *np,
 		return -EINVAL;
 
 	fwnode = &np->fwnode;
+	dev = fwnode->dev;
+
+	if (!dev)
+		genpd->sync_state = GENPD_SYNC_STATE_SIMPLE;
 
 	device_set_node(&genpd->dev, fwnode);
 
@@ -2718,8 +2724,10 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 {
 	struct generic_pm_domain *genpd;
 	struct fwnode_handle *fwnode;
+	struct device *dev;
 	unsigned int i;
 	int ret = -EINVAL;
+	bool sync_state = false;
 
 	if (!np || !data)
 		return -EINVAL;
@@ -2728,6 +2736,10 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 		data->xlate = genpd_xlate_onecell;
 
 	fwnode = &np->fwnode;
+	dev = fwnode->dev;
+
+	if (!dev)
+		sync_state = true;
 
 	for (i = 0; i < data->num_domains; i++) {
 		genpd = data->domains[i];
@@ -2737,6 +2749,11 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 		if (!genpd_present(genpd))
 			goto error;
 
+		if (sync_state) {
+			genpd->sync_state = GENPD_SYNC_STATE_ONECELL;
+			sync_state = false;
+		}
+
 		device_set_node(&genpd->dev, fwnode);
 
 		ret = device_add(&genpd->dev);
@@ -3453,6 +3470,25 @@ static void genpd_provider_remove(struct device *dev)
 
 static void genpd_provider_sync_state(struct device *dev)
 {
+	struct generic_pm_domain *genpd = container_of(dev, struct generic_pm_domain, dev);
+
+	switch (genpd->sync_state) {
+	case GENPD_SYNC_STATE_OFF:
+		break;
+
+	case GENPD_SYNC_STATE_ONECELL:
+		of_genpd_sync_state(dev);
+		break;
+
+	case GENPD_SYNC_STATE_SIMPLE:
+		genpd_lock(genpd);
+		genpd_power_off(genpd, false, 0);
+		genpd_unlock(genpd);
+		break;
+
+	default:
+		break;
+	}
 }
 
 static struct genpd_provider_drv genpd_provider_drv = {
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index e9a1f8975c4f..2185ee9e4f7c 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -127,6 +127,12 @@ enum genpd_notication {
 	GENPD_NOTIFY_ON,
 };
 
+enum genpd_sync_state {
+	GENPD_SYNC_STATE_OFF = 0,
+	GENPD_SYNC_STATE_SIMPLE,
+	GENPD_SYNC_STATE_ONECELL,
+};
+
 struct dev_power_governor {
 	bool (*power_down_ok)(struct dev_pm_domain *domain);
 	bool (*suspend_ok)(struct device *dev);
@@ -187,6 +193,7 @@ struct generic_pm_domain {
 	unsigned int performance_state;	/* Aggregated max performance state */
 	cpumask_var_t cpus;		/* A cpumask of the attached CPUs */
 	bool synced_poweroff;		/* A consumer needs a synced poweroff */
+	enum genpd_sync_state sync_state; /* How sync_state is managed. */
 	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 */
-- 
2.43.0

Re: [PATCH 08/11] pmdomain: core: Add internal ->sync_state() support for genpd providers

[Saravana Kannan]

On Thu, Apr 17, 2025 at 7:25 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> If the genpd provider's fwnode doesn't have an associated struct device
> with it, we can make use of the generic genpd->dev and it corresponding
> driver internally in genpd to manage ->sync_state().
>
> More precisely, while adding a genpd OF provider let's check if the fwnode
> has a device and if not, make the preparation to handle ->sync_state()
> internally through the genpd_provider_driver and the genpd_provider_bus.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/pmdomain/core.c   | 36 ++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h |  7 +++++++
>  2 files changed, 43 insertions(+)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 512f89e6d302..9c5a77bf59d2 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -2374,6 +2374,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>         INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
>         atomic_set(&genpd->sd_count, 0);
>         genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
> +       genpd->sync_state = GENPD_SYNC_STATE_OFF;
>         genpd->device_count = 0;
>         genpd->provider = NULL;
>         genpd->device_id = -ENXIO;
> @@ -2656,6 +2657,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
>                                  struct generic_pm_domain *genpd)
>  {
>         struct fwnode_handle *fwnode;
> +       struct device *dev;
>         int ret;
>
>         if (!np || !genpd)
> @@ -2665,6 +2667,10 @@ int of_genpd_add_provider_simple(struct device_node *np,
>                 return -EINVAL;
>
>         fwnode = &np->fwnode;
> +       dev = fwnode->dev;
> +
> +       if (!dev)
> +               genpd->sync_state = GENPD_SYNC_STATE_SIMPLE;

I don't want people directly poking into fwnode.

Use get_device_from_fwnode() that's in drivers/base/core.c? Might need
to move it to a header. Make sure to put_device() it back.

Or ideally, figure it out using some other means like looking for
#power-domain-cells number? (if that'll always give the right answer).
Point being, the framework should know which type it's registering
even if there was no fwnode/fw_devlink.

>
>         device_set_node(&genpd->dev, fwnode);
>
> @@ -2718,8 +2724,10 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>  {
>         struct generic_pm_domain *genpd;
>         struct fwnode_handle *fwnode;
> +       struct device *dev;
>         unsigned int i;
>         int ret = -EINVAL;
> +       bool sync_state = false;
>
>         if (!np || !data)
>                 return -EINVAL;
> @@ -2728,6 +2736,10 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>                 data->xlate = genpd_xlate_onecell;
>
>         fwnode = &np->fwnode;
> +       dev = fwnode->dev;

Can you do this some other way please? I really don't like this look up.

-Saravana

> +
> +       if (!dev)
> +               sync_state = true;
>
>         for (i = 0; i < data->num_domains; i++) {
>                 genpd = data->domains[i];
> @@ -2737,6 +2749,11 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>                 if (!genpd_present(genpd))
>                         goto error;
>
> +               if (sync_state) {
> +                       genpd->sync_state = GENPD_SYNC_STATE_ONECELL;
> +                       sync_state = false;
> +               }
> +
>                 device_set_node(&genpd->dev, fwnode);
>
>                 ret = device_add(&genpd->dev);
> @@ -3453,6 +3470,25 @@ static void genpd_provider_remove(struct device *dev)
>
>  static void genpd_provider_sync_state(struct device *dev)
>  {
> +       struct generic_pm_domain *genpd = container_of(dev, struct generic_pm_domain, dev);
> +
> +       switch (genpd->sync_state) {
> +       case GENPD_SYNC_STATE_OFF:
> +               break;
> +
> +       case GENPD_SYNC_STATE_ONECELL:
> +               of_genpd_sync_state(dev);
> +               break;
> +
> +       case GENPD_SYNC_STATE_SIMPLE:
> +               genpd_lock(genpd);
> +               genpd_power_off(genpd, false, 0);
> +               genpd_unlock(genpd);
> +               break;
> +
> +       default:
> +               break;
> +       }
>  }
>
>  static struct genpd_provider_drv genpd_provider_drv = {
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index e9a1f8975c4f..2185ee9e4f7c 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -127,6 +127,12 @@ enum genpd_notication {
>         GENPD_NOTIFY_ON,
>  };
>
> +enum genpd_sync_state {
> +       GENPD_SYNC_STATE_OFF = 0,
> +       GENPD_SYNC_STATE_SIMPLE,
> +       GENPD_SYNC_STATE_ONECELL,
> +};
> +
>  struct dev_power_governor {
>         bool (*power_down_ok)(struct dev_pm_domain *domain);
>         bool (*suspend_ok)(struct device *dev);
> @@ -187,6 +193,7 @@ struct generic_pm_domain {
>         unsigned int performance_state; /* Aggregated max performance state */
>         cpumask_var_t cpus;             /* A cpumask of the attached CPUs */
>         bool synced_poweroff;           /* A consumer needs a synced poweroff */
> +       enum genpd_sync_state sync_state; /* How sync_state is managed. */
>         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 */
> --
> 2.43.0
>

Re: [PATCH 08/11] pmdomain: core: Add internal ->sync_state() support for genpd providers

[Ulf Hansson]

On Fri, 18 Apr 2025 at 02:24, Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Apr 17, 2025 at 7:25 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > If the genpd provider's fwnode doesn't have an associated struct device
> > with it, we can make use of the generic genpd->dev and it corresponding
> > driver internally in genpd to manage ->sync_state().
> >
> > More precisely, while adding a genpd OF provider let's check if the fwnode
> > has a device and if not, make the preparation to handle ->sync_state()
> > internally through the genpd_provider_driver and the genpd_provider_bus.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/pmdomain/core.c   | 36 ++++++++++++++++++++++++++++++++++++
> >  include/linux/pm_domain.h |  7 +++++++
> >  2 files changed, 43 insertions(+)
> >
> > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > index 512f89e6d302..9c5a77bf59d2 100644
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -2374,6 +2374,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> >         INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
> >         atomic_set(&genpd->sd_count, 0);
> >         genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
> > +       genpd->sync_state = GENPD_SYNC_STATE_OFF;
> >         genpd->device_count = 0;
> >         genpd->provider = NULL;
> >         genpd->device_id = -ENXIO;
> > @@ -2656,6 +2657,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
> >                                  struct generic_pm_domain *genpd)
> >  {
> >         struct fwnode_handle *fwnode;
> > +       struct device *dev;
> >         int ret;
> >
> >         if (!np || !genpd)
> > @@ -2665,6 +2667,10 @@ int of_genpd_add_provider_simple(struct device_node *np,
> >                 return -EINVAL;
> >
> >         fwnode = &np->fwnode;
> > +       dev = fwnode->dev;
> > +
> > +       if (!dev)
> > +               genpd->sync_state = GENPD_SYNC_STATE_SIMPLE;
>
> I don't want people directly poking into fwnode.

There are already other subsystems doing it like this. Like
drivers/gpio/gpiolib.c for example. I didn't think it was a big deal,
my bad!

>
> Use get_device_from_fwnode() that's in drivers/base/core.c? Might need
> to move it to a header. Make sure to put_device() it back.

Sure, I can certainly do that!

>
> Or ideally, figure it out using some other means like looking for
> #power-domain-cells number? (if that'll always give the right answer).
> Point being, the framework should know which type it's registering
> even if there was no fwnode/fw_devlink.

When adding the genpd-of-provider we are only passing a device node.
Looking for "#power-domain-cells" can't tell us whether the node will
have a device or not.

>
> >
> >         device_set_node(&genpd->dev, fwnode);
> >
> > @@ -2718,8 +2724,10 @@ int of_genpd_add_provider_onecell(struct device_node *np,
> >  {
> >         struct generic_pm_domain *genpd;
> >         struct fwnode_handle *fwnode;
> > +       struct device *dev;
> >         unsigned int i;
> >         int ret = -EINVAL;
> > +       bool sync_state = false;
> >
> >         if (!np || !data)
> >                 return -EINVAL;
> > @@ -2728,6 +2736,10 @@ int of_genpd_add_provider_onecell(struct device_node *np,
> >                 data->xlate = genpd_xlate_onecell;
> >
> >         fwnode = &np->fwnode;
> > +       dev = fwnode->dev;
>
> Can you do this some other way please? I really don't like this look up.

Okay, I will use get_device_from_fwnode().

Thanks again for reviewing!

[...]

Kind regards
Uffe

[PATCH 09/11] driver core: Add dev_set_drv_sync_state()

[Ulf Hansson]

From: Saravana Kannan <saravanak@google.com>

This can be used by frameworks to set the sync_state() helper functions
for drivers that don't already have them set.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Note that, I have picked this patch from another series [1] that are about to be
re-freshed.

[1]
https://lore.kernel.org/all/YG6lhT7vuiCNvvDg@kroah.com/

---
 include/linux/device.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 79e49fe494b7..544523d48cd4 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -955,6 +955,18 @@ static inline bool dev_has_sync_state(struct device *dev)
 	return false;
 }
 
+static inline int dev_set_drv_sync_state(struct device *dev,
+					 void (*fn)(struct device *dev))
+{
+	if (!dev || !dev->driver)
+		return 0;
+	if (dev->driver->sync_state && dev->driver->sync_state != fn)
+		return -EBUSY;
+	if (!dev->driver->sync_state)
+		dev->driver->sync_state = fn;
+	return 0;
+}
+
 static inline void dev_set_removable(struct device *dev,
 				     enum device_removable removable)
 {
-- 
2.43.0

Re: [PATCH 09/11] driver core: Add dev_set_drv_sync_state()

[Abel Vesa]

On 25-04-17 16:25:07, Ulf Hansson wrote:
> From: Saravana Kannan <saravanak@google.com>
> 
> This can be used by frameworks to set the sync_state() helper functions
> for drivers that don't already have them set.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

[PATCH 10/11] pmdomain: core: Default to use of_genpd_sync_state() for genpd providers

[Ulf Hansson]

Unless the typical platform driver that act as genpd provider, has its own
->sync_state() callback implemented let's default to use
of_genpd_sync_state().

More precisely, while adding a genpd OF provider let's assign the
->sync_state() callback, in case the fwnode has a device and its driver/bus
doesn't have the ->sync_state() set already. In this way the typical
platform driver doesn't need to assign ->sync_state(), unless it has some
additional things to manage beyond genpds.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/pmdomain/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 9c5a77bf59d2..695d7d9e5582 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2671,6 +2671,8 @@ int of_genpd_add_provider_simple(struct device_node *np,
 
 	if (!dev)
 		genpd->sync_state = GENPD_SYNC_STATE_SIMPLE;
+	else if (!dev_has_sync_state(dev))
+		dev_set_drv_sync_state(dev, of_genpd_sync_state);
 
 	device_set_node(&genpd->dev, fwnode);
 
@@ -2740,6 +2742,8 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 
 	if (!dev)
 		sync_state = true;
+	else if (!dev_has_sync_state(dev))
+		dev_set_drv_sync_state(dev, of_genpd_sync_state);
 
 	for (i = 0; i < data->num_domains; i++) {
 		genpd = data->domains[i];
-- 
2.43.0

Re: [PATCH 10/11] pmdomain: core: Default to use of_genpd_sync_state() for genpd providers

[Saravana Kannan]

On Thu, Apr 17, 2025 at 7:25 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Unless the typical platform driver that act as genpd provider, has its own
> ->sync_state() callback implemented let's default to use
> of_genpd_sync_state().
>
> More precisely, while adding a genpd OF provider let's assign the
> ->sync_state() callback, in case the fwnode has a device and its driver/bus
> doesn't have the ->sync_state() set already. In this way the typical
> platform driver doesn't need to assign ->sync_state(), unless it has some
> additional things to manage beyond genpds.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/pmdomain/core.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 9c5a77bf59d2..695d7d9e5582 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -2671,6 +2671,8 @@ int of_genpd_add_provider_simple(struct device_node *np,
>
>         if (!dev)
>                 genpd->sync_state = GENPD_SYNC_STATE_SIMPLE;
> +       else if (!dev_has_sync_state(dev))
> +               dev_set_drv_sync_state(dev, of_genpd_sync_state);

Do you need the dev_has_sync_state() check? dev_set_drv_sync_state()
already check for everything under dev and drv. The only think it
doesn't check is "bus", but if the bus has a sync_state() it should
call the drv->sync_state() anyway.

-Saravana

>         device_set_node(&genpd->dev, fwnode);
>
> @@ -2740,6 +2742,8 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>
>         if (!dev)
>                 sync_state = true;
> +       else if (!dev_has_sync_state(dev))
> +               dev_set_drv_sync_state(dev, of_genpd_sync_state);
>
>         for (i = 0; i < data->num_domains; i++) {
>                 genpd = data->domains[i];
> --
> 2.43.0
>

[PATCH 11/11] pmdomain: core: Leave powered-on genpds on until ->sync_state()

[Ulf Hansson]

Powering-off a genpd that was on during boot, before all of its consumer
devices have been probed, is certainly prone to problems.

Let's fix these problems by preventing these genpds from being powered-off
until ->sync_state(). Note that, this only works for OF based platform as
->sync_state() are relying on fw_devlink.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/pmdomain/core.c   | 12 +++++++++++-
 include/linux/pm_domain.h |  1 +
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 695d7d9e5582..a8c56f7a7ba0 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -212,6 +212,12 @@ static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
 	return ret;
 }
 
+#ifdef CONFIG_PM_GENERIC_DOMAINS_OF
+static bool genpd_may_stay_on(bool on) { return on; }
+#else
+static bool genpd_may_stay_on(bool on) { return false; }
+#endif
+
 static int genpd_runtime_suspend(struct device *dev);
 
 /*
@@ -933,11 +939,12 @@ static void genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 	 * The domain is already in the "power off" state.
 	 * System suspend is in progress.
 	 * The domain is configured as always on.
+	 * The domain was on at boot and still need to stay on.
 	 * The domain has a subdomain being powered on.
 	 */
 	if (!genpd_status_on(genpd) || genpd->prepared_count > 0 ||
 	    genpd_is_always_on(genpd) || genpd_is_rpm_always_on(genpd) ||
-	    atomic_read(&genpd->sd_count) > 0)
+	    genpd->stay_on || atomic_read(&genpd->sd_count) > 0)
 		return;
 
 	/*
@@ -2374,6 +2381,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	atomic_set(&genpd->sd_count, 0);
 	genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
+	genpd->stay_on = genpd_may_stay_on(!is_off);
 	genpd->sync_state = GENPD_SYNC_STATE_OFF;
 	genpd->device_count = 0;
 	genpd->provider = NULL;
@@ -2640,6 +2648,7 @@ void of_genpd_sync_state(struct device *dev)
 	list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
 		if (genpd->provider == &np->fwnode) {
 			genpd_lock(genpd);
+			genpd->stay_on = false;
 			genpd_power_off(genpd, false, 0);
 			genpd_unlock(genpd);
 		}
@@ -3486,6 +3495,7 @@ static void genpd_provider_sync_state(struct device *dev)
 
 	case GENPD_SYNC_STATE_SIMPLE:
 		genpd_lock(genpd);
+		genpd->stay_on = false;
 		genpd_power_off(genpd, false, 0);
 		genpd_unlock(genpd);
 		break;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 2185ee9e4f7c..c5358cccacad 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -193,6 +193,7 @@ struct generic_pm_domain {
 	unsigned int performance_state;	/* Aggregated max performance state */
 	cpumask_var_t cpus;		/* A cpumask of the attached CPUs */
 	bool synced_poweroff;		/* A consumer needs a synced poweroff */
+	bool stay_on;			/* Stay powered-on during boot. */
 	enum genpd_sync_state sync_state; /* How sync_state is managed. */
 	int (*power_off)(struct generic_pm_domain *domain);
 	int (*power_on)(struct generic_pm_domain *domain);
-- 
2.43.0

Re: [PATCH 11/11] pmdomain: core: Leave powered-on genpds on until ->sync_state()

[Saravana Kannan]

On Thu, Apr 17, 2025 at 7:25 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Powering-off a genpd that was on during boot, before all of its consumer
> devices have been probed, is certainly prone to problems.
>
> Let's fix these problems by preventing these genpds from being powered-off
> until ->sync_state(). Note that, this only works for OF based platform as
> ->sync_state() are relying on fw_devlink.

For non-OF platforms, this will still wait until late_initcall(). So,
there's at least SOME protection. We could potentially even move that
to happen after deferred probe timeout expires.

-Saravana

>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/pmdomain/core.c   | 12 +++++++++++-
>  include/linux/pm_domain.h |  1 +
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 695d7d9e5582..a8c56f7a7ba0 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -212,6 +212,12 @@ static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
>         return ret;
>  }
>
> +#ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> +static bool genpd_may_stay_on(bool on) { return on; }
> +#else
> +static bool genpd_may_stay_on(bool on) { return false; }
> +#endif
> +
>  static int genpd_runtime_suspend(struct device *dev);
>
>  /*
> @@ -933,11 +939,12 @@ static void genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>          * The domain is already in the "power off" state.
>          * System suspend is in progress.
>          * The domain is configured as always on.
> +        * The domain was on at boot and still need to stay on.
>          * The domain has a subdomain being powered on.
>          */
>         if (!genpd_status_on(genpd) || genpd->prepared_count > 0 ||
>             genpd_is_always_on(genpd) || genpd_is_rpm_always_on(genpd) ||
> -           atomic_read(&genpd->sd_count) > 0)
> +           genpd->stay_on || atomic_read(&genpd->sd_count) > 0)
>                 return;
>
>         /*
> @@ -2374,6 +2381,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>         INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
>         atomic_set(&genpd->sd_count, 0);
>         genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
> +       genpd->stay_on = genpd_may_stay_on(!is_off);
>         genpd->sync_state = GENPD_SYNC_STATE_OFF;
>         genpd->device_count = 0;
>         genpd->provider = NULL;
> @@ -2640,6 +2648,7 @@ void of_genpd_sync_state(struct device *dev)
>         list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
>                 if (genpd->provider == &np->fwnode) {
>                         genpd_lock(genpd);
> +                       genpd->stay_on = false;
>                         genpd_power_off(genpd, false, 0);
>                         genpd_unlock(genpd);
>                 }
> @@ -3486,6 +3495,7 @@ static void genpd_provider_sync_state(struct device *dev)
>
>         case GENPD_SYNC_STATE_SIMPLE:
>                 genpd_lock(genpd);
> +               genpd->stay_on = false;
>                 genpd_power_off(genpd, false, 0);
>                 genpd_unlock(genpd);
>                 break;
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 2185ee9e4f7c..c5358cccacad 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -193,6 +193,7 @@ struct generic_pm_domain {
>         unsigned int performance_state; /* Aggregated max performance state */
>         cpumask_var_t cpus;             /* A cpumask of the attached CPUs */
>         bool synced_poweroff;           /* A consumer needs a synced poweroff */
> +       bool stay_on;                   /* Stay powered-on during boot. */
>         enum genpd_sync_state sync_state; /* How sync_state is managed. */
>         int (*power_off)(struct generic_pm_domain *domain);
>         int (*power_on)(struct generic_pm_domain *domain);
> --
> 2.43.0
>

Re: [PATCH 11/11] pmdomain: core: Leave powered-on genpds on until ->sync_state()

[Ulf Hansson]

On Fri, 18 Apr 2025 at 02:51, Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Apr 17, 2025 at 7:25 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > Powering-off a genpd that was on during boot, before all of its consumer
> > devices have been probed, is certainly prone to problems.
> >
> > Let's fix these problems by preventing these genpds from being powered-off
> > until ->sync_state(). Note that, this only works for OF based platform as
> > ->sync_state() are relying on fw_devlink.
>
> For non-OF platforms, this will still wait until late_initcall(). So,
> there's at least SOME protection. We could potentially even move that
> to happen after deferred probe timeout expires.

If I understand correctly, you are suggesting that we could update
genpd_power_off_unused() (late_initcall_sync) to clear genpd->stay_on
for the non-OF case? That should work I think.

>
> -Saravana

Kind regards
Uffe

>
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/pmdomain/core.c   | 12 +++++++++++-
> >  include/linux/pm_domain.h |  1 +
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > index 695d7d9e5582..a8c56f7a7ba0 100644
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -212,6 +212,12 @@ static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
> >         return ret;
> >  }
> >
> > +#ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> > +static bool genpd_may_stay_on(bool on) { return on; }
> > +#else
> > +static bool genpd_may_stay_on(bool on) { return false; }
> > +#endif
> > +
> >  static int genpd_runtime_suspend(struct device *dev);
> >
> >  /*
> > @@ -933,11 +939,12 @@ static void genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
> >          * The domain is already in the "power off" state.
> >          * System suspend is in progress.
> >          * The domain is configured as always on.
> > +        * The domain was on at boot and still need to stay on.
> >          * The domain has a subdomain being powered on.
> >          */
> >         if (!genpd_status_on(genpd) || genpd->prepared_count > 0 ||
> >             genpd_is_always_on(genpd) || genpd_is_rpm_always_on(genpd) ||
> > -           atomic_read(&genpd->sd_count) > 0)
> > +           genpd->stay_on || atomic_read(&genpd->sd_count) > 0)
> >                 return;
> >
> >         /*
> > @@ -2374,6 +2381,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> >         INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
> >         atomic_set(&genpd->sd_count, 0);
> >         genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
> > +       genpd->stay_on = genpd_may_stay_on(!is_off);
> >         genpd->sync_state = GENPD_SYNC_STATE_OFF;
> >         genpd->device_count = 0;
> >         genpd->provider = NULL;
> > @@ -2640,6 +2648,7 @@ void of_genpd_sync_state(struct device *dev)
> >         list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> >                 if (genpd->provider == &np->fwnode) {
> >                         genpd_lock(genpd);
> > +                       genpd->stay_on = false;
> >                         genpd_power_off(genpd, false, 0);
> >                         genpd_unlock(genpd);
> >                 }
> > @@ -3486,6 +3495,7 @@ static void genpd_provider_sync_state(struct device *dev)
> >
> >         case GENPD_SYNC_STATE_SIMPLE:
> >                 genpd_lock(genpd);
> > +               genpd->stay_on = false;
> >                 genpd_power_off(genpd, false, 0);
> >                 genpd_unlock(genpd);
> >                 break;
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index 2185ee9e4f7c..c5358cccacad 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -193,6 +193,7 @@ struct generic_pm_domain {
> >         unsigned int performance_state; /* Aggregated max performance state */
> >         cpumask_var_t cpus;             /* A cpumask of the attached CPUs */
> >         bool synced_poweroff;           /* A consumer needs a synced poweroff */
> > +       bool stay_on;                   /* Stay powered-on during boot. */
> >         enum genpd_sync_state sync_state; /* How sync_state is managed. */
> >         int (*power_off)(struct generic_pm_domain *domain);
> >         int (*power_on)(struct generic_pm_domain *domain);
> > --
> > 2.43.0
> >

Re: [PATCH 00/11] pmdomain: Add generic ->sync_state() support to genpd

[Saravana Kannan]

On Thu, Apr 17, 2025 at 7:25 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> If a PM domain (genpd) is powered-on during boot, there is probably a good
> reason for it. Therefore it's known to be a bad idea to allow such genpd to be
> powered-off before all of its consumer devices have been probed. This series
> intends to fix this problem.
>
> We have been discussing these issues at LKML and at various Linux-conferences
> in the past. I have therefore tried to include the people I can recall being
> involved, but I may have forgotten some (my apologies), feel free to loop them
> in.
>
> A few notes:
> *)
> Even if this looks good, the last patch can't go in without some additional
> changes to a couple of existing genpd provider drivers. Typically genpd provider
> drivers that implements ->sync_state() need to call of_genpd_sync_state(), but I
> will fix this asap, if we think the series makes sense.
>
> *)
> Patch 1 -> 3 are just preparatory cleanups.
>
> *)
> I have tested this with QEMU with a bunch of local test-drivers and DT nodes.
> Let me know if you want me to share this code too.
>
>
> Please help review and test!
> Finally, a big thanks to Saravana for all the support!

You are welcome! Thank you for getting this series done! Happy to see
sync_state() support being added to another framework.

I skimmed the series and at a high level it looks right. Not too
familiar with power domain code, so I didn't go deep. Just reviewed
the fw_devlink and driver core parts of it.

-Saravana

>
> Kind regards
> Ulf Hansson
>
>
> Saravana Kannan (1):
>   driver core: Add dev_set_drv_sync_state()
>
> Ulf Hansson (10):
>   pmdomain: core: Convert genpd_power_off() to void
>   pmdomain: core: Simplify return statement in genpd_power_off()
>   pmdomain: core: Use genpd->opp_table to simplify error/remove path
>   pmdomain: core: Add a bus and a driver for genpd providers
>   pmdomain: core: Use device_set_node() to assign the fwnode too
>   pmdomain: core: Add the genpd->dev to the genpd provider bus
>   pmdomain: core: Export a common ->sync_state() helper for genpd
>     providers
>   pmdomain: core: Add internal ->sync_state() support for genpd
>     providers
>   pmdomain: core: Default to use of_genpd_sync_state() for genpd
>     providers
>   pmdomain: core: Leave powered-on genpds on until ->sync_state()
>
>  drivers/pmdomain/core.c   | 273 +++++++++++++++++++++++++++++++-------
>  include/linux/device.h    |  12 ++
>  include/linux/pm_domain.h |  11 ++
>  3 files changed, 249 insertions(+), 47 deletions(-)
>
> --
> 2.43.0
>

Re: [PATCH 00/11] pmdomain: Add generic ->sync_state() support to genpd

[Tomi Valkeinen]

Hi,

On 17/04/2025 17:24, Ulf Hansson wrote:
> If a PM domain (genpd) is powered-on during boot, there is probably a good
> reason for it. Therefore it's known to be a bad idea to allow such genpd to be
> powered-off before all of its consumer devices have been probed. This series
> intends to fix this problem.
> 
> We have been discussing these issues at LKML and at various Linux-conferences
> in the past. I have therefore tried to include the people I can recall being
> involved, but I may have forgotten some (my apologies), feel free to loop them
> in.
> 
> A few notes:
> *)
> Even if this looks good, the last patch can't go in without some additional
> changes to a couple of existing genpd provider drivers. Typically genpd provider
> drivers that implements ->sync_state() need to call of_genpd_sync_state(), but I
> will fix this asap, if we think the series makes sense.
> 
> *)
> Patch 1 -> 3 are just preparatory cleanups.
> 
> *)
> I have tested this with QEMU with a bunch of local test-drivers and DT nodes.
> Let me know if you want me to share this code too.
> 
> 
> Please help review and test!
> Finally, a big thanks to Saravana for all the support!

I had a quick test with this on TI's AM62 board. A few observations.

With this series, all the individual PDs seem to get a state_synced file:

...
/sys/devices/genpd_provider/pd:143/state_synced
/sys/devices/genpd_provider/pd:54/state_synced
/sys/devices/genpd_provider/pd:105/state_synced
/sys/devices/genpd_provider/pd:62/state_synced
/sys/devices/genpd_provider/pd:141/state_synced
...

Is that on purpose? What do these files represent? They all seem to be "1".

When I boot up, I see the sync_state pending:

[   22.541292] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
2b10000.audio-contro
ller
[   22.554839] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
e0f0000.watchdog
[   22.566550] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
e030000.watchdog
[   22.577854] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
e020000.watchdog
[   22.589239] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
e010000.watchdog
[   22.600674] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
e000000.watchdog
[   22.611875] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
30200000.dss
[   22.622813] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
fd00000.gpu
[   22.633565] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
b00000.temperature-s
ensor
[   22.645540] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
2b300050.target-modu
le
[   22.657067] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
chosen:framebuffer@0

The "real" state_synced file on this platform is:

/sys/devices/platform/bus@f0000/44043000.system-controller/44043000.system-controller:power-controller/state_synced

In strict mode, this shows 0, and if I echo 1 (interestingly "echo 1 > 
/sys/..." doesn't work, I need "echo -n 1 > /sys/...), I see PDs getting 
powered off (added a debug print there):

[   87.335487] ti_sci_pd_power_off 88
[   87.342896] ti_sci_pd_power_off 87
[   87.347404] ti_sci_pd_power_off 86
[   87.356464] ti_sci_pd_power_off 128
[   87.361296] ti_sci_pd_power_off 127
[   87.368714] ti_sci_pd_power_off 126
[   87.373349] ti_sci_pd_power_off 125
[   87.378077] ti_sci_pd_power_off 62
[   87.382587] ti_sci_pd_power_off 60
[   87.387194] ti_sci_pd_power_off 59
[   87.391759] ti_sci_pd_power_off 53
[   87.396648] ti_sci_pd_power_off 52
[   87.400801] ti_sci_pd_power_off 51
[   87.405131] ti_sci_pd_power_off 75
[   87.409238] ti_sci_pd_power_off 143
[   87.413328] ti_sci_pd_power_off 142
[   87.417403] ti_sci_pd_power_off 141
[   87.421494] ti_sci_pd_power_off 105
[   87.425632] ti_sci_pd_power_off 104
[   87.429815] ti_sci_pd_power_off 103
[   87.433941] ti_sci_pd_power_off 102
[   87.438054] ti_sci_pd_power_off 158
[   87.442151] ti_sci_pd_power_off 156
[   87.446324] ti_sci_pd_power_off 155
[   87.450463] ti_sci_pd_power_off 154
[   87.454549] ti_sci_pd_power_off 153
[   87.458671] ti_sci_pd_power_off 152
[   87.462571] ti_sci_pd_power_off 43
[   87.466425] ti_sci_pd_power_off 42
[   87.470254] ti_sci_pd_power_off 41
[   87.474032] ti_sci_pd_power_off 40
[   87.477825] ti_sci_pd_power_off 39
[   87.481609] ti_sci_pd_power_off 38
[   87.485432] ti_sci_pd_power_off 37
[   87.489256] ti_sci_pd_power_off 36
[   87.493077] ti_sci_pd_power_off 95
[   87.496845] ti_sci_pd_power_off 132
[   87.500780] ti_sci_pd_power_off 107
[   87.504583] ti_sci_pd_power_off 114
[   87.508429] ti_sci_pd_power_off 79
[   87.512050] ti_sci_pd_power_off 148
[   87.515859] ti_sci_pd_power_off 147
[   87.519644] ti_sci_pd_power_off 106
[   87.523414] ti_sci_pd_power_off 149
[   87.527203] ti_sci_pd_power_off 50
[   87.530971] ti_sci_pd_power_off 49
[   87.534708] ti_sci_pd_power_off 48
[   87.538401] ti_sci_pd_power_off 35
[   87.542040] ti_sci_pd_power_off 186

We do have a lot of "extra" PDs enabled by the bootloader...

With the timeout mode, I see the sync_state() getting called some 
seconds after the boot has finished.

So... I think it all works as expected. You can take this as some kind 
of Tested-by, but it'd be good if someone from TI who knows more about 
PDs would test this too =).

Interestingly, I see a difference in behavior to the old patches from 
Abel: with the old patches, if I boot up with the DSS (display 
subsystem) enabled by the bootloader, it looks the same as with these 
patches. However, with the old patches, when I load the DSS driver, and 
it probes successfully, the DSS PD will get managed correctly, i.e. if I 
blank the screen, the DSS PD will go to off, even if the sync_state has 
not been called.

With these patches the DSS PD will stay on, no matter if I load the DSS 
driver or not, and will only go off after sync_state has been called.

I'm not quite sure here, but I think the behavior with the old patches 
makes sense: when the driver for a particular PD loads, the PD no longer 
needs to be kept on. Or... Is this about the case where a PD has 
multiple consumers? The PD provider cannot know how many consumers there 
are for a single PD, so it must keep all boot-time-enabled PDs on until 
sync_state() (i.e. all the consumer drivers have probed)?

  Tomi

Re: [PATCH 00/11] pmdomain: Add generic ->sync_state() support to genpd

[Ulf Hansson]

On Thu, 24 Apr 2025 at 12:59, Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> Hi,
>
> On 17/04/2025 17:24, Ulf Hansson wrote:
> > If a PM domain (genpd) is powered-on during boot, there is probably a good
> > reason for it. Therefore it's known to be a bad idea to allow such genpd to be
> > powered-off before all of its consumer devices have been probed. This series
> > intends to fix this problem.
> >
> > We have been discussing these issues at LKML and at various Linux-conferences
> > in the past. I have therefore tried to include the people I can recall being
> > involved, but I may have forgotten some (my apologies), feel free to loop them
> > in.
> >
> > A few notes:
> > *)
> > Even if this looks good, the last patch can't go in without some additional
> > changes to a couple of existing genpd provider drivers. Typically genpd provider
> > drivers that implements ->sync_state() need to call of_genpd_sync_state(), but I
> > will fix this asap, if we think the series makes sense.
> >
> > *)
> > Patch 1 -> 3 are just preparatory cleanups.
> >
> > *)
> > I have tested this with QEMU with a bunch of local test-drivers and DT nodes.
> > Let me know if you want me to share this code too.
> >
> >
> > Please help review and test!
> > Finally, a big thanks to Saravana for all the support!
>
> I had a quick test with this on TI's AM62 board. A few observations.
>
> With this series, all the individual PDs seem to get a state_synced file:
>
> ...
> /sys/devices/genpd_provider/pd:143/state_synced
> /sys/devices/genpd_provider/pd:54/state_synced
> /sys/devices/genpd_provider/pd:105/state_synced
> /sys/devices/genpd_provider/pd:62/state_synced
> /sys/devices/genpd_provider/pd:141/state_synced
> ...
>
> Is that on purpose? What do these files represent? They all seem to be "1".

It's on purpose, but in this case there are no fw_devlink tracking
them, but instead that's done via..

/sys/devices/platform/bus@f0000/44043000.system-controller/44043000.system-controller:power-controller/state_synced

..as you point out below.

Depending on the DT layout these nodes may be useful, but not in the
TI PM domain case.

>
> When I boot up, I see the sync_state pending:
>
> [   22.541292] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> 2b10000.audio-contro
> ller
> [   22.554839] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> e0f0000.watchdog
> [   22.566550] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> e030000.watchdog
> [   22.577854] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> e020000.watchdog
> [   22.589239] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> e010000.watchdog
> [   22.600674] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> e000000.watchdog
> [   22.611875] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> 30200000.dss
> [   22.622813] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> fd00000.gpu
> [   22.633565] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> b00000.temperature-s
> ensor
> [   22.645540] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> 2b300050.target-modu
> le
> [   22.657067] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> chosen:framebuffer@0
>
> The "real" state_synced file on this platform is:
>
> /sys/devices/platform/bus@f0000/44043000.system-controller/44043000.system-controller:power-controller/state_synced
>
> In strict mode, this shows 0, and if I echo 1 (interestingly "echo 1 >
> /sys/..." doesn't work, I need "echo -n 1 > /sys/...), I see PDs getting
> powered off (added a debug print there):
>
> [   87.335487] ti_sci_pd_power_off 88
> [   87.342896] ti_sci_pd_power_off 87
> [   87.347404] ti_sci_pd_power_off 86
> [   87.356464] ti_sci_pd_power_off 128
> [   87.361296] ti_sci_pd_power_off 127
> [   87.368714] ti_sci_pd_power_off 126
> [   87.373349] ti_sci_pd_power_off 125
> [   87.378077] ti_sci_pd_power_off 62
> [   87.382587] ti_sci_pd_power_off 60
> [   87.387194] ti_sci_pd_power_off 59
> [   87.391759] ti_sci_pd_power_off 53
> [   87.396648] ti_sci_pd_power_off 52
> [   87.400801] ti_sci_pd_power_off 51
> [   87.405131] ti_sci_pd_power_off 75
> [   87.409238] ti_sci_pd_power_off 143
> [   87.413328] ti_sci_pd_power_off 142
> [   87.417403] ti_sci_pd_power_off 141
> [   87.421494] ti_sci_pd_power_off 105
> [   87.425632] ti_sci_pd_power_off 104
> [   87.429815] ti_sci_pd_power_off 103
> [   87.433941] ti_sci_pd_power_off 102
> [   87.438054] ti_sci_pd_power_off 158
> [   87.442151] ti_sci_pd_power_off 156
> [   87.446324] ti_sci_pd_power_off 155
> [   87.450463] ti_sci_pd_power_off 154
> [   87.454549] ti_sci_pd_power_off 153
> [   87.458671] ti_sci_pd_power_off 152
> [   87.462571] ti_sci_pd_power_off 43
> [   87.466425] ti_sci_pd_power_off 42
> [   87.470254] ti_sci_pd_power_off 41
> [   87.474032] ti_sci_pd_power_off 40
> [   87.477825] ti_sci_pd_power_off 39
> [   87.481609] ti_sci_pd_power_off 38
> [   87.485432] ti_sci_pd_power_off 37
> [   87.489256] ti_sci_pd_power_off 36
> [   87.493077] ti_sci_pd_power_off 95
> [   87.496845] ti_sci_pd_power_off 132
> [   87.500780] ti_sci_pd_power_off 107
> [   87.504583] ti_sci_pd_power_off 114
> [   87.508429] ti_sci_pd_power_off 79
> [   87.512050] ti_sci_pd_power_off 148
> [   87.515859] ti_sci_pd_power_off 147
> [   87.519644] ti_sci_pd_power_off 106
> [   87.523414] ti_sci_pd_power_off 149
> [   87.527203] ti_sci_pd_power_off 50
> [   87.530971] ti_sci_pd_power_off 49
> [   87.534708] ti_sci_pd_power_off 48
> [   87.538401] ti_sci_pd_power_off 35
> [   87.542040] ti_sci_pd_power_off 186
>
> We do have a lot of "extra" PDs enabled by the bootloader...
>
> With the timeout mode, I see the sync_state() getting called some
> seconds after the boot has finished.
>
> So... I think it all works as expected. You can take this as some kind
> of Tested-by, but it'd be good if someone from TI who knows more about
> PDs would test this too =).

Thanks a lot for testing and sharing your information!

>
> Interestingly, I see a difference in behavior to the old patches from
> Abel: with the old patches, if I boot up with the DSS (display
> subsystem) enabled by the bootloader, it looks the same as with these
> patches. However, with the old patches, when I load the DSS driver, and
> it probes successfully, the DSS PD will get managed correctly, i.e. if I
> blank the screen, the DSS PD will go to off, even if the sync_state has
> not been called.
>
> With these patches the DSS PD will stay on, no matter if I load the DSS
> driver or not, and will only go off after sync_state has been called.
>
> I'm not quite sure here, but I think the behavior with the old patches
> makes sense: when the driver for a particular PD loads, the PD no longer
> needs to be kept on. Or... Is this about the case where a PD has
> multiple consumers? The PD provider cannot know how many consumers there
> are for a single PD, so it must keep all boot-time-enabled PDs on until
> sync_state() (i.e. all the consumer drivers have probed)?

You are correct!

ti_sci_pm_domains are modelled in DT by using:
#power-domain-cells = <1>;
or
#power-domain-cells = <2>;

fw_devlink doesn't look at those additional specifiers in DT. For
example, if a consumer has "power-domains = <&k2g_pds 5>;" the '5'
will not be considered as a separate domain, but instead all consumers
of &k2g_pds needs to be probed, before the ->sync_state() gets called.

Theoretically, if we could treat the specifier ('5' in this case) as
being a separate domain, that should would for most cases. The
question is, how difficult it would be to extend fw_devlink to support
this, so that when all consumers that has  "power-domains = <&k2g_pds
5>" has probed, the ->sync_state() get's invoked for the corresponding
genpd->dev.

If Saravanna want to comment on this, that would be nice, otherwise I
will chat with him offlist about this.

That said, it seems like this is working fine for the TI platforms,
which is great!

Kind regards
Uffe