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

["Markus Schneider-Pargmann" <msp@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 5951 bytes --]

Hi Nishanth,

On Thu Sep 24, 2026 at 4:55 PM CEST, Nishanth Menon wrote:
> 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/

No, we don't need to protect this here. The devicetree nodes changed
here are only ever loaded with firmware in the bootloader. A monolithic
firmware ELF is never loaded in Linux because the driver code in
k3_r5_rproc_configure_mode() detects that it is already running and puts
it in RPROC_DETACHED for these cores.

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

Sure, will do that in the future. Does sashiko pick these up then?

>
>> 
>> > +	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.

These bootph-pre-ram properties did not exist before this patch for
nearly all memory regions. R5 SPL or SPL u-boot does not care about
these memory regions. It is all configured through Kconfig options or
binman. Only the upcoming IO+DDR resume code cares about just a specific
region. I personally would prefer to keep the DT as small as possible
for SPL.

Best
Markus

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 289 bytes --]