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

[Simon Goldschmidt]

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.

Regards,
Simon

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