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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAPDyKFrY2kNaP=Hk-81B4WEGxyKUTYqBWJHQKtHnyTPWTFUOEQ@mail.gmail.com>
Date: Wed, 2 Jul 2025 14:12:20 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Kevin Hilman <khilman@...libre.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, devicetree@...r.kernel.org, 
	linux-pm@...r.kernel.org, arm-scmi@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v3 2/2] pmdomain: core: add support for subdomains
 using power-domain-map

On Mon, 30 Jun 2025 at 20:17, Kevin Hilman <khilman@...libre.com> wrote:
>
> Ulf Hansson <ulf.hansson@...aro.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