Re: [PATCH 09/10] clk: amlogic: Add A9 peripherals clock controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing multiplexer value table for `usb_48m_pre` clock
- [High] Register bit overlap between `gen` clock divider and gate
- [High] Incorrect parent clock selector referenced by `hdmitx1_pixel`
--

commit 4a10654bfda3abb70d26d386a44e76675278fccb
Author: Jian Hu <jian.hu@amlogic.com>

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/a9-peripherals.c b/drivers/clk/meson/a9-peripherals.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/meson/a9-peripherals.c

[ ... ]

> /* 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 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);
                   ^^^^

Since channels 5 and 6 are unconnected, should this macro use a value table
similar to a9_usb_250m_parents_val_table?

Passing NULL might cause the framework to write the unconnected index 5 for
the fdiv2p5 clock, potentially silencing the clock output instead of
selecting index 7.

[ ... ]

> 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);
                                         ^^

Could this cause a register bit overlap? 

The divider is configured to be 12 bits wide starting at bit 0, which means
it occupies bits 0 through 11. However, the clock gate is also defined at
bit 11 in the same register.

Writing to the divider might inadvertently toggle the clock gate, and
toggling the gate could corrupt the most significant bit of the divider.

[ ... ]

> static struct clk_regmap a9_hdmitx1_pixel = {
> 	.data = &(struct clk_regmap_gate_data){
> 		.offset = VID_CLK_CTRL2,
> 		.bit_idx = 12,
> 	},
> 	.hw.init = &(struct clk_init_data) {
> 		.name = "hdmitx1_pixel",
> 		.ops = &clk_regmap_gate_ops,
> 		.parent_hws = (const struct clk_hw *[]) {
> 			&a9_hdmitx_pixel_sel.hw
                        ^^^^^^^^^^^^^^^^^^^^^^^

Is this the intended parent selector? 

This references a9_hdmitx_pixel_sel instead of a9_hdmitx1_pixel_sel. This
hierarchy mismatch might cause parent and rate adjustments for hdmitx1_pixel
to mutate the wrong clock selector, leaving its own hardware unconfigured.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-b4-a9_clk-v1-0-41cb4071b7c9@amlogic.com?part=9