Re: [PATCH v6 26/27] clk: mediatek: Add MT8189 scp clock support
From: sashiko-bot@kernel.org
Date: 2026-09-04 16:28:42
Also in:
linux-clk
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect register offsets defined in `scp_cg_regs` for SCP SPI clocks break clock operations and status reporting.
--
commit 761866944873dbacd9da82a6105c5dd33a62520c
Author: Louis-Alexis Eyraud [off-list ref]
clk: mediatek: Add MT8189 scp clock support
Add support for the MT8189 scp clock controller,
which provides clock gate control for System Control Processor.
quoted hunk ↗ jump to hunk
diff --git a/drivers/clk/mediatek/clk-mt8189-scp.c b/drivers/clk/mediatek/clk-mt8189-scp.c new file mode 100644 index 0000000000000..75f087d819c08 --- /dev/null +++ b/drivers/clk/mediatek/clk-mt8189-scp.c@@ -0,0 +1,76 @@
[ ... ]
+static const struct mtk_gate_regs scp_cg_regs = {
+ .set_ofs = 0x4,
+ .clr_ofs = 0x8,
+ .sta_ofs = 0x4,
+};
+
+#define GATE_SCP(_id, _name, _parent, _shift) \
+ GATE_MTK(_id, _name, _parent, &scp_cg_regs, _shift, &mtk_clk_gate_ops_setclr_inv)
+
+static const struct mtk_gate scp_clks[] = {
+ GATE_SCP(CLK_SCP_SET_SPI0, "scp_set_spi0", "clk26m", 0),
+ GATE_SCP(CLK_SCP_SET_SPI1, "scp_set_spi1", "clk26m", 1),
+};[Severity: High] Does the scp_cg_regs struct assign the correct register offsets for the SCP SPI clocks? It appears that sta_ofs is set identically to set_ofs at 0x4, which is typically a write-only W1S register in MediaTek SET/CLR clock gates. If the status offset points to a write-only register, will the common clock framework read invalid data when checking the status of scp_set_spi0 and scp_set_spi1 during late init? This could break state tracking, preventing clk_disable_unused() from correctly powering off these clocks. Additionally, the set_ofs and clr_ofs are defined as 0x4 and 0x8 here, but are inverted compared to scp_iic_cg_regs below.
+static const struct mtk_gate_regs scp_iic_cg_regs = {
+ .set_ofs = 0x8,
+ .clr_ofs = 0x4,
+ .sta_ofs = 0x0,
+};
+
+#define GATE_SCP_IIC(_id, _name, _parent, _shift) \
+ GATE_MTK(_id, _name, _parent, &scp_iic_cg_regs, _shift, &mtk_clk_gate_ops_setclr_inv)[Severity: High] Since GATE_SCP and GATE_SCP_IIC both use mtk_clk_gate_ops_setclr_inv, doesn't this mean that enabling an SPI clock writes to 0x4, while enabling an IIC clock writes to 0x8? Could this swap in offsets cause the SPI clocks to remain disabled when requested by consumer drivers, breaking clock operations deterministically when clk_prepare_enable() or clk_disable_unprepare() are called on scp_set_spi0 or scp_set_spi1? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260904-mt8189-clocks-system-base-v6-0-5df247f56938@collabora.com?part=26