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 <devar...@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 <devar...@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 <s...@chromium.org> >>>> Signed-off-by: Devarsh Thakkar <devar...@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