Re: [PATCH v2 02/11] clk: sunxi-ng: add Allwinner B288 CCU driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clk: sunxi-ng: pll-ddr0/1 lack required update bit logic causing rate desync
--

commit e45a1df66351c0556d843abaca74cd27fa7b5854
Author: Pablo Mazzini <pmazzini@gmail.com>

clk: sunxi-ng: add Allwinner B288 CCU driver

This commit introduces the Clock Control Unit (CCU) driver for the Allwinner 
B288 SoC. It covers the PLLs, the CPU/AHB/APB tree, the module clocks, and 
the bus gates and resets.

> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-b288.c b/drivers/clk/sunxi-ng/ccu-sun8i-b288.c
> new file mode 100644
> index 0000000000000..3327517c4c36e
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-b288.c

[ ... ]

> +/*
> + * PLL_DDR0 @ 0x020 -- DIVERGES FROM A64, which is NKM with N[12:8] K[5:4].
> + * The B288 has a 7-bit N and no K: the value boot0 leaves here sets
> + * bit 13, which the A64's 5-bit N field cannot hold.
> + *
> + * The vendor's bit-30 apply/update has no mainline equivalent and is not
> + * modelled, nor are the sigma-delta patterns at 0x2ac..0x2b8.
> + */
> +static SUNXI_CCU_NM_WITH_GATE_LOCK(pll_ddr0_clk, "pll-ddr0",
> +				   "osc24M", 0x020,
> +				   8, 7,	/* N */
> +				   0, 2,	/* M */
> +				   BIT(31),	/* gate */
> +				   BIT(28),	/* lock */
> +				   CLK_SET_RATE_UNGATE);

[Severity: Medium]
Does this configuration break frequency scaling for pll-ddr0? 

The pll-ddr0 clock is registered with the CLK_SET_RATE_UNGATE flag but relies 
on ccu_nm_ops. If a driver or subsystem (such as devfreq) calls clk_set_rate() 
on pll-ddr0, it will write new M/N factors to the registers but fail to toggle 
the required update bit (bit 30).

This causes the hardware to ignore the new configuration while the kernel 
state reflects the new frequency, breaking clock tree consistency. Since the 
comment above mentions the vendor's bit-30 apply/update has no mainline 
equivalent and is not modelled, how does the driver intend to handle rate 
updates safely?

[ ... ]

> +/* PLL_DDR1 @ 0x04c -- B288 only. Same layout; this is the one feeding DRAM. */
> +static SUNXI_CCU_NM_WITH_GATE_LOCK(pll_ddr1_clk, "pll-ddr1",
> +				   "osc24M", 0x04c,
> +				   8, 7,	/* N */
> +				   0, 2,	/* M */
> +				   BIT(31),	/* gate */
> +				   BIT(28),	/* lock */
> +				   CLK_SET_RATE_UNGATE);

[Severity: Medium]
Does pll-ddr1 also suffer from the missing bit 30 update logic during 
frequency scaling?

Similar to pll_ddr0_clk, this clock uses the same operations and exposes a 
set_rate callback, but appears to lack the necessary mechanism to toggle 
the bit-30 update bit when writing the new configuration.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927151016.186493-1-pmazzini@gmail.com?part=2