Re: [PATCH v8 2/2] clk: add TI CDCE6214 clock driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] Severe copy-paste errors exist in pin configuration for IO standards on OUT3 and OUT4, corrupting unrelated registers.
- [High] Rate request callbacks do not sanitize zero inputs, leading to division by zero panics.
- [High] Clock registration for OUT0 uses the wrong parent array size, leading to stack out-of-bounds reads.
- [High] Incorrect rate calculation prevents usage of the 2x clock multiplier and causes CCF state desynchronization for OUT0.
- [High] Mismatched registers are used for getting and setting the parent clock for OUT0.
- [High] The driver systematically ignores the return value of `regmap_read()`, resulting in the use of uninitialized stack memory when I2C transfers fail.
- [High] Clock math relies on 32-bit arithmetic (`unsigned long`), which will integer overflow on 32-bit architectures at standard high frequencies.
- [Medium] Contradictory and flawed handling of CMOS N/P polarity and enable bits between OUT1 and OUT4.
- [Medium] Raw divisor values are passed into FIELD_PREP for a 2-bit mask in PSA/PSB configuration.
- [Medium] Incorrect use of `do_div()` with an `unsigned long` divisor.
- [Medium] Unreasonably short timeout for I2C regmap polling in PLL lock.
- [Medium] Zero-initialized clk_parent_data element causes unintended fallback to DT index 0.
--

commit 3cfc7967fe6fb49dcdf0e635b82371b0f59bf24e
Author: Sascha Hauer <s.hauer@pengutronix.de>

clk: add TI CDCE6214 clock driver

The CDCE6214 is a Ultra-Low Power Clock Generator With One PLL, Four
Differential Outputs, Two Inputs, and Internal EEPROM. This patch adds
a common clk framework driver for this chip.

> diff --git a/drivers/clk/clk-cdce6214.c b/drivers/clk/clk-cdce6214.c
> new file mode 100644
> index 0000000000000..d012b79f37880
> --- /dev/null
> +++ b/drivers/clk/clk-cdce6214.c
> @@ -0,0 +1,1620 @@

[ ... ]

> +static int cdce6214_clk_out0_determine_rate(struct clk_hw *hw,
> +					    struct clk_rate_request *req)
> +{
> +	unsigned int div;
> +
> +	if (req->rate >= req->best_parent_rate)
> +		req->rate = req->best_parent_rate * 2;

[Severity: High]
Does this multiplication by 2 overflow a 32-bit unsigned long when the
VCO operates near its maximum limit of 2.625 GHz?

Also, does this branch lack an early return? If the consumer requests
the 2x multiplier rate, it appears to fall through and recalculate div,
inadvertently resetting the rate back to the 1x target.

[ ... ]

> +static u8 cdce6214_clk_out0_get_parent(struct clk_hw *hw)
> +{
> +	struct cdce6214_clock *clock = hw_to_cdce6214_clk(hw);
> +	struct cdce6214 *priv = clock->priv;
> +	unsigned int val;
> +
> +	regmap_read(priv->regmap, R2, &val);
> +
> +	if (FIELD_GET(R2_REFSEL_SW, val) == 2)
> +		return 1;

[Severity: High]
Is R2 the correct register here? cdce6214_clk_out0_set_parent writes
the mux state to R25. Operating on different registers prevents the
clock framework from accurately tracking the hardware parent state.

[ ... ]

> +static unsigned int cdce6214_get_out_div(unsigned long rate, unsigned long parent_rate)
> +{
> +	unsigned int div;
> +
> +	div = divider_get_val(rate, parent_rate, NULL, 14, CLK_DIVIDER_ONE_BASED);

[Severity: High]
Can rate be zero here? If a consumer requests a rate of 0 Hz,
divider_get_val() triggers a divide-by-zero panic internally when it
calls DIV_ROUND_UP_ULL(parent_rate, 0).

[ ... ]

> +static u8 cdce6214_clk_out_get_parent(struct clk_hw *hw)
> +{
> +	struct cdce6214_clock *clock = hw_to_cdce6214_clk(hw);
> +	struct cdce6214 *priv = clock->priv;
> +	unsigned int val, idx;
> +
> +	switch (clock->index) {
> +	case CDCE6214_CLK_OUT1:
> +		regmap_read(priv->regmap, R56, &val);
> +		idx = FIELD_GET(R56_CH1_MUX, val);

[Severity: High]
If regmap_read() fails due to an I2C error, does val remain
uninitialized? This appears to feed stack garbage into FIELD_GET(),
eventually returning a non-deterministic hardware index to the clock core.

[ ... ]

> +static unsigned long cdce6214_clk_pll_recalc_rate(struct clk_hw *hw,
> +						  unsigned long parent_rate)
> +{

[ ... ]

> +	return parent_rate * ndiv + DIV_ROUND_CLOSEST(parent_rate * num, den);

[Severity: High]
Could parent_rate * num overflow on 32-bit architectures? A standard 
parent rate multiplied by a large fractional numerator might easily
exceed the 32-bit unsigned long maximum limit.

[ ... ]

> +static int pll_calc_values(unsigned long parent_rate, unsigned long out,
> +			   unsigned long *ndiv, unsigned long *num, unsigned long *den)
> +{
> +	u64 a;
> +
> +	if (out < CDCE6214_VCO_MIN || out > CDCE6214_VCO_MAX)
> +		return -EINVAL;
> +
> +	*den = 10000000;
> +	*ndiv = out / parent_rate;

[Severity: High]
Can parent_rate be zero here if the upstream reference clock is disabled
or returns 0? This would result in a direct division by zero panic.

> +	a = out % parent_rate;
> +	a *= *den;
> +	do_div(a, parent_rate);

[Severity: Medium]
Is it safe to pass parent_rate (an unsigned long) to do_div()?
do_div() expects a 32-bit divisor, and passing a 64-bit value on
64-bit architectures causes silent truncation and type checking warnings.

[ ... ]

> +static int cdce6214_wait_pll_lock(struct cdce6214 *priv)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read_poll_timeout(priv->regmap, R7, val,
> +				       val & R7_LOCK_DET, 0, 1000);

[Severity: Medium]
Is a 1000us timeout sufficient for I2C regmap polling? Since standard
100kHz I2C transactions take around 300us each, this timeout might only allow
2 or 3 polling attempts and could result in spurious failures.

[ ... ]

> +static int cdce6214_clk_register(struct cdce6214 *priv)
> +{

[ ... ]

> +	init[CDCE6214_CLK_OUT0].ops = &cdce6214_clk_out0_ops;
> +	init[CDCE6214_CLK_OUT0].num_parents = ARRAY_SIZE(pdata_out);
> +	init[CDCE6214_CLK_OUT0].parent_data = pdata_out0;

[Severity: High]
Does this incorrectly use ARRAY_SIZE(pdata_out) (size 4) instead of
ARRAY_SIZE(pdata_out0) (size 2)? This instructs the clock framework to
read past the bounds of the 2-element stack-allocated array.

> +	init[CDCE6214_CLK_OUT0].flags = CLK_SET_RATE_NO_REPARENT;
> +
> +	pdata_out[0].hw = &priv->clk[CDCE6214_CLK_PSA].hw;
> +	pdata_out[1].hw = &priv->clk[CDCE6214_CLK_PSB].hw;
> +	pdata_out[3].hw = &priv->clk[CDCE6214_CLK_OUT0].hw;

[Severity: Medium]
Element pdata_out[2] is left uninitialized (all zeros). If the hardware
mux returns an index of 2, the zero-initialized entry instructs the
framework to fall back to DT index 0, which could create a bogus clock topology.

[ ... ]

> +static int cdce6214_clk_psx_set_rate(struct clk_hw *hw, unsigned long rate,
> +				     unsigned long parent_rate)
> +{

[ ... ]

> +	switch (clock->index) {
> +	case CDCE6214_CLK_PSA:
> +		regmap_update_bits(priv->regmap, R47, R47_PLL_PSA,
> +				   FIELD_PREP(R47_PLL_PSA, div));

[Severity: Medium]
Does div (which can be 4, 5, or 6) exceed the bounds of the 2-bit
R47_PLL_PSA mask? Passing values larger than the mask field to
FIELD_PREP() is an API violation, even if bitwise truncation happens
to map to the correct register indices.

[ ... ]

> +static int cdce6214_pinconf_set_iostd(struct cdce6214 *priv, unsigned int pin,
> +				      unsigned int param)
> +{

[ ... ]

> +	case OUT3:
> +		switch (param) {
> +		case CDCE6214_IOSTD_LVDS:
> +			regmap_set_bits(reg, R70, R70_CH3_LVDS_EN);
> +			regmap_clear_bits(reg, R68, R68_CH3_LPHCSL_EN);
> +			break;
> +		case CDCE6214_IOSTD_LP_HCSL:
> +			regmap_set_bits(reg, R70, R70_CH3_LVDS_EN);
> +			regmap_clear_bits(reg, R68, R65_CH2_LVDS_EN);
> +			break;

[Severity: High]
Should setting CDCE6214_IOSTD_LP_HCSL on OUT3 write to
R65_CH2_LVDS_EN? This appears to be a copy-paste error that writes
the wrong LVDS bits to an unrelated hardware register.

> +		}
> +		break;
> +	case OUT4:
> +		switch (param) {
> +		case CDCE6214_IOSTD_CMOS:
> +			regmap_clear_bits(reg, R75, R75_CH4_LVDS_EN);
> +			regmap_clear_bits(reg, R73, R73_CH4_LPHCSL_EN);
> +			break;
> +		case CDCE6214_IOSTD_LVDS:
> +			regmap_clear_bits(reg, R73, R73_CH4_LPHCSL_EN);
> +			regmap_set_bits(reg, R75, R75_CH4_LVDS_EN);
> +			break;
> +		case CDCE6214_IOSTD_LP_HCSL:
> +			regmap_clear_bits(reg, R75, R75_CH4_LVDS_EN);
> +			regmap_set_bits(reg, R72, R73_CH4_LPHCSL_EN);
> +			break;

[Severity: High]
Does setting CDCE6214_IOSTD_LP_HCSL on OUT4 inadvertently write R73
flags into R72? R72 controls the CH4 divider and muxing, which
corrupts unrelated settings.

[ ... ]

> +static int cdce6214_pinconf_set_cmosn_mode(struct cdce6214 *priv, unsigned int pin,
> +					   unsigned int param)
> +{

[ ... ]

> +	/* Relevant fields are identical for register 59 and 75 */
> +	regmap_update_bits(priv->regmap, reg, R59_CH1_CMOSN_POL | R59_CH1_CMOSN_EN, val);

[Severity: Medium]
Are these fields truly identical? It appears that register 75 swaps the
positions of the N and P bits compared to register 59. This might cause the
wrong output leg to be enabled when configuring OUT4.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-clk-cdce6214-v8-0-4dfa355c1f65@pengutronix.de?part=2