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

[Simon Glass]

Hi Albert,

On Sun, Dec 11, 2011 at 2:27 PM, Albert ARIBAUD
<albert.u.boot at aribaud.net> wrote:
> 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.

OK but sorry I am still a bit unclear.

Since I need to call from C the assembler function which sets up the
stack, invalidates the I-cache and jumps to board_init_r(), I need
this function to be somewhere. Perhaps in the future we might devise a
way of doing some of this in C code, or we might change the API. But
for now we need it.

Given that we need it, it makes little sense to me to put it in
start.S. It then gets repeated 10 times throughout the code, with
every cpu having its own version.

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

OK, so where should this code go? Repeated 10 times in start.S? Or are
you asking for a new start.S file in arch/arm/lib/ ? If so, I would
want to call it something else since link scripts may assume there is
only one start.o.

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

OK. Should is use something like 'reloc', or just drop the prefix altogether?

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

And sorry for mine :-)

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

OK, will do.

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

OK, will await your response on where this goes.

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

Well let's leave that one as it is a matter of taste. For the moment
we have a start.S for each cpu so my objection would be repeating this
code in each one. See above.

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

OK, will await your response.

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