Dear Michael Zaidman, In message <d520c6ef298416a03789ebfa4e05e257b5331693.1284965175.git.michael.zaid...@gmail.com> you wrote: > - Revives POST for blackfin arch; > - Removes redundant code: > arch/blackfin/lib/post.c > arch/powerpc/cpu/ppc4xx/commproc.c > arch/powerpc/cpu/mpc512x/common.c > - fixes up the post_word_{load|store} usage.
Unfortunately it turns out that the code now contains a few nasty bugs... ... > #define CONFIG_SYS_GBL_DATA_OFFSET (CONFIG_SYS_INIT_RAM_END - > CONFIG_SYS_GBL_DATA_SIZE) > -#define CONFIG_SYS_POST_WORD_ADDR (CONFIG_SYS_GBL_DATA_OFFSET - 0x4) > -#define CONFIG_SYS_INIT_SP_OFFSET CONFIG_SYS_POST_WORD_ADDR > +#define CONFIG_SYS_INIT_SP_OFFSET (CONFIG_SYS_GBL_DATA_OFFSET - 0x4) This is a seriously broken design, as it sneaks in storage for a variable in a storage location where it is not expected. The "official" layout is that we have CONFIG_SYS_INIT_RAM_BYTES available; the top CONFIG_SYS_GBL_DATA_SIZE bytes (now GENERATED_GBL_DATA_SIZE) are used for global data, and the part below is used for the stack. No other room is reserved there. Shifting down the stack by 4 bytes as it's done here causes that the stack is not correctly aligned any more, which may cause really nasty subsequent errors. But it's even worse. > diff --git a/include/configs/mpc5121-common.h > b/include/configs/mpc5121-common.h > index 96fab20..afae1ab 100644 > --- a/include/configs/mpc5121-common.h > +++ b/include/configs/mpc5121-common.h ... > -#define CONFIG_SYS_POST_WORD_ADDR (CONFIG_SYS_GBL_DATA_OFFSET - 0x4) > -#define CONFIG_SYS_INIT_SP_OFFSET CONFIG_SYS_POST_WORD_ADDR > +#define CONFIG_SYS_INIT_SP_OFFSET (CONFIG_SYS_GBL_DATA_OFFSET - 0x4) There the same is done, but what happens actually? Have a look how the stack setup gets implemented in "arch/powerpc/cpu/mpc512x/start.S": ... 244 in_flash: 245 lis r1, (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET)@h 246 ori r1, r1, (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET)@l 247 248 li r0, 0 /* Make room for stack frame header and */ 249 stwu r0, -4(r1) /* clear final stack frame so that */ 250 stwu r0, -4(r1) /* stack backtraces terminate cleanly */ ... As you can see, the code does not use CONFIG_SYS_INIT_SP_OFFSET at all; instead it performs a calculation which should be redundant, but in the current code it means that the location of the POST_WORD is right in the initial stack. I did not check if the code for other processors has similar issues. "Reserving" private storage like that is bad, as other involved parties probably have no knowledge of such a private reservation. Why do we not simply reserve a word in the global data structure instead? This bug needs pretty urgent fixing. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de I wish Captain Vimes were here. He wouldn't have known what to do either, but he's got a much better vocabulary to be baffled in. - Terry Pratchett, _Guards! Guards!_ _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot