Re: [PATCH v2] board: sifive: Fix a potential build warning in board_fdt_blob_setup()

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

[...]
> > >  int board_init(void)
> > > diff --git a/board/sifive/unmatched/unmatched.c b/board/sifive/unmatched/unmatched.c
> > > index d90b252bae..8773b660fa 100644
> > > --- a/board/sifive/unmatched/unmatched.c
> > > +++ b/board/sifive/unmatched/unmatched.c
> > > @@ -16,9 +16,9 @@ void *board_fdt_blob_setup(void)
> > >       if (IS_ENABLED(CONFIG_OF_SEPARATE)) {
> > >               if (gd->arch.firmware_fdt_addr)
> > >                       return (ulong *)gd->arch.firmware_fdt_addr;
> > > -             else
> > > -                     return (ulong *)&_end;
> > >       }
> > > +
> > > +     return (ulong *)&_end;
> >
> > is _end correct here? Doesn't the placement depend on CONFIG_SPL_BUILD and
> > CONFIG_SPL_SEPARATE_BSS?
> >
> 
> I will have to leave this to Zong to comment as he introduced this via
> commit 47d73ba4f4a4 ("board: sifive: overwrite board_fdt_blob_setup in
> u-boot proper").
> 
> I don't know what user case that Zong wanted to support on Unleased
> and Unmatched.

Okay.  The I think we can fix this correctly.  We can probably use OF_BOARD
in those boards,  as long as the SPL generated DTB is always there and
passed over to OpenSBI.

> 
> > To sum up the other thread I think the overall approach here is not ideal.
> > OF_SEPARATE is used in conjunction with SPL in these boards.  What happens
> > (assuming I didn't miss anything),  is that SPL passes the DTB to OpenSBI,
> > which in it turn copies to a1 before invoking U-Boot.
> > But we are better of with stricter rules for DTB.
> > - OF_SEPARATE means: I'll read the DTB from the U-Boot binary.
> > - OF_BOARD: The board will somehow provide it.
> >
> > So II think the patch should look something along the lines of:
> >
> > if (IS_ENABLED(CONFIG_OF_SEPARATE)) {
> > #ifdef CONFIG_SPL_BUILD
> >         /* FDT is at end of BSS unless it is in a different memory region */
> >         if (IS_ENABLED(CONFIG_SPL_SEPARATE_BSS))
> >                 fdt_blob = (void *)&_image_binary_end;
> >         else
> >                 fdt_blob = (void *)&__bss_end;
> > #else
> >         /* FDT is at end of image */
> >         fdt_blob = (void *)&_end;
> >
> 
> Missing #endif here?o

Yea it wasn't supposed to be real code.

> 
> > }
> >
> > if (IS_ENABLED(CONFIG_OF_BOARD)) {
> >         fdt_blob = (void *)gd->arch.firmware_fdt_addr;
> > }
> >
> > >  }
> > >
> > >  int board_init(void)
> > > --
> 
> Regards,
> Bin

Regards
/Ilias