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

[Simon Goldschmidt]

Hi Lokesh,

On 03.05.19 16:24, Lokesh Vutla wrote:
> 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?

No, sorry, not update. I'll send the patch I suggested above and we'll 
see how the discussion continues.

Regards,
Simon