linux-kernel - Re: [PATCH V2 3/6] clk: qcom: clk-alpha-pll: Remove explicit CAL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <695d7b39-2759-690d-5ff8-5aff6b37e39c@quicinc.com>
Date:   Wed, 14 Jun 2023 17:23:28 +0530
From:   Jagadeesh Kona <quic_jkona@...cinc.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC:     Konrad Dybcio <konrad.dybcio@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
        Vinod Koul <vkoul@...nel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-clk@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, Taniya Das <quic_tdas@...cinc.com>,
        Satya Priya Kakitapalli <quic_skakitap@...cinc.com>,
        Imran Shaik <quic_imrashai@...cinc.com>,
        Ajit Pandey <quic_ajipan@...cinc.com>
Subject: Re: [PATCH V2 3/6] clk: qcom: clk-alpha-pll: Remove explicit CAL_L
 configuration for EVO PLL



On 6/9/2023 5:55 PM, Dmitry Baryshkov wrote:
> On Fri, 9 Jun 2023 at 14:50, Jagadeesh Kona <quic_jkona@...cinc.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@...cinc.com>
>>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@...cinc.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