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/
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.
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.
}
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.
+ struct device *dev = pd_list->pd_devs[i];
+ struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
+
+ pm_genpd_remove_subdomain(genpd, subdomain);
+ }
+}
+
int gdsc_register(struct gdsc_desc *desc,
struct reset_controller_dev *rcdev, struct regmap *regmap)
{
@@ -558,6 +588,9 @@ int gdsc_register(struct gdsc_desc *desc,
ret = pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
else if (!IS_ERR_OR_NULL(dev->pm_domain))
ret = pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
+ else if (desc->pd_list)
+ ret = gdsc_add_subdomain_list(desc->pd_list, &scs[i]->pd);
+
if (ret)
return ret;
}
@@ -580,6 +613,8 @@ void gdsc_unregister(struct gdsc_desc *desc)
pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
else if (!IS_ERR_OR_NULL(dev->pm_domain))
pm_genpd_remove_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
+ else if (desc->pd_list)
+ gdsc_remove_subdomain_list(desc->pd_list, &scs[i]->pd);
}
of_genpd_del_provider(dev->of_node);
}
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 1e2779b823d1c8ca077c9b4cd0a0dbdf5f9457ef..dd843e86c05b2f30e6d9e978681580016333839d 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -80,6 +80,7 @@ struct gdsc_desc {
struct device *dev;
struct gdsc **scs;
size_t num;
+ struct dev_pm_domain_list *pd_list;
};
#ifdef CONFIG_QCOM_GDSC
--
Best wishes,
Vladimir
