Re: [U-Boot] [PATCH] global_data: Move x86 specific GD flags into common flags

2019-08-15 Thread Simon Goldschmidt
Hi Stefan,

On Thu, Aug 15, 2019 at 10:06 AM Stefan Roese  wrote:
>
> Hi Simon,
>
> On 15.08.19 09:35, Simon Goldschmidt wrote:
> > Hi Stefan,
> >
> > On Thu, Aug 15, 2019 at 9:05 AM Stefan Roese  wrote:
> >>
> >> Currently the upper 16 bits of the GD flags are reserved for
> >> architecture specific flags. But only x86 uses 2 bits of these 16 bits
> >> and sprinkling those flags in multiple headers is confusing and does not
> >> scale.
> >>
> >> This patch now moves the x86 flags into the common header and removes
> >> the comment about the reservation of the upper 16 bits.
> >>
> >> Signed-off-by: Stefan Roese 
> >> Cc: Bin Meng 
> >> Cc: Simon Glass 
> >> Cc: Tom Rini 
> >> Cc: Simon Goldschmidt 
> >> ---
> >>   arch/x86/include/asm/global_data.h | 6 --
> >>   include/asm-generic/global_data.h  | 6 +-
> >>   2 files changed, 5 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/global_data.h 
> >> b/arch/x86/include/asm/global_data.h
> >> index 797397e697..17a4d34491 100644
> >> --- a/arch/x86/include/asm/global_data.h
> >> +++ b/arch/x86/include/asm/global_data.h
> >> @@ -137,10 +137,4 @@ static inline __attribute__((no_instrument_function)) 
> >> gd_t *get_fs_gd_ptr(void)
> >>
> >>   #endif
> >>
> >> -/*
> >> - * Our private Global Data Flags
> >> - */
> >> -#define GD_FLG_COLD_BOOT   0x1 /* Cold Boot */
> >> -#define GD_FLG_WARM_BOOT   0x2 /* Warm Boot */
> >> -
> >>   #endif /* __ASM_GBL_DATA_H */
> >> diff --git a/include/asm-generic/global_data.h 
> >> b/include/asm-generic/global_data.h
> >> index 5372d5d8cd..42ae61c781 100644
> >> --- a/include/asm-generic/global_data.h
> >> +++ b/include/asm-generic/global_data.h
> >> @@ -150,7 +150,7 @@ typedef struct global_data {
> >>   #endif
> >>
> >>   /*
> >> - * Global Data Flags - the top 16 bits are reserved for arch-specific 
> >> flags
> >> + * Common Global Data Flags
> >>*/
> >>   #define GD_FLG_RELOC   0x1 /* Code was relocated to RAM  
> >>  */
> >>   #define GD_FLG_DEVINIT 0x2 /* Devices have been initialized  
> >>  */
> >> @@ -170,4 +170,8 @@ typedef struct global_data {
> >>   #define GD_FLG_LOG_READY   0x08000 /* Log system is ready for use
> >>  */
> >>   #define GD_FLG_WDT_READY   0x1 /* Watchdog is ready for use  
> >>  */
> >>
> >> +/* x86 specific GD flags */
> >> +#define GD_FLG_COLD_BOOT   0x2 /* Cold Boot */
> >> +#define GD_FLG_WARM_BOOT   0x4 /* Warm Boot */
> >
> > I think this change is generally good, but I failed to find code using the 
> > flag
> > GD_FLG_WARM_BOOT (it's only set, not checked). Only the cold-boot flag is 
> > used
> > (in coreboot's last_stage_init).
> >
> > So, since we don't have too many bits free here, instead of wasting 2 for 
> > x86,
> > could we probably remove one of these. Especially since you can either have
> > warm OR cold, but not both (so the absence of 'WARM' could mean 'COLD')?
>
> Yes. I already commented on those 2 x86 bits and their (non-)usage:
>
> https://lists.denx.de/pipermail/u-boot/2019-August/380634.html
> &
> https://lists.denx.de/pipermail/u-boot/2019-August/380800.html
>
> My suggestion is to apply this patch now and re-work the x86 flags usage
> in a later patch (most likely remove one of those bits). The main reason
> being that this double usage of 0x1 needs to get fixed quickly.

You're right. In that case:
Reviewed-by: Simon Goldschmidt 

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot


Re: [U-Boot] [PATCH] global_data: Move x86 specific GD flags into common flags

2019-08-15 Thread Stefan Roese

Hi Simon,

On 15.08.19 09:35, Simon Goldschmidt wrote:

Hi Stefan,

On Thu, Aug 15, 2019 at 9:05 AM Stefan Roese  wrote:


Currently the upper 16 bits of the GD flags are reserved for
architecture specific flags. But only x86 uses 2 bits of these 16 bits
and sprinkling those flags in multiple headers is confusing and does not
scale.

This patch now moves the x86 flags into the common header and removes
the comment about the reservation of the upper 16 bits.

Signed-off-by: Stefan Roese 
Cc: Bin Meng 
Cc: Simon Glass 
Cc: Tom Rini 
Cc: Simon Goldschmidt 
---
  arch/x86/include/asm/global_data.h | 6 --
  include/asm-generic/global_data.h  | 6 +-
  2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/global_data.h 
b/arch/x86/include/asm/global_data.h
index 797397e697..17a4d34491 100644
--- a/arch/x86/include/asm/global_data.h
+++ b/arch/x86/include/asm/global_data.h
@@ -137,10 +137,4 @@ static inline __attribute__((no_instrument_function)) gd_t 
*get_fs_gd_ptr(void)

  #endif

-/*
- * Our private Global Data Flags
- */
-#define GD_FLG_COLD_BOOT   0x1 /* Cold Boot */
-#define GD_FLG_WARM_BOOT   0x2 /* Warm Boot */
-
  #endif /* __ASM_GBL_DATA_H */
diff --git a/include/asm-generic/global_data.h 
b/include/asm-generic/global_data.h
index 5372d5d8cd..42ae61c781 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -150,7 +150,7 @@ typedef struct global_data {
  #endif

  /*
- * Global Data Flags - the top 16 bits are reserved for arch-specific flags
+ * Common Global Data Flags
   */
  #define GD_FLG_RELOC   0x1 /* Code was relocated to RAM   */
  #define GD_FLG_DEVINIT 0x2 /* Devices have been initialized   */
@@ -170,4 +170,8 @@ typedef struct global_data {
  #define GD_FLG_LOG_READY   0x08000 /* Log system is ready for use */
  #define GD_FLG_WDT_READY   0x1 /* Watchdog is ready for use   */

+/* x86 specific GD flags */
+#define GD_FLG_COLD_BOOT   0x2 /* Cold Boot */
+#define GD_FLG_WARM_BOOT   0x4 /* Warm Boot */


I think this change is generally good, but I failed to find code using the flag
GD_FLG_WARM_BOOT (it's only set, not checked). Only the cold-boot flag is used
(in coreboot's last_stage_init).

So, since we don't have too many bits free here, instead of wasting 2 for x86,
could we probably remove one of these. Especially since you can either have
warm OR cold, but not both (so the absence of 'WARM' could mean 'COLD')?


Yes. I already commented on those 2 x86 bits and their (non-)usage:

https://lists.denx.de/pipermail/u-boot/2019-August/380634.html
&
https://lists.denx.de/pipermail/u-boot/2019-August/380800.html

My suggestion is to apply this patch now and re-work the x86 flags usage
in a later patch (most likely remove one of those bits). The main reason
being that this double usage of 0x1 needs to get fixed quickly.

Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot


Re: [U-Boot] [PATCH] global_data: Move x86 specific GD flags into common flags

2019-08-15 Thread Simon Goldschmidt
Hi Stefan,

On Thu, Aug 15, 2019 at 9:05 AM Stefan Roese  wrote:
>
> Currently the upper 16 bits of the GD flags are reserved for
> architecture specific flags. But only x86 uses 2 bits of these 16 bits
> and sprinkling those flags in multiple headers is confusing and does not
> scale.
>
> This patch now moves the x86 flags into the common header and removes
> the comment about the reservation of the upper 16 bits.
>
> Signed-off-by: Stefan Roese 
> Cc: Bin Meng 
> Cc: Simon Glass 
> Cc: Tom Rini 
> Cc: Simon Goldschmidt 
> ---
>  arch/x86/include/asm/global_data.h | 6 --
>  include/asm-generic/global_data.h  | 6 +-
>  2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/include/asm/global_data.h 
> b/arch/x86/include/asm/global_data.h
> index 797397e697..17a4d34491 100644
> --- a/arch/x86/include/asm/global_data.h
> +++ b/arch/x86/include/asm/global_data.h
> @@ -137,10 +137,4 @@ static inline __attribute__((no_instrument_function)) 
> gd_t *get_fs_gd_ptr(void)
>
>  #endif
>
> -/*
> - * Our private Global Data Flags
> - */
> -#define GD_FLG_COLD_BOOT   0x1 /* Cold Boot */
> -#define GD_FLG_WARM_BOOT   0x2 /* Warm Boot */
> -
>  #endif /* __ASM_GBL_DATA_H */
> diff --git a/include/asm-generic/global_data.h 
> b/include/asm-generic/global_data.h
> index 5372d5d8cd..42ae61c781 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -150,7 +150,7 @@ typedef struct global_data {
>  #endif
>
>  /*
> - * Global Data Flags - the top 16 bits are reserved for arch-specific flags
> + * Common Global Data Flags
>   */
>  #define GD_FLG_RELOC   0x1 /* Code was relocated to RAM   */
>  #define GD_FLG_DEVINIT 0x2 /* Devices have been initialized   */
> @@ -170,4 +170,8 @@ typedef struct global_data {
>  #define GD_FLG_LOG_READY   0x08000 /* Log system is ready for use */
>  #define GD_FLG_WDT_READY   0x1 /* Watchdog is ready for use   */
>
> +/* x86 specific GD flags */
> +#define GD_FLG_COLD_BOOT   0x2 /* Cold Boot */
> +#define GD_FLG_WARM_BOOT   0x4 /* Warm Boot */

I think this change is generally good, but I failed to find code using the flag
GD_FLG_WARM_BOOT (it's only set, not checked). Only the cold-boot flag is used
(in coreboot's last_stage_init).

So, since we don't have too many bits free here, instead of wasting 2 for x86,
could we probably remove one of these. Especially since you can either have
warm OR cold, but not both (so the absence of 'WARM' could mean 'COLD')?

Regards,
Simon

> +
>  #endif /* __ASM_GENERIC_GBL_DATA_H */
> --
> 2.22.1
>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot


Re: [U-Boot] [PATCH] global_data: Move x86 specific GD flags into common flags

2019-08-15 Thread Bin Meng
On Thu, Aug 15, 2019 at 3:05 PM Stefan Roese  wrote:
>
> Currently the upper 16 bits of the GD flags are reserved for
> architecture specific flags. But only x86 uses 2 bits of these 16 bits
> and sprinkling those flags in multiple headers is confusing and does not
> scale.
>
> This patch now moves the x86 flags into the common header and removes
> the comment about the reservation of the upper 16 bits.
>
> Signed-off-by: Stefan Roese 
> Cc: Bin Meng 
> Cc: Simon Glass 
> Cc: Tom Rini 
> Cc: Simon Goldschmidt 
> ---
>  arch/x86/include/asm/global_data.h | 6 --
>  include/asm-generic/global_data.h  | 6 +-
>  2 files changed, 5 insertions(+), 7 deletions(-)
>

Reviewed-by: Bin Meng 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot