Re: [PATCH v2 1/5] 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:

New issues:
- [Low] The newly introduced `input_fref` field in `struct rzv2h_pll_limits` is explicitly documented as expecting a value in MHz, but the implementation treats it strictly as Hz.

Pre-existing issues:
- [High] On 32-bit platforms, the fractional PLL calculation `p.k * input_fref` silently overflows and undergoes incorrect zero-extension, leading to severely incorrect frequencies.
--

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

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

Introduce a per-SoC PLL reference input frequency parameter to avoid
relying on a hardcoded 24MHz constant during PLL configuration math.
This abstraction permits the reuse of the common PLL divider logic on
newer SoC platforms like the RZ/T2H, which feature a 48 MHz PLL reference
clock input instead of the 24 MHz signal used by RZ/V2H(P).

> diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
> index f6c47fb89bca5..513f8283c4796 100644
> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c

[ ... ]

> @@ -330,9 +330,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;

[Severity: High]
This is a pre-existing issue, but does this calculation silently overflow on
32-bit platforms?

Looking at rzv2h_get_pll_pars(), p.k is an s16 which can be negative.
Because input_fref is an unsigned long, which is 32-bit on 32-bit
architectures, multiplying the signed negative p.k promotes it to an
unsigned long.

This results in a large positive 32-bit integer instead of a properly
signed negative value. Since the destination output is a u64, this 32-bit
unsigned result is zero-extended to 64 bits, incorrectly increasing the
output frequency rather than appropriately subtracting from it.

Should this explicitly cast to 64-bit to ensure proper sign extension?
For example:

output += (s64)p.k * (u64)input_fref;

>  				/* 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..94ee70b20b44c 100644
> --- a/include/linux/clk/renesas.h
> +++ b/include/linux/clk/renesas.h
> @@ -53,6 +53,9 @@ 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). If set
> + * to 0, a default value of 24MHz is used.
> + *

[Severity: Low]
Is the documentation for input_fref in struct rzv2h_pll_limits accurate?

The kernel-doc mentions this field expects a value in MHz. However, looking
at the core calculation helper rzv2h_get_pll_pars(), the code expects this
to be in Hz.

If a driver author sets input_fref to 48 (following the MHz documentation),
the ternary operator in rzv2h_get_pll_pars() evaluates to 48 instead of the
24MHz fallback:

unsigned long input_fref = limits->input_fref ?: (24 * MEGA);

This results in an input frequency of exactly 48 Hz, which would cause
subsequent fvco bounds checks to fail.

Could the documentation be updated to clarify that this field expects the
frequency in Hz?

>   * @fout: Output frequency range (in MHz)
>   * @fout.min: Minimum allowed output frequency
>   * @fout.max: Maximum allowed output frequency

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