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

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190422/670deeaa/attachment.sig>