Hi, On Fri, Mar 1, 2013 at 2:54 PM, Benoît Thébaudeau <[email protected]> wrote: > 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 >> <[email protected]> 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 >> > > <[email protected]> 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 <[email protected]> >> > > > --- >> > > > 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.
Yes I have hit this myself. Someone should do a couple of patches to fix this. I will put it on my list in case someone else doesn't get to it first. Specifically: - Don't touch/add Signed-off-by: but perhaps just want if there is not at least one in a patch - Don't touch Reviewed-by: in the normal case - but perhaps provide a flag to remove this Geritt tag Regards, Simon > > Best regards, > Benoît _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
