Re: [PATCH v7 9/9] clk: eyeq: Add EyeQ7H compatibles

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Calculations for PLL multipliers, dividers, and accuracy in `eqc_pll_parse_jfracr` suffer from severe truncation and overflows on 32-bit platforms.
- [High] The PLL parsing functions for AINTP and JFRACR completely ignore the defined post-divider configurations when calculating the clock frequency.

Pre-existing issues:
- [High] Using `readq()` to read two distinct 32-bit configuration registers simultaneously risks bus faults on 32-bit peripheral buses.
--

commit 2a35962c5575980a3ba602361782cb07def54c96
Author: Benoît Monin <benoit.monin@bootlin.com>

clk: eyeq: Add EyeQ7H compatibles
    
Add the entries for the 14 Other Logic Blocks found in the EyeQ7H SoC. The
clock tree is more complex than the previous generation of SoC, as some
OLB depend on the clock output of other OLB instead of all referring to
the main oscillator.

> diff --git a/drivers/clk/clk-eyeq.c b/drivers/clk/clk-eyeq.c
> index 5200d5f0ff060..d55be678c597f 100644
> --- a/drivers/clk/clk-eyeq.c
> +++ b/drivers/clk/clk-eyeq.c

[ ... ]

> @@ -160,6 +196,29 @@ static void eqc_pll_downshift_factors(unsigned long *mult, unsigned long *div)
>  	*div >>= shift;
>  }
>  
> +static int eqc_pll_parse_aintp(void __iomem *base, unsigned long *mult, unsigned long *div)
> +{
> +	u32 r0;
> +
> +	r0 = readl(base);
> +	if (r0 & AINTP_PCSR_BYPASS) {
> +		*mult = 1;
> +		*div = 1;
> +		return 0;
> +	}
> +
> +	if (!(r0 & AINTP_PCSR_PLL_LOCKED))
> +		return -EINVAL;
> +
> +	*mult = FIELD_GET(AINTP_PCSR_FB_DIV, r0);
> +	*div = FIELD_GET(AINTP_PCSR_REF_DIV, r0);

[Severity: High]
Does this calculation ignore post-dividers? The macros AINTP_PCSR_POST_DIV1
and AINTP_PCSR_POST_DIV2 are explicitly defined, but aren't used here. If the
hardware is configured to output the post-divided clock, will the driver
report an erroneously high frequency to the clock framework?

> +
> +	if (!*mult || !*div)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static int eqc_pll_parse_fracg(void __iomem *base, unsigned long *mult,
>  			       unsigned long *div, unsigned long *acc)
>  {

[ ... ]

> @@ -229,6 +288,60 @@ static int eqc_pll_parse_fracg(void __iomem *base, unsigned long *mult,
>  	return 0;
>  }
>  
> +static int eqc_pll_parse_jfracr(void __iomem *base, unsigned long *mult,
> +				unsigned long *div, unsigned long *acc)
> +{
> +	unsigned long spread;
> +	u32 r0, r1, r2;
> +	u64 val;
> +
> +	val = readq(base);

[Severity: High]
This is a pre-existing issue, but does using readq() to read two distinct
32-bit configuration registers risk bus faults on 32-bit peripheral buses?

Without explicit hardware support for 64-bit wide reads, should these
registers be read strictly using distinct readl() calls?

> +	r0 = val;
> +	r1 = val >> 32;
> +	r2 = readl(base + 8);
> +
> +	if (r0 & JFRACR_PCSR0_BYPASS) {
> +		*mult = 1;
> +		*div = 1;
> +		*acc = 0;
> +		return 0;
> +	}
> +
> +	/* Consider the PLL locked if either the phase or the frequency is locked */
> +	if (!(r0 & JFRACR_PCSR0_PLL_LOCKED))
> +		return -EINVAL;
> +
> +	*mult = FIELD_GET(JFRACR_PCSR0_FB_DIV, r0);
> +	*div = FIELD_GET(JFRACR_PCSR0_REF_DIV, r0);

[Severity: High]
Similar to the AINTP parser above, does this miss the post-dividers
JFRACR_PCSR0_POST_DIV1 and JFRACR_PCSR0_POST_DIV2 when calculating the
clock frequency?

> +
> +	/* fractional part on 24 bits */
> +	if (r1 & JFRACR_PCSR1_DSM_EN) {
> +		*div *= (1ULL << 24);
> +		*mult = *mult * (1ULL << 24) + FIELD_GET(JFRACR_PCSR1_FRAC_IN, r1);

[Severity: High]
On 32-bit platforms, mult is a pointer to a 32-bit unsigned long. Will
assigning the 64-bit result of this calculation back into *mult silently
truncate it before eqc_pll_downshift_factors() executes?

Should intermediate calculations use u64 variables instead?

> +	}
> +
> +	if (!*mult || !*div)
> +		return -EINVAL;
> +
> +	if (r2 & (JFRACR_PCSR2_RESET | JFRACR_PCSR2_DIS_SSCG)) {
> +		*acc = 0;
> +	} else {
> +		/* spread spectrum is identical to FRACG PLL */
> +		spread = FIELD_GET(JFRACR_PCSR2_SPREAD, r2);
> +		*acc = DIV_ROUND_CLOSEST(spread * 1000000000, 1024 * 2);

[Severity: High]
If spread is larger than 4, will spread * 1000000000 overflow a 32-bit
unsigned integer native calculation?

Should this use 1000000000ULL and DIV_ROUND_CLOSEST_ULL to prevent
truncation and incorrect clock rates?

> +
> +		if (r2 & JFRACR_PCSR2_DOWN_SPREAD) {
> +			*mult *= 2048 - spread;
> +			*div *= 2048;
> +		}
> +	}
> +
> +	/* make sure mult and div fit in 32 bits */
> +	eqc_pll_downshift_factors(mult, div);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522-clk-eyeq7-v7-0-14ad6c2c5213@bootlin.com?part=9