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

[Simon Glass]

Hi Albert,

On Sun, Dec 11, 2011 at 6:47 AM, Albert ARIBAUD
<albert.u.boot at aribaud.net> wrote:
> Hi Simon,
>
> General comments here, detailed comments in reply to n/6 patches.

Thanks for looking at this.

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

Yes I agree. However this is not actually the purpose of this series,
which is to head (so far as is possible) towards a generic board.c
file for all architectures. While I completely agree that start.S is a
mess, that is a separate problem!

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

Yes that takes things one step further. I did look at it but could not
find an elf.h with defines for all the different relocations. Do you
know where to find that?

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

My understanding of start.S is that it is the start-up code for the
machine, and that over time we would hope to reduce it to a small
amount of code which is specific to that particular cpu (i.e. the
shared and common code moves out).

We can do this in many small steps - certainly I don't have the
appetite for changing everything around all at once.

Anyway. as I may have mentioned before, I feel that calling back into
start.S from C code is wrong. That file sits at the beginning of the
image whereas the code you call back to doesn't need to. It also means
that there is (currently) no generic ARM assembler file (apart from
low_level.S) which C code can call to do things which are specific to
the architecture (but not the cpu).

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

About 48 bytes all up from memory.

>
>
>> - Make relocation symbols global so we can use them outside start.S
>
>
> Why should they relocation symbols ever be used outside of actually
> relocating?

Well since relocation is moving out of start.S to a generic library
which is not architecture-specific, we must make these symbols
available to it. Otherwise the generic code doesn't know what it is
relocating.

>
> Amicalement,
> --
> Albert.

Regards,
Simon