Re: [PATCH v3 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:
- [High] Zero-width P-divider in mbus_clk triggers GENMASK underflow, wiping the entire clock register on rate changes.
- [High] MBUS clock rate changes will not take effect because ccu_mp_set_rate() ignores CCU_FEATURE_UPDATE_BIT.
--

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

    clk: sunxi-ng: a733: Add bus clocks support
    
    Add the essential bus clocks in the Allwinner A733 CCU, including AHB,
    APB0, APB1, APB_UART, and MBUS. These buses are necessary for many other
    functional modules. An additional trace clock is also added as it falls
    within the register address range of the bus clocks, even though it is
    not strictly a bus clock.
    
    The MBUS clock is marked as critical to ensure the memory bus remains
    operational at all times. And the hardware requires an update bit
    (bit 27) to be set so that the configuration takes effect and the
    updated parameters can be correctly read back for the MBUS clock.

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 SUNXI_CCU_MP_DATA_WITH_MUX_GATE_FEAT(mbus_clk, "mbus", mbus_parents, 0x588,
> +					    0, 5,	/* M */
> +					    0, 0,	/* no P */
> +					    24, 3,	/* mux */
> +					    BIT(31),	/* gate */
> +					    CLK_IS_CRITICAL,
> +					    CCU_FEATURE_UPDATE_BIT);

[Severity: High]
Does a zero-width P-divider here inadvertently wipe the register during
rate changes?

Looking at ccu_mp_set_rate(), it evaluates the mask using GENMASK based
on the width and shift:

reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);

With width 0 and shift 0, this yields GENMASK(-1, 0). On 64-bit
architectures like ARM64, the internal shift by BITS_PER_LONG is
modulo-truncated to 0, resulting in ~0UL. Applying ~GENMASK (which is 0)
then zeroes out the entire register. This would clear the critical enable
gate (BIT(31)) and mux settings.

[Severity: High]
Does ccu_mp_set_rate() actually support the update bit feature?

The commit message notes that the hardware requires bit 27 to be set
for the MBUS clock configuration to take effect. However, mbus_clk is
declared using a ccu_mp type macro.

Looking at ccu_mp_set_rate(), it doesn't appear to check for or apply
CCU_FEATURE_UPDATE_BIT like the div and mux callbacks do. Because the MP
framework ignores this flag, could this result in rate changes silently
being ignored by the hardware?

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