| Message ID | 56442AC0.2030306@wytron.com.tw |
|---|---|
| State | RFC |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
Hello Thomas, On Thu, 12 Nov 2015 13:59:28 +0800, Thomas Chou <thomas@wytron.com.tw> wrote: > Hi Albert, > > On 2015年11月11日 02:30, 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> > > --- > > For NIOS2, this patch hopefully contains all manual > > fixes by Thomas. > > > > 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 | 20 +++++++++++++--- > > arch/arm/lib/crt0.S | 10 ++++++-- > > arch/arm/lib/crt0_64.S | 10 ++++++-- > > arch/microblaze/cpu/start.S | 4 ++-- > > arch/nios2/cpu/start.S | 17 ++++++++++++-- > > arch/powerpc/cpu/ppc4xx/start.S | 18 ++++++++++---- > > arch/x86/cpu/start.S | 10 ++++++-- > > arch/x86/lib/fsp/fsp_common.c | 4 ++-- > > common/init/board_init.c | 31 ++++++++++++++---------- > > include/common.h | 52 +++++++++++++++++++++++++---------------- > > 10 files changed, 125 insertions(+), 51 deletions(-) > > > > Additional fixes, > ------------------------------------------------------------------------ > diff --git a/common/init/board_init.c b/common/init/board_init.c > index 8839a4a..703e6d8 100644 > --- a/common/init/board_init.c > +++ b/common/init/board_init.c > @@ -46,6 +46,7 @@ void board_init_f_gd(struct global_data *gd_ptr) > for (ptr = (int *)gd_ptr; ptr < (int *)(gd_ptr + 1); ) > *ptr++ = 0; > #endif > + arch_setup_gd(gd_ptr); Correct -- in ARM (Thumb-1 at least) we cannot use arch_setup_gd() so we set GD (in r9) from within arch/arm/lib/crt0.S, but for NIOS2 it might (and apparently does) work. Where is GD stored in NIOS2? > } > > ulong board_init_f_malloc_size(void) > -------------------------------------------------------------------------- > diff --git a/arch/nios2/cpu/start.S b/arch/nios2/cpu/start.S > index c163ce1..0adff46 100644 > --- a/arch/nios2/cpu/start.S > +++ b/arch/nios2/cpu/start.S > @@ -110,7 +110,7 @@ _reloc: > movhi r2, %hi(board_init_f_gd_size@h) > ori r2, r2, %lo(board_init_f_gd_size@h) > callr r2 > - sub sp, sp, r4 > + sub sp, sp, r2 Sorry, didn't know / realize the NIOS2 ABI has r2 iass the function value return register, not r4. > mov r4, sp > movhi r2, %hi(board_init_f_gd@h) > ori r2, r2, %lo(board_init_f_gd@h) > @@ -119,16 +119,12 @@ _reloc: > movhi r2, %hi(board_init_f_malloc_size@h) > ori r2, r2, %lo(board_init_f_malloc_size@h) > callr r2 > - sub sp, sp, r4 > + sub sp, sp, r2 Ditto. > mov r4, sp > movhi r2, %hi(board_init_f_malloc@h) > ori r2, r2, %lo(board_init_f_malloc@h) > callr r2 > > - /* Update stack- and frame-pointers */ > - mov sp, r2 > - mov fp, sp > - Oops. > /* Call board_init_f -- never returns */ > mov r4, r0 > movhi r2, %hi(board_init_f@h) > ---------------------------------------------------------------------------- > Otherwise, > > Tested-by: Thomas Chou <thomas@wytron.com.tw> > Acked-by: Thomas Chou <thomas@wytron.com.tw> > > Thanks. Thanks to you; Will send a v3 to account for your and Simon's comments. > Best regards, > Thomas Amicalement,
Hi Albert, On 2015年11月12日 15:17, Albert ARIBAUD wrote: >> ------------------------------------------------------------------------ >> diff --git a/common/init/board_init.c b/common/init/board_init.c >> index 8839a4a..703e6d8 100644 >> --- a/common/init/board_init.c >> +++ b/common/init/board_init.c >> @@ -46,6 +46,7 @@ void board_init_f_gd(struct global_data *gd_ptr) >> for (ptr = (int *)gd_ptr; ptr < (int *)(gd_ptr + 1); ) >> *ptr++ = 0; >> #endif >> + arch_setup_gd(gd_ptr); > > Correct -- in ARM (Thumb-1 at least) we cannot use arch_setup_gd() so > we set GD (in r9) from within arch/arm/lib/crt0.S, but for NIOS2 it > might (and apparently does) work. Where is GD stored in NIOS2? > It is a register, r26 "gp". I have another question. Will it be simpler to have two calls instead of four? 1. get size of gd plus malloc. 2. init gd and malloc. Best regards, Thomas
Hello Thomas, On Thu, 12 Nov 2015 16:28:38 +0800, Thomas Chou <thomas@wytron.com.tw> wrote: > Hi Albert, > > On 2015年11月12日 15:17, Albert ARIBAUD wrote: > >> ------------------------------------------------------------------------ > >> diff --git a/common/init/board_init.c b/common/init/board_init.c > >> index 8839a4a..703e6d8 100644 > >> --- a/common/init/board_init.c > >> +++ b/common/init/board_init.c > >> @@ -46,6 +46,7 @@ void board_init_f_gd(struct global_data *gd_ptr) > >> for (ptr = (int *)gd_ptr; ptr < (int *)(gd_ptr + 1); ) > >> *ptr++ = 0; > >> #endif > >> + arch_setup_gd(gd_ptr); > > > > Correct -- in ARM (Thumb-1 at least) we cannot use arch_setup_gd() so > > we set GD (in r9) from within arch/arm/lib/crt0.S, but for NIOS2 it > > might (and apparently does) work. Where is GD stored in NIOS2? > > > > It is a register, r26 "gp". Ok. In ARM it is r9, and gcc is prevented from ever using r9 by adding the compiler option -ffixed-r9 to the compiler command lines. I guess the same goes for NIOS2 -- maybe the NIOS2 gcc does not even need an option, and always leaves gp/r26 alone. > I have another question. Will it be simpler to have two calls instead of > four? > > 1. get size of gd plus malloc. > > 2. init gd and malloc. I'd hinted at reducing the number of functions, but not the number of calls, in my reply to Simon Glass in this thread, see here: http://article.gmane.org/gmane.comp.boot-loaders.u-boot/240520 Your proposal might indeed help reducing the number of calls as well. The first function would receive the current top of the stack and would subtract the (aligned) gd then the (aligned) malloc arena size, and return the new top-of-stack, which the C runtime code would enforce in sp (or whatever the equivalent is in each arch) -- BUT it would not write anything in that space as it would not be reserved at that point. The second function would receive this new top-of-stack again, this time as a base of the reserved space (or it could receive the old top-of-stack and work its way downward) and would be able to safely write whatever it wants inside this space. The only caveat is, we need to be sure that the second function can reconstruct the base addresses of all allocated chunks (gd, malloc, whatever they'll be wanting to add later) just like the first function computed them (it could still be a single function called twice as I'd suggested, BTW). Thanks for the suggestion! I'll consider it for v3. > Best regards, > Thomas Amicalement,
Hello Thomas, On Thu, 12 Nov 2015 13:59:28 +0800, Thomas Chou <thomas@wytron.com.tw> wrote: > Hi Albert, > - /* Update stack- and frame-pointers */ > - mov sp, r2 > - mov fp, sp Just a sec here on second thought. I understand the 'mov sp, r2' must be removed as it is replaced by a 'sub sp, sp, r2' a few lines earlier. But doesn't the 'mov fp, sp' need to remain? Without it, fp is not set any more, is it? > Best regards, > Thomas Amicalement,
Hi Albert, On 2015年11月13日 14:41, Albert ARIBAUD wrote: > Hello Thomas, > > On Thu, 12 Nov 2015 13:59:28 +0800, Thomas Chou <thomas@wytron.com.tw> > wrote: >> Hi Albert, > >> - /* Update stack- and frame-pointers */ >> - mov sp, r2 >> - mov fp, sp > > Just a sec here on second thought. > > I understand the 'mov sp, r2' must be removed as it is replaced by a > 'sub sp, sp, r2' a few lines earlier. > > But doesn't the 'mov fp, sp' need to remain? Without it, fp is not set > any more, is it? > Yes, the 'mov fp, sp' need to remain. I forgot to add them. Best regards, Thomas
diff --git a/common/init/board_init.c b/common/init/board_init.c index 8839a4a..703e6d8 100644 --- a/common/init/board_init.c +++ b/common/init/board_init.c @@ -46,6 +46,7 @@ void board_init_f_gd(struct global_data *gd_ptr) for (ptr = (int *)gd_ptr; ptr < (int *)(gd_ptr + 1); ) *ptr++ = 0; #endif + arch_setup_gd(gd_ptr); } ulong board_init_f_malloc_size(void) -------------------------------------------------------------------------- diff --git a/arch/nios2/cpu/start.S b/arch/nios2/cpu/start.S index c163ce1..0adff46 100644 --- a/arch/nios2/cpu/start.S +++ b/arch/nios2/cpu/start.S @@ -110,7 +110,7 @@ _reloc: movhi r2, %hi(board_init_f_gd_size@h) ori r2, r2, %lo(board_init_f_gd_size@h) callr r2 - sub sp, sp, r4 + sub sp, sp, r2 mov r4, sp movhi r2, %hi(board_init_f_gd@h) ori r2, r2, %lo(board_init_f_gd@h) @@ -119,16 +119,12 @@ _reloc: movhi r2, %hi(board_init_f_malloc_size@h) ori r2, r2, %lo(board_init_f_malloc_size@h) callr r2 - sub sp, sp, r4 + sub sp, sp, r2 mov r4, sp movhi r2, %hi(board_init_f_malloc@h) ori r2, r2, %lo(board_init_f_malloc@h) callr r2 - /* Update stack- and frame-pointers */ - mov sp, r2 - mov fp, sp - /* Call board_init_f -- never returns */ mov r4, r0 movhi r2, %hi(board_init_f@h)