Re: [PATCH 2/4] clk: ingenic: Add Ingenic X1600 CGU clock driver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] clk: ingenic: uninitialized m and n in x1600_pll_calc_m_n_od()
- [High] clk: ingenic: x1600: overlapping mux and div bits for X1600_CLK_CIMMCLK
- [High] clk: ingenic: x1600: missing .gate initializer for X1600_CLK_RTC
- [High] clk: ingenic: x1600: duplicate CLKGR0 gate bit for SADC and UART0
- [High] clk: ingenic: broken mdiv handling in ingenic_clk_calc_div() and set_rate()
- [High] clk: ingenic: x1600-cgu requires undocumented clk12m external DT clock
- [Medium] clk: ingenic: off-by-one od_max check in ingenic_pll_recalc_rate_od()
- [Medium] clk: ingenic: uninitialized d_reg and signed shift in ingenic_clk_set_rate()
- [Medium] clk: ingenic: x1600: missing -1 parent sentinels for X1600_CLK_OTGPHY
- [Medium] clk: ingenic: x1600: missing cgu->lock in OTG PHY enable/disable callbacks
--

commit 3e8079f374773cb8ca4b149b680046e60d28f341
Author: Paul Boddie <paul@boddie.org.uk>

clk: ingenic: Add Ingenic X1600 CGU clock driver support

This commit adds Clock Generation Unit (CGU) driver support for the Ingenic
X1600 SoC, including custom OTG PHY and PLL rate calculation handlers. It also
extends the common Ingenic CGU core to handle secondary post-VCO PLL dividers
(OD1) and fractional I2S M/N/D divider fields.

> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
> index 41e4c69131bd6..b4ef8ddf043a7 100644
> --- a/drivers/clk/ingenic/cgu.c
> +++ b/drivers/clk/ingenic/cgu.c
> @@ -76,6 +77,40 @@ ingenic_cgu_gate_set(struct ingenic_cgu *cgu,
>   * PLL operations
>   */
>  
> +static unsigned
> +ingenic_pll_recalc_rate_od(u32 ctl, u8 od_shift, u8 od_bits, u8 od_max,
> +			   const s8 *od_encoding)
> +{
> +	unsigned int od, od_enc = 0;
> +
> +	if (od_bits > 0) {
> +		od_enc = ctl >> od_shift;
> +		od_enc &= GENMASK(od_bits - 1, 0);
> +	}
> +
> +	/*
> +	 * Use the encoding table if indicated. Otherwise, use the value
> +	 * directly, interpreting an encoded value of zero as a divider
> +	 * value of one.
> +	 */
> +	if (od_encoding) {
> +		for (od = 0; od < od_max; od++)
> +			if (od_encoding[od] == od_enc)
> +				break;
> +		od++;
> +	} else {
> +		od = od_enc ? od_enc : 1;
> +	}
> +
> +	/* if od_max = 0, od_bits should be 0 and od is fixed to 1. */
> +	if (od_max == 0)
> +		WARN_ON_ONCE(od_bits != 0);
> +	else
> +		WARN_ON_ONCE(od == od_max);

[Severity: Medium]
In ingenic_pll_recalc_rate_od(), does moving od++ before
WARN_ON_ONCE(od == od_max) cause an off-by-one regression in the bounds check
when od_encoding is used?

Previously in ingenic_pll_recalc_rate(), the check against pll_info->od_max
was performed on the 0-based loop index before incrementing od:

	for (od = 0; od < pll_info->od_max; od++)
		if (pll_info->od_encoding[od] == od_enc)
			break;

	if (pll_info->od_max == 0)
		BUG_ON(pll_info->od_bits != 0);
	else
		BUG_ON(od == pll_info->od_max);
	od++;

With od++ now happening inside the if (od_encoding) branch before the check,
matching the highest valid entry at od_encoding[od_max - 1] increments od to
od_max and triggers a false WARN_ON_ONCE(od == od_max). Conversely, if od_enc
is not found in od_encoding, the loop finishes with od == od_max, od++ makes
it od_max + 1, and the warning does not fire.

[ ... ]

> @@ -531,7 +590,7 @@ ingenic_clk_set_rate(struct clk_hw *hw, unsigned long req_rate,
>  	struct ingenic_cgu *cgu = ingenic_clk->cgu;
>  	unsigned long rate, flags;
>  	unsigned int hw_div, div;
> -	u32 reg, mask;
> +	u32 reg, mask, d_reg;
>  	int ret = 0;
>  
>  	if (clk_info->type & CGU_CLK_DIV) {
> @@ -554,6 +613,30 @@ ingenic_clk_set_rate(struct clk_hw *hw, unsigned long req_rate,
>  		reg &= ~(mask << clk_info->div.shift);
>  		reg |= hw_div << clk_info->div.shift;
>  
> +		/*
> +		 * NOTE: Special treatment for the I2S dividers in the X1600.
> +		 * Set the multiplier to 1, imposing an N >= M*2 constraint,
> +		 * also setting D to "automatic" calculation. This permits the
> +		 * treatment of these dividers as normal dividers, although
> +		 * they are configured more similarly to PLLs.
> +		 */
> +		if (clk_info->mdiv.reg) {
> +			if (div < 2) {
> +				spin_unlock_irqrestore(&cgu->lock, flags);
> +				return -EINVAL;
> +			}
> +
> +			mask = GENMASK(clk_info->mdiv.bits - 1, 0);
> +			reg &= ~(mask << clk_info->mdiv.shift);
> +			reg |= 1 << clk_info->mdiv.shift;

[Severity: High]
How does the div < 2 constraint here in ingenic_clk_set_rate() interact with
ingenic_clk_calc_div() and the hardware N field encoding?

In ingenic_clk_calc_div(), div is clamped to a minimum of clk_info->div.div
(which is 1 for X1600_CLK_I2S0 and X1600_CLK_I2S1) without checking
clk_info->mdiv.reg:

	div = clamp_t(unsigned int, div, clk_info->div.div,
		      clk_info->div.div << clk_info->div.bits);

When a rate >= parent_rate is requested, ingenic_clk_determine_rate() ->
ingenic_clk_calc_div() calculates div == 1 and reports parent_rate as a valid
rounded rate, but the subsequent call to ingenic_clk_set_rate() fails with
-EINVAL because div < 2.

Additionally, just before the if (clk_info->mdiv.reg) block,
ingenic_clk_set_rate() calculates:

	hw_div = ((div / clk_info->div.div) - 1);

and writes hw_div (which is div - 1) into the N field at clk_info->div.shift,
while writing a raw 1 into the M field at clk_info->mdiv.shift. Because the
I2S fractional divider uses unoffset M and N values (f_out = f_in * M / N
with N >= 2 * M), doesn't subtracting 1 from div program N = div - 1 instead
of N = div (and write N = 1, M = 1 when div == 2, violating N >= 2 * M), while
ingenic_clk_recalc_rate() adds 1 back when reading N and ignores mdiv?

> +
> +			if (clk_info->nddiv.reg) {
> +				d_reg = readl(cgu->base + clk_info->nddiv.reg);
> +				d_reg &= ~(1 << clk_info->nddiv.explicit_d_bit);
> +				d_reg |= 1 << clk_info->nddiv.explicit_n_bit;
> +			}
> +		}
> +
>  		/* clear the stop bit */
>  		if (clk_info->div.stop_bit != -1)
>  			reg &= ~BIT(clk_info->div.stop_bit);
> @@ -565,6 +648,9 @@ ingenic_clk_set_rate(struct clk_hw *hw, unsigned long req_rate,
>  		/* update the hardware */
>  		writel(reg, cgu->base + clk_info->div.reg);
>  
> +		if (clk_info->nddiv.reg)
> +			writel(d_reg, cgu->base + clk_info->nddiv.reg);

[Severity: Medium]
Can d_reg be used uninitialized here in ingenic_clk_set_rate(), and does the
explicit_n_bit shift overflow a signed 32-bit int?

d_reg is only initialized when both clk_info->mdiv.reg and clk_info->nddiv.reg
are non-zero, whereas writel(d_reg, cgu->base + clk_info->nddiv.reg) only
checks clk_info->nddiv.reg, which triggers a -Wmaybe-uninitialized compiler
warning when compiling cgu.c.

Also, in x1600_cgu_clocks[], X1600_CLK_I2S0 and X1600_CLK_I2S1 set
explicit_n_bit to 31:

	.nddiv = { CGU_REG_I2S0CDR1, 31, 30 },

Should the bit operations on explicit_d_bit and explicit_n_bit use BIT()
instead of shifting a signed int 1 << 31?

> diff --git a/drivers/clk/ingenic/x1600-cgu.c b/drivers/clk/ingenic/x1600-cgu.c
> new file mode 100644
> index 0000000000000..627203f5cabe8
> --- /dev/null
> +++ b/drivers/clk/ingenic/x1600-cgu.c

[ ... ]

> +static int x1600_otg_phy_enable(struct clk_hw *hw)
> +{
> +	void __iomem *reg_opcr		= cgu->base + CGU_REG_OPCR;
> +
> +	writel((readl(reg_opcr) & ~OPCR_GATEUSBPHYCLK) | OPCR_SPENDN0, reg_opcr);
> +
> +	return 0;
> +}
> +
> +static void x1600_otg_phy_disable(struct clk_hw *hw)
> +{
> +	void __iomem *reg_opcr		= cgu->base + CGU_REG_OPCR;
> +
> +	writel((readl(reg_opcr) & ~OPCR_SPENDN0) | OPCR_GATEUSBPHYCLK, reg_opcr);
> +}

[Severity: Medium]
Should x1600_otg_phy_enable() and x1600_otg_phy_disable() acquire cgu->lock
around the read-modify-write of CGU_REG_OPCR?

X1600_CLK_RTC also uses CGU_REG_OPCR (.mux = { CGU_REG_OPCR, 2, 1 }), and
ingenic_clk_set_parent() modifies CGU_REG_OPCR under the clock framework
prepare_lock and cgu->lock, while clk_enable() and clk_disable() invoke
x1600_otg_phy_enable() and x1600_otg_phy_disable() under enable_lock without
holding cgu->lock. Can concurrent OTG PHY enable/disable and RTC reparenting
race on CGU_REG_OPCR?

[ ... ]

> +static void
> +x1600_pll_calc_m_n_od(const struct ingenic_cgu_pll_info *pll_info,
> +		      unsigned long rate, unsigned long parent_rate,
> +		      unsigned int *pm, unsigned int *pn, unsigned int *pod,
> +		      unsigned int *pod1)
> +{
> +	const unsigned int od_max = pll_info->od_max - 1;
> +	const unsigned int od1_max = pll_info->od1_max - 1;
> +	const unsigned int m_max = GENMASK(pll_info->m_bits - 1, 0);
> +	const unsigned int n_max = GENMASK(pll_info->n_bits - 1, 0);
> +	unsigned long m, n;
> +	unsigned int od = 1, od1 = 1;
> +	const unsigned long fvco_min = 600000000, fvco_max = 2400000000;
> +	unsigned long fvco;
> +
> +	/*
> +	 * Combined output divider range:
> +	 *
> +	 * ceil(fvco_min / rate) - fvco_max / rate
> +	 */
> +	unsigned int od_combined_min = (fvco_min + rate) / rate;
> +	unsigned int od_combined_max = fvco_max / rate;
> +	unsigned int od_combined;
> +
> +	for (od_combined = od_combined_min; od_combined <= od_combined_max; od_combined++) {
> +		od = int_sqrt(od_combined);
> +		od1 = od_combined / od;
> +
> +		/* od == floor(sqrt(od_combined)); od1 >= od */
> +		if ((od * od1 == od_combined) && od <= od_max && od1 <= od1_max) {
> +			fvco = rate * od_combined;
> +			rational_best_approximation(fvco, parent_rate, m_max, n_max, &m, &n);
> +			break;
> +		}
> +	}
> +
> +	*pm = m;
> +	*pn = n;
> +	*pod = od;
> +	*pod1 = od1;
> +}

[Severity: High]
What happens in x1600_pll_calc_m_n_od() if the loop finishes without finding a
matching od_combined?

m and n are declared without initialization and are only assigned inside the
if ((od * od1 == od_combined) && od <= od_max && od1 <= od1_max) block. Since
od_max and od1_max are 7, the maximum valid od * od1 product is 49. If a
consumer requests a rate > fvco_max (where od_combined_min > od_combined_max),
a rate <= ~12.24 MHz (where od_combined_min >= 50), or a rate where
[od_combined_min, od_combined_max] contains no integer divisible by
int_sqrt(od_combined) with both factors <= 7 (such as 43..48), the loop exits
without initializing m and n, and leaves od and od1 with the last tested
values (which can exceed od_max / od1_max).

When x1600_pll_calc_m_n_od() returns to ingenic_pll_calc() and
ingenic_pll_set_rate():

	return div_u64((u64)parent_rate * m * pll_info->rate_multiplier,
		n * od * od1);

can uninitialized m and n cause a division-by-zero panic if n is 0, or cause
ingenic_pll_set_rate() to write uninitialized m and n and out-of-range od and
od1 values into the PLL control register?

Also, in the od_combined_min calculation:

	unsigned int od_combined_min = (fvco_min + rate) / rate;

when fvco_min is an exact multiple of rate (for example, rate == 600 MHz),
this computes (fvco_min / rate) + 1 instead of ceil(fvco_min / rate). Should
this use DIV_ROUND_UP(fvco_min, rate) (after guarding against rate == 0)?

> +
> +static const struct ingenic_cgu_clk_info x1600_cgu_clocks[] = {
> +
> +	/* External clocks */
> +
> +	[X1600_CLK_EXCLK] = { "ext", CGU_CLK_EXT },
> +	[X1600_CLK_RTCLK] = { "rtc", CGU_CLK_EXT },
> +	[X1600_CLK_12M] = { "clk12m", CGU_CLK_EXT },

[Severity: High]
Will registering X1600_CLK_12M as CGU_CLK_EXT cause x1600_cgu_init() ->
ingenic_cgu_register_clocks() -> ingenic_register_clock() to fail with
-ENODEV on Device Trees that follow the ingenic,cgu.yaml binding?

In ingenic_register_clock(), every CGU_CLK_EXT clock is required to be
present in the CGU Device Tree node:

	if (clk_info->type == CGU_CLK_EXT) {
		clk = of_clk_get_by_name(cgu->np, clk_info->name);
		if (IS_ERR(clk)) {
			pr_err("%s: no external clock '%s' provided\n",
			       __func__, clk_info->name);
			err = -ENODEV;
			goto out;
		}

However, Documentation/devicetree/bindings/clock/ingenic,cgu.yaml only allows
two input clocks in clock-names ("ext" and "rtc"/"osc32k") and does not
define "clk12m".

[ ... ]

> +	[X1600_CLK_CIMMCLK] = {
> +		"cim_mclk", CGU_CLK_MUX | CGU_CLK_DIV | CGU_CLK_GATE,
> +		.parents = { X1600_CLK_SCLKA, X1600_CLK_MPLL,
> +			     X1600_CLK_EPLL, -1 },
> +		.mux = { CGU_REG_CIMCDR, 30, 2 },
> +		.div = { CGU_REG_CIMCDR, 0, 1, 8, 30, 29, 28 },
> +		.gate = { CGU_REG_CLKGR0, 22 },
> +	},

[Severity: High]
In x1600_cgu_clocks[X1600_CLK_CIMMCLK], do the .mux and .div definitions
overlap at bit 30 of CGU_REG_CIMCDR?

.mux = { CGU_REG_CIMCDR, 30, 2 } occupies bits 31:30 of CGU_REG_CIMCDR, while
.div = { CGU_REG_CIMCDR, 0, 1, 8, 30, 29, 28 } sets ce_bit = 30, busy_bit = 29,
and stop_bit = 28. When ingenic_clk_set_rate() sets BIT(clk_info->div.ce_bit),
won't it overwrite bit 30 of the parent mux field in CGU_REG_CIMCDR instead of
setting CE_CIM (bit 29) and polling BUSY_CIM (bit 28)?

> +
> +	[X1600_CLK_EXCLK_DIV512] = {
> +		"exclk_div512", CGU_CLK_FIXDIV,
> +		.parents = { X1600_CLK_EXCLK },
> +		.fixdiv = { 512 },
> +	},
> +
> +	[X1600_CLK_RTC] = {
> +		"rtc_ercs", CGU_CLK_MUX | CGU_CLK_GATE,
> +		.parents = { X1600_CLK_EXCLK_DIV512, X1600_CLK_RTCLK },
> +		.mux = { CGU_REG_OPCR, 2, 1},
> +	},

[Severity: High]
X1600_CLK_RTC specifies CGU_CLK_MUX | CGU_CLK_GATE, but .gate is not
initialized here.

Because .gate is zero-initialized to { .reg = 0, .bit = 0 }, won't enabling or
disabling rtc_ercs (including during clk_disable_unused() or
ingenic_clk_set_parent()) call ingenic_cgu_gate_set() on register 0x00
(CGU_REG_CLOCKCONTROL) bit 0, modifying the LSB of the X1600_CLK_CPU divider?

> +
> +	/* Custom (SoC-specific) OTG PHY */
> +
> +	[X1600_CLK_OTGPHY] = {
> +		"otg_phy", CGU_CLK_CUSTOM,
> +		.parents = { X1600_CLK_12M },
> +		.custom = { &x1600_otg_phy_ops },
> +	},

[Severity: Medium]
Should .parents for X1600_CLK_OTGPHY initialize the remaining slots to -1, i.e.
{ X1600_CLK_12M, -1, -1, -1 }?

In ingenic_register_clock(), clocks with CGU_CLK_CUSTOM iterate over all
ARRAY_SIZE(clk_info->parents) (4) entries and register every entry that is not
-1:

	if (caps & (CGU_CLK_MUX | CGU_CLK_CUSTOM)) {
		clk_init.num_parents = 0;

		if (caps & CGU_CLK_MUX)
			num_possible = 1 << clk_info->mux.bits;
		else
			num_possible = ARRAY_SIZE(clk_info->parents);

		for (i = 0; i < num_possible; i++) {
			if (clk_info->parents[i] == -1)
				continue;

With .parents = { X1600_CLK_12M }, the remaining three entries are
zero-initialized to 0 (X1600_CLK_EXCLK, "ext"), registering otg_phy with four
parents ("clk12m", "ext", "ext", "ext") instead of one.

[ ... ]

> +	[X1600_CLK_SADC] = {
> +		"sadc", CGU_CLK_GATE,
> +		.parents = { X1600_CLK_EXCLK, -1, -1, -1 },
> +		.gate = { CGU_REG_CLKGR0, 14 },
> +	},
> +
> +	[X1600_CLK_UART0] = {
> +		"uart0", CGU_CLK_GATE,
> +		.parents = { X1600_CLK_EXCLK, -1, -1, -1 },
> +		.gate = { CGU_REG_CLKGR0, 14 },
> +	},

[Severity: High]
Both X1600_CLK_SADC and X1600_CLK_UART0 use .gate = { CGU_REG_CLKGR0, 14 }.
Should X1600_CLK_SADC use bit 13 of CGU_REG_CLKGR0 instead, so that gating
unused sadc (for instance in clk_disable_unused()) does not gate uart0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792955907.git.hns@goldelico.com?part=2