[PATCH] powerpc: fix regression in arch_initr_trap()

[PATCH] powerpc: fix regression in arch_initr_trap()

[Matt Merhar]

The assembly output of the arch_initr_trap() function differed by a
single byte after common.h was removed from traps.c:

 fff49a18 <arch_initr_trap>:
 fff49a18:      94 21 ff f0     stwu    r1,-16(r1)
 fff49a1c:      7c 08 02 a6     mflr    r0
 fff49a20:      90 01 00 14     stw     r0,20(r1)
-fff49a24:      80 62 00 44     lwz     r3,68(r2)
+fff49a24:      80 62 00 38     lwz     r3,56(r2)
 fff49a28:      4b ff 76 19     bl      fff41040 <trap_init>
 fff49a2c:      80 01 00 14     lwz     r0,20(r1)
 fff49a30:      38 60 00 00     li      r3,0
 fff49a34:      38 21 00 10     addi    r1,r1,16
 fff49a38:      7c 08 03 a6     mtlr    r0

This was causing a consistent hard lockup during the MMC read / loading
of the QoriQ FMan firmware on a P2041RDB board.

Re-adding the header causes identical assembly to be emitted and allows
the firmware loading and subsequent boot to succeed.

Fixes: 401d1c4f5d ("common: Drop asm/global_data.h from common header")
Signed-off-by: Matt Merhar <mattmerhar@protonmail.com>
---

 arch/powerpc/lib/traps.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/lib/traps.c b/arch/powerpc/lib/traps.c
index c7bce82a44..ab8ca269a5 100644
--- a/arch/powerpc/lib/traps.c
+++ b/arch/powerpc/lib/traps.c
@@ -4,6 +4,7 @@
  * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
  */
 
+#include <common.h>
 #include <init.h>
 #include <asm/global_data.h>
 
-- 
2.25.1

[PATCH] powerpc: fix regression in arch_initr_trap()

[Stefan Roese]

Hi Matt,

On 17.05.21 19:32, Matt Merhar wrote:
> The assembly output of the arch_initr_trap() function differed by a
> single byte after common.h was removed from traps.c:
> 
>   fff49a18 <arch_initr_trap>:
>   fff49a18:      94 21 ff f0     stwu    r1,-16(r1)
>   fff49a1c:      7c 08 02 a6     mflr    r0
>   fff49a20:      90 01 00 14     stw     r0,20(r1)
> -fff49a24:      80 62 00 44     lwz     r3,68(r2)
> +fff49a24:      80 62 00 38     lwz     r3,56(r2)
>   fff49a28:      4b ff 76 19     bl      fff41040 <trap_init>
>   fff49a2c:      80 01 00 14     lwz     r0,20(r1)
>   fff49a30:      38 60 00 00     li      r3,0
>   fff49a34:      38 21 00 10     addi    r1,r1,16
>   fff49a38:      7c 08 03 a6     mtlr    r0
> 
> This was causing a consistent hard lockup during the MMC read / loading
> of the QoriQ FMan firmware on a P2041RDB board.
> 
> Re-adding the header causes identical assembly to be emitted and allows
> the firmware loading and subsequent boot to succeed.
> 
> Fixes: 401d1c4f5d ("common: Drop asm/global_data.h from common header")
> Signed-off-by: Matt Merhar <mattmerhar@protonmail.com>

Did you try to investigate what exactly causes this difference in the
assembly code, when the header is not included? Re-adding this header
seems like "papering over" the real problem to me.

Thanks,
Stefan

> ---
> 
>   arch/powerpc/lib/traps.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/powerpc/lib/traps.c b/arch/powerpc/lib/traps.c
> index c7bce82a44..ab8ca269a5 100644
> --- a/arch/powerpc/lib/traps.c
> +++ b/arch/powerpc/lib/traps.c
> @@ -4,6 +4,7 @@
>    * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
>    */
>   
> +#include <common.h>
>   #include <init.h>
>   #include <asm/global_data.h>
>   
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

[PATCH] powerpc: fix regression in arch_initr_trap()

[Rasmus Villemoes]

On 18/05/2021 07.50, Stefan Roese wrote:
> Hi Matt,
> 
> On 17.05.21 19:32, Matt Merhar wrote:
>> The assembly output of the arch_initr_trap() function differed by a
>> single byte after common.h was removed from traps.c:
>>
>> ? fff49a18 <arch_initr_trap>:
>> ? fff49a18:????? 94 21 ff f0???? stwu??? r1,-16(r1)
>> ? fff49a1c:????? 7c 08 02 a6???? mflr??? r0
>> ? fff49a20:????? 90 01 00 14???? stw???? r0,20(r1)
>> -fff49a24:????? 80 62 00 44???? lwz???? r3,68(r2)
>> +fff49a24:????? 80 62 00 38???? lwz???? r3,56(r2)
>> ? fff49a28:????? 4b ff 76 19???? bl????? fff41040 <trap_init>
>> ? fff49a2c:????? 80 01 00 14???? lwz???? r0,20(r1)
>> ? fff49a30:????? 38 60 00 00???? li????? r3,0
>> ? fff49a34:????? 38 21 00 10???? addi??? r1,r1,16
>> ? fff49a38:????? 7c 08 03 a6???? mtlr??? r0
>>
>> This was causing a consistent hard lockup during the MMC read / loading
>> of the QoriQ FMan firmware on a P2041RDB board.
>>
>> Re-adding the header causes identical assembly to be emitted and allows
>> the firmware loading and subsequent boot to succeed.
>>
>> Fixes: 401d1c4f5d ("common: Drop asm/global_data.h from common header")
>> Signed-off-by: Matt Merhar <mattmerhar@protonmail.com>
> 
> Did you try to investigate what exactly causes this difference in the
> assembly code, when the header is not included? Re-adding this header
> seems like "papering over" the real problem to me.

Running pahole on traps.o without and with that patch shows (I took
P2041RDB_defconfig)

struct global_data {
        struct bd_info *           bd;                   /*     0     4 */
        long unsigned int          flags;                /*     4     4 */
        unsigned int               baudrate;             /*     8     4 */
        long unsigned int          cpu_clk;              /*    12     4 */
        long unsigned int          bus_clk;              /*    16     4 */
        long unsigned int          pci_clk;              /*    20     4 */
        long unsigned int          mem_clk;              /*    24     4 */
        long unsigned int          have_console;         /*    28     4 */
        long unsigned int          env_addr;             /*    32     4 */
        long unsigned int          env_valid;            /*    36     4 */
        long unsigned int          env_has_init;         /*    40     4 */
        int                        env_load_prio;        /*    44     4 */
        long unsigned int          ram_base;             /*    48     4 */

        /* XXX 4 bytes hole, try to pack */

        phys_addr_t                ram_top;              /*    56     8 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        long unsigned int          relocaddr;            /*    64     4 */


vs

struct global_data {
        struct bd_info *           bd;                   /*     0     4 */
        long unsigned int          flags;                /*     4     4 */
        unsigned int               baudrate;             /*     8     4 */
        long unsigned int          cpu_clk;              /*    12     4 */
        long unsigned int          bus_clk;              /*    16     4 */
        long unsigned int          pci_clk;              /*    20     4 */
        long unsigned int          mem_clk;              /*    24     4 */
        long unsigned int          post_log_word;        /*    28     4 */
        long unsigned int          post_log_res;         /*    32     4 */
        long unsigned int          post_init_f_time;     /*    36     4 */
        long unsigned int          have_console;         /*    40     4 */
        long unsigned int          env_addr;             /*    44     4 */
        long unsigned int          env_valid;            /*    48     4 */
        long unsigned int          env_has_init;         /*    52     4 */
        int                        env_load_prio;        /*    56     4 */
        long unsigned int          ram_base;             /*    60     4 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        phys_addr_t                ram_top;              /*    64     8 */
        long unsigned int          relocaddr;            /*    72     4 */

so it seems to be the

#if defined(CONFIG_POST)

in include/asm-generic/global_data.h which is broken or unreliable or
whatever the appropriate way to describe it is.

It would be nice if the linker could catch that kind of thing when
collecting debug info from various TUs.

Rasmus

[RFC PATCH 0/2] sizeof(gd_t) sanity checking

[Rasmus Villemoes]

I haven't done a whole lot of testing of these, just enough to see
that it normally works _and_ that it would catch the bug Matt
reported. I don't have time or resources to find and fix the bugs this
would reveal, hence just an RFC for people to consider.

Rasmus Villemoes (2):
  build_bug.h: add wrapper for _Static_assert
  global-data.h: add build-time sanity check of sizeof(struct
    global_data)

 include/asm-generic/global_data.h |  5 +++++
 include/linux/build_bug.h         | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+)

-- 
2.29.2

[RFC PATCH 1/2] build_bug.h: add wrapper for _Static_assert

[Rasmus Villemoes]

[Linux commit 6bab69c65013bed5fce9f101a64a84d0385b3946]

BUILD_BUG_ON() is a little annoying, since it cannot be used outside
function scope.  So one cannot put assertions about the sizeof() a
struct next to the struct definition, but has to hide that in some more
or less arbitrary function.

Since gcc 4.6 (which is now also the required minimum), there is support
for the C11 _Static_assert in all C modes, including gnu89.  So add a
simple wrapper for that.

_Static_assert() requires a message argument, which is usually quite
redundant (and I believe that bug got fixed at least in newer C++
standards), but we can easily work around that with a little macro
magic, making it optional.

For example, adding

  static_assert(sizeof(struct printf_spec) == 8);

in vsprintf.c and modifying that struct to violate it, one gets

./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct printf_spec) == 8"
 #define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")

godbolt.org suggests that _Static_assert() has been support by clang
since at least 3.0.0.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 include/linux/build_bug.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
index b7d22d6000..9c7088bafa 100644
--- a/include/linux/build_bug.h
+++ b/include/linux/build_bug.h
@@ -79,6 +79,25 @@
  */
 #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
 
+/**
+ * static_assert - check integer constant expression at build time
+ *
+ * static_assert() is a wrapper for the C11 _Static_assert, with a
+ * little macro magic to make the message optional (defaulting to the
+ * stringification of the tested expression).
+ *
+ * Contrary to BUILD_BUG_ON(), static_assert() can be used at global
+ * scope, but requires the expression to be an integer constant
+ * expression (i.e., it is not enough that __builtin_constant_p() is
+ * true for expr).
+ *
+ * Also note that BUILD_BUG_ON() fails the build if the condition is
+ * true, while static_assert() fails the build if the expression is
+ * false.
+ */
+#define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
+#define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
+
 #endif	/* __CHECKER__ */
 
 #endif	/* _LINUX_BUILD_BUG_H */
-- 
2.29.2

[RFC PATCH 1/2] build_bug.h: add wrapper for _Static_assert

[Simon Glass]

On Tue, 18 May 2021 at 03:19, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> [Linux commit 6bab69c65013bed5fce9f101a64a84d0385b3946]
>
> BUILD_BUG_ON() is a little annoying, since it cannot be used outside
> function scope.  So one cannot put assertions about the sizeof() a
> struct next to the struct definition, but has to hide that in some more
> or less arbitrary function.
>
> Since gcc 4.6 (which is now also the required minimum), there is support
> for the C11 _Static_assert in all C modes, including gnu89.  So add a
> simple wrapper for that.
>
> _Static_assert() requires a message argument, which is usually quite
> redundant (and I believe that bug got fixed at least in newer C++
> standards), but we can easily work around that with a little macro
> magic, making it optional.
>
> For example, adding
>
>   static_assert(sizeof(struct printf_spec) == 8);
>
> in vsprintf.c and modifying that struct to violate it, one gets
>
> ./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct printf_spec) == 8"
>  #define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
>
> godbolt.org suggests that _Static_assert() has been support by clang
> since at least 3.0.0.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  include/linux/build_bug.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [RFC PATCH 1/2] build_bug.h: add wrapper for _Static_assert

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

On Tue, May 18, 2021 at 11:19:46AM +0200, Rasmus Villemoes wrote:

> [Linux commit 6bab69c65013bed5fce9f101a64a84d0385b3946]
> 
> BUILD_BUG_ON() is a little annoying, since it cannot be used outside
> function scope.  So one cannot put assertions about the sizeof() a
> struct next to the struct definition, but has to hide that in some more
> or less arbitrary function.
> 
> Since gcc 4.6 (which is now also the required minimum), there is support
> for the C11 _Static_assert in all C modes, including gnu89.  So add a
> simple wrapper for that.
> 
> _Static_assert() requires a message argument, which is usually quite
> redundant (and I believe that bug got fixed at least in newer C++
> standards), but we can easily work around that with a little macro
> magic, making it optional.
> 
> For example, adding
> 
>   static_assert(sizeof(struct printf_spec) == 8);
> 
> in vsprintf.c and modifying that struct to violate it, one gets
> 
> ./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct printf_spec) == 8"
>  #define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
> 
> godbolt.org suggests that _Static_assert() has been support by clang
> since at least 3.0.0.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[RFC PATCH 2/2] global-data.h: add build-time sanity check of sizeof(struct global_data)

[Rasmus Villemoes]

The layout and contents of struct global_data depends on a lot of
CONFIG_* preprocessor macros, not all of which are entirely converted
to Kconfig - not to mention weird games played here and there. This
can result in one translation unit using one definition of struct
global_data while the actual layout is another.

That can be very hard to debug. But we already have a mechanism that
can help catch such bugs at build time, namely the asm-offsets
machinery which is necessary anyway to provide assembly code with the
necessary constants. So make sure that every C translation unit that
include global_data.h actually sees the same size of struct
global_data as that which was seen by the asm-offsets.c TU.

It is likely that this patch will break the build of some boards. For
example, without the patch from Matt Merhar
(https://lists.denx.de/pipermail/u-boot/2021-May/450135.html) or some
other fix, this breaks P2041RDB_defconfig:

  CC      arch/powerpc/lib/traps.o
  AS      arch/powerpc/cpu/mpc85xx/start.o
In file included from include/asm-generic/global_data.h:26,
                 from ./arch/powerpc/include/asm/global_data.h:109,
                 from include/init.h:21,
                 from arch/powerpc/lib/traps.c:7:
include/linux/build_bug.h:99:41: error: static assertion failed: "sizeof(struct global_data) == GD_SIZE"
   99 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
      |                                         ^~~~~~~~~~~~~~
include/linux/build_bug.h:98:34: note: in expansion of macro ?__static_assert?
   98 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
      |                                  ^~~~~~~~~~~~~~~
include/asm-generic/global_data.h:470:1: note: in expansion of macro ?static_assert?
  470 | static_assert(sizeof(struct global_data) == GD_SIZE);
      | ^~~~~~~~~~~~~
make[1]: *** [scripts/Makefile.build:266: arch/powerpc/lib/traps.o] Error 1
make: *** [Makefile:1753: arch/powerpc/lib] Error 2
make: *** Waiting for unfinished jobs....

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 include/asm-generic/global_data.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index 47921d27b1..1233ec0ed6 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -23,6 +23,8 @@
 #include <fdtdec.h>
 #include <membuff.h>
 #include <linux/list.h>
+#include <linux/build_bug.h>
+#include <asm-offsets.h>
 
 struct acpi_ctx;
 struct driver_rt;
@@ -464,6 +466,9 @@ struct global_data {
 	char *smbios_version;
 #endif
 };
+#ifndef DO_DEPS_ONLY
+static_assert(sizeof(struct global_data) == GD_SIZE);
+#endif
 
 /**
  * gd_board_type() - retrieve board type
-- 
2.29.2

[RFC PATCH 2/2] global-data.h: add build-time sanity check of sizeof(struct global_data)

[Simon Glass]

On Tue, 18 May 2021 at 03:20, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> The layout and contents of struct global_data depends on a lot of
> CONFIG_* preprocessor macros, not all of which are entirely converted
> to Kconfig - not to mention weird games played here and there. This
> can result in one translation unit using one definition of struct
> global_data while the actual layout is another.
>
> That can be very hard to debug. But we already have a mechanism that
> can help catch such bugs at build time, namely the asm-offsets
> machinery which is necessary anyway to provide assembly code with the
> necessary constants. So make sure that every C translation unit that
> include global_data.h actually sees the same size of struct
> global_data as that which was seen by the asm-offsets.c TU.
>
> It is likely that this patch will break the build of some boards. For
> example, without the patch from Matt Merhar
> (https://lists.denx.de/pipermail/u-boot/2021-May/450135.html) or some
> other fix, this breaks P2041RDB_defconfig:
>
>   CC      arch/powerpc/lib/traps.o
>   AS      arch/powerpc/cpu/mpc85xx/start.o
> In file included from include/asm-generic/global_data.h:26,
>                  from ./arch/powerpc/include/asm/global_data.h:109,
>                  from include/init.h:21,
>                  from arch/powerpc/lib/traps.c:7:
> include/linux/build_bug.h:99:41: error: static assertion failed: "sizeof(struct global_data) == GD_SIZE"
>    99 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
>       |                                         ^~~~~~~~~~~~~~
> include/linux/build_bug.h:98:34: note: in expansion of macro ?__static_assert?
>    98 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
>       |                                  ^~~~~~~~~~~~~~~
> include/asm-generic/global_data.h:470:1: note: in expansion of macro ?static_assert?
>   470 | static_assert(sizeof(struct global_data) == GD_SIZE);
>       | ^~~~~~~~~~~~~
> make[1]: *** [scripts/Makefile.build:266: arch/powerpc/lib/traps.o] Error 1
> make: *** [Makefile:1753: arch/powerpc/lib] Error 2
> make: *** Waiting for unfinished jobs....
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  include/asm-generic/global_data.h | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [RFC PATCH 2/2] global-data.h: add build-time sanity check of sizeof(struct global_data)

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]

On Tue, May 18, 2021 at 11:19:47AM +0200, Rasmus Villemoes wrote:

> The layout and contents of struct global_data depends on a lot of
> CONFIG_* preprocessor macros, not all of which are entirely converted
> to Kconfig - not to mention weird games played here and there. This
> can result in one translation unit using one definition of struct
> global_data while the actual layout is another.
> 
> That can be very hard to debug. But we already have a mechanism that
> can help catch such bugs at build time, namely the asm-offsets
> machinery which is necessary anyway to provide assembly code with the
> necessary constants. So make sure that every C translation unit that
> include global_data.h actually sees the same size of struct
> global_data as that which was seen by the asm-offsets.c TU.
> 
> It is likely that this patch will break the build of some boards. For
> example, without the patch from Matt Merhar
> (https://lists.denx.de/pipermail/u-boot/2021-May/450135.html) or some
> other fix, this breaks P2041RDB_defconfig:
> 
>   CC      arch/powerpc/lib/traps.o
>   AS      arch/powerpc/cpu/mpc85xx/start.o
> In file included from include/asm-generic/global_data.h:26,
>                  from ./arch/powerpc/include/asm/global_data.h:109,
>                  from include/init.h:21,
>                  from arch/powerpc/lib/traps.c:7:
> include/linux/build_bug.h:99:41: error: static assertion failed: "sizeof(struct global_data) == GD_SIZE"
>    99 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
>       |                                         ^~~~~~~~~~~~~~
> include/linux/build_bug.h:98:34: note: in expansion of macro ‘__static_assert’
>    98 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
>       |                                  ^~~~~~~~~~~~~~~
> include/asm-generic/global_data.h:470:1: note: in expansion of macro ‘static_assert’
>   470 | static_assert(sizeof(struct global_data) == GD_SIZE);
>       | ^~~~~~~~~~~~~
> make[1]: *** [scripts/Makefile.build:266: arch/powerpc/lib/traps.o] Error 1
> make: *** [Makefile:1753: arch/powerpc/lib] Error 2
> make: *** Waiting for unfinished jobs....
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 1/4] powerpc: Don't use relative include for config.h in global_data.h

[Tom Rini]

As there is an arch/powerpc/include/asm/config.h file using "" to get
config.h here can lead to using that rather than include/config.h.  This
in turn can lead to a mismatch in the size of gd.

Cc: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
 arch/powerpc/include/asm/global_data.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/global_data.h b/arch/powerpc/include/asm/global_data.h
index 192a02d347e7..90bf5a2aea5d 100644
--- a/arch/powerpc/include/asm/global_data.h
+++ b/arch/powerpc/include/asm/global_data.h
@@ -8,7 +8,7 @@
 #ifndef	__ASM_GBL_DATA_H
 #define __ASM_GBL_DATA_H
 
-#include "config.h"
+#include <config.h>
 #include "asm/types.h"
 
 /* Architecture-specific global data */
-- 
2.17.1

[PATCH 2/4] Revert "powerpc: fix regression in arch_initr_trap()"

[Tom Rini]

With the changes in commit 588efcdd72fc ("powerpc: Don't use relative
include for config.h in global_data.h") fixing the root of the problem,
we no longer need this re-inclusion.

This reverts commit f6c0d365d3e8ee8e4fd3ebe2ed957c2bca9d3328.

Cc: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
I will need to reword this to have the right commit ID when merging.
---
 arch/powerpc/lib/traps.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/lib/traps.c b/arch/powerpc/lib/traps.c
index ab8ca269a50c..c7bce82a44b3 100644
--- a/arch/powerpc/lib/traps.c
+++ b/arch/powerpc/lib/traps.c
@@ -4,7 +4,6 @@
  * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
  */
 
-#include <common.h>
 #include <init.h>
 #include <asm/global_data.h>
 
-- 
2.17.1

Re: [PATCH 2/4] Revert "powerpc: fix regression in arch_initr_trap()"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

On Thu, Jun 03, 2021 at 09:39:00AM -0400, Tom Rini wrote:

> With the changes in commit 588efcdd72fc ("powerpc: Don't use relative
> include for config.h in global_data.h") fixing the root of the problem,
> we no longer need this re-inclusion.
> 
> This reverts commit f6c0d365d3e8ee8e4fd3ebe2ed957c2bca9d3328.
> 
> Cc: Matt Merhar <mattmerhar@protonmail.com>
> Signed-off-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 3/4] socfpga64: Do not define CONFIG_SYS_MEM_RESERVE_SECURE to 0

[Tom Rini]

Based on the comment in socfpga_soc64_common.h, the intention is for
CONFIG_SYS_MEM_RESERVE_SECURE to be unused.  However, in the code we do:
...

and that will evaluate to true.  This leads to unwanted code being
compiled.  Further, as CONFIG_SYS_MEM_RESERVE_SECURE has not been
migrated to Kconfig, this leads to a mismatch in the size of gd
depending on if we have or have not also had <configs/BOARD.h> also
included yet.

Remove the define as it's not needed.

Cc: Siew Chin Lim <elly.siew.chin.lim@intel.com>
Cc: Chee Hong Ang <chee.hong.ang@intel.com>
Cc: Dalon Westergreen <dalon.westergreen@intel.com>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
 include/configs/socfpga_soc64_common.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/configs/socfpga_soc64_common.h b/include/configs/socfpga_soc64_common.h
index 5afdb104543e..38fd775b5b6e 100644
--- a/include/configs/socfpga_soc64_common.h
+++ b/include/configs/socfpga_soc64_common.h
@@ -21,7 +21,6 @@
 /* sysmgr.boot_scratch_cold4 & 5 (64bit) will be used for PSCI_CPU_ON call */
 #define CPU_RELEASE_ADDR		0xFFD12210
 #define CONFIG_SYS_CACHELINE_SIZE	64
-#define CONFIG_SYS_MEM_RESERVE_SECURE	0	/* using OCRAM, not DDR */
 
 /*
  * U-Boot console configurations
-- 
2.17.1

Re: [PATCH 3/4] socfpga64: Do not define CONFIG_SYS_MEM_RESERVE_SECURE to 0

[Rasmus Villemoes]

On 03/06/2021 15.39, Tom Rini wrote:
> Based on the comment in socfpga_soc64_common.h, the intention is for
> CONFIG_SYS_MEM_RESERVE_SECURE to be unused.  However, in the code we do:
> ...
> 
> and that will evaluate to true.  This leads to unwanted code being

Some cleanup made lines beginning with # disappear? I can _strongly_
recommend setting commit.cleanup to scissors; I have lost count of the
number of times I've had a commit message mangled and become practically
useless because important lines got removed.

For example
<http://lists.busybox.net/pipermail/busybox/2018-September/086647.html>
became
<https://git.busybox.net/busybox/commit/?id=571e525a141a2de87b9c2ced485745e96418d921>

Rasmus

Re: [PATCH 3/4] socfpga64: Do not define CONFIG_SYS_MEM_RESERVE_SECURE to 0

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]

On Thu, Jun 03, 2021 at 05:07:17PM +0200, Rasmus Villemoes wrote:
> On 03/06/2021 15.39, Tom Rini wrote:
> > Based on the comment in socfpga_soc64_common.h, the intention is for
> > CONFIG_SYS_MEM_RESERVE_SECURE to be unused.  However, in the code we do:
> > ...
> > 
> > and that will evaluate to true.  This leads to unwanted code being
> 
> Some cleanup made lines beginning with # disappear? I can _strongly_
> recommend setting commit.cleanup to scissors; I have lost count of the
> number of times I've had a commit message mangled and become practically
> useless because important lines got removed.
> 
> For example
> <http://lists.busybox.net/pipermail/busybox/2018-September/086647.html>
> became
> <https://git.busybox.net/busybox/commit/?id=571e525a141a2de87b9c2ced485745e96418d921>

Bah!  Yes, thanks.  It should have said
#ifdef CONFIG_SYS_MEM_RESERVE_SECURE
...
#endif

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/4] socfpga64: Do not define CONFIG_SYS_MEM_RESERVE_SECURE to 0

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 814 bytes --]

On Thu, Jun 03, 2021 at 09:39:01AM -0400, Tom Rini wrote:

> Based on the comment in socfpga_soc64_common.h, the intention is for
> CONFIG_SYS_MEM_RESERVE_SECURE to be unused.  However, in the code we do:
> ...
> 
> and that will evaluate to true.  This leads to unwanted code being
> compiled.  Further, as CONFIG_SYS_MEM_RESERVE_SECURE has not been
> migrated to Kconfig, this leads to a mismatch in the size of gd
> depending on if we have or have not also had <configs/BOARD.h> also
> included yet.
> 
> Remove the define as it's not needed.
> 
> Cc: Siew Chin Lim <elly.siew.chin.lim@intel.com>
> Cc: Chee Hong Ang <chee.hong.ang@intel.com>
> Cc: Dalon Westergreen <dalon.westergreen@intel.com>
> Signed-off-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 4/4] global_data: Ensure we have <config.h> when symbols are not in Kconfig yet

[Tom Rini]

All symbols that are defined in Kconfig will always be defined (or not)
prior to preprocessing due to the -include directive while building.
However, symbols which are not yet migrated will only be defined (or
not) once the board config.h is included, via <config.h>.  While the end
goal must be to migrate all symbols, today we have cases where the size
of gd will get mismatched within the build, based on include order.
Mitigate this by making sure that any <asm/global_data.h> that uses
symbols not in Kconfig does start with <config.h>.  Remove this when not
needed.

Cc: Alexey Brodkin <alexey.brodkin@synopsys.com>
Cc: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
Cc: Huan Wang <alison.wang@nxp.com>
Cc: Angelo Dureghello <angelo@sysam.it>
Cc: Rick Chen <rick@andestech.com>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
 arch/arc/include/asm/global_data.h   | 2 --
 arch/arm/include/asm/global_data.h   | 2 ++
 arch/m68k/include/asm/global_data.h  | 2 ++
 arch/nds32/include/asm/global_data.h | 2 ++
 4 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arc/include/asm/global_data.h b/arch/arc/include/asm/global_data.h
index 8f9c83d3c28d..e35a26f1eb14 100644
--- a/arch/arc/include/asm/global_data.h
+++ b/arch/arc/include/asm/global_data.h
@@ -6,8 +6,6 @@
 #ifndef	__ASM_ARC_GLOBAL_DATA_H
 #define __ASM_ARC_GLOBAL_DATA_H
 
-#include <config.h>
-
 #ifndef __ASSEMBLY__
 /* Architecture-specific global data */
 struct arch_global_data {
diff --git a/arch/arm/include/asm/global_data.h b/arch/arm/include/asm/global_data.h
index 2aff1c467c14..79432f3bbd24 100644
--- a/arch/arm/include/asm/global_data.h
+++ b/arch/arm/include/asm/global_data.h
@@ -9,6 +9,8 @@
 
 #ifndef __ASSEMBLY__
 
+#include <config.h>
+
 #include <asm/types.h>
 #include <linux/types.h>
 
diff --git a/arch/m68k/include/asm/global_data.h b/arch/m68k/include/asm/global_data.h
index 188055e9d314..273e843c4ae6 100644
--- a/arch/m68k/include/asm/global_data.h
+++ b/arch/m68k/include/asm/global_data.h
@@ -7,6 +7,8 @@
 #ifndef	__ASM_GBL_DATA_H
 #define __ASM_GBL_DATA_H
 
+#include <config.h>
+
 /* Architecture-specific global data */
 struct arch_global_data {
 #ifdef CONFIG_SYS_I2C_FSL
diff --git a/arch/nds32/include/asm/global_data.h b/arch/nds32/include/asm/global_data.h
index be04a18857a1..297481beaaef 100644
--- a/arch/nds32/include/asm/global_data.h
+++ b/arch/nds32/include/asm/global_data.h
@@ -17,6 +17,8 @@
 #ifndef	__ASM_GBL_DATA_H
 #define __ASM_GBL_DATA_H
 
+#include <config.h>
+
 /* Architecture-specific global data */
 struct arch_global_data {
 };
-- 
2.17.1

Re: [PATCH 4/4] global_data: Ensure we have <config.h> when symbols are not in Kconfig yet

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 987 bytes --]

On Thu, Jun 03, 2021 at 09:39:02AM -0400, Tom Rini wrote:

> All symbols that are defined in Kconfig will always be defined (or not)
> prior to preprocessing due to the -include directive while building.
> However, symbols which are not yet migrated will only be defined (or
> not) once the board config.h is included, via <config.h>.  While the end
> goal must be to migrate all symbols, today we have cases where the size
> of gd will get mismatched within the build, based on include order.
> Mitigate this by making sure that any <asm/global_data.h> that uses
> symbols not in Kconfig does start with <config.h>.  Remove this when not
> needed.
> 
> Cc: Alexey Brodkin <alexey.brodkin@synopsys.com>
> Cc: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> Cc: Huan Wang <alison.wang@nxp.com>
> Cc: Angelo Dureghello <angelo@sysam.it>
> Cc: Rick Chen <rick@andestech.com>
> Signed-off-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/4] powerpc: Don't use relative include for config.h in global_data.h

[Matt Merhar]

On Thu, 03 Jun 2021 09:38:59 -0400
"Tom Rini" <trini@konsulko.com> wrote:

> As there is an arch/powerpc/include/asm/config.h file using "" to get
> config.h here can lead to using that rather than include/config.h.
> This in turn can lead to a mismatch in the size of gd.
> 
> Cc: Matt Merhar <mattmerhar@protonmail.com>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
>  arch/powerpc/include/asm/global_data.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/global_data.h
> b/arch/powerpc/include/asm/global_data.h index
> 192a02d347e7..90bf5a2aea5d 100644 ---
> a/arch/powerpc/include/asm/global_data.h +++
> b/arch/powerpc/include/asm/global_data.h @@ -8,7 +8,7 @@
>  #ifndef	__ASM_GBL_DATA_H
>  #define __ASM_GBL_DATA_H
> 
> -#include "config.h"
> +#include <config.h>
>  #include "asm/types.h"
> 
>  /* Architecture-specific global data */
> --
> 2.17.1
> 

I think this is a better solution, as it also appears to fix another
global_data size mismatch caught by the RFC from Rasmus "global-data.h:
add build-time sanity check of sizeof(struct global_data)".

Tested-by: Matt Merhar <mattmerhar@protonmail.com>

Re: [PATCH 1/4] powerpc: Don't use relative include for config.h in global_data.h

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]

On Thu, Jun 03, 2021 at 09:38:59AM -0400, Tom Rini wrote:

> As there is an arch/powerpc/include/asm/config.h file using "" to get
> config.h here can lead to using that rather than include/config.h.  This
> in turn can lead to a mismatch in the size of gd.
> 
> Cc: Matt Merhar <mattmerhar@protonmail.com>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> Tested-by: Matt Merhar <mattmerhar@protonmail.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] powerpc: fix regression in arch_initr_trap()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]

On Mon, May 17, 2021 at 05:32:48PM +0000, Matt Merhar wrote:

> The assembly output of the arch_initr_trap() function differed by a
> single byte after common.h was removed from traps.c:
> 
>  fff49a18 <arch_initr_trap>:
>  fff49a18:      94 21 ff f0     stwu    r1,-16(r1)
>  fff49a1c:      7c 08 02 a6     mflr    r0
>  fff49a20:      90 01 00 14     stw     r0,20(r1)
> -fff49a24:      80 62 00 44     lwz     r3,68(r2)
> +fff49a24:      80 62 00 38     lwz     r3,56(r2)
>  fff49a28:      4b ff 76 19     bl      fff41040 <trap_init>
>  fff49a2c:      80 01 00 14     lwz     r0,20(r1)
>  fff49a30:      38 60 00 00     li      r3,0
>  fff49a34:      38 21 00 10     addi    r1,r1,16
>  fff49a38:      7c 08 03 a6     mtlr    r0
> 
> This was causing a consistent hard lockup during the MMC read / loading
> of the QoriQ FMan firmware on a P2041RDB board.
> 
> Re-adding the header causes identical assembly to be emitted and allows
> the firmware loading and subsequent boot to succeed.
> 
> Fixes: 401d1c4f5d ("common: Drop asm/global_data.h from common header")
> Signed-off-by: Matt Merhar <mattmerhar@protonmail.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]