Re: [PATCH 09/13] clk: qcom: cpu-8996: fix PLL configuration sequence

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 11.01.2023 23:05, Dmitry Baryshkov wrote:
> On 11/01/2023 23:08, Konrad Dybcio wrote:
>>
>>
>> On 11.01.2023 20:20, Dmitry Baryshkov wrote:
>>> Switch both power and performance clocks to the GPLL0/2 (sys_apcs_aux)
>>> before PLL configuration. Switch them to the ACD afterwards.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/clk/qcom/clk-cpu-8996.c | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>>> index 571ed52b3026..47c58bb5f21a 100644
>>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>>> @@ -432,13 +432,27 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>>>   {
>>>       int i, ret;
>>>   +    /* Select GPLL0 for 300MHz for the both clusters */
>> superfluous 'the'
>>
>>> +    regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0xc);
>>> +    regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0xc);
>>> +
>>> +    /* Ensure write goes through before PLLs are reconfigured */
>>> +    udelay(5);
>> Is this value based on n clock cycles, or 'good enough'?
> 
> Don't know, this is based on downstream direclty.
Right, I see it now.

> 
>>
>>> +
>>>       clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
>>>       clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
>>>       clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
>>>       clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
>>>   +    /* Wait for PLL(s) to lock */
>>> +        udelay(50);
>> Weird indentation
>>
>> Maybe wait_for_pll_enable_lock() to be super sure?
> 
> Does it work for HWFSM PLLs?
Not sure, but wait_for_pll_update_ack_clear() should, since it's
called by 

clk_alpha_pll_hwfsm_set_rate() ->
  __clk_alpha_pll_set_rate() ->
    clk_alpha_pll_update_latch() ->
      __clk_alpha_pll_update_latch()

Konrad
> 
>>
>>> +
>>>       qcom_cpu_clk_msm8996_acd_init(regmap);
>>>   +    /* Switch clusters to use the ACD leg */
>>> +    regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0x2);
>>> +    regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0x2);
>>> +
>> No delays here?
> 
> No. Probably it isn't required since there is no additional PLL locking, etc.
> 
>>
>> Konrad
>>>       for (i = 0; i < ARRAY_SIZE(cpu_msm8996_hw_clks); i++) {
>>>           ret = devm_clk_hw_register(dev, cpu_msm8996_hw_clks[i]);
>>>           if (ret)
>