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

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
>

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

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

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

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

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

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

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.