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

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

[Albert ARIBAUD]

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

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.

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,
-- 
Albert.

[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

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

[Albert ARIBAUD]

Hello Stefan,

On Thu, 26 Nov 2015 07:23:51 +0100, Stefan Roese
<stefan.roese@gmail.com> wrote:
> 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.

Correct -- I'd patched arch/arm/cpu/arm926ejs/mxs/spl_boot.c with
a #error directive then blindly listed all boards given by buildman -s
which included boards not building cleanly even for other reasons that
the one I was looking for.

> > 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.

I'll try and find out if the weak board_init_f is used at all, and if
it is not, I'll amend the suggestion to 'removal of the whole function'

> Thanks,

Thanks for your review!

> Stefan

Amicalement,
-- 
Albert.