While trying to compile u-boot in thumb due space constraints on a mxs
v2 cover While trying to compile u-boot in thumb due space constraints on a mxs
platform it was observed that there are some thumb-interwork issues in the handwritten assembly files.
Since the first patch only applies to ARM926EJS and no board on that platform has thumb enabled for now,it was probably never observed.
The second one applies to the ARM specific assembly memcpy implementation that is not enabled on any of the boards enabling thumb.
grep -l CONFIG_SYS_THUMB_BUILD=y configs/* | \
xargs grep -c CONFIG_USE_ARCH_MEMCPY
configs/apalis_imx6_nospl_com_defconfig:0
configs/apalis_imx6_nospl_it_defconfig:0
configs/bk4r1_defconfig:0
configs/colibri_imx6_nospl_defconfig:0
configs/colibri_imx7_defconfig:0
configs/colibri_vf_defconfig:0
configs/highbank_defconfig:0
configs/openrd_base_defconfig:0
configs/openrd_client_defconfig:0
configs/openrd_ultimate_defconfig:0
configs/pcm052_defconfig:0
configs/tbs2910_defconfig:0
configs/x600_defconfig:0
With CONFIG_USE_ARCH_MEMCPY on our mxs platform the speedup for memcopy was about 100%.
Changes in v2:
- use bl instead of blx to call lowlevel_init
- remove mxs tag as it apply to all arm926ejs platforms
Klaus Goger (2):
arm: make arm926ejs startup code thumb compatible
arm: Make arch specific memcpy thumb-safe.
arch/arm/cpu/arm926ejs/start.S | 6 +++--- arch/arm/lib/memcpy.S | 8 +++++--- 2 files changed, 8 insertions(+), 6 deletions(-)
Signature · 1 line
2.11.0
PATCH v2 1/2 arm: make arm926ejs startup code thumb compatible
When building the mxs platform in thumb mode gcc generates code using the intra procedure call scratch register (ip/r12) for the calling the lowlevel_init function. This modifies the lr in flush_dcache which causes u-boot proper to end in an endless loop.
40002334: e1a0c00e mov ip, lr 40002338: eb00df4c bl 4003a070 <__lowlevel_init_from_arm> 4000233c: e1a0e00c mov lr, ip 40002340: e1a0f00e mov pc, lr
[...]4003a070 <__lowlevel_init_from_arm>:4003a070: e59fc004 ldr ip, [pc, #4] ; 4003a07c
<__lowlevel_init_from_arm+0xc>
4003a074: e08fc00c add ip, pc, ip 4003a078: e12fff1c bx ip 4003a07c: fffc86cd .word 0xfffc86cd
Instead of using the the ip/r12 register we use sl/r10 to preserve the link register.
According to "Procedure Call Standard for the ARM Architecture" by ARM subroutines have to preserve the contents of register r4-r8, r10, r11 and SP. So using r10 instead of r12 should be save.
---
Changes in v2:
- use bl instead of blx to call lowlevel_init
- remove mxs tag as it apply to all arm926ejs platforms
arch/arm/cpu/arm926ejs/start.S | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
arch/arm/cpu/arm926ejs/start.S +3 -3
@@ -105,9 +105,9 @@ flush_dcache: /* * Go setup Memory and board specific bits prior to relocation. */ - mov ip, lr /* perserve link reg across call */ + mov sl, lr /* perserve link reg across call */ bl lowlevel_init /* go setup pll,mux,memory */ - mov lr, ip /* restore link */ + mov lr, sl /* restore link */ #endif - mov pc, lr /* back to my caller */ + bx lr /* back to my caller */ #endif /* CONFIG_SKIP_LOWLEVEL_INIT */
Signature · 1 line
2.11.0
PATCH v2 1/2 arm: make arm926ejs startup code thumb compatible
Quoted 25 lines · Klaus Goger
When building the mxs platform in thumb mode gcc generates code using the intra procedure call scratch register (ip/r12) for the calling the
lowlevel_initfunction. This modifies the lr inflush_dcachewhich causes u-boot proper to end in an endless loop.40002334: e1a0c00e mov ip, lr 40002338: eb00df4c bl 4003a070 <__lowlevel_init_from_arm> 4000233c: e1a0e00c mov lr, ip 40002340: e1a0f00e mov pc, lr[...]
4003a070<__lowlevel_init_from_arm>:4003a070:e59fc004ldr ip, [pc, #4] ;4003a07c
<__lowlevel_init_from_arm+0xc>4003a074: e08fc00c add ip, pc, ip 4003a078: e12fff1c bx ip 4003a07c: fffc86cd .word 0xfffc86cdInstead of using the the ip/r12 register we use sl/r10 to preserve the link register.
According to "Procedure Call Standard for the ARM Architecture" by ARM subroutines have to preserve the contents of register r4-r8, r10, r11 and SP. So using r10 instead of r12 should be save.
Quoted 8 lines
Signed-off-by Klaus Goger <[email protected]>Signed-off-by Christoph Muellner <[email protected]>---
Changes in v2:
- use bl instead of blx to calllowlevel_init
- remove mxs tag as it apply to all arm926ejs platforms
What is lowlevel_init() is implemented as a C code and therefore thumb ? :-) doesn't ie. armv7a handle this somehow already ?
Quoted 21 lines
arch/arm/cpu/arm926ejs/start.S | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)arch/arm/cpu/arm926ejs/start.S +3
-3@@ -105,9 +105,9 @@ flush_dcache: /* * Go setup Memory and board specific bits prior to relocation. */ - mov ip, lr /* perserve link reg across call */ + mov sl, lr /* perserve link reg across call */ bl lowlevel_init /* go setup pll,mux,memory */ - mov lr, ip /* restore link */ + mov lr, sl /* restore link */ #endif - mov pc, lr /* back to my caller */ + bx lr /* back to my caller */ #endif /* CONFIG_SKIP_LOWLEVEL_INIT */
Signature · 2 lines
Best regards, Marek Vasut
PATCH v2 1/2 arm: make arm926ejs startup code thumb compatible
Quoted 33 lines · Marek Vasut
Quoted 28 lines · Klaus Goger
When building the mxs platform in thumb mode gcc generates code using the intra procedure call scratch register (ip/r12) for the calling the
lowlevel_initfunction. This modifies the lr inflush_dcachewhich causes u-boot proper to end in an endless loop.40002334: e1a0c00e mov ip, lr 40002338: eb00df4c bl 4003a070 <__lowlevel_init_from_arm> 4000233c: e1a0e00c mov lr, ip 40002340: e1a0f00e mov pc, lr[...]
4003a070<__lowlevel_init_from_arm>:4003a070:e59fc004ldr ip, [pc, #4] ;4003a07c
<__lowlevel_init_from_arm+0xc>4003a074: e08fc00c add ip, pc, ip 4003a078: e12fff1c bx ip 4003a07c: fffc86cd .word 0xfffc86cdInstead of using the the ip/r12 register we use sl/r10 to preserve the link register.
According to "Procedure Call Standard for the ARM Architecture" by ARM subroutines have to preserve the contents of register r4-r8, r10, r11 and SP. So using r10 instead of r12 should be save.
Signed-off-by Klaus Goger <[email protected]>Signed-off-by Christoph Muellner <[email protected]>
Quoted 8 lines
Quoted 5 lines
---
Changes in v2:
- use bl instead of blx to calllowlevel_init
- remove mxs tag as it apply to all arm926ejs platformsWhat is
lowlevel_init()is implemented as a C code and therefore thumb ? :-) doesn't ie. armv7a handle this somehow already ?
lowlevel_init() is implemented in C (you wrote it :)) and therefore thumb if compiled as thumb.
armv7 lowlevel_init is implemented in assembly.
As the compiler will generate the necessary interwork code there was no need to change the branch instruction to blx. So I changed it back to bl as the original code as this will work with both generated codes (arm and thumb) and also if someone decides to reimplement lowlevel_init as arm assembly.
Quoted 27 lines
Quoted 22 lines
arch/arm/cpu/arm926ejs/start.S| 6 +++---1 file changed, 3 insertions(+), 3 deletions(-)arch/arm/cpu/arm926ejs/start.S +3
-3@@ -105,9 +105,9 @@ flush_dcache: /* * Go setup Memory and board specific bits prior to relocation. */ - mov ip, lr /* perserve link reg across call */ + mov sl, lr /* perserve link reg across call */ bl lowlevel_init /* go setup pll,mux,memory */ - mov lr, ip /* restore link */ + mov lr, sl /* restore link */ #endif - mov pc, lr /* back to my caller */ + bx lr /* back to my caller */ #endif /* CONFIG_SKIP_LOWLEVEL_INIT */Signature · 2 lines
Best regards, Marek Vasut
PATCH v2 1/2 arm: make arm926ejs startup code thumb compatible
In reply to Klaus GogerQuoted 21 lines · Klaus Goger
When building the mxs platform in thumb mode gcc generates code using the intra procedure call scratch register (ip/r12) for the calling the
lowlevel_initfunction. This modifies the lr inflush_dcachewhich causes u-boot proper to end in an endless loop.40002334: e1a0c00e mov ip, lr 40002338: eb00df4c bl 4003a070 <__lowlevel_init_from_arm> 4000233c: e1a0e00c mov lr, ip 40002340: e1a0f00e mov pc, lr[...]
4003a070<__lowlevel_init_from_arm>:4003a070:e59fc004ldr ip, [pc, #4] ;4003a07c
<__lowlevel_init_from_arm+0xc>4003a074: e08fc00c add ip, pc, ip 4003a078: e12fff1c bx ip 4003a07c: fffc86cd .word 0xfffc86cdInstead of using the the ip/r12 register we use sl/r10 to preserve the link register.
Quoted 6 lines
According to "Procedure Call Standard for the ARM Architecture" by ARM subroutines have to preserve the contents of register r4-r8, r10, r11 and SP. So using r10 instead of r12 should be save.
Signed-off-by Klaus Goger <[email protected]>Signed-off-by Christoph Muellner <[email protected]>
This problem isn't specific to Thumb mode. An ARM build would also break if the lowlevel_init function happened to clobber r12, which it is permitted to do. It's just dumb luck that this hasn't happened yet.
Quoted 14 lines
---
Changes in v2:
- use bl instead of blx to calllowlevel_init
- remove mxs tag as it apply to all arm926ejs platformsarch/arm/cpu/arm926ejs/start.S | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)arch/arm/cpu/arm926ejs/start.S +0
-0
arch/arm/cpu/arm926ejs/start.S flush_dcache: · line 110
Quoted 8 lines
arch/arm/cpu/arm926ejs/start.S +2
-2@@ -105,9 +105,9 @@ flush_dcache: /* * Go setup Memory and board specific bits prior to relocation. */ - mov ip, lr /* perserve link reg across call */ + mov sl, lr /* perserve link reg across call */ bl lowlevel_init /* go setup pll,mux,memory */ - mov lr, ip /* restore link */ + mov lr, sl /* restore link */
I prefer to use plain register names (r10) rather than the aliases (sl) when not using them for the special functions indicated by the latter.
arch/arm/cpu/arm926ejs/start.S flush_dcache: · line 112
Quoted 3 lines
arch/arm/cpu/arm926ejs/start.S +1
-1#endif - mov pc, lr /* back to my caller */ + bx lr /* back to my caller */
This change seems unrelated. Yes, bx is the preferred instruction, but using mov here isn't breaking anything. If it bothers you, feel free to make a separate patch fixing all the instances of mov to the pc register, not just this one.
Signature · 1 line
Måns Rullgård
PATCH v2 1/2 arm: make arm926ejs startup code thumb compatible
Quoted 30 lines · Måns Rullgård
Quoted 25 lines · Klaus Goger <klaus.goger at theobroma-systems.com…
When building the mxs platform in thumb mode gcc generates code using the intra procedure call scratch register (ip/r12) for the calling the
lowlevel_initfunction. This modifies the lr inflush_dcachewhich causes u-boot proper to end in an endless loop.40002334: e1a0c00e mov ip, lr 40002338: eb00df4c bl 4003a070 <__lowlevel_init_from_arm> 4000233c: e1a0e00c mov lr, ip 40002340: e1a0f00e mov pc, lr[...]
4003a070<__lowlevel_init_from_arm>:4003a070:e59fc004ldr ip, [pc, #4] ;4003a07c
<__lowlevel_init_from_arm+0xc>4003a074: e08fc00c add ip, pc, ip 4003a078: e12fff1c bx ip 4003a07c: fffc86cd .word 0xfffc86cdInstead of using the the ip/r12 register we use sl/r10 to preserve the link register.
According to "Procedure Call Standard for the ARM Architecture" by ARM subroutines have to preserve the contents of register r4-r8, r10, r11 and SP. So using r10 instead of r12 should be save.
Quoted 6 lines
Quoted 2 lines
Signed-off-by Klaus Goger <[email protected]>Signed-off-by Christoph Muellner <[email protected]>This problem isn't specific to Thumb mode. An ARM build would also break if the
lowlevel_initfunction happened to clobber r12, which it is permitted to do. It's just dumb luck that this hasn't happened yet.
True. Since we are in low-level code and are not called by any C-functions (we are startup code), using sl/r10 is ok.
Quoted 10 lines
Quoted 10 lines
---
Changes in v2:
- use bl instead of blx to calllowlevel_init
- remove mxs tag as it apply to all arm926ejs platforms
arch/arm/cpu/arm926ejs/start.S| 6 +++---1 file changed, 3 insertions(+), 3 deletions(-)
Quoted 16 lines
arch/arm/cpu/arm926ejs/start.S flush_dcache: · line 110
Quoted 13 lines
arch/arm/cpu/arm926ejs/start.S +2
-2@@ -105,9 +105,9 @@ flush_dcache: /* * Go setup Memory and board specific bits prior to relocation. */ - mov ip, lr /* perserve link reg across call */ + mov sl, lr /* perserve link reg across call */ bl lowlevel_init /* go setup pll,mux,memory */ - mov lr, ip /* restore link */ + mov lr, sl /* restore link */I prefer to use plain register names (r10) rather than the aliases (sl) when not using them for the special functions indicated by the latter.
Ok, will change that.
Quoted 9 lines
arch/arm/cpu/arm926ejs/start.S flush_dcache: · line 112
Quoted 4 lines
arch/arm/cpu/arm926ejs/start.S +1
-1#endif - mov pc, lr /* back to my caller */ + bx lr /* back to my caller */This change seems unrelated. Yes, bx is the preferred instruction, but using mov here isn't breaking anything. If it bothers you, feel free to make a separate patch fixing all the instances of mov to the pc register, not just this one.
We’ll remove it from the patch.
Thanks,
Christoph
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 874 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180421/acafbc3b/attachment.sig>
PATCH v2 2/2 arm: Make arch specific memcpy thumb-safe.
In reply to Klaus GogerThe current arch implementation of memcpy cannot be called from thumb code, because it does not use bx instructions on return. This patch addresses that. Note, that this patch does not touch the hot loop of memcpy, so performance is not affected.
Tested on MXS (arm926ejs) with and without thumb-mode enabled.
---
Changes in v2: None
arch/arm/lib/memcpy.S | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
arch/arm/lib/memcpy.S +5 -3
@@ -62,7 +62,7 @@ #endif ENTRY(memcpy) cmp r0, r1 - moveq pc, lr + bxeq lr enter r4, lr
@@ -150,7 +150,8 @@ ENTRY(memcpy) str1b r0, r4, cs, abort=21f str1b r0, ip, cs, abort=21f - exit r4, pc + exit r4, lr + bx lr 9: rsb ip, ip, #4 cmp ip, #2
@@ -259,7 +260,8 @@ ENTRY(memcpy) .endm .macro copy_abort_end - ldmfd sp!, {r4, pc} + ldmfd sp!, {r4, lr} + bx lr .endm ENDPROC(memcpy)
Signature · 1 line
2.11.0
PATCH v2 2/2 arm: Make arch specific memcpy thumb-safe.
Quoted 9 lines · Klaus Goger
The current arch implementation of memcpy cannot be called from thumb code, because it does not use bx instructions on return. This patch addresses that. Note, that this patch does not touch the hot loop of memcpy, so performance is not affected.
Tested on MXS (arm926ejs) with and without thumb-mode enabled.
Signed-off-by Klaus Goger <[email protected]>Signed-off-by Christoph Muellner <[email protected]>
There are many more instances of mov to pc that ought to be fixed too. Why not do them all at once rather than picking them off one by one as they happen to break things?
Signature · 1 line
Måns Rullgård
PATCH v2 2/2 arm: Make arch specific memcpy thumb-safe.
Quoted 10 lines · Måns Rullgård
Quoted 5 lines · Klaus Goger
The current arch implementation of memcpy cannot be called from thumb code, because it does not use bx instructions on return. This patch addresses that. Note, that this patch does not touch the hot loop of memcpy, so performance is not affected.
Quoted 8 lines
Quoted 4 lines
Tested on MXS (arm926ejs) with and without thumb-mode enabled.
Signed-off-by Klaus Goger <[email protected]>Signed-off-by Christoph Muellner <[email protected]>There are many more instances of mov to pc that ought to be fixed too. Why not do them all at once rather than picking them off one by one as they happen to break things?
I could not find exit points within memcpy other than those which we fixed. The many other mov pc, $lr instructions are just branches within memcpy. Am I overseeing anything here?
Thanks,
Christoph
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 874 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180421/b00e3e48/attachment.sig>
PATCH v2 2/2 arm: Make arch specific memcpy thumb-safe.
Quoted 14 lines · Christoph Müllner
Quoted 14 lines · Måns Rullgård
Quoted 9 lines · Klaus Goger
The current arch implementation of memcpy cannot be called from thumb code, because it does not use bx instructions on return. This patch addresses that. Note, that this patch does not touch the hot loop of memcpy, so performance is not affected.
Tested on MXS (arm926ejs) with and without thumb-mode enabled.
Signed-off-by Klaus Goger <[email protected]>Signed-off-by Christoph Muellner <[email protected]>
Quoted 7 lines
Quoted 3 lines
There are many more instances of mov to pc that ought to be fixed too. Why not do them all at once rather than picking them off one by one as they happen to break things?
I could not find exit points within memcpy other than those which we fixed. The many other mov pc, $lr instructions are just branches within memcpy. Am I overseeing anything here?
I meant elsewhere, not just in memcpy.
Signature · 1 line
Måns Rullgård
PATCH v2 2/2 arm: Make arch specific memcpy thumb-safe.
Quoted 23 lines · Måns Rullgård
Quoted 18 lines · Christoph Müllner <christoph.muellner at theobroma-systems.com…
Quoted 17 lines · Måns Rullgård
Quoted 9 lines · Klaus Goger
The current arch implementation of memcpy cannot be called from thumb code, because it does not use bx instructions on return. This patch addresses that. Note, that this patch does not touch the hot loop of memcpy, so performance is not affected.
Tested on MXS (arm926ejs) with and without thumb-mode enabled.
Signed-off-by Klaus Goger <[email protected]>Signed-off-by Christoph Muellner <[email protected]>There are many more instances of mov to pc that ought to be fixed too. Why not do them all at once rather than picking them off one by one as they happen to break things?
Quoted 5 lines
Quoted 3 lines
I could not find exit points within memcpy other than those which we fixed. The many other mov pc, $lr instructions are just branches within memcpy. Am I overseeing anything here?
I meant elsewhere, not just in memcpy.
We just bumped in to the start.S and the memcpy code when running with C-code compiled to thumb. We were able to boot a Linux kernel with those changes. Why should we touch code, which we don’t use (and therefore don’t test)?
Background is, that we have to meet restricted size limitations for the resulting U-Boot image. Turning off all not-required features was not enough, so we had the idea to compile C-code to thumb (which U-Boot offers via Kconfig option). This did the trick and we wanted to share what’s needed to get this configuration up and running.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 874 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180421/53b552bd/attachment.sig>