Re: [PATCH v6 26/27] clk: mediatek: Add MT8189 scp clock support

[sashiko-bot@kernel.org]

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 <louisalexis.eyraud@collabora.com>

    clk: mediatek: Add MT8189 scp clock support
    
    Add support for the MT8189 scp clock controller,
    which provides clock gate control for System Control Processor.

> 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