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

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

[Bin Meng]

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]:

  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@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;
 }
 
 int board_init(void)
-- 
2.25.1

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

[Leo Liang]

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]:
>
>   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@gmail.com>

Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
CONFIDENTIALITY NOTICE:

This e-mail (and its attachments) may contain confidential and legally privileged information or information protected from disclosure. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein is strictly prohibited. In this case, please immediately notify the sender by return e-mail, delete the message (and any accompanying documents) and destroy all printed hard copies. Thank you for your cooperation.

Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.

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

[Rick Chen]

> From: Bin Meng <bmeng.cn@gmail.com>
> Sent: Saturday, September 11, 2021 10:31 PM
> To: Zong Li <zong.li@sifive.com>; Leo Yu-Chi Liang(梁育齊) <ycliang@andestech.com>; Rick Jian-Zhi Chen(陳建志) <rick@andestech.com>; u-boot@lists.denx.de
> Subject: [PATCH v2] board: sifive: Fix a potential build warning in board_fdt_blob_setup()
>
> 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]:
>
>   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@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(-)

Reviewed-by: Rick Chen <rick@andestech.com>

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

[Bin Meng]

On Fri, Sep 17, 2021 at 2:52 PM Rick Chen <rickchen36@gmail.com> wrote:
>
> > From: Bin Meng <bmeng.cn@gmail.com>
> > Sent: Saturday, September 11, 2021 10:31 PM
> > To: Zong Li <zong.li@sifive.com>; Leo Yu-Chi Liang(梁育齊) <ycliang@andestech.com>; Rick Jian-Zhi Chen(陳建志) <rick@andestech.com>; u-boot@lists.denx.de
> > Subject: [PATCH v2] board: sifive: Fix a potential build warning in board_fdt_blob_setup()
> >
> > 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]:
> >
> >   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@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(-)
>
> Reviewed-by: Rick Chen <rick@andestech.com>

Ping for apply?

Re: [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@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

Re: [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@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@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

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

[Ilias Apalodimas]

[...]
> > >  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