Re: [PATCH v5 4/5] arm64: dts: ti: k3-am62p-ti-ipc-firmware: Split r5f memory region

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Fragmentation of the reserved memory region breaks backward compatibility for firmware loading.
- [High] Board-specific firmware memory sizes are overwritten and duplicated, causing overlapping reservations and memory starvation.
- [Medium] Encoding firmware-specific software layout policy into the hardware description device tree.
--

commit c6bb6a1bb6896ca4a20a285fa611156e2c906d69
Author: Markus Schneider-Pargmann (TI) <msp@baylibre.com>
arm64: dts: ti: k3-am62p-ti-ipc-firmware: Split r5f memory region

This patch splits the firmware memory region in the AM62p IPC device tree
into multiple smaller parts. The stated goal is to describe where to find
specific information, such as LPM metadata, for bootloaders like U-Boot.

> diff --git a/arch/arm64/boot/dts/ti/k3-am62p-ti-ipc-firmware.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-ti-ipc-firmware.dtsi
> index 12902231e58a7..437b1df02142c 100644
> --- a/arch/arm64/boot/dts/ti/k3-am62p-ti-ipc-firmware.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am62p-ti-ipc-firmware.dtsi
> @@ -24,9 +24,33 @@ wkup_r5fss0_core0_dma_memory_region: memory@9c800000 {
>  		no-map;
>  	};
>  
> -	wkup_r5fss0_core0_memory_region: memory@9c900000 {
> +	wkup_r5fss0_core0_ipc_region: memory@9c900000 {
>  		compatible = "shared-dma-pool";
> -		reg = <0x00 0x9c900000 0x00 0xf00000>;
> +		reg = <0x00 0x9c900000 0x00 0x100000>;
> +		no-map;
> +	};

[Severity: High]
Does splitting this contiguous 15MB firmware region into smaller adjacent
sub-regions break backward compatibility for firmware loading?

Looking at k3_rproc_da_to_va() in drivers/remoteproc/ti_k3_common.c, there is
an exact check that enforces a loaded ELF segment must fit within a single
DT memory region:

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

If an existing firmware ELF segment (like .data or .bss) spans across these
new boundaries, it looks like it will fail this bounds check, returning NULL
and preventing the remote processor from booting.

[Severity: High]
Could this redefinition truncate or duplicate board-specific firmware memory
sizes?

This common dtsi file is typically included at the end of board-specific
device trees. For example, in arch/arm64/boot/dts/ti/k3-am62p-verdin.dtsi,
the board defines a 30MB region at memory@9c900000. This patch redefines
memory@9c900000 with a size of 1MB, which seems to silently truncate the
board's memory allocation.

Additionally, in arch/arm64/boot/dts/ti/k3-am62p5-var-som.dtsi, the board
labels the node r5f-memory@9c900000:

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

Because the node names differ (memory vs r5f-memory), both would be
instantiated at the same unit address, causing overlapping memory
reservations.

> +
> +	wkup_r5fss0_core0_lpm_fs_stub_region: memory@9ca00000 {

[ ... ]

> @@ -56,8 +80,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";

[Severity: High]
Will hardcoding the memory-region array here override board-level DT
configurations?

By explicitly redefining the memory-region array, it discards board-level
allocations. This appears to restrict the remoteproc driver to only 15MB of
mapped memory, which might starve the firmware on boards that require a
larger allocation.

[Severity: Medium]
Does hardcoding these specific memory sections encode firmware-specific
software layout policy into the hardware description?

The device tree is intended to describe hardware architecture and memory
reservations, not software layout. By defining granular sections like
lpm-stub and lpm-metadata here, the DT becomes tightly coupled to a
specific version of the firmware linker script.

If future firmware changes the internal memory layout, this would require a
device tree update. Should the bootloader locate metadata using an
in-memory structure or a fixed offset instead of fracturing the device
tree?

>  	status = "okay";
>  };
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-topic-am62a-ioddr-dt-v6-19-v5-0-3856a023aff2@baylibre.com?part=4