Add support to configure PLLS and clk registers in qcom_cc_really_probe().
This ensures all required power domains are enabled and kept ON by runtime
PM code in qcom_cc_really_probe() before configuring the PLLS or clock
registers.

Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
 drivers/clk/qcom/common.c | 28 ++++++++++++++++++++++++++++
 drivers/clk/qcom/common.h | 19 +++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 9cbf1c5296dad3ee5477a2f5a445488707663b9d..c4d980c6145834969fada14863360ee81c9aa251 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -14,6 +14,8 @@
 #include <linux/of.h>
 
 #include "common.h"
+#include "clk-alpha-pll.h"
+#include "clk-branch.h"
 #include "clk-rcg.h"
 #include "clk-regmap.h"
 #include "reset.h"
@@ -285,6 +287,29 @@ static int qcom_cc_icc_register(struct device *dev,
 						     desc->num_icc_hws, icd);
 }
 
+static void qcom_cc_clk_pll_configure(const struct qcom_cc_desc *desc,
+				      struct regmap *regmap)
+{
+	int i;
+
+	for (i = 0; i < desc->num_alpha_plls; i++)
+		qcom_clk_alpha_pll_configure(desc->alpha_plls[i], regmap);
+}
+
+static void qcom_cc_clk_regs_configure(const struct qcom_cc_desc *desc,
+				       struct regmap *regmap)
+{
+	struct qcom_clk_reg_setting *clk_regs = desc->clk_regs;
+	int i;
+
+	for (i = 0; i < desc->num_clk_cbcrs; i++)
+		qcom_branch_set_clk_en(regmap, desc->clk_cbcrs[i]);
+
+	for (i = 0 ; i < desc->num_clk_regs; i++)
+		regmap_update_bits(regmap, clk_regs[i].offset,
+				   clk_regs[i].mask, clk_regs[i].val);
+}
+
 int qcom_cc_really_probe(struct device *dev,
 			 const struct qcom_cc_desc *desc, struct regmap *regmap)
 {
@@ -315,6 +340,9 @@ int qcom_cc_really_probe(struct device *dev,
 			return ret;
 	}
 
+	qcom_cc_clk_pll_configure(desc, regmap);
+	qcom_cc_clk_regs_configure(desc, regmap);
+
 	reset = &cc->reset;
 	reset->rcdev.of_node = dev->of_node;
 	reset->rcdev.ops = &qcom_reset_ops;
diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
index 9c10bc8c197cd7dfa25ccd245763ad6acb081523..01b1ae52f2dc580350409d6244578944cce571f0 100644
--- a/drivers/clk/qcom/common.h
+++ b/drivers/clk/qcom/common.h
@@ -25,6 +25,19 @@ struct qcom_icc_hws_data {
 	int clk_id;
 };
 
+/**
+ * struct qcom_clk_reg_setting - Represents miscellaneous clock register settings
+ * @offset: address offset for the clock register
+ * @mask: bit mask indicating the bits to be updated
+ * @val: Encoded value to be set within the specified bit mask
+ *       (e.g., if writing 7 to bits 4-7, mask = 0xF0 and val = 0x70)
+ */
+struct qcom_clk_reg_setting {
+	u32 offset;
+	u32 mask;
+	u32 val;
+};
+
 struct qcom_cc_desc {
 	const struct regmap_config *config;
 	struct clk_regmap **clks;
@@ -38,6 +51,12 @@ struct qcom_cc_desc {
 	const struct qcom_icc_hws_data *icc_hws;
 	size_t num_icc_hws;
 	unsigned int icc_first_node_id;
+	u32 *clk_cbcrs;
+	size_t num_clk_cbcrs;
+	struct clk_alpha_pll **alpha_plls;
+	size_t num_alpha_plls;
+	struct qcom_clk_reg_setting *clk_regs;
+	size_t num_clk_regs;
 	bool use_rpm;
 };
 

-- 
2.34.1

On Thu, Mar 27, 2025 at 03:22:26PM +0530, Jagadeesh Kona wrote:
> Add support to configure PLLS and clk registers in qcom_cc_really_probe().
> This ensures all required power domains are enabled and kept ON by runtime
> PM code in qcom_cc_really_probe() before configuring the PLLS or clock
> registers.
> 
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> ---
>  drivers/clk/qcom/common.c | 28 ++++++++++++++++++++++++++++
>  drivers/clk/qcom/common.h | 19 +++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 9cbf1c5296dad3ee5477a2f5a445488707663b9d..c4d980c6145834969fada14863360ee81c9aa251 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -14,6 +14,8 @@
>  #include <linux/of.h>
>  
>  #include "common.h"
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
>  #include "clk-rcg.h"
>  #include "clk-regmap.h"
>  #include "reset.h"
> @@ -285,6 +287,29 @@ static int qcom_cc_icc_register(struct device *dev,
>  						     desc->num_icc_hws, icd);
>  }
>  
> +static void qcom_cc_clk_pll_configure(const struct qcom_cc_desc *desc,
> +				      struct regmap *regmap)
> +{
> +	int i;
> +
> +	for (i = 0; i < desc->num_alpha_plls; i++)
> +		qcom_clk_alpha_pll_configure(desc->alpha_plls[i], regmap);
> +}
> +
> +static void qcom_cc_clk_regs_configure(const struct qcom_cc_desc *desc,
> +				       struct regmap *regmap)
> +{
> +	struct qcom_clk_reg_setting *clk_regs = desc->clk_regs;
> +	int i;
> +
> +	for (i = 0; i < desc->num_clk_cbcrs; i++)
> +		qcom_branch_set_clk_en(regmap, desc->clk_cbcrs[i]);
> +
> +	for (i = 0 ; i < desc->num_clk_regs; i++)
> +		regmap_update_bits(regmap, clk_regs[i].offset,
> +				   clk_regs[i].mask, clk_regs[i].val);

I think there are other semantic functions which we don't want to
convert to offset-mask-val tuples. See drivers/clk/qcom/clk-branch.h.
I'd suggest to move setup steps to a driver callback. We can improve it
later on if it is found to make sense, but it won't block this series
from being merged.

> +}
> +
>  int qcom_cc_really_probe(struct device *dev,
>  			 const struct qcom_cc_desc *desc, struct regmap *regmap)
>  {
> @@ -315,6 +340,9 @@ int qcom_cc_really_probe(struct device *dev,
>  			return ret;
>  	}
>  
> +	qcom_cc_clk_pll_configure(desc, regmap);
> +	qcom_cc_clk_regs_configure(desc, regmap);
> +
>  	reset = &cc->reset;
>  	reset->rcdev.of_node = dev->of_node;
>  	reset->rcdev.ops = &qcom_reset_ops;
> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> index 9c10bc8c197cd7dfa25ccd245763ad6acb081523..01b1ae52f2dc580350409d6244578944cce571f0 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> @@ -25,6 +25,19 @@ struct qcom_icc_hws_data {
>  	int clk_id;
>  };
>  
> +/**
> + * struct qcom_clk_reg_setting - Represents miscellaneous clock register settings
> + * @offset: address offset for the clock register
> + * @mask: bit mask indicating the bits to be updated
> + * @val: Encoded value to be set within the specified bit mask
> + *       (e.g., if writing 7 to bits 4-7, mask = 0xF0 and val = 0x70)
> + */
> +struct qcom_clk_reg_setting {
> +	u32 offset;
> +	u32 mask;
> +	u32 val;
> +};
> +
>  struct qcom_cc_desc {
>  	const struct regmap_config *config;
>  	struct clk_regmap **clks;
> @@ -38,6 +51,12 @@ struct qcom_cc_desc {
>  	const struct qcom_icc_hws_data *icc_hws;
>  	size_t num_icc_hws;
>  	unsigned int icc_first_node_id;
> +	u32 *clk_cbcrs;
> +	size_t num_clk_cbcrs;
> +	struct clk_alpha_pll **alpha_plls;
> +	size_t num_alpha_plls;
> +	struct qcom_clk_reg_setting *clk_regs;
> +	size_t num_clk_regs;
>  	bool use_rpm;
>  };
>  
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

On 3/27/2025 6:20 PM, Dmitry Baryshkov wrote:
> On Thu, Mar 27, 2025 at 03:22:26PM +0530, Jagadeesh Kona wrote:
>> Add support to configure PLLS and clk registers in qcom_cc_really_probe().
>> This ensures all required power domains are enabled and kept ON by runtime
>> PM code in qcom_cc_really_probe() before configuring the PLLS or clock
>> registers.
>>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> ---
>>  drivers/clk/qcom/common.c | 28 ++++++++++++++++++++++++++++
>>  drivers/clk/qcom/common.h | 19 +++++++++++++++++++
>>  2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>> index 9cbf1c5296dad3ee5477a2f5a445488707663b9d..c4d980c6145834969fada14863360ee81c9aa251 100644
>> --- a/drivers/clk/qcom/common.c
>> +++ b/drivers/clk/qcom/common.c
>> @@ -14,6 +14,8 @@
>>  #include <linux/of.h>
>>  
>>  #include "common.h"
>> +#include "clk-alpha-pll.h"
>> +#include "clk-branch.h"
>>  #include "clk-rcg.h"
>>  #include "clk-regmap.h"
>>  #include "reset.h"
>> @@ -285,6 +287,29 @@ static int qcom_cc_icc_register(struct device *dev,
>>  						     desc->num_icc_hws, icd);
>>  }
>>  
>> +static void qcom_cc_clk_pll_configure(const struct qcom_cc_desc *desc,
>> +				      struct regmap *regmap)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < desc->num_alpha_plls; i++)
>> +		qcom_clk_alpha_pll_configure(desc->alpha_plls[i], regmap);
>> +}
>> +
>> +static void qcom_cc_clk_regs_configure(const struct qcom_cc_desc *desc,
>> +				       struct regmap *regmap)
>> +{
>> +	struct qcom_clk_reg_setting *clk_regs = desc->clk_regs;
>> +	int i;
>> +
>> +	for (i = 0; i < desc->num_clk_cbcrs; i++)
>> +		qcom_branch_set_clk_en(regmap, desc->clk_cbcrs[i]);
>> +
>> +	for (i = 0 ; i < desc->num_clk_regs; i++)
>> +		regmap_update_bits(regmap, clk_regs[i].offset,
>> +				   clk_regs[i].mask, clk_regs[i].val);
> 
> I think there are other semantic functions which we don't want to
> convert to offset-mask-val tuples. See drivers/clk/qcom/clk-branch.h.
> I'd suggest to move setup steps to a driver callback. We can improve it
> later on if it is found to make sense, but it won't block this series
> from being merged.
> 

Yes, there are other wrapper functions as well but they are unused in most
clock controllers. We will check more on how we can improve this in a separate
series.

Thanks,
Jagadeesh

>> +}
>> +
>>  int qcom_cc_really_probe(struct device *dev,
>>  			 const struct qcom_cc_desc *desc, struct regmap *regmap)
>>  {
>> @@ -315,6 +340,9 @@ int qcom_cc_really_probe(struct device *dev,
>>  			return ret;
>>  	}
>>  
>> +	qcom_cc_clk_pll_configure(desc, regmap);
>> +	qcom_cc_clk_regs_configure(desc, regmap);
>> +
>>  	reset = &cc->reset;
>>  	reset->rcdev.of_node = dev->of_node;
>>  	reset->rcdev.ops = &qcom_reset_ops;
>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>> index 9c10bc8c197cd7dfa25ccd245763ad6acb081523..01b1ae52f2dc580350409d6244578944cce571f0 100644
>> --- a/drivers/clk/qcom/common.h
>> +++ b/drivers/clk/qcom/common.h
>> @@ -25,6 +25,19 @@ struct qcom_icc_hws_data {
>>  	int clk_id;
>>  };
>>  
>> +/**
>> + * struct qcom_clk_reg_setting - Represents miscellaneous clock register settings
>> + * @offset: address offset for the clock register
>> + * @mask: bit mask indicating the bits to be updated
>> + * @val: Encoded value to be set within the specified bit mask
>> + *       (e.g., if writing 7 to bits 4-7, mask = 0xF0 and val = 0x70)
>> + */
>> +struct qcom_clk_reg_setting {
>> +	u32 offset;
>> +	u32 mask;
>> +	u32 val;
>> +};
>> +
>>  struct qcom_cc_desc {
>>  	const struct regmap_config *config;
>>  	struct clk_regmap **clks;
>> @@ -38,6 +51,12 @@ struct qcom_cc_desc {
>>  	const struct qcom_icc_hws_data *icc_hws;
>>  	size_t num_icc_hws;
>>  	unsigned int icc_first_node_id;
>> +	u32 *clk_cbcrs;
>> +	size_t num_clk_cbcrs;
>> +	struct clk_alpha_pll **alpha_plls;
>> +	size_t num_alpha_plls;
>> +	struct qcom_clk_reg_setting *clk_regs;
>> +	size_t num_clk_regs;
>>  	bool use_rpm;
>>  };
>>  
>>
>> -- 
>> 2.34.1
>>
>

On Fri, 11 Apr 2025 at 10:14, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>
>
>
> On 3/27/2025 6:20 PM, Dmitry Baryshkov wrote:
> > On Thu, Mar 27, 2025 at 03:22:26PM +0530, Jagadeesh Kona wrote:
> >> Add support to configure PLLS and clk registers in qcom_cc_really_probe().
> >> This ensures all required power domains are enabled and kept ON by runtime
> >> PM code in qcom_cc_really_probe() before configuring the PLLS or clock
> >> registers.
> >>
> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> >> ---
> >>  drivers/clk/qcom/common.c | 28 ++++++++++++++++++++++++++++
> >>  drivers/clk/qcom/common.h | 19 +++++++++++++++++++
> >>  2 files changed, 47 insertions(+)
> >>
> >> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> >> index 9cbf1c5296dad3ee5477a2f5a445488707663b9d..c4d980c6145834969fada14863360ee81c9aa251 100644
> >> --- a/drivers/clk/qcom/common.c
> >> +++ b/drivers/clk/qcom/common.c
> >> @@ -14,6 +14,8 @@
> >>  #include <linux/of.h>
> >>
> >>  #include "common.h"
> >> +#include "clk-alpha-pll.h"
> >> +#include "clk-branch.h"
> >>  #include "clk-rcg.h"
> >>  #include "clk-regmap.h"
> >>  #include "reset.h"
> >> @@ -285,6 +287,29 @@ static int qcom_cc_icc_register(struct device *dev,
> >>                                                   desc->num_icc_hws, icd);
> >>  }
> >>
> >> +static void qcom_cc_clk_pll_configure(const struct qcom_cc_desc *desc,
> >> +                                  struct regmap *regmap)
> >> +{
> >> +    int i;
> >> +
> >> +    for (i = 0; i < desc->num_alpha_plls; i++)
> >> +            qcom_clk_alpha_pll_configure(desc->alpha_plls[i], regmap);
> >> +}
> >> +
> >> +static void qcom_cc_clk_regs_configure(const struct qcom_cc_desc *desc,
> >> +                                   struct regmap *regmap)
> >> +{
> >> +    struct qcom_clk_reg_setting *clk_regs = desc->clk_regs;
> >> +    int i;
> >> +
> >> +    for (i = 0; i < desc->num_clk_cbcrs; i++)
> >> +            qcom_branch_set_clk_en(regmap, desc->clk_cbcrs[i]);
> >> +
> >> +    for (i = 0 ; i < desc->num_clk_regs; i++)
> >> +            regmap_update_bits(regmap, clk_regs[i].offset,
> >> +                               clk_regs[i].mask, clk_regs[i].val);
> >
> > I think there are other semantic functions which we don't want to
> > convert to offset-mask-val tuples. See drivers/clk/qcom/clk-branch.h.
> > I'd suggest to move setup steps to a driver callback. We can improve it
> > later on if it is found to make sense, but it won't block this series
> > from being merged.
> >
>
> Yes, there are other wrapper functions as well but they are unused in most
> clock controllers. We will check more on how we can improve this in a separate
> series.

Please do it the other way around. Implement a generic callback, then
we can check how to sort things out.

>
> Thanks,
> Jagadeesh
>
> >> +}
> >> +
> >>  int qcom_cc_really_probe(struct device *dev,
> >>                       const struct qcom_cc_desc *desc, struct regmap *regmap)
> >>  {
> >> @@ -315,6 +340,9 @@ int qcom_cc_really_probe(struct device *dev,
> >>                      return ret;
> >>      }
> >>
> >> +    qcom_cc_clk_pll_configure(desc, regmap);
> >> +    qcom_cc_clk_regs_configure(desc, regmap);
> >> +
> >>      reset = &cc->reset;
> >>      reset->rcdev.of_node = dev->of_node;
> >>      reset->rcdev.ops = &qcom_reset_ops;
> >> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> >> index 9c10bc8c197cd7dfa25ccd245763ad6acb081523..01b1ae52f2dc580350409d6244578944cce571f0 100644
> >> --- a/drivers/clk/qcom/common.h
> >> +++ b/drivers/clk/qcom/common.h
> >> @@ -25,6 +25,19 @@ struct qcom_icc_hws_data {
> >>      int clk_id;
> >>  };
> >>
> >> +/**
> >> + * struct qcom_clk_reg_setting - Represents miscellaneous clock register settings
> >> + * @offset: address offset for the clock register
> >> + * @mask: bit mask indicating the bits to be updated
> >> + * @val: Encoded value to be set within the specified bit mask
> >> + *       (e.g., if writing 7 to bits 4-7, mask = 0xF0 and val = 0x70)
> >> + */
> >> +struct qcom_clk_reg_setting {
> >> +    u32 offset;
> >> +    u32 mask;
> >> +    u32 val;
> >> +};
> >> +
> >>  struct qcom_cc_desc {
> >>      const struct regmap_config *config;
> >>      struct clk_regmap **clks;
> >> @@ -38,6 +51,12 @@ struct qcom_cc_desc {
> >>      const struct qcom_icc_hws_data *icc_hws;
> >>      size_t num_icc_hws;
> >>      unsigned int icc_first_node_id;
> >> +    u32 *clk_cbcrs;
> >> +    size_t num_clk_cbcrs;
> >> +    struct clk_alpha_pll **alpha_plls;
> >> +    size_t num_alpha_plls;
> >> +    struct qcom_clk_reg_setting *clk_regs;
> >> +    size_t num_clk_regs;
> >>      bool use_rpm;
> >>  };
> >>
> >>
> >> --
> >> 2.34.1
> >>
> >



-- 
With best wishes
Dmitry

On 4/11/2025 2:21 PM, Dmitry Baryshkov wrote:
> On Fri, 11 Apr 2025 at 10:14, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>>
>>
>>
>> On 3/27/2025 6:20 PM, Dmitry Baryshkov wrote:
>>> On Thu, Mar 27, 2025 at 03:22:26PM +0530, Jagadeesh Kona wrote:
>>>> Add support to configure PLLS and clk registers in qcom_cc_really_probe().
>>>> This ensures all required power domains are enabled and kept ON by runtime
>>>> PM code in qcom_cc_really_probe() before configuring the PLLS or clock
>>>> registers.
>>>>
>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>> ---
>>>>  drivers/clk/qcom/common.c | 28 ++++++++++++++++++++++++++++
>>>>  drivers/clk/qcom/common.h | 19 +++++++++++++++++++
>>>>  2 files changed, 47 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>>> index 9cbf1c5296dad3ee5477a2f5a445488707663b9d..c4d980c6145834969fada14863360ee81c9aa251 100644
>>>> --- a/drivers/clk/qcom/common.c
>>>> +++ b/drivers/clk/qcom/common.c
>>>> @@ -14,6 +14,8 @@
>>>>  #include <linux/of.h>
>>>>
>>>>  #include "common.h"
>>>> +#include "clk-alpha-pll.h"
>>>> +#include "clk-branch.h"
>>>>  #include "clk-rcg.h"
>>>>  #include "clk-regmap.h"
>>>>  #include "reset.h"
>>>> @@ -285,6 +287,29 @@ static int qcom_cc_icc_register(struct device *dev,
>>>>                                                   desc->num_icc_hws, icd);
>>>>  }
>>>>
>>>> +static void qcom_cc_clk_pll_configure(const struct qcom_cc_desc *desc,
>>>> +                                  struct regmap *regmap)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < desc->num_alpha_plls; i++)
>>>> +            qcom_clk_alpha_pll_configure(desc->alpha_plls[i], regmap);
>>>> +}
>>>> +
>>>> +static void qcom_cc_clk_regs_configure(const struct qcom_cc_desc *desc,
>>>> +                                   struct regmap *regmap)
>>>> +{
>>>> +    struct qcom_clk_reg_setting *clk_regs = desc->clk_regs;
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < desc->num_clk_cbcrs; i++)
>>>> +            qcom_branch_set_clk_en(regmap, desc->clk_cbcrs[i]);
>>>> +
>>>> +    for (i = 0 ; i < desc->num_clk_regs; i++)
>>>> +            regmap_update_bits(regmap, clk_regs[i].offset,
>>>> +                               clk_regs[i].mask, clk_regs[i].val);
>>>
>>> I think there are other semantic functions which we don't want to
>>> convert to offset-mask-val tuples. See drivers/clk/qcom/clk-branch.h.
>>> I'd suggest to move setup steps to a driver callback. We can improve it
>>> later on if it is found to make sense, but it won't block this series
>>> from being merged.
>>>
>>
>> Yes, there are other wrapper functions as well but they are unused in most
>> clock controllers. We will check more on how we can improve this in a separate
>> series.
> 
> Please do it the other way around. Implement a generic callback, then
> we can check how to sort things out.
> 

Yeah, but since this series doesn't require any misc register settings update, I
will remove the above regmap_update_bits() code for now. I will check further on
this and post a separate series for it.

Thanks,
Jagadeesh

>>
>> Thanks,
>> Jagadeesh
>>
>>>> +}
>>>> +
>>>>  int qcom_cc_really_probe(struct device *dev,
>>>>                       const struct qcom_cc_desc *desc, struct regmap *regmap)
>>>>  {
>>>> @@ -315,6 +340,9 @@ int qcom_cc_really_probe(struct device *dev,
>>>>                      return ret;
>>>>      }
>>>>
>>>> +    qcom_cc_clk_pll_configure(desc, regmap);
>>>> +    qcom_cc_clk_regs_configure(desc, regmap);
>>>> +
>>>>      reset = &cc->reset;
>>>>      reset->rcdev.of_node = dev->of_node;
>>>>      reset->rcdev.ops = &qcom_reset_ops;
>>>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>>>> index 9c10bc8c197cd7dfa25ccd245763ad6acb081523..01b1ae52f2dc580350409d6244578944cce571f0 100644
>>>> --- a/drivers/clk/qcom/common.h
>>>> +++ b/drivers/clk/qcom/common.h
>>>> @@ -25,6 +25,19 @@ struct qcom_icc_hws_data {
>>>>      int clk_id;
>>>>  };
>>>>
>>>> +/**
>>>> + * struct qcom_clk_reg_setting - Represents miscellaneous clock register settings
>>>> + * @offset: address offset for the clock register
>>>> + * @mask: bit mask indicating the bits to be updated
>>>> + * @val: Encoded value to be set within the specified bit mask
>>>> + *       (e.g., if writing 7 to bits 4-7, mask = 0xF0 and val = 0x70)
>>>> + */
>>>> +struct qcom_clk_reg_setting {
>>>> +    u32 offset;
>>>> +    u32 mask;
>>>> +    u32 val;
>>>> +};
>>>> +
>>>>  struct qcom_cc_desc {
>>>>      const struct regmap_config *config;
>>>>      struct clk_regmap **clks;
>>>> @@ -38,6 +51,12 @@ struct qcom_cc_desc {
>>>>      const struct qcom_icc_hws_data *icc_hws;
>>>>      size_t num_icc_hws;
>>>>      unsigned int icc_first_node_id;
>>>> +    u32 *clk_cbcrs;
>>>> +    size_t num_clk_cbcrs;
>>>> +    struct clk_alpha_pll **alpha_plls;
>>>> +    size_t num_alpha_plls;
>>>> +    struct qcom_clk_reg_setting *clk_regs;
>>>> +    size_t num_clk_regs;
>>>>      bool use_rpm;
>>>>  };
>>>>
>>>>
>>>> --
>>>> 2.34.1
>>>>
>>>
> 
> 
>

On Mon, Apr 14, 2025 at 03:39:04PM +0530, Jagadeesh Kona wrote:
> 
> 
> On 4/11/2025 2:21 PM, Dmitry Baryshkov wrote:
> > On Fri, 11 Apr 2025 at 10:14, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 3/27/2025 6:20 PM, Dmitry Baryshkov wrote:
> >>> On Thu, Mar 27, 2025 at 03:22:26PM +0530, Jagadeesh Kona wrote:
> >>>> Add support to configure PLLS and clk registers in qcom_cc_really_probe().
> >>>> This ensures all required power domains are enabled and kept ON by runtime
> >>>> PM code in qcom_cc_really_probe() before configuring the PLLS or clock
> >>>> registers.
> >>>>
> >>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> >>>> ---
> >>>>  drivers/clk/qcom/common.c | 28 ++++++++++++++++++++++++++++
> >>>>  drivers/clk/qcom/common.h | 19 +++++++++++++++++++
> >>>>  2 files changed, 47 insertions(+)
> >>>>
> >>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> >>>> index 9cbf1c5296dad3ee5477a2f5a445488707663b9d..c4d980c6145834969fada14863360ee81c9aa251 100644
> >>>> --- a/drivers/clk/qcom/common.c
> >>>> +++ b/drivers/clk/qcom/common.c
> >>>> @@ -14,6 +14,8 @@
> >>>>  #include <linux/of.h>
> >>>>
> >>>>  #include "common.h"
> >>>> +#include "clk-alpha-pll.h"
> >>>> +#include "clk-branch.h"
> >>>>  #include "clk-rcg.h"
> >>>>  #include "clk-regmap.h"
> >>>>  #include "reset.h"
> >>>> @@ -285,6 +287,29 @@ static int qcom_cc_icc_register(struct device *dev,
> >>>>                                                   desc->num_icc_hws, icd);
> >>>>  }
> >>>>
> >>>> +static void qcom_cc_clk_pll_configure(const struct qcom_cc_desc *desc,
> >>>> +                                  struct regmap *regmap)
> >>>> +{
> >>>> +    int i;
> >>>> +
> >>>> +    for (i = 0; i < desc->num_alpha_plls; i++)
> >>>> +            qcom_clk_alpha_pll_configure(desc->alpha_plls[i], regmap);
> >>>> +}
> >>>> +
> >>>> +static void qcom_cc_clk_regs_configure(const struct qcom_cc_desc *desc,
> >>>> +                                   struct regmap *regmap)
> >>>> +{
> >>>> +    struct qcom_clk_reg_setting *clk_regs = desc->clk_regs;
> >>>> +    int i;
> >>>> +
> >>>> +    for (i = 0; i < desc->num_clk_cbcrs; i++)
> >>>> +            qcom_branch_set_clk_en(regmap, desc->clk_cbcrs[i]);
> >>>> +
> >>>> +    for (i = 0 ; i < desc->num_clk_regs; i++)
> >>>> +            regmap_update_bits(regmap, clk_regs[i].offset,
> >>>> +                               clk_regs[i].mask, clk_regs[i].val);
> >>>
> >>> I think there are other semantic functions which we don't want to
> >>> convert to offset-mask-val tuples. See drivers/clk/qcom/clk-branch.h.
> >>> I'd suggest to move setup steps to a driver callback. We can improve it
> >>> later on if it is found to make sense, but it won't block this series
> >>> from being merged.
> >>>
> >>
> >> Yes, there are other wrapper functions as well but they are unused in most
> >> clock controllers. We will check more on how we can improve this in a separate
> >> series.
> > 
> > Please do it the other way around. Implement a generic callback, then
> > we can check how to sort things out.
> > 
> 
> Yeah, but since this series doesn't require any misc register settings update, I
> will remove the above regmap_update_bits() code for now. I will check further on
> this and post a separate series for it.

This way we end up with a non-generic solution with no proposed path.
I'd really suggest using a callback for now to configure all
platform-specific and then post a separte series adding clk_cbcrs and
related functionality.

> 
> Thanks,
> Jagadeesh
> 
> >>
> >> Thanks,
> >> Jagadeesh
> >>
> >>>> +}
> >>>> +
> >>>>  int qcom_cc_really_probe(struct device *dev,
> >>>>                       const struct qcom_cc_desc *desc, struct regmap *regmap)
> >>>>  {
> >>>> @@ -315,6 +340,9 @@ int qcom_cc_really_probe(struct device *dev,
> >>>>                      return ret;
> >>>>      }
> >>>>
> >>>> +    qcom_cc_clk_pll_configure(desc, regmap);
> >>>> +    qcom_cc_clk_regs_configure(desc, regmap);
> >>>> +
> >>>>      reset = &cc->reset;
> >>>>      reset->rcdev.of_node = dev->of_node;
> >>>>      reset->rcdev.ops = &qcom_reset_ops;
> >>>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> >>>> index 9c10bc8c197cd7dfa25ccd245763ad6acb081523..01b1ae52f2dc580350409d6244578944cce571f0 100644
> >>>> --- a/drivers/clk/qcom/common.h
> >>>> +++ b/drivers/clk/qcom/common.h
> >>>> @@ -25,6 +25,19 @@ struct qcom_icc_hws_data {
> >>>>      int clk_id;
> >>>>  };
> >>>>
> >>>> +/**
> >>>> + * struct qcom_clk_reg_setting - Represents miscellaneous clock register settings
> >>>> + * @offset: address offset for the clock register
> >>>> + * @mask: bit mask indicating the bits to be updated
> >>>> + * @val: Encoded value to be set within the specified bit mask
> >>>> + *       (e.g., if writing 7 to bits 4-7, mask = 0xF0 and val = 0x70)
> >>>> + */
> >>>> +struct qcom_clk_reg_setting {
> >>>> +    u32 offset;
> >>>> +    u32 mask;
> >>>> +    u32 val;
> >>>> +};
> >>>> +
> >>>>  struct qcom_cc_desc {
> >>>>      const struct regmap_config *config;
> >>>>      struct clk_regmap **clks;
> >>>> @@ -38,6 +51,12 @@ struct qcom_cc_desc {
> >>>>      const struct qcom_icc_hws_data *icc_hws;
> >>>>      size_t num_icc_hws;
> >>>>      unsigned int icc_first_node_id;
> >>>> +    u32 *clk_cbcrs;
> >>>> +    size_t num_clk_cbcrs;
> >>>> +    struct clk_alpha_pll **alpha_plls;
> >>>> +    size_t num_alpha_plls;
> >>>> +    struct qcom_clk_reg_setting *clk_regs;
> >>>> +    size_t num_clk_regs;
> >>>>      bool use_rpm;
> >>>>  };
> >>>>
> >>>>
> >>>> --
> >>>> 2.34.1
> >>>>
> >>>
> > 
> > 
> > 

-- 
With best wishes
Dmitry