Hi Ilias,

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

Signed-off-by: Bin Meng bmeng.cn@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