Re: [PATCH v2 3/5] common/board_f: Catch bloblist before starting resevations

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:

...

Start reservations needed for init sequence only after catching bloblists from previous stage.

This is to avoid catching bloblists in the middle causing gaps while u-boot is reserving.

Adjust the relocaddr as per video hand-off information received from previous stage so that further reservations start only after regions reserved for previous stages

Skip reservation for video memory if it was already filled by a bloblist.

Signed-off-by: Devarsh Thakkar devarsht@ti.com

V2: Fix typo in commit title and checkpatch warnings/checks

common/board_f.c | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c index d4d7d01f8f..acf802c9cb 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -403,7 +403,7 @@ __weak int arch_reserve_mmu(void) return 0; }

-static int reserve_video(void) +static int reserve_video_from_videoblob(void) { if (IS_ENABLED(CONFIG_SPL_VIDEO_HANDOFF) && spl_phase() > PHASE_SPL) { struct video_handoff *ho; @@ -412,8 +412,34 @@ static int reserve_video(void) if (!ho) return log_msg_ret("blf", -ENOENT); video_reserve_from_bloblist(ho);

•           gd->relocaddr = ho->fb;
  

•   } else if (CONFIG_IS_ENABLED(VIDEO)) {
  

•           /* Sanity check fb from blob is before current relocaddr */
  

•           if (likely(gd->relocaddr > (unsigned long)ho->fb))
  

•                   gd->relocaddr = ho->fb;
  

•   }
  

•   return 0;
  

+}

+/*

• • Check if any bloblist received specifying reserved areas from previous stage and adjust
• • gd->relocaddr accordingly, so that we start reserving after pre-reserved areas
• • from previous stage.
• • NOTE:
• • IT is recommended that all bloblists from previous stage are reserved from ram_top
• • as next stage will simply start reserving further regions after them.
• */

+static int setup_relocaddr_from_bloblist(void) +{

•   reserve_video_from_videoblob();
  

•   return 0;
  

+}

+static int reserve_video(void)

[1] Here.

...

+{

•   if (CONFIG_IS_ENABLED(VIDEO)) {
             ulong addr;
             int ret;
  
  

@@ -923,6 +949,7 @@ static const init_fnc_t init_sequence_f[] = { reserve_pram, #endif reserve_round_4k,

•   setup_relocaddr_from_bloblist,
     arch_reserve_mmu,
     reserve_video,
  

But you have renamed this function, so how does this commit build?

Sorry I didn't get you, maybe the diff is creating an illusion, this is a new function to reserve from bloblist and a wrapper around reserve_video_from_bloblist. I created the wrapper so that in future if any new blobs are getting passed they can be added inside this (they need to be after video region which is from end of ram) and relocaddr adjusted accordingly before proceeding with uboot proper reservations. This is inline with our strategy to have SPL regions reserved from end of RAM so that uboot starts reserving right after them.

The existing function to reserve_video is still there [1]

buildman -b <branch> --board sandbox_spl

I remember testing this with make and also testing the functionality part of it, will try check with this command too.

Regards Devarsh

might help?

...

     reserve_trace,

-- 2.34.1

Regards, Simon