[U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds

[U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds

[Stefan Agner]

Resynchronize memcpy/memset with kernel and build them explicitly
in Thumb2 mode (unified syntax). Those assembler files can be
built and linked in ARM mode too, however when calling them from
Thumb2 built code, the stack got corrupted and the copy did not
succeed (the exact details have not been traced back). Hoever,
the Linux kernel builds those files in Thumb2 mode. Hence U-Boot
should build them in Thumb2 mode too when CONFIG_SYS_THUMB_BUILD
is set.

Also add implicit-it=always to AFLAGS when building for Thumb2.
Furthermore add no-warn-deprecated option to AFLAGS to rid of
deprecated unified syntax:
arch/arm/lib/memcpy.S: Assembler messages:
arch/arm/lib/memcpy.S:153: Warning: conditional infixes are deprecated in unified syntax
arch/arm/lib/memcpy.S:154: Warning: conditional infixes are deprecated in unified syntax
...

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
This patch grew out of a issue we have seen on our Vybrid based
module. My first proposal only covered memcpy. This patchset
synchronizes the memcpy/memset assembly with the kernel and makes
sure both files are built in Thumb mode.

This patchset is tested with Vybrid in ARM and Thumb2 mode.

 arch/arm/config.mk               |   5 +-
 arch/arm/include/asm/assembler.h |  33 ++++++++++--
 arch/arm/lib/memcpy.S            |  75 ++++++++++++++++++--------
 arch/arm/lib/memset.S            | 112 ++++++++++++++++++++-------------------
 4 files changed, 141 insertions(+), 84 deletions(-)

diff --git a/arch/arm/config.mk b/arch/arm/config.mk
index f0eafd6..7336455 100644
--- a/arch/arm/config.mk
+++ b/arch/arm/config.mk
@@ -26,7 +26,10 @@ PLATFORM_CPPFLAGS += -D__ARM__
 
 # Choose between ARM/Thumb instruction sets
 ifeq ($(CONFIG_SYS_THUMB_BUILD),y)
-PF_CPPFLAGS_ARM := $(call cc-option, -mthumb -mthumb-interwork,\
+AFLAGS_AUTOIT	:= $(call as-option,-Wa$(comma)-mimplicit-it=always,-Wa$(comma)-mauto-it)
+AFLAGS_NOWARN	:=$(call as-option,-Wa$(comma)-mno-warn-deprecated,-Wa$(comma)-W)
+PF_CPPFLAGS_ARM := $(AFLAGS_AUTOIT) $(AFLAGS_NOWARN) \
+			$(call cc-option, -mthumb -mthumb-interwork,\
 			$(call cc-option,-marm,)\
 			$(call cc-option,-mno-thumb-interwork,)\
 		)
diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index 5e4789b..11b80fb 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -14,12 +14,14 @@
  *  assembler source.
  */
 
+#include <config.h>
+
 /*
  * Endian independent macros for shifting bytes within registers.
  */
 #ifndef __ARMEB__
-#define pull		lsr
-#define push		lsl
+#define lspull		lsr
+#define lspush		lsl
 #define get_byte_0	lsl #0
 #define get_byte_1	lsr #8
 #define get_byte_2	lsr #16
@@ -29,8 +31,8 @@
 #define put_byte_2	lsl #16
 #define put_byte_3	lsl #24
 #else
-#define pull		lsl
-#define push		lsr
+#define lspull		lsl
+#define lspush		lsr
 #define get_byte_0	lsr #24
 #define get_byte_1	lsr #16
 #define get_byte_2	lsr #8
@@ -54,7 +56,28 @@
 #define PLD(code...)
 #endif
 
+	.irp	c,,eq,ne,cs,cc,mi,pl,vs,vc,hi,ls,ge,lt,gt,le,hs,lo
+	.macro	ret\c, reg
+#if defined(__ARM_ARCH_5E__) || defined(__ARM_ARCH_5TE__)
+	mov\c	pc, \reg
+#else
+	.ifeqs	"\reg", "lr"
+	bx\c	\reg
+	.else
+	mov\c	pc, \reg
+	.endif
+#endif
+	.endm
+	.endr
+
 /*
- * Cache alligned
+ * Cache aligned, used for optimized memcpy/memset
+ * In the kernel this is only enabled for Feroceon CPU's...
+ * We disable it especially for Thumb builds since those instructions
+ * are not made in a Thumb ready way...
  */
+#ifdef CONFIG_SYS_THUMB_BUILD
+#define CALGN(code...)
+#else
 #define CALGN(code...) code
+#endif
diff --git a/arch/arm/lib/memcpy.S b/arch/arm/lib/memcpy.S
index f655256..c9424ba 100644
--- a/arch/arm/lib/memcpy.S
+++ b/arch/arm/lib/memcpy.S
@@ -10,9 +10,14 @@
  *  published by the Free Software Foundation.
  */
 
+#include <linux/linkage.h>
 #include <asm/assembler.h>
 
+#ifdef CONFIG_SYS_THUMB_BUILD
+#define W(instr)	instr.w
+#else
 #define W(instr)	instr
+#endif
 
 #define LDR1W_SHIFT	0
 #define STR1W_SHIFT	0
@@ -56,13 +61,16 @@
 	.text
 
 /* Prototype: void *memcpy(void *dest, const void *src, size_t n); */
-
-.globl memcpy
-memcpy:
-
+#ifdef CONFIG_SYS_THUMB_BUILD
+	.syntax unified
+	.thumb
+	.thumb_func
+#endif
+ENTRY(memcpy)
+/*
 		cmp	r0, r1
 		moveq	pc, lr
-
+*/
 		enter	r4, lr
 
 		subs	r2, r2, #4
@@ -193,24 +201,24 @@ memcpy:
 
 12:	PLD(	pld	[r1, #124]		)
 13:		ldr4w	r1, r4, r5, r6, r7, abort=19f
-		mov	r3, lr, pull #\pull
+		mov	r3, lr, lspull #\pull
 		subs	r2, r2, #32
 		ldr4w	r1, r8, r9, ip, lr, abort=19f
-		orr	r3, r3, r4, push #\push
-		mov	r4, r4, pull #\pull
-		orr	r4, r4, r5, push #\push
-		mov	r5, r5, pull #\pull
-		orr	r5, r5, r6, push #\push
-		mov	r6, r6, pull #\pull
-		orr	r6, r6, r7, push #\push
-		mov	r7, r7, pull #\pull
-		orr	r7, r7, r8, push #\push
-		mov	r8, r8, pull #\pull
-		orr	r8, r8, r9, push #\push
-		mov	r9, r9, pull #\pull
-		orr	r9, r9, ip, push #\push
-		mov	ip, ip, pull #\pull
-		orr	ip, ip, lr, push #\push
+		orr	r3, r3, r4, lspush #\push
+		mov	r4, r4, lspull #\pull
+		orr	r4, r4, r5, lspush #\push
+		mov	r5, r5, lspull #\pull
+		orr	r5, r5, r6, lspush #\push
+		mov	r6, r6, lspull #\pull
+		orr	r6, r6, r7, lspush #\push
+		mov	r7, r7, lspull #\pull
+		orr	r7, r7, r8, lspush #\push
+		mov	r8, r8, lspull #\pull
+		orr	r8, r8, r9, lspush #\push
+		mov	r9, r9, lspull #\pull
+		orr	r9, r9, ip, lspush #\push
+		mov	ip, ip, lspull #\pull
+		orr	ip, ip, lr, lspush #\push
 		str8w	r0, r3, r4, r5, r6, r7, r8, r9, ip, , abort=19f
 		bge	12b
 	PLD(	cmn	r2, #96			)
@@ -221,10 +229,10 @@ memcpy:
 14:		ands	ip, r2, #28
 		beq	16f
 
-15:		mov	r3, lr, pull #\pull
+15:		mov	r3, lr, lspull #\pull
 		ldr1w	r1, lr, abort=21f
 		subs	ip, ip, #4
-		orr	r3, r3, lr, push #\push
+		orr	r3, r3, lr, lspush #\push
 		str1w	r0, r3, abort=21f
 		bgt	15b
 	CALGN(	cmp	r2, #0			)
@@ -241,3 +249,24 @@ memcpy:
 17:		forward_copy_shift	pull=16	push=16
 
 18:		forward_copy_shift	pull=24	push=8
+
+
+/*
+ * Abort preamble and completion macros.
+ * If a fixup handler is required then those macros must surround it.
+ * It is assumed that the fixup code will handle the private part of
+ * the exit macro.
+ */
+
+	.macro	copy_abort_preamble
+19:	ldmfd	sp!, {r5 - r9}
+	b	21f
+20:	ldmfd	sp!, {r5 - r8}
+21:
+	.endm
+
+	.macro	copy_abort_end
+	ldmfd	sp!, {r4, pc}
+	.endm
+
+ENDPROC(memcpy)
diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
index 0cdf895..08810eb 100644
--- a/arch/arm/lib/memset.S
+++ b/arch/arm/lib/memset.S
@@ -9,32 +9,25 @@
  *
  *  ASM optimised string functions
  */
+#include <linux/linkage.h>
 #include <asm/assembler.h>
 
 	.text
 	.align	5
-	.word	0
 
-1:	subs	r2, r2, #4		@ 1 do we have enough
-	blt	5f			@ 1 bytes to align with?
-	cmp	r3, #2			@ 1
-	strltb	r1, [r0], #1		@ 1
-	strleb	r1, [r0], #1		@ 1
-	strb	r1, [r0], #1		@ 1
-	add	r2, r2, r3		@ 1 (r2 = r2 - (4 - r3))
-/*
- * The pointer is now aligned and the length is adjusted.  Try doing the
- * memset again.
- */
-
-.globl memset
-memset:
+#ifdef CONFIG_SYS_THUMB_BUILD
+	.syntax unified
+	.thumb
+	.thumb_func
+#endif
+ENTRY(memset)
 	ands	r3, r0, #3		@ 1 unaligned?
-	bne	1b			@ 1
+	mov	ip, r0			@ preserve r0 as return value
+	bne	6f			@ 1
 /*
- * we know that the pointer in r0 is aligned to a word boundary.
+ * we know that the pointer in ip is aligned to a word boundary.
  */
-	orr	r1, r1, r1, lsl #8
+1:	orr	r1, r1, r1, lsl #8
 	orr	r1, r1, r1, lsl #16
 	mov	r3, r1
 	cmp	r2, #16
@@ -43,29 +36,28 @@ memset:
 #if ! CALGN(1)+0
 
 /*
- * We need an extra register for this loop - save the return address and
- * use the LR
+ * We need 2 extra registers for this loop - use r8 and the LR
  */
-	str	lr, [sp, #-4]!
-	mov	ip, r1
+	stmfd	sp!, {r8, lr}
+	mov	r8, r1
 	mov	lr, r1
 
 2:	subs	r2, r2, #64
-	stmgeia	r0!, {r1, r3, ip, lr}	@ 64 bytes at a time.
-	stmgeia	r0!, {r1, r3, ip, lr}
-	stmgeia	r0!, {r1, r3, ip, lr}
-	stmgeia	r0!, {r1, r3, ip, lr}
+	stmgeia	ip!, {r1, r3, r8, lr}	@ 64 bytes at a time.
+	stmgeia	ip!, {r1, r3, r8, lr}
+	stmgeia	ip!, {r1, r3, r8, lr}
+	stmgeia	ip!, {r1, r3, r8, lr}
 	bgt	2b
-	ldmeqfd	sp!, {pc}		@ Now <64 bytes to go.
+	ldmeqfd	sp!, {r8, pc}		@ Now <64 bytes to go.
 /*
  * No need to correct the count; we're only testing bits from now on
  */
 	tst	r2, #32
-	stmneia	r0!, {r1, r3, ip, lr}
-	stmneia	r0!, {r1, r3, ip, lr}
+	stmneia	ip!, {r1, r3, r8, lr}
+	stmneia	ip!, {r1, r3, r8, lr}
 	tst	r2, #16
-	stmneia	r0!, {r1, r3, ip, lr}
-	ldr	lr, [sp], #4
+	stmneia	ip!, {r1, r3, r8, lr}
+	ldmfd	sp!, {r8, lr}
 
 #else
 
@@ -74,53 +66,63 @@ memset:
  * whole cache lines@once.
  */
 
-	stmfd	sp!, {r4-r7, lr}
+	stmfd	sp!, {r4-r8, lr}
 	mov	r4, r1
 	mov	r5, r1
 	mov	r6, r1
 	mov	r7, r1
-	mov	ip, r1
+	mov	r8, r1
 	mov	lr, r1
 
 	cmp	r2, #96
-	tstgt	r0, #31
+	tstgt	ip, #31
 	ble	3f
 
-	and	ip, r0, #31
-	rsb	ip, ip, #32
-	sub	r2, r2, ip
-	movs	ip, ip, lsl #(32 - 4)
-	stmcsia	r0!, {r4, r5, r6, r7}
-	stmmiia	r0!, {r4, r5}
-	tst	ip, #(1 << 30)
-	mov	ip, r1
-	strne	r1, [r0], #4
+	and	r8, ip, #31
+	rsb	r8, r8, #32
+	sub	r2, r2, r8
+	movs	r8, r8, lsl #(32 - 4)
+	stmcsia	ip!, {r4, r5, r6, r7}
+	stmmiia	ip!, {r4, r5}
+	tst	r8, #(1 << 30)
+	mov	r8, r1
+	strne	r1, [ip], #4
 
 3:	subs	r2, r2, #64
-	stmgeia	r0!, {r1, r3-r7, ip, lr}
-	stmgeia	r0!, {r1, r3-r7, ip, lr}
+	stmgeia	ip!, {r1, r3-r8, lr}
+	stmgeia	ip!, {r1, r3-r8, lr}
 	bgt	3b
-	ldmeqfd	sp!, {r4-r7, pc}
+	ldmeqfd	sp!, {r4-r8, pc}
 
 	tst	r2, #32
-	stmneia	r0!, {r1, r3-r7, ip, lr}
+	stmneia	ip!, {r1, r3-r8, lr}
 	tst	r2, #16
-	stmneia	r0!, {r4-r7}
-	ldmfd	sp!, {r4-r7, lr}
+	stmneia	ip!, {r4-r7}
+	ldmfd	sp!, {r4-r8, lr}
 
 #endif
 
 4:	tst	r2, #8
-	stmneia	r0!, {r1, r3}
+	stmneia	ip!, {r1, r3}
 	tst	r2, #4
-	strne	r1, [r0], #4
+	strne	r1, [ip], #4
 /*
  * When we get here, we've got less than 4 bytes to zero.  We
  * may have an unaligned pointer as well.
  */
 5:	tst	r2, #2
-	strneb	r1, [r0], #1
-	strneb	r1, [r0], #1
+	strneb	r1, [ip], #1
+	strneb	r1, [ip], #1
 	tst	r2, #1
-	strneb	r1, [r0], #1
-	mov	pc, lr
+	strneb	r1, [ip], #1
+	ret	lr
+
+6:	subs	r2, r2, #4		@ 1 do we have enough
+	blt	5b			@ 1 bytes to align with?
+	cmp	r3, #2			@ 1
+	strltb	r1, [ip], #1		@ 1
+	strleb	r1, [ip], #1		@ 1
+	strb	r1, [ip], #1		@ 1
+	add	r2, r2, r3		@ 1 (r2 = r2 - (4 - r3))
+	b	1b
+ENDPROC(memset)
-- 
2.1.3

[U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds

[Jeroen Hofstee]

Hello Stefan,

On 19-11-14 15:16, Stefan Agner wrote:
> Resynchronize memcpy/memset with kernel and build them explicitly
> in Thumb2 mode (unified syntax). Those assembler files can be
> built and linked in ARM mode too, however when calling them from
> Thumb2 built code, the stack got corrupted and the copy did not
> succeed (the exact details have not been traced back). Hoever,
> the Linux kernel builds those files in Thumb2 mode. Hence U-Boot
> should build them in Thumb2 mode too when CONFIG_SYS_THUMB_BUILD
> is set.
>
> Also add implicit-it=always to AFLAGS when building for Thumb2.
> Furthermore add no-warn-deprecated option to AFLAGS to rid of
> deprecated unified syntax:


> arch/arm/lib/memcpy.S: Assembler messages:
> arch/arm/lib/memcpy.S:153: Warning: conditional infixes are deprecated in unified syntax
> arch/arm/lib/memcpy.S:154: Warning: conditional infixes are deprecated in unified syntax
> ...

Any particular reason not to fix these warnings instead? It Is
just a matter of making the conditionals suffixes. [I guess
you can even disassemble to file to get the UAL represenation].
Or are there gas version around which actually choke on that?

Regards,
Jeroen

[U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds

[Stefan Agner]

Hi Jeroen,

On 2014-11-20 10:21, Jeroen Hofstee wrote:
> Hello Stefan,
> 
> On 19-11-14 15:16, Stefan Agner wrote:
>> Resynchronize memcpy/memset with kernel and build them explicitly
>> in Thumb2 mode (unified syntax). Those assembler files can be
>> built and linked in ARM mode too, however when calling them from
>> Thumb2 built code, the stack got corrupted and the copy did not
>> succeed (the exact details have not been traced back). Hoever,
>> the Linux kernel builds those files in Thumb2 mode. Hence U-Boot
>> should build them in Thumb2 mode too when CONFIG_SYS_THUMB_BUILD
>> is set.
>>
>> Also add implicit-it=always to AFLAGS when building for Thumb2.
>> Furthermore add no-warn-deprecated option to AFLAGS to rid of
>> deprecated unified syntax:
> 
> 
>> arch/arm/lib/memcpy.S: Assembler messages:
>> arch/arm/lib/memcpy.S:153: Warning: conditional infixes are deprecated in unified syntax
>> arch/arm/lib/memcpy.S:154: Warning: conditional infixes are deprecated in unified syntax
>> ...
> 
> Any particular reason not to fix these warnings instead? It Is
> just a matter of making the conditionals suffixes. [I guess
> you can even disassemble to file to get the UAL represenation].
> Or are there gas version around which actually choke on that?

No particular reason, I did not know how to fix this without digging
into it. Hence, after I discovered this, I checked why those warnings do
not happen for the kernel, then I applied just the AFLAGS the kernel is
using. I guess fixing the underlying issue is the better option, and
doing this also for the kernel would be the best way... Maybe the kernel
community also knows better why they choose to use the AFLAGS instead
(and if there are gas version which do have problems with a proper
fix)...

--
Stefan


> 
> Regards,
> Jeroen

[U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds

[Jeroen Hofstee]

Hello Stefan,

On 20-11-14 13:15, Stefan Agner wrote:
> Hi Jeroen,
>
> On 2014-11-20 10:21, Jeroen Hofstee wrote:
>> Hello Stefan,
>>
>> On 19-11-14 15:16, Stefan Agner wrote:
>>> Resynchronize memcpy/memset with kernel and build them explicitly
>>> in Thumb2 mode (unified syntax). Those assembler files can be
>>> built and linked in ARM mode too, however when calling them from
>>> Thumb2 built code, the stack got corrupted and the copy did not
>>> succeed (the exact details have not been traced back). Hoever,
>>> the Linux kernel builds those files in Thumb2 mode. Hence U-Boot
>>> should build them in Thumb2 mode too when CONFIG_SYS_THUMB_BUILD
>>> is set.
>>>
>>> Also add implicit-it=always to AFLAGS when building for Thumb2.
>>> Furthermore add no-warn-deprecated option to AFLAGS to rid of
>>> deprecated unified syntax:
>>
>>> arch/arm/lib/memcpy.S: Assembler messages:
>>> arch/arm/lib/memcpy.S:153: Warning: conditional infixes are deprecated in unified syntax
>>> arch/arm/lib/memcpy.S:154: Warning: conditional infixes are deprecated in unified syntax
>>> ...
>> Any particular reason not to fix these warnings instead? It Is
>> just a matter of making the conditionals suffixes. [I guess
>> you can even disassemble to file to get the UAL represenation].
>> Or are there gas version around which actually choke on that?
> No particular reason, I did not know how to fix this without digging
> into it. Hence, after I discovered this, I checked why those warnings do
> not happen for the kernel, then I applied just the AFLAGS the kernel is
> using. I guess fixing the underlying issue is the better option, and
> doing this also for the kernel would be the best way... Maybe the kernel
> community also knows better why they choose to use the AFLAGS instead
> (and if there are gas version which do have problems with a proper
> fix)...
>

for what it is worth, I have attached patch hanging around, but I never
actually tested it. It is for the current version.

Regards,
Jeroen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-arm-memset-make-it-UAL-compliant.patch
Type: text/x-patch
Size: 2546 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141120/a1ea6c16/attachment.bin>

[U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds

[Bill Pringlemeir]

> On 20-11-14 13:15, Stefan Agner wrote:

>> No particular reason, I did not know how to fix this without digging
>> into it. Hence, after I discovered this, I checked why those warnings
>> do not happen for the kernel, then I applied just the AFLAGS the
>> kernel is using. I guess fixing the underlying issue is the better
>> option, and doing this also for the kernel would be the best
>> way... Maybe the kernel community also knows better why they choose
>> to use the AFLAGS instead (and if there are gas version which do have
>> problems with a proper fix)...

On 20 Nov 2014, jeroen at myspectrum.nl wrote:

> for what it is worth, I have attached patch hanging around, but I
> never actually tested it. It is for the current version.

>> From c151254b3de49d8fccb69ab4f9442d884b9ff85c Mon Sep 17 00:00:00
>> 2001
> From: Jeroen Hofstee <jeroen@myspectrum.nl>
> Date: Thu, 20 Nov 2014 14:06:26 +0100
> Subject: [PATCH] arm: memset: make it UAL compliant

> ---
> arch/arm/lib/memset.S | 40 ++++++++++++++++++++--------------------
> 1 file changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
> index 0cdf895..4fe38f6 100644
> --- a/arch/arm/lib/memset.S
> +++ b/arch/arm/lib/memset.S
> @@ -18,8 +18,8 @@
> 1:	subs	r2, r2, #4		@ 1 do we have enough
> 	blt	5f			@ 1 bytes to align with?
> 	cmp	r3, #2			@ 1
> -	strltb	r1, [r0], #1		@ 1
> -	strleb	r1, [r0], #1		@ 1
> +	strblt	r1, [r0], #1		@ 1
> +	strble	r1, [r0], #1		@ 1

To test this, can we just use 'objdump'.  The hex codes should be
identical; there is only one encoding.  It should produce the same
binaries.  No need to run test-suites, etc.

Fwiw,
Bill.

[U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds

[Jeroen Hofstee]

Hi,

On 20-11-14 16:18, Bill Pringlemeir wrote:
>
>> ---
>> arch/arm/lib/memset.S | 40 ++++++++++++++++++++--------------------
>> 1 file changed, 20 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
>> index 0cdf895..4fe38f6 100644
>> --- a/arch/arm/lib/memset.S
>> +++ b/arch/arm/lib/memset.S
>> @@ -18,8 +18,8 @@
>> 1:	subs	r2, r2, #4		@ 1 do we have enough
>> 	blt	5f			@ 1 bytes to align with?
>> 	cmp	r3, #2			@ 1
>> -	strltb	r1, [r0], #1		@ 1
>> -	strleb	r1, [r0], #1		@ 1
>> +	strblt	r1, [r0], #1		@ 1
>> +	strble	r1, [r0], #1		@ 1
> To test this, can we just use 'objdump'.  The hex codes should be
> identical; there is only one encoding.  It should produce the same
> binaries.  No need to run test-suites, etc.
>

yes, I should be trivial to test (and find the trivial problem, with
the patch I attached). I am wondering though if all version of
gas accept the suffix notation... any idea?

Regards,
Jeroen

[U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds

[Bill Pringlemeir]

>>> ---
>>> arch/arm/lib/memset.S | 40 ++++++++++++++++++++--------------------
>>> 1 file changed, 20 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
>>> index 0cdf895..4fe38f6 100644
>>> --- a/arch/arm/lib/memset.S
>>> +++ b/arch/arm/lib/memset.S
>>>>> -18,8 +18,8 @@
>>> 1:	subs	r2, r2, #4		@ 1 do we have enough
>>> 	blt	5f			@ 1 bytes to align with?
>>> 	cmp	r3, #2			@ 1
>>> -	strltb	r1, [r0], #1		@ 1
>>> -	strleb	r1, [r0], #1		@ 1
>>> +	strblt	r1, [r0], #1		@ 1
>>> +	strble	r1, [r0], #1		@ 1

>> To test this, can we just use 'objdump'.  The hex codes should be
>> identical; there is only one encoding.  It should produce the same
>> binaries.  No need to run test-suites, etc.

On 20 Nov 2014, jeroen at myspectrum.nl wrote:

> yes, I should be trivial to test (and find the trivial problem, with
> the patch I attached). I am wondering though if all version of
> gas accept the suffix notation... any idea?

One part of the answer is here,

 https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=history;f=gas/config/tc-arm.c;hb=HEAD

The 'strCCb' version is definitely more popular in older ARM books.
Certainly there could be bugs and/or patched versions that make a
difference.  Probably it would be helpful to know what versions are
supported.

Back in 1999 it seems that the code at least tries to take conditions
anywhere,

 https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gas/config/tc-arm.c;hb=858f4ff6ff40a73f2a569fc8886157568f08c6db#l6099

I think it is most likely to result in a parse error if it wasn't
supported.  Any version since Thumb2/Unified (2003-2005?) was introduced
should be accepting this syntax with less issues.  Ie, it seems like a
better way forward.

Historical versions are here,

http://ftp.mirrorservice.org/sites/sourceware.org/pub/binutils/old-releases/

Who knows if some vendor patched things to mess something up?  Probably
grabbing an older 'gas' version and verifying it was the same binary
before/after the patch would probably be fair confirmation?  I don't
think you can 100% guarantee this doesn't break with some archaic
vendors gas.

Fwiw,
Bill Pringlemeir.

[U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds

[Jeroen Hofstee]

Hi,

On 20-11-14 19:21, Bill Pringlemeir wrote:
>>>> ---
>>>> arch/arm/lib/memset.S | 40 ++++++++++++++++++++--------------------
>>>> 1 file changed, 20 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
>>>> index 0cdf895..4fe38f6 100644
>>>> --- a/arch/arm/lib/memset.S
>>>> +++ b/arch/arm/lib/memset.S
>>>>>> -18,8 +18,8 @@
>>>> 1:	subs	r2, r2, #4		@ 1 do we have enough
>>>> 	blt	5f			@ 1 bytes to align with?
>>>> 	cmp	r3, #2			@ 1
>>>> -	strltb	r1, [r0], #1		@ 1
>>>> -	strleb	r1, [r0], #1		@ 1
>>>> +	strblt	r1, [r0], #1		@ 1
>>>> +	strble	r1, [r0], #1		@ 1
>>> To test this, can we just use 'objdump'.  The hex codes should be
>>> identical; there is only one encoding.  It should produce the same
>>> binaries.  No need to run test-suites, etc.
> On 20 Nov 2014, jeroen at myspectrum.nl wrote:
>
>> yes, I should be trivial to test (and find the trivial problem, with
>> the patch I attached). I am wondering though if all version of
>> gas accept the suffix notation... any idea?
> One part of the answer is here,
>
>   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=history;f=gas/config/tc-arm.c;hb=HEAD
>
> The 'strCCb' version is definitely more popular in older ARM books.
> Certainly there could be bugs and/or patched versions that make a
> difference.  Probably it would be helpful to know what versions are
> supported.
>
> Back in 1999 it seems that the code at least tries to take conditions
> anywhere,
>
>   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gas/config/tc-arm.c;hb=858f4ff6ff40a73f2a569fc8886157568f08c6db#l6099
>
> I think it is most likely to result in a parse error if it wasn't
> supported.  Any version since Thumb2/Unified (2003-2005?) was introduced
> should be accepting this syntax with less issues.  Ie, it seems like a
> better way forward.
>
> Historical versions are here,
>
> http://ftp.mirrorservice.org/sites/sourceware.org/pub/binutils/old-releases/
>
> Who knows if some vendor patched things to mess something up?  Probably
> grabbing an older 'gas' version and verifying it was the same binary
> before/after the patch would probably be fair confirmation?  I don't
> think you can 100% guarantee this doesn't break with some archaic
> vendors gas.

Ok thanks for digging that up, that doesn't sound like a problem
then. Stefan, can you check if you can actually fix the warnings
instead of suppressing them?

Regards,
Jeroen

[U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds

[Stefan Agner]

On 2014-11-20 20:14, Jeroen Hofstee wrote:
> Hi,
> 
> On 20-11-14 19:21, Bill Pringlemeir wrote:
>>>>> ---
>>>>> arch/arm/lib/memset.S | 40 ++++++++++++++++++++--------------------
>>>>> 1 file changed, 20 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
>>>>> index 0cdf895..4fe38f6 100644
>>>>> --- a/arch/arm/lib/memset.S
>>>>> +++ b/arch/arm/lib/memset.S
>>>>>>> -18,8 +18,8 @@
>>>>> 1:	subs	r2, r2, #4		@ 1 do we have enough
>>>>> 	blt	5f			@ 1 bytes to align with?
>>>>> 	cmp	r3, #2			@ 1
>>>>> -	strltb	r1, [r0], #1		@ 1
>>>>> -	strleb	r1, [r0], #1		@ 1
>>>>> +	strblt	r1, [r0], #1		@ 1
>>>>> +	strble	r1, [r0], #1		@ 1
>>>> To test this, can we just use 'objdump'.  The hex codes should be
>>>> identical; there is only one encoding.  It should produce the same
>>>> binaries.  No need to run test-suites, etc.
>> On 20 Nov 2014, jeroen at myspectrum.nl wrote:
>>
>>> yes, I should be trivial to test (and find the trivial problem, with
>>> the patch I attached). I am wondering though if all version of
>>> gas accept the suffix notation... any idea?
>> One part of the answer is here,
>>
>>   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=history;f=gas/config/tc-arm.c;hb=HEAD
>>
>> The 'strCCb' version is definitely more popular in older ARM books.
>> Certainly there could be bugs and/or patched versions that make a
>> difference.  Probably it would be helpful to know what versions are
>> supported.
>>
>> Back in 1999 it seems that the code at least tries to take conditions
>> anywhere,
>>
>>   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gas/config/tc-arm.c;hb=858f4ff6ff40a73f2a569fc8886157568f08c6db#l6099
>>
>> I think it is most likely to result in a parse error if it wasn't
>> supported.  Any version since Thumb2/Unified (2003-2005?) was introduced
>> should be accepting this syntax with less issues.  Ie, it seems like a
>> better way forward.
>>
>> Historical versions are here,
>>
>> http://ftp.mirrorservice.org/sites/sourceware.org/pub/binutils/old-releases/
>>
>> Who knows if some vendor patched things to mess something up?  Probably
>> grabbing an older 'gas' version and verifying it was the same binary
>> before/after the patch would probably be fair confirmation?  I don't
>> think you can 100% guarantee this doesn't break with some archaic
>> vendors gas.
> 
> Ok thanks for digging that up, that doesn't sound like a problem
> then. Stefan, can you check if you can actually fix the warnings
> instead of suppressing them?
> 

Ok, I could apply the changes from your patch and it fixed the warnings
in memset.S. However, when I build the file in ARM mode then (without
CONFIG_SYS_THUMB_BUILD set). I get this:

arch/arm/lib/memset.S: Assembler messages:
arch/arm/lib/memset.S:92: Error: bad instruction `stmiage
ip!,{r1,r3-r8,lr}'
arch/arm/lib/memset.S:93: Error: bad instruction `stmiage
ip!,{r1,r3-r8,lr}'
arch/arm/lib/memset.S:95: Error: bad instruction `ldmfdeq
sp!,{r4-r8,pc}'
arch/arm/lib/memset.S:98: Error: bad instruction `stmiane
ip!,{r1,r3-r8,lr}'
arch/arm/lib/memset.S:100: Error: bad instruction `stmiane ip!,{r4-r7}'
arch/arm/lib/memset.S:106: Error: bad instruction `stmiane ip!,{r1,r3}'
arch/arm/lib/memset.S:114: Error: bad instruction `strbne r1,[ip],#1'
arch/arm/lib/memset.S:115: Error: bad instruction `strbne r1,[ip],#1'
arch/arm/lib/memset.S:117: Error: bad instruction `strbne r1,[ip],#1'
arch/arm/lib/memset.S:123: Error: bad instruction `strblt r1,[ip],#1'
arch/arm/lib/memset.S:124: Error: bad instruction `strble r1,[ip],#1'

--
Stefan


> Regards,
> Jeroen

[U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds

[Albert ARIBAUD]

Hello Jeroen,

Adding Stefan as it seems he was dropped from the recipients list.

Plus :

On Thu, 20 Nov 2014 20:14:01 +0100, Jeroen Hofstee
<jeroen@myspectrum.nl> wrote:
> Hi,
> 
> On 20-11-14 19:21, Bill Pringlemeir wrote:
> >>>> ---
> >>>> arch/arm/lib/memset.S | 40 ++++++++++++++++++++--------------------
> >>>> 1 file changed, 20 insertions(+), 20 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
> >>>> index 0cdf895..4fe38f6 100644
> >>>> --- a/arch/arm/lib/memset.S
> >>>> +++ b/arch/arm/lib/memset.S
> >>>>>> -18,8 +18,8 @@
> >>>> 1:	subs	r2, r2, #4		@ 1 do we have enough
> >>>> 	blt	5f			@ 1 bytes to align with?
> >>>> 	cmp	r3, #2			@ 1
> >>>> -	strltb	r1, [r0], #1		@ 1
> >>>> -	strleb	r1, [r0], #1		@ 1
> >>>> +	strblt	r1, [r0], #1		@ 1
> >>>> +	strble	r1, [r0], #1		@ 1
> >>> To test this, can we just use 'objdump'.  The hex codes should be
> >>> identical; there is only one encoding.  It should produce the same
> >>> binaries.  No need to run test-suites, etc.
> > On 20 Nov 2014, jeroen at myspectrum.nl wrote:
> >
> >> yes, I should be trivial to test (and find the trivial problem, with
> >> the patch I attached). I am wondering though if all version of
> >> gas accept the suffix notation... any idea?
> > One part of the answer is here,
> >
> >   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=history;f=gas/config/tc-arm.c;hb=HEAD
> >
> > The 'strCCb' version is definitely more popular in older ARM books.
> > Certainly there could be bugs and/or patched versions that make a
> > difference.  Probably it would be helpful to know what versions are
> > supported.
> >
> > Back in 1999 it seems that the code at least tries to take conditions
> > anywhere,
> >
> >   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gas/config/tc-arm.c;hb=858f4ff6ff40a73f2a569fc8886157568f08c6db#l6099
> >
> > I think it is most likely to result in a parse error if it wasn't
> > supported.  Any version since Thumb2/Unified (2003-2005?) was introduced
> > should be accepting this syntax with less issues.  Ie, it seems like a
> > better way forward.
> >
> > Historical versions are here,
> >
> > http://ftp.mirrorservice.org/sites/sourceware.org/pub/binutils/old-releases/
> >
> > Who knows if some vendor patched things to mess something up?  Probably
> > grabbing an older 'gas' version and verifying it was the same binary
> > before/after the patch would probably be fair confirmation?  I don't
> > think you can 100% guarantee this doesn't break with some archaic
> > vendors gas.
> 
> Ok thanks for digging that up, that doesn't sound like a problem
> then. Stefan, can you check if you can actually fix the warnings
> instead of suppressing them?

(in the right thread this time) Stefan, can you also drop the
-mauto-it part?

> Regards,
> Jeroen

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] arm: make arch memset/memcpy to work with Thumb2 builds

[Bill Pringlemeir]

> On 2014-11-20 20:14, Jeroen Hofstee wrote:

>> Ok thanks for digging that up, that doesn't sound like a problem
>> then. Stefan, can you check if you can actually fix the warnings
>> instead of suppressing them?

On 21 Nov 2014, stefan at agner.ch wrote:

> Ok, I could apply the changes from your patch and it fixed the
> warnings in memset.S. However, when I build the file in ARM mode then
> (without CONFIG_SYS_THUMB_BUILD set). I get this:

> arch/arm/lib/memset.S: Assembler messages: arch/arm/lib/memset.S:92:
> Error: bad instruction `stmiage ip!,{r1,r3-r8,lr}'

I think you need '.syntax unified' if you want those in ARM mode.  I
guess you found that out too?  I see,

+	.syntax unified
+#ifdef CONFIG_SYS_THUMB_BUILD
+	.thumb
+	.thumb_func
+#endif

in '[PATCH v2] arm: build arch memset/memcpy in Thumb2 mode'; so this
solved it?

I think it is a very nice feature to have Thumb2 on Vybrid.  Many boot
devices may have limited bandwidth compared to the running system.
Thanks for your work.

Regards,
Bill Pringlemeir