Hi Albert, On Friday, March 1, 2013 10:56:50 PM, Albert ARIBAUD wrote: > Hi Benoît, > > On Fri, 1 Mar 2013 16:50:44 +0100 (CET), Benoît Thébaudeau > <benoit.thebaud...@advansee.com> wrote: > > > Hi Albert, > > > > On Friday, March 1, 2013 4:46:07 PM, Albert ARIBAUD wrote: > > > Hi Benoît, > > > > > > On Fri, 1 Mar 2013 13:10:40 +0100, Benoît Thébaudeau > > > <benoit.thebaud...@advansee.com> wrote: > > > > > > > Factorize start.S code as much as possible. > > > > > > > > Functions that may need to be customized for some start.S are defined > > > > weak > > > > for > > > > that purpose. > > > > > > > > relocate_code_prepare() and relocate_code_finish() are introduced as > > > > hooks > > > > to be > > > > executed at the beginning and at the end of relocate_code() if needed > > > > by > > > > some > > > > start.S, e.g. for special cache or MMU operations. > > > > > > NAK. > > > > > > 1. I don't like this idea of planting hooks inside relocate-code(). > > > This function is about relocating code, not about MMU stuff. If there > > > are any MMU steps to be performed between calls to board_init_f(), > > > relocate_code() or board_init_r(), I want them laid out as calls of > > > their own right in crt0.S. > > > > I also don't like it. The finish hook was used by SMDK6400 before it was > > removed, and the prepare hook is still used by pxa. > > > > So is it OK for you if I just drop relocate_code_finish() and move and > > rename the call to relocate_code_prepare() to crt0.S? > > Fine, except for the name: "prepare for relocation" is what every > instruction does from board_init_f() return to relocate_code() entry. > This 'hook' does only a small part, if at all, of preparing for > relocation, and this part must get a less improper name. If we are > enabling the I-cache here, then let's name the function accordingly. > Better yet, let us find out if we do need to enable the instruction > cache here at all.
Correct. For PXA25X, this cpu_init_crit() is paired with lock_cache_for_stack(), apparently for some hardware hack, but this is not very clear if this is required or if it could not be done otherwise. Do you know a PXA expert? Marek, you introduced that in commit 7f4cfcf. Do you know the details about why? If we could drop it or move it elsewhere, that would be great. > > > 2. If we're going to factorize out relocate_code() from the various > > > start.S files, I want it moved not in crt0.S but in its own file. > > > > It is not in crt0.S, but in arch/arm/include/asm/start_marco.S, which is > > almost its own file apart from another macro. > > I do not want it as a macro. It is and should stay a function. > Regarding your added comment: > > Actually, I'd stopped dead at the relocate_code() changes, but the > other macros I don't like much either; I don't see the point of it. > > To be faire, I don't see the point of the whole patch wrt the > objective. This patch is just appended to the series because it depends on it, not because the series needs it. This patch only aims at cleaning up code by removing copied/pasted code in order to simplify maintenance and to clarify things. There have been many changes in this relocate_code(), and not always the same for all start.S, so from the outside, the purpose is unclear because one might wonder if those differences have been created on purpose or not. > > And in case you ask, with relocate_code() as a function in its own > > file instead of a macro called from start.S, it does not work because > > of the _start-relative word values that require relocate_code() to be > > in _start's section. > > How does it "not work" exactly? The assembler issues an error (I don't remember the exact message) for all lines like ".word __rel_dyn_start - _start", complaining that this is not a kind of expression that it can prepare for the linker to resolve. Though, it would still be possible to find a more complicated way of doing the same thing at runtime. > > > This > > > way, i) people can easily create binaries which use crt0.S but do not > > > relocate, ii) people who want to make relocate_code() a C function > > > will have it easier, and iii) crt0.S keeps being the ugly ASM glue > > > needed for flash inits, relocation and RAM inits to have a C proper > > > run-time environment. > > > > Which is already the case with this implementation? > > Not with relocate_code() as a macro, though. > > This whole thing/way of "factorizing" start.S using macros feels wrong > to me; this departs from what I have started with crt0.S, where code > is actually really factorized in a single file which is actually a > compilation unit, not a helper file. Yes. Well, for this patch, I had first moved relocate_code() to crt0.S (could have been its own file), but I eventually switched to the macro solution because of the assembler errors. > Do you need patch 31/31 in your series? As explained above, no. But I really think that something should be done to stop relocate_code() duplication, one way or another, by me or someone else. I just wanted to help here. > > > Incidentally, CC:ing Simon: > > > > > > > Signed-off-by: Benoît Thébaudeau <benoit.thebaud...@advansee.com> > > > > --- > > > > Changes in v8: > > > > - New patch. > > > > > > > > Changes in v7: None > > > > Changes in v6: None > > > > Changes in v5: None > > > > Changes in v4: None > > > > Changes in v3: None > > > > Changes in v2: None > > > > > > Is this produced by patman? > > > > Yes [...] > > Ok, then, don't bother to fix patman's behavior manually in your > own patches -- I'll try and see if I can submit a patch to fix patman > itself. OK. patman had also removed some "Reviewed-by" that I had to restore manually before sending. This is a documented behavior, but not cool. And contrary to what the documentation says, patman adds my SoB line even if I have forced another SoB in the commit message, which I also had to fix manually. Best regards, Benoît _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot