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

[Kevin Hilman <khilman@baylibre.com>]

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.

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)
+{
+	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