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) >> >> 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