[U-Boot] [RFC] Removal of superfluous gd assignments

[Stefan Roese]

Hi Albert,

first, thank you very much for taking the time to go over this
in such depth.

On 25.11.2015 16:20, Albert ARIBAUD wrote:
> Hello all,
>
> This is a follow-up to discussions on the IRC chan re: the fact that gd
> (the global data pointer) is being assigned in various places, which is
> not too much of a good thing.
>
> In all architectures, common/init/board_init.c is built in, which
> provides board_init_f_{mem,init_reserve}, in which gd is assigned by
> calling arch_setup_gd().
>
> (two architectures, ARM and x86, cannot assign gd from C code, so they
> don't call arch_setup_gd() but assign GD from another single location)
>
> However, there are several places apart from arch_setup_gd() where gd is
> assigned to. Superfluous assignments lead, at best, to confusion, and
> at worst, to subtle as well as not-so-subtle bugs, so we need to find
> and remove such superfluous assignments.
>
> This post gives a list of places where gd is being assigned (courtesy
> of a git grep '^\s\+gd\s*=\s*.*$' command) and suggestions re removal.
>
> Comments welcome, of course.
>
> -----------------------------------
>
> arch/arm/cpu/arm926ejs/mxs/spl_boot.c:126:      gd = &gdata;
>
> 	This assignment is in mxs_common_spl_init() which is called
> 	from a board_init_ll() defined by various boards, and itself is
> 	called from arch/arm/cpu/arm926ejs/mxs/start.S. Setting gs is
> 	required because mxs_common_spl_init() calls lots of functions
> 	which depend on gd.
>
> 	Removal: requires MXS SPL to move over to using crt0.S.
>
> 	Note: since MXS SPL actually returns to ROM to get U-Boot
> 	launched, it may require cooperation from crt0.S for saving
> 	important registers at reset entry point and restoring them and
> 	returning to ROM. See OMAP (for register saving) and FEL.
>
> 	Boards affected are:
>
> 	mx28evk_nand xfi3 bg0900 sansa_fuze_plus mx23evk m28evk
> 	sc_sps_1 mx28evk_spi axm mx28evk apx4devkit mx23_olinuxino
> 	firefly-rk3288 x600 mx28evk_auart_console taurus

This list of affected boards does not seem to be correct. I fail to
see, how firefly-rk3288 or x600 are affected by this "mxs" file.

> arch/arm/cpu/armv8/fsl-layerscape/spl.c:48:     gd = &gdata;
>
> 	Performed in board_init_f() by boards:
>
> 	ls1043ardb_nand ls2085ardb_nand ls1043ardb_sdcard
> 	ls2085aqds_nand
>
> 	These four boards build SPL using arch/arm/lib/crt0_64.S which
> 	unconditionally executes arch_set_gd() and thereby has gd
> 	properly set when arch_set_ board_init_f() is entered, so that
> 	assignment is redundant.
>
> 	Removal: unconditional.
>
> arch/arm/lib/spl.c:40:  gd = &gdata;
>
> 	This assignment is in a weak board_init_f(), therefore executed
> 	from crt0.S (no ARM board calls board_init_f() from elsewhere);
> 	therefore gd is already set when this assignment is reached.
>
> 	Removal: unconditional.

I'm wondering, if this weak default board_init_f() function is
called for any ARM platform at all? If not, it would be clearer
to remove this function completely.

> arch/arm/mach-exynos/spl_boot.c:279:    gd = gdp;
>
> 	This assignment occurs from withing a machine-specific
> 	board_init_f(), therefore executed from crt0.S (no ARM board
> 	calls board_init_f() from elsewhere); therefore gd is already
> 	set when this assignment is reached.
>
> 	Removal: unconditional.
>
> (non-ARM architectures skipped over)
>
> (non-ARM boards skipped over)
>
> common/board_f.c:982:   gd = &data;
>
> 	Only m68k, avr32 and nds32 use this. It could be conditioned to
> 	these architectures and later removed if and when they switch
> 	to the same boot sequence as ARM et al.
>
> 	Removal: only if and when arch switches to a boot sequence
> 	similar to ARM's.
>
> common/board_r.c:942:   gd = new_gd;
>
> 	Done by arches other than ARM, AARCH64 and X86. For ARM, gd is
> 	switched from early to final location in crt0. For x86? In any
> 	case, this assignment cannot be removed unless all arches have
> 	switched to a boot sequence similar to ARM's.
>
> 	Removal: only if and when arch switches to a boot sequence
> 	similar to ARM's.
>
> common/init/board_init.c:28:    gd = gd_ptr;
>
> 	That is the only assignment we should keep, for architectures
> 	other than ARM or x86.
>
> 	Removal: no.
>
> common/spl/spl.c:450:   gd = new_gd;
>
> 	This is the second place where gd assignment should be kept,
> 	and for ARM it must be fixed the same way arch_setup_gd was,
> 	since this assignment to gd may not work with ARM gcc (this
> 	change will go in v8 of my patch)
>
> 	Removal: no.
>
> Amicalement,

Thanks,
Stefan