[U-Boot] [PATCH v8 31/31] arm: Remove duplicated start.S code

[Simon Glass]

Hi,

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