Re: [PATCH v7 09/11] arm64: dts: ti: k3-am62a-ti-ipc-firmware: Split r5f memory region

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing `bootph-pre-ram` properties on newly split firmware memory regions will leave dangling phandles in U-Boot SPL and risks memory corruption.
- [High] Arbitrarily subdividing the firmware carveout breaks `rproc_da_to_va` translation for existing firmware ELF segments that span the new boundaries.
--

commit 235186e608d68e805ecd28c0d36c56c91fb3c688
Author: Markus Schneider-Pargmann (TI) <msp@baylibre.com>

arm64: dts: ti: k3-am62a-ti-ipc-firmware: Split r5f memory region
    
Split the firmware memory region in more specific parts so it is better
described where to find which information. Specifically the LPM metadata
region is important as bootloader software like U-Boot has to know where
that data is to be able to read that data.

> diff --git a/arch/arm64/boot/dts/ti/k3-am62a-ti-ipc-firmware.dtsi b/arch/arm64/boot/dts/ti/k3-am62a-ti-ipc-firmware.dtsi
> index 682b1c9f3071d..fe10d3e75ceee 100644
> --- a/arch/arm64/boot/dts/ti/k3-am62a-ti-ipc-firmware.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am62a-ti-ipc-firmware.dtsi

[ ... ]

> -	wkup_r5fss0_core0_memory_region: memory@9c900000 {
> +	wkup_r5fss0_core0_ipc_region: memory@9c900000 {
>  		compatible = "shared-dma-pool";
> -		reg = <0x00 0x9c900000 0x00 0x01d00000>;
> +		reg = <0x00 0x9c900000 0x00 0x100000>;
> +		no-map;
> +	};

[Severity: High]
Does subdividing this contiguous region break backwards compatibility for
existing firmware binaries?

Looking at drivers/remoteproc/ti_k3_common.c:k3_rproc_da_to_va():

    if (da >= dev_addr && ((da + len) <= (dev_addr + size))) {
        offset = da - dev_addr;
        va = kproc->rmem[i].cpu_addr + offset;
        return (__force void *)va;
    }

The translation logic requires a firmware ELF segment to fit entirely within a
single parsed device tree memory region.

If an older monolithic firmware binary has a segment that spans across these
new boundaries (for example, from lpm_metadata into lpm_rest), won't this
check fail and return NULL?

[ ... ]

> @@ -78,8 +102,14 @@ &wkup_r5fss0 {
>  &wkup_r5fss0_core0 {
>  	mboxes = <&mailbox0_cluster0>, <&mbox_r5_0>;
>  	memory-region = <&wkup_r5fss0_core0_dma_memory_region>,
> -			<&wkup_r5fss0_core0_memory_region>;
> -	memory-region-names = "dma", "firmware";
> +			<&wkup_r5fss0_core0_ipc_region>,
> +			<&wkup_r5fss0_core0_lpm_fs_stub_region>,
> +			<&wkup_r5fss0_core0_lpm_metadata_region>,
> +			<&wkup_r5fss0_core0_lpm_rest_region>,
> +			<&wkup_r5fss0_core0_dm_region>;
> +	memory-region-names = "dma", "ipc", "lpm-stub",
> +			      "lpm-metadata", "lpm-context",
> +			      "dm-firmware";
>  	bootph-pre-ram;
>  	status = "okay";
>  };

[Severity: High]
Will this cause issues during the U-Boot SPL build?

Since this node retains the bootph-pre-ram property, but newly added regions
like wkup_r5fss0_core0_dm_region and wkup_r5fss0_core0_ipc_region do not have
it, won't fdtgrep strip out the new regions?

If they are stripped, the memory-region array here will contain dangling
phandles, which could cause U-Boot's remoteproc driver to fail probing.

Additionally, because the stripped regions lose their no-map status in the
SPL device tree, is there a risk the SPL allocator might overwrite and corrupt
the Device Manager firmware or IPC data?

I've also checked later commits in this series (e.g., commit e36960554d0b),
and it appears the bootph-pre-ram property remains absent for these other
memory regions.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-topic-am62a-ioddr-dt-v6-19-v7-0-e9db8b16821a@baylibre.com?part=9