[PATCH v8 0/3] clk: qcom: Add support for multiple power-domains for a clock controller.

[PATCH v8 0/3] clk: qcom: Add support for multiple power-domains for a clock controller.

[Bryan O'Donoghue]

Changes in v8:
- Picks up change I agreed with Vlad but failed to cherry-pick into my b4
  tree - Vlad/Bod
- Rewords the commit log for patch #3. As I read it I decided I might
  translate bits of it from thought-stream into English - Bod
- Link to v7: https://lore.kernel.org/r/20241211-b4-linux-next-24-11-18-clock-multiple-power-domains-v7-0-7e302fd09488@linaro.org

Changes in v7:
- Expand commit log in patch #3
  I've discussed with Bjorn on IRC and video what to put into the log here
  and captured most of what we discussed.

  Mostly the point here is voting for voltages in the power-domain list
  is up to the drivers to do with performance states/opp-tables not for the
  GDSC code. - Bjorn/Bryan
- Link to v6: https://lore.kernel.org/r/20241129-b4-linux-next-24-11-18-clock-multiple-power-domains-v6-0-24486a608b86@linaro.org

Changes in v6:
- Passes NULL to second parameter of devm_pm_domain_attach_list - Vlad
- Link to v5: https://lore.kernel.org/r/20241128-b4-linux-next-24-11-18-clock-multiple-power-domains-v5-0-ca2826c46814@linaro.org

Changes in v5:
- In-lines devm_pm_domain_attach_list() in probe() directly - Vlad
- Link to v4: https://lore.kernel.org/r/20241127-b4-linux-next-24-11-18-clock-multiple-power-domains-v4-0-4348d40cb635@linaro.org

v4:
- Adds Bjorn's RB to first patch - Bjorn
- Drops the 'd' in "and int" - Bjorn
- Amends commit log of patch 3 to capture a number of open questions -
  Bjorn
- Link to v3: https://lore.kernel.org/r/20241126-b4-linux-next-24-11-18-clock-multiple-power-domains-v3-0-836dad33521a@linaro.org

v3:
- Fixes commit log "per which" - Bryan
- Link to v2: https://lore.kernel.org/r/20241125-b4-linux-next-24-11-18-clock-multiple-power-domains-v2-0-a5e7554d7e45@linaro.org

v2:
The main change in this version is Bjorn's pointing out that pm_runtime_*
inside of the gdsc_enable/gdsc_disable path would be recursive and cause a
lockdep splat. Dmitry alluded to this too.

Bjorn pointed to stuff being done lower in the gdsc_register() routine that
might be a starting point.

I iterated around that idea and came up with patch #3. When a gdsc has no
parent and the pd_list is non-NULL then attach that orphan GDSC to the
clock controller power-domain list.

Existing subdomain code in gdsc_register() will connect the parent GDSCs in
the clock-controller to the clock-controller subdomain, the new code here
does that same job for a list of power-domains the clock controller depends
on.

To Dmitry's point about MMCX and MCX dependencies for the registers inside
of the clock controller, I have switched off all references in a test dtsi
and confirmed that accessing the clock-controller regs themselves isn't
required.

On the second point I also verified my test branch with lockdep on which
was a concern with the pm_domain version of this solution but I wanted to
cover it anyway with the new approach for completeness sake.

Here's the item-by-item list of changes:

- Adds a patch to capture pm_genpd_add_subdomain() result code - Bryan
- Changes changelog of second patch to remove singleton and generally
  to make the commit log easier to understand - Bjorn
- Uses demv_pm_domain_attach_list - Vlad
- Changes error check to if (ret < 0 && ret != -EEXIST) - Vlad
- Retains passing &pd_data instead of NULL - because NULL doesn't do
  the same thing - Bryan/Vlad
- Retains standalone function qcom_cc_pds_attach() because the pd_data
  enumeration looks neater in a standalone function - Bryan/Vlad
- Drops pm_runtime in favour of gdsc_add_subdomain_list() for each
  power-domain in the pd_list.
  The pd_list will be whatever is pointed to by power-domains = <>
  in the dtsi - Bjorn
- Link to v1: https://lore.kernel.org/r/20241118-b4-linux-next-24-11-18-clock-multiple-power-domains-v1-0-b7a2bd82ba37@linaro.org

v1:
On x1e80100 and it's SKUs the Camera Clock Controller - CAMCC has
multiple power-domains which power it. Usually with a single power-domain
the core platform code will automatically switch on the singleton
power-domain for you. If you have multiple power-domains for a device, in
this case the clock controller, you need to switch those power-domains
on/off yourself.

The clock controllers can also contain Global Distributed
Switch Controllers - GDSCs which themselves can be referenced from dtsi
nodes ultimately triggering a gdsc_en() in drivers/clk/qcom/gdsc.c.

As an example:

cci0: cci@ac4a000 {
	power-domains = <&camcc TITAN_TOP_GDSC>;
};

This series adds the support to attach a power-domain list to the
clock-controllers and the GDSCs those controllers provide so that in the
case of the above example gdsc_toggle_logic() will trigger the power-domain
list with pm_runtime_resume_and_get() and pm_runtime_put_sync()
respectively.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
Bryan O'Donoghue (3):
      clk: qcom: gdsc: Capture pm_genpd_add_subdomain result code
      clk: qcom: common: Add support for power-domain attachment
      clk: qcom: Support attaching GDSCs to multiple parents

 drivers/clk/qcom/common.c |  6 ++++++
 drivers/clk/qcom/gdsc.c   | 41 +++++++++++++++++++++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h   |  1 +
 3 files changed, 46 insertions(+), 2 deletions(-)
---
base-commit: 744cf71b8bdfcdd77aaf58395e068b7457634b2c
change-id: 20241118-b4-linux-next-24-11-18-clock-multiple-power-domains-a5f994dc452a

Best regards,
-- 
Bryan O'Donoghue <bryan.odonoghue@linaro.org>

[PATCH v8 1/3] clk: qcom: gdsc: Capture pm_genpd_add_subdomain result code

[Bryan O'Donoghue]

Adding a new clause to this if/else I noticed the existing usage of
pm_genpd_add_subdomain() wasn't capturing and returning the result code.

pm_genpd_add_subdomain() returns an int and can fail. Capture that result
code and throw it up the call stack if something goes wrong.

Fixes: 1b771839de05 ("clk: qcom: gdsc: enable optional power domain support")
Cc: stable@vger.kernel.org
Reviewed-by: Bjorn Andersson <andersson@kernel.org>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/clk/qcom/gdsc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index fa5fe4c2a2ee7786c2e8858f3e41301f639e5d59..4fc6f957d0b846cc90e50ef243f23a7a27e66899 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -555,9 +555,11 @@ int gdsc_register(struct gdsc_desc *desc,
 		if (!scs[i])
 			continue;
 		if (scs[i]->parent)
-			pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
+			ret = pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
 		else if (!IS_ERR_OR_NULL(dev->pm_domain))
-			pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
+			ret = pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
+		if (ret)
+			return ret;
 	}
 
 	return of_genpd_add_provider_onecell(dev->of_node, data);

-- 
2.45.2

[PATCH v8 2/3] clk: qcom: common: Add support for power-domain attachment

[Bryan O'Donoghue]

Right now we support one power-domain per clock controller.
These single power-domains are switched on by the driver platform logic.

However when we have multiple power-domains attached to a clock-controller
that list of power-domains must be handled outside of driver platform
logic.

Use devm_pm_domain_attach_list() to automatically hook the list of given
power-domains in the dtsi for the clock-controller driver.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/clk/qcom/common.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 33cc1f73c69d1f875a193aea0552902268dc8716..b79e6a73b53a4113ca324d102d7be5504a9fe85e 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -22,6 +22,7 @@ struct qcom_cc {
 	struct qcom_reset_controller reset;
 	struct clk_regmap **rclks;
 	size_t num_rclks;
+	struct dev_pm_domain_list *pd_list;
 };
 
 const
@@ -299,6 +300,10 @@ int qcom_cc_really_probe(struct device *dev,
 	if (!cc)
 		return -ENOMEM;
 
+	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
+	if (ret < 0 && ret != -EEXIST)
+		return ret;
+
 	reset = &cc->reset;
 	reset->rcdev.of_node = dev->of_node;
 	reset->rcdev.ops = &qcom_reset_ops;

-- 
2.45.2

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

[Bryan O'Donoghue]

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

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

-- 
2.45.2

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

[Vladimir Zapolskiy]

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

Re: [PATCH v8 2/3] clk: qcom: common: Add support for power-domain attachment

[Vladimir Zapolskiy]

On 12/11/24 18:54, Bryan O'Donoghue wrote:
> Right now we support one power-domain per clock controller.
> These single power-domains are switched on by the driver platform logic.
> 
> However when we have multiple power-domains attached to a clock-controller
> that list of power-domains must be handled outside of driver platform
> logic.
> 
> Use devm_pm_domain_attach_list() to automatically hook the list of given
> power-domains in the dtsi for the clock-controller driver.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>   drivers/clk/qcom/common.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 33cc1f73c69d1f875a193aea0552902268dc8716..b79e6a73b53a4113ca324d102d7be5504a9fe85e 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -22,6 +22,7 @@ struct qcom_cc {
>   	struct qcom_reset_controller reset;
>   	struct clk_regmap **rclks;
>   	size_t num_rclks;
> +	struct dev_pm_domain_list *pd_list;
>   };
>   
>   const
> @@ -299,6 +300,10 @@ int qcom_cc_really_probe(struct device *dev,
>   	if (!cc)
>   		return -ENOMEM;
>   
> +	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
> +	if (ret < 0 && ret != -EEXIST)
> +		return ret;
> +
>   	reset = &cc->reset;
>   	reset->rcdev.of_node = dev->of_node;
>   	reset->rcdev.ops = &qcom_reset_ops;
> 

Looks good, thank you for the update.

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

--
Best wishes,
Vladimir

Re: [PATCH v8 1/3] clk: qcom: gdsc: Capture pm_genpd_add_subdomain result code

[Vladimir Zapolskiy]

On 12/11/24 18:54, Bryan O'Donoghue wrote:
> Adding a new clause to this if/else I noticed the existing usage of
> pm_genpd_add_subdomain() wasn't capturing and returning the result code.
> 
> pm_genpd_add_subdomain() returns an int and can fail. Capture that result
> code and throw it up the call stack if something goes wrong.
> 
> Fixes: 1b771839de05 ("clk: qcom: gdsc: enable optional power domain support")
> Cc: stable@vger.kernel.org
> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>   drivers/clk/qcom/gdsc.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index fa5fe4c2a2ee7786c2e8858f3e41301f639e5d59..4fc6f957d0b846cc90e50ef243f23a7a27e66899 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -555,9 +555,11 @@ int gdsc_register(struct gdsc_desc *desc,
>   		if (!scs[i])
>   			continue;
>   		if (scs[i]->parent)
> -			pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> +			ret = pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
>   		else if (!IS_ERR_OR_NULL(dev->pm_domain))
> -			pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
> +			ret = pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
> +		if (ret)
> +			return ret;

This is wrong, on the error path you shall rollback the registered subdomains
in the reverse order.

>   	}
>   
>   	return of_genpd_add_provider_onecell(dev->of_node, data);
> 

--
Best wishes,
Vladimir

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

[Bryan O'Donoghue]

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

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

[Vladimir Zapolskiy]

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