Re: [PATCH v2 2/3] clk: nuvoton: ma35d1: fix PLL_CTL1_FRAC bit field width and fractional calc

[Joey Lu <a0987203069@gmail.com>]

On 5/19/2026 10:53 PM, Brian Masney wrote:
> Hi Joey,
>
> On Wed, May 13, 2026 at 01:56:25PM +0800, Joey Lu wrote:
>> PLL_CTL1_FRAC was defined as GENMASK(31, 24), covering only 8 bits.
>> The hardware fractional field occupies bits [31:8] (24 bits), so the
>> mask must be GENMASK(31, 8).
>>
>> The previous fractional-mode calculation used FIELD_MAX(PLL_CTL1_FRAC)
>> as the denominator to obtain 2 decimal places.  With the corrected 24-bit
>> mask the old divisor is wrong; replace the arithmetic with a proper
>> 24-bit fixed-point rounding to 3 decimal places:
>>
>>    n_frac = n * 1000 + (x * 1000 + 500) >> 24
>>
>> The +500 term provides round-to-nearest before the right shift.
>>
>> Fixes: 691521a367cf ("clk: nuvoton: Add clock driver for ma35d1 clock controller")
>> Signed-off-by: Joey Lu <a0987203069@gmail.com>
>> ---
>>   drivers/clk/nuvoton/clk-ma35d1-pll.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/nuvoton/clk-ma35d1-pll.c b/drivers/clk/nuvoton/clk-ma35d1-pll.c
>> index bfedd45bd04b..7e6b30d20c01 100644
>> --- a/drivers/clk/nuvoton/clk-ma35d1-pll.c
>> +++ b/drivers/clk/nuvoton/clk-ma35d1-pll.c
>> @@ -48,7 +48,7 @@
>>   #define PLL_CTL1_PD		BIT(0)
>>   #define PLL_CTL1_BP		BIT(1)
>>   #define PLL_CTL1_OUTDIV		GENMASK(6, 4)
>> -#define PLL_CTL1_FRAC		GENMASK(31, 24)
>> +#define PLL_CTL1_FRAC		GENMASK(31, 8)
>>   #define PLL_CTL2_SLOPE		GENMASK(23, 0)
>>   
>>   #define INDIV_MIN		1
>> @@ -113,9 +113,9 @@ static unsigned long ma35d1_calc_pll_freq(u8 mode, u32 *reg_ctl, unsigned long p
>>   		pll_freq = div_u64(pll_freq, m * p);
>>   	} else {
>>   		x = FIELD_GET(PLL_CTL1_FRAC, reg_ctl[1]);
>> -		/* 2 decimal places floating to integer (ex. 1.23 to 123) */
>> -		n = n * 100 + ((x * 100) / FIELD_MAX(PLL_CTL1_FRAC));
>> -		pll_freq = div_u64(parent_rate * n, 100 * m * p);
>> +		/* x is 24-bit fractional part, convert to 3 decimal digits */
>> +		n = n * 1000 + (u32)(((u64)x * 1000 + 500) >> 24);
>                                             ^^^^^^^^^^^^^^^^^^^^^
> You should be able to use DIV_ROUND_CLOSEST_ULL() here.
>
> Brian

Thanks for the suggestion! I'll update this to use DIV_ROUND_CLOSEST_ULL 
in the next version.

BR, Joey

>
>> +		pll_freq = div_u64((u64)parent_rate * n, 1000 * m * p);
>>   	}
>>   	return pll_freq;
>>   }
>> -- 
>> 2.43.0
>>