[PATCH v3 06/13] pmdomain: core: Add initial fine grained sync_state support
Ulf Hansson
ulf.hansson at linaro.org
Wed May 13 02:35:28 PDT 2026
On Wed, 13 May 2026 at 07:34, Saravana Kannan <saravanak at kernel.org> wrote:
>
> On Mon, May 11, 2026 at 3:25 AM Ulf Hansson <ulf.hansson at linaro.org> wrote:
> >
> > On Mon, 11 May 2026 at 07:09, Saravana Kannan <saravanak at kernel.org> wrote:
> > >
> > > 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?
> >
> > 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.
>
> That patch should be pretty simple, so let's just do it? You are
> literally just moving the code to another file and massaging it a bit.
> I can send one out if you want/
Okay, I will fold in a patch into my series that adds the helper. No
point you sending it as it needs to be apart of my series to have a
user for it.
>
> > >
> > > 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.
>
> Ok, correct me if I'm wrong here, but it looks like you are looking at
> all the consumers, finding the ones that haven't probed yet and then
> trying to see which specific genpd provider each one of them is using
> by parsing their dt properties and then keep those ON.
The code cares only about the genpd OF provider that the supplier
device belongs to, if any. Any other genpd OF providers and their
genpds is untouched.
The corresponding genpd OF provider may provide multiple genpds
through the same fwnode.
>
> If my understanding above is wrong then, please help me understand
> what the code is trying to do.
>
> If it's right, then here's the issue:
>
> fw_devlink relies on device links and fwnode links to keep track of
> consumers. fwnode links get created first and then get converted to
> device links when both the supplier AND consumer devices are created.
>
> Consider a case when the supplier S is created and the consumer device
> C is several levels deep inside a parent device A.
> S { #power-domain-cells = <1>; }
> A { B { C { power-domains = <&S MY_DOMAIN>; } } }
>
> fw_devlink can't just go "oh there's no consumer device that hasn't
> probed yet, let me call sync_state()". It needs to wait for C. But
> there's no way to create a device link to C. So, here's the sequence
> that happens:
>
> 1. When device A gets added, it creates a "proxy" SYNC_STATE_ONLY link
> between S and A.
> 2. When A probes, it adds device B.
> 3. The SYNC_STATE_ONLY link between S to B is created.
> 4. The SYNC_STATE_ONLY link between S to A is deleted.
> 5. When B probes, it adds device C.
> 6. The actual device link between S and C is created.
> 7. The SYNC_STATE_ONLY link between S to B is deleted.
> 8. Device C probes, the device links get updated, sync_state() gets called.
>
> So, if your code doesn't account for SYNC_STATE_ONLY links, you are
> going to check device "A" to see which providers are used. You'll
> think that MY_DOMAIN isn't used by any unprobed consumer and turn it
> off.
>
> >
> > >
> > > 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.
>
> I'm not sure what you are trying to say here. Yes, what you are saying
> is true. But at the point the current code returns before calling
> sync_state(), your patch 2/13 ends up calling the "consumer_probed()"
> callback.
>
> If you see the example I gave above, there is a SYNC_STATE_ONLY link
> all the way up to step 7 that'll prevent sync_state() from being
> called. And until step 5, there is no device link between S and C and
> you'll have to go look at S's fwnode links to find C.
Thanks for the details, but I don't think the code in genpd needs to
take these details into account. Or at least that is my goal, if
possible.
As I understand it, all of the above should already been taken care of
when __device_links_queue_sync_state() is called, as it's at that
point when we can validate whether all consumers for a supplier have
been probed, right?
>
> > 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()?
>
> Does my explanation above make sense?
I think you may have misunderstood how the code in $subject patch
works. Let's try again.
>
> Welcome to the annoying worlds of fw_devlink corner cases/nuances.
> There's a case where the child is the supplier of the
> parent/grandparent. There's a comment for that in the fw_devlink code.
Yeah, I am fully aware of the high level of complexity and as I said,
I don't want genpd to have to know about *all* of that.
[...]
Kind regards
Uffe
More information about the linux-arm-kernel
mailing list