Re: [PATCH v8 3/3] clk: qcom: Support attaching GDSCs to multiple parents
From: Vladimir Zapolskiy
Date: Thu Dec 12 2024 - 13:49:58 EST
Hi Bryan.
On 12/12/24 17:18, Bryan O'Donoghue wrote:
On 12/12/2024 15:06, Vladimir Zapolskiy wrote:
Hi Bryan.
On 12/11/24 18:54, Bryan O'Donoghue wrote:
When a clock-controller has multiple power-domains we need to attach the
GDSCs provided by the clock-controller to each of the list of power-
domains
as power subdomains of each of the power-domains respectively.
GDSCs come in three forms:
1. A GDSC which has no parent GDSC in the controller and no child GDSCs.
2. A GDSC which has no parent GDSC in the controller and has child GDSCs.
3. A child GDSC which derives power from the parent GDSC @ #2.
Cases 1 and 2 are "top-level" GDSCs which depend on the power-domains
- the
power-rails attached to the clock-controller to power-on.
When dtsi::power-domains = <> points to a single power-domain, Linux'
platform probe code takes care of hooking up the referenced power-domains
to the clock-controller.
When dtsi::power-domains = <> points to more than one power-domain we
must
take responsibility to attach the list of power-domains to our
clock-controller.
An added complexity is that currently gdsc_enable() and gdsc_disable() do
not register the top-level GDSCs as power subdomains of the controller's
power-domains.
This patch makes the subdomain association between whatever list of
top-level GDSCs a clock-controller provides and the power-domain list of
that clock-controller.
What we don't do here is take responsibility to adjust the voltages on
those power-rails when ramping clock frequencies - PLL rates - inside of
the clock-controller.
That voltage adjustment should be performed by operating-point/
performance
setpoint code in the driver requesting the new frequency.
There are some questions that it is worth discussing in the commit log:
1. Should there be a hierarchy of power-domains in the clock-controller ?
In other words if a list of dtsi::power-domains = <pd_a, pd_b, ..>
should a specific hierarchy be applied to power pd_a then pd_b etc.
It may be appropriate to introduce such a hierarchy however reasoning
this point out some more, any hierarchy of power-domain dependencies
should probably be handled in dtsi with a chain of power-domains.
If so, the description shall be found under Documentation/devicetree/
bindings/
I agree, I don't get your statement here, are you asking for additional
text ?
No need, I think I grasped your idea here.
One power-domain provider would point to another via
dtsi::power-domains = <>.
For the case of GDSC on/off there is no clear use-case to implement
a mechanism for a dependency list in the GDSC logic in-lieu of
already
existing methods to do dependencies in dtsi::power-domains = <>.
A defacto ordering happens because the first power-domain pd_a
will be
powered before pd_b as the list of domains is iterated through
linearly.
This defacto hierarchical structure would not be reliable and should
not be relied upon.
If you need to have a hierarchy of power-domains then structuring the
dependencies in the dtsi to
Do this:
pd_a {
compat = "qcom, power-domain-a";
Please remove spaces in compat property values.
Not real names but, sure.
It's just to be aligned with the principle of least astonishment, thank you.
power-domains = <&pd_c>;
};
pd_b {
compat = "qcom, power-domain-b";
};
pd_c {
compat = "qcom, power-domain-c";
};
clock-controller {
compat ="qcom, some-clock-controller";
power-domains = <&pd_a, &pd_b>;
}
Not this:
pd_a {
compat = "qcom, power-domain-a";
};
pd_b {
compat = "qcom, power-domain-b";
};
pd_c {
compat = "qcom, power-domain-c";
};
clock-controller {
compat ="qcom, some-clock-controller";
power-domains = <&pd_c, &pd_a, &pd_b>;
IMO it's a very fragile scheme, and like I've stated above at the bare
minimum for future usecases the description shall be found outside of
commit messages, preferably in the device tree bindings documentation.
So I stated above "Not this" very deliberately.
Thou shalt not rely on the ordering of power-domains in the dtsi.
Yes, this is correct, and my understanding is corrected in turn.
}
Thus ensuring that pd_a directly references its dependency to pd_c
without assuming the order of references in clock-controller imparts
or implements a deliberate and specific dependency hierarchy.
2. Should each GDSC inside a clock-controller be attached to each
power-domain listed in dtsi::power-domains = <> ?
In other words should child GDSCs attach to the power-domain list.
The answer to this is no. GDSCs which are children of a GDSC within a
clock-controller need only attach to the parent GDSC.
With a single power-domain or a list of power-domains either way only
the parent/top-level GDSC needs to be a subdomain of the input
dtsi::power-domains = <>.
3. Should top-level GDSCs inside the clock-controller attach to each
power-domain in the clock-controller.
Yes a GDSC that has no parent GDSC inside of the clock-controller
has an
inferred dependency on the power-domains powering the clock-
controller.
4. Performance states
Right now the best information we have is that performance states
should
be applied to a power-domain list equally.
Future implementations may have more detail to differentiate the
option
to vote for different voltages on different power-domains when
setting
clock frequencies.
Either way setting the performance state of the power-domains for the
clock-controller should be represented by operating-point code in the
hardware driver which depends on the clocks not in the
gdsc_enable()/gdsc_disable() path.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
---
drivers/clk/qcom/common.c | 1 +
drivers/clk/qcom/gdsc.c | 35 +++++++++++++++++++++++++++++++++++
drivers/clk/qcom/gdsc.h | 1 +
3 files changed, 37 insertions(+)
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index
b79e6a73b53a4113ca324d102d7be5504a9fe85e..9e3380fd718198c9fe63d7361615a91c3ecb3d60 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -323,6 +323,7 @@ int qcom_cc_really_probe(struct device *dev,
scd->dev = dev;
scd->scs = desc->gdscs;
scd->num = desc->num_gdscs;
+ scd->pd_list = cc->pd_list;
ret = gdsc_register(scd, &reset->rcdev, regmap);
if (ret)
return ret;
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index
4fc6f957d0b846cc90e50ef243f23a7a27e66899..cb4afa6d584899f3dafa380d5e01be6de9711737 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -506,6 +506,36 @@ static int gdsc_init(struct gdsc *sc)
return ret;
}
+static int gdsc_add_subdomain_list(struct dev_pm_domain_list *pd_list,
+ struct generic_pm_domain *subdomain)
+{
+ int i, ret;
+
+ for (i = 0; i < pd_list->num_pds; i++) {
+ struct device *dev = pd_list->pd_devs[i];
+ struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
+
+ ret = pm_genpd_add_subdomain(genpd, subdomain);
+ if (ret)
+ return ret;
It's needed to rollback call pm_genpd_remove_subdomain() for all added
subdomains on the error path.
+ }
+
+ return 0;
+}
+
+static void gdsc_remove_subdomain_list(struct dev_pm_domain_list
*pd_list,
+ struct generic_pm_domain *subdomain)
+{
+ int i;
+
+ for (i = 0; i < pd_list->num_pds; i++) {
To be on the safe side, and especially because the order on the list has
high importance, please remove subdomains in the reverse order.
The order shouldn't have any meaning at all but I agree the removal
should happen in reverse order anyway.
It would be so nice.
I've tried to make the point in the commit log that we 100% _shouldn't_
rely on the order a pd gets added by a for(){} loop.
If one PD depends on another then that dependency should be expressed in
the dtsi with an explicit power-domains = <> from one domain to the other.
IMO that's the right way to express such a dependency - via an explicit
statement in the dtsi not a defacto outcome as a result of a for() loop
in gdsc.
Right, agreed!
--
Best wishes,
Vladimir