diff mbox series

[1/4] clk: qcom: clk-alpha-pll: Add support for lucid ole pll ops

Message ID 20230509161218.11979-2-quic_jkona@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Add Video Clock Controller driver for SM8550 | expand

Commit Message

Jagadeesh Kona May 9, 2023, 4:12 p.m. UTC 
From: Taniya Das <quic_tdas@quicinc.com>

Add support for lucid ole pll ops to configure and control the
lucid ole pll. The lucid ole pll has an additional test control
register which is required to be programmed, add support to
program the same.

Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
 drivers/clk/qcom/clk-alpha-pll.c | 2 ++
 drivers/clk/qcom/clk-alpha-pll.h | 4 ++++
 2 files changed, 6 insertions(+)

Comments

Konrad Dybcio May 9, 2023, 8:06 p.m. UTC | #1 
On 9.05.2023 18:12, Jagadeesh Kona wrote:
> From: Taniya Das <quic_tdas@quicinc.com>
> 
> Add support for lucid ole pll ops to configure and control the
> lucid ole pll. The lucid ole pll has an additional test control
> register which is required to be programmed, add support to
> program the same.
> 
> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> ---
Isn't this commit "write to PLL_TEST_CTL_U2 on LUCID_EVO" instead?

Meaninglessly duplicating ops does not seem useful.

Konrad
>  drivers/clk/qcom/clk-alpha-pll.c | 2 ++
>  drivers/clk/qcom/clk-alpha-pll.h | 4 ++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index b9f6535a7ba7..f81c7c561352 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -55,6 +55,7 @@
>  #define PLL_TEST_CTL(p)		((p)->offset + (p)->regs[PLL_OFF_TEST_CTL])
>  #define PLL_TEST_CTL_U(p)	((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U])
>  #define PLL_TEST_CTL_U1(p)     ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U1])
> +#define PLL_TEST_CTL_U2(p)     ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U2])
>  #define PLL_STATUS(p)		((p)->offset + (p)->regs[PLL_OFF_STATUS])
>  #define PLL_OPMODE(p)		((p)->offset + (p)->regs[PLL_OFF_OPMODE])
>  #define PLL_FRAC(p)		((p)->offset + (p)->regs[PLL_OFF_FRAC])
> @@ -2096,6 +2097,7 @@ void clk_lucid_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regma
>  	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL(pll), config->test_ctl_val);
>  	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U(pll), config->test_ctl_hi_val);
>  	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U1(pll), config->test_ctl_hi1_val);
> +	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U2(pll), config->test_ctl_hi2_val);
>  
>  	/* Disable PLL output */
>  	regmap_update_bits(regmap, PLL_MODE(pll), PLL_OUTCTRL, 0);
> diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
> index d07b17186b90..4d9b6d5b7062 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.h
> +++ b/drivers/clk/qcom/clk-alpha-pll.h
> @@ -125,6 +125,7 @@ struct alpha_pll_config {
>  	u32 test_ctl_val;
>  	u32 test_ctl_hi_val;
>  	u32 test_ctl_hi1_val;
> +	u32 test_ctl_hi2_val;
>  	u32 main_output_mask;
>  	u32 aux_output_mask;
>  	u32 aux2_output_mask;
> @@ -171,6 +172,7 @@ extern const struct clk_ops clk_alpha_pll_zonda_ops;
>  #define clk_alpha_pll_postdiv_zonda_ops clk_alpha_pll_postdiv_fabia_ops
>  
>  extern const struct clk_ops clk_alpha_pll_lucid_evo_ops;
> +#define clk_alpha_pll_lucid_ole_ops clk_alpha_pll_lucid_evo_ops
>  extern const struct clk_ops clk_alpha_pll_reset_lucid_evo_ops;
>  #define clk_alpha_pll_reset_lucid_ole_ops clk_alpha_pll_reset_lucid_evo_ops
>  extern const struct clk_ops clk_alpha_pll_fixed_lucid_evo_ops;
> @@ -196,6 +198,8 @@ void clk_zonda_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>  			     const struct alpha_pll_config *config);
>  void clk_lucid_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>  				 const struct alpha_pll_config *config);
> +#define clk_lucid_ole_pll_configure(pll, regmap, config) \
> +			clk_lucid_evo_pll_configure(pll, regmap, config)
>  void clk_rivian_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>  				  const struct alpha_pll_config *config);
>  void clk_stromer_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
Jagadeesh Kona May 19, 2023, 12:49 p.m. UTC | #2 
Hi,

Thanks Konrad for your review!

On 5/10/2023 1:36 AM, Konrad Dybcio wrote:
> 
> 
> On 9.05.2023 18:12, Jagadeesh Kona wrote:
>> From: Taniya Das <quic_tdas@quicinc.com>
>>
>> Add support for lucid ole pll ops to configure and control the
>> lucid ole pll. The lucid ole pll has an additional test control
>> register which is required to be programmed, add support to
>> program the same.
>>
>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> ---
> Isn't this commit "write to PLL_TEST_CTL_U2 on LUCID_EVO" instead?
> 
> Meaninglessly duplicating ops does not seem useful.
> 
> Konrad

Though we are reusing same ops for EVO and OLE, PLL_TEST_CTL_U2 register 
programming is applicable only to OLE PLL type. And PLL type is useful 
to properly refer respective hardware datasheets. Hence added separate 
ops for OLE PLL type.


>>   drivers/clk/qcom/clk-alpha-pll.c | 2 ++
>>   drivers/clk/qcom/clk-alpha-pll.h | 4 ++++
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
>> index b9f6535a7ba7..f81c7c561352 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>> @@ -55,6 +55,7 @@
>>   #define PLL_TEST_CTL(p)		((p)->offset + (p)->regs[PLL_OFF_TEST_CTL])
>>   #define PLL_TEST_CTL_U(p)	((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U])
>>   #define PLL_TEST_CTL_U1(p)     ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U1])
>> +#define PLL_TEST_CTL_U2(p)     ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U2])
>>   #define PLL_STATUS(p)		((p)->offset + (p)->regs[PLL_OFF_STATUS])
>>   #define PLL_OPMODE(p)		((p)->offset + (p)->regs[PLL_OFF_OPMODE])
>>   #define PLL_FRAC(p)		((p)->offset + (p)->regs[PLL_OFF_FRAC])
>> @@ -2096,6 +2097,7 @@ void clk_lucid_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regma
>>   	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL(pll), config->test_ctl_val);
>>   	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U(pll), config->test_ctl_hi_val);
>>   	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U1(pll), config->test_ctl_hi1_val);
>> +	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U2(pll), config->test_ctl_hi2_val);
>>   
>>   	/* Disable PLL output */
>>   	regmap_update_bits(regmap, PLL_MODE(pll), PLL_OUTCTRL, 0);
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
>> index d07b17186b90..4d9b6d5b7062 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.h
>> +++ b/drivers/clk/qcom/clk-alpha-pll.h
>> @@ -125,6 +125,7 @@ struct alpha_pll_config {
>>   	u32 test_ctl_val;
>>   	u32 test_ctl_hi_val;
>>   	u32 test_ctl_hi1_val;
>> +	u32 test_ctl_hi2_val;
>>   	u32 main_output_mask;
>>   	u32 aux_output_mask;
>>   	u32 aux2_output_mask;
>> @@ -171,6 +172,7 @@ extern const struct clk_ops clk_alpha_pll_zonda_ops;
>>   #define clk_alpha_pll_postdiv_zonda_ops clk_alpha_pll_postdiv_fabia_ops
>>   
>>   extern const struct clk_ops clk_alpha_pll_lucid_evo_ops;
>> +#define clk_alpha_pll_lucid_ole_ops clk_alpha_pll_lucid_evo_ops
>>   extern const struct clk_ops clk_alpha_pll_reset_lucid_evo_ops;
>>   #define clk_alpha_pll_reset_lucid_ole_ops clk_alpha_pll_reset_lucid_evo_ops
>>   extern const struct clk_ops clk_alpha_pll_fixed_lucid_evo_ops;
>> @@ -196,6 +198,8 @@ void clk_zonda_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>>   			     const struct alpha_pll_config *config);
>>   void clk_lucid_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>>   				 const struct alpha_pll_config *config);
>> +#define clk_lucid_ole_pll_configure(pll, regmap, config) \
>> +			clk_lucid_evo_pll_configure(pll, regmap, config)
>>   void clk_rivian_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>>   				  const struct alpha_pll_config *config);
>>   void clk_stromer_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,

Thanks & Regards,
Jagadeesh
Konrad Dybcio May 19, 2023, 1:09 p.m. UTC | #3 
On 19.05.2023 14:49, Jagadeesh Kona wrote:
> Hi,
> 
> Thanks Konrad for your review!
> 
> On 5/10/2023 1:36 AM, Konrad Dybcio wrote:
>>
>>
>> On 9.05.2023 18:12, Jagadeesh Kona wrote:
>>> From: Taniya Das <quic_tdas@quicinc.com>
>>>
>>> Add support for lucid ole pll ops to configure and control the
>>> lucid ole pll. The lucid ole pll has an additional test control
>>> register which is required to be programmed, add support to
>>> program the same.
>>>
>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>> ---
>> Isn't this commit "write to PLL_TEST_CTL_U2 on LUCID_EVO" instead?
>>
>> Meaninglessly duplicating ops does not seem useful.
>>
>> Konrad
> 
> Though we are reusing same ops for EVO and OLE, PLL_TEST_CTL_U2 register programming is applicable only to OLE PLL type.
Well, your patch makes it unconditional (modulo programmer error) so
I think that makes little sense.. A comment would be enough, imo.

Konrad
And PLL type is useful to properly refer respective hardware datasheets. Hence added separate ops for OLE PLL type.
> 
> 
>>>   drivers/clk/qcom/clk-alpha-pll.c | 2 ++
>>>   drivers/clk/qcom/clk-alpha-pll.h | 4 ++++
>>>   2 files changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
>>> index b9f6535a7ba7..f81c7c561352 100644
>>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>>> @@ -55,6 +55,7 @@
>>>   #define PLL_TEST_CTL(p)        ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL])
>>>   #define PLL_TEST_CTL_U(p)    ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U])
>>>   #define PLL_TEST_CTL_U1(p)     ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U1])
>>> +#define PLL_TEST_CTL_U2(p)     ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U2])
>>>   #define PLL_STATUS(p)        ((p)->offset + (p)->regs[PLL_OFF_STATUS])
>>>   #define PLL_OPMODE(p)        ((p)->offset + (p)->regs[PLL_OFF_OPMODE])
>>>   #define PLL_FRAC(p)        ((p)->offset + (p)->regs[PLL_OFF_FRAC])
>>> @@ -2096,6 +2097,7 @@ void clk_lucid_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regma
>>>       clk_alpha_pll_write_config(regmap, PLL_TEST_CTL(pll), config->test_ctl_val);
>>>       clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U(pll), config->test_ctl_hi_val);
>>>       clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U1(pll), config->test_ctl_hi1_val);
>>> +    clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U2(pll), config->test_ctl_hi2_val);
>>>         /* Disable PLL output */
>>>       regmap_update_bits(regmap, PLL_MODE(pll), PLL_OUTCTRL, 0);
>>> diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
>>> index d07b17186b90..4d9b6d5b7062 100644
>>> --- a/drivers/clk/qcom/clk-alpha-pll.h
>>> +++ b/drivers/clk/qcom/clk-alpha-pll.h
>>> @@ -125,6 +125,7 @@ struct alpha_pll_config {
>>>       u32 test_ctl_val;
>>>       u32 test_ctl_hi_val;
>>>       u32 test_ctl_hi1_val;
>>> +    u32 test_ctl_hi2_val;
>>>       u32 main_output_mask;
>>>       u32 aux_output_mask;
>>>       u32 aux2_output_mask;
>>> @@ -171,6 +172,7 @@ extern const struct clk_ops clk_alpha_pll_zonda_ops;
>>>   #define clk_alpha_pll_postdiv_zonda_ops clk_alpha_pll_postdiv_fabia_ops
>>>     extern const struct clk_ops clk_alpha_pll_lucid_evo_ops;
>>> +#define clk_alpha_pll_lucid_ole_ops clk_alpha_pll_lucid_evo_ops
>>>   extern const struct clk_ops clk_alpha_pll_reset_lucid_evo_ops;
>>>   #define clk_alpha_pll_reset_lucid_ole_ops clk_alpha_pll_reset_lucid_evo_ops
>>>   extern const struct clk_ops clk_alpha_pll_fixed_lucid_evo_ops;
>>> @@ -196,6 +198,8 @@ void clk_zonda_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>>>                    const struct alpha_pll_config *config);
>>>   void clk_lucid_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>>>                    const struct alpha_pll_config *config);
>>> +#define clk_lucid_ole_pll_configure(pll, regmap, config) \
>>> +            clk_lucid_evo_pll_configure(pll, regmap, config)
>>>   void clk_rivian_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>>>                     const struct alpha_pll_config *config);
>>>   void clk_stromer_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
> 
> Thanks & Regards,
> Jagadeesh
Jagadeesh Kona May 24, 2023, 2:27 p.m. UTC | #4 
Hi Konrad,

Thanks for your review!

On 5/19/2023 6:39 PM, Konrad Dybcio wrote:
> 
> 
> On 19.05.2023 14:49, Jagadeesh Kona wrote:
>> Hi,
>>
>> Thanks Konrad for your review!
>>
>> On 5/10/2023 1:36 AM, Konrad Dybcio wrote:
>>>
>>>
>>> On 9.05.2023 18:12, Jagadeesh Kona wrote:
>>>> From: Taniya Das <quic_tdas@quicinc.com>
>>>>
>>>> Add support for lucid ole pll ops to configure and control the
>>>> lucid ole pll. The lucid ole pll has an additional test control
>>>> register which is required to be programmed, add support to
>>>> program the same.
>>>>
>>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>> ---
>>> Isn't this commit "write to PLL_TEST_CTL_U2 on LUCID_EVO" instead?
>>>
>>> Meaninglessly duplicating ops does not seem useful.
>>>
>>> Konrad
>>
>> Though we are reusing same ops for EVO and OLE, PLL_TEST_CTL_U2 register programming is applicable only to OLE PLL type.
> Well, your patch makes it unconditional (modulo programmer error) so
> I think that makes little sense.. A comment would be enough, imo.
> 
> Konrad
Yes, will remove the duplicate definitions and will reuse the existing ops.

> And PLL type is useful to properly refer respective hardware datasheets. Hence added separate ops for OLE PLL type.
>>

Thanks & Regards,
Jagadeesh
diff mbox series

Patch

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index b9f6535a7ba7..f81c7c561352 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -55,6 +55,7 @@ 
 #define PLL_TEST_CTL(p)		((p)->offset + (p)->regs[PLL_OFF_TEST_CTL])
 #define PLL_TEST_CTL_U(p)	((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U])
 #define PLL_TEST_CTL_U1(p)     ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U1])
+#define PLL_TEST_CTL_U2(p)     ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U2])
 #define PLL_STATUS(p)		((p)->offset + (p)->regs[PLL_OFF_STATUS])
 #define PLL_OPMODE(p)		((p)->offset + (p)->regs[PLL_OFF_OPMODE])
 #define PLL_FRAC(p)		((p)->offset + (p)->regs[PLL_OFF_FRAC])
@@ -2096,6 +2097,7 @@  void clk_lucid_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regma
 	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL(pll), config->test_ctl_val);
 	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U(pll), config->test_ctl_hi_val);
 	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U1(pll), config->test_ctl_hi1_val);
+	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U2(pll), config->test_ctl_hi2_val);
 
 	/* Disable PLL output */
 	regmap_update_bits(regmap, PLL_MODE(pll), PLL_OUTCTRL, 0);
diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
index d07b17186b90..4d9b6d5b7062 100644
--- a/drivers/clk/qcom/clk-alpha-pll.h
+++ b/drivers/clk/qcom/clk-alpha-pll.h
@@ -125,6 +125,7 @@  struct alpha_pll_config {
 	u32 test_ctl_val;
 	u32 test_ctl_hi_val;
 	u32 test_ctl_hi1_val;
+	u32 test_ctl_hi2_val;
 	u32 main_output_mask;
 	u32 aux_output_mask;
 	u32 aux2_output_mask;
@@ -171,6 +172,7 @@  extern const struct clk_ops clk_alpha_pll_zonda_ops;
 #define clk_alpha_pll_postdiv_zonda_ops clk_alpha_pll_postdiv_fabia_ops
 
 extern const struct clk_ops clk_alpha_pll_lucid_evo_ops;
+#define clk_alpha_pll_lucid_ole_ops clk_alpha_pll_lucid_evo_ops
 extern const struct clk_ops clk_alpha_pll_reset_lucid_evo_ops;
 #define clk_alpha_pll_reset_lucid_ole_ops clk_alpha_pll_reset_lucid_evo_ops
 extern const struct clk_ops clk_alpha_pll_fixed_lucid_evo_ops;
@@ -196,6 +198,8 @@  void clk_zonda_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
 			     const struct alpha_pll_config *config);
 void clk_lucid_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
 				 const struct alpha_pll_config *config);
+#define clk_lucid_ole_pll_configure(pll, regmap, config) \
+			clk_lucid_evo_pll_configure(pll, regmap, config)
 void clk_rivian_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
 				  const struct alpha_pll_config *config);
 void clk_stromer_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,