* [PATCH] powerpc: fix regression in arch_initr_trap()
@ 2021-05-19 3:16 Matt Merhar
0 siblings, 0 replies; 5+ messages in thread
From: Matt Merhar @ 2021-05-19 3:16 UTC (permalink / raw)
To: u-boot
On Tue, 18 May 2021 01:50:56 -0400
"Stefan Roese" <sr@denx.de> 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.
As Rasmus helpfully pointed out, the global_data struct has a different
layout depending on whether CONFIG_POST is defined.
In this case, CONFIG_POST is inconsistently defined depending on which
headers are included by the various bits that reference that struct.
For the P2041RDB, the define now comes in like:
common.h ->
config.h ->
configs/P2041RDB.h ->
#define CONFIG_POST CONFIG_SYS_POST_MEMORY
Would it help to clarify this in the commit message? Would it be better
to include config.h instead of common.h? I could resend the patch with
either or both changes. CONFIG_POST seems to only be defined in a
handful of headers in include/configs/.
>
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc: fix regression in arch_initr_trap()
2021-05-17 17:32 Matt Merhar
2021-05-18 5:50 ` Stefan Roese
@ 2021-05-27 11:42 ` Tom Rini
1 sibling, 0 replies; 5+ messages in thread
From: Tom Rini @ 2021-05-27 11:42 UTC (permalink / raw)
To: Matt Merhar
Cc: u-boot, Priyanka Jain, Andy Fleming, Stefan Roese, Wolfgang Denk,
Mario Six, Daniel Schwierzeck, Ovidiu Panait, Simon Glass
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] powerpc: fix regression in arch_initr_trap()
2021-05-18 5:50 ` Stefan Roese
@ 2021-05-18 7:04 ` Rasmus Villemoes
0 siblings, 0 replies; 5+ messages in thread
From: Rasmus Villemoes @ 2021-05-18 7:04 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] powerpc: fix regression in arch_initr_trap()
2021-05-17 17:32 Matt Merhar
@ 2021-05-18 5:50 ` Stefan Roese
2021-05-18 7:04 ` Rasmus Villemoes
2021-05-27 11:42 ` Tom Rini
1 sibling, 1 reply; 5+ messages in thread
From: Stefan Roese @ 2021-05-18 5:50 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] powerpc: fix regression in arch_initr_trap()
@ 2021-05-17 17:32 Matt Merhar
2021-05-18 5:50 ` Stefan Roese
2021-05-27 11:42 ` Tom Rini
0 siblings, 2 replies; 5+ messages in thread
From: Matt Merhar @ 2021-05-17 17:32 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-05-27 11:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-19 3:16 [PATCH] powerpc: fix regression in arch_initr_trap() Matt Merhar
-- strict thread matches above, loose matches on Subject: below --
2021-05-17 17:32 Matt Merhar
2021-05-18 5:50 ` Stefan Roese
2021-05-18 7:04 ` Rasmus Villemoes
2021-05-27 11:42 ` Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.