Re: [PATCH v3 06/13] pmdomain: core: Add initial fine grained sync_state support

[Ulf Hansson <ulf.hansson@linaro.org>]

On Mon, 11 May 2026 at 07:09, Saravana Kannan <saravanak@kernel.org> wrote:
>
> On Fri, May 8, 2026 at 5:39 AM Ulf Hansson <ulf.hansson@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@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?

I guess it's a matter of personal preference. I'm not sure the code
gets any nicer with a do/while, but if you really insist I can change
it.

>
> > +
> > +               /*
> > +                * 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.

I understand your concern and I like the idea. However, maybe it's
better to get this landed (the series is complicated as is) first and
then can continue to improve the code on top, with helper functions
etc.

>
> 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.

I am not sure I understand how I should take SYNC_STATE_ONLY links
into account here.

At each call to the genpd_queue_sync_state(), we walk through all the
provided genpds for the provider. No previous state is cached to track
consumer counts.

>
> 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.

The ->queue_sync_state() callback is invoked *after*
__device_links_queue_sync_state() has been called for the device,
which is also when the conditions for calling ->sync_state() is
checked.

If there are problems with not yet registered consumer device links,
why isn't that as problem for regular ->sync_state() in
__device_links_queue_sync_state()?

>
> 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);
> > +}
> > +

[...]

Kind regards
Uffe