[PATCH RFC v3 0/2] pmdomain: core: add support for domain hierarchies in DT

[PATCH RFC v3 0/2] pmdomain: core: add support for domain hierarchies in DT

[Kevin Hilman]

Currently, PM domains can only support hierarchy for simple
providers (e.g. ones with #power-domain-cells = 0).

Add more generic support by adding support for a nexus node map, as
described in section 2.5.1 of the DT spec.

Signed-off-by: Kevin Hilman <khilman@baylibre.com>
---
Changes in v3:
- use of_parse_phandle_with_args_map() instead of custom parsing
- probe when device is attatched to PM domain
- Link to v2: https://lore.kernel.org/r/20250528-pmdomain-hierarchy-onecell-v2-0-7885ae45e59c@baylibre.com

Changes in v2:
- Use nexus map instead of creating new property as suggested by Rob H.
- Link to v1: https://lore.kernel.org/r/20250528-pmdomain-hierarchy-onecell-v1-1-851780700c68@baylibre.com

---
Kevin Hilman (2):
      dt-bindings: power: add nexus map for power-domains
      pmdomain: core: add support for subdomains using power-domain-map

 Documentation/devicetree/bindings/power/power-domain.yaml | 35 +++++++++++++++++++++++++++++++++++
 drivers/pmdomain/core.c                                   | 24 ++++++++++++++++++++++--
 2 files changed, 57 insertions(+), 2 deletions(-)
---
base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
change-id: 20250528-pmdomain-hierarchy-onecell-a46fad47d855

Best regards,
-- 
Kevin Hilman <khilman@baylibre.com>

[PATCH RFC v3 1/2] dt-bindings: power: add nexus map for power-domains

[Kevin Hilman]

Add support for nexus map `power-domain-map` to be able to support
hierarchical power domains for providers with #power-domain-cells > 0.

Suggested-by: Rob Herring <robh@kernel.org>
Signed-off-by: Kevin Hilman <khilman@baylibre.com>
---
 Documentation/devicetree/bindings/power/power-domain.yaml | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/power-domain.yaml b/Documentation/devicetree/bindings/power/power-domain.yaml
index 8fdb529d560b..9f099d326aee 100644
--- a/Documentation/devicetree/bindings/power/power-domain.yaml
+++ b/Documentation/devicetree/bindings/power/power-domain.yaml
@@ -68,6 +68,15 @@ properties:
       by the given provider should be subdomains of the domain specified
       by this binding.
 
+  power-domains-map:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      Nexus node mapping property that establishes parent-child relationships
+      for PM domains using the format defined in the Device Tree specification
+      section 2.5.1. Each map entry consists of child domain specifier,
+      parent phandle, and optional parent specifier arguments. This property
+      is only supported for onecell providers (#power-domain-cells = 1).
+
 required:
   - "#power-domain-cells"
 
@@ -133,3 +142,29 @@ examples:
             min-residency-us = <7000>;
         };
     };
+
+  - |
+    // Example using power-domains-map for Nexus mapping
+    main_pd: power-controller@12370000 {
+        compatible = "foo,power-controller";
+        reg = <0x12370000 0x1000>;
+        #power-domain-cells = <0>;
+    };
+
+    wkup_pd: power-controller@12380000 {
+        compatible = "foo,power-controller";
+        reg = <0x12380000 0x1000>;
+        #power-domain-cells = <0>;
+    };
+
+    scmi_pds protocol@11 {
+        compatible = "arm,scmi-power-domain";
+        reg = <0x11>;
+        #power-domain-cells = <1>;
+        power-domains-map = <15 &main_pd>,
+                            <19 &wkup_pd>;
+    };
+
+    // In this example using Nexus node mapping:
+    // - Child domain 15 (scmi_pds 15) becomes a subdomain of main_pd
+    // - Child domain 19 (scmi_pds 19) becomes a subdomain of wkup_pd

-- 
2.49.0

Re: [PATCH RFC v3 1/2] dt-bindings: power: add nexus map for power-domains

[Rob Herring (Arm)]

On Fri, 13 Jun 2025 15:39:27 -0700, Kevin Hilman wrote:
> Add support for nexus map `power-domain-map` to be able to support
> hierarchical power domains for providers with #power-domain-cells > 0.
> 
> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> ---
>  Documentation/devicetree/bindings/power/power-domain.yaml | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/power/power-domain.example.dts:136.18-19 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/power/power-domain.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1519: dt_binding_check] Error 2
make: *** [Makefile:248: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250613-pmdomain-hierarchy-onecell-v3-1-5c770676fce7@baylibre.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Kevin Hilman]

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@baylibre.com>
---
 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);
+	}
+
 	ret = genpd_set_required_opp(dev, index);
 	if (ret)
 		goto err;
@@ -3056,6 +3072,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 			dev_gpd_data(dev)->default_pstate = 0;
 		}
 
+		if (parent_pd)
+			pm_genpd_remove_subdomain(parent_pd, pd);
 		genpd_remove_device(pd, dev);
 		return -EPROBE_DEFER;
 	}
@@ -3063,6 +3081,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 	return 1;
 
 err:
+	if (parent_pd)
+		pm_genpd_remove_subdomain(parent_pd, pd);
 	genpd_remove_device(pd, dev);
 	return ret;
 }

-- 
2.49.0

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

[Ulf Hansson]

On Sat, 14 Jun 2025 at 00:39, Kevin Hilman <khilman@baylibre.com> 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@baylibre.com>
> ---
>  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).

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

> +
>         ret = genpd_set_required_opp(dev, index);
>         if (ret)
>                 goto err;
> @@ -3056,6 +3072,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>                         dev_gpd_data(dev)->default_pstate = 0;
>                 }
>
> +               if (parent_pd)
> +                       pm_genpd_remove_subdomain(parent_pd, pd);
>                 genpd_remove_device(pd, dev);
>                 return -EPROBE_DEFER;
>         }
> @@ -3063,6 +3081,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>         return 1;
>
>  err:
> +       if (parent_pd)
> +               pm_genpd_remove_subdomain(parent_pd, pd);
>         genpd_remove_device(pd, dev);
>         return ret;
>  }
>
> --
> 2.49.0
>

Kind regards
Uffe

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

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:
--text follows this line--
> On Sat, 14 Jun 2025 at 00:39, Kevin Hilman <khilman@baylibre.com> 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@baylibre.com>
>> ---
>>  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.

From 12a3e5669dc18f4a9fdf9f25398cba4245135a43 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@baylibre.com>
Date: Fri, 13 Jun 2025 13:49:45 -0700
Subject: [PATCH 2/3] pmdomain: core: add support for subdomains via
 power-domain-map

---
 drivers/pmdomain/core.c   | 60 +++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h | 11 +++++++
 2 files changed, 71 insertions(+)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 88819659df83..a0dc60d4160d 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -3100,6 +3100,66 @@ struct device *genpd_dev_pm_attach_by_name(struct device *dev, const char *name)
 	return genpd_dev_pm_attach_by_id(dev, index);
 }
 
+/**
+ * genpd_dev_pm_attach_subdomain - Associate a PM domain with its parent domain
+ * @domain: The PM domain to lookup whether it has any parent
+ * @dev: The device being attached to the PM domain.
+ *
+ * Check if @domain has a power-domain-map.  If present, use that map
+ * to determine the parent PM domain, and attach @domain as a
+ * subdomain to the parent PM domain.
+ *
+ * Intended to called from a PM domain provider's ->attach_dev()
+ * callback, where &gpd_list_lock will already be held by the genpd
+ * add_device() path.
+ */
+struct generic_pm_domain *
+genpd_dev_pm_attach_subdomain(struct generic_pm_domain *domain,
+			      struct device *dev)
+{
+	struct of_phandle_args parent_args;
+	struct generic_pm_domain *parent_pd = NULL;
+	int ret;
+
+	/*
+	 * 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", 0, &parent_args);
+	if (!ret && parent_args.np) {
+		parent_pd = genpd_get_from_provider(&parent_args);
+		of_node_put(parent_args.np);
+
+		ret = genpd_add_subdomain(parent_pd, domain);
+		if (!ret) {
+			dev_dbg(dev, "adding PM domain %s as subdomain of %s\n",
+				domain->name, parent_pd->name);
+			return parent_pd;
+		}
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_subdomain);
+
+/**
+ * genpd_dev_pm_detach_subdomain - Detatch a PM domain from its parent domain
+ * @domain: The PM subdomain to detach
+ * @parent: The parent PM domain
+ * @dev: The device being attached to the PM subdomain.
+ *
+ * Remove @domain from @parent.
+ * Intended to cleanup after genpd_dev_pm_attach_subdomain()
+ */
+int genpd_dev_pm_detach_subdomain(struct generic_pm_domain *domain,
+				  struct generic_pm_domain *parent,
+				  struct device *dev)
+{
+	return pm_genpd_remove_subdomain(parent, domain);
+}
+EXPORT_SYMBOL_GPL(genpd_dev_pm_detach_subdomain);
+
 static const struct of_device_id idle_state_match[] = {
 	{ .compatible = "domain-idle-state", },
 	{ }
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index cf4b11be3709..5d7eb3ae59dd 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -410,6 +410,11 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
 					 unsigned int index);
 struct device *genpd_dev_pm_attach_by_name(struct device *dev,
 					   const char *name);
+struct generic_pm_domain *genpd_dev_pm_attach_subdomain(struct generic_pm_domain *domain,
+							struct device *dev);
+int genpd_dev_pm_detach_subdomain(struct generic_pm_domain *domain,
+				  struct generic_pm_domain *parent,
+				  struct device *dev);
 #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
 static inline int of_genpd_add_provider_simple(struct device_node *np,
 					struct generic_pm_domain *genpd)
@@ -466,6 +471,12 @@ static inline struct device *genpd_dev_pm_attach_by_name(struct device *dev,
 	return NULL;
 }
 
+static inline
+struct generic_pm_domain *genpd_dev_pm_attach_subdomain(struct generic_pm_domain *domain,
+							struct device *dev)
+{
+	return NULL;
+}
 static inline
 struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
 {
-- 
2.49.0

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

[Ulf Hansson]

On Tue, 17 Jun 2025 at 02:50, Kevin Hilman <khilman@baylibre.com> wrote:
>
> Ulf Hansson <ulf.hansson@linaro.org> writes:
> --text follows this line--
> > On Sat, 14 Jun 2025 at 00:39, Kevin Hilman <khilman@baylibre.com> 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@baylibre.com>
> >> ---
> >>  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.

Even if this may work for your particular use case, in general it does not.

We simply can't defer to build the topology (parent/child-domains)
until there is a device getting attached to some part of it.

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

See above.

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

Okay.

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

No, not during ->attach|detach_dev(), but during ->probe() of the SCMI
PM domain, immediately after the genpd OF providers has been added.

See more comments below.

>
> 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.
>
> From 12a3e5669dc18f4a9fdf9f25398cba4245135a43 Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@baylibre.com>
> Date: Fri, 13 Jun 2025 13:49:45 -0700
> Subject: [PATCH 2/3] pmdomain: core: add support for subdomains via
>  power-domain-map
>
> ---
>  drivers/pmdomain/core.c   | 60 +++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h | 11 +++++++
>  2 files changed, 71 insertions(+)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 88819659df83..a0dc60d4160d 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -3100,6 +3100,66 @@ struct device *genpd_dev_pm_attach_by_name(struct device *dev, const char *name)
>         return genpd_dev_pm_attach_by_id(dev, index);
>  }
>
> +/**
> + * genpd_dev_pm_attach_subdomain - Associate a PM domain with its parent domain
> + * @domain: The PM domain to lookup whether it has any parent
> + * @dev: The device being attached to the PM domain.
> + *
> + * Check if @domain has a power-domain-map.  If present, use that map
> + * to determine the parent PM domain, and attach @domain as a
> + * subdomain to the parent PM domain.
> + *
> + * Intended to called from a PM domain provider's ->attach_dev()
> + * callback, where &gpd_list_lock will already be held by the genpd
> + * add_device() path.
> + */
> +struct generic_pm_domain *
> +genpd_dev_pm_attach_subdomain(struct generic_pm_domain *domain,
> +                             struct device *dev)

A couple of comments below:

*) I think the function-name should have a prefix "of_genpd_*, to be
consistent with other names. Maye "of_genpd_add_subdomain_by_map"
would be a better name?

*) We need to decide if we want to add one child-domain (subdomain)
per function call - or whether we should walk the entire nexus-map and
hook up all child-domains to its parent in one go. I tend to like the
second one better, but I'm not really sure what would work best here.

No matter what, I think the in-parameters to the function should be of
type "struct of_phandle_args * or maybe struct device_node *", similar
to how of_genpd_add_subdomain() works.

> +{
> +       struct of_phandle_args parent_args;
> +       struct generic_pm_domain *parent_pd = NULL;
> +       int ret;
> +
> +       /*
> +        * 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", 0, &parent_args);
> +       if (!ret && parent_args.np) {
> +               parent_pd = genpd_get_from_provider(&parent_args);
> +               of_node_put(parent_args.np);
> +
> +               ret = genpd_add_subdomain(parent_pd, domain);
> +               if (!ret) {
> +                       dev_dbg(dev, "adding PM domain %s as subdomain of %s\n",
> +                               domain->name, parent_pd->name);
> +                       return parent_pd;
> +               }
> +       }
> +
> +       return NULL;
> +}
> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_subdomain);
> +
> +/**
> + * genpd_dev_pm_detach_subdomain - Detatch a PM domain from its parent domain
> + * @domain: The PM subdomain to detach
> + * @parent: The parent PM domain
> + * @dev: The device being attached to the PM subdomain.
> + *
> + * Remove @domain from @parent.
> + * Intended to cleanup after genpd_dev_pm_attach_subdomain()
> + */
> +int genpd_dev_pm_detach_subdomain(struct generic_pm_domain *domain,
> +                                 struct generic_pm_domain *parent,
> +                                 struct device *dev)
> +{
> +       return pm_genpd_remove_subdomain(parent, domain);
> +}
> +EXPORT_SYMBOL_GPL(genpd_dev_pm_detach_subdomain);
> +
>  static const struct of_device_id idle_state_match[] = {
>         { .compatible = "domain-idle-state", },
>         { }
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index cf4b11be3709..5d7eb3ae59dd 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -410,6 +410,11 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
>                                          unsigned int index);
>  struct device *genpd_dev_pm_attach_by_name(struct device *dev,
>                                            const char *name);
> +struct generic_pm_domain *genpd_dev_pm_attach_subdomain(struct generic_pm_domain *domain,
> +                                                       struct device *dev);
> +int genpd_dev_pm_detach_subdomain(struct generic_pm_domain *domain,
> +                                 struct generic_pm_domain *parent,
> +                                 struct device *dev);
>  #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
>  static inline int of_genpd_add_provider_simple(struct device_node *np,
>                                         struct generic_pm_domain *genpd)
> @@ -466,6 +471,12 @@ static inline struct device *genpd_dev_pm_attach_by_name(struct device *dev,
>         return NULL;
>  }
>
> +static inline
> +struct generic_pm_domain *genpd_dev_pm_attach_subdomain(struct generic_pm_domain *domain,
> +                                                       struct device *dev)
> +{
> +       return NULL;
> +}
>  static inline
>  struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
>  {
> --
> 2.49.0
>
>

Kind regards
Uffe

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

[Kevin Hilman]

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

> On Tue, 17 Jun 2025 at 02:50, Kevin Hilman <khilman@baylibre.com> wrote:
>>
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>> --text follows this line--
>> > On Sat, 14 Jun 2025 at 00:39, Kevin Hilman <khilman@baylibre.com> 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@baylibre.com>
>> >> ---
>> >>  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.
>
> Even if this may work for your particular use case, in general it does not.
>
> We simply can't defer to build the topology (parent/child-domains)
> until there is a device getting attached to some part of it.

OK, not defering building topology makes a lot of sense.  Thanks for the
explanation.

>>
>> 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.)
>>
>
> See above.
>
>> > 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.
>
> Okay.
>
>>
>> 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.
>
> No, not during ->attach|detach_dev(), but during ->probe() of the SCMI
> PM domain, immediately after the genpd OF providers has been added.
>
> See more comments below.
>
>>
>> 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.
>>
>> From 12a3e5669dc18f4a9fdf9f25398cba4245135a43 Mon Sep 17 00:00:00 2001
>> From: Kevin Hilman <khilman@baylibre.com>
>> Date: Fri, 13 Jun 2025 13:49:45 -0700
>> Subject: [PATCH 2/3] pmdomain: core: add support for subdomains via
>>  power-domain-map
>>
>> ---
>>  drivers/pmdomain/core.c   | 60 +++++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h | 11 +++++++
>>  2 files changed, 71 insertions(+)
>>
>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
>> index 88819659df83..a0dc60d4160d 100644
>> --- a/drivers/pmdomain/core.c
>> +++ b/drivers/pmdomain/core.c
>> @@ -3100,6 +3100,66 @@ struct device *genpd_dev_pm_attach_by_name(struct device *dev, const char *name)
>>         return genpd_dev_pm_attach_by_id(dev, index);
>>  }
>>
>> +/**
>> + * genpd_dev_pm_attach_subdomain - Associate a PM domain with its parent domain
>> + * @domain: The PM domain to lookup whether it has any parent
>> + * @dev: The device being attached to the PM domain.
>> + *
>> + * Check if @domain has a power-domain-map.  If present, use that map
>> + * to determine the parent PM domain, and attach @domain as a
>> + * subdomain to the parent PM domain.
>> + *
>> + * Intended to called from a PM domain provider's ->attach_dev()
>> + * callback, where &gpd_list_lock will already be held by the genpd
>> + * add_device() path.
>> + */
>> +struct generic_pm_domain *
>> +genpd_dev_pm_attach_subdomain(struct generic_pm_domain *domain,
>> +                             struct device *dev)
>
> A couple of comments below:
>
> *) I think the function-name should have a prefix "of_genpd_*, to be
> consistent with other names. Maye "of_genpd_add_subdomain_by_map"
> would be a better name?
>
> *) We need to decide if we want to add one child-domain (subdomain)
> per function call - or whether we should walk the entire nexus-map and
> hook up all child-domains to its parent in one go. I tend to like the
> second one better, but I'm not really sure what would work best here.

I like the first one better since most other related functions work on
single domains, not groups.

> No matter what, I think the in-parameters to the function should be of
> type "struct of_phandle_args * or maybe struct device_node *", similar
> to how of_genpd_add_subdomain() works.

I've done an implementation with struct device_node *.  This works
better (IMO) than struct of_phandle_args * because the caller (in my
case scmi_pm_domain.c) already has device nodes, but not phandle args.

The result will be that the pmdomain helper will call
pm_genpd_add_subdomain() instead of of_genpd_add_subdomain().  

Below[1] is the current working version, which includes adding the
helper to the PM domain core and showing the usage by the SCMI provider.

How does this look?

Note that doing this at provider creation time instead of
<genpd>->attach_dev() time will require some changes to
of_parse_phandle_with_args_map() because that function expects to be
called for a device that has a `power-domains = <provider>` property,
not for the provider itself.  But I have it working with some local
changes to make that helper work if called for the provider directly.
If you're OK with the PM domains approach, I'll post another rev of this
series which includes the OF changes for review by DT maintainers.

Kevin

[1]
---
 drivers/pmdomain/arm/scmi_pm_domain.c | 12 ++++++++--
 drivers/pmdomain/core.c               | 34 +++++++++++++++++++++++++++
 include/linux/pm_domain.h             | 11 ++++++++-
 3 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
index a7784a8bb5db..8197447e9d17 100644
--- a/drivers/pmdomain/arm/scmi_pm_domain.c
+++ b/drivers/pmdomain/arm/scmi_pm_domain.c
@@ -54,7 +54,7 @@ static int scmi_pd_power_off(struct generic_pm_domain *domain)
 
 static int scmi_pm_domain_probe(struct scmi_device *sdev)
 {
-	int num_domains, i;
+	int num_domains, i, ret;
 	struct device *dev = &sdev->dev;
 	struct device_node *np = dev->of_node;
 	struct scmi_pm_domain *scmi_pd;
@@ -115,7 +115,15 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
 
 	dev_set_drvdata(dev, scmi_pd_data);
 
-	return of_genpd_add_provider_onecell(np, scmi_pd_data);
+	ret = of_genpd_add_provider_onecell(np, scmi_pd_data);
+	if (ret)
+		return ret;
+
+	/* check for (optional) subdomain mapping with power-domain-map */
+	for (i = 0; i < num_domains; i++, scmi_pd++)
+		of_genpd_add_subdomain_map(np, domains[i], i);
+
+	return ret;
 }
 
 static void scmi_pm_domain_remove(struct scmi_device *sdev)
diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 88819659df83..3ede4baa4bee 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -3220,6 +3220,40 @@ int of_genpd_parse_idle_states(struct device_node *dn,
 }
 EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
 
+int of_genpd_add_subdomain_map(struct device_node *np,
+			       struct generic_pm_domain *domain,
+			       int index)
+{
+	struct of_phandle_args parent_args;
+	struct generic_pm_domain *parent_pd;
+	struct device *dev = &domain->dev;
+	int ret;
+
+	if (!domain)
+		return -ENODEV;
+
+	/*
+	 * 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(np, NULL, "power-domain",
+					     index, &parent_args);
+	if (!ret && parent_args.np) {
+		parent_pd = genpd_get_from_provider(&parent_args);
+		of_node_put(parent_args.np);
+
+		if (IS_ERR(parent_pd))
+			return -EINVAL;
+
+		ret = pm_genpd_add_subdomain(parent_pd, domain);
+		if (!ret)
+			dev_dbg(dev, "adding PM domain %s as subdomain of %s\n",
+				domain->name, parent_pd->name);
+	}
+
+	return ret;
+}
+
 static int __init genpd_bus_init(void)
 {
 	return bus_register(&genpd_bus_type);
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index cf4b11be3709..65d459d703bb 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -402,9 +402,11 @@ int of_genpd_add_subdomain(const struct of_phandle_args *parent_spec,
 int of_genpd_remove_subdomain(const struct of_phandle_args *parent_spec,
 			      const struct of_phandle_args *subdomain_spec);
 struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
+int of_genpd_add_subdomain_map(struct device_node *np,
+			       struct generic_pm_domain *genpd,
+			       int index);
 int of_genpd_parse_idle_states(struct device_node *dn,
 			       struct genpd_power_state **states, int *n);
-
 int genpd_dev_pm_attach(struct device *dev);
 struct device *genpd_dev_pm_attach_by_id(struct device *dev,
 					 unsigned int index);
@@ -443,6 +445,13 @@ static inline int of_genpd_remove_subdomain(const struct of_phandle_args *parent
 	return -ENODEV;
 }
 
+static int of_genpd_add_subdomain_map(struct device_node *np,
+				      struct generic_pm_domain *genpd,
+				      int index)
+{
+	return -ENODEV;
+}
+
 static inline int of_genpd_parse_idle_states(struct device_node *dn,
 			struct genpd_power_state **states, int *n)
 {
-- 
2.49.0

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

[Dan Carpenter]

On Wed, Jun 18, 2025 at 10:48:09AM -0700, Kevin Hilman wrote:
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 88819659df83..3ede4baa4bee 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -3220,6 +3220,40 @@ int of_genpd_parse_idle_states(struct device_node *dn,
>  }
>  EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>  
> +int of_genpd_add_subdomain_map(struct device_node *np,
> +			       struct generic_pm_domain *domain,
> +			       int index)
> +{
> +	struct of_phandle_args parent_args;
> +	struct generic_pm_domain *parent_pd;
> +	struct device *dev = &domain->dev;
> +	int ret;
> +
> +	if (!domain)
> +		return -ENODEV;
> +
> +	/*
> +	 * 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(np, NULL, "power-domain",
> +					     index, &parent_args);
> +	if (!ret && parent_args.np) {

Sorry for the pedanticry but could we flip this around?

	if (ret)
		return ret;

	if (!parent_args.np)
		return 0;

> +		parent_pd = genpd_get_from_provider(&parent_args);
> +		of_node_put(parent_args.np);
> +
> +		if (IS_ERR(parent_pd))
> +			return -EINVAL;

		return PTR_ERR(parent_pd);

> +
> +		ret = pm_genpd_add_subdomain(parent_pd, domain);
> +		if (!ret)

	if (ret)
		return ret;

	dev_dbg(dev, "adding PM domain %s as subdomain of %s\n",
		domain->name, parent_pd->name);

	return 0;

> +			dev_dbg(dev, "adding PM domain %s as subdomain of %s\n",
> +				domain->name, parent_pd->name);
> +	}
> +
> +	return ret;
> +}
> +

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

[Kevin Hilman]

Hi Dan,

Dan Carpenter <dan.carpenter@linaro.org> writes:

> On Wed, Jun 18, 2025 at 10:48:09AM -0700, Kevin Hilman wrote:
>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
>> index 88819659df83..3ede4baa4bee 100644
>> --- a/drivers/pmdomain/core.c
>> +++ b/drivers/pmdomain/core.c
>> @@ -3220,6 +3220,40 @@ int of_genpd_parse_idle_states(struct device_node *dn,
>>  }
>>  EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>>  
>> +int of_genpd_add_subdomain_map(struct device_node *np,
>> +			       struct generic_pm_domain *domain,
>> +			       int index)
>> +{
>> +	struct of_phandle_args parent_args;
>> +	struct generic_pm_domain *parent_pd;
>> +	struct device *dev = &domain->dev;
>> +	int ret;
>> +
>> +	if (!domain)
>> +		return -ENODEV;
>> +
>> +	/*
>> +	 * 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(np, NULL, "power-domain",
>> +					     index, &parent_args);
>> +	if (!ret && parent_args.np) {
>
> Sorry for the pedanticry but could we flip this around?

Sure.  This is early prototype code for discsussing the big-picture
approach, but I appreciate the review.  I'll clean that up when I get
past the RFC phase.

Thanks!

Kevin

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

[Ulf Hansson]

[...]

> I've done an implementation with struct device_node *.  This works
> better (IMO) than struct of_phandle_args * because the caller (in my
> case scmi_pm_domain.c) already has device nodes, but not phandle args.
>
> The result will be that the pmdomain helper will call
> pm_genpd_add_subdomain() instead of of_genpd_add_subdomain().
>
> Below[1] is the current working version, which includes adding the
> helper to the PM domain core and showing the usage by the SCMI provider.
>
> How does this look?

It's a lot better in my opinion. Although, I have a few comments below.

>
> Note that doing this at provider creation time instead of
> <genpd>->attach_dev() time will require some changes to
> of_parse_phandle_with_args_map() because that function expects to be
> called for a device that has a `power-domains = <provider>` property,
> not for the provider itself.  But I have it working with some local
> changes to make that helper work if called for the provider directly.
> If you're OK with the PM domains approach, I'll post another rev of this
> series which includes the OF changes for review by DT maintainers.
>
> Kevin
>
> [1]
> ---
>  drivers/pmdomain/arm/scmi_pm_domain.c | 12 ++++++++--
>  drivers/pmdomain/core.c               | 34 +++++++++++++++++++++++++++
>  include/linux/pm_domain.h             | 11 ++++++++-
>  3 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
> index a7784a8bb5db..8197447e9d17 100644
> --- a/drivers/pmdomain/arm/scmi_pm_domain.c
> +++ b/drivers/pmdomain/arm/scmi_pm_domain.c
> @@ -54,7 +54,7 @@ static int scmi_pd_power_off(struct generic_pm_domain *domain)
>
>  static int scmi_pm_domain_probe(struct scmi_device *sdev)
>  {
> -       int num_domains, i;
> +       int num_domains, i, ret;
>         struct device *dev = &sdev->dev;
>         struct device_node *np = dev->of_node;
>         struct scmi_pm_domain *scmi_pd;
> @@ -115,7 +115,15 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
>
>         dev_set_drvdata(dev, scmi_pd_data);
>
> -       return of_genpd_add_provider_onecell(np, scmi_pd_data);
> +       ret = of_genpd_add_provider_onecell(np, scmi_pd_data);
> +       if (ret)
> +               return ret;
> +
> +       /* check for (optional) subdomain mapping with power-domain-map */
> +       for (i = 0; i < num_domains; i++, scmi_pd++)
> +               of_genpd_add_subdomain_map(np, domains[i], i);
> +
> +       return ret;
>  }
>
>  static void scmi_pm_domain_remove(struct scmi_device *sdev)
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 88819659df83..3ede4baa4bee 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -3220,6 +3220,40 @@ int of_genpd_parse_idle_states(struct device_node *dn,
>  }
>  EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>
> +int of_genpd_add_subdomain_map(struct device_node *np,
> +                              struct generic_pm_domain *domain,
> +                              int index)

Providing the struct generic_pm_domain *domain as an in-parameter for
the child-domain seems unnecessary and limiting to me.

Instead I think we should parse the power-domain-map DT property at
'index', to find the corresponding child-domain's specifier/index and
its corresponding parent-domain.

In other words, we don't need the struct generic_pm_domain *domain as
an in-parameter, right?

> +{
> +       struct of_phandle_args parent_args;
> +       struct generic_pm_domain *parent_pd;
> +       struct device *dev = &domain->dev;
> +       int ret;
> +
> +       if (!domain)
> +               return -ENODEV;
> +
> +       /*
> +        * 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(np, NULL, "power-domain",
> +                                            index, &parent_args);
> +       if (!ret && parent_args.np) {
> +               parent_pd = genpd_get_from_provider(&parent_args);
> +               of_node_put(parent_args.np);
> +
> +               if (IS_ERR(parent_pd))
> +                       return -EINVAL;
> +
> +               ret = pm_genpd_add_subdomain(parent_pd, domain);
> +               if (!ret)
> +                       dev_dbg(dev, "adding PM domain %s as subdomain of %s\n",
> +                               domain->name, parent_pd->name);
> +       }
> +
> +       return ret;
> +}
> +
>  static int __init genpd_bus_init(void)
>  {
>         return bus_register(&genpd_bus_type);
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index cf4b11be3709..65d459d703bb 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -402,9 +402,11 @@ int of_genpd_add_subdomain(const struct of_phandle_args *parent_spec,
>  int of_genpd_remove_subdomain(const struct of_phandle_args *parent_spec,
>                               const struct of_phandle_args *subdomain_spec);
>  struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
> +int of_genpd_add_subdomain_map(struct device_node *np,
> +                              struct generic_pm_domain *genpd,
> +                              int index);
>  int of_genpd_parse_idle_states(struct device_node *dn,
>                                struct genpd_power_state **states, int *n);
> -
>  int genpd_dev_pm_attach(struct device *dev);
>  struct device *genpd_dev_pm_attach_by_id(struct device *dev,
>                                          unsigned int index);
> @@ -443,6 +445,13 @@ static inline int of_genpd_remove_subdomain(const struct of_phandle_args *parent
>         return -ENODEV;
>  }
>
> +static int of_genpd_add_subdomain_map(struct device_node *np,
> +                                     struct generic_pm_domain *genpd,
> +                                     int index)
> +{
> +       return -ENODEV;
> +}
> +
>  static inline int of_genpd_parse_idle_states(struct device_node *dn,
>                         struct genpd_power_state **states, int *n)
>  {
> --
> 2.49.0
>
>

Kind regards
Uffe

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

[Ulf Hansson]

On Thu, 19 Jun 2025 at 12:04, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> [...]
>
> > I've done an implementation with struct device_node *.  This works
> > better (IMO) than struct of_phandle_args * because the caller (in my
> > case scmi_pm_domain.c) already has device nodes, but not phandle args.
> >
> > The result will be that the pmdomain helper will call
> > pm_genpd_add_subdomain() instead of of_genpd_add_subdomain().
> >
> > Below[1] is the current working version, which includes adding the
> > helper to the PM domain core and showing the usage by the SCMI provider.
> >
> > How does this look?
>
> It's a lot better in my opinion. Although, I have a few comments below.
>
> >
> > Note that doing this at provider creation time instead of
> > <genpd>->attach_dev() time will require some changes to
> > of_parse_phandle_with_args_map() because that function expects to be
> > called for a device that has a `power-domains = <provider>` property,
> > not for the provider itself.  But I have it working with some local
> > changes to make that helper work if called for the provider directly.
> > If you're OK with the PM domains approach, I'll post another rev of this
> > series which includes the OF changes for review by DT maintainers.
> >
> > Kevin
> >
> > [1]
> > ---
> >  drivers/pmdomain/arm/scmi_pm_domain.c | 12 ++++++++--
> >  drivers/pmdomain/core.c               | 34 +++++++++++++++++++++++++++
> >  include/linux/pm_domain.h             | 11 ++++++++-
> >  3 files changed, 54 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
> > index a7784a8bb5db..8197447e9d17 100644
> > --- a/drivers/pmdomain/arm/scmi_pm_domain.c
> > +++ b/drivers/pmdomain/arm/scmi_pm_domain.c
> > @@ -54,7 +54,7 @@ static int scmi_pd_power_off(struct generic_pm_domain *domain)
> >
> >  static int scmi_pm_domain_probe(struct scmi_device *sdev)
> >  {
> > -       int num_domains, i;
> > +       int num_domains, i, ret;
> >         struct device *dev = &sdev->dev;
> >         struct device_node *np = dev->of_node;
> >         struct scmi_pm_domain *scmi_pd;
> > @@ -115,7 +115,15 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
> >
> >         dev_set_drvdata(dev, scmi_pd_data);
> >
> > -       return of_genpd_add_provider_onecell(np, scmi_pd_data);
> > +       ret = of_genpd_add_provider_onecell(np, scmi_pd_data);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* check for (optional) subdomain mapping with power-domain-map */
> > +       for (i = 0; i < num_domains; i++, scmi_pd++)
> > +               of_genpd_add_subdomain_map(np, domains[i], i);
> > +
> > +       return ret;
> >  }
> >
> >  static void scmi_pm_domain_remove(struct scmi_device *sdev)
> > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > index 88819659df83..3ede4baa4bee 100644
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -3220,6 +3220,40 @@ int of_genpd_parse_idle_states(struct device_node *dn,
> >  }
> >  EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
> >
> > +int of_genpd_add_subdomain_map(struct device_node *np,
> > +                              struct generic_pm_domain *domain,
> > +                              int index)
>
> Providing the struct generic_pm_domain *domain as an in-parameter for
> the child-domain seems unnecessary and limiting to me.
>
> Instead I think we should parse the power-domain-map DT property at
> 'index', to find the corresponding child-domain's specifier/index and
> its corresponding parent-domain.
>
> In other words, we don't need the struct generic_pm_domain *domain as
> an in-parameter, right?

Having said that, why not skip the index as the in-parameter too and
just walk the list of the power-domain-map DT property.

In this way, there is no pre-parsing needed for the genpd provider
driver - or need to try all child-domain indexes (not all may have a
parent-domain associated with it).

[...]

Kind regards
Uffe

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

[Kevin Hilman]

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

> [...]
>
>> I've done an implementation with struct device_node *.  This works
>> better (IMO) than struct of_phandle_args * because the caller (in my
>> case scmi_pm_domain.c) already has device nodes, but not phandle args.
>>
>> The result will be that the pmdomain helper will call
>> pm_genpd_add_subdomain() instead of of_genpd_add_subdomain().
>>
>> Below[1] is the current working version, which includes adding the
>> helper to the PM domain core and showing the usage by the SCMI provider.
>>
>> How does this look?
>
> It's a lot better in my opinion. Although, I have a few comments below.
>
>>
>> Note that doing this at provider creation time instead of
>> <genpd>->attach_dev() time will require some changes to
>> of_parse_phandle_with_args_map() because that function expects to be
>> called for a device that has a `power-domains = <provider>` property,
>> not for the provider itself.  But I have it working with some local
>> changes to make that helper work if called for the provider directly.
>> If you're OK with the PM domains approach, I'll post another rev of this
>> series which includes the OF changes for review by DT maintainers.
>>
>> Kevin
>>
>> [1]
>> ---
>>  drivers/pmdomain/arm/scmi_pm_domain.c | 12 ++++++++--
>>  drivers/pmdomain/core.c               | 34 +++++++++++++++++++++++++++
>>  include/linux/pm_domain.h             | 11 ++++++++-
>>  3 files changed, 54 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
>> index a7784a8bb5db..8197447e9d17 100644
>> --- a/drivers/pmdomain/arm/scmi_pm_domain.c
>> +++ b/drivers/pmdomain/arm/scmi_pm_domain.c
>> @@ -54,7 +54,7 @@ static int scmi_pd_power_off(struct generic_pm_domain *domain)
>>
>>  static int scmi_pm_domain_probe(struct scmi_device *sdev)
>>  {
>> -       int num_domains, i;
>> +       int num_domains, i, ret;
>>         struct device *dev = &sdev->dev;
>>         struct device_node *np = dev->of_node;
>>         struct scmi_pm_domain *scmi_pd;
>> @@ -115,7 +115,15 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
>>
>>         dev_set_drvdata(dev, scmi_pd_data);
>>
>> -       return of_genpd_add_provider_onecell(np, scmi_pd_data);
>> +       ret = of_genpd_add_provider_onecell(np, scmi_pd_data);
>> +       if (ret)
>> +               return ret;
>> +
>> +       /* check for (optional) subdomain mapping with power-domain-map */
>> +       for (i = 0; i < num_domains; i++, scmi_pd++)
>> +               of_genpd_add_subdomain_map(np, domains[i], i);
>> +
>> +       return ret;
>>  }
>>
>>  static void scmi_pm_domain_remove(struct scmi_device *sdev)
>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
>> index 88819659df83..3ede4baa4bee 100644
>> --- a/drivers/pmdomain/core.c
>> +++ b/drivers/pmdomain/core.c
>> @@ -3220,6 +3220,40 @@ int of_genpd_parse_idle_states(struct device_node *dn,
>>  }
>>  EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>>
>> +int of_genpd_add_subdomain_map(struct device_node *np,
>> +                              struct generic_pm_domain *domain,
>> +                              int index)
>
> Providing the struct generic_pm_domain *domain as an in-parameter for
> the child-domain seems unnecessary and limiting to me.
>
> Instead I think we should parse the power-domain-map DT property at
> 'index', to find the corresponding child-domain's specifier/index and
> its corresponding parent-domain.
>
> In other words, we don't need the struct generic_pm_domain *domain as
> an in-parameter, right?

I'm not sure I follow.  The `struct generic pm_domain *domain` is the
SCMI child domain.  From the map, we use the index to find the parent
domain.  And then we add the child as a subdomain of the parent.

Are you suggesting that I (re)parse the DT for to find the child domain
also? 

Thanks for the review & guidance,

Kevin

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

[Ulf Hansson]

On Mon, 30 Jun 2025 at 20:17, Kevin Hilman <khilman@baylibre.com> wrote:
>
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
> > [...]
> >
> >> I've done an implementation with struct device_node *.  This works
> >> better (IMO) than struct of_phandle_args * because the caller (in my
> >> case scmi_pm_domain.c) already has device nodes, but not phandle args.
> >>
> >> The result will be that the pmdomain helper will call
> >> pm_genpd_add_subdomain() instead of of_genpd_add_subdomain().
> >>
> >> Below[1] is the current working version, which includes adding the
> >> helper to the PM domain core and showing the usage by the SCMI provider.
> >>
> >> How does this look?
> >
> > It's a lot better in my opinion. Although, I have a few comments below.
> >
> >>
> >> Note that doing this at provider creation time instead of
> >> <genpd>->attach_dev() time will require some changes to
> >> of_parse_phandle_with_args_map() because that function expects to be
> >> called for a device that has a `power-domains = <provider>` property,
> >> not for the provider itself.  But I have it working with some local
> >> changes to make that helper work if called for the provider directly.
> >> If you're OK with the PM domains approach, I'll post another rev of this
> >> series which includes the OF changes for review by DT maintainers.
> >>
> >> Kevin
> >>
> >> [1]
> >> ---
> >>  drivers/pmdomain/arm/scmi_pm_domain.c | 12 ++++++++--
> >>  drivers/pmdomain/core.c               | 34 +++++++++++++++++++++++++++
> >>  include/linux/pm_domain.h             | 11 ++++++++-
> >>  3 files changed, 54 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
> >> index a7784a8bb5db..8197447e9d17 100644
> >> --- a/drivers/pmdomain/arm/scmi_pm_domain.c
> >> +++ b/drivers/pmdomain/arm/scmi_pm_domain.c
> >> @@ -54,7 +54,7 @@ static int scmi_pd_power_off(struct generic_pm_domain *domain)
> >>
> >>  static int scmi_pm_domain_probe(struct scmi_device *sdev)
> >>  {
> >> -       int num_domains, i;
> >> +       int num_domains, i, ret;
> >>         struct device *dev = &sdev->dev;
> >>         struct device_node *np = dev->of_node;
> >>         struct scmi_pm_domain *scmi_pd;
> >> @@ -115,7 +115,15 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
> >>
> >>         dev_set_drvdata(dev, scmi_pd_data);
> >>
> >> -       return of_genpd_add_provider_onecell(np, scmi_pd_data);
> >> +       ret = of_genpd_add_provider_onecell(np, scmi_pd_data);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       /* check for (optional) subdomain mapping with power-domain-map */
> >> +       for (i = 0; i < num_domains; i++, scmi_pd++)
> >> +               of_genpd_add_subdomain_map(np, domains[i], i);
> >> +
> >> +       return ret;
> >>  }
> >>
> >>  static void scmi_pm_domain_remove(struct scmi_device *sdev)
> >> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> >> index 88819659df83..3ede4baa4bee 100644
> >> --- a/drivers/pmdomain/core.c
> >> +++ b/drivers/pmdomain/core.c
> >> @@ -3220,6 +3220,40 @@ int of_genpd_parse_idle_states(struct device_node *dn,
> >>  }
> >>  EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
> >>
> >> +int of_genpd_add_subdomain_map(struct device_node *np,
> >> +                              struct generic_pm_domain *domain,
> >> +                              int index)
> >
> > Providing the struct generic_pm_domain *domain as an in-parameter for
> > the child-domain seems unnecessary and limiting to me.
> >
> > Instead I think we should parse the power-domain-map DT property at
> > 'index', to find the corresponding child-domain's specifier/index and
> > its corresponding parent-domain.
> >
> > In other words, we don't need the struct generic_pm_domain *domain as
> > an in-parameter, right?
>
> I'm not sure I follow.  The `struct generic pm_domain *domain` is the
> SCMI child domain.  From the map, we use the index to find the parent
> domain.  And then we add the child as a subdomain of the parent.
>
> Are you suggesting that I (re)parse the DT for to find the child domain
> also?

Correct!

The DT property ("power-domains-map") that you added in patch1/2,
contains all the information so let's just parse it and assign
child/parent domains based on it.

Kind regards
Uffe