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 wd@denx.de wrote:
Dear Michael Zaidman,
In message d520c6ef298416a03789ebfa4e05e257b5331693.1284965175.git.michael.zaidman@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