Re: [PATCH v2 1/2] clk: rockchip: pll: Fix the fractional part denominator on RK3588/RK3576

Hi Chaoyi,

On 7/23/26 12:32 PM, Chaoyi Chen wrote:
> Hello Alexey,
> 
> On 7/23/2026 6:21 PM, Alexey Charkov wrote:
>> According to the TRM, the fractional PLL coefficient should be divided by
>> 65536 rather than 65535 to obtain the output rate.
>>
>> Fix the denominator and add a comment with the TRM provided clock formulae
>> for future reference.
>>
>> See RK3576 TRM Part 1 V1.2 section 2.13.1.4 Setting Guide on P, M, S and K
>> or equivalently RK3588 TRM part 1 V1.0 section 2.17.1.4 Setting Guide on P,
>> M, S and K.
>>
>> Fractional PLL rates don't seem to be used by any current mainline
>> consumers, so this is purely a correctness fix. It will also be important
>> to properly support DisplayPort output going forward, as the video output
>> controller derives its pixel clock from system PLLs with no dedicated PHY
>> PLL option for DP unlike HDMI, and some display modes are only achievable
>> with fractional PLL rates.
>>
>> Fixes: 8f6594494b1c ("clk: rockchip: add pll type for RK3588")
>> Signed-off-by: Alexey Charkov <[email protected]>
>> ---
>>   drivers/clk/rockchip/clk-pll.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
>> index 6b853800cb6b..bf8acf7cee0d 100644
>> --- a/drivers/clk/rockchip/clk-pll.c
>> +++ b/drivers/clk/rockchip/clk-pll.c
>> @@ -900,6 +900,13 @@ static void rockchip_rk3588_pll_get_params(struct rockchip_clk_pll *pll,
>>   	rate->k = ((pllcon >> RK3588_PLLCON2_K_SHIFT) & RK3588_PLLCON2_K_MASK);
>>   }
>>   
>> +/*
>> + * 2250 MHz <= Fvco <= 4500 MHz
>> + * For Fvco > 3 GHz: period jitter +-1% frac PLL, +-0.75% int PLL
>> + * For Fvco < 3 GHz: period jitter +-2% frac PLL, +-1.50% int PLL
>> + * Fvco = ((m + k / 65536) * Fin) / p
>> + * Fout = ((m + k / 65536) * Fin) / (p * 2^s)
>> + */
>>   static unsigned long rockchip_rk3588_pll_recalc_rate(struct clk_hw *hw, unsigned long prate)
>>   {
>>   	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
>> @@ -915,7 +922,7 @@ static unsigned long rockchip_rk3588_pll_recalc_rate(struct clk_hw *hw, unsigned
>>   		/* fractional mode */
>>   		u64 frac_rate64 = prate * cur.k;
>>   
>> -		postdiv = cur.p * 65535;
>> +		postdiv = cur.p * 65536;
> 
> 
> How about using 65536ULL? Will the multiplication here overflow?
> 

cur.p is an unsigned int. p comes from the HW register directly and it's 
masked with 0x3f so it can be max 63 * 65536 which is well below 
UINT_MAX and also below U64_MAX so should be fine. It could overflow if 
it's set wrong in the static table we have in the driver, but then in 
any case we would write something that cannot work in registers (as it's 
masked when being written and wouldn't match the value set in the table).

Reviewed-by: Quentin Schulz <[email protected]>

Thanks!
Quentin