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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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.

> 
>> +
>>   	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?

> 
>> +
>>   	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)

-- 
With best wishes
Dmitry