Re: [PATCH v2 1/9] riscv/boot: Add a missing header include

2020-05-14 Thread Bin Meng 
Hi Alistair,

On Thu, May 14, 2020 at 11:38 PM Alistair Francis  wrote:
>
> On Thu, May 14, 2020 at 8:34 AM Bin Meng  wrote:
> >
> > On Fri, May 8, 2020 at 3:24 AM Alistair Francis
> >  wrote:
> > >
> > > Signed-off-by: Alistair Francis 
> > > ---
> > >  include/hw/riscv/boot.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> > > index 474a940ad5..9daa98da08 100644
> > > --- a/include/hw/riscv/boot.h
> > > +++ b/include/hw/riscv/boot.h
> > > @@ -21,6 +21,7 @@
> > >  #define RISCV_BOOT_H
> > >
> > >  #include "exec/cpu-defs.h"
> > > +#include "hw/loader.h"
> >
> > Why is this needed? Currently this does not break build.
>
> Currently every c file that includes boot.h also includes loader.h
> before it. Which is why the build works fine. We should be able to
> include just boot.h though so this is a small fixup to allow that.
>

Please include such in the commit message to help people understand.

Reviewed-by: Bin Meng 

Regards,
Bin

Re: [PATCH v2 1/9] riscv/boot: Add a missing header include

2020-05-14 Thread Philippe Mathieu-Daudé 

On 5/14/20 5:46 PM, Bin Meng wrote:

On Thu, May 14, 2020 at 11:38 PM Alistair Francis  wrote:


On Thu, May 14, 2020 at 8:34 AM Bin Meng  wrote:


On Fri, May 8, 2020 at 3:24 AM Alistair Francis
 wrote:


Signed-off-by: Alistair Francis 
---
  include/hw/riscv/boot.h | 1 +
  1 file changed, 1 insertion(+)

diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index 474a940ad5..9daa98da08 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -21,6 +21,7 @@
  #define RISCV_BOOT_H

  #include "exec/cpu-defs.h"
+#include "hw/loader.h"


Why is this needed? Currently this does not break build.


Currently every c file that includes boot.h also includes loader.h
before it. Which is why the build works fine. We should be able to
include just boot.h though so this is a small fixup to allow that.



I wonder if this is a required convention to make the header file
self-contained? The only thing that is offending seems to be the
symbol_fn_t typedef.


Indeed the use of the symbol_fn_t typedef justifies including its 
declaration :)


Reviewed-by: Philippe Mathieu-Daudé 



Regards,
Bin

Re: [PATCH v2 1/9] riscv/boot: Add a missing header include

2020-05-14 Thread Bin Meng 
On Thu, May 14, 2020 at 11:38 PM Alistair Francis  wrote:
>
> On Thu, May 14, 2020 at 8:34 AM Bin Meng  wrote:
> >
> > On Fri, May 8, 2020 at 3:24 AM Alistair Francis
> >  wrote:
> > >
> > > Signed-off-by: Alistair Francis 
> > > ---
> > >  include/hw/riscv/boot.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> > > index 474a940ad5..9daa98da08 100644
> > > --- a/include/hw/riscv/boot.h
> > > +++ b/include/hw/riscv/boot.h
> > > @@ -21,6 +21,7 @@
> > >  #define RISCV_BOOT_H
> > >
> > >  #include "exec/cpu-defs.h"
> > > +#include "hw/loader.h"
> >
> > Why is this needed? Currently this does not break build.
>
> Currently every c file that includes boot.h also includes loader.h
> before it. Which is why the build works fine. We should be able to
> include just boot.h though so this is a small fixup to allow that.
>

I wonder if this is a required convention to make the header file
self-contained? The only thing that is offending seems to be the
symbol_fn_t typedef.

Regards,
Bin

Re: [PATCH v2 1/9] riscv/boot: Add a missing header include

2020-05-14 Thread Alistair Francis 
On Thu, May 14, 2020 at 8:34 AM Bin Meng  wrote:
>
> On Fri, May 8, 2020 at 3:24 AM Alistair Francis
>  wrote:
> >
> > Signed-off-by: Alistair Francis 
> > ---
> >  include/hw/riscv/boot.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> > index 474a940ad5..9daa98da08 100644
> > --- a/include/hw/riscv/boot.h
> > +++ b/include/hw/riscv/boot.h
> > @@ -21,6 +21,7 @@
> >  #define RISCV_BOOT_H
> >
> >  #include "exec/cpu-defs.h"
> > +#include "hw/loader.h"
>
> Why is this needed? Currently this does not break build.

Currently every c file that includes boot.h also includes loader.h
before it. Which is why the build works fine. We should be able to
include just boot.h though so this is a small fixup to allow that.

Alistair

>
> >
> >  void riscv_find_and_load_firmware(MachineState *machine,
> >const char *default_machine_firmware,
>
> Regards,
> Bin

Re: [PATCH v2 1/9] riscv/boot: Add a missing header include

2020-05-14 Thread Bin Meng 
On Fri, May 8, 2020 at 3:24 AM Alistair Francis
 wrote:
>
> Signed-off-by: Alistair Francis 
> ---
>  include/hw/riscv/boot.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 474a940ad5..9daa98da08 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -21,6 +21,7 @@
>  #define RISCV_BOOT_H
>
>  #include "exec/cpu-defs.h"
> +#include "hw/loader.h"

Why is this needed? Currently this does not break build.

>
>  void riscv_find_and_load_firmware(MachineState *machine,
>const char *default_machine_firmware,

Regards,
Bin

[PATCH v2 1/9] riscv/boot: Add a missing header include

2020-05-07 Thread Alistair Francis 
Signed-off-by: Alistair Francis 
---
 include/hw/riscv/boot.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index 474a940ad5..9daa98da08 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -21,6 +21,7 @@
 #define RISCV_BOOT_H
 
 #include "exec/cpu-defs.h"
+#include "hw/loader.h"
 
 void riscv_find_and_load_firmware(MachineState *machine,
   const char *default_machine_firmware,
-- 
2.26.2