[U-Boot] [RFC PATCH 0/7] reboard: Introduce generic relocation feature

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

Hi Simon,

Le 22/11/2011 00:57, 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 creates a libboard
> library and implements relocation in it. 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.

Agreed.

> This series establishes a new libboard library in the board/ subdir and
> puts some relocation code in it. Each architecture which uses this
> framework needs to provide a function called arch_elf_relocate_entry()
> which processes a single relocation entry. If there is concern about
> calling a function for all 2000-odd relocations then I can change this.

NAK -- a generic relocation function should be not be in board/, it 
should be in lib/

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

As commented in detail, I am not sure creating a function for three asm 
instructions is worthwhile. The overhead for calling the code might be 
bigger than the body of the function. Did you compare with inline 
functions with asm statements and/or intrinsics?

> It may be useful for other architectures to have a similar file.
>
> This series moves ARM over to use this framework. Overall this means that
> two new files are required 'early' in boot: board/reloc.c and
> arch/arm/lib/proc.S.  This is tricky mainly due to SPL.

Can you develop what the issue is with SPL exactly?

> I believe that
> we may need to adjust link scripts to put these two files early in the
> link scripts also. But I am not sure about this and can't actually find
> a problem as yet. I would much prefer to solve this with a new section
> name like .text.early if we can.
>
> (I should really cc all arch maintainers but I think in that case I get
> an error from the list server. Not sure what the limit is.)

This would not cause an error, but Wolfgang would have to "OK' the post, 
which is a good thing if many people are involved. :)

> Comments please...

Generic comment is that the patch is not about generic *board*. It is 
about generic *relocation*, which is not a thing of boards IMO: it is 
not related to peripheral or HW configuration, and is not actually 
memory-mapped related.

Other than that, as stated in the detailed answers, we need to keep the 
relocation code as efficient as possible. As happy as I am to see the 
ELF relocation code rewritten in C for easy understanding and 
maintenance, I would not want it done at the expense of speed or overall 
architecture soundness. So:

- the C relocation code (both the generic function and the ELF specific 
code) are neither board-related nor ARM-specific, so it has no reason to 
reside in board/ or arch/arm. My first reflex was lib/, but indeed 
common/ might be a better place.

- the relocation function should be as efficient as possible. Compared 
numbers between 'before' and 'after' should also be provided -- I do not 
necessarily expect the same or better performance, but we need to assess 
what the performance issue is.

- I would prefer each patch to be as self-contained as possible -- 
'preparatory' patches I don't like much. For instance, to relace ASM 
relocation with C relocation, I would want a single patch introducing 
the C code and removing the ASM code or moving it out of the way.

- I am not sure why the code should be "early" or, more precisely, 
should be specifically located in the linker file: any code in there is 
accessible at any time, and the only special case is for _start because 
we want it to be first in placement, not in time. Can you clarify this 
'early' issue?

- indeed I would prefer a weak symbol for the relocation function. This 
would allow a given config to easily override the generic C code if needed.

Amicalement,
-- 
Albert.