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

[Simon Glass]

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.

>
> >>          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.

[..]

Regards,
Simon