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

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

[Kevin Hilman (TI.com)]

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

Add more generic support by creating an of_genpd helper which can
parse a nexus node map, and create domain hierarchy.

described in section 2.5.1 of the DT spec.

Signed-off-by: Kevin Hilman <khilman@baylibre.com>
---
Changes in v4:
- use new OF iterator for parsing map. See:
  https://lore.kernel.org/r/20251119-topic-lpm-of-map-iterator-v6-18-v1-1-1f0075d771a3@baylibre.com
- instead of mapping on probe, create of_genpd helper to be called by providers
- Link to v3: https://lore.kernel.org/r/20250613-pmdomain-hierarchy-onecell-v3-0-5c770676fce7@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 (TI.com) (2):
      pmdomain: core: support domain hierarchy via power-domain-map
      pmdomain: arm_scmi: add support for domain hierarchies

 drivers/pmdomain/arm/scmi_pm_domain.c | 11 ++++--
 drivers/pmdomain/core.c               | 64 ++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h             |  9 +++++
 3 files changed, 82 insertions(+), 2 deletions(-)
---
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
change-id: 20250528-pmdomain-hierarchy-onecell-a46fad47d855
prerequisite-change-id: 20251119-topic-lpm-of-map-iterator-v6-18-a61447423adc:v1
prerequisite-patch-id: e2c4a8c727d0f172166cfa622e60d97048a97b26

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

[PATCH RFC v4 1/2] pmdomain: core: support domain hierarchy via power-domain-map

[Kevin Hilman (TI.com)]

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.

Signed-off-by: Kevin Hilman (TI.com) <khilman@baylibre.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);
 
+int of_genpd_add_subdomain_map(struct device_node *np,
+			       struct genpd_onecell_data *data)
+{
+	struct generic_pm_domain *genpd, *parent_genpd;
+	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) {
+			of_node_put(parent_args.np);
+			ret = -EINVAL;
+			break;
+		}
+		child_index = child_args.args[0];
+
+		/* Validate child domain index */
+		if (child_index >= data->num_domains) {
+			of_node_put(parent_args.np);
+			continue;
+		}
+
+		genpd = data->domains[child_index];
+		if (!genpd) {
+			of_node_put(parent_args.np);
+			continue;
+		}
+
+		/* 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);
+			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;
+}
+
 /**
  * of_genpd_sync_state() - A common sync_state function for genpd providers
  * @np: The device node the genpd provider is associated with.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index f67a2cb7d781..3585acb89b83 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -462,6 +462,8 @@ 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 genpd_onecell_data *data);
 int of_genpd_parse_idle_states(struct device_node *dn,
 			       struct genpd_power_state **states, int *n);
 void of_genpd_sync_state(struct device_node *np);
@@ -504,6 +506,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.51.0

[PATCH RFC v4 2/2] pmdomain: arm_scmi: add support for domain hierarchies

[Kevin Hilman (TI.com)]

After primary SCMI pmdomain is created, use new of_genpd helper to
check subdomain mappings, and create domain hierarchy.

Signed-off-by: Kevin Hilman (TI.com) <khilman@baylibre.com>
---
 drivers/pmdomain/arm/scmi_pm_domain.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
index 8fe1c0a501c9..a36bb50c7cf6 100644
--- a/drivers/pmdomain/arm/scmi_pm_domain.c
+++ b/drivers/pmdomain/arm/scmi_pm_domain.c
@@ -41,7 +41,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;
@@ -110,7 +110,14 @@ 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 */
+	of_genpd_add_subdomain_map(np, scmi_pd_data);
+
+	return ret;
 }
 
 static void scmi_pm_domain_remove(struct scmi_device *sdev)

-- 
2.51.0

Re: [PATCH RFC v4 2/2] pmdomain: arm_scmi: add support for domain hierarchies

[Dan Carpenter]

On Wed, Nov 19, 2025 at 04:58:46PM -0800, Kevin Hilman (TI.com) wrote:
> @@ -110,7 +110,14 @@ 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 */
> +	of_genpd_add_subdomain_map(np, scmi_pd_data);
> +
> +	return ret;

return 0, plz.

regards,
dan carpenter

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

[Ulf Hansson]

On Thu, 20 Nov 2025 at 01:58, Kevin Hilman (TI.com)
<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 by creating an of_genpd helper which can
> parse a nexus node map, and create domain hierarchy.
>
> described in section 2.5.1 of the DT spec.

Even if that is generally described, shouldn't we update the generic
DT doc for power-domains [1] to mention this too?

At least we should show some examples of how this can be used.

>
> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> ---
> Changes in v4:
> - use new OF iterator for parsing map. See:
>   https://lore.kernel.org/r/20251119-topic-lpm-of-map-iterator-v6-18-v1-1-1f0075d771a3@baylibre.com
> - instead of mapping on probe, create of_genpd helper to be called by providers
> - Link to v3: https://lore.kernel.org/r/20250613-pmdomain-hierarchy-onecell-v3-0-5c770676fce7@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 (TI.com) (2):
>       pmdomain: core: support domain hierarchy via power-domain-map
>       pmdomain: arm_scmi: add support for domain hierarchies
>
>  drivers/pmdomain/arm/scmi_pm_domain.c | 11 ++++--
>  drivers/pmdomain/core.c               | 64 ++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h             |  9 +++++
>  3 files changed, 82 insertions(+), 2 deletions(-)
> ---
> base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
> change-id: 20250528-pmdomain-hierarchy-onecell-a46fad47d855
> prerequisite-change-id: 20251119-topic-lpm-of-map-iterator-v6-18-a61447423adc:v1
> prerequisite-patch-id: e2c4a8c727d0f172166cfa622e60d97048a97b26
>
> Best regards,
> --
> Kevin Hilman (TI.com) <khilman@baylibre.com>
>

Thanks for continuing to work on this! I will look into the series asap.

Kind regards
Uffe

[1]
Documentation/devicetree/bindings/power/power-domain.yaml

Re: [PATCH RFC v4 1/2] pmdomain: core: support domain hierarchy via power-domain-map

[Ulf Hansson]

On Thu, 20 Nov 2025 at 01:58, Kevin Hilman (TI.com)
<khilman@baylibre.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?

>
> Signed-off-by: Kevin Hilman (TI.com) <khilman@baylibre.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.

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

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

> +       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?

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

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

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

Ditto.

> +               }
> +
> +               /* 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.

> +                       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?

> +
>  /**
>   * of_genpd_sync_state() - A common sync_state function for genpd providers
>   * @np: The device node the genpd provider is associated with.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index f67a2cb7d781..3585acb89b83 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -462,6 +462,8 @@ 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 genpd_onecell_data *data);
>  int of_genpd_parse_idle_states(struct device_node *dn,
>                                struct genpd_power_state **states, int *n);
>  void of_genpd_sync_state(struct device_node *np);
> @@ -504,6 +506,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.51.0
>

Kind regards
Uffe

Re: [PATCH RFC v4 2/2] pmdomain: arm_scmi: add support for domain hierarchies

[Ulf Hansson]

On Thu, 20 Nov 2025 at 01:58, Kevin Hilman (TI.com)
<khilman@baylibre.com> wrote:
>
> After primary SCMI pmdomain is created, use new of_genpd helper to
> check subdomain mappings, and create domain hierarchy.
>
> Signed-off-by: Kevin Hilman (TI.com) <khilman@baylibre.com>
> ---
>  drivers/pmdomain/arm/scmi_pm_domain.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
> index 8fe1c0a501c9..a36bb50c7cf6 100644
> --- a/drivers/pmdomain/arm/scmi_pm_domain.c
> +++ b/drivers/pmdomain/arm/scmi_pm_domain.c
> @@ -41,7 +41,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;
> @@ -110,7 +110,14 @@ 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 */
> +       of_genpd_add_subdomain_map(np, scmi_pd_data);

I think we need to take better care of dealing with errors here.
Typically a negative return value should be treated as an error, while
0 should be fine, right?

> +
> +       return ret;
>  }
>
>  static void scmi_pm_domain_remove(struct scmi_device *sdev)
>
> --
> 2.51.0
>

Moreover, I wonder if we need to update the scmi-DT doc [1] too?

Kind regards
Uffe

[1]
Documentation/devicetree/bindings/firmware/arm,scmi.yaml

Re: [PATCH RFC v4 1/2] pmdomain: core: support domain hierarchy via power-domain-map

[Kevin Hilman]

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

> On Thu, 20 Nov 2025 at 01:58, Kevin Hilman (TI.com)
> <khilman@baylibre.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@baylibre.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