Re: [PATCH 5/5] arm64: dts: imx8mp: Drop VIDEO_PLL1 assigned rate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64: dts: imx8mp: assigned-clock-rates mismatch in helios-lvds overlay
- [Medium] clk: imx8mp: bus composite helper used for IP slice in imx8mp_clocks_probe()
--

commit 540b176866f9d4811ba6779f199a55adb4dc8a78
Author: Miquel Raynal <miquel.raynal@bootlin.com>

arm64: dts: imx8mp: Drop VIDEO_PLL1 assigned rate

This commit removes the IMX8MP_VIDEO_PLL1 entry and its 1039.5 MHz rate from
assigned-clocks and assigned-clock-rates in the media_blk_ctrl node of
arch/arm64/boot/dts/freescale/imx8mp.dtsi, relying on common clock framework
rate negotiation instead.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index 9b2b3a9bf9e80..2a02e80e60e7b 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -1988,16 +1988,14 @@ media_blk_ctrl: blk-ctrl@32ec0000 {
>  						  <&clk IMX8MP_CLK_MEDIA_APB>,
>  						  <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>,
>  						  <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>,
> -						  <&clk IMX8MP_CLK_MEDIA_ISP>,
> -						  <&clk IMX8MP_VIDEO_PLL1>;
> +						  <&clk IMX8MP_CLK_MEDIA_ISP>;
>  				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
>  							 <&clk IMX8MP_SYS_PLL1_800M>,
>  							 <&clk IMX8MP_VIDEO_PLL1_OUT>,
>  							 <&clk IMX8MP_VIDEO_PLL1_OUT>,
>  							 <&clk IMX8MP_SYS_PLL2_500M>;
>  				assigned-clock-rates = <500000000>, <200000000>,
> -						       <0>, <0>, <500000000>,
> -						       <1039500000>;
> +						       <0>, <0>, <500000000>;

[Severity: Medium]
Does dropping <&clk IMX8MP_VIDEO_PLL1> from assigned-clocks on media_blk_ctrl
break arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios-lvds.dtso?

In arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios-lvds.dtso, the
overlay overrides assigned-clock-rates on &media_blk_ctrl with six entries
without overriding assigned-clocks:

&media_blk_ctrl {
	/*
	 * currently it is not possible to let display clocks configure
	 * automatically, so we need to set them manually
	 */
	assigned-clock-rates = <500000000>, <200000000>, <0>,
		/* IMX8MP_CLK_MEDIA_DISP2_PIX = pixelclk of lvds panel */
		<33260000>, <0>,
		/* IMX8MP_VIDEO_PLL1 = IMX8MP_CLK_MEDIA_LDB * 2 */
		<465640000>;
};

With <&clk IMX8MP_VIDEO_PLL1> removed from media_blk_ctrl in
arch/arm64/boot/dts/freescale/imx8mp.dtsi, the merged &media_blk_ctrl node in
imx8mp-aristainetos3-helios-lvds.dtb has five entries in assigned-clocks and
six entries in assigned-clock-rates, which fails device tree schema
validation.

At boot, when of_clk_set_defaults() -> __set_clk_rates() in
drivers/clk/clk-conf.c iterates over assigned-clock-rates on &media_blk_ctrl,
the sixth entry (465640000) is out of bounds for assigned-clocks:

drivers/clk/clk-conf.c:__set_clk_rates() {
	...
		if (rate) {
			rc = of_parse_phandle_with_args(node, "assigned-clocks",
					"#clock-cells",	index, &clkspec);
			if (rc < 0) {
				/* skip empty (null) phandles */
				if (rc == -ENOENT)
					continue;
				else
					return rc;
			}
	...
}

Because of_parse_phandle_with_args() returns -ENOENT for the missing sixth
clock, __set_clk_rates() skips setting IMX8MP_VIDEO_PLL1 to 465640000 Hz,
leaving the display clock tree misconfigured on that board.

>  				#power-domain-cells = <1>;
>  
>  				lvds_bridge: bridge@5c {

[Severity: Medium]
In the earlier commits in this series enabling rate negotiation on
IMX8MP_CLK_MEDIA_LDB in drivers/clk/imx/clk-imx8mp.c, should
imx8mp_clocks_probe() use imx8m_clk_hw_composite_flags() instead of
imx8m_clk_hw_composite_bus_flags()?

In drivers/clk/imx/clk-imx8mp.c, imx8mp_clocks_probe() registers
IMX8MP_CLK_MEDIA_LDB (at ccm_base + 0xbf00, in the peripheral clock root
slice range) with imx8m_clk_hw_composite_bus_flags():

drivers/clk/imx/clk-imx8mp.c:imx8mp_clocks_probe() {
	...
	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT | CLK_V2_RATE_NEGOTIATION);
	...
}

Passing imx8m_clk_hw_composite_bus_flags() sets IMX_COMPOSITE_BUS in
drivers/clk/imx/clk-composite-8m.c:__imx8m_clk_hw_composite():

drivers/clk/imx/clk-composite-8m.c:__imx8m_clk_hw_composite() {
	...
	} else if (composite_flags & IMX_COMPOSITE_BUS) {
		div->shift = PCG_PREDIV_SHIFT;
		div->width = PCG_PREDIV_WIDTH;
		divider_ops = &imx8m_clk_composite_divider_ops;
		mux_ops = &imx8m_clk_composite_mux_ops;
	} else {
		div->shift = PCG_PREDIV_SHIFT;
		div->width = PCG_PREDIV_WIDTH;
		divider_ops = &imx8m_clk_composite_divider_ops;
		mux_ops = &clk_mux_ops;
		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
			flags |= CLK_SET_PARENT_GATE;
	}
	...
}

Because IMX_COMPOSITE_BUS selects the bus slice branch, IMX8MP_CLK_MEDIA_LDB
uses imx8m_clk_composite_mux_ops instead of clk_mux_ops and drops
CLK_SET_PARENT_GATE, allowing clk_set_parent() while media_ldb is enabled.
Using imx8m_clk_hw_composite_flags() from drivers/clk/imx/clk.h would keep
the peripheral slice configuration while passing CLK_SET_RATE_PARENT |
CLK_V2_RATE_NEGOTIATION.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-ge-clk-lcm-v1-0-246cc35b5ffb@bootlin.com?part=5