Hi,
On Fri, Mar 1, 2013 at 2:54 PM, Benoît Thébaudeau benoit.thebaudeau@advansee.com 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 benoit.thebaudeau@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.thebaudeau@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.
- 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.
- 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.thebaudeau@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.
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