[U-Boot] [PATCH v2 0/6] reboard: Introduce generic relocation feature

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

Hi Simon,

General comments here, detailed comments in reply to n/6 patches.

Le 10/12/2011 20:16, Simon Glass a ?crit :
> This is the second patch series aiming to unify the various board.c files
> in each architecture into a single one. This series implements a generic
> relocation feature, which is the bridge between board_init_f() and
> board_init_r(). It then moves ARM over to use this framework, as an
> example.
>
> On ARM the relocation code is duplicated for each CPU yet it
> is the same. We can bring this up to the arch level. But since (I believe)
> Elf relocation is basically the same process for all archs, there is no
> reason not to bring it up to the generic level.

Actually most of start.S is very similar across all its avatars in 
arch/arm, and is a good candidate for being generalized. I would prefer 
a generalization of start.S with the vector table, generic startup 
sequence prepare for calling board_init_f, jump to board_init_r with a 
possible stack switch, exception handlers) , and anything specific moved 
into the adequate subdirectory.

> Each architecture which uses this framework needs to provide a function
> called arch_elf_relocate_entry() which processes a single relocation
> entry. This is a static inline function to reduce code size overhead.

I assume this is due to some arch other than armnot using the same set 
of entry types, right? If so, I think it would be better to keep 
arch-specific entry relocation code under conditional complilation in 
the generic ELF relication source file, so that all ELF-structure 
dependent code is in a single file.

> For ARM, a new arch/arm/lib/proc.S file is created, which holds generic
> ARM assembler code (things that cannot be written in C and are common
> functions used by all ARM CPUs). This helps reduce duplication. Interrupt
> handling code and perhaps even some startup code can move there later.
>
> It may be useful for other architectures with a lot of different CPUs
> to have a similar file.

Indeed, but I think start.S is a better candidate for this, see comment 
above.

> Code size on my ARMv7 system increases by 54 bytes with generic
> relocation. This overhead is mostly just literal pool access and setting
> up to call the relocated U-Boot at the end.
>
> On my system, execution time increases from 10.8ms to 15.6ms due to the
> less efficient C implementations of the copy and zero loops. If execution
> time is of concern, you can define CONFIG_USE_ARCH_MEMSET and
> CONFIG_USE_ARCH_MEMCPY to reduce it. For met this reduces relocation time
> to 5.4ms, i.e. twice as fast as the old system.

> One problem remains which causes mx31pdk to fail to build. It doesn't
> have string.c in its SPL code and the architecture-specific versions of
> memset()/memcpy() are too large. I propose to add a local change to
> reloc.c that uses inline code for boards that use the old legacy SPL
> framework. We can remove it later. This is not included in v2 but I am
> interested in comments on this approach. An alternative would be just
> to add simple memset()/memcpy() functions just for this board (and one
> other affected MX31 board).
>
> Changes in v2:
> - Use CONFIG_SYS_SKIP_RELOC instead of CONFIG_SYS_LEGACY_BOARD
> - Import asm-generic/sections.h from Linux and add U-Boot extras
> - Squash generic link symbols patch into generic relocation patch
> - Move reloc.c into common/
> - Add function comments
> - Use memset, memcpy instead of inline code
> - Add README file for relocation
> - Invalidate I-cache when we jump to relocated code
> - Use an inline relocation function to reduce code size

Seeing as inline will only avoid pushing a couple argument registers and 
doing a branch, and OTOH may require additional register shuffling in 
the calling code, how much code does this inlining save?

> - Make relocation symbols global so we can use them outside start.S

Why should they relocation symbols ever be used outside of actually 
relocating?

Amicalement,
-- 
Albert.