Re: [PATCH V2 3/6] clk: qcom: clk-alpha-pll: Remove explicit CAL_L configuration for EVO PLL
From: Jagadeesh Kona
Date: Wed Jun 14 2023 - 07:54:13 EST
On 6/9/2023 5:55 PM, Dmitry Baryshkov wrote:
On Fri, 9 Jun 2023 at 14:50, Jagadeesh Kona <quic_jkona@xxxxxxxxxxx> 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@xxxxxxxxxxx>
Signed-off-by: Jagadeesh Kona <quic_jkona@xxxxxxxxxxx>
---
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