[U-Boot] [PATCH v4] Fix board init code to use a valid C runtime environment

Albert ARIBAUD
2015-11-16T13:34:25Z

Hello Alexey,

On Mon, 16 Nov 2015 13:12:15 +0000, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> Hi Albert,
>
> On Sun, 2015-11-15 at 19:25 +0100, Albert ARIBAUD wrote:
> > board_init_f_mem() alters the C runtime environment's
> > stack it ls actually already using. This is not a valid
> > C runtime environment.
> >
> > Split board_init_f_mem into C functions which do not
> > alter their own stack and therefore run in a valid C
> > runtime environment.
> >
> > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> > ---
> > Cc:ing custodians for all architectures which this
> > patch affects.
> >
> > This patch has been build-tested for all there
> > architectures, and run-tested on ARM OpenRD Client.
> >
> > For NIOS2, this patch contains all manual v1 and v2
> > fixes by Thomas.
> >
> > For x86, this patch contains all manual v2 fixes by Bin.
> >
> > Changes in v4:
> > - Add comments on reserving GD at the lowest location
> > - Add comments on post-incrementing base for each "chunk"
> >
> > Changes in v3:
> > - Rebase after commit 9ac4fc82
> > - Simplify malloc conditional as per commit 9ac4fc82
> > - Fix NIOS2 return value register (r2, not r4)
> > - Fix x86 subl argument inversion
> > - Group allocations in a single function
> > - Group initializations in a single function
> >
> > Changes in v2:
> > - Fix all checkpatch issues
> > - Fix board_init_f_malloc prototype mismatch
> > - Fix board_init_[f_]xxx typo in NIOS2
> > - Fix aarch64 asm 'sub' syntax error
> >
> > arch/arc/lib/start.S | 12 +++--
> > arch/arm/lib/crt0.S | 6 ++-
> > arch/arm/lib/crt0_64.S | 6 ++-
> > arch/microblaze/cpu/start.S | 4 +-
> > arch/nios2/cpu/start.S | 13 +++--
> > arch/powerpc/cpu/ppc4xx/start.S | 10 ++--
> > arch/x86/cpu/start.S | 5 +-
> > arch/x86/lib/fsp/fsp_common.c | 4 +-
> > common/init/board_init.c | 108 +++++++++++++++++++++++++++++++++++-----
> > include/common.h | 33 +++++-------
> > 10 files changed, 144 insertions(+), 57 deletions(-)
> >
> > diff --git a/arch/arc/lib/start.S b/arch/arc/lib/start.S
> > index 26a5934..2a99193 100644
> > --- a/arch/arc/lib/start.S
> > +++ b/arch/arc/lib/start.S
> > @@ -54,14 +54,16 @@ ENTRY(_start)
> > mov %sp, CONFIG_SYS_INIT_SP_ADDR
> > mov %fp, %sp
> >
> > - /* Allocate and zero GD, update SP */
> > - mov %r0, %sp
> > - bl board_init_f_mem
> > -
> > + /* Get reserved area size, update SP and FP */
> > + bl board_init_f_get_reserve_size
> > /* Update stack- and frame-pointers */
>
> I think we don't need to mention SP/FP update in comments twice here.
> I.e. either strip ", update SP and FP" from your introduced comment or
> which I really like more remove following line with comment entirely:
> ---------->8----------
> /* Update stack- and frame-pointers */
> ---------->8----------

Not sure where you see two SP+FP 'update' comments here; probably
you're referring to the 'setup' comment on line 53 and the 'update'
one on line 59. If that is what you meant, I tink these comments are
different and deserve staying both...

... However, these comments also pretty much just paraphrase the code
which follows them and thus serve little purpose; they could be
reworded to show less of what is being done and more of why it is being
done:

- the "Update stack- and frame-pointer" comment could be turned into
"Allocate reserved size on stack and adjust frame pointer
accordingly", and

- the "Setup stack- and frame-pointers" comment could be turned into
"Establish C runtime stack and frame".

Opinions?

> Not sure if this tiny nitpick worth respinning thought.

That's fine, since I've got the commit message to fix too.

> Otherwise build and run tested on free nSIM,
> see "Running U-Boot on ARC in Free nSIM" section in
> http://www.denx.de/wiki/U-Boot/ARCNotes if of any interest how to do it
> yourself.

Thanks, I'll try this, and I4ll certainly keep it in my bookmarks.

> Feel free to add
> ---------->8----------
> Acked-by: Alexey Brodkin <abrodkin@synopsys.com>
> ---------->8----------
>
> -Alexey

Amicalement,
--
Albert.