[PATCH] powerpc: fix regression in arch_initr_trap()

[Matt Merhar]

On Tue, 18 May 2021 01:50:56 -0400
"Stefan Roese" <sr at 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 at 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