[v2] Add CMN PLL clock controller support for IPQ5332

[PATCH v2 1/5] clk: qcom: cmnpll: Account for reference clock divider

Posted by Luo Jie 1 month ago

The clk_cmn_pll_recalc_rate() function must account for the reference clock
divider programmed in CMN_PLL_REFCLK_CONFIG. Without this fix, platforms
with a reference divider other than 1 calculate incorrect CMN PLL rates.
For example, on IPQ5332 where the reference divider is 2, the computed rate
becomes twice the actual output.

Read CMN_PLL_REFCLK_DIV and divide the parent rate by this value before
applying the 2 * FACTOR scaling. This yields the correct rate calculation:
rate = (parent_rate / ref_div) * 2 * factor.

Maintain backward compatibility with earlier platforms (e.g. IPQ9574,
IPQ5424, IPQ5018) that use ref_div = 1.

Fixes: f81715a4c87c ("clk: qcom: Add CMN PLL clock controller driver for IPQ SoC")
Signed-off-by: Luo Jie <jie.luo@oss.qualcomm.com>
---
 drivers/clk/qcom/ipq-cmn-pll.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
index dafbf5732048..369798d1ce42 100644
--- a/drivers/clk/qcom/ipq-cmn-pll.c
+++ b/drivers/clk/qcom/ipq-cmn-pll.c
@@ -185,7 +185,7 @@ static unsigned long clk_cmn_pll_recalc_rate(struct clk_hw *hw,
 					     unsigned long parent_rate)
 {
 	struct clk_cmn_pll *cmn_pll = to_clk_cmn_pll(hw);
-	u32 val, factor;
+	u32 val, factor, ref_div;
 
 	/*
 	 * The value of CMN_PLL_DIVIDER_CTRL_FACTOR is automatically adjusted
@@ -193,8 +193,15 @@ static unsigned long clk_cmn_pll_recalc_rate(struct clk_hw *hw,
 	 */
 	regmap_read(cmn_pll->regmap, CMN_PLL_DIVIDER_CTRL, &val);
 	factor = FIELD_GET(CMN_PLL_DIVIDER_CTRL_FACTOR, val);
+	if (WARN_ON(factor == 0))
+		factor = 1;
 
-	return parent_rate * 2 * factor;
+	regmap_read(cmn_pll->regmap, CMN_PLL_REFCLK_CONFIG, &val);
+	ref_div = FIELD_GET(CMN_PLL_REFCLK_DIV, val);
+	if (WARN_ON(ref_div == 0))
+		ref_div = 1;
+
+	return div_u64((u64)parent_rate * 2 * factor, ref_div);
 }
 
 static int clk_cmn_pll_determine_rate(struct clk_hw *hw,

-- 
2.43.0

Re: [PATCH v2 1/5] clk: qcom: cmnpll: Account for reference clock divider

Posted by Konrad Dybcio 1 month ago

On 1/7/26 6:35 AM, Luo Jie wrote:
> The clk_cmn_pll_recalc_rate() function must account for the reference clock
> divider programmed in CMN_PLL_REFCLK_CONFIG. Without this fix, platforms
> with a reference divider other than 1 calculate incorrect CMN PLL rates.
> For example, on IPQ5332 where the reference divider is 2, the computed rate
> becomes twice the actual output.
> 
> Read CMN_PLL_REFCLK_DIV and divide the parent rate by this value before
> applying the 2 * FACTOR scaling. This yields the correct rate calculation:
> rate = (parent_rate / ref_div) * 2 * factor.
> 
> Maintain backward compatibility with earlier platforms (e.g. IPQ9574,
> IPQ5424, IPQ5018) that use ref_div = 1.
> 
> Fixes: f81715a4c87c ("clk: qcom: Add CMN PLL clock controller driver for IPQ SoC")
> Signed-off-by: Luo Jie <jie.luo@oss.qualcomm.com>
> ---
>  drivers/clk/qcom/ipq-cmn-pll.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
> index dafbf5732048..369798d1ce42 100644
> --- a/drivers/clk/qcom/ipq-cmn-pll.c
> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
> @@ -185,7 +185,7 @@ static unsigned long clk_cmn_pll_recalc_rate(struct clk_hw *hw,
>  					     unsigned long parent_rate)
>  {
>  	struct clk_cmn_pll *cmn_pll = to_clk_cmn_pll(hw);
> -	u32 val, factor;
> +	u32 val, factor, ref_div;
>  
>  	/*
>  	 * The value of CMN_PLL_DIVIDER_CTRL_FACTOR is automatically adjusted
> @@ -193,8 +193,15 @@ static unsigned long clk_cmn_pll_recalc_rate(struct clk_hw *hw,
>  	 */
>  	regmap_read(cmn_pll->regmap, CMN_PLL_DIVIDER_CTRL, &val);
>  	factor = FIELD_GET(CMN_PLL_DIVIDER_CTRL_FACTOR, val);
> +	if (WARN_ON(factor == 0))
> +		factor = 1;

FWIW the docs tell me the value of this field is '192' on IPQ5332..

Konrad

Re: [PATCH v2 1/5] clk: qcom: cmnpll: Account for reference clock divider

Posted by Jie Luo 1 month ago


On 1/7/2026 8:16 PM, Konrad Dybcio wrote:
> On 1/7/26 6:35 AM, Luo Jie wrote:
>> The clk_cmn_pll_recalc_rate() function must account for the reference clock
>> divider programmed in CMN_PLL_REFCLK_CONFIG. Without this fix, platforms
>> with a reference divider other than 1 calculate incorrect CMN PLL rates.
>> For example, on IPQ5332 where the reference divider is 2, the computed rate
>> becomes twice the actual output.
>>
>> Read CMN_PLL_REFCLK_DIV and divide the parent rate by this value before
>> applying the 2 * FACTOR scaling. This yields the correct rate calculation:
>> rate = (parent_rate / ref_div) * 2 * factor.
>>
>> Maintain backward compatibility with earlier platforms (e.g. IPQ9574,
>> IPQ5424, IPQ5018) that use ref_div = 1.
>>
>> Fixes: f81715a4c87c ("clk: qcom: Add CMN PLL clock controller driver for IPQ SoC")
>> Signed-off-by: Luo Jie <jie.luo@oss.qualcomm.com>
>> ---
>>  drivers/clk/qcom/ipq-cmn-pll.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
>> index dafbf5732048..369798d1ce42 100644
>> --- a/drivers/clk/qcom/ipq-cmn-pll.c
>> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
>> @@ -185,7 +185,7 @@ static unsigned long clk_cmn_pll_recalc_rate(struct clk_hw *hw,
>>  					     unsigned long parent_rate)
>>  {
>>  	struct clk_cmn_pll *cmn_pll = to_clk_cmn_pll(hw);
>> -	u32 val, factor;
>> +	u32 val, factor, ref_div;
>>  
>>  	/*
>>  	 * The value of CMN_PLL_DIVIDER_CTRL_FACTOR is automatically adjusted
>> @@ -193,8 +193,15 @@ static unsigned long clk_cmn_pll_recalc_rate(struct clk_hw *hw,
>>  	 */
>>  	regmap_read(cmn_pll->regmap, CMN_PLL_DIVIDER_CTRL, &val);
>>  	factor = FIELD_GET(CMN_PLL_DIVIDER_CTRL_FACTOR, val);
>> +	if (WARN_ON(factor == 0))
>> +		factor = 1;
> 
> FWIW the docs tell me the value of this field is '192' on IPQ5332..
> 
> Konrad

Although the register description lists the default value as 192, the
actual runtime value is 125 on IPQ5332, as shown in the dump below.

# devmem 0x9B794
0x00006C7D

# cat /sys/kernel/debug/clk/clk_summary | grep cmn_pll -B 2
 xo-clk                              1       1        0        48000000
  0          0     50000      Y   deviceless
no_connection_id
    ref-48mhz-clk                    2       2        0        48000000
  0          0     50000      Y      deviceless
no_connection_id
       cmn_pll                       3       3        0
6000000000  0          0     50000      Y         deviceless
         no_connection_id

Re: [PATCH v2 1/5] clk: qcom: cmnpll: Account for reference clock divider

Posted by Konrad Dybcio 1 month ago

On 1/8/26 7:39 AM, Jie Luo wrote:
> 
> 
> On 1/7/2026 8:16 PM, Konrad Dybcio wrote:
>> On 1/7/26 6:35 AM, Luo Jie wrote:
>>> The clk_cmn_pll_recalc_rate() function must account for the reference clock
>>> divider programmed in CMN_PLL_REFCLK_CONFIG. Without this fix, platforms
>>> with a reference divider other than 1 calculate incorrect CMN PLL rates.
>>> For example, on IPQ5332 where the reference divider is 2, the computed rate
>>> becomes twice the actual output.
>>>
>>> Read CMN_PLL_REFCLK_DIV and divide the parent rate by this value before
>>> applying the 2 * FACTOR scaling. This yields the correct rate calculation:
>>> rate = (parent_rate / ref_div) * 2 * factor.
>>>
>>> Maintain backward compatibility with earlier platforms (e.g. IPQ9574,
>>> IPQ5424, IPQ5018) that use ref_div = 1.
>>>
>>> Fixes: f81715a4c87c ("clk: qcom: Add CMN PLL clock controller driver for IPQ SoC")
>>> Signed-off-by: Luo Jie <jie.luo@oss.qualcomm.com>
>>> ---
>>>  drivers/clk/qcom/ipq-cmn-pll.c | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
>>> index dafbf5732048..369798d1ce42 100644
>>> --- a/drivers/clk/qcom/ipq-cmn-pll.c
>>> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
>>> @@ -185,7 +185,7 @@ static unsigned long clk_cmn_pll_recalc_rate(struct clk_hw *hw,
>>>  					     unsigned long parent_rate)
>>>  {
>>>  	struct clk_cmn_pll *cmn_pll = to_clk_cmn_pll(hw);
>>> -	u32 val, factor;
>>> +	u32 val, factor, ref_div;
>>>  
>>>  	/*
>>>  	 * The value of CMN_PLL_DIVIDER_CTRL_FACTOR is automatically adjusted
>>> @@ -193,8 +193,15 @@ static unsigned long clk_cmn_pll_recalc_rate(struct clk_hw *hw,
>>>  	 */
>>>  	regmap_read(cmn_pll->regmap, CMN_PLL_DIVIDER_CTRL, &val);
>>>  	factor = FIELD_GET(CMN_PLL_DIVIDER_CTRL_FACTOR, val);
>>> +	if (WARN_ON(factor == 0))
>>> +		factor = 1;
>>
>> FWIW the docs tell me the value of this field is '192' on IPQ5332..
>>
>> Konrad
> 
> Although the register description lists the default value as 192, the
> actual runtime value is 125 on IPQ5332, as shown in the dump below.
> 
> # devmem 0x9B794
> 0x00006C7D
> 
> # cat /sys/kernel/debug/clk/clk_summary | grep cmn_pll -B 2
>  xo-clk                              1       1        0        48000000
>   0          0     50000      Y   deviceless
> no_connection_id
>     ref-48mhz-clk                    2       2        0        48000000
>   0          0     50000      Y      deviceless
> no_connection_id
>        cmn_pll                       3       3        0
> 6000000000  0          0     50000      Y         deviceless
>          no_connection_id

Aaah I totally forgot about the xo rate in the calculations.. 1 vs 2
vs 100-something threw me off :)

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad