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

[Benoît Thébaudeau]

Hi Albert,

On Saturday, March 2, 2013 7:45:06 AM, Albert ARIBAUD wrote:
> 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.

Yes, I think so. In the worst case, it should be possible to access out-of-range
symbol relatively using adr or adrl extensively at runtime instead of
pre-computed _start-relative offsets.

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

OK, let's do this. It will also help to stop postponing the application of this
series because of more new versions.

Please just Cc me when you will post these patches so that I review them.

Best regards,
Benoît