| Message ID | 20231016160611.1353458-2-devarsht@ti.com |
|---|---|
| State | RFC |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
| Series | Move video memory reservation for SPL at the end of RAM | expand |
Hi Devarsh, On Mon, 16 Oct 2023 at 10:06, Devarsh Thakkar <devarsht@ti.com> wrote: > > Move the function to 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 to enable the next stage to directly skip > the pre-reserved area from previous stage without > having to making any gaps/holes to accomodate those > regions which was the case if previous stage > reserved region say somewhere in the middle and not at > the end of RAM. > > Suggested-by: Simon Glass <sjg@chromium.org> > Signed-off-by: Devarsh Thakkar <devarsht@ti.com> > --- > arch/arm/mach-k3/common.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c > index cc755dd1bf..3978b9ccca 100644 > --- a/arch/arm/mach-k3/common.c > +++ b/arch/arm/mach-k3/common.c > @@ -27,6 +27,7 @@ > #include <env.h> > #include <elf.h> > #include <soc.h> > +#include <video.h> > > #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF) > enum { > @@ -522,6 +523,24 @@ void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size) > } > } > > +static int video_setup(void) Shouldn't this be in generic code? > +{ > + 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; > +} > + > void spl_enable_dcache(void) > { > #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF)) > @@ -537,6 +556,8 @@ void spl_enable_dcache(void) > if (ram_top >= 0x100000000) > ram_top = (phys_addr_t) 0x100000000; > > + gd->relocaddr = ram_top; > + video_setup(); > 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, > -- > 2.34.1 > Regards, Simon
Hi Simon, Thanks for the review. On 19/10/23 19:26, Simon Glass wrote: > Hi Devarsh, > > On Mon, 16 Oct 2023 at 10:06, Devarsh Thakkar <devarsht@ti.com> wrote: >> >> Move the function to 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 to enable the next stage to directly skip >> the pre-reserved area from previous stage without >> having to making any gaps/holes to accomodate those >> regions which was the case if previous stage >> reserved region say somewhere in the middle and not at >> the end of RAM. >> >> Suggested-by: Simon Glass <sjg@chromium.org> >> Signed-off-by: Devarsh Thakkar <devarsht@ti.com> >> --- >> arch/arm/mach-k3/common.c | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c >> index cc755dd1bf..3978b9ccca 100644 >> --- a/arch/arm/mach-k3/common.c >> +++ b/arch/arm/mach-k3/common.c >> @@ -27,6 +27,7 @@ >> #include <env.h> >> #include <elf.h> >> #include <soc.h> >> +#include <video.h> >> >> #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF) >> enum { >> @@ -522,6 +523,24 @@ void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size) >> } >> } >> >> +static int video_setup(void) > > Shouldn't this be in generic code? > The reason I kept it here instead of video-uclass was since this function also uses and updates gd->relocaddr and not just reserves the memory region and I don't see any function in video-uclass playing with gd->relocaddr and this is inspired by and parallel to reserve_video from common/board_f.c which is also static function defined in board_f instead of video-uclass. This basically is a wrapper function which calls video_reserve and also uses/updates gd->relocaddr, since I am calling this for SPL I could not find any common layer to define this since most vendors call board_init_f from platform specific file which in turn call this function. Could you please share your opinion and suggestions for generic location if above still not look good? Regards Devarsh >> +{ >> + 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; >> +} >> + >> void spl_enable_dcache(void) >> { >> #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF)) >> @@ -537,6 +556,8 @@ void spl_enable_dcache(void) >> if (ram_top >= 0x100000000) >> ram_top = (phys_addr_t) 0x100000000; >> >> + gd->relocaddr = ram_top; >> + video_setup(); >> 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, >> -- >> 2.34.1 >> > > Regards, > Simon
On Mon, Oct 23, 2023 at 05:41:10PM +0530, Devarsh Thakkar wrote: > Hi Simon, > > Thanks for the review. > > On 19/10/23 19:26, Simon Glass wrote: > > Hi Devarsh, > > > > On Mon, 16 Oct 2023 at 10:06, Devarsh Thakkar <devarsht@ti.com> wrote: > >> > >> Move the function to 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 to enable the next stage to directly skip > >> the pre-reserved area from previous stage without > >> having to making any gaps/holes to accomodate those > >> regions which was the case if previous stage > >> reserved region say somewhere in the middle and not at > >> the end of RAM. > >> > >> Suggested-by: Simon Glass <sjg@chromium.org> > >> Signed-off-by: Devarsh Thakkar <devarsht@ti.com> > >> --- > >> arch/arm/mach-k3/common.c | 21 +++++++++++++++++++++ > >> 1 file changed, 21 insertions(+) > >> > >> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c > >> index cc755dd1bf..3978b9ccca 100644 > >> --- a/arch/arm/mach-k3/common.c > >> +++ b/arch/arm/mach-k3/common.c > >> @@ -27,6 +27,7 @@ > >> #include <env.h> > >> #include <elf.h> > >> #include <soc.h> > >> +#include <video.h> > >> > >> #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF) > >> enum { > >> @@ -522,6 +523,24 @@ void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size) > >> } > >> } > >> > >> +static int video_setup(void) > > > > Shouldn't this be in generic code? > > > > The reason I kept it here instead of video-uclass was since this function also > uses and updates gd->relocaddr and not just reserves the memory region and I > don't see any function in video-uclass playing with gd->relocaddr > and this is inspired by and parallel to reserve_video from common/board_f.c > which is also static function defined in board_f instead of video-uclass. > > This basically is a wrapper function which calls video_reserve and also > uses/updates gd->relocaddr, since I am calling this for SPL I could not find > any common layer to define this since most vendors call board_init_f from > platform specific file which in turn call this function. > > Could you please share your opinion and suggestions for generic location if > above still not look good? Unless Simon speaks up, this is probably fine enough and we can always do further cleanups afterwards. Please do a non-RFC where you address Simon's minor comments, thanks.
diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c index cc755dd1bf..3978b9ccca 100644 --- a/arch/arm/mach-k3/common.c +++ b/arch/arm/mach-k3/common.c @@ -27,6 +27,7 @@ #include <env.h> #include <elf.h> #include <soc.h> +#include <video.h> #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF) enum { @@ -522,6 +523,24 @@ void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size) } } +static int video_setup(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; +} + void spl_enable_dcache(void) { #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF)) @@ -537,6 +556,8 @@ void spl_enable_dcache(void) if (ram_top >= 0x100000000) ram_top = (phys_addr_t) 0x100000000; + gd->relocaddr = ram_top; + video_setup(); 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,
Move the function to 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 to enable the next stage to directly skip the pre-reserved area from previous stage without having to making any gaps/holes to accomodate those regions which was the case if previous stage reserved region say somewhere in the middle and not at the end of RAM. Suggested-by: Simon Glass <sjg@chromium.org> Signed-off-by: Devarsh Thakkar <devarsht@ti.com> --- arch/arm/mach-k3/common.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)