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

[Simon Glass]

Hi Albert,

On Wed, Dec 7, 2011 at 12:10 AM, Albert ARIBAUD
<albert.u.boot at aribaud.net> wrote:
> 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/

Well based on your other comments I have gone with common/ for now.

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

Well it still needs the I$ stuff and other archs may need more. It
could be an inline function, but what would be the benefit? No time or
code size saving I think.

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

Well it is just that SPL brings in source files from around U-Boot and
builds them into an image. If we add a new object file which is needed
by SPL then we may need to change all those Makefiles. I don't think
that applies to relocation luckily. We will hit this issue anytime we
move code out of start.S.

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

OK, well I may try that with the next revision.

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

Well it is the first thing I have chosen to deal with on the path to a
generic board.c. Relocation is a large part of what
arch/xxx/lib/board.c currently does. And after all I am actually
moving it out of board.c.

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

ok

>
> - 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 sent some numbers, but hope to improve on them.

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

Well that's a bit tricky unless you want be to replace the code in
start.S to call my proc.S function, just before I delete the code in
start.S!

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

I was referring to SPL, but I think we have established that it
shouldn't matter.

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

Ok.

>
> Amicalement,
> --
> Albert.

Regards,
Simon