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

[Ulf Hansson]

On Sat, 11 Apr 2026 at 01:44, Kevin Hilman (TI) <khilman@xxxxxxxxxxxx> wrote:
>
> Currently, PM domains can only support hierarchy for simple
> providers (e.g. ones with #power-domain-cells = 0).
>
> Add support for oncell providers as well by adding a new property
> `power-domains-child-ids` to describe the parent/child relationship.
>
> For example, an SCMI PM domain provider has multiple domains, each of
> which might be a child of diffeent parent domains. In this example,
> the parent domains are MAIN_PD and WKUP_PD:
>
>     scmi_pds: protocol@11 {
>         reg = <0x11>;
>         #power-domain-cells = <1>;
>         power-domains = <&MAIN_PD>, <&WKUP_PD>;
>         power-domains-child-ids = <15>, <19>;
>     };
>
> With this example using the new property, SCMI PM domain 15 becomes a
> child domain of MAIN_PD, and SCMI domain 19 becomes a child domain of
> WKUP_PD.
>
> To support this feature, add two new core functions
>
> - of_genpd_add_child_ids()
> - of_genpd_remove_child_ids()
>
> which can be called by pmdomain providers to add/remove child domains
> if they support the new property power-domains-child-ids.
>
> The add function is "all or nothing".  If it cannot add all of the
> child domains in the list, it will unwind any additions already made
> and report a failure.
>
> Signed-off-by: Kevin Hilman (TI) <khilman@xxxxxxxxxxxx>
> ---
>  drivers/pmdomain/core.c   | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h |  16 ++++++++++++++++
>  2 files changed, 182 insertions(+)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 61c2277c9ce3..f978477dd546 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -2909,6 +2909,172 @@ static struct generic_pm_domain *genpd_get_from_provider(
>         return genpd;
>  }
>
> +/**
> + * of_genpd_add_child_ids() - Parse power-domains-child-ids property
> + * @np: Device node pointer associated with the PM domain provider.
> + * @data: Pointer to the onecell data associated with the PM domain provider.
> + *
> + * Parse the power-domains and power-domains-child-ids properties to establish
> + * parent-child relationships for PM domains. The power-domains property lists
> + * parent domains, and power-domains-child-ids lists which child domain IDs
> + * should be associated with each parent.
> + *
> + * Uses "all or nothing" semantics: either all relationships are established
> + * successfully, or none are (any partially-added relationships are unwound
> + * on error).
> + *
> + * Returns 0 on success, -ENOENT if properties don't exist, or negative error code.
> + */

As I mentioned in my earlier reply for the previous version, returning
a specific error code when the property doesn't exist will complicate
handling for the caller. Moreover, we also need to make sure we don't
returning the same error code (-ENOENT) for a different error further
down the execution path in of_genpd_add_child_ids(). Otherwise it
would the caller treat the error code in the wrong way.

To me, there are two better ways to address this. For both options,
of_genpd_add_child_ids() should return 0 when
"power-domains-child-ids" is missing.

1) Add another helper function that checks if
"power-domains-child-ids" exists. The caller can then use this to
pre-parse the property and decide whether to treat it as an error.

2) As I suggested earlier, let of_genpd_add_child_ids() return the
number of assigned parents/children, while still using the all or
nothing approach, of course.

Kind regards
Uffe

> +int of_genpd_add_child_ids(struct device_node *np,
> +                          struct genpd_onecell_data *data)
> +{
> +       struct of_phandle_args parent_args;
> +       struct generic_pm_domain *parent_genpd, *child_genpd;
> +       struct generic_pm_domain **pairs; /* pairs[2*i]=parent, pairs[2*i+1]=child */
> +       u32 child_id;
> +       int i, ret, count, child_count, added = 0;
> +
> +       /* Check if both properties exist */
> +       count = of_count_phandle_with_args(np, "power-domains", "#power-domain-cells");
> +       if (count <= 0)
> +               return -ENOENT;
> +
> +       child_count = of_property_count_u32_elems(np, "power-domains-child-ids");
> +       if (child_count < 0)
> +               return -ENOENT;
> +       if (child_count != count)
> +               return -EINVAL;
> +
> +       /* Allocate tracking array for error unwind (parent/child pairs) */
> +       pairs = kmalloc_array(count * 2, sizeof(*pairs), GFP_KERNEL);
> +       if (!pairs)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < count; i++) {
> +               ret = of_property_read_u32_index(np, "power-domains-child-ids",
> +                                                i, &child_id);
> +               if (ret)
> +                       goto err_unwind;
> +
> +               /* Validate child ID is within bounds */
> +               if (child_id >= data->num_domains) {
> +                       pr_err("Child ID %u out of bounds (max %u) for %pOF\n",
> +                              child_id, data->num_domains - 1, np);
> +                       ret = -EINVAL;
> +                       goto err_unwind;
> +               }
> +
> +               /* Get the child domain */
> +               child_genpd = data->domains[child_id];
> +               if (!child_genpd) {
> +                       pr_err("Child domain %u is NULL for %pOF\n", child_id, np);
> +                       ret = -EINVAL;
> +                       goto err_unwind;
> +               }
> +
> +               ret = of_parse_phandle_with_args(np, "power-domains",
> +                                                "#power-domain-cells", i,
> +                                                &parent_args);
> +               if (ret)
> +                       goto err_unwind;
> +
> +               /* Get the parent domain */
> +               parent_genpd = genpd_get_from_provider(&parent_args);
> +               of_node_put(parent_args.np);
> +               if (IS_ERR(parent_genpd)) {
> +                       pr_err("Failed to get parent domain for %pOF: %ld\n",
> +                              np, PTR_ERR(parent_genpd));
> +                       ret = PTR_ERR(parent_genpd);
> +                       goto err_unwind;
> +               }
> +
> +               /* Establish parent-child relationship */
> +               ret = pm_genpd_add_subdomain(parent_genpd, child_genpd);
> +               if (ret) {
> +                       pr_err("Failed to add child domain %u to parent in %pOF: %d\n",
> +                              child_id, np, ret);
> +                       goto err_unwind;
> +               }
> +
> +               /* Track for potential unwind */
> +               pairs[2 * added] = parent_genpd;
> +               pairs[2 * added + 1] = child_genpd;
> +               added++;
> +
> +               pr_debug("Added child domain %u (%s) to parent %s for %pOF\n",
> +                        child_id, child_genpd->name, parent_genpd->name, np);
> +       }
> +
> +       kfree(pairs);
> +       return 0;
> +
> +err_unwind:
> +       /* Reverse all previously established relationships */
> +       while (added-- > 0)
> +               pm_genpd_remove_subdomain(pairs[2 * added], pairs[2 * added + 1]);
> +       kfree(pairs);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(of_genpd_add_child_ids);
> +
> +/**
> + * of_genpd_remove_child_ids() - Remove parent-child PM domain relationships
> + * @np: Device node pointer associated with the PM domain provider.
> + * @data: Pointer to the onecell data associated with the PM domain provider.
> + *
> + * Reverses the effect of of_genpd_add_child_ids() by parsing the same
> + * power-domains and power-domains-child-ids properties and calling
> + * pm_genpd_remove_subdomain() for each established relationship.
> + *
> + * Returns 0 on success, -ENOENT if properties don't exist, or negative error
> + * code on failure.
> + */
> +int of_genpd_remove_child_ids(struct device_node *np,
> +                          struct genpd_onecell_data *data)
> +{
> +       struct of_phandle_args parent_args;
> +       struct generic_pm_domain *parent_genpd, *child_genpd;
> +       u32 child_id;
> +       int i, ret, count, child_count;
> +
> +       /* Check if both properties exist */
> +       count = of_count_phandle_with_args(np, "power-domains", "#power-domain-cells");
> +       if (count <= 0)
> +               return -ENOENT;
> +
> +       child_count = of_property_count_u32_elems(np, "power-domains-child-ids");
> +       if (child_count < 0)
> +               return -ENOENT;
> +       if (child_count != count)
> +               return -EINVAL;
> +
> +       for (i = 0; i < count; i++) {
> +               if (of_property_read_u32_index(np, "power-domains-child-ids",
> +                                              i, &child_id))
> +                       continue;
> +
> +               if (child_id >= data->num_domains || !data->domains[child_id])
> +                       continue;
> +
> +               ret = of_parse_phandle_with_args(np, "power-domains",
> +                                                "#power-domain-cells", i,
> +                                                &parent_args);
> +               if (ret)
> +                       continue;
> +
> +               parent_genpd = genpd_get_from_provider(&parent_args);
> +               of_node_put(parent_args.np);
> +               if (IS_ERR(parent_genpd))
> +                       continue;
> +
> +               child_genpd = data->domains[child_id];
> +               pm_genpd_remove_subdomain(parent_genpd, child_genpd);
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_genpd_remove_child_ids);
> +
>  /**
>   * of_genpd_add_device() - Add a device to an I/O PM domain
>   * @genpdspec: OF phandle args to use for look-up PM domain
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index f67a2cb7d781..b44615d79af6 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -465,6 +465,10 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
>  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);
> +int of_genpd_add_child_ids(struct device_node *np,
> +                          struct genpd_onecell_data *data);
> +int of_genpd_remove_child_ids(struct device_node *np,
> +                             struct genpd_onecell_data *data);
>
>  int genpd_dev_pm_attach(struct device *dev);
>  struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> @@ -534,6 +538,18 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
>  {
>         return ERR_PTR(-EOPNOTSUPP);
>  }
> +
> +static inline int of_genpd_add_child_ids(struct device_node *np,
> +                                        struct genpd_onecell_data *data)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +static inline int of_genpd_remove_child_ids(struct device_node *np,
> +                                           struct genpd_onecell_data *data)
> +{
> +       return -EOPNOTSUPP;
> +}
>  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>
>  #ifdef CONFIG_PM
>
> --
> 2.51.0
>