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

[Brian Masney <bmasney@redhat.com>]

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


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