Dear Wolfgang, Sorry for delayed response, I did not follow the u-boot list for few weeks an missed your e-mails somehow. Please see my comments below.
On Tue, Oct 26, 2010 at 11:09 PM, Wolfgang Denk <w...@denx.de> wrote: > 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... Thanks for catching this, I expected a feedback due to a huge number of boards and architectures the patch has touched... > ... >> #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. There were a number of boards implemented this design (alpr, karef, katmai, metrobox, ocotea, redwood, taishan, xpedite1000, yucca, etc...) I just grouped their definitions in a single place. But, you are right, it should be fixed. > But it's even worse. > 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. This code should use the CONFIG_SYS_INIT_SP_OFFSET. This should be fixed regardless of the POST_WORD location. I figured out that there a number of architectures that implement this redundancy. > > Why do we not simply reserve a word in the global data structure instead? I guess, because the global data structure is cleared in the cpu_init_f() upon startup? memset ((void *) gd, 0, sizeof (gd_t)); We can prevent part or single field of the gd structure from to be cleared. Does this make sense to you? Regards, Michael _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot