From mboxrd@z Thu Jan 1 00:00:00 1970 From: Albert ARIBAUD Date: Mon, 16 Nov 2015 14:34:25 +0100 Subject: [U-Boot] [PATCH v4] Fix board init code to use a valid C runtime environment In-Reply-To: <1447679534.6240.6.camel@synopsys.com> References: <1447611925-4028-1-git-send-email-albert.u.boot@aribaud.net> <1447679534.6240.6.camel@synopsys.com> Message-ID: <20151116143425.1384e63e@lilith> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Alexey, On Mon, 16 Nov 2015 13:12:15 +0000, Alexey Brodkin 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 > > --- > > 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 > ---------->8---------- > > -Alexey Amicalement, -- Albert.