Re: [PATCH RFC v3 2/2] pmdomain: core: add support for subdomains using power-domain-map

[Kevin Hilman]

Ulf Hansson <ulf.hansson@xxxxxxxxxx> writes:
--text follows this line--
> On Sat, 14 Jun 2025 at 00:39, Kevin Hilman <khilman@xxxxxxxxxxxx> wrote:
>>
>> Currently, PM domains can only support hierarchy for simple
>> providers (e.g. ones with #power-domain-cells = 0).
>>
>> Add more generic support for hierarchy by using nexus node
>> maps (c.f. section 2.5.1 of the DT spec.)
>>
>> For example, we could describe SCMI PM domains with multiple parents
>> domains (MAIN_PD and WKUP_PD) like this:
>>
>>     scmi_pds: protocol@11 {
>>         reg = <0x11>;
>>         #power-domain-cells = <1>;
>>
>>         power-domain-map = <15 &MAIN_PD>,
>>                            <19 &WKUP_PD>;
>>     };
>>
>> which should mean that <&scmi_pds 15> is a subdomain of MAIN_PD and
>> <&scmi_pds 19> is a subdomain of WKUP_PD.
>>
>> IOW, given an SCMI device which uses SCMI PM domains:
>>
>>    main_timer0: timer@2400000 {
>>       power-domains = <&scmi_pds 15>;
>>    };
>>
>> it already implies that main_timer0 is PM domain <&scmi_pds 15>
>>
>> With the new map, this *also* now implies <&scmi_pds 15> is a
>> subdomain of MAIN_PD.
>>
>> Signed-off-by: Kevin Hilman <khilman@xxxxxxxxxxxx>
>> ---
>>  drivers/pmdomain/core.c | 24 ++++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
>> index d6c1ddb807b2..adf022b45d95 100644
>> --- a/drivers/pmdomain/core.c
>> +++ b/drivers/pmdomain/core.c
>> @@ -2998,8 +2998,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>                                  unsigned int index, unsigned int num_domains,
>>                                  bool power_on)
>>  {
>> -       struct of_phandle_args pd_args;
>> -       struct generic_pm_domain *pd;
>> +       struct of_phandle_args pd_args, parent_args;
>> +       struct generic_pm_domain *pd, *parent_pd = NULL;
>>         int ret;
>>
>>         ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
>> @@ -3039,6 +3039,22 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>                         goto err;
>>         }
>>
>> +       /*
>> +        * Check for power-domain-map, which implies the primary
>> +        * power-doamin is a subdomain of the parent found in the map.
>> +        */
>> +       ret = of_parse_phandle_with_args_map(dev->of_node, "power-domains",
>> +                                            "power-domain", index, &parent_args);
>> +       if (!ret && (pd_args.np != parent_args.np)) {
>> +               parent_pd = genpd_get_from_provider(&parent_args);
>> +               of_node_put(parent_args.np);
>> +
>> +               ret = pm_genpd_add_subdomain(parent_pd, pd);
>> +               if (!ret)
>> +                       dev_dbg(dev, "adding PM domain %s as subdomain of %s\n",
>> +                               pd->name, parent_pd->name);
>> +       }
>
> Please move the above new code to a separate shared genpd helper
> function, that genpd providers can call build the topology. This, to
> be consistent with the current way for how we usually add
> parent/child-domains in genpd (see of_genpd_add_subdomain).

Yeah, you had the same comment on v2, and I'm not ignoring you.  But I
thought that moving this code to when devices are attatched to domains
(instead of when providers are created) would solve that problem.  IOW,
in this approach, `power-domain-map` is handled at the same time as a
devices `power-domains = ` property.

So, while I don't really understand the reason that every PM domain
provider has to handle this individually, I've given that a try (see
below.)

> Moreover, we also need a corresponding "cleanup" helper function to
> remove the child-domain (subdomain) correctly, similar to
> of_genpd_remove_subdomain().

Yes, I'll handle that better once I get through this RFC phase to make
sure I'm on th right path.

OK, so below[1] is a shot at just adding helpers to the PM domain core.  I
will then uses these from the SCMI PM domains ->attach_dev() and
->detatch_dev callbacks.

If you think this is better, I'll send a v4 tomorrow.

Kevin

[1] NOTE: this is based on v6.12 because that's where I have a functioning BSP
for this SoC.  If you're OK with this, I'll rebase to v6.15 and submit upstream.