[RFC PATCH 1/5] arm: mach-k3: common: Reserve video memory from end of the RAM

[Devarsh Thakkar]

Hi Simon,

Thanks for the review.

On 19/10/23 19:26, Simon Glass wrote:
> Hi Devarsh,
> 
> On Mon, 16 Oct 2023 at 10:06, Devarsh Thakkar <devarsht at ti.com> wrote:
>>
>> Move the function to setup video memory before page table
>> reservation so that framebuffer memory gets reserved from
>> the end of RAM.
>>
>> This is as per the new policy being discussed for passing
>> blobs where each of the reserved areas for bloblists
>> to be passed need to be reserved at the end of RAM.
>>
>> This is to enable the next stage to directly skip
>> the pre-reserved area from previous stage without
>> having to making any gaps/holes to accomodate those
>> regions which was the case if previous stage
>> reserved region say somewhere in the middle and not at
>> the end of RAM.
>>
>> Suggested-by: Simon Glass <sjg at chromium.org>
>> Signed-off-by: Devarsh Thakkar <devarsht at ti.com>
>> ---
>>  arch/arm/mach-k3/common.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>> index cc755dd1bf..3978b9ccca 100644
>> --- a/arch/arm/mach-k3/common.c
>> +++ b/arch/arm/mach-k3/common.c
>> @@ -27,6 +27,7 @@
>>  #include <env.h>
>>  #include <elf.h>
>>  #include <soc.h>
>> +#include <video.h>
>>
>>  #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF)
>>  enum {
>> @@ -522,6 +523,24 @@ void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size)
>>         }
>>  }
>>
>> +static int video_setup(void)
> 
> Shouldn't this be in generic code?
> 

The reason I kept it here instead of video-uclass was since this function also
uses and updates gd->relocaddr and not just reserves the memory region and I
don't see any function in video-uclass playing with gd->relocaddr
and this is inspired by and parallel to reserve_video from common/board_f.c
which is also static function defined in board_f instead of video-uclass.

This basically is a wrapper function which calls video_reserve and also
uses/updates gd->relocaddr, since I am calling this for SPL I could not find
any common layer to define this since most vendors call board_init_f from
platform specific file which in turn call this function.

Could you please share your opinion and suggestions for generic location if
above still not look good?

Regards
Devarsh

>> +{
>> +       if (CONFIG_IS_ENABLED(VIDEO)) {
>> +               ulong addr;
>> +               int ret;
>> +
>> +               addr = gd->relocaddr;
>> +               ret = video_reserve(&addr);
>> +               if (ret)
>> +                       return ret;
>> +               debug("Reserving %luk for video at: %08lx\n",
>> +                     ((unsigned long)gd->relocaddr - addr) >> 10, addr);
>> +               gd->relocaddr = addr;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  void spl_enable_dcache(void)
>>  {
>>  #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF))
>> @@ -537,6 +556,8 @@ void spl_enable_dcache(void)
>>         if (ram_top >= 0x100000000)
>>                 ram_top = (phys_addr_t) 0x100000000;
>>
>> +       gd->relocaddr = ram_top;
>> +       video_setup();
>>         gd->arch.tlb_addr = ram_top - gd->arch.tlb_size;
>>         gd->arch.tlb_addr &= ~(0x10000 - 1);
>>         debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
>> --
>> 2.34.1
>>
> 
> Regards,
> Simon