Re: [PATCH v2 10/14] clk: samsung: exynos8855: add block usb clock support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: samsung: incorrect gate_dbg_offset causes double-offset memory reads
--

commit 8e0eb8d00c0f187c13c3ca956facb4eb6bd98db3
Author: Raghav Sharma <raghav.s@samsung.com>

clk: samsung: exynos8855: add block usb clock support
    
Register compatible and cmu_info data to support clocks.
CMU_USB, this provides clocks for USB

> diff --git a/drivers/clk/samsung/clk-exynos8855.c b/drivers/clk/samsung/clk-exynos8855.c
> index 16d71db11ad06..010ca79116d30 100644
> --- a/drivers/clk/samsung/clk-exynos8855.c
> +++ b/drivers/clk/samsung/clk-exynos8855.c
> @@ -20,11 +20,13 @@
>  #define CLKS_NR_PERIC                   (CLK_GOUT_USI_PERIC + 1)
>  #define CLKS_NR_PERIS                   (CLK_GOUT_WDT1_PERIS + 1)
>  #define CLKS_NR_HSI                     (CLK_GOUT_SYSREG_HSI  + 1)
> +#define CLKS_NR_USB                     (CLK_GOUT_SYSREG_USB + 1)
>  
>  #define EXYNOS8855_HSI_GATE_DBG_OFFSET      0x2000
>  #define EXYNOS8855_PERIS_GATE_DBG_OFFSET      0x2004
>  #define EXYNOS8855_PERIC_GATE_DBG_OFFSET      0x2024
>  #define EXYNOS8855_TOP_GATE_DBG_OFFSET      0x20A0
> +#define EXYNOS8855_USB_GATE_DBG_OFFSET      0x2004
>  
>  #define EXYNOS8855_DRCG_EN_OFFSET       0x104
>  #define EXYNOS8855_MEMCLK_OFFSET        0x108
> @@ -1261,6 +1263,79 @@ static const struct samsung_cmu_info hsi_cmu_info __initconst = {
>  	.memclk_offset          = EXYNOS8855_MEMCLK_OFFSET,
>  };
>  
> +/* ---- CMU_USB --------------------------------------------------------- */
> +
> +/* Register Offset definitions for CMU_USB (0x13000000) */
> +#define CMU_USB_PLL_CON0_MUX_CLKCMU_USB_NOC_USER                   0x600
> +#define CMU_USB_PLL_CON0_MUX_CLKCMU_USB_USB20DRD_USER              0x610
> +#define CMU_USB_CLK_CON_MUX_CLK_USB_USB20DRD                       0x1000
> +#define CMU_USB_CLK_CON_DIV_CLK_USB_NOC_DIV3                       0x1800
> +#define CMU_USB_CLK_CON_GAT_BLK_USB_UID_CMU_USB_IPCLKPORT_PCLK     0x2000
> +#define CMU_USB_CLK_CON_GAT_BLK_USB_UID_SYSREG_USB_IPCLKPORT_PCLK  0x2030
> +#define CMU_USB_CONTROLLER_OPTION0_CMU_CTRL                        0x800

[ ... ]

> +static const struct samsung_gate_clock usb_gate_clks[] __initconst = {
> +	/* System will hang if this critical clock is gated */
> +	GATE(CLK_GOUT_CMU_USB, "gout_cmu_usb",
> +	     "mout_clkcmu_usb_noc_user",
> +	     CMU_USB_CLK_CON_GAT_BLK_USB_UID_CMU_USB_IPCLKPORT_PCLK,
> +	     21, 0, 0),
> +	GATE(CLK_GOUT_SYSREG_USB, "gout_sysreg_usb",
> +	     "mout_clkcmu_usb_noc_user",
> +	     CMU_USB_CLK_CON_GAT_BLK_USB_UID_SYSREG_USB_IPCLKPORT_PCLK,
> +	     21, 0, 0),
> +};
> +
> +static const struct samsung_cmu_info usb_cmu_info __initconst = {
> +	.mux_clks               = usb_mux_clks,
> +	.nr_mux_clks            = ARRAY_SIZE(usb_mux_clks),
> +	.div_clks               = usb_div_clks,
> +	.nr_div_clks            = ARRAY_SIZE(usb_div_clks),
> +	.gate_clks              = usb_gate_clks,
> +	.nr_gate_clks           = ARRAY_SIZE(usb_gate_clks),
> +	.nr_clk_ids             = CLKS_NR_USB,
> +	.clk_regs               = usb_clk_regs,
> +	.nr_clk_regs            = ARRAY_SIZE(usb_clk_regs),
> +	.sysreg_clk_regs	= dcrg_memclk_sysreg,
> +	.nr_sysreg_clk_regs	= ARRAY_SIZE(dcrg_memclk_sysreg),
> +	.clk_name               = "bus",
> +	.auto_clock_gate        = true,
> +	.gate_dbg_offset        = EXYNOS8855_USB_GATE_DBG_OFFSET,

[Severity: High]
Could this offset result in reading incorrect physical addresses when checking
auto-clock-gating state?

Looking at the CCF framework, the gate debug register address is computed by
adding the gate's absolute offset and the gate_dbg_offset:

drivers/clk/samsung/clk.c:
		reg_offs = ctx->reg_base + list->offset;

		if (ctx->auto_clock_gate && ctx->gate_dbg_offset)
			clk_hw = samsung_register_auto_gate(ctx->dev, NULL,
				list->name, list->parent_name, NULL, NULL,
				list->flags, reg_offs + ctx->gate_dbg_offset,
				list->bit_idx, list->gate_flags, &ctx->lock);

If list->offset is already an absolute offset like 0x2000
(CMU_USB_CLK_CON_GAT_BLK_USB_UID_CMU_USB_IPCLKPORT_PCLK), and we add 0x2004 to
it, the final address becomes reg_base + 0x4004 instead of 0x2004. This would
read unmapped or incorrect hardware registers.

Should EXYNOS8855_USB_GATE_DBG_OFFSET be a relative shift (like 0x4) instead
of an absolute offset?

This problem wasn't introduced by this patch, but it appears the existing
EXYNOS8855_PERIC_GATE_DBG_OFFSET and other definitions also specify absolute
offsets, which would incorrectly double the offset for those gates as well.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009120137.2801911-1-raghav.s@samsung.com?part=10