Re: [PATCH 2/3] pmdomain: core: add support for power-domains-child-ids

[Ulf Hansson]

On Fri, 10 Apr 2026 at 02:45, Kevin Hilman <khilman@xxxxxxxxxxxx> wrote:
>
> Ulf Hansson <ulf.hansson@xxxxxxxxxx> writes:
>
> > On Wed, 11 Mar 2026 at 01:19, Kevin Hilman (TI) <khilman@xxxxxxxxxxxx> wrote:
> >>
> >> Currently, PM domains can only support hierarchy for simple
> >> providers (e.g. ones with #power-domain-cells = 0).
> >>
> >> Add support for oncell providers as well by adding a new property
> >> `power-domains-child-ids` to describe the parent/child relationship.
> >>
> >> For example, an SCMI PM domain provider has multiple domains, each of
> >> which might be a child of diffeent parent domains. In this example,
> >> the parent domains are MAIN_PD and WKUP_PD:
> >>
> >>     scmi_pds: protocol@11 {
> >>         reg = <0x11>;
> >>         #power-domain-cells = <1>;
> >>         power-domains = <&MAIN_PD>, <&WKUP_PD>;
> >>         power-domains-child-ids = <15>, <19>;
> >>     };
> >>
> >> With this example using the new property, SCMI PM domain 15 becomes a
> >> child domain of MAIN_PD, and SCMI domain 19 becomes a child domain of
> >> WKUP_PD.
> >>
> >> To support this feature, add two new core functions
> >>
> >> - of_genpd_add_child_ids()
> >> - of_genpd_remove_child_ids()
> >>
> >> which can be called by pmdomain providers to add/remove child domains
> >> if they support the new property power-domains-child-ids.
> >>
> >> Signed-off-by: Kevin Hilman (TI) <khilman@xxxxxxxxxxxx>
> >
> > Thanks for working on this! It certainly is a missing feature!
>
> You're welcome, thanks for the detailed review.
>
> >> ---
> >>  drivers/pmdomain/core.c   | 169 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/pm_domain.h |  16 ++++++++++++++++
> >>  2 files changed, 185 insertions(+)
> >>
> >> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> >> index 61c2277c9ce3..acb45dd540b7 100644
> >> --- a/drivers/pmdomain/core.c
> >> +++ b/drivers/pmdomain/core.c
> >> @@ -2909,6 +2909,175 @@ static struct generic_pm_domain *genpd_get_from_provider(
> >>         return genpd;
> >>  }
> >>
> >> +/**
> >> + * of_genpd_add_child_ids() - Parse power-domains-child-ids property
> >> + * @np: Device node pointer associated with the PM domain provider.
> >> + * @data: Pointer to the onecell data associated with the PM domain provider.
> >> + *
> >> + * Parse the power-domains and power-domains-child-ids properties to establish
> >> + * parent-child relationships for PM domains. The power-domains property lists
> >> + * parent domains, and power-domains-child-ids lists which child domain IDs
> >> + * should be associated with each parent.
> >> + *
> >> + * Returns 0 on success, -ENOENT if properties don't exist, or negative error code.
> >
> > I think we should avoid returning specific error codes for specific
> > errors, simply because it usually becomes messy.
> >
> > If I understand correctly the intent here is to allow the caller to
> > check for -ENOENT and potentially avoid bailing out as it may not
> > really be an error, right?
>
> Right, -ENOENT is not an error of parsing, it's to indicate that there
> are no child-ids to be parsed.
>
> > Perhaps a better option is to return the number of children for whom
> > we successfully assigned parents. Hence 0 or a positive value allows
> > the caller to understand what happened. More importantly, a negative
> > error code then really becomes an error for the caller to consider.
>
> I explored this a bit, but it gets messy quick.  It means we have to
> track cases where only some of the children were added as well as when
> all children were added.   Personally, I think this should be an "all or
> nothing" thing.  If all the children cannot be parsed/added, then none
> of them should be added.
>
> This also allows the remove to not have to care about how many were
> added, and just remove them all, with the additional benefit of not
> having to track the state of how many children were successfully added.
>

I fully agree, it should be all or nothing. Failing with one
child/parent should end up with an error code being returned.

That said, it still seems to make perfect sense to return the number
of children for whom we assigned parents for, no?

[...]

> >> +int of_genpd_remove_child_ids(struct device_node *np,
> >> +                          struct genpd_onecell_data *data)
> >> +{
> >> +       struct of_phandle_args parent_args;
> >> +       struct generic_pm_domain *parent_genpd, *child_genpd;
> >> +       struct of_phandle_iterator it;
> >> +       const struct property *prop;
> >> +       const __be32 *item;
> >> +       u32 child_id;
> >> +       int ret;
> >> +
> >> +       /* Check if both properties exist */
> >> +       if (of_count_phandle_with_args(np, "power-domains", "#power-domain-cells") <= 0)
> >> +               return -ENOENT;
> >> +
> >> +       prop = of_find_property(np, "power-domains-child-ids", NULL);
> >> +       if (!prop)
> >> +               return -ENOENT;
> >> +
> >> +       item = of_prop_next_u32(prop, NULL, &child_id);
> >
> > Similar comments as for of_genpd_add_child_ids().
> >
> > Moreover, I think we should remove the children in the reverse order
> > of how we added them.
>
> I'm curious why does the order matter?  The children are all siblings
> (no hierarchy), so why would the order be important?

It might not be that important, but generally, it seems like a good
idea to me to reverse the order when undoing things.

>
> I'm not ware of a phandle iterator/helper to parse in the reverse, so
> that would mean iterating once to create a list, and then walking it in
> reverse.  Seems unnecessary.

Sure, I leave the call to you, to see what fits best.

Kind regards
Uffe