[V2,3/6] clk: qcom: clk-alpha-pll: Remove explicit CAL_L configuration for EVO PLL

  In lucid evo pll, the CAL_L field is part of L value register itself, and
the l value configuration passed from clock controller driver includes
CAL_L and L values as well. Hence remove explicit configuration of CAL_L
for evo pll.

Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL configuration interfaces")
Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
Changes since V1:
 - Newly added.

 drivers/clk/qcom/clk-alpha-pll.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
  

On 25.05.2023 19:21, Jagadeesh Kona wrote:
> In lucid evo pll, the CAL_L field is part of L value register itself, and
> the l value configuration passed from clock controller driver includes
> CAL_L and L values as well. Hence remove explicit configuration of CAL_L
> for evo pll.
> 
> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL configuration interfaces")
> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> ---
Oh that isn't obvious at first sight, nice find!

I'd suggest a different solution though:

#define LUCID_EVO_PLL_L_LVAL	GENMASK(..
#define LUCID_EVO_PLL_L_CAL_L	GENMASK(..

lval = FIELD_PREP(LUCID_EVO_PLL_L_LVAL, config->l) |
       FIELD_PREP(LUCID_EVO_PLL_L_CAL_L, config->cal_l);

This would make the separation between the two parts more explicit

however

config->l would then represent the L value and not the end value
written to the L register

Up to you, whichever you find saner!

Konrad

> Changes since V1:
>  - Newly added.
> 
>  drivers/clk/qcom/clk-alpha-pll.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index f81c7c561352..68a80395997b 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -270,7 +270,6 @@ EXPORT_SYMBOL_GPL(clk_alpha_pll_regs);
>  #define LUCID_EVO_PCAL_NOT_DONE		BIT(8)
>  #define LUCID_EVO_ENABLE_VOTE_RUN       BIT(25)
>  #define LUCID_EVO_PLL_L_VAL_MASK        GENMASK(15, 0)
> -#define LUCID_EVO_PLL_CAL_L_VAL_SHIFT	16
>  
>  /* ZONDA PLL specific */
>  #define ZONDA_PLL_OUT_MASK	0xf
> @@ -2084,10 +2083,7 @@ EXPORT_SYMBOL_GPL(clk_alpha_pll_zonda_ops);
>  void clk_lucid_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>  				 const struct alpha_pll_config *config)
>  {
> -	u32 lval = config->l;
> -
> -	lval |= TRION_PLL_CAL_VAL << LUCID_EVO_PLL_CAL_L_VAL_SHIFT;
> -	clk_alpha_pll_write_config(regmap, PLL_L_VAL(pll), lval);
> +	clk_alpha_pll_write_config(regmap, PLL_L_VAL(pll), config->l);
>  	clk_alpha_pll_write_config(regmap, PLL_ALPHA_VAL(pll), config->alpha);
>  	clk_alpha_pll_write_config(regmap, PLL_CONFIG_CTL(pll), config->config_ctl_val);
>  	clk_alpha_pll_write_config(regmap, PLL_CONFIG_CTL_U(pll), config->config_ctl_hi_val);
  

On 26/05/2023 12:33, Konrad Dybcio wrote:
> 
> 
> On 25.05.2023 19:21, Jagadeesh Kona wrote:
>> In lucid evo pll, the CAL_L field is part of L value register itself, and
>> the l value configuration passed from clock controller driver includes
>> CAL_L and L values as well. Hence remove explicit configuration of CAL_L
>> for evo pll.
>>
>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL configuration interfaces")
>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> ---
> Oh that isn't obvious at first sight, nice find!
> 
> I'd suggest a different solution though:
> 
> #define LUCID_EVO_PLL_L_LVAL	GENMASK(..
> #define LUCID_EVO_PLL_L_CAL_L	GENMASK(..
> 
> lval = FIELD_PREP(LUCID_EVO_PLL_L_LVAL, config->l) |
>         FIELD_PREP(LUCID_EVO_PLL_L_CAL_L, config->cal_l);
> 
> This would make the separation between the two parts more explicit
> 
> however
> 
> config->l would then represent the L value and not the end value
> written to the L register

Yes. I think there should be separate config->l and config->cal_l values 
(and probably ringosc_cal_l, basing on the comment in the source).

Just a question: is camcc-sm8550 using the same PLL type or is it some 
kind of subtype of lucid_evo PLL?
  

On 25/05/2023 18:21, Jagadeesh Kona wrote:
> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL configuration interfaces")

Is this a "Fixes" without the previous patch to stuff the CAL_L_VAL and 
VAL_L fields ?

[PATCH V2 3/6] clk: qcom: clk-alpha-pll: Remove explicit CAL_L 
configuration for EVO PLL

Surely you need _both_ with this patch depending on the previous, per 
your comment ?

-	.l = 0x3e,
+	/* .l includes CAL_L_VAL, L_VAL fields */
+	.l = 0x0044003e,

---
bod
  

On 26/05/2023 16:54, Bryan O'Donoghue wrote:
> On 25/05/2023 18:21, Jagadeesh Kona wrote:
>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL 
>> configuration interfaces")
> 
> Is this a "Fixes" without the previous patch to stuff the CAL_L_VAL and 
> VAL_L fields ?
> 
> [PATCH V2 3/6] clk: qcom: clk-alpha-pll: Remove explicit CAL_L 
> configuration for EVO PLL
> 
> Surely you need _both_ with this patch depending on the previous, per 
> your comment ?
> 
> -    .l = 0x3e,
> +    /* .l includes CAL_L_VAL, L_VAL fields */
> +    .l = 0x0044003e,
> 
> ---
> bod

i.e. if you pick up this patch on its own you won't populate 
CAL_L_VAL... right ?

It would make more sense to squash the two patches.

---
bod
  

Hi Bryan,

Thanks for your review!

On 5/26/2023 9:27 PM, Bryan O'Donoghue wrote:
> On 26/05/2023 16:54, Bryan O'Donoghue wrote:
>> On 25/05/2023 18:21, Jagadeesh Kona wrote:
>>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL 
>>> configuration interfaces")
>>
>> Is this a "Fixes" without the previous patch to stuff the CAL_L_VAL 
>> and VAL_L fields ?
>>
>> [PATCH V2 3/6] clk: qcom: clk-alpha-pll: Remove explicit CAL_L 
>> configuration for EVO PLL
>>
>> Surely you need _both_ with this patch depending on the previous, per 
>> your comment ?
>>
>> -    .l = 0x3e,
>> +    /* .l includes CAL_L_VAL, L_VAL fields */
>> +    .l = 0x0044003e,
>>
>> ---
>> bod
> 
> i.e. if you pick up this patch on its own you won't populate 
> CAL_L_VAL... right ?
> 
> It would make more sense to squash the two patches.
> 
Sure, will squash both the patches in next series.
> ---
> bod

Thanks & Regards,
Jagadeesh
  

Hi Dmitry, Konrad,

On 5/26/2023 9:23 PM, Dmitry Baryshkov wrote:
> On 26/05/2023 12:33, Konrad Dybcio wrote:
>>
>>
>> On 25.05.2023 19:21, Jagadeesh Kona wrote:
>>> In lucid evo pll, the CAL_L field is part of L value register itself, 
>>> and
>>> the l value configuration passed from clock controller driver includes
>>> CAL_L and L values as well. Hence remove explicit configuration of CAL_L
>>> for evo pll.
>>>
>>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL 
>>> configuration interfaces")
>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>> ---
>> Oh that isn't obvious at first sight, nice find!
>>
>> I'd suggest a different solution though:
>>
>> #define LUCID_EVO_PLL_L_LVAL    GENMASK(..
>> #define LUCID_EVO_PLL_L_CAL_L    GENMASK(..
>>
>> lval = FIELD_PREP(LUCID_EVO_PLL_L_LVAL, config->l) |
>>         FIELD_PREP(LUCID_EVO_PLL_L_CAL_L, config->cal_l);
>>
>> This would make the separation between the two parts more explicit
>>
>> however
>>
>> config->l would then represent the L value and not the end value
>> written to the L register
> 
> Yes. I think there should be separate config->l and config->cal_l values 
> (and probably ringosc_cal_l, basing on the comment in the source).
> Thanks for your suggestions. In all recent chipsets, L & CAL_L fields 
are encapsulated in the same register, so we feel it is better to 
directly pass the combined configuration value in config->l itself and 
program it directly into register without any additional handling 
required in pll driver code.

Also the evo pll code is currently reused for both lucid evo and ole 
pll's. Lucid ole PLL has an additional RINGOSC_CAL_L field along with L, 
CAL_L fields in the same L register. By passing combined configuration 
value in config->l itself, we feel we can avoid all the additional 
handling required in PLL code.

> Just a question: is camcc-sm8550 using the same PLL type or is it some 
> kind of subtype of lucid_evo PLL?
> 
No, it is not the same lucid evo PLL. It uses lucid ole PLL.

Thanks & Regards,
Jagadeesh
  

On 01/06/2023 17:33, Jagadeesh Kona wrote:
> Hi Dmitry, Konrad,
> 
> On 5/26/2023 9:23 PM, Dmitry Baryshkov wrote:
>> On 26/05/2023 12:33, Konrad Dybcio wrote:
>>>
>>>
>>> On 25.05.2023 19:21, Jagadeesh Kona wrote:
>>>> In lucid evo pll, the CAL_L field is part of L value register 
>>>> itself, and
>>>> the l value configuration passed from clock controller driver includes
>>>> CAL_L and L values as well. Hence remove explicit configuration of 
>>>> CAL_L
>>>> for evo pll.
>>>>
>>>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL 
>>>> configuration interfaces")
>>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>> ---
>>> Oh that isn't obvious at first sight, nice find!
>>>
>>> I'd suggest a different solution though:
>>>
>>> #define LUCID_EVO_PLL_L_LVAL    GENMASK(..
>>> #define LUCID_EVO_PLL_L_CAL_L    GENMASK(..
>>>
>>> lval = FIELD_PREP(LUCID_EVO_PLL_L_LVAL, config->l) |
>>>         FIELD_PREP(LUCID_EVO_PLL_L_CAL_L, config->cal_l);
>>>
>>> This would make the separation between the two parts more explicit
>>>
>>> however
>>>
>>> config->l would then represent the L value and not the end value
>>> written to the L register
>>
>> Yes. I think there should be separate config->l and config->cal_l 
>> values (and probably ringosc_cal_l, basing on the comment in the source).
>> Thanks for your suggestions. In all recent chipsets, L & CAL_L fields 
> are encapsulated in the same register, so we feel it is better to 
> directly pass the combined configuration value in config->l itself and 
> program it directly into register without any additional handling 
> required in pll driver code.

My feeling is that it is better to split it, since these are the 
different fields. The value .l = 0x4444003e doesn't mean anything per se.

Three values are much more meaningful:
.l = 0x3e,
.cal_l = 0x44,
.ringosc_cal_l = 0x44,

Not to mention that this way you don't have to touch pll configuration 
for the existing Lucid EVO PLL. Not to mention that for the Lucid ole 
PLLs the cal_l and ringosc_cal_l values seem to be static (0x44), so 
there is no need to put them to the variable data.

> 
> Also the evo pll code is currently reused for both lucid evo and ole 
> pll's. Lucid ole PLL has an additional RINGOSC_CAL_L field along with L, 
> CAL_L fields in the same L register. By passing combined configuration 
> value in config->l itself, we feel we can avoid all the additional 
> handling required in PLL code.
> 
>> Just a question: is camcc-sm8550 using the same PLL type or is it some 
>> kind of subtype of lucid_evo PLL?
>>
> No, it is not the same lucid evo PLL. It uses lucid ole PLL.

Then please don't reuse the clk_lucid_evo_pll_configure() call.
You can add a new one, which will handle L/CAL_L/RINGOSC_CAL_L differences.
  

Hi Dmitry,

Thanks for your review!

On 6/1/2023 8:13 PM, Dmitry Baryshkov wrote:
> On 01/06/2023 17:33, Jagadeesh Kona wrote:
>> Hi Dmitry, Konrad,
>>
>> On 5/26/2023 9:23 PM, Dmitry Baryshkov wrote:
>>> On 26/05/2023 12:33, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 25.05.2023 19:21, Jagadeesh Kona wrote:
>>>>> In lucid evo pll, the CAL_L field is part of L value register 
>>>>> itself, and
>>>>> the l value configuration passed from clock controller driver includes
>>>>> CAL_L and L values as well. Hence remove explicit configuration of 
>>>>> CAL_L
>>>>> for evo pll.
>>>>>
>>>>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL 
>>>>> configuration interfaces")
>>>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>>> ---
>>>> Oh that isn't obvious at first sight, nice find!
>>>>
>>>> I'd suggest a different solution though:
>>>>
>>>> #define LUCID_EVO_PLL_L_LVAL    GENMASK(..
>>>> #define LUCID_EVO_PLL_L_CAL_L    GENMASK(..
>>>>
>>>> lval = FIELD_PREP(LUCID_EVO_PLL_L_LVAL, config->l) |
>>>>         FIELD_PREP(LUCID_EVO_PLL_L_CAL_L, config->cal_l);
>>>>
>>>> This would make the separation between the two parts more explicit
>>>>
>>>> however
>>>>
>>>> config->l would then represent the L value and not the end value
>>>> written to the L register
>>>
>>> Yes. I think there should be separate config->l and config->cal_l 
>>> values (and probably ringosc_cal_l, basing on the comment in the 
>>> source).
>>> Thanks for your suggestions. In all recent chipsets, L & CAL_L fields 
>> are encapsulated in the same register, so we feel it is better to 
>> directly pass the combined configuration value in config->l itself and 
>> program it directly into register without any additional handling 
>> required in pll driver code.
> 
> My feeling is that it is better to split it, since these are the 
> different fields. The value .l = 0x4444003e doesn't mean anything per se.
> 
> Three values are much more meaningful:
> .l = 0x3e,
> .cal_l = 0x44,
> .ringosc_cal_l = 0x44,
> 
> Not to mention that this way you don't have to touch pll configuration 
> for the existing Lucid EVO PLL. Not to mention that for the Lucid ole 
> PLLs the cal_l and ringosc_cal_l values seem to be static (0x44), so 
> there is no need to put them to the variable data.
> 

Sure, will keep the existing code as is and will remove this patch in 
the next series.

>>
>> Also the evo pll code is currently reused for both lucid evo and ole 
>> pll's. Lucid ole PLL has an additional RINGOSC_CAL_L field along with 
>> L, CAL_L fields in the same L register. By passing combined 
>> configuration value in config->l itself, we feel we can avoid all the 
>> additional handling required in PLL code.
>>
>>> Just a question: is camcc-sm8550 using the same PLL type or is it 
>>> some kind of subtype of lucid_evo PLL?
>>>
>> No, it is not the same lucid evo PLL. It uses lucid ole PLL.
> 
> Then please don't reuse the clk_lucid_evo_pll_configure() call.
> You can add a new one, which will handle L/CAL_L/RINGOSC_CAL_L differences.
> 

The only difference between evo and ole pll configure is extra 
RINGOSC_CAL_L programming needed only for ole pll. We can achieve the 
same with clk_lucid_evo_pll_configure() itself by directly including 
RINGOSC_CAL_L field in L configuration for OLE PLL's.

Thanks & Regards,
Jagadeesh
  

On Fri, 9 Jun 2023 at 14:50, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>
> Hi Dmitry,
>
> Thanks for your review!
>
> On 6/1/2023 8:13 PM, Dmitry Baryshkov wrote:
> > On 01/06/2023 17:33, Jagadeesh Kona wrote:
> >> Hi Dmitry, Konrad,
> >>
> >> On 5/26/2023 9:23 PM, Dmitry Baryshkov wrote:
> >>> On 26/05/2023 12:33, Konrad Dybcio wrote:
> >>>>
> >>>>
> >>>> On 25.05.2023 19:21, Jagadeesh Kona wrote:
> >>>>> In lucid evo pll, the CAL_L field is part of L value register
> >>>>> itself, and
> >>>>> the l value configuration passed from clock controller driver includes
> >>>>> CAL_L and L values as well. Hence remove explicit configuration of
> >>>>> CAL_L
> >>>>> for evo pll.
> >>>>>
> >>>>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL
> >>>>> configuration interfaces")
> >>>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> >>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> >>>>> ---
> >>>> Oh that isn't obvious at first sight, nice find!
> >>>>
> >>>> I'd suggest a different solution though:
> >>>>
> >>>> #define LUCID_EVO_PLL_L_LVAL    GENMASK(..
> >>>> #define LUCID_EVO_PLL_L_CAL_L    GENMASK(..
> >>>>
> >>>> lval = FIELD_PREP(LUCID_EVO_PLL_L_LVAL, config->l) |
> >>>>         FIELD_PREP(LUCID_EVO_PLL_L_CAL_L, config->cal_l);
> >>>>
> >>>> This would make the separation between the two parts more explicit
> >>>>
> >>>> however
> >>>>
> >>>> config->l would then represent the L value and not the end value
> >>>> written to the L register
> >>>
> >>> Yes. I think there should be separate config->l and config->cal_l
> >>> values (and probably ringosc_cal_l, basing on the comment in the
> >>> source).
> >>> Thanks for your suggestions. In all recent chipsets, L & CAL_L fields
> >> are encapsulated in the same register, so we feel it is better to
> >> directly pass the combined configuration value in config->l itself and
> >> program it directly into register without any additional handling
> >> required in pll driver code.
> >
> > My feeling is that it is better to split it, since these are the
> > different fields. The value .l = 0x4444003e doesn't mean anything per se.
> >
> > Three values are much more meaningful:
> > .l = 0x3e,
> > .cal_l = 0x44,
> > .ringosc_cal_l = 0x44,
> >
> > Not to mention that this way you don't have to touch pll configuration
> > for the existing Lucid EVO PLL. Not to mention that for the Lucid ole
> > PLLs the cal_l and ringosc_cal_l values seem to be static (0x44), so
> > there is no need to put them to the variable data.
> >
>
> Sure, will keep the existing code as is and will remove this patch in
> the next series.
>
> >>
> >> Also the evo pll code is currently reused for both lucid evo and ole
> >> pll's. Lucid ole PLL has an additional RINGOSC_CAL_L field along with
> >> L, CAL_L fields in the same L register. By passing combined
> >> configuration value in config->l itself, we feel we can avoid all the
> >> additional handling required in PLL code.
> >>
> >>> Just a question: is camcc-sm8550 using the same PLL type or is it
> >>> some kind of subtype of lucid_evo PLL?
> >>>
> >> No, it is not the same lucid evo PLL. It uses lucid ole PLL.
> >
> > Then please don't reuse the clk_lucid_evo_pll_configure() call.
> > You can add a new one, which will handle L/CAL_L/RINGOSC_CAL_L differences.
> >
>
> The only difference between evo and ole pll configure is extra
> RINGOSC_CAL_L programming needed only for ole pll. We can achieve the
> same with clk_lucid_evo_pll_configure() itself by directly including
> RINGOSC_CAL_L field in L configuration for OLE PLL's.

Please don't, that's all I can say. Those are different fields. By
looking at the config->l one can calculate PLL rate. If you overload
the config->l with CAL_L and RINGOSC_CAL_L, the purpose of this field
is gone.

As the CAL_L and RINGOSC_CAL_L fields are static, just move them to
the clk_lucid_ole_pll_configure().
  

On 6/9/2023 5:55 PM, Dmitry Baryshkov wrote:
> On Fri, 9 Jun 2023 at 14:50, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>>
>> Hi Dmitry,
>>
>> Thanks for your review!
>>
>> On 6/1/2023 8:13 PM, Dmitry Baryshkov wrote:
>>> On 01/06/2023 17:33, Jagadeesh Kona wrote:
>>>> Hi Dmitry, Konrad,
>>>>
>>>> On 5/26/2023 9:23 PM, Dmitry Baryshkov wrote:
>>>>> On 26/05/2023 12:33, Konrad Dybcio wrote:
>>>>>>
>>>>>>
>>>>>> On 25.05.2023 19:21, Jagadeesh Kona wrote:
>>>>>>> In lucid evo pll, the CAL_L field is part of L value register
>>>>>>> itself, and
>>>>>>> the l value configuration passed from clock controller driver includes
>>>>>>> CAL_L and L values as well. Hence remove explicit configuration of
>>>>>>> CAL_L
>>>>>>> for evo pll.
>>>>>>>
>>>>>>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL
>>>>>>> configuration interfaces")
>>>>>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>>>>> ---
>>>>>> Oh that isn't obvious at first sight, nice find!
>>>>>>
>>>>>> I'd suggest a different solution though:
>>>>>>
>>>>>> #define LUCID_EVO_PLL_L_LVAL    GENMASK(..
>>>>>> #define LUCID_EVO_PLL_L_CAL_L    GENMASK(..
>>>>>>
>>>>>> lval = FIELD_PREP(LUCID_EVO_PLL_L_LVAL, config->l) |
>>>>>>          FIELD_PREP(LUCID_EVO_PLL_L_CAL_L, config->cal_l);
>>>>>>
>>>>>> This would make the separation between the two parts more explicit
>>>>>>
>>>>>> however
>>>>>>
>>>>>> config->l would then represent the L value and not the end value
>>>>>> written to the L register
>>>>>
>>>>> Yes. I think there should be separate config->l and config->cal_l
>>>>> values (and probably ringosc_cal_l, basing on the comment in the
>>>>> source).
>>>>> Thanks for your suggestions. In all recent chipsets, L & CAL_L fields
>>>> are encapsulated in the same register, so we feel it is better to
>>>> directly pass the combined configuration value in config->l itself and
>>>> program it directly into register without any additional handling
>>>> required in pll driver code.
>>>
>>> My feeling is that it is better to split it, since these are the
>>> different fields. The value .l = 0x4444003e doesn't mean anything per se.
>>>
>>> Three values are much more meaningful:
>>> .l = 0x3e,
>>> .cal_l = 0x44,
>>> .ringosc_cal_l = 0x44,
>>>
>>> Not to mention that this way you don't have to touch pll configuration
>>> for the existing Lucid EVO PLL. Not to mention that for the Lucid ole
>>> PLLs the cal_l and ringosc_cal_l values seem to be static (0x44), so
>>> there is no need to put them to the variable data.
>>>
>>
>> Sure, will keep the existing code as is and will remove this patch in
>> the next series.
>>
>>>>
>>>> Also the evo pll code is currently reused for both lucid evo and ole
>>>> pll's. Lucid ole PLL has an additional RINGOSC_CAL_L field along with
>>>> L, CAL_L fields in the same L register. By passing combined
>>>> configuration value in config->l itself, we feel we can avoid all the
>>>> additional handling required in PLL code.
>>>>
>>>>> Just a question: is camcc-sm8550 using the same PLL type or is it
>>>>> some kind of subtype of lucid_evo PLL?
>>>>>
>>>> No, it is not the same lucid evo PLL. It uses lucid ole PLL.
>>>
>>> Then please don't reuse the clk_lucid_evo_pll_configure() call.
>>> You can add a new one, which will handle L/CAL_L/RINGOSC_CAL_L differences.
>>>
>>
>> The only difference between evo and ole pll configure is extra
>> RINGOSC_CAL_L programming needed only for ole pll. We can achieve the
>> same with clk_lucid_evo_pll_configure() itself by directly including
>> RINGOSC_CAL_L field in L configuration for OLE PLL's.
> 
> Please don't, that's all I can say. Those are different fields. By
> looking at the config->l one can calculate PLL rate. If you overload
> the config->l with CAL_L and RINGOSC_CAL_L, the purpose of this field
> is gone.
> 
> As the CAL_L and RINGOSC_CAL_L fields are static, just move them to
> the clk_lucid_ole_pll_configure().
> 

We feel it is better to include them in config->l and reuse existing 
code than adding separate function for lucid ole pll configure. Even the 
other pll configurations(like .user_ctl_val) contains multiple fields 
but are passed as a single value from driver.

We also added a comment in code stating all the fields included in 
config->l value, so user will be aware while calculating PLL frequency.

Thanks,
Jagadeesh
  

On Wed, 14 Jun 2023 at 14:53, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>
>
>
> On 6/9/2023 5:55 PM, Dmitry Baryshkov wrote:
> > On Fri, 9 Jun 2023 at 14:50, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
> >>
> >> Hi Dmitry,
> >>
> >> Thanks for your review!
> >>
> >> On 6/1/2023 8:13 PM, Dmitry Baryshkov wrote:
> >>> On 01/06/2023 17:33, Jagadeesh Kona wrote:
> >>>> Hi Dmitry, Konrad,
> >>>>
> >>>> On 5/26/2023 9:23 PM, Dmitry Baryshkov wrote:
> >>>>> On 26/05/2023 12:33, Konrad Dybcio wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 25.05.2023 19:21, Jagadeesh Kona wrote:
> >>>>>>> In lucid evo pll, the CAL_L field is part of L value register
> >>>>>>> itself, and
> >>>>>>> the l value configuration passed from clock controller driver includes
> >>>>>>> CAL_L and L values as well. Hence remove explicit configuration of
> >>>>>>> CAL_L
> >>>>>>> for evo pll.
> >>>>>>>
> >>>>>>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL
> >>>>>>> configuration interfaces")
> >>>>>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> >>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> >>>>>>> ---
> >>>>>> Oh that isn't obvious at first sight, nice find!
> >>>>>>
> >>>>>> I'd suggest a different solution though:
> >>>>>>
> >>>>>> #define LUCID_EVO_PLL_L_LVAL    GENMASK(..
> >>>>>> #define LUCID_EVO_PLL_L_CAL_L    GENMASK(..
> >>>>>>
> >>>>>> lval = FIELD_PREP(LUCID_EVO_PLL_L_LVAL, config->l) |
> >>>>>>          FIELD_PREP(LUCID_EVO_PLL_L_CAL_L, config->cal_l);
> >>>>>>
> >>>>>> This would make the separation between the two parts more explicit
> >>>>>>
> >>>>>> however
> >>>>>>
> >>>>>> config->l would then represent the L value and not the end value
> >>>>>> written to the L register
> >>>>>
> >>>>> Yes. I think there should be separate config->l and config->cal_l
> >>>>> values (and probably ringosc_cal_l, basing on the comment in the
> >>>>> source).
> >>>>> Thanks for your suggestions. In all recent chipsets, L & CAL_L fields
> >>>> are encapsulated in the same register, so we feel it is better to
> >>>> directly pass the combined configuration value in config->l itself and
> >>>> program it directly into register without any additional handling
> >>>> required in pll driver code.
> >>>
> >>> My feeling is that it is better to split it, since these are the
> >>> different fields. The value .l = 0x4444003e doesn't mean anything per se.
> >>>
> >>> Three values are much more meaningful:
> >>> .l = 0x3e,
> >>> .cal_l = 0x44,
> >>> .ringosc_cal_l = 0x44,
> >>>
> >>> Not to mention that this way you don't have to touch pll configuration
> >>> for the existing Lucid EVO PLL. Not to mention that for the Lucid ole
> >>> PLLs the cal_l and ringosc_cal_l values seem to be static (0x44), so
> >>> there is no need to put them to the variable data.
> >>>
> >>
> >> Sure, will keep the existing code as is and will remove this patch in
> >> the next series.
> >>
> >>>>
> >>>> Also the evo pll code is currently reused for both lucid evo and ole
> >>>> pll's. Lucid ole PLL has an additional RINGOSC_CAL_L field along with
> >>>> L, CAL_L fields in the same L register. By passing combined
> >>>> configuration value in config->l itself, we feel we can avoid all the
> >>>> additional handling required in PLL code.
> >>>>
> >>>>> Just a question: is camcc-sm8550 using the same PLL type or is it
> >>>>> some kind of subtype of lucid_evo PLL?
> >>>>>
> >>>> No, it is not the same lucid evo PLL. It uses lucid ole PLL.
> >>>
> >>> Then please don't reuse the clk_lucid_evo_pll_configure() call.
> >>> You can add a new one, which will handle L/CAL_L/RINGOSC_CAL_L differences.
> >>>
> >>
> >> The only difference between evo and ole pll configure is extra
> >> RINGOSC_CAL_L programming needed only for ole pll. We can achieve the
> >> same with clk_lucid_evo_pll_configure() itself by directly including
> >> RINGOSC_CAL_L field in L configuration for OLE PLL's.
> >
> > Please don't, that's all I can say. Those are different fields. By
> > looking at the config->l one can calculate PLL rate. If you overload
> > the config->l with CAL_L and RINGOSC_CAL_L, the purpose of this field
> > is gone.
> >
> > As the CAL_L and RINGOSC_CAL_L fields are static, just move them to
> > the clk_lucid_ole_pll_configure().
> >
>
> We feel it is better to include them in config->l and reuse existing
> code than adding separate function for lucid ole pll configure. Even the
> other pll configurations(like .user_ctl_val) contains multiple fields
> but are passed as a single value from driver.

I suppose it was done this way because these fields are pretty much
not documented in the openly published data. And sometimes this
strikes, one can not easily check PLL's configuration. Or tune
it.There was a discussion whether we should start handling PLL outputs
properly (in CCF) rather than configuring them in a static way.

Also mentioned registers differ from PLL to PLL. For the RISCOSC_CAL_L
and CAL_L the value is static, if I'm not mistaken. Having them in the
configurable field doesn't sound correct.

Last, but not least. We are already handling CAL_L value completely in
the clock-alpha-pll.c for triton, lucid and lucid evo PLLs. What would
be the _reason_ to change that?

>
> We also added a comment in code stating all the fields included in
> config->l value, so user will be aware while calculating PLL frequency.
>
> Thanks,
> Jagadeesh
  

On 6/14/2023 5:56 PM, Dmitry Baryshkov wrote:
> On Wed, 14 Jun 2023 at 14:53, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>>
>>
>>
>> On 6/9/2023 5:55 PM, Dmitry Baryshkov wrote:
>>> On Fri, 9 Jun 2023 at 14:50, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>>>>
>>>> Hi Dmitry,
>>>>
>>>> Thanks for your review!
>>>>
>>>> On 6/1/2023 8:13 PM, Dmitry Baryshkov wrote:
>>>>> On 01/06/2023 17:33, Jagadeesh Kona wrote:
>>>>>> Hi Dmitry, Konrad,
>>>>>>
>>>>>> On 5/26/2023 9:23 PM, Dmitry Baryshkov wrote:
>>>>>>> On 26/05/2023 12:33, Konrad Dybcio wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 25.05.2023 19:21, Jagadeesh Kona wrote:
>>>>>>>>> In lucid evo pll, the CAL_L field is part of L value register
>>>>>>>>> itself, and
>>>>>>>>> the l value configuration passed from clock controller driver includes
>>>>>>>>> CAL_L and L values as well. Hence remove explicit configuration of
>>>>>>>>> CAL_L
>>>>>>>>> for evo pll.
>>>>>>>>>
>>>>>>>>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL
>>>>>>>>> configuration interfaces")
>>>>>>>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>>>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>>>>>>> ---
>>>>>>>> Oh that isn't obvious at first sight, nice find!
>>>>>>>>
>>>>>>>> I'd suggest a different solution though:
>>>>>>>>
>>>>>>>> #define LUCID_EVO_PLL_L_LVAL    GENMASK(..
>>>>>>>> #define LUCID_EVO_PLL_L_CAL_L    GENMASK(..
>>>>>>>>
>>>>>>>> lval = FIELD_PREP(LUCID_EVO_PLL_L_LVAL, config->l) |
>>>>>>>>           FIELD_PREP(LUCID_EVO_PLL_L_CAL_L, config->cal_l);
>>>>>>>>
>>>>>>>> This would make the separation between the two parts more explicit
>>>>>>>>
>>>>>>>> however
>>>>>>>>
>>>>>>>> config->l would then represent the L value and not the end value
>>>>>>>> written to the L register
>>>>>>>
>>>>>>> Yes. I think there should be separate config->l and config->cal_l
>>>>>>> values (and probably ringosc_cal_l, basing on the comment in the
>>>>>>> source).
>>>>>>> Thanks for your suggestions. In all recent chipsets, L & CAL_L fields
>>>>>> are encapsulated in the same register, so we feel it is better to
>>>>>> directly pass the combined configuration value in config->l itself and
>>>>>> program it directly into register without any additional handling
>>>>>> required in pll driver code.
>>>>>
>>>>> My feeling is that it is better to split it, since these are the
>>>>> different fields. The value .l = 0x4444003e doesn't mean anything per se.
>>>>>
>>>>> Three values are much more meaningful:
>>>>> .l = 0x3e,
>>>>> .cal_l = 0x44,
>>>>> .ringosc_cal_l = 0x44,
>>>>>
>>>>> Not to mention that this way you don't have to touch pll configuration
>>>>> for the existing Lucid EVO PLL. Not to mention that for the Lucid ole
>>>>> PLLs the cal_l and ringosc_cal_l values seem to be static (0x44), so
>>>>> there is no need to put them to the variable data.
>>>>>
>>>>
>>>> Sure, will keep the existing code as is and will remove this patch in
>>>> the next series.
>>>>
>>>>>>
>>>>>> Also the evo pll code is currently reused for both lucid evo and ole
>>>>>> pll's. Lucid ole PLL has an additional RINGOSC_CAL_L field along with
>>>>>> L, CAL_L fields in the same L register. By passing combined
>>>>>> configuration value in config->l itself, we feel we can avoid all the
>>>>>> additional handling required in PLL code.
>>>>>>
>>>>>>> Just a question: is camcc-sm8550 using the same PLL type or is it
>>>>>>> some kind of subtype of lucid_evo PLL?
>>>>>>>
>>>>>> No, it is not the same lucid evo PLL. It uses lucid ole PLL.
>>>>>
>>>>> Then please don't reuse the clk_lucid_evo_pll_configure() call.
>>>>> You can add a new one, which will handle L/CAL_L/RINGOSC_CAL_L differences.
>>>>>
>>>>
>>>> The only difference between evo and ole pll configure is extra
>>>> RINGOSC_CAL_L programming needed only for ole pll. We can achieve the
>>>> same with clk_lucid_evo_pll_configure() itself by directly including
>>>> RINGOSC_CAL_L field in L configuration for OLE PLL's.
>>>
>>> Please don't, that's all I can say. Those are different fields. By
>>> looking at the config->l one can calculate PLL rate. If you overload
>>> the config->l with CAL_L and RINGOSC_CAL_L, the purpose of this field
>>> is gone.
>>>
>>> As the CAL_L and RINGOSC_CAL_L fields are static, just move them to
>>> the clk_lucid_ole_pll_configure().
>>>
>>
>> We feel it is better to include them in config->l and reuse existing
>> code than adding separate function for lucid ole pll configure. Even the
>> other pll configurations(like .user_ctl_val) contains multiple fields
>> but are passed as a single value from driver.
> 
> I suppose it was done this way because these fields are pretty much
> not documented in the openly published data. And sometimes this
> strikes, one can not easily check PLL's configuration. Or tune
> it.There was a discussion whether we should start handling PLL outputs
> properly (in CCF) rather than configuring them in a static way.
> 
> Also mentioned registers differ from PLL to PLL. For the RISCOSC_CAL_L
> and CAL_L the value is static, if I'm not mistaken. Having them in the
> configurable field doesn't sound correct.
> 
> Last, but not least. We are already handling CAL_L value completely in
> the clock-alpha-pll.c for triton, lucid and lucid evo PLLs. What would
> be the _reason_ to change that?
> 

Yes, will follow the approach similar to other existing PLL's and will 
add a separate function for clk_lucid_ole_pll_configure() in next series.

Thanks,
Jagadeesh

>>
>> We also added a comment in code stating all the fields included in
>> config->l value, so user will be aware while calculating PLL frequency.
>>
>> Thanks,
>> Jagadeesh
> 
> 
>