Re: [PATCH 1/4] clk: renesas: rzv2h-cpg: Use per-SoC PLL reference frequency for calculations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] On 32-bit architectures, the calculation `output += p.k * input_fref;` performs an incorrect zero-extended addition when `p.k` is negative, corrupting the PLL calculation.
- [Low] The newly added `input_fref` field is documented as being in MHz, but the code initializes and expects it in Hz.
--

commit a6beaf951f9f846b0e14a16acf1b5de978587208
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

clk: renesas: rzv2h-cpg: Use per-SoC PLL reference frequency for calculations

This commit modifies the rzv2h-cpg driver to use a per-SoC PLL reference
input frequency instead of a hardcoded 24MHz constant. It adds an
input_fref field to struct rzv2h_pll_limits and updates the PLL parameter
calculations to use it.

> diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
> index f6c47fb89bca5..cd990dc10fa96 100644
> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c
[ ... ]
> @@ -330,9 +331,9 @@ bool rzv2h_get_pll_pars(const struct rzv2h_pll_limits *limits,
>  					continue;
>  
>  				/* PLL_M component of (output * 65536 * PLL_P) */
> -				output = mul_u32_u32(p.m * 65536, RZ_V2H_OSC_CLK_IN_MEGA);
> +				output = mul_u32_u32(p.m * 65536, input_fref);
>  				/* PLL_K component of (output * 65536 * PLL_P) */
> -				output += p.k * RZ_V2H_OSC_CLK_IN_MEGA;
> +				output += p.k * input_fref;

On 32-bit architectures, does this calculation perform an incorrect
zero-extended addition when p.k is negative?

Since p.k is a signed 16-bit integer (s16) and input_fref is unsigned long
(32-bit on ARM32 architectures), negative values of p.k might get promoted to
a 32-bit unsigned integer before multiplication.

This would yield a 32-bit unsigned result. When added to the u64 output
variable, this result would be zero-extended to 64 bits instead of being
sign-extended, adding a large positive value rather than subtracting the
frequency offset.

While this existed prior to this patch, replacing the RZ_V2H_OSC_CLK_IN_MEGA
constant with input_fref maintains the same integer promotion behavior.

Could p.k be explicitly cast to a signed 64-bit integer here?

>  				/* Make it in mHz */
>  				output *= MILLI;
>  				output = DIV_U64_ROUND_CLOSEST(output, 65536 * p.p * divider);
> diff --git a/include/linux/clk/renesas.h b/include/linux/clk/renesas.h
> index c360df9fa735c..402bfeaffdfc4 100644
> --- a/include/linux/clk/renesas.h
> +++ b/include/linux/clk/renesas.h
> @@ -53,6 +53,8 @@ static inline void rzg2l_cpg_dsi_div_set_divider(u8 divider, int target) { }
>   * various parameters used to configure a PLL. These limits ensure
>   * the PLL operates within valid and stable ranges.
>   *
> + * @input_fref: Reference input frequency to the PLL (in MHz)

This isn't a bug, but is the documentation correct about the unit being in MHz?

Looking later in the diff, the field appears to be initialized in Hz:

[ ... ]

> @@ -156,6 +160,7 @@ struct rzv2h_pll_div_pars {
>  
>  #define RZV2H_CPG_PLL_DSI_LIMITS(name)					\
>  	static const struct rzv2h_pll_limits (name) = {			\
> +		.input_fref = 24 * MEGA,				\
>  		.fout = { .min = 25 * MEGA, .max = 375 * MEGA },	\

It seems the field is initialized and expected in Hz (24,000,000 Hz) rather
than MHz. Should the documentation be updated to state (in Hz) to match the
implementation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511191910.1945705-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=1