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

[Alexey Brodkin]

Hi Albert,

On Mon, 2015-11-16 at 14:34 +0100, Albert ARIBAUD wrote:
> Hello Alexey,
> 
> On Mon, 16 Nov 2015 13:12:15 +, Alexey Brodkin
>  wrote:
> > Hi Albert,

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

Ok, that's what I have after your patch application:

-->8--
/* Setup stack- and frame-pointers */
mov %sp, CONFIG_SYS_INIT_SP_ADDR
mov %fp, %sp

/* Get reserved area size, update SP and FP */
bl  board_init_f_get_reserve_size
/* Update stack- and frame-pointers */  <-- that's already mentioned 2 
lines above
sub %sp, %sp, %r0
mov %fp, %sp
-->8--


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

Totally agree, care to implement it?

-Alexey
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Albert ARIBAUD]

Hello Alexey,

On Mon, 16 Nov 2015 13:12:15 +, 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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Albert ARIBAUD]

Hello Alexey,

On Mon, 16 Nov 2015 13:43:19 +, Alexey Brodkin
 wrote:
> Hi Albert,
> 
> On Mon, 2015-11-16 at 14:34 +0100, Albert ARIBAUD wrote:
> > Hello Alexey,
> > 
> > On Mon, 16 Nov 2015 13:12:15 +, Alexey Brodkin
> >  wrote:
> > > Hi Albert,
> 
> > > >  
> > > > -   /* 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...
> 
> Ok, that's what I have after your patch application:
> 
> -->8--
>   /* Setup stack- and frame-pointers */
>   mov %sp, CONFIG_SYS_INIT_SP_ADDR
>   mov %fp, %sp
> 
>   /* Get reserved area size, update SP and FP */
>   bl  board_init_f_get_reserve_size
>   /* Update stack- and frame-pointers */  <-- that's already mentioned 2 
> lines above
>   sub %sp, %sp, %r0
>   mov %fp, %sp
> -->8--

My bad, I'd missed that one. I'll turn

/* Get reserved area size, update SP and FP */

Into

/* Get reserved area size */

> > ... 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?
> 
> Totally agree, care to implement it?

That, and the removal of the repetition. v5 in approach.

> -Alexey

Amicalement,
-- 
Albert.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Alexey Brodkin]

Hi Albert,

On Mon, 2015-11-16 at 15:15 +0100, Albert ARIBAUD wrote:
> Hello Alexey,
> 
> On Mon, 16 Nov 2015 13:43:19 +, Alexey Brodkin
>  wrote:
> > Hi Albert,
> > 
> > On Mon, 2015-11-16 at 14:34 +0100, Albert ARIBAUD wrote:
> > > Hello Alexey,
> > > 
> > > On Mon, 16 Nov 2015 13:12:15 +, Alexey Brodkin
> > >  wrote:
> > > > Hi Albert,
> > 
> > > > >  
> > > > > - /* 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...
> > 
> > Ok, that's what I have after your patch application:
> > 
> > -->8--
> > /* Setup stack- and frame-pointers */
> > mov %sp, CONFIG_SYS_INIT_SP_ADDR
> > mov %fp, %sp
> > 
> > /* Get reserved area size, update SP and FP */
> > bl  board_init_f_get_reserve_size
> > /* Update stack- and frame-pointers */  <-- that's already mentioned 2 
> > lines above
> > sub %sp, %sp, %r0
> > mov %fp, %sp
> > -->8--
> 
> My bad, I'd missed that one. I'll turn
> 
>   /* Get reserved area size, update SP and FP */
> 
> Into
> 
>   /* Get reserved area size */
> 
> > > ... 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?
> > 
> > Totally agree, care to implement it?
> 
> That, and the removal of the repetition. v5 in approach.
> 
> > -Alexey
> 
> Amicalement,

Thanks for doing that!

-Alexey
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Alexey Brodkin]

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 if this tiny nitpick worth respinning thought.

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.

Feel free to add
-->8--
Acked-by: Alexey Brodkin 
-->8--

-Alexey
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Albert ARIBAUD]

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 */
-   mov %sp, %r0
+   sub %sp, %sp, %r0
mov %fp, %sp
 
+   /* Initialize reserved area */
+   mov %r0, %sp
+   bl  board_init_f_init_reserve
+
/* Zero the one and only argument of "board_init_f" */
mov_s   %r0, 0
j   board_init_f
diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S
index 80548eb..4ec89a4 100644
--- a/arch/arm/lib/crt0.S
+++ b/arch/arm/lib/crt0.S
@@ -82,9 +82,11 @@ ENTRY(_main)
 #else
bic sp, sp, #7  /* 8-byte alignment for ABI compliance */
 #endif
+   bl  board_init_f_get_reserve_size
+   sub sp, sp, r0
+   mov r9, sp
mov r0, sp
-   bl  board_init_f_mem
-   mov sp, r0
+   bl  board_init_f_init_reserve
 
mov r0, #0
bl  board_init_f
diff --git a/arch/arm/lib/crt0_64.S b/arch/arm/lib/crt0_64.S
index cef1c71..2891071 100644
--- a/arch/arm/lib/crt0_64.S
+++ b/arch/arm/lib/crt0_64.S
@@ -75,8 +75,10 @@ ENTRY(_main)
ldr x0, =(CONFIG_SYS_INIT_SP_ADDR)
 #endif
bic sp, x0, #0xf/* 16-byte alignment for ABI compliance */
-   bl  board_init_f_mem
-   mov sp, x0
+   bl  board_init_f_get_reserve_size
+   sub sp, sp, x0
+   mov x0, sp
+   bl  board_init_f_init_reserve
 
mov x0, #0
bl  board_init_f
diff --git a/arch/microblaze/cpu/start.S b/arch/microblaze/cpu/start.S
index 14f46a8..206be3e 100644
--- a/arch/microblaze/cpu/start.S
+++ b/arch/microblaze/cpu/start.S
@@ -25,7 +25,7 @@ _start:
 
addir8, r0, __end
mts rslr, r8
-   /* TODO: Redo this code to call board_init_f_mem() */
+   /* TODO: Redo this code to call board_init_f_*() */
 #if defined(CONFIG_SPL_BUILD)
addir1, r0, CONFIG_SPL_STACK_ADDR
mts rshr, r1
@@ -142,7 +142,7 @@ _start:
ori r12, r12, 0x1a0
mts rmsr, r12
 
-   /* TODO: Redo this code to call board_init_f_mem() */
+   /* TODO: Redo this code to call board_init_f_*() */
 clear_bss:
/* clear BSS segments */
addir5, r0, __bss_start
diff --git a/arch/nios2/cpu/start.S b/arch/nios2/cpu/start.S
index 54787c5..45bf0fd 100644
--- a/arch/nios2/cpu/start.S
+++ b/arch/nios2/cpu/start.S
@@ -106,14 +106,17 @@ _reloc:
stw r0, 4(sp)
mov fp, sp
 
-   /* Allocate and zero GD, update SP */
+   /* Allocate and initialize reserved area, update SP */
+   movhi   r2, %hi(board_init_f_get_reserve_size@h)
+   ori r2, r2, %lo(board_init_f_get_reserve_size@h)
+   callr   r2
+   sub sp, sp, r2
mov r4, sp
-   movhi   r2, %hi(board_init_f_mem@h)
-   ori r2, r2, %lo(board_init_f_mem@h)
+   

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

[Albert ARIBAUD]

Hello Albert,

On Sun, 15 Nov 2015 19:25: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.

Of course, I only see the typos above now: that paragraph
should read:

board_init_f_mem() alters the C runtime environment's
stack it is actually already using. This is not a valid
behaviour within a C runtime environment.

Split board_init_f_mem into C functions which do not alter
their own stack and always behave properly with respect to
their C runtime environment.

If no comment calls for a v5, then whoever applies this v4 please fix
the commit message accordingly.

Amicalement,
-- 
Albert.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot