[PATCH v3 06/13] pmdomain: core: Add initial fine grained sync_state support
Saravana Kannan
saravanak at kernel.org
Sun May 10 22:09:03 PDT 2026
On Fri, May 8, 2026 at 5:39 AM Ulf Hansson <ulf.hansson at linaro.org> wrote:
>
> A onecell (#power-domain-cells = <1 or 2>; in DT) power domain provider
> typically provides multiple independent power domains, each with their own
> corresponding consumers. In these cases we have to wait for all consumers
> for all the provided power domains before the ->sync_state() callback gets
> called for the supplier.
>
> In a first step to improve this, let's implement support for fine grained
> sync_state support a per genpd basis by using the ->queue_sync_state()
> callback. To take step by step, let's initially limit the improvement to
> the internal genpd provider driver and to its corresponding genpd devices
> for onecell providers.
>
> Signed-off-by: Ulf Hansson <ulf.hansson at linaro.org>
> ---
>
> Changes in v3:
> - Addressed some cosmetic comments from Geert.
>
> ---
> drivers/pmdomain/core.c | 124 ++++++++++++++++++++++++++++++++++++++
> include/linux/pm_domain.h | 1 +
> 2 files changed, 125 insertions(+)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index ad57846f02a3..c01a9a96e5c2 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -2699,6 +2699,119 @@ static struct generic_pm_domain *genpd_get_from_provider(
> return genpd;
> }
>
> +static bool genpd_should_wait_for_consumer(struct device_node *np)
> +{
> + struct generic_pm_domain *genpd;
> + bool should_wait = false;
> +
> + mutex_lock(&gpd_list_lock);
> + list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> + if (genpd->provider == of_fwnode_handle(np)) {
> + genpd_lock(genpd);
> +
> + /* Clear the previous state before reevaluating. */
> + genpd->wait_for_consumer = false;
> +
> + /*
> + * Unless there is at least one genpd for the provider
> + * that is being kept powered-on, we don't have to care
> + * about waiting for consumers.
> + */
> + if (genpd->stay_on)
> + should_wait = true;
> +
> + genpd_unlock(genpd);
> + }
> + }
> + mutex_unlock(&gpd_list_lock);
I think I understand the intent of this function, but haven't dug into
genpd code enough to comment on this yet. I'll come back to this
later.
> +
> + return should_wait;
> +}
> +
> +static void genpd_parse_for_consumer(struct device_node *sup,
> + struct device_node *con)
> +{
> + struct generic_pm_domain *genpd;
> +
> + for (unsigned int i = 0; ; i++) {
> + struct of_phandle_args pd_args;
> +
> + if (of_parse_phandle_with_args(con, "power-domains",
> + "#power-domain-cells",
> + i, &pd_args))
> + break;
Why not use a while or a do while() instead of this infinite for loop
with a break?
> +
> + /*
> + * The phandle must correspond to the supplier's genpd provider
> + * to be relevant else let's move to the next index.
> + */
> + if (sup != pd_args.np) {
> + of_node_put(pd_args.np);
> + continue;
> + }
> +
> + mutex_lock(&gpd_list_lock);
> + genpd = genpd_get_from_provider(&pd_args);
> + if (!IS_ERR(genpd)) {
> + genpd_lock(genpd);
> + genpd->wait_for_consumer = true;
> + genpd_unlock(genpd);
> + }
> + mutex_unlock(&gpd_list_lock);
> +
> + of_node_put(pd_args.np);
> + }
> +}
> +
> +static void _genpd_queue_sync_state(struct device_node *np)
> +{
> + struct generic_pm_domain *genpd;
> +
> + mutex_lock(&gpd_list_lock);
> + list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> + if (genpd->provider == of_fwnode_handle(np)) {
> + genpd_lock(genpd);
> + if (genpd->stay_on && !genpd->wait_for_consumer) {
> + genpd->stay_on = false;
> + genpd_queue_power_off_work(genpd);
> + }
> + genpd_unlock(genpd);
> + }
> + }
> + mutex_unlock(&gpd_list_lock);
> +}
> +
> +static void genpd_queue_sync_state(struct device *dev)
> +{
> + struct device_node *np = dev->of_node;
> + struct device_link *link;
> +
> + if (!genpd_should_wait_for_consumer(np))
> + return;
> +
> + list_for_each_entry(link, &dev->links.consumers, s_node) {
Couple of issues:
1. I don't want the frameworks to be so deeply aware of driver core
internals. I want the driver core maintainers to be able to change the
devlink implementation without having to worry about going and fixing
all the frameworks. So, please add a for_each_consumer_dev(supplier,
callback) and for_each_supplier_dev(consumer, callback) helper
functions.
2. This doesn't ignore "SYNC_STATE_ONLY" links and that's going to
confuse the consumer count/check you might do or at the least waste
parsing those.
3. **Device** links are not the complete list of consumers because
they can only link consumer **devices** once the consumer **device**
is created.
4. What you really need is a for_each_consumer_fwnode(supplier,
callback) that first loops through all the consumer device links and
calls the callback() on their fwnode and then the same function needs
to loop through all the fwnode links and then pass those consumer
fwnodes to the callback. And inside that callback you can do whatever
you want.
5. You might want to add a for_each_inactive_consumer(supplier,
callback) too to simplify your need for checking if a fwnode has a
device and then checking if it's probed.
Thanks,
Saravana
> + struct device *consumer = link->consumer;
> +
> + if (!device_link_test(link, DL_FLAG_MANAGED))
> + continue;
> +
> + if (link->status == DL_STATE_ACTIVE)
> + continue;
> +
> + if (!consumer->of_node)
> + continue;
> +
> + /*
> + * A consumer device has not been probed yet. Let's parse its
> + * device node for the power-domains property, to find out the
> + * genpds it may belong to and then prevent sync state for them.
> + */
> + genpd_parse_for_consumer(np, consumer->of_node);
> + }
> +
> + _genpd_queue_sync_state(np);
> +}
> +
> static void genpd_sync_state(struct device *dev)
> {
> return of_genpd_sync_state(dev->of_node);
> @@ -3531,6 +3644,16 @@ static int genpd_provider_probe(struct device *dev)
> return 0;
> }
>
> +static void genpd_provider_queue_sync_state(struct device *dev)
> +{
> + struct generic_pm_domain *genpd = container_of(dev, struct generic_pm_domain, dev);
> +
> + if (genpd->sync_state != GENPD_SYNC_STATE_ONECELL)
> + return;
> +
> + genpd_queue_sync_state(dev);
> +}
> +
> static void genpd_provider_sync_state(struct device *dev)
> {
> struct generic_pm_domain *genpd = container_of(dev, struct generic_pm_domain, dev);
> @@ -3559,6 +3682,7 @@ static struct device_driver genpd_provider_drv = {
> .name = "genpd_provider",
> .bus = &genpd_provider_bus_type,
> .probe = genpd_provider_probe,
> + .queue_sync_state = genpd_provider_queue_sync_state,
> .sync_state = genpd_provider_sync_state,
> .suppress_bind_attrs = true,
> };
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index b299dc0128d6..7aa49721cde5 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -215,6 +215,7 @@ struct generic_pm_domain {
> 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. */
> + bool wait_for_consumer; /* Consumers awaits to be probed. */
> 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
>
More information about the linux-arm-kernel
mailing list