Re: [PATCH v3 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:
- [High] Incorrect configuration of `dram0_clk` and `mbus_clk` using `ccu_mp` results in undefined behavior (register wiping) and silent failure to latch hardware rate changes.
--

commit a122063531b2fdc98cf1753dabf17b8cdf9be8ae
Author: Junhui Liu <junhui.liu@pigmoral.tech>

clk: sunxi-ng: a733: Add mod clocks support

This patch adds module clocks found in the Allwinner A733 SoC, including
video, storage, interfaces, and others. The implementation accounts for
register offset changes compared to the A523 SoC and introduces new modules.

> diff --git a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
> index 3a25c349f11ba..e590b211be853 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c

[ ... ]

> @@ -492,6 +499,913 @@ static SUNXI_CCU_MP_DATA_WITH_MUX_GATE_FEAT(mbus_clk, "mbus", mbus_parents, 0x58
>  					    CLK_IS_CRITICAL,
>  					    CCU_FEATURE_UPDATE_BIT);
>  
> +/**************************************************************************
> + *                          mod clocks                                    *
> + **************************************************************************/

[ ... ]

> +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: High]
Does configuring dram0_clk (and similarly mbus_clk, visible in the context
above) using SUNXI_CCU_MP_DATA_WITH_MUX_GATE_FEAT with a 0-width P divider
result in undefined behavior?

When a frequency change is requested for clocks mapped to ccu_mp,
ccu_mp_set_rate() evaluates:
GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift)

With a 0-width P divider, this becomes GENMASK(-1, 0), which expands to
a shift by BITS_PER_LONG. On ARM64 systems, this undefined behavior typically
clears the entire clock register (erasing the critical gate and mux), which
can immediately crash the system.

Additionally, the macro passes the CCU_FEATURE_UPDATE_BIT flag. However,
ccu_mp_set_rate() does not implement this feature flag (unlike ccu_div.c).
Will this cause rate changes to silently fail to latch in the hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905-a733-clk-v3-0-50f238f39c78@pigmoral.tech?part=6