diff mbox series

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

Message ID 20241211-b4-linux-next-24-11-18-clock-multiple-power-domains-v8-3-5d93cef910a4@linaro.org (mailing list archive)
State New
Headers show
Series clk: qcom: Add support for multiple power-domains for a clock controller. | expand

Commit Message

Bryan O'Donoghue Dec. 11, 2024, 4:54 p.m. UTC 
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.
   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";
        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>;
   }

   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(+)

Comments

Vladimir Zapolskiy Dec. 12, 2024, 3:06 p.m. UTC | #1 
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@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.

> +		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
Bryan O'Donoghue Dec. 12, 2024, 3:18 p.m. UTC | #2 
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
Vladimir Zapolskiy Dec. 12, 2024, 6:49 p.m. UTC | #3 
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
diff mbox series

Patch

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;
+	}
+
+	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++) {
+		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