Re: [PATCH v5 26/26] fdt: Don't call board_fdt_blob_setup() without OF_BOARD

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Simon,

On Tue, 26 Oct 2021 at 18:27, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Tue, 26 Oct 2021 at 07:56, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > As I said here [1], this is moving on an entirely different direction I had
> > in mind. I'd much prefer starting the discussions for a solution that
> > allows us to scale.
>
> I am missing the point here. Is there something in the plans that I
> don't know about?

I have some ideas, but haven't found time to code it and send patches yet.

>
> > FWIW I think the current code is still not clean for my taste.  Commit
> > 3b595da441cf ("fdtdec: allow board to provide fdt for CONFIG_OF_SEPARATE")
> > allowed this function to be used regardless of the config options.  IMHO we
> > should have 2 clear options:
> > - U-Boot provides the DTB
>
> Supported with: OF_SEPARATE
>
> > - It's somehow passed over to U-Boot
>
> Supported with: OF_SEPARATE + OF_BOARD

That's exactly what I don't personally like. In your example OF_BOARD
means "U-Boot has a DTB and here's a way to override it".  In my head
that options means "U-Boot was handed over a DTB on a register and it
must be able to deal with it".  The latter is obviously less
restrictive to previous bootloaders.

>
> I actually hit this problem with my qemu testing, in that it is
> actually not possible to use U-Boot's devicetree without hacking the
> code. We need to be able to select this feature explicitly, or turn it
> off, at least for development.

Alternatively we can just keep U-Boot config node in a .dtbo and apply
it on the fly after QEMU has handed us over the DTB it created.

Regards
/Ilias
>
> Regards,
> Simon
>
>
> >
> > On Mon, Oct 25, 2021 at 06:23:44PM -0600, Simon Glass wrote:
> > > At present this override function is called even when OF_BOARD Is not
> > > enabled. This makes it impossible to disable this feature and in fact
> > > makes the OF_BOARD option useless.
> > >
> > > Reinstate its intended purpose, so that it is possible to switch between
> > > the appended devicetree and one provided by the board's custom function.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > Changes in v5:
> > > - Add new patches to clean up fdtdec_setup() and surrounds
> > >
> > >  include/fdtdec.h |  7 +++++--
> > >  lib/fdtdec.c     | 17 +++++++++++------
> > >  2 files changed, 16 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > > index 386f6611294..b2faa84008e 100644
> > > --- a/include/fdtdec.h
> > > +++ b/include/fdtdec.h
> > > @@ -1170,8 +1170,11 @@ int fdtdec_resetup(int *rescan);
> > >
> > >  /**
> > >   * Board-specific FDT initialization. Returns the address to a device tree blob.
> > > - * Called when CONFIG_OF_BOARD is defined, or if CONFIG_OF_SEPARATE is defined
> > > - * and the board implements it.
> > > + * Called when CONFIG_OF_BOARD is defined.
> > > + *
> > > + * The existing devicetree is available at gd->fdt_blob
> > > + *
> > > + * @returns new devicetree blob pointer
> > >   */
> > >  void *board_fdt_blob_setup(void);
> > >
> > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > > index 067c27d0aa3..da36dffec62 100644
> > > --- a/lib/fdtdec.c
> > > +++ b/lib/fdtdec.c
> > > @@ -1203,11 +1203,12 @@ static int uncompress_blob(const void *src, ulong sz_src, void **dstp)
> > >       return 0;
> > >  }
> > >
> > > -/*
> > > - * For CONFIG_OF_SEPARATE, the board may optionally implement this to
> > > - * provide and/or fixup the fdt.
> > > +/**
> > > + * fdt_find_separate() - Find a devicetree at the end of the image
> > > + *
> > > + * @return pointer to FDT blob
> > >   */
> > > -__weak void *board_fdt_blob_setup(void)
> > > +static void *fdt_find_separate(void)
> > >  {
> > >       void *fdt_blob = NULL;
> > >  #ifdef CONFIG_SPL_BUILD
> > > @@ -1623,11 +1624,15 @@ int fdtdec_setup(void)
> > >       int ret;
> > >
> > >       /* The devicetree is typically appended to U-Boot */
> > > -     if (IS_ENABLED(CONFIG_OF_SEPARATE) || IS_ENABLED(CONFIG_OF_BOARD))
> > > -             gd->fdt_blob = board_fdt_blob_setup();
> > > +     if (IS_ENABLED(CONFIG_OF_SEPARATE))
> > > +             gd->fdt_blob = fdt_find_separate();
> > >       else /* embed dtb in ELF file for testing / development */
> > >               gd->fdt_blob = dtb_dt_embedded();
> > >
> > > +     /* Allow the board to override the fdt address. */
> > > +     if (IS_ENABLED(CONFIG_OF_BOARD))
> > > +             gd->fdt_blob = board_fdt_blob_setup();
> > > +
> > >       if (!IS_ENABLED(CONFIG_SPL_BUILD)) {
> > >               /* Allow the early environment to override the fdt address */
> > >               gd->fdt_blob = map_sysmem(env_get_ulong("fdtcontroladdr", 16,
> > > --
> > > 2.33.0.1079.g6e70778dc9-goog
> > >
> >
> > [1] https://lore.kernel.org/u-boot/YXekTkeL73NM0UOU@apalos.home/
> >
> > Regards
> > /Ilias