From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Thu, 26 Nov 2015 07:23:51 +0100 Subject: [U-Boot] [RFC] Removal of superfluous gd assignments In-Reply-To: <20151125162050.1802c264@lilith> References: <20151125162050.1802c264@lilith> Message-ID: <5656A577.8060400@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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