Re: [PATCH v3 5/7] arm64: dts: allwinner: sun50i-h6: Add missing SRAM region for video engine

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The newly added `<&de2_sram 1>` phandle is silently ignored by the OS because the SRAM driver hardcodes parsing to only the first array element.
- [Medium] Assigning `de2_sram` to the Video Engine creates an unresolvable resource conflict with the Display Engine, guaranteeing a probe failure if the SRAM driver were to parse it.
--

--- Patch [5]: [PATCH v3 5/7] arm64: dts: allwinner: sun50i-h6: Add missing SRAM region for video engine ---
commit dbe16923a66cbda638350c813c16a8b1c65e3b6b
Author: Chen-Yu Tsai <wens@kernel.org>

arm64: dts: allwinner: sun50i-h6: Add missing SRAM region for video engine

The SRAM C region contains a partial alias to the VE SRAM already
referenced by the video engine. To avoid access through this alias
window, the region should also be claimed by the video engine.

Add a reference to the SRAM C region to the video engine node.

> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> index 72ce1a75647bf..88c6e3e105c07 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi

[ ... ]

> @@ -205,7 +205,7 @@ video-codec@1c0e000 {
>  			clock-names = "ahb", "mod", "ram";
>  			resets = <&ccu RST_BUS_VE>;
>  			interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> -			allwinner,sram = <&ve_sram 1>;
> +			allwinner,sram = <&ve_sram 1>, <&de2_sram 1>;

[Severity: Medium]
Will this second phandle actually be parsed by the OS?

Looking at the driver code in drivers/soc/sunxi/sunxi_sram.c, it seems
the parser hardcodes the index to 0:

drivers/soc/sunxi/sunxi_sram.c:sunxi_sram_of_parse() {
	...
	ret = of_parse_phandle_with_fixed_args(node, "allwinner,sram", 1, 0,
					       &args);
	...
}

Does the driver need to be updated to parse multiple SRAM elements so
this alias window is actually closed?


[Severity: Medium]
If the driver is updated to parse this second element, will it cause a probe
failure due to a resource conflict?

The de2_sram region is already claimed by the Display Engine (bus@1000000).
Looking at sunxi_sram_claim(), the API enforces strict exclusive ownership:

drivers/soc/sunxi/sunxi_sram.c:sunxi_sram_claim() {
	...
	if (sram_desc->claimed) {
		spin_unlock(&sram_lock);
		return -EBUSY;
	}
	...
}

If both the Display Engine and Video Engine try to claim de2_sram, won't the
second device to probe be rejected with -EBUSY?

>  			iommus = <&iommu 3>;
>  		};
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712030011.3778169-1-wens@kernel.org?part=5