Hi Graeme,
On Sun, Dec 11, 2011 at 9:58 PM, Graeme Russ graeme.russ@gmail.com wrote:
Hi Simon,
On Mon, Dec 12, 2011 at 4:20 PM, Simon Glass sjg@chromium.org wrote:
Hi Albert,
On Sun, Dec 11, 2011 at 2:27 PM, Albert ARIBAUD albert.u.boot@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