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

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(-)

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)
> +{
> +       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?

buildman -b <branch> --board sandbox_spl

might help?

>         reserve_trace,
> --
> 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:
>>
>> 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