| Message ID | 20250613-pmdomain-hierarchy-onecell-v3-2-5c770676fce7@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series | pmdomain: core: add support for domain hierarchies in DT | expand |
On Sat, 14 Jun 2025 at 00:39, Kevin Hilman <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 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@2400000 { > 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@baylibre.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). Moreover, we also need a corresponding "cleanup" helper function to remove the child-domain (subdomain) correctly, similar to of_genpd_remove_subdomain(). > + > ret = genpd_set_required_opp(dev, index); > if (ret) > goto err; > @@ -3056,6 +3072,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, > dev_gpd_data(dev)->default_pstate = 0; > } > > + if (parent_pd) > + pm_genpd_remove_subdomain(parent_pd, pd); > genpd_remove_device(pd, dev); > return -EPROBE_DEFER; > } > @@ -3063,6 +3081,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, > return 1; > > err: > + if (parent_pd) > + pm_genpd_remove_subdomain(parent_pd, pd); > genpd_remove_device(pd, dev); > return ret; > } > > -- > 2.49.0 > Kind regards Uffe
Ulf Hansson <ulf.hansson@linaro.org> writes: --text follows this line-- > On Sat, 14 Jun 2025 at 00:39, Kevin Hilman <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 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@2400000 { >> 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@baylibre.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. 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.) > 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. 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. 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.
On Tue, 17 Jun 2025 at 02:50, Kevin Hilman <khilman@baylibre.com> wrote: > > Ulf Hansson <ulf.hansson@linaro.org> writes: > --text follows this line-- > > On Sat, 14 Jun 2025 at 00:39, Kevin Hilman <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 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@2400000 { > >> 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@baylibre.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@baylibre.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
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); + } + ret = genpd_set_required_opp(dev, index); if (ret) goto err; @@ -3056,6 +3072,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, dev_gpd_data(dev)->default_pstate = 0; } + if (parent_pd) + pm_genpd_remove_subdomain(parent_pd, pd); genpd_remove_device(pd, dev); return -EPROBE_DEFER; } @@ -3063,6 +3081,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, return 1; err: + if (parent_pd) + pm_genpd_remove_subdomain(parent_pd, pd); genpd_remove_device(pd, dev); return ret; }
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@2400000 { 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@baylibre.com> --- drivers/pmdomain/core.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)