Skip to contentSkip to conversation

ARM: omap[34]: Thumb-2 compatibility fixes

PATCH Superseded Dave Martin 3 participants 15 messages Feb 9, 2011 – Feb 15, 2011

Subsystems (3)

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

v4 cover ARM: omap[34]: Thumb-2 compatibility fixes

This set of patches, along with some other patches under discussion on alkml, should enable omap3 and omap4 kernels to be built with CONFIG_THUMB2_KERNEL.

This patch set builds on recent cleanup done by the omap maintainers.

All affected low-level code is now built in Thumb-2. At least some of this code definitely works, most features have been tested successfully. Further testing, especially of Thumb-2 behaviour, is still welcome.

Seems to work with CONFIG_SMP_ON_UP and CONFIG_THUMB2_KERNEL enabled on Beagle xM A2 and Panda A1.

Thanks also to Santosh Shilimkar <[email protected]> for his help with testing.

v3:
* make SMC instruction syntax more consistent
* remove do_wfi() in favour of the generic wfi() macro

v4:
* revert to the OMAP-specific do_wfi() implementation, pending discussion of how generic macros can/should be provided

Details below.

Cheers,
Dave

The patches can be found, along with a buildable working tree, in the following repo:

git://git.linaro.org/people/dmart/linux-2.6-arm.git

* arm/omap-thumb2: has the patches proposed here
* arm/omap-thumb2+merged: additionally has some patches cherry- picked from other trees which are needed in order for the patches on arm/omap-thumb2 to work usefully.
* dirty/arm/omap-thumb2+merged: buildable test tree, which adds 2 local patches to work around a toolchain bug.

A working kernel config for this tree is here:
http://people.linaro.org/~dmart/arm_omap-thumb2+v2_config :

CONFIG_SMP_ON_UP=y
CONFIG_THUMB2_KERNEL=y
CONFIG_SERIAL_OMAP=n (to avoid garbage on xM; for Panda use console=ttyS2)

(The config is derived from the linaro omap config and so turns on loads of modules -- don't feel you have to build them all...)

Cherry-picked patches originated from Russell's devel tree and Tony Lindgren's omap-testing tree:

http://ftp.arm.linux.org.uk/pub/linux/arm/kernel/git-cur/linux-2.6-arm.git devel

git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap-2.6.git omap-testing

This series also depends on one un-merged patch to generic-ise the wfi() macro:

git://git.linaro.org/people/dmart/linux-2.6-arm.git arm/wfi-macro

Dave Martin (5):
ARM: omap4: Provide do_wfi() for Thumb-2
ARM: omap4: Convert END() to ENDPROC() for correct linkage with
CONFIG_THUMB2_KERNEL
ARM: omap3: Remove hand-encoded SMC instructions
ARM: omap3: Thumb-2 compatibility for sram34xx.S
ARM: omap3: Thumb-2 compatibility for sleep34xx.S

 arch/arm/mach-omap2/include/mach/omap4-common.h |    4 ++
 arch/arm/mach-omap2/omap-headsmp.S              |    2 +-
 arch/arm/mach-omap2/omap44xx-smc.S              |    8 ++--
 arch/arm/mach-omap2/sleep34xx.S                 |   56 ++++++++++++++++-------
 arch/arm/mach-omap2/sram34xx.S                  |   28 ++++++++---
 5 files changed, 68 insertions(+), 30 deletions(-)
#1 lore raw

PATCH v4 1/5 ARM: omap4: Provide do_wfi() for Thumb-2

For CONFIG_THUMB2_KERNEL, the existing definition of do_wfi() will insert invalid code into the instruction stream.

Any assembler which can assemble Thumb-2 is guaranteed to accept the "wfi" mnemonic, so for the Thumb-2 case, just use the mnemonic.

The ARM case is left as-is.

---
 arch/arm/mach-omap2/include/mach/omap4-common.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
arch/arm/mach-omap2/include/mach/omap4-common.h +4 -0
index 5b0270b..de441c0 100644
@@ -17,8 +17,12 @@
  * wfi used in low power code. Directly opcode is used instead
  * of instruction to avoid mulit-omap build break
  */
+#ifdef CONFIG_THUMB2_KERNEL
+#define do_wfi() __asm__ __volatile__ ("wfi" : : : "memory")
+#else
 #define do_wfi()			\
 		__asm__ __volatile__ (".word	0xe320f003" : : : "memory")
+#endif
 
 #ifdef CONFIG_CACHE_L2X0
 extern void __iomem *l2cache_base;
Signature · 1 line
1.7.1
#2 lore raw

PATCH v4 1/5 ARM: omap4: Provide do_wfi() for Thumb-2

Quoted 18 lines · Dave Martin

For CONFIG_THUMB2_KERNEL, the existing definition of do_wfi() will insert invalid code into the instruction stream.

Any assembler which can assemble Thumb-2 is guaranteed to accept the "wfi" mnemonic, so for the Thumb-2 case, just use the mnemonic.

The ARM case is left as-is.

---
 arch/arm/mach-omap2/include/mach/omap4-common.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
arch/arm/mach-omap2/include/mach/omap4-common.h +0 -0
index 5b0270b..de441c0 100644

arch/arm/mach-omap2/include/mach/omap4-common.h line 18

Quoted 2 lines
arch/arm/mach-omap2/include/mach/omap4-common.h +0 -0
@@ -17,8 +17,12 @@
  * wfi used in low power code. Directly opcode is used instead
  * of instruction to avoid mulit-omap build break

Might be a good idea to fix the typo above while at it.

Quoted 12 lines
arch/arm/mach-omap2/include/mach/omap4-common.h +4 -0
+#ifdef CONFIG_THUMB2_KERNEL
+#define do_wfi() __asm__ __volatile__ ("wfi" : : : "memory")
+#else
 #define do_wfi()			\
 		__asm__ __volatile__ (".word	0xe320f003" : : : "memory")
+#endif
 
 #ifdef CONFIG_CACHE_L2X0
 extern void __iomem *l2cache_base;
Signature · 1 line
1.7.1
#3 lore raw

PATCH v4 2/5 ARM: omap4: Convert END() to ENDPROC() for correct linkage with CONFIG_THUMB2_KERNEL

In reply to Dave Martin

Code marked with ENTRY() also needs a matching ENDPROC() directive, in order to ensure that the type and instruction set of the symbol are correctly annotated.

ENDPROC() tags the affected symbol as a function symbol, which will ensure that link-time fixups don't accidentally switch to the wrong instruction set.

---
 arch/arm/mach-omap2/omap-headsmp.S |    2 +-
 arch/arm/mach-omap2/omap44xx-smc.S |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)
arch/arm/mach-omap2/omap-headsmp.S +1 -1
index 6ae937a..4ee6aec 100644
@@ -45,5 +45,5 @@ hold:	ldr	r12,=0x103
 	 * should now contain the SVC stack for this core
 	 */
 	b	secondary_startup
-END(omap_secondary_startup)
+ENDPROC(omap_secondary_startup)
 
arch/arm/mach-omap2/omap44xx-smc.S +4 -4
index 1980dc3..e69d37d 100644
@@ -29,7 +29,7 @@ ENTRY(omap_smc1)
 	dsb
 	smc	#0
 	ldmfd   sp!, {r2-r12, pc}
-END(omap_smc1)
+ENDPROC(omap_smc1)
 
 ENTRY(omap_modify_auxcoreboot0)
 	stmfd   sp!, {r1-r12, lr}
@@ -37,7 +37,7 @@ ENTRY(omap_modify_auxcoreboot0)
 	dsb
 	smc	#0
 	ldmfd   sp!, {r1-r12, pc}
-END(omap_modify_auxcoreboot0)
+ENDPROC(omap_modify_auxcoreboot0)
 
 ENTRY(omap_auxcoreboot_addr)
 	stmfd   sp!, {r2-r12, lr}
@@ -45,7 +45,7 @@ ENTRY(omap_auxcoreboot_addr)
 	dsb
 	smc	#0
 	ldmfd   sp!, {r2-r12, pc}
-END(omap_auxcoreboot_addr)
+ENDPROC(omap_auxcoreboot_addr)
 
 ENTRY(omap_read_auxcoreboot0)
 	stmfd   sp!, {r2-r12, lr}
@@ -54,4 +54,4 @@ ENTRY(omap_read_auxcoreboot0)
 	smc	#0
 	mov	r0, r0, lsr #9
 	ldmfd   sp!, {r2-r12, pc}
-END(omap_read_auxcoreboot0)
+ENDPROC(omap_read_auxcoreboot0)
Signature · 1 line
1.7.1
#4 lore raw

PATCH v4 3/5 ARM: omap3: Remove hand-encoded SMC instructions

In reply to Dave Martin

For various reasons, Linux now only officially supports being built with tools which are new enough to understand the SMC instruction.

Replacing the hand-encoded instructions when the mnemonic also allows for correct assembly in Thumb-2 (otherwise, the result is random data in the middle of the code).

The Makefile already ensures that this file is built with a high enough gcc -march= flag (armv7-a).

---
 arch/arm/mach-omap2/sleep34xx.S |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)
arch/arm/mach-omap2/sleep34xx.S +7 -7
index 98d8232..a05c348 100644
@@ -133,7 +133,7 @@ ENTRY(save_secure_ram_context)
 	mov	r6, #0xff
 	mcr	p15, 0, r0, c7, c10, 4	@ data write barrier
 	mcr	p15, 0, r0, c7, c10, 5	@ data memory barrier
-	.word	0xE1600071		@ call SMI monitor (smi #1)
+	smc	#1			@ call SMI monitor (smi #1)
 	nop
 	nop
 	nop
@@ -408,7 +408,7 @@ skipl2dis:
 	adr	r3, l2_inv_api_params	@ r3 points to dummy parameters
 	mcr	p15, 0, r0, c7, c10, 4	@ data write barrier
 	mcr	p15, 0, r0, c7, c10, 5	@ data memory barrier
-	.word	0xE1600071		@ call SMI monitor (smi #1)
+	smc	#1			@ call SMI monitor (smi #1)
 	/* Write to Aux control register to set some bits */
 	mov	r0, #42			@ set service ID for PPA
 	mov	r12, r0			@ copy secure Service ID in r12
@@ -419,7 +419,7 @@ skipl2dis:
 	ldr	r3, [r4, #0xBC]		@ r3 points to parameters
 	mcr	p15, 0, r0, c7, c10, 4	@ data write barrier
 	mcr	p15, 0, r0, c7, c10, 5	@ data memory barrier
-	.word	0xE1600071		@ call SMI monitor (smi #1)
+	smc	#1			@ call SMI monitor (smi #1)
 
 #ifdef CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE
 	/* Restore L2 aux control register */
@@ -434,7 +434,7 @@ skipl2dis:
 	adds	r3, r3, #8		@ r3 points to parameters
 	mcr	p15, 0, r0, c7, c10, 4	@ data write barrier
 	mcr	p15, 0, r0, c7, c10, 5	@ data memory barrier
-	.word	0xE1600071		@ call SMI monitor (smi #1)
+	smc	#1			@ call SMI monitor (smi #1)
 #endif
 	b	logic_l1_restore
 
@@ -443,18 +443,18 @@ l2_inv_api_params:
 l2_inv_gp:
 	/* Execute smi to invalidate L2 cache */
 	mov r12, #0x1			@ set up to invalidate L2
-	.word 0xE1600070		@ Call SMI monitor (smieq)
+	smc	#0			@ Call SMI monitor (smieq)
 	/* Write to Aux control register to set some bits */
 	ldr	r4, scratchpad_base
 	ldr	r3, [r4,#0xBC]
 	ldr	r0, [r3,#4]
 	mov	r12, #0x3
-	.word	0xE1600070		@ Call SMI monitor (smieq)
+	smc	#0			@ Call SMI monitor (smieq)
 	ldr	r4, scratchpad_base
 	ldr	r3, [r4,#0xBC]
 	ldr	r0, [r3,#12]
 	mov	r12, #0x2
-	.word	0xE1600070		@ Call SMI monitor (smieq)
+	smc	#0			@ Call SMI monitor (smieq)
 logic_l1_restore:
 	ldr	r1, l2dis_3630
 	cmp	r1, #0x1		@ Test if L2 re-enable needed on 3630
Signature · 1 line
1.7.1
#5 lore raw

PATCH v4 4/5 ARM: omap3: Thumb-2 compatibility for sram34xx.S

In reply to Dave Martin

* Remove deprecated/undefined PC-relative stores
* Add the required ENDPROC() directive for each ENTRY().
* .align before data words

---
 arch/arm/mach-omap2/sram34xx.S |   28 ++++++++++++++++++++--------
 1 files changed, 20 insertions(+), 8 deletions(-)
arch/arm/mach-omap2/sram34xx.S +20 -8
index 7f893a2..829d235 100644
@@ -116,24 +116,34 @@ ENTRY(omap3_sram_configure_core_dpll)
 
 					@ pull the extra args off the stack
 					@  and store them in SRAM
+
+@ PC-relative stores lead to undefined behaviour in Thumb-2: use a r7 as a
+@ base instead.
+@ Be careful not to clobber r7 when maintaing this file.
+ THUMB(	adr	r7, omap3_sram_configure_core_dpll			)
+	.macro strtext Rt:req, label:req
+ ARM(	str	\Rt, \label						)
+ THUMB(	str	\Rt, [r7, \label - omap3_sram_configure_core_dpll]	)
+	.endm
+
 	ldr	r4, [sp, #52]
-	str     r4, omap_sdrc_rfr_ctrl_0_val
+	strtext	r4, omap_sdrc_rfr_ctrl_0_val
 	ldr	r4, [sp, #56]
-	str     r4, omap_sdrc_actim_ctrl_a_0_val
+	strtext	r4, omap_sdrc_actim_ctrl_a_0_val
 	ldr	r4, [sp, #60]
-	str     r4, omap_sdrc_actim_ctrl_b_0_val
+	strtext	r4, omap_sdrc_actim_ctrl_b_0_val
 	ldr	r4, [sp, #64]
-	str     r4, omap_sdrc_mr_0_val
+	strtext	r4, omap_sdrc_mr_0_val
 	ldr	r4, [sp, #68]
-	str     r4, omap_sdrc_rfr_ctrl_1_val
+	strtext	r4, omap_sdrc_rfr_ctrl_1_val
 	cmp	r4, #0			@ if SDRC_RFR_CTRL_1 is 0,
 	beq	skip_cs1_params		@  do not use cs1 params
 	ldr	r4, [sp, #72]
-	str     r4, omap_sdrc_actim_ctrl_a_1_val
+	strtext	r4, omap_sdrc_actim_ctrl_a_1_val
 	ldr	r4, [sp, #76]
-	str     r4, omap_sdrc_actim_ctrl_b_1_val
+	strtext	r4, omap_sdrc_actim_ctrl_b_1_val
 	ldr	r4, [sp, #80]
-	str     r4, omap_sdrc_mr_1_val
+	strtext	r4, omap_sdrc_mr_1_val
 skip_cs1_params:
 	mrc	p15, 0, r8, c1, c0, 0	@ read ctrl register
 	bic	r10, r8, #0x800		@ clear Z-bit, disable branch prediction
@@ -271,6 +281,7 @@ skip_cs1_prog:
 	ldr	r12, [r11]		@ posted-write barrier for SDRC
 	bx	lr
 
+	.align
 omap3_sdrc_power:
 	.word OMAP34XX_SDRC_REGADDR(SDRC_POWER)
 omap3_cm_clksel1_pll:
@@ -319,6 +330,7 @@ omap3_sdrc_dlla_ctrl:
 	.word OMAP34XX_SDRC_REGADDR(SDRC_DLLA_CTRL)
 core_m2_mask_val:
 	.word 0x07FFFFFF
+ENDPROC(omap3_sram_configure_core_dpll)
 
 ENTRY(omap3_sram_configure_core_dpll_sz)
 	.word	. - omap3_sram_configure_core_dpll
Signature · 1 line
1.7.1
#6 lore raw

PATCH v4 5/5 ARM: omap3: Thumb-2 compatibility for sleep34xx.S

In reply to Dave Martin

* Use BSYM() to get the correct Thumb branch address for adr <Rd>, <label>

* Fix an out-of-range ADR when building for ARM

* Correctly call es3_sdrc_fix as Thumb when copied to SRAM.

* Remove deprecated/undefined PC-relative stores

* Add the required ENDPROC() directive for each ENTRY().

* .align before data words

---
 arch/arm/mach-omap2/sleep34xx.S |   42 +++++++++++++++++++++++++++++---------
 1 files changed, 32 insertions(+), 10 deletions(-)
arch/arm/mach-omap2/sleep34xx.S +32 -10
index a05c348..a204c78 100644
@@ -80,8 +80,10 @@
 /* Function call to get the restore pointer for resume from OFF */
 ENTRY(get_restore_pointer)
 	stmfd	sp!, {lr}	@ save registers on stack
-	adr	r0, restore
+	adr	r0, BSYM(restore)
 	ldmfd	sp!, {pc}	@ restore regs and return
+ENDPROC(get_restore_pointer)
+	.align
 ENTRY(get_restore_pointer_sz)
 	.word	. - get_restore_pointer
 
@@ -89,8 +91,10 @@ ENTRY(get_restore_pointer_sz)
 /* Function call to get the restore pointer for 3630 resume from OFF */
 ENTRY(get_omap3630_restore_pointer)
 	stmfd	sp!, {lr}	@ save registers on stack
-	adr	r0, restore_3630
+	adr	r0, BSYM(restore_3630)
 	ldmfd	sp!, {pc}	@ restore regs and return
+ENDPROC(get_omap3630_restore_pointer)
+	.align
 ENTRY(get_omap3630_restore_pointer_sz)
 	.word	. - get_omap3630_restore_pointer
 
@@ -98,8 +102,10 @@ ENTRY(get_omap3630_restore_pointer_sz)
 /* Function call to get the restore pointer for ES3 to resume from OFF */
 ENTRY(get_es3_restore_pointer)
 	stmfd	sp!, {lr}	@ save registers on stack
-	adr	r0, restore_es3
+	adr	r0, BSYM(restore_es3)
 	ldmfd	sp!, {pc}	@ restore regs and return
+ENDPROC(get_es3_restore_pointer)
+	.align
 ENTRY(get_es3_restore_pointer_sz)
 	.word	. - get_es3_restore_pointer
 
@@ -113,8 +119,11 @@ ENTRY(enable_omap3630_toggle_l2_on_restore)
 	stmfd	sp!, {lr}	@ save registers on stack
 	/* Setup so that we will disable and enable l2 */
 	mov	r1, #0x1
-	str	r1, l2dis_3630
+ ARM(	adrl	r2, l2dis_3630	)	@ may be out of range for adr in ARM
+ THUMB(	adr	r2, l2dis_3630	)	@ Thumb has more range, but not adrl
+	str	r1, [r2]
 	ldmfd	sp!, {pc}	@ restore regs and return
+ENDPROC(enable_omap3630_toggle_l2_on_restore)
 
 	.text
 /* Function to call rom code to save secure ram context */
@@ -139,12 +148,14 @@ ENTRY(save_secure_ram_context)
 	nop
 	nop
 	ldmfd	sp!, {r1-r12, pc}
+	.align
 sram_phy_addr_mask:
 	.word	SRAM_BASE_P
 high_mask:
 	.word	0xffff
 api_params:
 	.word	0x4, 0x0, 0x0, 0x1, 0x1
+ENDPROC(save_secure_ram_context)
 ENTRY(save_secure_ram_context_sz)
 	.word	. - save_secure_ram_context
 
@@ -279,8 +290,7 @@ clean_l2:
 	 *  - 'might' have to copy address, load and jump to it
 	 */
 	ldr	r1, kernel_flush
-	mov	lr, pc
-	bx	r1
+	blx	r1
 
 omap3_do_wfi:
 	ldr	r4, sdrc_power		@ read the SDRC_POWER register
@@ -346,8 +356,8 @@ restore_es3:
 	and	r4, r4, #0x3
 	cmp	r4, #0x0	@ Check if previous power state of CORE is OFF
 	bne	restore
-	adr	r0, es3_sdrc_fix
-	ldr	r1, sram_base
+	adr	r0, es3_sdrc_fix	@ Not using BSYM clears the Thumb bit.
+	ldr	r1, sram_base	@ Must be 8-byte aligned to preserve alignment.
 	ldr	r2, es3_sdrc_fix_sz
 	mov	r2, r2, ror #2
 copy_to_sram:
@@ -356,6 +366,7 @@ copy_to_sram:
 	subs	r2, r2, #0x1	@ num_words--
 	bne	copy_to_sram
 	ldr	r1, sram_base
+ THUMB(	orr	r1, r1, #BSYM(es3_sdrc_fix) & 1 )
 	blx	r1
 	b	restore
 
@@ -438,6 +449,7 @@ skipl2dis:
 #endif
 	b	logic_l1_restore
 
+	.align
 l2_inv_api_params:
 	.word	0x1, 0x00
 l2_inv_gp:
@@ -607,6 +619,7 @@ usettbr0:
 
 /* This function implements the erratum ID i443 WA, applies to 34xx >= ES3.0 */
 	.text
+	.align	3
 ENTRY(es3_sdrc_fix)
 	ldr	r4, sdrc_syscfg		@ get config addr
 	ldr	r5, [r4]		@ get value
@@ -634,6 +647,7 @@ ENTRY(es3_sdrc_fix)
 	str	r5, [r4]		@ kick off refreshes
 	bx	lr
 
+	.align
 sdrc_syscfg:
 	.word	SDRC_SYSCONFIG_P
 sdrc_mr_0:
@@ -648,6 +662,7 @@ sdrc_emr2_1:
 	.word	SDRC_EMR2_1_P
 sdrc_manual_1:
 	.word	SDRC_MANUAL_1_P
+ENDPROC(es3_sdrc_fix)
 ENTRY(es3_sdrc_fix_sz)
 	.word	. - es3_sdrc_fix
 
@@ -682,6 +697,10 @@ wait_sdrc_ready:
 	bic	r5, r5, #0x40
 	str	r5, [r4]
 
+@ PC-relative stores lead to undefined behaviour in Thumb-2: use a r7 as a
+@ base instead.
+@ Be careful not to clobber r7 when maintaing this code.
+
 is_dll_in_lock_mode:
 	/* Is dll in lock mode? */
 	ldr	r4, sdrc_dlla_ctrl
@@ -689,10 +708,11 @@ is_dll_in_lock_mode:
 	tst	r5, #0x4
 	bxne	lr			@ Return if locked
 	/* wait till dll locks */
+	adr	r7, kick_counter
 wait_dll_lock_timed:
 	ldr	r4, wait_dll_lock_counter
 	add	r4, r4, #1
-	str	r4, wait_dll_lock_counter
+	str	r4, [r7, #wait_dll_lock_counter - kick_counter]
 	ldr	r4, sdrc_dlla_status
 	/* Wait 20uS for lock */
 	mov	r6, #8
@@ -718,9 +738,10 @@ kick_dll:
 	dsb
 	ldr	r4, kick_counter
 	add	r4, r4, #1
-	str	r4, kick_counter
+	str	r4, [r7]		@ kick_counter
 	b	wait_dll_lock_timed
 
+	.align
 cm_idlest1_core:
 	.word	CM_IDLEST1_CORE_V
 cm_idlest_ckgen:
@@ -763,6 +784,7 @@ kick_counter:
 	.word	0
 wait_dll_lock_counter:
 	.word	0
+ENDPROC(omap34xx_cpu_suspend)
 
 ENTRY(omap34xx_cpu_suspend_sz)
 	.word	. - omap34xx_cpu_suspend
Signature · 1 line
1.7.1
#7 lore raw

v4 cover ARM: omap[34]: Thumb-2 compatibility fixes

In reply to Dave Martin

Hi Dave,

Quoted 3 lines · Dave Martin

This set of patches, along with some other patches under discussion on alkml, should enable omap3 and omap4 kernels to be built with CONFIG_THUMB2_KERNEL.

OK, I tried some more testing with your 'dirty' branch merged with my PM branch.

Compiled in ARM mode, everything worked as expected on my 3530/omap3evm, including off-mode (in suspend and idle). My 3630 (Zoom3) also can't do CORE off due to i583, but MPU, PER etc. all can hit off.

To rebuild in Thumb-2 mode, I disabled OMAP2 support and added Thumb-2 mode:

CONFIG_ARCH_OMAP2=n
CONFIG_THUMB2_KERNEL=y

then tested on 3530/omap3evm. Testing suspend/resume to retention seemed to work fine. However, enabling retention during idle[1] hung someplace (didn't debug further.)

I also tried off-mode, and suspend/resume to off didn't even work.

I didn't have time to debug this any further, so this is just to report raw test results.

Hope that helps,

Kevin

[1] Here's what's needed to attempt low-power states during idle

# UART timeouts: omap-serial (4th UART only on OMAP36xx and OMAP4) echo 5 > /sys/devices/platform/omap/omap_uart.0/sleep_timeout echo 5 > /sys/devices/platform/omap/omap_uart.1/sleep_timeout echo 5 > /sys/devices/platform/omap/omap_uart.2/sleep_timeout echo 5 > /sys/devices/platform/omap/omap_uart.3/sleep_timeout

# enable low-power states during idle
echo 1 > /debug/pm_debug/sleep_while_idle  

[2] to enable off-mode

echo 1 > /debug/pm_debug/enable_off_mode

#8 lore raw

v4 cover ARM: omap[34]: Thumb-2 compatibility fixes

Quoted 21 lines · Kevin Hilman

Hi Dave,

Quoted 3 lines · Dave Martin

This set of patches, along with some other patches under discussion on alkml, should enable omap3 and omap4 kernels to be built with CONFIG_THUMB2_KERNEL.

OK, I tried some more testing with your 'dirty' branch merged with my PM branch.

Compiled in ARM mode, everything worked as expected on my 3530/omap3evm, including off-mode (in suspend and idle). My 3630 (Zoom3) also can't do CORE off due to i583, but MPU, PER etc. all can hit off.

To rebuild in Thumb-2 mode, I disabled OMAP2 support and added Thumb-2 mode:

CONFIG_ARCH_OMAP2=n
CONFIG_THUMB2_KERNEL=y

Quoted 8 lines

then tested on 3530/omap3evm. Testing suspend/resume to retention seemed to work fine. However, enabling retention during idle[1] hung someplace (didn't debug further.)

I also tried off-mode, and suspend/resume to off didn't even work.

I didn't have time to debug this any further, so this is just to report raw test results.

Thanks, that's a useful step forward anyhow.

A possibility is that the Secure firmware can't cope with interoperating with Thumb-2 code in the kernel, so that wakeup entry points the SMC call sites may need to be ARM code.

If you get a moment, if would be interesting to see if this patch makes a difference ... in the meantime, I'll see if I can get my hands on an EVM from somewhere.

Cheers
---Dave

arch/arm/mach-omap2/sleep34xx.S +20 -0
index a204c78..ee1edb1 100644
@@ -32,6 +32,14 @@
 #include "sdrc.h"
 #include "control.h"
 
+#undef ARM
+#undef THUMB
+#undef BSYM
+#define ARM(x...) x
+#define THUMB(x...)
+#define BSYM(x) (x)
+	.arm
+
 /*
  * Registers access definitions
  */
@@ -289,8 +297,20 @@ clean_l2:
 	 *  - should be faster and will change with kernel
 	 *  - 'might' have to copy address, load and jump to it
 	 */
+#ifdef CONFIG_THUMB2_KERNEL
+	/* kernel is non-interworking : must do this from Thumb */
+	adr	r1, . + 1
+	bx	r1
+	.thumb
+#endif
 	ldr	r1, kernel_flush
 	blx	r1
+#ifdef CONFIG_THUMB2_KERNEL
+	.align
+	bx	pc
+	nop
+	.arm
+#endif
 
 omap3_do_wfi:
 	ldr	r4, sdrc_power		@ read the SDRC_POWER register
arch/arm/mach-omap2/sram34xx.S +8 -0
index 829d235..64faab8 100644
@@ -34,6 +34,14 @@
 #include "sdrc.h"
 #include "cm2xxx_3xxx.h"
 
+#undef ARM
+#undef THUMB
+#undef BSYM
+#define ARM(x...) x
+#define THUMB(x...)
+#define BSYM(x) (x)
+	.arm
+
 	.text
 
 /* r1 parameters */
Signature · 1 line
1.7.1
#9 lore raw

v4 cover ARM: omap[34]: Thumb-2 compatibility fixes

arch/arm/mach-omap2/sleep34xx.S clean_l2: · line 306

Quoted 11 lines · Dave Martin
arch/arm/mach-omap2/sleep34xx.S +6 -0
@@ -289,8 +297,20 @@ clean_l2:
 	 *  - should be faster and will change with kernel
 	 *  - 'might' have to copy address, load and jump to it
 	 */
+#ifdef CONFIG_THUMB2_KERNEL
+	/* kernel is non-interworking : must do this from Thumb */
+	adr	r1, . + 1
+	bx	r1
+	.thumb
+#endif
 	ldr	r1, kernel_flush

Didn't you mean this instead:

	/* kernel is non-interworking : must do this from Thumb */
	adr	r1, 1f + 1
	bx	r1
	.thumb
1:	ldr	r1, kernel_flush
	...

?

arch/arm/mach-omap2/sleep34xx.S clean_l2: · line 312

Quoted 6 lines
arch/arm/mach-omap2/sleep34xx.S +5 -0
 	blx	r1
+#ifdef CONFIG_THUMB2_KERNEL
+	.align
+	bx	pc
+	nop
+	.arm

Also here, the .align has the potential to introduce a zero halfword in the instruction stream before the bx. What about:

	adr	r3, 1f
	bx	r3
	.align
	.arm

1: ...

Nicolas

#10 lore raw

v4 cover ARM: omap[34]: Thumb-2 compatibility fixes

Quoted 16 lines · Nicolas Pitre

Review clean_l2: · line 306

Quoted 11 lines · Dave Martin
(quoted diff) +6 -0
@@ -289,8 +297,20 @@ clean_l2:
 	 *  - should be faster and will change with kernel
 	 *  - 'might' have to copy address, load and jump to it
 	 */
+#ifdef CONFIG_THUMB2_KERNEL
+	/* kernel is non-interworking : must do this from Thumb */
+	adr	r1, . + 1
+	bx	r1
+	.thumb
+#endif
 	ldr	r1, kernel_flush

Didn't you mean this instead:

Quoted 6 lines
	/* kernel is non-interworking : must do this from Thumb */
	adr	r1, 1f + 1
	bx	r1
	.thumb
1:	ldr	r1, kernel_flush
	...

Note that this is intended as an experimental hack, not a real patch (apologies if I didn't make that clear...)

Well, actually I meant "add r1, pc, #1" ... which means I was too busy trying to be clever... oops!

That is of course exactly equivalent to your code...

Quoted 13 lines

?

Review clean_l2: · line 312

Quoted 6 lines
(quoted diff) +5 -0
 	blx	r1
+#ifdef CONFIG_THUMB2_KERNEL
+	.align
+	bx	pc
+	nop
+	.arm

Also here, the .align has the potential to introduce a zero halfword in the instruction stream before the bx. What about:

Quoted 5 lines
	adr	r3, 1f
	bx	r3
	.align
	.arm

1: ...

.align inserts a 16-bit nop when misaligned in Thumb in a text section, and a word-aligned bx pc is a specific architecturally allowed way to do an inline switch to ARM. The linker uses this trick for PLT veneers etc.

A nicer fix for doing this sort of call from low-level code which might be ARM is to convert arch/arm/mm/*-v7.S to use "bx lr" to return.

Generally, we can do this for all arches >= v5, without any incompatibility. However, since the need for it will be rare and it will generate patch noise for not much real benefit,
I haven't proposed this.

Updated patch below.

Cheers
---Dave

arch/arm/mach-omap2/sleep34xx.S +21 -1
index a204c78..6ae8a92 100644
@@ -32,6 +32,14 @@
 #include "sdrc.h"
 #include "control.h"
 
+#undef ARM
+#undef THUMB
+#undef BSYM
+#define ARM(x...) x
+#define THUMB(x...)
+#define BSYM(x) (x)
+	.arm
+
 /*
  * Registers access definitions
  */
@@ -289,8 +297,20 @@ clean_l2:
 	 *  - should be faster and will change with kernel
 	 *  - 'might' have to copy address, load and jump to it
 	 */
-	ldr	r1, kernel_flush
+#ifdef CONFIG_THUMB2_KERNEL
+	/* kernel is non-interworking : must do this from Thumb */
+	adr	r1, 1f + 1
+	bx	r1
+	.thumb
+#endif
+1:	ldr	r1, kernel_flush
 	blx	r1
+#ifdef CONFIG_THUMB2_KERNEL
+	.align
+	bx	pc
+	nop
+	.arm
+#endif
 
 omap3_do_wfi:
 	ldr	r4, sdrc_power		@ read the SDRC_POWER register
arch/arm/mach-omap2/sram34xx.S +8 -0
index 829d235..64faab8 100644
@@ -34,6 +34,14 @@
 #include "sdrc.h"
 #include "cm2xxx_3xxx.h"
 
+#undef ARM
+#undef THUMB
+#undef BSYM
+#define ARM(x...) x
+#define THUMB(x...)
+#define BSYM(x) (x)
+	.arm
+
 	.text
 
 /* r1 parameters */
Signature · 1 line
1.7.1
#11 lore raw

v4 cover ARM: omap[34]: Thumb-2 compatibility fixes

Quoted 10 lines · Dave Martin
Quoted 9 lines · Nicolas Pitre

arch/arm/mach-omap2/sleep34xx.S clean_l2: · line 312

Quoted 5 lines
arch/arm/mach-omap2/sleep34xx.S +5 -0
+#ifdef CONFIG_THUMB2_KERNEL
+	.align
+	bx	pc
+	nop
+	.arm

Also here, the .align has the potential to introduce a zero halfword in the instruction stream before the bx. What about:

Quoted 7 lines
Quoted 5 lines
	adr	r3, 1f
	bx	r3
	.align
	.arm

1: ...

.align inserts a 16-bit nop when misaligned in Thumb in a text section,

Ah, OK then. I didn't know about that. In ARM mode the all-zero bits decode to an instruction that sort of does nothing, but this isn't an eleguant nop, and that's what .align (used to?) insert as padding.

Quoted 3 lines

and a word-aligned bx pc is a specific architecturally allowed way to do an inline switch to ARM. The linker uses this trick for PLT veneers etc.

Yep. It's just that with my suggestion the bx wasn't necessarily aligned.

Quoted 7 lines

A nicer fix for doing this sort of call from low-level code which might be ARM is to convert arch/arm/mm/*-v7.S to use "bx lr" to return.

Generally, we can do this for all arches >= v5, without any incompatibility. However, since the need for it will be rare and it will generate patch noise for not much real benefit,
I haven't proposed this.

This can be done in those places where this might be needed without having to convert them all.

Nicolas

#12 lore raw

v4 cover ARM: omap[34]: Thumb-2 compatibility fixes

In reply to Dave Martin

Hi Dave,

Quoted 59 lines · Dave Martin
Quoted 16 lines · Nicolas Pitre

arch/arm/mach-omap2/sleep34xx.S clean_l2: · line 306

Quoted 11 lines · Dave Martin
arch/arm/mach-omap2/sleep34xx.S +6 -0
@@ -289,8 +297,20 @@ clean_l2:
 	 *  - should be faster and will change with kernel
 	 *  - 'might' have to copy address, load and jump to it
 	 */
+#ifdef CONFIG_THUMB2_KERNEL
+	/* kernel is non-interworking : must do this from Thumb */
+	adr	r1, . + 1
+	bx	r1
+	.thumb
+#endif
 	ldr	r1, kernel_flush

Didn't you mean this instead:

Quoted 6 lines
	/* kernel is non-interworking : must do this from Thumb */
	adr	r1, 1f + 1
	bx	r1
	.thumb
1:	ldr	r1, kernel_flush
	...

Note that this is intended as an experimental hack, not a real patch (apologies if I didn't make that clear...)

Well, actually I meant "add r1, pc, #1" ... which means I was too busy trying to be clever... oops!

That is of course exactly equivalent to your code...

Quoted 13 lines

?

arch/arm/mach-omap2/sleep34xx.S clean_l2: · line 312

Quoted 6 lines
arch/arm/mach-omap2/sleep34xx.S +5 -0
 	blx	r1
+#ifdef CONFIG_THUMB2_KERNEL
+	.align
+	bx	pc
+	nop
+	.arm

Also here, the .align has the potential to introduce a zero halfword in the instruction stream before the bx. What about:

Quoted 5 lines
	adr	r3, 1f
	bx	r3
	.align
	.arm

1: ...

.align inserts a 16-bit nop when misaligned in Thumb in a text section, and a word-aligned bx pc is a specific architecturally allowed way to do an inline switch to ARM. The linker uses this trick for PLT veneers etc.

A nicer fix for doing this sort of call from low-level code which might be ARM is to convert arch/arm/mm/*-v7.S to use "bx lr" to return.

Quoted 6 lines

Generally, we can do this for all arches >= v5, without any incompatibility. However, since the need for it will be rare and it will generate patch noise for not much real benefit,
I haven't proposed this.

Updated patch below.

I tested the updated patch on top of your "dirty" branch I tested with last week, and now see off-mode working just fine.

Kevin

Quoted 63 lines

Cheers
---Dave

arch/arm/mach-omap2/sleep34xx.S +21 -1
index a204c78..6ae8a92 100644
@@ -32,6 +32,14 @@
 #include "sdrc.h"
 #include "control.h"
 
+#undef ARM
+#undef THUMB
+#undef BSYM
+#define ARM(x...) x
+#define THUMB(x...)
+#define BSYM(x) (x)
+	.arm
+
 /*
  * Registers access definitions
  */
@@ -289,8 +297,20 @@ clean_l2:
 	 *  - should be faster and will change with kernel
 	 *  - 'might' have to copy address, load and jump to it
 	 */
-	ldr	r1, kernel_flush
+#ifdef CONFIG_THUMB2_KERNEL
+	/* kernel is non-interworking : must do this from Thumb */
+	adr	r1, 1f + 1
+	bx	r1
+	.thumb
+#endif
+1:	ldr	r1, kernel_flush
 	blx	r1
+#ifdef CONFIG_THUMB2_KERNEL
+	.align
+	bx	pc
+	nop
+	.arm
+#endif
 
 omap3_do_wfi:
 	ldr	r4, sdrc_power		@ read the SDRC_POWER register
arch/arm/mach-omap2/sram34xx.S +8 -0
index 829d235..64faab8 100644
@@ -34,6 +34,14 @@
 #include "sdrc.h"
 #include "cm2xxx_3xxx.h"
 
+#undef ARM
+#undef THUMB
+#undef BSYM
+#define ARM(x...) x
+#define THUMB(x...)
+#define BSYM(x) (x)
+	.arm
+
 	.text
 
 /* r1 parameters */
#13 lore raw

v4 cover ARM: omap[34]: Thumb-2 compatibility fixes

Quoted 68 lines · Kevin Hilman

Hi Dave,

Quoted 64 lines · Dave Martin
Quoted 16 lines · Nicolas Pitre
Quoted 11 lines · Dave Martin
(quoted diff) +0 -0
@@ -289,8 +297,20 @@ clean_l2:

? ? * ?- should be faster and will change with kernel ? ? * ?- 'might' have to copy address, load and jump to it ? ? */

(quoted diff) +6 -0
+#ifdef CONFIG_THUMB2_KERNEL
+ ?/* kernel is non-interworking : must do this from Thumb */
+ ?adr ? ? r1, . + 1
+ ?bx ? ? ?r1
+ ?.thumb
+#endif

? ?ldr ? ? r1, kernel_flush

Didn't you mean this instead:

Quoted 6 lines

? ? ?/* kernel is non-interworking : must do this from Thumb */ ? ? ?adr ? ? r1, 1f + 1
? ? ?bx ? ? ?r1
? ? ?.thumb
1: ? ldr ? ? r1, kernel_flush
? ? ?...

Note that this is intended as an experimental hack, not a real patch (apologies if I didn't make that clear...)

Well, actually I meant "add r1, pc, #1" ... which means I was too busy trying to be clever... oops!

That is of course exactly equivalent to your code...

Quoted 10 lines

?

Quoted 6 lines

? ?blx ? ? r1
+#ifdef CONFIG_THUMB2_KERNEL
+ ?.align
+ ?bx ? ? ?pc
+ ?nop
+ ?.arm

Quoted 8 lines

Also here, the .align has the potential to introduce a zero halfword in the instruction stream before the bx. ?What about:

? ? ?adr ? ? r3, 1f
? ? ?bx ? ? ?r3
? ? ?.align
? ? ?.arm
1: ? ...

.align inserts a 16-bit nop when misaligned in Thumb in a text section, and a word-aligned bx pc is a specific architecturally allowed way to do an inline switch to ARM. ?The linker uses this trick for PLT veneers etc.

A nicer fix for doing this sort of call from low-level code which might be ARM is to convert arch/arm/mm/*-v7.S to use "bx lr" to return.

Generally, we can do this for all arches >= v5, without any incompatibility. ?However, since the need for it will be rare and it will generate patch noise for not much real benefit,
I haven't proposed this.

Quoted 4 lines
Quoted 1 line

Updated patch below.

I tested the updated patch on top of your "dirty" branch I tested with last week, and now see off-mode working just fine.

Thanks for testing-- that's great news.

I will have a think about whether the patch can be tidied up to revert most of the code back to Thumb, though that isn't essential. If I've understood what's going on correctly, I think only the restore entry points, SMC call sites and the entry to omap3_sram_configure_core_dpll could need to be ARM; the rest shouldn't really make any difference...

Cheers
---Dave

#14 lore raw

v4 cover ARM: omap[34]: Thumb-2 compatibility fixes

Quoted 74 lines · Dave Martin
Quoted 68 lines · Kevin Hilman

Hi Dave,

Quoted 64 lines · Dave Martin
Quoted 16 lines · Nicolas Pitre
Quoted 11 lines · Dave Martin
(quoted diff) +0 -0
@@ -289,8 +297,20 @@ clean_l2:

? ? * ?- should be faster and will change with kernel ? ? * ?- 'might' have to copy address, load and jump to it ? ? */

(quoted diff) +6 -0
+#ifdef CONFIG_THUMB2_KERNEL
+ ?/* kernel is non-interworking : must do this from Thumb */
+ ?adr ? ? r1, . + 1
+ ?bx ? ? ?r1
+ ?.thumb
+#endif

? ?ldr ? ? r1, kernel_flush

Didn't you mean this instead:

Quoted 6 lines

? ? ?/* kernel is non-interworking : must do this from Thumb */ ? ? ?adr ? ? r1, 1f + 1
? ? ?bx ? ? ?r1
? ? ?.thumb
1: ? ldr ? ? r1, kernel_flush
? ? ?...

Note that this is intended as an experimental hack, not a real patch (apologies if I didn't make that clear...)

Well, actually I meant "add r1, pc, #1" ... which means I was too busy trying to be clever... oops!

That is of course exactly equivalent to your code...

Quoted 10 lines

?

Quoted 6 lines

? ?blx ? ? r1
+#ifdef CONFIG_THUMB2_KERNEL
+ ?.align
+ ?bx ? ? ?pc
+ ?nop
+ ?.arm

Quoted 8 lines

Also here, the .align has the potential to introduce a zero halfword in the instruction stream before the bx. ?What about:

? ? ?adr ? ? r3, 1f
? ? ?bx ? ? ?r3
? ? ?.align
? ? ?.arm
1: ? ...

.align inserts a 16-bit nop when misaligned in Thumb in a text section, and a word-aligned bx pc is a specific architecturally allowed way to do an inline switch to ARM. ?The linker uses this trick for PLT veneers etc.

A nicer fix for doing this sort of call from low-level code which might be ARM is to convert arch/arm/mm/*-v7.S to use "bx lr" to return.

Generally, we can do this for all arches >= v5, without any incompatibility. ?However, since the need for it will be rare and it will generate patch noise for not much real benefit,
I haven't proposed this.

Quoted 4 lines
Quoted 1 line

Updated patch below.

I tested the updated patch on top of your "dirty" branch I tested with last week, and now see off-mode working just fine.

Quoted 7 lines

Thanks for testing-- that's great news.

I will have a think about whether the patch can be tidied up to revert most of the code back to Thumb, though that isn't essential. If I've understood what's going on correctly, I think only the restore entry points, SMC call sites and the entry to omap3_sram_configure_core_dpll could need to be ARM; the rest shouldn't really make any difference...

Yes, that sounds right.

Kevin