Re: [PATCH v9 7/9] arm64: dts: ti: k3-am62a: Split r5f memory region

[Nishanth Menon <nm@ti.com>]

On 09:40-20260918, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] remoteproc: ti_k3: k3_rproc_da_to_va() rejects segments spanning split memory regions
> - [High] arm64: dts: ti: missing bootph-pre-ram breaks SPL DM firmware loading
> - [Medium] arm64: dts: ti: hardcoding volatile firmware layout into Device Tree
> --
> 
> commit 33643294863f45f1ac37617eafcfc525d230effc
> Author: Markus Schneider-Pargmann (TI) <msp@baylibre.com>
> 
> arm64: dts: ti: k3-am62a: 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 06d4e815b1670..94c1d60d8d7a1 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
> > @@ -65,8 +65,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>;
> 
> [Severity: High]
> Does this multi-region split cause a regression when loading a monolithic
> firmware ELF whose segments span across the new chunk boundaries?
> 
> Looking at k3_rproc_da_to_va() in drivers/remoteproc/ti_k3_common.c, it
> evaluates segments against each reserved memory region individually and
> lacks logic to aggregate sequential chunks:
> 
> 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;
>     }
>     ...
> }
> 
> Will this bounds check fail and abort the boot sequence for any system using
> standard firmware linked under the assumption of a single contiguous memory
> block?


Do we need to protect with something on the lines of
https://lore.kernel.org/all/20260921145325.300025-3-richard.genoud@bootlin.com/

Could you please add in diffstat any comments if the sashiko reviews
are'nt accurate from previous feedback?

> 
> > +	memory-region-names = "dma", "ipc", "lpm-stub",
> > +			      "lpm-metadata", "lpm-context",
> > +			      "dm-firmware";
> 
> [ ... ]
> 
> > diff --git a/arch/arm64/boot/dts/ti/k3-am62d2-evm.dts b/arch/arm64/boot/dts/ti/k3-am62d2-evm.dts
> > index a8c794d073431..7cba3ff99e6a3 100644
> > --- a/arch/arm64/boot/dts/ti/k3-am62d2-evm.dts
> > +++ b/arch/arm64/boot/dts/ti/k3-am62d2-evm.dts
> > @@ -65,13 +65,37 @@ wkup_r5fss0_core0_dma_memory_region: memory@9c800000 {
> [ ... ]
> > +		wkup_r5fss0_core0_lpm_fs_stub_region: memory@9ca00000 {
> > +			compatible = "shared-dma-pool";
> > +			reg = <0x00 0x9ca00000 0x00 0x8000>;
> > +			no-map;
> > +		};
> 
> [Severity: Medium]
> Does this create a regression by hardcoding a software-defined volatile
> firmware layout directly into the Device Tree?
> 
> If a subsequent firmware build increases the size of lpm_fs_stub beyond the
> 32KB hardcoded here, will this crash or require a lockstep DT update to
> function properly?
> 
> > +
> > +		wkup_r5fss0_core0_lpm_metadata_region: memory@9ca08000 {
> > +			compatible = "shared-dma-pool";
> > +			reg = <0x00 0x9ca08000 0x00 0x1000>;
> >  			no-map;
> >  			bootph-pre-ram;
> >  		};
> >  
> > +		wkup_r5fss0_core0_lpm_rest_region: memory@9ca09000 {
> > +			compatible = "shared-dma-pool";
> > +			reg = <0x00 0x9ca09000 0x00 0x97000>;
> > +			no-map;
> > +		};
> > +
> > +		wkup_r5fss0_core0_dm_region: memory@9caa0000 {
> > +			compatible = "shared-dma-pool";
> > +			reg = <0x00 0x9caa0000 0x00 0x1c60000>;
> > +			no-map;
> > +		};
> 
> [Severity: High]
> Does the omission of the bootph-pre-ram tag on this new DM region create a
> regression for SPL DM firmware loading?
> 
> Since U-Boot fdtgrep processes the SPL DTB by keeping only nodes tagged
> with bootph-pre-ram, these split regions will be stripped.
> 
> Will this result in the no-map protections being lost, causing U-Boot's
> remoteproc driver to error out parsing dangling phandles when attempting
> to load the Device Manager (DM) firmware?

Is'nt this valid? why would we let R5 SPL or U-boot SPL think it has
memory access? Adding a comment is probably worth in the code.

> 
> > +
> >  		secure_ddr: optee@9e800000 {
> >  			reg = <0x00 0x9e800000 0x00 0x01800000>; /* for OP-TEE */
> >  			no-map;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260918-topic-am62a-ioddr-dt-v6-19-v9-0-40972ef1f663@baylibre.com?part=7

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D
https://ti.com/opensource