Hi Graeme, On Sun, Dec 11, 2011 at 9:58 PM, Graeme Russ <graeme.r...@gmail.com> wrote: > Hi Simon, > > On Mon, Dec 12, 2011 at 4:20 PM, Simon Glass <s...@chromium.org> wrote: >> Hi Albert, >> >> On Sun, Dec 11, 2011 at 2:27 PM, Albert ARIBAUD >> <albert.u.b...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
