[PATCH v4 0/5] ARM: omap[34]: Thumb-2 compatibility fixes

Hi Dave,

Dave Martin [off-list ref] writes:

On Mon, Feb 14, 2011 at 10:00:23AM -0500, Nicolas Pitre wrote:

quoted

On Mon, 14 Feb 2011, Dave Martin wrote:

quoted

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

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

?

quoted

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

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

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

On Mon, Feb 14, 2011 at 11:15 PM, Kevin Hilman [off-list ref] wrote:

Hi Dave,

Dave Martin [off-list ref] writes:

quoted

On Mon, Feb 14, 2011 at 10:00:23AM -0500, Nicolas Pitre wrote:

quoted

On Mon, 14 Feb 2011, Dave Martin wrote:

quoted

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

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

?

quoted

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

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

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