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@xxxxxxxxxx> 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