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

[Albert ARIBAUD]

Hi againLe 11/12/2011 22:30, Simon Glass a écrit :

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

Indeed, Generalizing start.S is a separate problem from generalizing 
board.c; and that means your reboard patch series is unaffected whether 
you move some code into proc.S or not. Actually, that move to proc.S is 
just a code factorization, unrelated to relocation -- your series can 
work just as well without it, only it will keep on taking up code size 
that could be reduced regardless to the way relocation is done.

Now, seen from the reverse perspective of generalizing start.S, the move 
to proc.S in this reboard series is a step in the wrong direction, which 
is why I do not want it. Plus, it'll make your 'reboard' patch more 
focused on what it is about, i.e. generalizing relocation.

(BTW, even though I understand your greater goal about board.c, I still 
think the patch subjects should *not* contain this "reboard" prefix nor 
mention that greater goal. The patch series is about relocation, and 
should be clear about it.)

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

(sorry for the typoes, BTW)

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

We don't need to find a header file with all the relocation types we 
will need, we only need one with the two ARM relocation types we need 
now. Adding them to include/elf.h.

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

This view of start.S is kind of an /a priori/ -- just because the name 
has start.S does not mean that there should only be startup code in 
there. Conversively, one could argue that handling the returning from 
board_init_f and branching into board_init_r is *still* startup code.

But yes, maybe one day start.S will be split into a file with only the 
startup code and a file with everything else. Only this is totally 
unrelated to relocation, and thus will not happen in this patch series.

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

I fail to see the point in your argument about about start.S being "far 
away" from the C code that call into it or return to it. Never do we 
consider the "distance" between C callers and callees, so why should we 
do it here?

As for there being no generic ARM assembler file etc., before 
complaining that there isn't one, I would like to see why one should be 
created. For me, just having a couple pieces of code that are 'generic 
ARM code that C can call' is not a criterion for putting them in a new 
file. Files must group pieces of code that have common functional 
purpose, not simply pieces of code that have a common characteristic.

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

About 12 instructions? Funny, I wouldn't have thought inlining a 
function called only once would save that much. Thanks for the info.

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

Understood. My point is that they should not be made available to code 
other than the relocation code.

> Regards,
> Simon

Amicalement,
-- 
Albert.