Re: mixing SCMI and PSCI power domain hierarchy

[Kevin Hilman <khilman@baylibre.com>]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On Wed, 16 Apr 2025 at 02:22, Kevin Hilman <khilman@baylibre.com> wrote:
>>
>> Ulf Hansson <ulf.hansson@linaro.org> writes:

[...]

>> > 2) Enable parents to be described in the "scmi_pds" node. To do that,
>> > we need to extend the DT bindings a bit, as we need the SCMI index
>> > that should correspond to the SCMI-child-power-domain. In other words
>> > "power-domains = <&TOP1_PD>" would not be sufficient.
>>
>> This is the direction that I was thinking makes the most sense, and
>> actually reflects real hardware.
>>
>> If you have any thoughts about what that binding change should look
>> like, maybe I can have a look at implementing it.
>
> Usually we describe parent domains by using the original power-domain
> DT binding, but this is limited to work for when the child-domain node
> has #power-domain-cells = <0>.
>
> We need to investigate if there are any other DT bindings we can get
> some influence from, perhaps clocks have something similar?
>
> Anyway, without further thinking my idea would be to add a new
> property ("power-domain-child-ids") where we can put a list of indexes
> for the child-domain that corresponds to each of the parents that we
> can list in "power-domains".

OK, I have an idea.

First, the normal stuff.  We have the SCMI PDs:

&scmi {
	scmi_pds: protocol@11 {
		reg = <0x11>;
		#power-domain-cells = <1>;
	};
};

Then we have an example leaf node which uses that:

	main_timer0: timer@2400000 {
                [...]
		clocks = <&scmi_clk 47>;
		clock-names = "fck";
		power-domains = <&scmi_pds 15>;
	};

And then, we have the top-level PD controlled by PSCI:

&psci {
	compatible = "arm,psci-1.0";
	method = "smc";

	MAIN_PD: power-controller-main {
		#power-domain-cells = <0>;
	};
};

Now the new idea: The hierarchy I want to represent is that main_timer0
is in SCMI PD 15, which is in the top-level domain MAIN_PD.

So, what about simply extending the SCMI binding where we can
(optionally) define a parent domain for each SCMI ID?  So the SCMI node
would then look like this:

	scmi_pds: protocol@11 {
		reg = <0x11>;
		#power-domain-cells = <1>;

		/* main_timer0: SCMI ID: 15 */
		pd15: domain@15 {
		    reg = <15>;
		    parent-domain = <&MAIN_PD>;
	        };
	};

This requires adding a bit of code to the SCMI PM domain driver to check
for these subnodes with optional parent-domain properties, and I already
have a PoC[1] which works on v6.12.

One other reason I like this solution is that it actually kills two
birds with one stone.  The first one is describing the hieracrhy above,
but the second one is that it solves another limitation of the SCMI PM
domain implementation.

Currently you can only have a single group of SCMI PDs.  More
specifically, you can only have a single "protocol@11" or "reg = <0x11>"
SCMI node.  Which means, you cannot have more than one group of SCMI
PDs.  So if you have (like I do) groups of SCMI-controlled PDs that area
themselves grouped into multiple top-level PDs, there is no way to
describe that today since you can only have a single SCMI PD node.

To get more specific, on this TI SoC I'm working on now, there's more
than one top-level PD controlled by PSCI.  The top-level (PSCI) domains
look like this:

&psci {
	compatible = "arm,psci-1.0";
	method = "smc";

	MAIN_PD: power-controller-main {
		#power-domain-cells = <0>;
	};

	WKUP_PD: power-controller-wkup {
		#power-domain-cells = <0>;
	};
};

And in addition to the main_timer0 leaf device described above, I have
another timer that is also SCMI controlled like this:

	wkup_timer0: timer@2b100000 {
                [...]
		clocks = <&scmi_clk 85>;
		clock-names = "fck";
		power-domains = <&scmi_pds 19>;
	};

but..., this one is not in MAIN_PD, it is in WKUP_PD.  Which means I
need some way to declare that SCMI PDs that are part of the one (and
only) "protocol@11" node could have different parent domains.  With the
new idea, we just do this: (note that the subnodes have different
parent-domain properties):

	scmi_pds: protocol@11 {
		reg = <0x11>;
		#power-domain-cells = <1>;

		/* main_timer0: SCMI ID: 15 */
		pd15: domain@15 {
		    reg = <15>;
		    parent-domain = <&MAIN_PD>;
	        };

		/* wkup_timer0: SCMI ID: 19 */
		pd19: domain@19 {
		    reg = <19>;
		    parent-domain = <&WKUP_PD>;
	        };
	};

So.... what you think about this?  I guess's the "parent-domain" idea is
the really new concept here, but I guess it's not that much different
than a clock divider that points to its parent clock.

Anyways, curious what you think.  I quite like this solution because it
solves two fairly big limitations in the SCMI PM domain implementaion
that exist today with one relatively simple solution.

Kevin

P.S. maybe the most interesting thing about this solution is that it was
     co-developed with AI.  I've been experimenting with Anthropic's
     Claude Code[2] for both brainstorming and coding.  After coming up
     with some other ideas I rejected, Claude came up with this one, and
     when I realized it solved both my problems in one go, I was
     obviously drawn to it, and figured it's worth proposing for
     discussion.  I wrote this email, and had to manually write my own
     DT to test out this solution, but the patch below was entirely
     written by Claude.

[1]
diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
index a7784a8bb5db..1e4aa17ca1d2 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,53 @@ 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);
+	/* First register the SCMI domains with the power domain framework */
+	ret = of_genpd_add_provider_onecell(np, scmi_pd_data);
+	if (ret)
+		return ret;
+
+	/* AFTER registering domains, now try to establish subdomain relationships */
+	for (i = 0; i < num_domains; i++) {
+		struct of_phandle_args parent_pd_args;
+		struct device_node *child_np;
+
+		/* Skip if this domain is not defined */
+		if (!domains[i])
+			continue;
+
+		/* Look for child nodes with parent-domain property */
+		for_each_child_of_node(np, child_np) {
+			u32 domain_id;
+
+			/* Check if this child node refers to the current SCMI domain */
+			if (of_property_read_u32(child_np, "reg", &domain_id) || domain_id != i)
+				continue;
+
+			/* Check if it has a parent-domain property */
+			if (of_parse_phandle_with_args(child_np, "parent-domain", "#power-domain-cells",
+						      0, &parent_pd_args) < 0)
+				continue;
+
+			/* Create a phandle args structure for the SCMI domain */
+			struct of_phandle_args subdomain_args = {
+				.np = np,
+				.args_count = 1,
+				.args = {i}
+			};
+
+			/* Use of_genpd_add_subdomain to directly establish the relationship */
+			ret = of_genpd_add_subdomain(&parent_pd_args, &subdomain_args);
+			if (ret) {
+				dev_warn(dev, "Failed to add SCMI domain %d as subdomain: %d\n",
+					 i, ret);
+			} else {
+				dev_info(dev, "Added SCMI domain %s as subdomain of %s\n",
+					 domains[i]->name, parent_pd_args.np->name);
+			}
+		}
+	}
+
+	return 0;
 }
 
 static void scmi_pm_domain_remove(struct scmi_device *sdev)

[2] https://docs.anthropic.com/en/docs/agents-and-tools/claude-code/overview