Re: [PATCH v8 3/3] clk: qcom: Support attaching GDSCs to multiple parents

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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 ?

> 
>>     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.

>>          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.

> 
>>     }
>>
>>     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@linaro.org>
>> ---
>>   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.

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.

---
bod