[PATCH 2/2] aarch64/versal: Support DDRMC0 region 0 and 1

[Chris Johns]

On 26/7/2022 11:47 pm, Kinsey Moore wrote:
> On 7/22/2022 01:35, chrisj at rtems.org wrote:
>> From: Chris Johns <chrisj at rtems.org>
>>
>> - Support DDRMC0 region 0 up to 2G in size
>>
>> - Support DDRMC0 region 1 with DDR memory greater than 2G
>>    up to the DDRMC0 max amount
>>
>> - Extend the heap with region 1's memory
>>
>> Closes #4684
>> ---
>>   bsps/aarch64/xilinx-versal/include/bsp.h      |  9 ++++
>>   bsps/aarch64/xilinx-versal/start/bspstart.c   |  2 +
>>   .../aarch64/xilinx-versal/start/bspstartmmu.c | 36 ++++++++++++++
>>   .../aarch64/xilinx-versal/linkcmds_lp64.yml   | 48 +++++++++++++++++--
>>   4 files changed, 90 insertions(+), 5 deletions(-)
>>
>> diff --git a/bsps/aarch64/xilinx-versal/include/bsp.h
>> b/bsps/aarch64/xilinx-versal/include/bsp.h
>> index 2017e10ade..0bd93f28bc 100644
>> --- a/bsps/aarch64/xilinx-versal/include/bsp.h
>> +++ b/bsps/aarch64/xilinx-versal/include/bsp.h
>> @@ -47,6 +47,7 @@
>>   #ifndef ASM
>>     #include <bsp/default-initial-extension.h>
>> +#include <bsp/linker-symbols.h>
>>   #include <bsp/start.h>
>>     #include <rtems.h>
>> @@ -61,6 +62,14 @@ extern "C" {
>>     #define BSP_RESET_SMC
>>   +/*
>> + * DDRMC mapping
>> + */
>> +LINKER_SYMBOL(bsp_r0_ram_base)
>> +LINKER_SYMBOL(bsp_r0_ram_end)
>> +LINKER_SYMBOL(bsp_r1_ram_base)
>> +LINKER_SYMBOL(bsp_r1_ram_end)
>> +
>>   /**
>>    * @brief Versal specific set up of the MMU.
>>    *
>> diff --git a/bsps/aarch64/xilinx-versal/start/bspstart.c
>> b/bsps/aarch64/xilinx-versal/start/bspstart.c
>> index 2f0048ddf3..86b3024dd2 100644
>> --- a/bsps/aarch64/xilinx-versal/start/bspstart.c
>> +++ b/bsps/aarch64/xilinx-versal/start/bspstart.c
>> @@ -38,6 +38,8 @@
>>   #include <bsp/irq-generic.h>
>>   #include <bsp/linker-symbols.h>
>>   +#include <bsp/aarch64-mmu.h>
>> +
> Unnecessary include? Nothing referencing MMU functionality was added to this file.

Thanks, left in by mistake.

>>   void bsp_start( void )
>>   {
>>     bsp_interrupt_initialize();
>> diff --git a/bsps/aarch64/xilinx-versal/start/bspstartmmu.c
>> b/bsps/aarch64/xilinx-versal/start/bspstartmmu.c
>> index 5949111d0d..c2fcd62c9f 100644
>> --- a/bsps/aarch64/xilinx-versal/start/bspstartmmu.c
>> +++ b/bsps/aarch64/xilinx-versal/start/bspstartmmu.c
>> @@ -38,6 +38,9 @@
>>   #include <bsp/aarch64-mmu.h>
>>   #include <libcpu/mmu-vmsav8-64.h>
>>   +#include <rtems/malloc.h>
>> +#include <rtems/sysinit.h>
>> +
>>   BSP_START_DATA_SECTION static const aarch64_mmu_config_entry
>>   versal_mmu_config_table[] = {
>>     AARCH64_MMU_DEFAULT_SECTIONS,
>> @@ -57,6 +60,24 @@ versal_mmu_config_table[] = {
>>       .begin = 0xff000000U,
>>       .end = 0xffc00000U,
>>       .flags = AARCH64_MMU_DEVICE
>> +  }, { /* DDRMC0_region1_mem, if not used size is 0 and ignored */
>> +    .begin = (uintptr_t) bsp_r1_ram_base,
>> +    .end = (uintptr_t) bsp_r1_ram_end,
>> +    .flags = AARCH64_MMU_DATA_RW_CACHED
>> +  }
>> +};
>> +
>> +/*
>> + * Create an MMU table to get the R1 base and end. This avoids
>> + * relocation errors as the R1 addresses are in the upper A64 address
>> + * space.
>> + */
>> +static const aarch64_mmu_config_entry
>> +bsp_r1_region[] = {
>> +  { /* DDRMC0_region1_mem, if not used size is 0 and ignored */
>> +    .begin = (uintptr_t) bsp_r1_ram_base,
>> +    .end = (uintptr_t) bsp_r1_ram_end,
>> +    .flags = 0
> 
> Is there a reason this data is being repeated over using the data already in the
> table above? 

The MMU table does not have any identifiers or tags to indicate which region is
for R1. How would you locate that specific region in the table to extend the heap?

I looked for other ways to embed the addresses but this is the only way I found
that did not generate a relocation error.

> Even if repeating the data makes it easier, use of this data
> structure is a little confusing since it's not being fed into the MMU routines
> and setting flags here is unnecessary.

I reused the existing struct to avoid making a new one for this specific task. I
could change the MMU one to contain the pair of addresses and make it more
generic, I can add a new struct that is specific to this region or I reuse this one.

I will post a v3.

Chris