Re: [PATCH v2] board: sifive: Fix a potential build warning in board_fdt_blob_setup()
On Fri, Sep 17, 2021 at 2:52 PM Rick Chen wrote:
>
> > From: Bin Meng
> > Sent: Saturday, September 11, 2021 10:31 PM
> > To: Zong Li ; Leo Yu-Chi Liang(梁育齊)
> > ; Rick Jian-Zhi Chen(陳建志) ;
> > 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
> >
> > ---
> >
> > 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
Ping for apply?
Re: [PATCH v2] board: sifive: Fix a potential build warning in board_fdt_blob_setup()
[...]
> > > 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
Re: [PATCH v2] board: sifive: Fix a potential build warning in board_fdt_blob_setup()
Hi Ilias,
On Wed, Sep 29, 2021 at 9:07 PM Ilias Apalodimas
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
> >
> > ---
> >
> > 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()
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
>
> ---
>
> 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+sygzh...@apalos.home/
Cheers
/Ilias
Re: [PATCH v2] board: sifive: Fix a potential build warning in board_fdt_blob_setup()
> From: Bin Meng
> Sent: Saturday, September 11, 2021 10:31 PM
> To: Zong Li ; Leo Yu-Chi Liang(梁育齊)
> ; Rick Jian-Zhi Chen(陳建志) ;
> 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
>
> ---
>
> 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
Re: [PATCH v2] board: sifive: Fix a potential build warning in board_fdt_blob_setup()
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
Reviewed-by: Leo Yu-Chi Liang
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.
