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

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

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

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