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

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

[Bryan O'Donoghue]

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 (2):
      clk: qcom: common: Add support for power-domain attachment
      clk: qcom: gdsc: Add pm_runtime hooks

 drivers/clk/qcom/common.c | 24 ++++++++++++++++++++++++
 drivers/clk/qcom/gdsc.c   | 26 ++++++++++++++++++--------
 drivers/clk/qcom/gdsc.h   |  2 ++
 3 files changed, 44 insertions(+), 8 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 1/2] clk: qcom: common: Add support for power-domain attachment

[Bryan O'Donoghue]

Right now we have a plethora of singleton power-domains which power clock
controllers. These singletons are switched on by core logic when we probe
the clocks.

However when multiple power-domains are attached to a clock controller that
list of power-domains needs to be managed outside of core logic.

Use dev_pm_domain_attach_list() to automatically hook the list of given
power-domains in the dtsi for the clock being registered in
qcom_cc_really_probe().

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

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 33cc1f73c69d1f875a193aea0552902268dc8716..b4377fa09f7c0ec8d3c63dfc97d04fbb8cd6e10b 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
@@ -283,6 +284,25 @@ static int qcom_cc_icc_register(struct device *dev,
 						     desc->num_icc_hws, icd);
 }
 
+static int qcom_cc_pds_attach(struct device *dev, struct qcom_cc *cc)
+{
+	struct dev_pm_domain_attach_data pd_data = {
+		.pd_names = 0,
+		.num_pd_names = 0,
+	};
+	int ret;
+
+	/* Only one power-domain platform framework will hook it up */
+	if (dev->pm_domain)
+		return 0;
+
+	ret = dev_pm_domain_attach_list(dev, &pd_data, &cc->pd_list);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 int qcom_cc_really_probe(struct device *dev,
 			 const struct qcom_cc_desc *desc, struct regmap *regmap)
 {
@@ -299,6 +319,10 @@ int qcom_cc_really_probe(struct device *dev,
 	if (!cc)
 		return -ENOMEM;
 
+	ret = qcom_cc_pds_attach(dev, cc);
+	if (ret)
+		return ret;
+
 	reset = &cc->reset;
 	reset->rcdev.of_node = dev->of_node;
 	reset->rcdev.ops = &qcom_reset_ops;

-- 
2.45.2

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

[Dmitry Baryshkov]

On Mon, Nov 18, 2024 at 02:24:32AM +0000, Bryan O'Donoghue wrote:
> Right now we have a plethora of singleton power-domains which power clock
> controllers. These singletons are switched on by core logic when we probe
> the clocks.
> 
> However when multiple power-domains are attached to a clock controller that
> list of power-domains needs to be managed outside of core logic.
> 
> Use dev_pm_domain_attach_list() to automatically hook the list of given
> power-domains in the dtsi for the clock being registered in
> qcom_cc_really_probe().
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/clk/qcom/common.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 33cc1f73c69d1f875a193aea0552902268dc8716..b4377fa09f7c0ec8d3c63dfc97d04fbb8cd6e10b 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
> @@ -283,6 +284,25 @@ static int qcom_cc_icc_register(struct device *dev,
>  						     desc->num_icc_hws, icd);
>  }
>  
> +static int qcom_cc_pds_attach(struct device *dev, struct qcom_cc *cc)
> +{
> +	struct dev_pm_domain_attach_data pd_data = {
> +		.pd_names = 0,
> +		.num_pd_names = 0,
> +	};
> +	int ret;
> +
> +	/* Only one power-domain platform framework will hook it up */
> +	if (dev->pm_domain)
> +		return 0;
> +
> +	ret = dev_pm_domain_attach_list(dev, &pd_data, &cc->pd_list);
> +	if (ret < 0)
> +		return ret;

I don't think it is enough to just attach to power domains. We also need
to power on some of them (MMCX) in order to be able to access clock
controller registers.

> +
> +	return 0;
> +}
> +
>  int qcom_cc_really_probe(struct device *dev,
>  			 const struct qcom_cc_desc *desc, struct regmap *regmap)
>  {
> @@ -299,6 +319,10 @@ int qcom_cc_really_probe(struct device *dev,
>  	if (!cc)
>  		return -ENOMEM;
>  
> +	ret = qcom_cc_pds_attach(dev, cc);
> +	if (ret)
> +		return ret;
> +
>  	reset = &cc->reset;
>  	reset->rcdev.of_node = dev->of_node;
>  	reset->rcdev.ops = &qcom_reset_ops;
> 
> -- 
> 2.45.2
> 

-- 
With best wishes
Dmitry

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

[Bryan O'Donoghue]

On 18/11/2024 13:03, Dmitry Baryshkov wrote:
> I don't think it is enough to just attach to power domains. We also need
> to power on some of them (MMCX) in order to be able to access clock
> controller registers.

That the next patch.

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

[Vladimir Zapolskiy]

On 11/18/24 04:24, Bryan O'Donoghue wrote:
> Right now we have a plethora of singleton power-domains which power clock
> controllers. These singletons are switched on by core logic when we probe
> the clocks.
> 
> However when multiple power-domains are attached to a clock controller that
> list of power-domains needs to be managed outside of core logic.
> 
> Use dev_pm_domain_attach_list() to automatically hook the list of given
> power-domains in the dtsi for the clock being registered in
> qcom_cc_really_probe().
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>   drivers/clk/qcom/common.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 33cc1f73c69d1f875a193aea0552902268dc8716..b4377fa09f7c0ec8d3c63dfc97d04fbb8cd6e10b 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
> @@ -283,6 +284,25 @@ static int qcom_cc_icc_register(struct device *dev,
>   						     desc->num_icc_hws, icd);
>   }
>   
> +static int qcom_cc_pds_attach(struct device *dev, struct qcom_cc *cc)
> +{
> +	struct dev_pm_domain_attach_data pd_data = {
> +		.pd_names = 0,
> +		.num_pd_names = 0,
> +	};
> +	int ret;
> +
> +	/* Only one power-domain platform framework will hook it up */
> +	if (dev->pm_domain)
> +		return 0;
> +
> +	ret = dev_pm_domain_attach_list(dev, &pd_data, &cc->pd_list);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>   int qcom_cc_really_probe(struct device *dev,
>   			 const struct qcom_cc_desc *desc, struct regmap *regmap)
>   {
> @@ -299,6 +319,10 @@ int qcom_cc_really_probe(struct device *dev,
>   	if (!cc)
>   		return -ENOMEM;
>   
> +	ret = qcom_cc_pds_attach(dev, cc);
> +	if (ret)
> +		return ret;
> +

	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
	if (ret < 0 && ret != -EEXIST)
		return ret;

That's it. No need to introduce a new function, not saying about 20 LoC off.

Next, you have to call dev_pm_domain_detach_list() in your version of the
change on the error paths etc., fortunately this can be easily avoided,
if the resource management flavour of the same function is in use.

>   	reset = &cc->reset;
>   	reset->rcdev.of_node = dev->of_node;
>   	reset->rcdev.ops = &qcom_reset_ops;
> 

--
Best wishes,
Vladimir

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

[Bryan O'Donoghue]

On 18/11/2024 22:17, Vladimir Zapolskiy wrote:
>      ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
>      if (ret < 0 && ret != -EEXIST)
>          return ret;
> 
> That's it. No need to introduce a new function, not saying about 20 LoC 
> off.
> 
> Next, you have to call dev_pm_domain_detach_list() in your version of the
> change on the error paths etc., fortunately this can be easily avoided,
> if the resource management flavour of the same function is in use.

 From memory I _thought_ I concluded this was necessary

+    /* Only one power-domain platform framework will hook it up */
+    if (dev->pm_domain)
+        return 0;

=> for clocks which have a single power-domain the core framework will 
already have setup the linkage by the time we get to this point in the code.

But, I'll check again to make sure.

---
bod

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

[Bjorn Andersson]

On Mon, Nov 18, 2024 at 02:24:32AM +0000, Bryan O'Donoghue wrote:
> Right now we have a plethora of singleton power-domains which power clock
> controllers. These singletons are switched on by core logic when we probe
> the clocks.
> 
> However when multiple power-domains are attached to a clock controller that
> list of power-domains needs to be managed outside of core logic.
> 

I'd prefer if you rewrote this to make it clearer for the broader
audience what exactly you mean with "singleton" and "core logic".

> Use dev_pm_domain_attach_list() to automatically hook the list of given
> power-domains in the dtsi for the clock being registered in
> qcom_cc_really_probe().
> 

Do we need to power on/off all the associated power-domains every time
we access registers in the clock controller etc, or only in relation to
operating these GDSCs?

Regards,
Bjorn

> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/clk/qcom/common.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 33cc1f73c69d1f875a193aea0552902268dc8716..b4377fa09f7c0ec8d3c63dfc97d04fbb8cd6e10b 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
> @@ -283,6 +284,25 @@ static int qcom_cc_icc_register(struct device *dev,
>  						     desc->num_icc_hws, icd);
>  }
>  
> +static int qcom_cc_pds_attach(struct device *dev, struct qcom_cc *cc)
> +{
> +	struct dev_pm_domain_attach_data pd_data = {
> +		.pd_names = 0,
> +		.num_pd_names = 0,
> +	};
> +	int ret;
> +
> +	/* Only one power-domain platform framework will hook it up */
> +	if (dev->pm_domain)
> +		return 0;
> +
> +	ret = dev_pm_domain_attach_list(dev, &pd_data, &cc->pd_list);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  int qcom_cc_really_probe(struct device *dev,
>  			 const struct qcom_cc_desc *desc, struct regmap *regmap)
>  {
> @@ -299,6 +319,10 @@ int qcom_cc_really_probe(struct device *dev,
>  	if (!cc)
>  		return -ENOMEM;
>  
> +	ret = qcom_cc_pds_attach(dev, cc);
> +	if (ret)
> +		return ret;
> +
>  	reset = &cc->reset;
>  	reset->rcdev.of_node = dev->of_node;
>  	reset->rcdev.ops = &qcom_reset_ops;
> 
> -- 
> 2.45.2
> 

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

[Bryan O'Donoghue]

On 19/11/2024 15:41, Bjorn Andersson wrote:
audience what exactly you mean with "singleton" and "core logic".
> 
>> Use dev_pm_domain_attach_list() to automatically hook the list of given
>> power-domains in the dtsi for the clock being registered in
>> qcom_cc_really_probe().
>>
> Do we need to power on/off all the associated power-domains every time
> we access registers in the clock controller etc, or only in relation to
> operating these GDSCs?

Its a good question.

No I don't believe these PDs are required for the regs themselves i.e. 
we can write and read - I checked the regs in the clock's probe with the 
GDSCs off

         /* Keep clocks always enabled */
         qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
         qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */

only inside the probe where we actually try to switch the clock on, do 
we need the PD.

         ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_x1e80100_desc, 
regmap);

Which means the registers themselves don't need the PD. The clock 
remains "stuck" unless the GDSC is on which to me means that the PLL 
isn't powered until the GDSC is switched on.

So no, the regs are fine but the PLL won't budge without juice from the PD.

---
bod

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

[Dmitry Baryshkov]

On Wed, Nov 20, 2024 at 04:49:04PM +0000, Bryan O'Donoghue wrote:
> On 19/11/2024 15:41, Bjorn Andersson wrote:
> audience what exactly you mean with "singleton" and "core logic".
> > 
> > > Use dev_pm_domain_attach_list() to automatically hook the list of given
> > > power-domains in the dtsi for the clock being registered in
> > > qcom_cc_really_probe().
> > > 
> > Do we need to power on/off all the associated power-domains every time
> > we access registers in the clock controller etc, or only in relation to
> > operating these GDSCs?
> 
> Its a good question.
> 
> No I don't believe these PDs are required for the regs themselves i.e. we
> can write and read - I checked the regs in the clock's probe with the GDSCs
> off
> 
>         /* Keep clocks always enabled */
>         qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
>         qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
> 
> only inside the probe where we actually try to switch the clock on, do we
> need the PD.
> 
>         ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_x1e80100_desc,
> regmap);
>
> Which means the registers themselves don't need the PD. The clock remains
> "stuck" unless the GDSC is on which to me means that the PLL isn't powered
> until the GDSC is switched on.
> 
> So no, the regs are fine but the PLL won't budge without juice from the PD.

Is it for the MMCX or for MXC domain? If my memory doesn't play tricks
on me (it can) I think that on sm8250 I had to keep MMCX up to access
registers. But it also well might be that I didn't run the fine-grained
test and the MMCX was really required to power up the PLLs rather than
registers.


-- 
With best wishes
Dmitry

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

[Bryan O'Donoghue]

On 21/11/2024 21:59, Dmitry Baryshkov wrote:
> Is it for the MMCX or for MXC domain? If my memory doesn't play tricks
> on me (it can) I think that on sm8250 I had to keep MMCX up to access
> registers. But it also well might be that I didn't run the fine-grained
> test and the MMCX was really required to power up the PLLs rather than
> registers.

I see MXC is also used by the cdsp.

I'll have a poke to see if I can ensure both PDs are off and see what 
happens to reg access.

Perhaps my first pass test didn't cover it.

---
bod

[PATCH 2/2] clk: qcom: gdsc: Add pm_runtime hooks

[Bryan O'Donoghue]

Introduce pm_runtime_get() and pm_runtime_put_sync() on the
gdsc_toggle_logic().

This allows for the switching of the GDSC on/off to propagate to the parent
clock controller and consequently for any list of power-domains powering
that controller to be switched on/off.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/clk/qcom/gdsc.c | 26 ++++++++++++++++++--------
 drivers/clk/qcom/gdsc.h |  2 ++
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index fa5fe4c2a2ee7786c2e8858f3e41301f639e5d59..ff5df942147f87e0df24a70cf9ee53bb2df36e54 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/ktime.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset-controller.h>
@@ -141,10 +142,14 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status,
 {
 	int ret;
 
-	if (status == GDSC_ON && sc->rsupply) {
-		ret = regulator_enable(sc->rsupply);
-		if (ret < 0)
-			return ret;
+	if (status == GDSC_ON) {
+		if (sc->rsupply) {
+			ret = regulator_enable(sc->rsupply);
+			if (ret < 0)
+				return ret;
+		}
+		if (pm_runtime_enabled(sc->dev))
+			pm_runtime_resume_and_get(sc->dev);
 	}
 
 	ret = gdsc_update_collapse_bit(sc, status == GDSC_OFF);
@@ -177,10 +182,14 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status,
 	ret = gdsc_poll_status(sc, status);
 	WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
 
-	if (!ret && status == GDSC_OFF && sc->rsupply) {
-		ret = regulator_disable(sc->rsupply);
-		if (ret < 0)
-			return ret;
+	if (!ret && status == GDSC_OFF) {
+		if (pm_runtime_enabled(sc->dev))
+			pm_runtime_put_sync(sc->dev);
+		if (sc->rsupply) {
+			ret = regulator_disable(sc->rsupply);
+			if (ret < 0)
+				return ret;
+		}
 	}
 
 	return ret;
@@ -544,6 +553,7 @@ int gdsc_register(struct gdsc_desc *desc,
 			continue;
 		scs[i]->regmap = regmap;
 		scs[i]->rcdev = rcdev;
+		scs[i]->dev = dev;
 		ret = gdsc_init(scs[i]);
 		if (ret)
 			return ret;
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 1e2779b823d1c8ca077c9b4cd0a0dbdf5f9457ef..71ca02c78c5d089cdf96deadc417982ad6079255 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -30,6 +30,7 @@ struct reset_controller_dev;
  * @resets: ids of resets associated with this gdsc
  * @reset_count: number of @resets
  * @rcdev: reset controller
+ * @dev: device associated with this gdsc
  */
 struct gdsc {
 	struct generic_pm_domain	pd;
@@ -74,6 +75,7 @@ struct gdsc {
 
 	const char 			*supply;
 	struct regulator		*rsupply;
+	struct device			*dev;
 };
 
 struct gdsc_desc {

-- 
2.45.2

Re: [PATCH 2/2] clk: qcom: gdsc: Add pm_runtime hooks

[Dmitry Baryshkov]

On Mon, Nov 18, 2024 at 02:24:33AM +0000, Bryan O'Donoghue wrote:
> Introduce pm_runtime_get() and pm_runtime_put_sync() on the
> gdsc_toggle_logic().
> 
> This allows for the switching of the GDSC on/off to propagate to the parent
> clock controller and consequently for any list of power-domains powering
> that controller to be switched on/off.

What is the end result of this patch? Does it bring up a single PM
domain or all of them? Or should it be a part of the driver's PM
callbacks? If the CC has multiple parent PM domains, shouldn't we also
use some of them as GDSC's parents?

> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/clk/qcom/gdsc.c | 26 ++++++++++++++++++--------
>  drivers/clk/qcom/gdsc.h |  2 ++
>  2 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index fa5fe4c2a2ee7786c2e8858f3e41301f639e5d59..ff5df942147f87e0df24a70cf9ee53bb2df36e54 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -11,6 +11,7 @@
>  #include <linux/kernel.h>
>  #include <linux/ktime.h>
>  #include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/reset-controller.h>
> @@ -141,10 +142,14 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status,
>  {
>  	int ret;
>  
> -	if (status == GDSC_ON && sc->rsupply) {
> -		ret = regulator_enable(sc->rsupply);
> -		if (ret < 0)
> -			return ret;
> +	if (status == GDSC_ON) {
> +		if (sc->rsupply) {
> +			ret = regulator_enable(sc->rsupply);
> +			if (ret < 0)
> +				return ret;
> +		}
> +		if (pm_runtime_enabled(sc->dev))
> +			pm_runtime_resume_and_get(sc->dev);
>  	}
>  
>  	ret = gdsc_update_collapse_bit(sc, status == GDSC_OFF);
> @@ -177,10 +182,14 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status,
>  	ret = gdsc_poll_status(sc, status);
>  	WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
>  
> -	if (!ret && status == GDSC_OFF && sc->rsupply) {
> -		ret = regulator_disable(sc->rsupply);
> -		if (ret < 0)
> -			return ret;
> +	if (!ret && status == GDSC_OFF) {
> +		if (pm_runtime_enabled(sc->dev))
> +			pm_runtime_put_sync(sc->dev);
> +		if (sc->rsupply) {
> +			ret = regulator_disable(sc->rsupply);
> +			if (ret < 0)
> +				return ret;
> +		}
>  	}
>  
>  	return ret;
> @@ -544,6 +553,7 @@ int gdsc_register(struct gdsc_desc *desc,
>  			continue;
>  		scs[i]->regmap = regmap;
>  		scs[i]->rcdev = rcdev;
> +		scs[i]->dev = dev;
>  		ret = gdsc_init(scs[i]);
>  		if (ret)
>  			return ret;
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index 1e2779b823d1c8ca077c9b4cd0a0dbdf5f9457ef..71ca02c78c5d089cdf96deadc417982ad6079255 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -30,6 +30,7 @@ struct reset_controller_dev;
>   * @resets: ids of resets associated with this gdsc
>   * @reset_count: number of @resets
>   * @rcdev: reset controller
> + * @dev: device associated with this gdsc
>   */
>  struct gdsc {
>  	struct generic_pm_domain	pd;
> @@ -74,6 +75,7 @@ struct gdsc {
>  
>  	const char 			*supply;
>  	struct regulator		*rsupply;
> +	struct device			*dev;
>  };
>  
>  struct gdsc_desc {
> 
> -- 
> 2.45.2
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] clk: qcom: gdsc: Add pm_runtime hooks

[Bryan O'Donoghue]

On 18/11/2024 13:10, Dmitry Baryshkov wrote:
>> Introduce pm_runtime_get() and pm_runtime_put_sync() on the
>> gdsc_toggle_logic().
>>
>> This allows for the switching of the GDSC on/off to propagate to the parent
>> clock controller and consequently for any list of power-domains powering
>> that controller to be switched on/off.
> What is the end result of this patch? Does it bring up a single PM
> domain or all of them? Or should it be a part of the driver's PM
> callbacks? If the CC has multiple parent PM domains, shouldn't we also
> use some of them as GDSC's parents?

It brings up every PM domain in the list

clock_cc {
     power-domains = <somedomain0>, <another-domain>;
};

No different to what the core code does for a single domain - except we 
can actually turn the PDs off with the pm_runtime_put().

Re: [PATCH 2/2] clk: qcom: gdsc: Add pm_runtime hooks

[Dmitry Baryshkov]

On Mon, Nov 18, 2024 at 01:19:49PM +0000, Bryan O'Donoghue wrote:
> On 18/11/2024 13:10, Dmitry Baryshkov wrote:
> > > Introduce pm_runtime_get() and pm_runtime_put_sync() on the
> > > gdsc_toggle_logic().
> > > 
> > > This allows for the switching of the GDSC on/off to propagate to the parent
> > > clock controller and consequently for any list of power-domains powering
> > > that controller to be switched on/off.
> > What is the end result of this patch? Does it bring up a single PM
> > domain or all of them? Or should it be a part of the driver's PM
> > callbacks? If the CC has multiple parent PM domains, shouldn't we also
> > use some of them as GDSC's parents?
> 
> It brings up every PM domain in the list
> 
> clock_cc {
>     power-domains = <somedomain0>, <another-domain>;
> };
> 
> No different to what the core code does for a single domain - except we can
> actually turn the PDs off with the pm_runtime_put().

I see. I missed the device link part of the dev_pm_domain_attach_list().

Just to check, have you checked that this provides no splats in
lockdep-enabled kernels? 
-- 
With best wishes
Dmitry

Re: [PATCH 2/2] clk: qcom: gdsc: Add pm_runtime hooks

[Bryan O'Donoghue]

On 18/11/2024 13:39, Dmitry Baryshkov wrote:
>> It brings up every PM domain in the list
>>
>> clock_cc {
>>      power-domains = <somedomain0>, <another-domain>;
>> };
>>
>> No different to what the core code does for a single domain - except we can
>> actually turn the PDs off with the pm_runtime_put().
> I see. I missed the device link part of the dev_pm_domain_attach_list().
> 
> Just to check, have you checked that this provides no splats in
> lockdep-enabled kernels?

No, I haven't.

I'll have a look at that now. I did test on sc8280xp though.

I'll get back to you on lockdep.

---
bod

Re: [PATCH 2/2] clk: qcom: gdsc: Add pm_runtime hooks

[Bjorn Andersson]

On Mon, Nov 18, 2024 at 02:24:33AM +0000, Bryan O'Donoghue wrote:
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
[..]
> @@ -177,10 +182,14 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status,
>  	ret = gdsc_poll_status(sc, status);
>  	WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
>  
> -	if (!ret && status == GDSC_OFF && sc->rsupply) {
> -		ret = regulator_disable(sc->rsupply);
> -		if (ret < 0)
> -			return ret;
> +	if (!ret && status == GDSC_OFF) {
> +		if (pm_runtime_enabled(sc->dev))
> +			pm_runtime_put_sync(sc->dev);

I already made this mistake, and 4cc47e8add63 ("clk: qcom: gdsc: Remove
direct runtime PM calls") covers the reason why it was a mistake.

What I think you want is two things:
1) When you're accessing the registers, you want the clock controller's
power-domain to be on.
2) When the client vote for a GDSC, you want to have the PM framework
also ensure that parent power-domains are kept on.
For the single case, this is handled by the pm_genpd_add_subdomain()
call below. This, or something along those lines, seems like the
appropriate solution.

Regards,
Bjorn

Re: [PATCH 2/2] clk: qcom: gdsc: Add pm_runtime hooks

[Bryan O'Donoghue]

On 19/11/2024 15:34, Bjorn Andersson wrote:
> What I think you want is two things:
> 1) When you're accessing the registers, you want the clock controller's
> power-domain to be on.
> 2) When the client vote for a GDSC, you want to have the PM framework
> also ensure that parent power-domains are kept on.
> For the single case, this is handled by the pm_genpd_add_subdomain()
> call below. This, or something along those lines, seems like the
> appropriate solution.

Yes.

I'm finding with this patch reverted but, keeping the first patch that 
it pretty much works as you'd want with the caveat that gdsc_register -> 
gdsc_en -> gdsc_toggle fails the first time.

After that I see the GDSCs on/off as excpected

cat /sys/kernel/debug/pm_genpd/cam_cc_titan_top_gdsc/current_state
off-0

cat /sys/kernel/debug/pm_genpd/cam_cc_ife_0_gdsc/current_state
off-0

cam -c 1 --capture=10 --file

cat /sys/kernel/debug/pm_genpd/cam_cc_titan_top_gdsc/current_state
off-0

cat /sys/kernel/debug/pm_genpd/cam_cc_ife_0_gdsc/current_state
off-0

Perhaps we just need to fix the probe path @ gdsc_register()

---
bod

Re: [PATCH 2/2] clk: qcom: gdsc: Add pm_runtime hooks

[Bjorn Andersson]

On Wed, Nov 20, 2024 at 05:09:08PM +0000, Bryan O'Donoghue wrote:
> On 19/11/2024 15:34, Bjorn Andersson wrote:
> > What I think you want is two things:
> > 1) When you're accessing the registers, you want the clock controller's
> > power-domain to be on.
> > 2) When the client vote for a GDSC, you want to have the PM framework
> > also ensure that parent power-domains are kept on.
> > For the single case, this is handled by the pm_genpd_add_subdomain()
> > call below. This, or something along those lines, seems like the
> > appropriate solution.
> 
> Yes.
> 
> I'm finding with this patch reverted but, keeping the first patch that it
> pretty much works as you'd want with the caveat that gdsc_register ->
> gdsc_en -> gdsc_toggle fails the first time.
> 

Can you clarify that call graph for me? The one case I can see where
gdsc_register() leads to gdsc_enable() is if the sc is marked ALWAYS_ON
and I don't think that is your case.

What you describe sounds like we're trying to turn on the power-domain
without first enabling the supplies, or perhaps there are clock
dependencies that are not in order when this is being attempted?

Regards,
Bjorn

> After that I see the GDSCs on/off as excpected
> 
> cat /sys/kernel/debug/pm_genpd/cam_cc_titan_top_gdsc/current_state
> off-0
> 
> cat /sys/kernel/debug/pm_genpd/cam_cc_ife_0_gdsc/current_state
> off-0
> 
> cam -c 1 --capture=10 --file
> 
> cat /sys/kernel/debug/pm_genpd/cam_cc_titan_top_gdsc/current_state
> off-0
> 
> cat /sys/kernel/debug/pm_genpd/cam_cc_ife_0_gdsc/current_state
> off-0
> 
> Perhaps we just need to fix the probe path @ gdsc_register()
> 
> ---
> bod

Re: [PATCH 2/2] clk: qcom: gdsc: Add pm_runtime hooks

[Bryan O'Donoghue]

On 26/11/2024 17:23, Bjorn Andersson wrote:
>> I'm finding with this patch reverted but, keeping the first patch that it
>> pretty much works as you'd want with the caveat that gdsc_register ->
>> gdsc_en -> gdsc_toggle fails the first time.
>>
> Can you clarify that call graph for me? 

Ah no my apologies, that wasn't the call graph I realised about 2 
seconds after sending the mail and never corrected the record.

Please see the v3 of this series instead.

https://lore.kernel.org/lkml/20241126-b4-linux-next-24-11-18-clock-multiple-power-domains-v3-0-836dad33521a@linaro.org/

---
bod

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

[Dmitry Baryshkov]

On Mon, Nov 18, 2024 at 02:24:31AM +0000, Bryan O'Donoghue wrote:
> 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.

I think the series misses the platform-specific part. It is hard to
understand what kind of power relationship do you need to express. Is it
actually the whole CC being powered by several domains? Or are some of
those domains used to power up PLLs? Or as parents to some of GDSCs?

> 
> 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 (2):
>       clk: qcom: common: Add support for power-domain attachment
>       clk: qcom: gdsc: Add pm_runtime hooks
> 
>  drivers/clk/qcom/common.c | 24 ++++++++++++++++++++++++
>  drivers/clk/qcom/gdsc.c   | 26 ++++++++++++++++++--------
>  drivers/clk/qcom/gdsc.h   |  2 ++
>  3 files changed, 44 insertions(+), 8 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>
> 

-- 
With best wishes
Dmitry

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

[Bryan O'Donoghue]

On 18/11/2024 13:15, Dmitry Baryshkov wrote:
>> 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.

> I think the series misses the platform-specific part. 

I don't think I understand what you mean by that.

It is hard to
> understand what kind of power relationship do you need to express. Is it
> actually the whole CC being powered by several domains? Or are some of
> those domains used to power up PLLs? Or as parents to some of GDSCs?

Well for example the TITAN_TOP_GDSC needs both PDs to be switched on.

We _could_ do this just for the CAMCC on x1e80100 - i.e. make it just 
for the camcc device but then, the next clock controller that needs more 
than one PD will have to implement the same fix.

---
bod

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

[Bryan O'Donoghue]

On 18/11/2024 13:22, Bryan O'Donoghue wrote:
> On 18/11/2024 13:15, Dmitry Baryshkov wrote:
>>> 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.
> 
>> I think the series misses the platform-specific part. 
> 
> I don't think I understand what you mean by that.
> 
> It is hard to
>> understand what kind of power relationship do you need to express. Is it
>> actually the whole CC being powered by several domains? Or are some of
>> those domains used to power up PLLs? Or as parents to some of GDSCs?
> 
> Well for example the TITAN_TOP_GDSC needs both PDs to be switched on.
> 
> We _could_ do this just for the CAMCC on x1e80100 - i.e. make it just 
> for the camcc device but then, the next clock controller that needs more 
> than one PD will have to implement the same fix.

Can some of the PLLs run with one PD or the other ?

Perhaps, but without _both_ PDs on the GDSC will stay stuck @ off for my 
test case on x1e80100 and looking at other places we manage PDs directly 
- drivers/media/platform/venus.c for example its generally just all or 
nothing.

There may be more granularity to extract but, I don't think there's more 
granularity for the first case here. Both PDs are needed or the GDSC 
will remain stuck @ off.

As I see it we can either address

1. At the drivers/clk/qcom/camcc-somesoc.c level
2. At drivers/clk/qcom/[gdsc.c|common.c] level

I think option 2 is more how you'd expect this to work though. Its not 
particularly obvious but ATM with singleton power-domains for the clock 
controllers the singletons just get turned on and left that way.

So managing the PD on/off inside of common.c/gdsc.c means the calling 
clock controller can stay as a simple probe() type driver and also means 
we can reuse the generic common.c/gdsc.c approach.

---
bod

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

[Taniya Das]

On 11/18/2024 7:54 AM, Bryan O'Donoghue wrote:
> 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, as we were already in discussion with Bjorn to post the patches 
which take care of Multi GDSC and PLL requirements, I would request to 
kindly hold this series posting. I am in the final discussions with 
Bjorn to handle it gracefully to post the series.

> Bryan O'Donoghue (2):
>        clk: qcom: common: Add support for power-domain attachment
>        clk: qcom: gdsc: Add pm_runtime hooks
> 
>   drivers/clk/qcom/common.c | 24 ++++++++++++++++++++++++
>   drivers/clk/qcom/gdsc.c   | 26 ++++++++++++++++++--------
>   drivers/clk/qcom/gdsc.h   |  2 ++
>   3 files changed, 44 insertions(+), 8 deletions(-)
> ---
> base-commit: 744cf71b8bdfcdd77aaf58395e068b7457634b2c
> change-id: 20241118-b4-linux-next-24-11-18-clock-multiple-power-domains-a5f994dc452a
> 
> Best regards,

-- 
Thanks & Regards,
Taniya Das.

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

[Bryan O'Donoghue]

On 19/11/2024 06:08, Taniya Das wrote:
> 
> Bryan, as we were already in discussion with Bjorn to post the patches 
> which take care of Multi GDSC and PLL requirements, I would request to 
> kindly hold this series posting. I am in the final discussions with 
> Bjorn to handle it gracefully to post the series.

Is this not 'graceful' ?

Hmm. What specifically don't you like about this series ?

---
bod

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

[Bjorn Andersson]

On Tue, Nov 19, 2024 at 11:38:34AM +0530, Taniya Das wrote:
> 
> 
> On 11/18/2024 7:54 AM, Bryan O'Donoghue wrote:
> > 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, as we were already in discussion with Bjorn to post the patches which
> take care of Multi GDSC and PLL requirements, I would request to kindly hold
> this series posting.

There's no "hold before posting", this series is already posted.
Please review it.

> I am in the final discussions with Bjorn to handle it
> gracefully to post the series.
> 

You may in such discussion (the review) say "you're missing X, Y, Z, and
here is my patches that covers these aspects", but not "I'll ignore this
until we're done preparing our patches".

Regards,
Bjorn