Re: [PATCH v3] arm64: dts: freescale: imx8mp-verdin: Add Cortex-M7 remoteproc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] arm64: dts: imx8mp-verdin: unregistered IMX8MP_CLK_M7_DIV clock in imx8mp-cm7
--

commit 37e821a019361403bc773ac48a7bf68be6a133ac
Author: Ernest Van Hoecke <ernest.vanhoecke@toradex.com>

arm64: dts: freescale: imx8mp-verdin: Add Cortex-M7 remoteproc

This commit adds the Cortex-M7 remoteproc node and its associated
reserved-memory regions to the Verdin iMX8M Plus device tree.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-verdin.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-verdin.dtsi
> index 9fee2cf9ef546..45a4713c260f4 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp-verdin.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-verdin.dtsi
> @@ -77,6 +77,18 @@ key-wakeup {
>  		};
>  	};
>  
> +	imx8mp-cm7 {
> +		compatible = "fsl,imx8mp-cm7";
> +		clocks = <&clk IMX8MP_CLK_M7_DIV>;

[Severity: High]
Should this clock specifier be IMX8MP_CLK_M7_CORE instead of
IMX8MP_CLK_M7_DIV?

In drivers/clk/imx/clk-imx8mp.c, commit 8c83a8ff4dd9 ("clk: imx8mp: use
imx8m_clk_hw_composite_core to simplify code") replaced
hws[IMX8MP_CLK_M7_DIV] in imx8mp_clocks_probe() with
hws[IMX8MP_CLK_M7_CORE] without keeping an alias at IMX8MP_CLK_M7_DIV:

	hws[IMX8MP_CLK_A53_DIV] = imx8m_clk_hw_composite_core("arm_a53_div", imx8mp_a53_sels, ccm_base + 0x8000);
	hws[IMX8MP_CLK_A53_SRC] = hws[IMX8MP_CLK_A53_DIV];
	hws[IMX8MP_CLK_A53_CG] = hws[IMX8MP_CLK_A53_DIV];
	hws[IMX8MP_CLK_M7_CORE] = imx8m_clk_hw_composite_core("m7_core", imx8mp_m7_sels, ccm_base + 0x8080);

Because clk_hw_data is zero-allocated, hws[IMX8MP_CLK_M7_DIV] remains
NULL. When imx_rproc_probe() in drivers/remoteproc/imx_rproc.c requests
the clock for "fsl,imx8mp-cm7" (which sets IMX_RPROC_NEED_CLKS in
imx_rproc_cfg_imx8mn):

	if (dcfg->flags & IMX_RPROC_NEED_CLKS) {
		priv->clk = devm_clk_get_enabled(dev, NULL);
		if (IS_ERR(priv->clk))
			return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to enable clock\n");
	}

of_clk_hw_onecell_get() returns NULL for hw_data->hws[IMX8MP_CLK_M7_DIV],
causing devm_clk_get_enabled() to return NULL without an error. As a
result, m7_core is never prepared or enabled by imx_rproc_probe(), and
clk_disable_unused() can gate it off at late boot when the Cortex-M7 was
not pre-booted by the bootloader, leaving the core unclocked when
imx_rproc accesses TCM or starts the remote processor.

Other i.MX8MP Cortex-M7 device tree descriptions such as
arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-cm7.dtso
use IMX8MP_CLK_M7_CORE:

	imx8mp-cm7 {
		compatible = "fsl,imx8mp-cm7-mmio";
		clocks = <&clk IMX8MP_CLK_M7_CORE>;

> +		mbox-names = "tx", "rx", "rxdb";
> +		mboxes = <&mu 0 1
> +			  &mu 1 1
> +			  &mu 3 1>;
> +		memory-region = <&vdev0buffer>, <&vdev0vring0>, <&vdev0vring1>,
> +				<&rsc_table>;
> +		syscon = <&src>;
> +	};
> +
>  	sound_hdmi: sound-hdmi {
>  		compatible = "fsl,imx-audio-hdmi";
>  		model = "audio-hdmi";

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-verdin-imx8mp-m7-v3-1-af284e28531d@toradex.com?part=1