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

[Lokesh Vutla]

Simon,

On 22/04/19 8:15 PM, Tom Rini wrote:
> On Thu, Apr 18, 2019 at 08:23:45AM +0200, Simon Goldschmidt wrote:
>> On Thu, Apr 18, 2019 at 8:12 AM Lokesh Vutla <lokeshvutla at ti.com> wrote:
>>>
>>>
>>>
>>> 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?
>>
>> While it's good to know this fixes your issue, I'm not sure this doesn't break
>> anything else.
>>
>> Tom, Simon, can you add anything here? Why is the dtb memory added as
>> reserved to itself? It seems to me this is redundant, as the code using the
>> dtb should well know the pointer to it...
>>
>> Lokesh, I can send a patch once this is discussed.
> 
> OK, so the history of that call is really what's seen in:
> commit 41c5eaa7253ed82bbae1eda5667755872c615164
> Author: Andy Fleming <afleming at freescale.com>
> Date:   Mon Jun 16 13:58:56 2008 -0500
> 
>     Resize device tree to allow space for board changes and the chosen node
>     
>     Current code requires that a compiled device tree have space added to the end to
>     leave room for extra nodes added by board code (and the chosen node).  This
>     requires that device tree creators anticipate how much space U-Boot will add to
>     the tree, which is absurd.  Ideally, the code would resize and/or relocate the
>     tree when it needed more space, but this would require a systemic change to the
>     fdt code, which is non-trivial.  Instead, we resize the tree inside
>     boot_relocate_fdt, reserving either the remainder of the bootmap (in the case
>     where the fdt is inside the bootmap), or adding CFG_FDT_PAD bytes to the size.
>     
>     Signed-off-by: Andy Fleming <afleming at freescale.com>
> 
> Which is all about preparing things for passing the device tree we
> loaded specifically for Linux.  In:
> commit 3082d2348c8e13342f5fdd10e9b3f7408062dbf9
> Author: Kumar Gala <galak at kernel.crashing.org>
> Date:   Fri Aug 15 08:24:42 2008 -0500
> 
>     fdt: refactor fdt resize code
>     
>     Move the fdt resizing code out of ppc specific boot code and into
>     common fdt support code.
>     
>     Signed-off-by: Kumar Gala <galak at kernel.crashing.org>
> 
> The code was moved to a more common area.  So the intent on "reserve"
> was to make sure that we had enough space set aside for the dtb to be
> able to be updated at runtime, by U-Boot, for various needs and that we
> wouldn't give that memory away to something else / allow it to be
> clobbered.
> 

Any update on this? Any chance you will be posting your changes?

Thanks and regards,
Lokesh