[REGRESSION] Re: [PATCH v11 00/11] Add support for dynamic MMU configuration

[Emanuele Ghidoli]

On 29/10/2025 09:58, Anshul Dalal wrote:
> On Tue Oct 28, 2025 at 10:26 PM IST, Emanuele Ghidoli wrote:
>> On 28/10/2025 05:38, Anshul Dalal wrote:
>>> Hi Francesco,
>>>
>>> On Mon Oct 27, 2025 at 10:22 PM IST, Francesco Dolcini wrote:
>>>> Hello Anshul,
>>>>
>>>> On Fri, Oct 17, 2025 at 06:45:22PM +0530, Anshul Dalal wrote:
>>>>> Hi all,
>>>>>
>>>>> In U-Boot, TI only provides a single memory map for all k3 platforms, this
>>>>> does not scale for devices where atf and optee lie outside the range 0x80000000
>>>>> - 0x80080000 and 0x9e780000 - 0xa0000000 respectively.
>>>>>
>>>>> There are also issues for devices with < 2GiB of memory (eg am62SiP with 512MiB
>>>>> of RAM) as the maximum size for the first DRAM bank is hardcoded to 2GiB in the
>>>>> current memory map. Furthermore the second DRAM bank is mapped even for devices
>>>>> that only have a single bank.
>>>>>
>>>>> Therefore this patch set adds the required functionality to create the MMU table
>>>>> at runtime based on the device-tree.
>>>>>
>>>>> The patch set has been build tested on all effected platforms but boot-tested
>>>>> only on TI's K3 EVMs, the beagleplay and phytec's phycore-am6* platforms.
>>>>>
>>>>> The following effected boards have not been boot tested:
>>>>>  - verdin-am62
>>>>
>>>> it seems that this series introduce a regression on verdin-am62, I have
>>>> not done a bi-sect yet, but we run daily build of U-Boot master and the
>>>> regressions seems to have started when this patch series was
>>>> merged.
>>>>
>>>> On verdin-am62 we detect the RAM size at run-time, see
>>>> board/toradex/verdin-am62/verdin-am62.c:dram_init(), and now we always
>>>> get 2GiB even for modules with only 512MB or 1024MB of memory.
>>>>
>>>
>>> This patch series modified the behavior of enable_caches to configure the
>>> memory map of the device as per the device-tree instead of using a
>>> static map for all of K3.
>>>
>>> The issue with verdin-am62 seems to be that while you do properly
>>> configure gd->ram_size in your dram_init, the '/memory' node of the
>>> device-tree remains unchanged with the outdated 2GiB size.
>>>
>>> You could try updating the fdt's memory size to the correct value in
>>> dram_init and see if that fixes the problem.
>>>
>>> Regards,
>>> Anshul
>> Hello Anshul,
>>
>> I was bisecting the series, and I can confirm that the commit "mach-k3: map
>> all banks using mem_map_from_dram_banks" introduces the regression.
>>
>> Given that initcall_run_f() calls dram_init_banksize(), and after relocation
>> board_init_r() calls enable_caches() (call stack: board_init_r() ->
>> initcall_run_r() -> initr_caches() -> enable_caches()), I would expect that
>> enable_caches() should not override the bank sizes previously configured.
>> Currently, however, enable_caches() introduces this side effect.
>>
>> Wouldn’t it make more sense to call fdtdec_setup_memory_banksize() in the
>> default dram_init_banksize() (in arch/arm/mach-k3/k3-ddr.c) and avoid calling
>> it again in mem_map_from_dram_banks()?
>>
> 
> We could follow that order too but that would makes a call to
> mem_map_from_dram_banks dependent on gd->bd->di_dram being correctly
> populated. I had assumed whoever calls mem_map_from_dram_banks had made
> sure to properly fixup the memory node of the fdt.
> 
> Given that it seems like the root cause of the problem is with the
> U-Boot's device-tree not having the correct memory node, we could add a
> call to fixup_memory_node (arch/arm/mach-k3/k3-ddr.c) from A53 SPL to
> ensure the memory can be queried stright from the device-tree once we do
> get to U-Boot proper.
> 
> 	--- a/board/toradex/verdin-am62/verdin-am62.c
> 	+++ b/board/toradex/verdin-am62/verdin-am62.c
> 	@@ -46,6 +46,13 @@ int dram_init_banksize(void)
> 		return ret;
> 	 }
> 
> 	+#ifdef CONFIG_XPL_BUILD
> 	+void spl_perform_board_fiups(struct spl_image_info *spl_image)
> 	+{
> 	+       fixup_memory_node(spl_image);
> 	+}
> 	+#endif
> 	+
> 	 /*
> 	  * Avoid relocated U-Boot clash with Linux reserved-memory on 512 MB SoM
> 	  */
> 
> 
> If you could get to U-Boot prompt with the above diff, could you
> share the output of the 'meminfo' command with the following configs
> added:
> 	CONFIG_CMD_MEMINFO=y
> 	CONFIG_CMD_MEMINFO_MAP=y
> 
> Though so far, I have been unsuccessful in my attempts to reproduce a
> boot failure on our own 512MiB platforms (AM62x SiP). Could you share
> the boot logs with '#define DEBUG' in common/board_r.c, common/board_f.c
> and mach-k3/common.c to help further narrow down the issue.
> 
> Regards,
> Anshul

Hello Anshul,
let me try to rephrase.

The enable_caches() function is not only enabling caches, but also updating
the memory map.
It is not expected from the point of view of the caller and this side effect
introduces a regression on our U-Boot, since the memory banks are already
configured in dram_init().

I understand that updating the device tree, as you proposed, could work around
the issue, but that does not really fix the root cause.
Having a function perform unexpected operations is the best way to introduce
regressions now and bugs in the future.

We should either revert the patch or properly fix the issue.

Regards,
Emanuele