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

[Ilias Apalodimas]

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?

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;

} 

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

>  }
>  
>  int board_init(void)
> -- 
> 2.25.1
> 

[1] https://lore.kernel.org/u-boot/YVRiVFP+sYGZhJiY@apalos.home/

Cheers
/Ilias