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

[Anshul Dalal <anshuld@ti.com>]

On Fri Oct 31, 2025 at 4:30 PM IST, Emanuele Ghidoli wrote:
>
>
> 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().

Hi Emanuele,

Ah, I see your point. In that case mem_map_from_dram_banks should only
rely on the gd with dram_init and dram_init_banksize taking care of
configuring gd properly.

I'll send a fix as per your suggestion (moving
fdtdec_setup_memory_banksize to the default dram_init_banksize) shortly.

Regards,
Anshul

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