Re: [PATCH v2 6/8] clk: sunxi-ng: a733: Add mod clocks support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Undefined Behavior in ccu_mp_set_rate() due to GENMASK(-1, 0) silently clears clock registers.
--

--- Patch [6]: [PATCH v2 6/8] clk: sunxi-ng: a733: Add mod clocks support ---
commit 9fae602071158ab4bb78bf12e40f53a7e4b2f550
Author: Junhui Liu <junhui.liu@pigmoral.tech>

clk: sunxi-ng: a733: Add mod clocks support
    
Add the module clocks found in the Allwinner A733 SoC, including video,
storage, interfaces and others. This implementation accounts for changes
in register offsets and introduces support for new modules.

> diff --git a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
> index c42619afb52e3..bf26e310f08a9 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
[ ... ]
> +static SUNXI_CCU_MP_DATA_WITH_MUX_GATE(timer0_clk, "timer0", timer_parents, 0x800,
> +				       0, 0,		/* no M */
> +				       0, 3,		/* P */
> +				       24, 3,		/* mux */
> +				       BIT(31),		/* gate */
> +				       0);

[Severity: Critical]
This is a pre-existing issue, but won't specifying a width of 0 for the
M divider cause undefined behavior in the CCU framework?

When a driver or the clock framework calls clk_set_rate() on these newly
introduced clocks (e.g., timer0_clk through timer9_clk, mbus_clk),
ccu_mp_set_rate() clears the M and P divider fields:

drivers/clk/sunxi-ng/ccu_mp.c:ccu_mp_set_rate() {
	...
	reg &= ~GENMASK(cmp->m.width + cmp->m.shift - 1, cmp->m.shift);
	reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);
	...
}

Because these clocks are instantiated with width = 0, this evaluates to
~GENMASK(-1, 0) at runtime. GENMASK(-1, 0) computes ~0UL >> 64. On ARM64
hardware, the dynamic shift amount is read from the bottom 6 bits of the
register (64 & 63 == 0), resulting in a shift of 0, leaving the value as
~0UL (all ones).

The ~GENMASK thus evaluates to 0, which completely zeroes the hardware
register, including the clock gate bit (BIT(31)).

[ ... ]
> +static SUNXI_CCU_MP_DATA_WITH_MUX_GATE_FEAT(dram0_clk, "dram0", dram_parents, 0xc00,
> +					    0, 4,	/* M */
> +					    0, 0,	/* no P */
> +					    24, 3,	/* mux */
> +					    BIT(31),	/* gate */
> +					    CLK_IS_CRITICAL,
> +					    CCU_FEATURE_UPDATE_BIT);

[Severity: Critical]
This isn't a bug introduced by this patch, but dram0_clk is instantiated
with a P width of 0. Calling set_rate on it will trigger ~GENMASK(-1, 0)
and wipe its register. Since this is the DRAM clock, won't disabling it
instantly hang the system?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-a733-clk-v2-0-974d188cbe0c@pigmoral.tech?part=6