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

[Bin Meng]

Hi Ilias,

On Wed, Sep 29, 2021 at 9:07 PM Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Bin
>
> There's a similar discussion in a cleanup series I've sent [1]
> On Sat, Sep 11, 2021 at 10:31:23PM +0800, Bin Meng wrote:
> > Commit 47d73ba4f4a4 ("board: sifive: overwrite board_fdt_blob_setup in u-boot proper")
> > added a board-specific implementation of board_fdt_blob_setup() which
> > takes a pointer as the return value, but it does not return anything
> > if CONFIG_OF_SEPARATE is not enabled. This will cause a build warning
> > seen when testing booting S-mode U-Boot directly from QEMU, per the
> > instructions in [1]:
>
> It's not only a build warning, you don't even have a DTB to begin with.
>
> >
> >   board/sifive/unleashed/unleashed.c: In function ‘board_fdt_blob_setup’:
> >   board/sifive/unleashed/unleashed.c:125:1: warning: control reaches end of non-void function [-Wreturn-type]
> >
> > Return &_end as the default case.
> >
> > [1] https://qemu.readthedocs.io/en/latest/system/riscv/sifive_u.html#running-u-boot
> >
> > Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> >
> > ---
> >
> > Changes in v2:
> > - Correct the commit title
> >
> >  board/sifive/unleashed/unleashed.c | 4 ++--
> >  board/sifive/unmatched/unmatched.c | 4 ++--
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/board/sifive/unleashed/unleashed.c b/board/sifive/unleashed/unleashed.c
> > index 8cd514df30..33baeda986 100644
> > --- a/board/sifive/unleashed/unleashed.c
> > +++ b/board/sifive/unleashed/unleashed.c
> > @@ -119,9 +119,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;
> >  }
> >
> >  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.

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

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

Regards,
Bin