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