Message-ID: <7hpl72wip9.fsf@baylibre.com>
Date: Wed, 21 Jan 2026 17:16:02 -0800
From: Kevin Hilman <khilman@...libre.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 devicetree@...r.kernel.org, linux-pm@...r.kernel.org,
 arm-scmi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v4 1/2] pmdomain: core: support domain hierarchy via
 power-domain-map

Ulf Hansson <ulf.hansson@...aro.org> writes:

> On Thu, 20 Nov 2025 at 01:58, Kevin Hilman (TI.com)
> <khilman@...libre.com> wrote:
>>
>> Add of_genpd_add_subdomain_map() helper function to support
>> hierarchical PM domains defined by using power-domains-map
>> property (c.f. nexus node maps in DT spec, section 2.5.1).
>>
>> This enables PM domain providers with #power-domain-cells > 0 to
>> establish subdomain relationships via the power-domain-map property,
>> which was not previously possible.
>>
>> This new helper function
>> - uses an OF helper to iterate to over entries in power-domain-map
>> - For each mapped entry: extracts child specifier, resolves parent phandle,
>>   extracts parent specifier args, and establishes subdomain relationship
>> - Uses genpd_add_subdomain() with proper gpd_list_lock mutex protection
>>
>> Example from k3-am62l.dtsi:
>>
>>   scmi_pds: protocol@11 {
>>       #power-domain-cells = <1>;
>>       power-domain-map = <15 &MAIN_PD>,  /* TIMER0 */
>>                          <19 &WKUP_PD>;  /* WKUP_TIMER0 */
>>   };
>>
>>   MAIN_PD: power-controller-main {
>>       #power-domain-cells = <0>;
>>   };
>>
>>   WKUP_PD: power-controller-main {
>>       #power-domain-cells = <0>;
>>   };
>>
>> This allows SCMI power domain 15 to become a subdomain of MAIN_PD, and
>> domain 19 to become a subdomain of WKUP_PD.
>
> Nitpick:
> As long as possible, please use the terminology "parent-domain" and
> "child-domain" and avoid "subdomain". There are a couple of cases of
> this, in the code too, can you please update all of them?

OK.

>>
>> Signed-off-by: Kevin Hilman (TI.com) <khilman@...libre.com>
>> ---
>>  drivers/pmdomain/core.c   | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h |  9 +++++++
>>  2 files changed, 73 insertions(+)
>>
>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
>> index 61c2277c9ce3..592e9126896c 100644
>> --- a/drivers/pmdomain/core.c
>> +++ b/drivers/pmdomain/core.c
>> @@ -3483,6 +3483,70 @@ int of_genpd_parse_idle_states(struct device_node *dn,
>>  }
>>  EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>>
>
> We need to add some description of the function here.

OK.

>> +int of_genpd_add_subdomain_map(struct device_node *np,
>
> Nitpick:
> Hmm, either we should keep consistency with the name
> "of_genpd_add_subdomain", according to what you propose - or we should
> take the opportunity to move to use "child" in the name instead
> (of_genpd_add_child_domain_map()).
>
> Sooner or later it would be nice if we could rename
> of_genpd_add_subdomain() (and friends) to of_genpd_add_child_domain().
>
> No big deal at this point, I am fine with whatever name you decide to use.

I will update the changelogs/comments/descriptions etc. to use
parent/child, but I will leave subdomain in the function name since
that's what all the other APIs use.  Then in a later cleanup step, we
could rename the subdomain APIs to child APIs.

>> +                              struct genpd_onecell_data *data)
>> +{
>> +       struct generic_pm_domain *genpd, *parent_genpd;
>
> Maybe use "child" and "parent" as variable names instead. This should
> make the code a bit more clear.

OK.

>> +       struct of_phandle_args child_args, parent_args;
>> +       int index = 0;
>> +       int ret = 0;
>> +       u32 child_index;
>> +
>> +       if (!np || !data)
>> +               return -EINVAL;
>> +
>> +       /* Iterate through power-domain-map entries using the OF helper */
>> +       while (!of_parse_map_iter(np, "power-domain", &index,
>> +                                  &child_args, &parent_args)) {
>> +               /* Extract the child domain index from the child specifier */
>> +               if (child_args.args_count < 1) {
>
> This should be exactly 1, right?

Hmm, I'm not sure exactly what you mean.  Are you suggesting this check
should be "!= 1" instead of "< 1"?

I think args_count should match #power-domain-cells.  So for SCMI, this
should indeed be 1.  But if this function is used for other domains
where #power-domain-cells is > 1, then the current check for "< 1" is
correct.

>> +                       of_node_put(parent_args.np);
>> +                       ret = -EINVAL;
>> +                       break;
>
> If we fail here, we should remove child domains that we added for the
> earlier indexes in the while loop, rather than just bailing out.
>
> This applies to other error paths below too.

Yeah, the current error handling isn't really in place (hence the RFC)
but I will add it for the next version.

I'm planning to take the approach that all children in the map have to
be successfully added in order for this function to be considered
successful.  If any of the children fail to get added (for any reason),
then they all should be removed.

This remove function will look *very* similar to the add function
because it will have to parse the map (again), finding parent and child
info and attempting to remove each child from the parent.

At first, this seems pretty inefficient, but I think it's better than
the add function being required to keep track of the state of which
domains were successfully added.  Which gets even more complicated if
there are multiple domains which use power-domain-map.

So fora now, I plan to avoid tracking all that state, and have the
remove function be a simple reversal of the add, but looping through the
whole map, even if it fails to remove some items (because they may not
have been added in the first place.)

>> +               }
>> +               child_index = child_args.args[0];
>> +
>> +               /* Validate child domain index */
>> +               if (child_index >= data->num_domains) {
>> +                       of_node_put(parent_args.np);
>> +                       continue;
>
> I don't think we should just continue here, but instead treat this as an error.

Yes.

>> +               }
>> +
>> +               genpd = data->domains[child_index];
>> +               if (!genpd) {
>> +                       of_node_put(parent_args.np);
>> +                       continue;
>
> Ditto.

Yes.

>> +               }
>> +
>> +               /* Get parent power domain from provider and establish subdomain relationship */
>> +               mutex_lock(&gpd_list_lock);
>> +
>> +               parent_genpd = genpd_get_from_provider(&parent_args);
>> +               if (IS_ERR(parent_genpd)) {
>> +                       mutex_unlock(&gpd_list_lock);
>> +                       of_node_put(parent_args.np);
>> +                       ret = PTR_ERR(parent_genpd);
>> +                       dev_err(&genpd->dev, "failed to get parent domain: %d\n", ret);
>
> Perhaps clarify the print by changing the text to state that we can't
> find the parent's OF provider. If the print is needed at all.

Print probably isn't needed at all, but just useful for development
debug purposes.

>> +                       break;
>> +               }
>> +
>> +               ret = genpd_add_subdomain(parent_genpd, genpd);
>> +               mutex_unlock(&gpd_list_lock);
>> +               of_node_put(parent_args.np);
>> +
>> +               if (ret) {
>> +                       dev_err(&genpd->dev, "failed to add as subdomain of %s: %d\n",
>> +                               parent_genpd->name, ret);
>> +                       break;
>> +               }
>> +
>> +               dev_info(&genpd->dev, "added as subdomain of %s\n",
>> +                       parent_genpd->name);
>> +       }
>> +
>> +       return ret;
>> +}
>
> Except for taking better care in the error path, it also looks like we
> are missing a corresponding function to remove the child-domains that
> was added with the above new function.
>
> Perhaps that function can be used in the error paths too?

Yeah, I as describe above.  I will add that for the next version.

Kevin

Powered by blists - more mailing lists