Re: [PATCH 3/4] arm64: dts: imx8ulp: add HiFi4 DSP node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] arm64: dts: imx8ulp: dsp node bypasses sim_lpav clock gates
--

commit ab34f7c1f3585869d7aa05e36a532cefcf3ccd37
Author: Shengjiu Wang <shengjiu.wang@nxp.com>

arm64: dts: imx8ulp: add HiFi4 DSP node

This patch adds a disabled HiFi4 DSP node to arch/arm64/boot/dts/freescale/imx8ulp.dtsi with its register region, clocks, mailbox channels, power domain, and resets.

> diff --git a/arch/arm64/boot/dts/freescale/imx8ulp.dtsi b/arch/arm64/boot/dts/freescale/imx8ulp.dtsi
> index 777b89ed68490..4041f8ff5654d 100644
> --- a/arch/arm64/boot/dts/freescale/imx8ulp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8ulp.dtsi
[ ... ]
> @@ -200,6 +201,28 @@ soc: soc@0 {
>  		ranges = <0x0 0x0 0x0 0x40000000>,
>  			 <0x60000000 0x0 0x60000000 0x1000000>;
>  
> +		dsp: dsp@21170000 {
> +			compatible = "fsl,imx8ulp-hifi4";
> +			reg = <0x21170000 0x20000>;
> +			clocks = <&cgc2 IMX8ULP_CLK_LPAV_BUS_DIV>,
> +				 <&cgc2 IMX8ULP_CLK_HIFI_DIVPLAT>,
> +				 <&cgc2 IMX8ULP_CLK_HIFI_DIVCORE>,
> +				 <&cgc1 IMX8ULP_CLK_DUMMY>,
> +				 <&pcc5 IMX8ULP_CLK_MU3_B>;
> +			clock-names = "ipg", "ocram", "core", "debug", "mu";

[Severity: High]
Should the ipg, ocram, and core clocks in dsp@21170000 reference the leaf
clock gates in sim_lpav rather than the parent dividers in cgc2 directly?

In arch/arm64/boot/dts/freescale/imx8ulp.dtsi, sim_lpav consumes these three
cgc2 dividers as its input clocks:

	sim_lpav: clock-controller@2da50000 {
		compatible = "fsl,imx8ulp-sim-lpav";
		reg = <0x2da50000 0x10000>;
		clocks = <&cgc2 IMX8ULP_CLK_LPAV_BUS_DIV>,
			 <&cgc2 IMX8ULP_CLK_HIFI_DIVCORE>,
			 <&cgc2 IMX8ULP_CLK_HIFI_DIVPLAT>;
		clock-names = "bus", "core", "plat";

In drivers/clk/imx/clk-imx8ulp-sim-lpav.c, clk_imx8ulp_sim_lpav_probe()
registers the downstream HiFi4 clock gates in SIM_LPAV SYSCTRL0 (bits 17, 18,
and 19) without CLK_IGNORE_UNUSED or CLK_IS_CRITICAL:

	static struct clk_imx8ulp_sim_lpav_gate gates[] = {
		IMX8ULP_HIFI_CLK_GATE("hifi_core", CORE, "core", 17),
		IMX8ULP_HIFI_CLK_GATE("hifi_pbclk", PBCLK, "bus", 18),
		IMX8ULP_HIFI_CLK_GATE("hifi_plat", PLAT, "plat", 19)
	};

Because dsp@21170000 points directly to &cgc2 instead of
<&sim_lpav IMX8ULP_CLK_SIM_LPAV_HIFI_PBCLK>,
<&sim_lpav IMX8ULP_CLK_SIM_LPAV_HIFI_PLAT>, and
<&sim_lpav IMX8ULP_CLK_SIM_LPAV_HIFI_CORE>, the sim_lpav gates have no
consumers and will be gated off by clk_disable_unused().

When the DSP is later enabled and booted via remoteproc,
imx_dsp_runtime_resume() -> clk_bulk_prepare_enable() will only enable the
upstream cgc2 dividers while leaving the SIM_LPAV SYSCTRL0 clock gates
disabled (and imx8ulp_dsp_reset() in drivers/remoteproc/imx_dsp_rproc.c only
manipulates the reset lines). Won't this leave the HiFi4 core, PBCLK, and
platform clocks gated off when imx_dsp_rproc_load() accesses DSP TCM/OCRAM at
0x21170000?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029073257.84594-1-shengjiu.wang@oss.nxp.com?part=3