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

[Graeme Russ]

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

Regards,

Graeme