[U-Boot] [PATCH] board_f: Do not mark pre-relocated fdt space as reserved

[Lokesh Vutla]

On 18/04/19 12:46 AM, Simon Goldschmidt wrote:
> [This is a follow-up to https://patchwork.ozlabs.org/patch/1033584/ right?]

Right.

> 
> Am 16.04.2019 um 10:43 schrieb Lokesh Vutla:
>> SPL while copying u-boot and dtb it does the following:
>> - Copy u-boot
>> - Copy right dtb.
>> - mark dtb location as reserved in dtb.
> 
> Hmm, why does it do that? Reserving space when U-Boot starts Linux *might* make
> sense, but why should SPL add this reservation when starting U-Boot? These steps
> are for U-Boot in a fit-image, right? Because I can't see this for U-Boot as
> uImage.
> 
> Am I correct that 'fdt_shrink_to_minimum()' adds this reservation? The name of
> that function doesn't suggest that to me. Would it make more sense to let this
> funtion only *change* the reservation, i.e. only add it if it is removed at the
> beginning of said function? Would that fix your issue?
> 
> Something like this:
> 
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index ab08a0114f..4e7cf6ebe9 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -597,6 +597,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
>         uint64_t addr, size;
>         int total, ret;
>         uint actualsize;
> +       int fdt_memrsv = 0;
> 
>         if (!blob)
>                 return 0;
> @@ -606,6 +607,7 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
>                 fdt_get_mem_rsv(blob, i, &addr, &size);
>                 if (addr == (uintptr_t)blob) {
>                         fdt_del_mem_rsv(blob, i);
> +                       fdt_memrsv = 1;
>                         break;
>                 }
>         }
> @@ -627,10 +629,12 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
>         /* Change the fdt header to reflect the correct size */
>         fdt_set_totalsize(blob, actualsize);
> 
> -       /* Add the new reservation */
> -       ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
> -       if (ret < 0)
> -               return ret;
> +       if (fdt_memrsv) {
> +               /* Add the new reservation */
> +               ret = fdt_add_mem_rsv(blob, map_to_sysmem(blob), actualsize);
> +               if (ret < 0)
> +                       return ret;
> +       }

Agreed. This works and more appropriate place to fix it. Will you post a
separate patch or do you want me to post it on your behalf?

Thanks and regards,
Lokesh

> 
>         return actualsize;
>  }
> 
> 
>>
>> U-Boot when copying images at U-Boot prompt, is not able to copy to the
>> above dtb location as it sees the region as reserved. But at this stage
>> dtb is re located to end of DDR. And the above dtb region is not reserved
>> anymore. So delete this reserved region when re locating dtb.
>>
>> Reported-by: Keerthy <j-keerthy at ti.com>
>> Signed-off-by: Lokesh Vutla <lokeshvutla at ti.com>
>> ---
>>   common/board_f.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/common/board_f.c b/common/board_f.c
>> index 149a7229e8..e4383ae3fa 100644
>> --- a/common/board_f.c
>> +++ b/common/board_f.c
>> @@ -651,10 +651,23 @@ static int init_post(void)
>>   static int reloc_fdt(void)
>>   {
>>   #ifndef CONFIG_OF_EMBED
>> +    uint64_t addr, size;
>> +    int i, cnt, err;
>> +
>>       if (gd->flags & GD_FLG_SKIP_RELOC)
>>           return 0;
>>       if (gd->new_fdt) {
>>           memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
>> +
>> +        /* Deleting the previously marked FDT reserved region */
>> +        cnt = fdt_num_mem_rsv(gd->new_fdt);
>> +        for (i = 0; i < cnt ; i++) {
>> +            err = fdt_get_mem_rsv(gd->new_fdt, i, &addr, &size);
>> +            if (!err && addr == (uintptr_t)gd->fdt_blob) {
>> +                fdt_del_mem_rsv(gd->new_fdt, i);
>> +                break;
>> +            }
>> +        }
> 
> That code should work, but it's a bit unexpected in this location
> 
>>           gd->fdt_blob = gd->new_fdt;
>>       }
>>   #endif
>>
>