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

[Albert ARIBAUD]

Hi Benoît,

On Fri, 1 Mar 2013 23:54:26 +0100 (CET), Benoît Thébaudeau
<benoit.thebaudeau at advansee.com> wrote:

Re: assembler error messages:

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

(later)

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

I've had issue in the past similar to this when I implemented the ELF
relocation, the crt0.S factorization and more recently the R_ARM_ABS32
relocation record removal. These must be fixed with much attention, for
instance in order to not produce R_ARM_ABS32 relocations, the removal
of which I have just submitted a patch for. I think there is a way to
factorize relocate_code() (and other parts) out of start.S files, built
on what I did for crt0.S, and which should not cause these issues.

Re: patch 31/31 generally:

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

(picked up from later in the reply)

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

First, let me say that I appreciate the great help that you're giving us
with this (30-patch!) series.

And I agree about the premise that ARM startup sequence, including
but not limited to relocate_code(), is literally all over the place and
that there is a need to fix this; I wrote so in the cover letter of
my crt0.S patch series, which I consider a starting point and example
of how I consider this should be done.

Also, we must keep in mind that part of the code in ARM should, and
eventually will, be merged into a single U-Boot-wide version. ELF code
relocation is not ARM specific except for the two (to be reduced soon
to only one) ARM relocation record types. Thus, when we touch this
code, we must keep it close, or make it closer, to the code in other
U-Boot architectures; IIRC there are already patches out there to make
relocate_code() a single project-wide true C function.

And I think that this newly added patch 31/31 in your series does not
match either the way I want ARM startup simplification to go or the
general goal of unifying relocate_code().

Thus, if you don't mind, I'd prefer patch 31/31 to move out of the
series. And, since I want to avoid anyone the hassle of going through
this again, I guess I will have to submit a patch for relocate_code()
factorization -- quite probably above your series, since many fixes
you make in it may be useful or even needed.

> Best regards,
> Benoît

Amicalement,
-- 
Albert.