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

[Devarsh Thakkar]

Hi Simon,

Thanks for the review.

On 05/11/23 01:13, Simon Glass wrote:
> Hi Devarsh,
> 
> On Fri, 3 Nov 2023 at 11:48, Devarsh Thakkar <devarsht at ti.com> wrote:
>>
>> 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 at 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 at chromium.org>
>>>> Signed-off-by: Devarsh Thakkar <devarsht at 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.
> 
> No, we need to fail if something is wrong.
> 

Yes the caller of the function is having void return value so can't propagate
it but I can still do a exit or hang. But besides this the API as such returns
the success/failure and it is left upto user to decide. As the user in this
case is TI (mack-k3) the previous behavior was preserved where it proceed with
boot on video reservation failure, I guess I will check again with more folks
here at TI and based on that take a decision.

>>
>>>>          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 ?
> 
> Yes, but specifically at the top of RAM, I believe?
> 
>>
>>> - 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.
> 
> That seems OK to me, so far as I understand what you are saying.
> 

Ok, will make it spl_video_reserve_from_ram_top, thanks!.

Regards
Devarsh

> [..]
> 
> Regards,
> Simon