Re: [PATCH 4/4] arm64: dts: imx8ulp-evk: enable HiFi4 DSP device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] arm64: dts: imx8ulp: dsp node bypasses sim_lpav clock gates
- [Low] dt-bindings: dsp: fsl,dsp: missing required resets for imx8ulp-hifi4
- [Low] arm64: dts: imx8ulp-evk: invalid underscore in dsp_reserved node name
--

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

arm64: dts: imx8ulp-evk: enable HiFi4 DSP device

This commit enables the HiFi4 DSP remoteproc node and its associated MU3
mailbox on imx8ulp-evk, configuring its assigned clocks and reserved memory
regions.

> diff --git a/arch/arm64/boot/dts/freescale/imx8ulp-evk.dts b/arch/arm64/boot/dts/freescale/imx8ulp-evk.dts
> index 4b925832bb662..358554a5e727e 100644
> --- a/arch/arm64/boot/dts/freescale/imx8ulp-evk.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8ulp-evk.dts
> @@ -60,6 +60,27 @@ rsc_table: rsc-table@20000000 {
>  			no-map;
>  		};
>  
> +		dsp_reserved: dsp_reserved@8e000000 {
> +			reg = <0 0x8e000000 0 0x1000000>;
> +			no-map;
> +		};

[Severity: Low]
Should the node name dsp_reserved@8e000000 avoid using an underscore?

Per Documentation/devicetree/bindings/dts-coding-style.rst and the
Devicetree specification, underscores are valid in labels like dsp_reserved,
but node names should use hyphens or generic names (such as
dsp-reserved@8e000000 or memory@8e000000) to avoid dtc W=2
node_name_chars_strict and dtbs_check warnings.

[ ... ]

> @@ -144,6 +165,15 @@ &cm33 {
>  	status = "okay";
>  };
>  
> +&dsp {
> +	assigned-clocks = <&cgc2 IMX8ULP_CLK_HIFI_SEL>, <&cgc2 IMX8ULP_CLK_HIFI_DIVCORE>;
> +	assigned-clock-parents = <&cgc2 IMX8ULP_CLK_PLL4_PFD0>;
> +	assigned-clock-rates = <0>, <475200000>;
> +	memory-region = <&dsp_vdev0buffer>, <&dsp_vdev0vring0>,
> +			<&dsp_vdev0vring1>, <&dsp_reserved>;
> +	status = "okay";
> +};

[Severity: High]
When enabling &dsp here, will the HiFi4 clock gates in sim_lpav be disabled
by clk_disable_unused() and left gated when the DSP boots?

In arch/arm64/boot/dts/freescale/imx8ulp.dtsi, the dsp node takes its ipg,
ocram, and core clocks directly from &cgc2 instead of &sim_lpav:

	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";

Meanwhile, sim_lpav in arch/arm64/boot/dts/freescale/imx8ulp.dtsi consumes
those three cgc2 clocks, and clk_imx8ulp_sim_lpav_probe() in
drivers/clk/imx/clk-imx8ulp-sim-lpav.c registers the three HiFi4 clock gates
in SYSCTRL0 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 nothing references IMX8ULP_CLK_SIM_LPAV_HIFI_PBCLK,
IMX8ULP_CLK_SIM_LPAV_HIFI_PLAT, or IMX8ULP_CLK_SIM_LPAV_HIFI_CORE,
clk_disable_unused() clears bits 17, 18, and 19 in SYSCTRL0 at late_initcall.
When rproc_boot() -> imx_dsp_rproc_prepare() -> imx_dsp_runtime_resume() ->
clk_bulk_prepare_enable() later runs, only the upstream cgc2 dividers are
enabled while the sim_lpav gates remain closed during imx_dsp_rproc_load()
and imx_dsp_rproc_start().

Should the dsp node reference the three &sim_lpav clock gates for its ipg,
ocram, and core clocks instead of &cgc2 directly?

[Severity: Low]
Also related to enabling fsl,imx8ulp-hifi4, should
Documentation/devicetree/bindings/dsp/fsl,dsp.yaml be updated to require
resets and reset-names for fsl,imx8ulp-hifi4?

In drivers/remoteproc/imx_dsp_rproc.c, dsp_rproc_cfg_imx8ulp uses
imx_dsp_rproc_ops_reset_ctrl:

	static const struct imx_rproc_dcfg dsp_rproc_cfg_imx8ulp = {
		.att		= imx_dsp_rproc_att_imx8ulp,
		.att_size	= ARRAY_SIZE(imx_dsp_rproc_att_imx8ulp),
		.ops		= &imx_dsp_rproc_ops_reset_ctrl,
	};

During imx_dsp_rproc_probe(), imx_dsp_rproc_reset_ctrl_detect_mode() requires
the runstall reset control (and imx8ulp_dsp_reset() uses runstall, softreset,
and dbgreset), but the allOf conditional block in
Documentation/devicetree/bindings/dsp/fsl,dsp.yaml requiring resets and
reset-names currently only lists fsl,imx8mp-dsp and fsl,imx8mp-hifi4:

	  - if:
	      properties:
	        compatible:
	          contains:
	            enum:
	              - fsl,imx8mp-dsp
	              - fsl,imx8mp-hifi4
	    then:
	      required:
	        - resets
	        - reset-names

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