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

[Simon Glass]

Hi Graeme,

On Sun, Dec 11, 2011 at 9:58 PM, Graeme Russ <graeme.russ at gmail.com> wrote:
> Hi Simon,
>
> On Mon, Dec 12, 2011 at 4:20 PM, Simon Glass <sjg at chromium.org> wrote:
>> 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.
>>
>
> [snip]
>
> The 'problem' is that in order to implement the centralisation of the
> relocation code, you need to do some non-relocation related fix-ups along
> the way.
>
> I think it would be good to seperate what you are doing into a 'prepare'
> series which simply shuffles code around ready for the actual relocation
> patches - The compiled code should be identical before and after the
> 'prepare' patches. If you find some code that can be optimised
> (consolidating duplicate code across SoCs for example) then do this either
> immediately before are after the 'prepare' patches (if those optimisations
> make no difference to the relocation changes, then you can even leave them
> until after the relocation patches)
>
> After the 'prepare' patches, the relocation changes will be much clearer
>
> Remember, there is nothing wrong with submitting multiple series of patches
> where one depends on the other, but you need to make the precedence clear
> in the description. This approach is preferable over interleaving patches
> which are not, technically, related to the subject of what you are doing in
> the series. If it gets to the point where you simply must have some
> interleaving patches, then it is OK to do it within the series, but change
> the subject of the interleaving patches to make it clear that they are not
> directly related
>
> Rember, once the patches are applied, the concept of a 'series' will be
> lost, so the tag at the beginning of the subject must clearly represent
> what that individual patch is about - Having a 'reloc' tag on a code
> consolidation patch will not make sense in a years time...

Can you please be specific about these non-relocation fix-ups? The
last patch of the removes all the relocation code from the various
start.S files. Is that what you mean? There is obviously a bit of a
problem here, but I just don't see what it is.

Here are the patches with my questions / comments:

 reboard: Create reloc.h and include it where needed
- I think this is fine - it was requested by review feedback

 reboard: define CONFIG_SYS_SKIP_RELOC for all archs
- This option is intended to deal with architectures which don't yet
use the generic relocation method. It is waiting on Albert to say what
is actually wanted here, perhaps a separate 'skip-relocation' patch
which can be turned on per board (but I thought that was NAKed), or
perhaps renaming this option.

 reboard: Add generic relocation feature
- this just adds the generic code so I think is ok

 reboard: arm: Add processor function library
- this adds the 'jump to board_init_r()' function, which is currently
a few instructions at the end of the assembler version of
relocate_code(), repeated in each start.S

 reboard: arm: Move over to generic relocation
- this turns off CONFIG_SYS_SKIP_RELOC for ARM and makes it use the
generic reloc

 reboard: arm: Remove unused code in start.S
- this removes the relocate_code() implementation in each start.S, now
that this is not needed

Regards,
Simon

>
> Regards,
>
> Graeme