Re: [PATCH v2 2/2] clk: amlogic: Add A9 peripherals clock controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The divider and gate definitions for the `gen` clock overlap on bit 11 in the `GEN_CLK_CTRL` register.
- [High] The `a9_hdmitx1_pixel` clock incorrectly references `a9_hdmitx_pixel_sel.hw` as its parent instead of its own multiplexer `a9_hdmitx1_pixel_sel.hw`.
- [High] The `usb_48m_pre` multiplexer is missing a value translation table, mapping the 6th parent to an unconnected hardware channel.
- [Low] The Kconfig symbol `COMMON_CLK_A9_PERIPHERALS` implies a non-existent symbol `COMMON_CLK_A9_PLL`.
- [Low] The `COMPILE_TEST` option is used incorrectly in the Kconfig `default` statement and is missing from `depends on`.
--

commit bae2f7359df3d153442d5e8943999c98d3682a16
Author: Jian Hu <jian.hu@amlogic.com>
Subject: clk: amlogic: Add A9 peripherals clock controller driver

Add the peripherals clock controller driver for the Amlogic A9 SoC family.

> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -132,6 +132,21 @@ config COMMON_CLK_A1_PERIPHERALS
>  	  device, A1 SoC Family. Say Y if you want A1 Peripherals clock
>  	  controller to work.
>  
> +config COMMON_CLK_A9_PERIPHERALS
> +	tristate "Amlogic A9 SoC peripherals clock controller support"
> +	depends on ARM64
> +	default ARCH_MESON || COMPILE_TEST

[Severity: Low]
Is the COMPILE_TEST directive intended to be here? Placing COMPILE_TEST in the
default expression enables this driver by default on all ARM64 architectures
when compile testing is active. Should it instead use
"depends on ARM64 || COMPILE_TEST" and "default ARCH_MESON"?

> +	select COMMON_CLK_MESON_REGMAP
> +	select COMMON_CLK_MESON_CLKC_UTILS
> +	select COMMON_CLK_MESON_DUALDIV
> +	select COMMON_CLK_MESON_VID_PLL_DIV
> +	imply COMMON_CLK_SCMI
> +	imply COMMON_CLK_A9_PLL

[Severity: Low]
Does COMMON_CLK_A9_PLL exist in the tree? Kconfig will silently ignore
non-existent implied symbols, which might mean a missing patch or typo.

> +	help

[ ... ]

> diff --git a/drivers/clk/meson/a9-peripherals.c b/drivers/clk/meson/a9-peripherals.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/meson/a9-peripherals.c
> @@ -0,0 +1,1927 @@

[ ... ]

> +/* Channel 5 and 6 are unconnected. */
> +static u32 a9_usb_250m_parents_val_table[] = { 0, 1, 2, 3, 4, 7 };
> +static const struct clk_parent_data a9_usb_250m_parents[] = {
> +	{ .fw_name = "fdiv4", },
> +	{ .fw_name = "fdiv3", },
> +	{ .fw_name = "fdiv5", },
> +	{ .fw_name = "fdiv2", },
> +	{ .fw_name = "fdiv7", },
> +	{ .fw_name = "fdiv2p5", }
> +};
> +
> +static A9_COMP_SEL(usb_250m, USB_CLK_CTRL, 9, 0x7, a9_usb_250m_parents,
> +		   a9_usb_250m_parents_val_table);
> +static A9_COMP_DIV(usb_250m, USB_CLK_CTRL, 0, 7);
> +static A9_COMP_GATE(usb_250m, USB_CLK_CTRL, 8, 0);
> +
> +static const struct clk_parent_data a9_usb_48m_pre_parents[] = {
> +	{ .fw_name = "fdiv4", },
> +	{ .fw_name = "fdiv3", },
> +	{ .fw_name = "fdiv5", },
> +	{ .fw_name = "fdiv2", },
> +	{ .fw_name = "fdiv7", },
> +	{ .fw_name = "fdiv2p5", }
> +};
> +
> +static A9_COMP_SEL(usb_48m_pre, USB_CLK_CTRL, 25, 0x7, a9_usb_48m_pre_parents,
> +		   NULL);

[Severity: High]
Does usb_48m_pre need a value translation table as well? It shares the same
parent list as usb_250m, where the 6th parent (fdiv2p5) maps to hardware value
7 because channels 5 and 6 are unconnected. By passing NULL here, selecting
fdiv2p5 writes value 5 to the register, connecting it to a dead channel.

> +static A9_COMP_DIV(usb_48m_pre, USB_CLK_CTRL, 16, 7);
> +static A9_COMP_GATE(usb_48m_pre, USB_CLK_CTRL, 24, 0);

[ ... ]

> +static A9_COMP_SEL(gen, GEN_CLK_CTRL, 12, 0x1f, a9_gen_parents,
> +		   a9_gen_parents_val_table);
> +static A9_COMP_DIV(gen, GEN_CLK_CTRL, 0, 12);
> +static A9_COMP_GATE(gen, GEN_CLK_CTRL, 11, 0);

[Severity: High]
Can these definitions cause conflicts? The divider for the gen clock has a
width of 12 (bits 0-11), while the gate is defined at bit 11 of the same
register.

If the divider is updated, it might unintentionally toggle the gate, and
toggling the gate might corrupt the most significant bit of the divider.

[ ... ]

> +static struct clk_regmap a9_hdmitx1_pixel_sel = {
> +	.data = &(struct clk_regmap_mux_data){
> +		.offset = HDMI_CLK_CTRL,
> +		.mask = 0xf,
> +		.shift = 24,
> +		.table = a9_vid_parents_val_table,
> +	},
> +	.hw.init = CLK_HW_INIT_PARENTS_HW("hdmitx1_pixel_sel", a9_vid_parents
> +			, &clk_regmap_mux_ops, 0),
> +};
> +
> +static struct clk_regmap a9_hdmitx1_pixel = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = VID_CLK_CTRL2,
> +		.bit_idx = 12,
> +	},
> +	.hw.init = CLK_HW_INIT_HW("hdmitx1_pixel", &a9_hdmitx_pixel_sel.hw,

[Severity: High]
Should this reference a9_hdmitx1_pixel_sel.hw instead? It appears to be bound
to the primary HDMI TX selector (a9_hdmitx_pixel_sel.hw). This would tie the
secondary HDMI output to the primary multiplexer.

> +				  &clk_regmap_gate_ops, CLK_SET_RATE_PARENT),
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-a9_peripherals-v2-0-ee1b8c0a1e6c@amlogic.com?part=2