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