[U-Boot] Initializing global_data on SuperH before board_init_f() ?

[Thomas Petazzoni]

Hello,

On Thu, 17 Aug 2017 21:30:34 +0300, Vladimir Zapolskiy wrote:

> > As you probably noticed with the few patches I sent late July, I am
> > porting U-Boot to an old SH7786 platform.  
> 
> nice, as time passes by, more and more U-Boot/SH users appear, now
> there are two of us :)

He, yes :-)

> Are you going to upstream the board support?

No, because it's a custom, vendor-specific/internal board. But I intend
to upstream everything I can except the board specific bits.

> > As part of this effort, I stumbled across a bug: the global_data
> > structure is not initialized to zero by the SuperH architecture
> > code before calling board_init_f().  
> 
> Right, there is such a problem. Can you reconfirm that you point
> out a problem of garbage in global data _before_ relocation (from
> board_init_f() call to relocate_code() call)?

I'm not sure what you mean here. One problem I had for example was that:

static int reserve_board(void)
{
        if (!gd->bd) {

gd->bd was not NULL, so reserve_board() assumed that gd->bd was a
correct pointer... except it was pointing to garbage.

I also had a similar problem earlier in fdtdec_setup().

> > Hence, we enter board_init_f() with global_data uninitialized, which
> > have caused me quite some troubles, as I was seeing semi-random
> > behavior: in various places, we test if a pointer in global_data is
> > NULL or not to decide to do something (or not).  
> 
> It'd be good to get a list of all such cases.

See above for two examples.

> 1) board_init_f_alloc_reserve() / board_init_f_init_reserve() called
>    from start.S, the functions are too heavyweight IMHO, because it is
>    possible to avoid them, and the functions require and do unnecessary 
>    things on SH; I can share with you a (fragile) implementation, if
>    you ask,

I really think this approach is the right one. I'm not sure why you say
they do unnecessary things on SH: board_init_f_alloc_reserve() only
adjusts the "top" value, i.e 2 calculations, and
board_init_f_init_reserve() only zero-initialize it.

These two functions are used on many other architectures, and having SH
be more similar to other architectures (ARM, x86) is good for
maintenance.

Those functions are only executed once at boot time, so it's not like
we need to micro-optimize this code sequence.

> 2) introduce a SH specific startup function instead of board_init_f(),
>    do board_init_f_alloc_reserve() / board_init_f_init_reserve()
>    from it, again it is too heavy, but it's a movement from asm to C,
> 
> 3) leave a zero_global_data() hook in board_init_f() for SH, I've
>    noticed your CONFIG_SYS_GENERIC_GLOBAL_DATA removal, it makes sense,
>    and I found that on my SH4 board (IODATA Landisk which is still
>    found on second hand markets, board support is not sent to U-Boot
>    inclusion, but I can do it, if anybody) I enable the option, that's
>    why I've missed the bug reported by you,
> 
> 4) wipe pre-relocated global data space, that's the optimal change
>    among other ones, please test my implementation:
> 
> ----8<----
> diff --git a/arch/sh/lib/start.S b/arch/sh/lib/start.S
> index 37d38d5fb849..e126a39761ca 100644
> --- a/arch/sh/lib/start.S
> +++ b/arch/sh/lib/start.S
> @@ -46,6 +46,11 @@ _start:
>  	bf	3b
>  
>  	mov.l	._gd_init, r13		/* global data */
> +	mov.l	._reloc_dst, r4
> +4:	mov.l	r1, @-r4
> +	cmp/hs	r4, r13
> +	bf	4b
> +

This indeed should solve the problem, though I'm not able to test right
now, as I don't have access to the board and JTAG right now. I'll get
back to you when I've been able to test.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com