Thread (11 messages) flat view 11 messages, 2 authors, 1d ago

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

From: sashiko-bot@kernel.org
Date: 2026-09-05 16:48:55
Also in: linux-clk, linux-sunxi

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 [off-list ref]

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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help