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

Stefan Roese
2015-11-26T06:23:51Z

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