| Message ID | 20231031131208.435268-2-devarsht@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
| Series | Move video memory reservation for SPL to the | expand |
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 > 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 - where the video memory is allocated - where the allocation is stored (in the video-device plat?) - return value > + */ > +int spl_reserve_video(void); > > /** > * spl_invoke_atf - boot using an ARM trusted firmware image > -- > 2.34.1 > Regards, Simon
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
Hi Devarsh, On Fri, 3 Nov 2023 at 11:48, Devarsh Thakkar <devarsht@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@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. 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
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@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@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. > > 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
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(); 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 + */ +int spl_reserve_video(void); /** * spl_invoke_atf - boot using an ARM trusted firmware image
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(+)