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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Defining `mbus_clk` with a 0-width `P` prescaler causes undefined behavior in `ccu_mp_set_rate()`, leading to the critical memory bus clock register being completely cleared.
- [High] The MBUS clock requires an update bit to commit rate changes, but it is defined using `ccu_mp` operations which completely ignore the `CCU_FEATURE_UPDATE_BIT` flag.
--

--- Patch [5]: [PATCH v2 5/8] clk: sunxi-ng: a733: Add bus clocks support ---
commit 40f96b18647c7ab7dc42f9a7456046beab510ac6
Author: Junhui Liu <junhui.liu@pigmoral.tech>

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

This commit adds the essential bus clocks in the Allwinner A733 CCU,
including AHB, APB0, APB1, APB_UART, and MBUS. It also marks the MBUS
clock as critical to ensure the memory bus remains operational.

> diff --git a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
> --- a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c

[ ... ]

> +static const struct clk_parent_data mbus_parents[] = {
> +	{ .hw = &sys_24M_clk.hw },
> +	{ .hw = &pll_periph1_600M_clk.hw },
> +	{ .hw = &pll_ddr_clk.common.hw },
> +	{ .hw = &pll_periph1_480M_clk.common.hw },
> +	{ .hw = &pll_periph1_400M_clk.hw },
> +	{ .hw = &pll_npu_clk.common.hw },
> +};
> +static SUNXI_CCU_MP_DATA_WITH_MUX_GATE_FEAT(mbus_clk, "mbus", mbus_parents, 0x588,
> +					    0, 5,	/* M */
> +					    0, 0,	/* no P */

[Severity: Critical]
This is a pre-existing issue in the framework, but does defining mbus_clk
with a 0-width P prescaler cause undefined behavior when the rate is set?

When ccu_mp_set_rate() is called in drivers/clk/sunxi-ng/ccu_mp.c, it
evaluates the mask for the P prescaler:

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

Since p.width is 0, this expands to GENMASK(-1, 0). On 64-bit architectures,
the shift by 64 is masked to 0, which makes ~GENMASK evaluate to 0 and clears
the entire mbus_clk register, including the critical clock gate bit.

Could this lead to a system crash during boot or runtime frequency scaling?

> +					    24, 3,	/* mux */
> +					    BIT(31),	/* gate */
> +					    CLK_IS_CRITICAL,
> +					    CCU_FEATURE_UPDATE_BIT);

[Severity: High]
This is also a pre-existing issue in the framework, but will the hardware
actually commit rate changes for this clock?

The commit message notes that the hardware requires bit 27 to be set for
the configuration to take effect. However, ccu_mp_set_rate() does not
process the CCU_FEATURE_UPDATE_BIT flag before writing the register:

drivers/clk/sunxi-ng/ccu_mp.c:ccu_mp_set_rate() {
    ...
    writel(reg, cmp->common.base + cmp->common.reg);

    spin_unlock_irqrestore(cmp->common.lock, flags);
    ...
}

Since the update bit is never toggled by the ccu_mp operations, does this
leave the software state permanently out of sync with the hardware state
when the mbus_clk rate is configured?

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