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

[Devarsh Thakkar <devarsht@ti.com>]

Hi Simon,

Thanks for the review.

On 03/11/23 04:16, Simon Glass wrote:
> Hi Devarsh,
> 
> On Tue, 31 Oct 2023 at 13:12, Devarsh Thakkar <devarsht@ti.com> wrote:
>>
>> Add function spl_reserve_video which is a wrapper
>> around video_reserve to setup video memory and update
>> the relocation address pointer.
>>
>> 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 done to enable the next stage to directly skip
>> the pre-reserved area from previous stage right from the end of RAM
>> without having to make any gaps/holes to accommodate those
>> regions which was the case before as previous stage
>> reserved region not from the end of RAM.
>>
>> Suggested-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>> V2: Make a generic function "spl_reserve_video" under
>>      common/spl which can be re-used by other platforms too
>>      for reserving video memory from spl.
>> ---
>>   arch/arm/mach-k3/common.c |  2 ++
>>   common/spl/spl.c          | 19 +++++++++++++++++++
>>   include/spl.h             |  4 ++++
>>   3 files changed, 25 insertions(+)
> 
>>
>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>> index c3006ba387..03e3b46282 100644
>> --- a/arch/arm/mach-k3/common.c
>> +++ b/arch/arm/mach-k3/common.c
>> @@ -537,6 +537,8 @@ void spl_enable_dcache(void)
>>          if (ram_top >= 0x100000000)
>>                  ram_top = (phys_addr_t) 0x100000000;
>>
>> +       gd->relocaddr = ram_top;
>> +       spl_reserve_video();
> 
> Need to check error here
> 

Ok, I can check for error and print a message, but would still proceed 
with build (similar to reserve_video in u-boot proper (from which this 
is derived)), I hope that is fine.

>>          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,
>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>> index 732d90d39e..89172f2ebf 100644
>> --- a/common/spl/spl.c
>> +++ b/common/spl/spl.c
>> @@ -41,6 +41,7 @@
>>   #include <fdt_support.h>
>>   #include <bootcount.h>
>>   #include <wdt.h>
>> +#include <video.h>
>>
>>   DECLARE_GLOBAL_DATA_PTR;
>>   DECLARE_BINMAN_MAGIC_SYM;
>> @@ -151,6 +152,24 @@ void spl_fixup_fdt(void *fdt_blob)
>>   #endif
>>   }
>>
>> +int spl_reserve_video(void)
>> +{
>> +       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;
>> +}
>> +
>>   ulong spl_get_image_pos(void)
>>   {
>>          if (!CONFIG_IS_ENABLED(BINMAN_UBOOT_SYMBOLS))
>> diff --git a/include/spl.h b/include/spl.h
>> index 0d49e4a454..9682e51fc1 100644
>> --- a/include/spl.h
>> +++ b/include/spl.h
>> @@ -825,6 +825,10 @@ int spl_usb_load(struct spl_image_info *spl_image,
>>
>>   int spl_ymodem_load_image(struct spl_image_info *spl_image,
>>                            struct spl_boot_device *bootdev);
>> +/**
>> + * spl_reserve_video() - Reserve video and update relocation address
> 
> This needs more detail about:
> - gd->relocaddr

Points to current relocation address which points to region below 
reserved area ?

> - where the video memory is allocated
I didn't get you, allocation is stored in RAM, is that what you mean ?

> - where the allocation is stored (in the video-device plat?)ok

> - return value
> 

Just a point of note, this spl_reserve_video is inspired by 
reserve_video from u-boot proper and we are essentially leaving upto 
users to follow the "guideline" to reserve video from RAM by setting
gd->relocaddr to end of RAM before calling to function.

But if we want to directly enforce the guideline, I can move that logic 
inside this API and we can have spl_reserve_video_from_ramtop, let me 
know your opinion on this if we want to go this way.

Regards
Devarsh

>> + */
>> +int spl_reserve_video(void);
>>
>>   /**
>>    * spl_invoke_atf - boot using an ARM trusted firmware image
>> --
>> 2.34.1
>>
> 
> Regards,
> Simon