[U-Boot] [PATCH V7] POST cleanup.

[Michael Zaidman]

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