Hi Ilias,
On Wed, Sep 29, 2021 at 9:07 PM Ilias Apalodimas
<ilias.apalodi...@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...@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
