| 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
| ||
|
Message-ID: <CAPDyKFo-iPBPgkM43q+5cGR2sptkLk4E6TAERCQbCu24o1RfFQ@mail.gmail.com>
Date: Tue, 17 Jun 2025 15:41:04 +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 Tue, 17 Jun 2025 at 02:50, Kevin Hilman <khilman@...libre.com> wrote:
>
> Ulf Hansson <ulf.hansson@...aro.org> writes:
> --text follows this line--
> > On Sat, 14 Jun 2025 at 00:39, Kevin Hilman <khilman@...libre.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@...0000 {
> >> 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@...libre.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@...libre.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
Powered by blists - more mailing lists