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

[Albert ARIBAUD]

Hi Albert,

On Fri, 1 Mar 2013 22:56:50 +0100, Albert ARIBAUD
<albert.u.boot at aribaud.net> 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.
> 
> > > 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:

Sorry for the mixup here. Drop the "Regarding..." just above...

> 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.

... it should go here.
 
> > 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?
> 
> > > 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.
> 
> Do you need patch 31/31 in your series?
> 
> > > 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.
> 
> > Best regards,
> > Benoît
> 
> Amicalement,


Amicalement,
-- 
Albert.