Re: [PATCH v10 08/12] clk: zte: Introduce a driver for zx297520v3 top clocks

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] `ahb-wclk` is assigned the wrong parent mux, breaking the clock topology.
- [Low] The driver fails to compile because it uses `const` variables to initialize static array elements, which violates the C standard for constant expressions.
--

--- Patch [8]: [PATCH v10 08/12] clk: zte: Introduce a driver for zx297520v3 top clocks ---
commit e0f9a1aafed8640102d7aca0bbb0d89079e89c5f
Author: Stefan Dösinger <[email protected]>

clk: zte: Introduce a driver for zx297520v3 top clocks

This commit introduces a new clock driver for the ZTE zx297520v3 SoC.
It supports basic clocks, including PLLs, the AHB bus, timers, and the
USB controller.

> diff --git a/drivers/clk/zte/clk-zx297520v3.c b/drivers/clk/zte/clk-zx297520v3.c
> new file mode 100644
> index 0000000000000..504075590e6d2
> --- /dev/null
> +++ b/drivers/clk/zte/clk-zx297520v3.c
[ ... ]
> +static const struct zx_clock zx297520v3_top_clocks[] = {
[ ... ]
> +	/* osc26m division for clk_outX */
> +	[ZX_CLK_OSC26M_D2]         = FIXED_DIV("osc26m-d2", clk_main[0], 2),

[Severity: Low]
Will this cause a compiler error?

In standard C, variables qualified with const are not considered constant
expressions. Using clk_main array elements here to initialize elements of a
static array will likely trigger an initializer element is not constant error.

[ ... ]
> +	[ZX_CLK_WDT_T18_DIV]       = DIV("wdt-t18-div", PARENT_ID(ZX_CLK_WDT_T18_MUX),
> +					 0x48, 8, 4),
> +	[ZX_CLK_USIM1_DIV]         = DIV("usim1-div", clk_unknown,
> +					 0x48, 12, 1),

[Severity: Low]
Is this another place where a const variable (clk_unknown) will trigger a
compiler error when used as a static array initializer?

[ ... ]
> +	/*
> +	 * AHB clock gates: Disabling these cuts off the A53 from register access into 0x130000/
> +	 * 0x140000, including the topcrm region, so it can't be re-enabled. Mark them critical for
> +	 * that reason.
> +	 *
> +	 * Gating this off does not stop the M0 rproc, nor does it stop M0 from accessing topcrm. It
> +	 * does however gate off M0 from the AXI interconnect, so it can't read RAM, Matrix, etc. So
> +	 * these bits here merely gate the bus, not the actual devices.
> +	 */
> +	[ZX_CLK_AHB_PCLK]          = GATE("ahb-pclk", PARENT_ID(ZX_CLK_AHB_MUX), 0x54, 12,
> +					  CLK_IS_CRITICAL),
> +	[ZX_CLK_AHB_WCLK]          = GATE("ahb-wclk", PARENT_ID(ZX_CLK_M0_MUX), 0x54, 13,
> +					  CLK_IS_CRITICAL),

[Severity: Medium]
Does ahb-wclk have the wrong parent mux assigned here?

It is currently assigned ZX_CLK_M0_MUX instead of ZX_CLK_AHB_MUX. The comments
state that changing the AHB mux affects USB network speeds, which implies
that ZX_CLK_AHB_MUX is the actual hardware parent.

[ ... ]
> +	/*
> +	 * How does this RTC work? I don't know, the ZTE kernel does not talk to it. The actual RTC
> +	 * is on the I2C-connected PMIC.
> +	 */
> +	[ZX_CLK_RTC_WCLK]         = GATE("rtc-wclk", clk_unknown, 0x54, 1, 0),

[Severity: Low]
Will this face the same static array initializer error as the other usages
of clk_unknown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy