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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Integer Truncation and Overflow in PLL Rate Calculation
- [High] DDR Dividers Unsafely Share a Single Hardware Bit
- [Medium] Incorrect Error Code Returned in Probe Functions
- [Medium] Ambiguous Multi-Bit PLL Lock Status Check
--

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

clk: eyeq: Add EyeQ7H compatibles

This commit adds entries for the 14 Other Logic Blocks found in the EyeQ7H
SoC. It also introduces parsing functions for the JFRACR and AINTP PLLs,
which have unique properties and register layouts compared to previous SoC
generations.

> diff --git a/drivers/clk/clk-eyeq.c b/drivers/clk/clk-eyeq.c
> index 83c6f50024513..c8c924d63bade 100644
> --- a/drivers/clk/clk-eyeq.c
> +++ b/drivers/clk/clk-eyeq.c
[ ... ]
> @@ -71,6 +72,41 @@
>  #define FRACG_PCSR1_DOWN_SPREAD			BIT(11)
>  #define FRACG_PCSR1_FRAC_IN			GENMASK(31, 12)
>  
> +#define JFRACR_PCSR0_BYPASS			BIT(0)
> +#define JFRACR_PCSR0_PLL_EN			BIT(1)
> +#define JFRACR_PCSR0_FOUTVCO_EN			BIT(2)
> +#define JFRACR_PCSR0_FOUTPOSTDIV_EN		BIT(3)
> +#define JFRACR_PCSR0_POST_DIV1			GENMASK(6, 4)
> +#define JFRACR_PCSR0_POST_DIV2			GENMASK(9, 7)
> +#define JFRACR_PCSR0_REF_DIV			GENMASK(15, 10)
> +#define JFRACR_PCSR0_FB_DIV			GENMASK(27, 16)
> +#define JFRACR_PCSR0_VCO_SEL			GENMASK(29, 28)
> +#define JFRACR_PCSR0_PLL_LOCKED			GENMASK(31, 30)

[ ... ]

> +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);
> +	r0 = val;
> +	r1 = val >> 32;
> +	r2 = readl(base + 8);
> +
> +	if (r0 & JFRACR_PCSR0_BYPASS) {
> +		*mult = 1;
> +		*div = 1;
> +		*acc = 0;
> +		return 0;
> +	}
> +
> +	if (!(r0 & JFRACR_PCSR0_PLL_LOCKED))
> +		return -EINVAL;

JFRACR_PCSR0_PLL_LOCKED is defined as a 2-bit mask. Does this check incorrectly
evaluate to true if only one of the two bits is set? Usually a multi-bit field
represents specific states rather than just checking for any non-zero value.

> +
> +	*mult = FIELD_GET(JFRACR_PCSR0_FB_DIV, r0);
> +	*div = FIELD_GET(JFRACR_PCSR0_REF_DIV, r0);
> +
> +	/* 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);
> +	}

On 32-bit architectures, unsigned long is 32 bits. Does assigning the 64-bit
calculation to *mult silently truncate the result since FB_DIV allows the
multiplier to reach up to 4095, which requires up to 36 bits?

> +
> +	if (!*mult || !*div)
> +		return -EINVAL;
> +
> +	if (r2 & (JFRACR_PCSR2_RESET | JFRACR_PCSR2_DIS_SSCG)) {
> +		*acc = 0;
> +		return 0;
> +	}

If spread spectrum is disabled, the function returns early here. Does this skip
calling eqc_pll_downshift_factors()? If skipped, would the function pass a
potentially 36-bit value into clk_hw_register_fixed_factor_pdata(), which
accepts unsigned int and silently discards the top 4 bits?

> +
> +	/* spread spectrum is identical to FRACG PLL */
> +	spread = FIELD_GET(JFRACR_PCSR2_SPREAD, r2);
> +	*acc = DIV_ROUND_CLOSEST(spread * 1000000000, 1024 * 2);

Could the accuracy calculation spread * 1000000000 overflow a 32-bit integer on
32-bit systems, since spread can be up to 31?

[ ... ]

> +static int eqc_probe_pll_aintp(struct device *dev, struct device_node *np,
> +			       const struct eqc_clock *clk, void __iomem *base,
> +			       struct clk_hw_onecell_data *cells)
> +{
> +	struct clk_parent_data parent_data = { };
> +	unsigned long mult, div;
> +	struct clk_hw *hw;
> +	int ret;
> +
> +	ret = eqc_pll_parse_aintp(base + clk->pll.reg, &mult, &div);
> +	if (ret)
> +		return ret;
> +
> +	ret = eqc_fill_parent_data(clk, cells, &parent_data);
> +	if (ret)
> +		return ret;
> +
> +	hw = clk_hw_register_fixed_factor_pdata(dev, np, clk->name, &parent_data,
> +						0, mult, div, 0, 0);
> +
> +	if (IS_ERR(hw))
> +		return IS_ERR(hw);

Is this returning 1 on failure instead of the actual negative error code like
-ENOMEM? Could this be changed to use PTR_ERR(hw) so the caller logs the
correct error code?

> +
> +	cells->hws[clk->index] = hw;
> +	return 0;
> +}

[ ... ]

> +static int eqc_probe_pll_jfracr(struct device *dev, struct device_node *np,
> +				const struct eqc_clock *clk, void __iomem *base,
> +				struct clk_hw_onecell_data *cells)
> +{
[ ... ]
> +	hw = clk_hw_register_fixed_factor_pdata(dev, np, clk->name, &parent_data, 0, mult,
> +						div, acc, CLK_FIXED_FACTOR_FIXED_ACCURACY);
> +	if (IS_ERR(hw))
> +		return IS_ERR(hw);

Same issue here. Does this return 1 instead of PTR_ERR(hw)?

> +
> +	cells->hws[clk->index] = hw;
> +	return 0;
> +}

[ ... ]

> +static const struct eqc_clock eqc_eyeq7h_ddr0_clks[] = {
> +	PLL_AINTP(EQ7HC_DDR_PLL, PARENT_BY_FWNAME, "pll-ddr0", "ref", 0x0),
> +
> +	DIV(EQ7HC_DDR_DIV_APB, EQ7HC_DDR_PLL, "div-ddr0_apb", NULL,
> +	    0x08, 10, 1, eqc_eyeq7h_ddr_apb_div_table),
> +	DIV(EQ7HC_DDR_DIV_PLLREF, EQ7HC_DDR_PLL, "div-ddr0_pllref", NULL,
> +	    0x08, 10, 1, eqc_eyeq7h_ddr_ref_div_table),
> +	DIV(EQ7HC_DDR_DIV_DFI, EQ7HC_DDR_PLL, "div-ddr0-dfi", NULL,
> +	    0x08, 10, 1, eqc_eyeq7h_ddr_dfi_div_table),
> +};

These three clocks use the exact same register (0x08), shift (10), and width
(1). Because they are registered as independent dividers, does changing the rate
of one silently alter the hardware state for the other two?

If so, will this desynchronize the CCF's internal state from the actual
hardware configuration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-clk-eyeq7-v6-0-0540cce18fb2@bootlin.com?part=9