On Fri Sep 12, 2025 at 12:56 PM IST, Ilias Apalodimas wrote:
> On Fri, 12 Sept 2025 at 09:38, Anshul Dalal <ansh...@ti.com> wrote:
>>
>> Hi Ilias,
>>
>> On Fri Sep 12, 2025 at 12:03 PM IST, Ilias Apalodimas wrote:
>> > Hi Anshul,
>> >
>> > On Fri, 5 Sept 2025 at 11:19, Anshul Dalal <ansh...@ti.com> wrote:
>> >>
>> >> For armv8, U-Boot uses a static map defined as 'mem_map' for configuring
>> >> the MMU as part of mmu_setup.
>> >>
>> >> But since the exact configuration of memory banks might not be known at
>> >> build time, many platforms such as imx9, versal2 etc. utilize
>> >> gd->bd->bi_dram to configure the static map at runtime.
>> >>
>> >> Therefore this patch adds a new API mem_map_fix_dram_banks that modifies
>> >> the static map in a similar way. Allowing for anyone to map all dram
>> >> banks by just passing the index to last entry in their mem_map and it's
>> >> length.
>> >>
>> >> Signed-off-by: Anshul Dalal <ansh...@ti.com>
>> >> ---
>> >> arch/arm/cpu/armv8/cache_v8.c | 27 +++++++++++++++++++++++++++
>> >> arch/arm/include/asm/armv8/mmu.h | 10 ++++++++++
>> >> 2 files changed, 37 insertions(+)
>> >>
>> >> diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
>> >> index 6e662395a83..edb895da1f2 100644
>> >> --- a/arch/arm/cpu/armv8/cache_v8.c
>> >> +++ b/arch/arm/cpu/armv8/cache_v8.c
>> >> @@ -58,6 +58,33 @@ static int get_effective_el(void)
>> >> return el;
>> >> }
>> >>
>> >> +int mem_map_fix_dram_banks(unsigned int index, unsigned int len, u64
>> >> attrs)
>> >> +{
>> >> + unsigned int i;
>> >> + int ret = fdtdec_setup_memory_banksize();
>> >> +
>> >> + if (ret) {
>> >> + printf("%s: Failed to setup dram banks\n", __func__);
>> >> + return ret;
>> >> + }
>> >> +
>> >> + if (index + CONFIG_NR_DRAM_BANKS >= len) {
>> >> + printf("%s: Not sufficient space in memory map\n",
>> >> __func__);
>> >> + return -ENOMEM;
>> >> + }
>> >
>> > The length of the mem_map array is configured at compile time.
>> > So this is going to fail for all existing boards unless someone
>> > explicitly changes the size of their struct mm_region.
>> > I think if we go down that path, we need to change the error message
>> > as well and point them to the right direction
>> >
>> >> +
>> >> + for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
>> >> + mem_map[index].virt = gd->bd->bi_dram[i].start;
>> >> + mem_map[index].phys = gd->bd->bi_dram[i].start;
>> >> + mem_map[index].size = gd->bd->bi_dram[i].size;
>> >> + mem_map[index].attrs = attrs;
>> >> + index++;
>> >> + }
>> >> +
>> >> + mem_map[index] = (const struct mm_region){ 0 };
>> >
>> > Why do you need the const?
>> > Can we do
>> > memset(&mem_map[index], 0, sizeof(mem_map[index]));
>> > instead?
>> >
>>
>> Yeah, we could do that instead. But it essentially achieves the same
>> thing, right?
>
> Yes, i just find it a bit easier to read
>
That's fair, will update in a v2 along with a better error message for
mem_map array size.
Thanks for the review,
Anshul
