Skip to contentSkip to conversation

Fix omap3 booting with thumb2 compiled kernel

RFC In review Tony Lindgren 5 participants 13 messages May 27, 2015 – May 29, 2015

Subsystems (3)
Arm (32-bit) Arm sub-architectures OMAP2+ support
lore
Share & tools
lore

Tony Lindgren

author maintainer
#0 lore raw

RFC 1/1 Fix omap3 booting with thumb2 compiled kernel

The power management related assembly needs to interact with ARM mode bootrom code, so we need to keep most of the related assembly in ARM mode.

Currently we are entering into and ARM mode assembly function from thumb2 mode, so we need to make sure we switch to ARM mode. And we need to do that again after the cache flush.

---

Kevin told me about this earlier today.. Anybody got better ideas for a fix here?

arch/arm/mach-omap2/sleep34xx.S +25 -6
@@ -151,6 +151,17 @@ ENTRY(save_secure_ram_context_sz)
  */
 	.align	3
 ENTRY(omap34xx_cpu_suspend)
+
+	/*
+	 * This ARM assembly can also be called from thumb2 kernel code.
+	 * Make sure we switch to ARM mode first.
+	 */
+ THUMB(	.thumb		)
+ THUMB(	.align		)
+ THUMB(	bx	pc	)
+ THUMB(	nop		)
+	.arm
+
 	stmfd	sp!, {r4 - r11, lr}	@ save registers on stack
 
 	/*

@@ -187,6 +198,18 @@ save_context_wfi:
 	bx	r1
 
 	/*
+	 * The kernel doesn't interwork: v7_flush_dcache_all in particluar will
+	 * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled.
+	 * This sequence switches back to ARM.  Note that .align may insert a
+	 * nop: bx pc needs to be word-aligned in order to work.
+	 */
+ THUMB(	.thumb		)
+ THUMB(	.align		)
+ THUMB(	bx	pc	)
+ THUMB(	nop		)
+	.arm
+
+	/*
 	 * Clear the SCTLR.C bit to prevent further data cache
 	 * allocation. Clearing SCTLR.C would make all the data accesses
 	 * strongly ordered and would not hit the cache.

@@ -203,12 +226,8 @@ save_context_wfi:
 	 */
 	ldr	r1, kernel_flush
 	blx	r1
-	/*
-	 * The kernel doesn't interwork: v7_flush_dcache_all in particluar will
-	 * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled.
-	 * This sequence switches back to ARM.  Note that .align may insert a
-	 * nop: bx pc needs to be word-aligned in order to work.
-	 */
+
+	/* See the comments above about v7_flush_dcache_all */
  THUMB(	.thumb		)
  THUMB(	.align		)
  THUMB(	bx	pc	)
#1 lore raw

[ fix email for Dave Martin, +Tyler ]

Quoted 11 lines · Tony Lindgren

The power management related assembly needs to interact with ARM mode bootrom code, so we need to keep most of the related assembly in ARM mode.

Currently we are entering into and ARM mode assembly function from thumb2 mode, so we need to make sure we switch to ARM mode. And we need to do that again after the cache flush.

---

Kevin told me about this earlier today..

And for full boot log/panics, see the kernelci.org thumb2 kernel boots that fail: http://kernelci.org/boot/?THUMB2_KERNEL&fail

Quoted 1 line

Anybody got better ideas for a fix here?

FWIW, a quick test of this patch makes my omap3-beagle-xm pass a simple boot test, but it fails with off idle.

Kevin

author maintainer
#2 lore raw

RFC 1/1 Fix omap3 booting with thumb2 compiled kernel

Quoted 17 lines · Kevin Hilman

[ fix email for Dave Martin, +Tyler ]

Quoted 11 lines · Tony Lindgren

The power management related assembly needs to interact with ARM mode bootrom code, so we need to keep most of the related assembly in ARM mode.

Currently we are entering into and ARM mode assembly function from thumb2 mode, so we need to make sure we switch to ARM mode. And we need to do that again after the cache flush.

---

Kevin told me about this earlier today..

Quoted 7 lines

And for full boot log/panics, see the kernelci.org thumb2 kernel boots that fail: http://kernelci.org/boot/?THUMB2_KERNEL&fail

Quoted 1 line

Anybody got better ideas for a fix here?

FWIW, a quick test of this patch makes my omap3-beagle-xm pass a simple boot test, but it fails with off idle.

Thanks to Stephen Boyd's suggestion of checking the missing ENDPROC, here's a better fix :) Now off idle works too.

Regards,

Tony

8 <----------------------
From: Tony Lindgren <[email protected]>
Date: Wed, 27 May 2015 15:33:57 -0700
Subject: [PATCH] ARM: OMAP3: Fix booting with thumb2 kernel

We get a NULL pointer dereference on omap3 for thumb2 compiled kernels:

Internal error: Oops: 80000005 [#1] SMP THUMB2

...
[<c046497b>] (_raw_spin_unlock_irqrestore) from [<c0024375>] (omap3_enter_idle_bm+0xc5/0x178)
[<c0024375>] (omap3_enter_idle_bm) from [<c0374e63>] (cpuidle_enter_state+0x77/0x27c)
[<c0374e63>] (cpuidle_enter_state) from [<c00627f1>] (cpu_startup_entry+0x155/0x23c)
[<c00627f1>] (cpu_startup_entry) from [<c06b9a47>] (start_kernel+0x32f/0x338)
[<c06b9a47>] (start_kernel) from [<8000807f>] (0x8000807f)

The power management related assembly on moaps needs to interact with ARM mode bootrom code, so we need to keep most of the related assembly in ARM mode.

Turns out this error is because of missing ENDPROC for assembly code as suggested by Stephen Boyd <[email protected]>. Let's add the missing ENDPROC in two places to sleep34xx.S, and also remove the earlier mystery code that was probably also caused by missing ENDPROC for earlier kernels.

Reported-by Kevin Hilman <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
arch/arm/mach-omap2/sleep34xx.S +2 -20
@@ -203,23 +203,8 @@ save_context_wfi:
 	 */
 	ldr	r1, kernel_flush
 	blx	r1
-	/*
-	 * The kernel doesn't interwork: v7_flush_dcache_all in particluar will
-	 * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled.
-	 * This sequence switches back to ARM.  Note that .align may insert a
-	 * nop: bx pc needs to be word-aligned in order to work.
-	 */
- THUMB(	.thumb		)
- THUMB(	.align		)
- THUMB(	bx	pc	)
- THUMB(	nop		)
-	.arm
-
 	b	omap3_do_wfi
-
-/*
- * Local variables
- */
+ENDPROC(omap34xx_cpu_suspend)
 omap3_do_wfi_sram_addr:
 	.word omap3_do_wfi_sram
 kernel_flush:

@@ -364,10 +349,7 @@ exit_nonoff_modes:
  * ===================================
  */
 	ldmfd	sp!, {r4 - r11, pc}	@ restore regs and return
-
-/*
- * Local variables
- */
+ENDPROC(omap3_do_wfi)
 sdrc_power:
 	.word	SDRC_POWER_V
 cm_idlest1_core:
Tested-by on v1 1/1
#3 lore raw
Quoted 56 lines · Tony Lindgren
Quoted 17 lines · Kevin Hilman

[ fix email for Dave Martin, +Tyler ]

Quoted 11 lines · Tony Lindgren

The power management related assembly needs to interact with ARM mode bootrom code, so we need to keep most of the related assembly in ARM mode.

Currently we are entering into and ARM mode assembly function from thumb2 mode, so we need to make sure we switch to ARM mode. And we need to do that again after the cache flush.

---

Kevin told me about this earlier today..

Quoted 7 lines

And for full boot log/panics, see the kernelci.org thumb2 kernel boots that fail: http://kernelci.org/boot/?THUMB2_KERNEL&fail

Quoted 1 line

Anybody got better ideas for a fix here?

FWIW, a quick test of this patch makes my omap3-beagle-xm pass a simple boot test, but it fails with off idle.

Thanks to Stephen Boyd's suggestion of checking the missing ENDPROC, here's a better fix :) Now off idle works too.

Regards,

Tony

8 <----------------------
From: Tony Lindgren <[email protected]>
Date: Wed, 27 May 2015 15:33:57 -0700
Subject: [PATCH] ARM: OMAP3: Fix booting with thumb2 kernel

We get a NULL pointer dereference on omap3 for thumb2 compiled kernels:

Internal error: Oops: 80000005 [#1] SMP THUMB2

...
[<c046497b>] (_raw_spin_unlock_irqrestore) from [<c0024375>] (omap3_enter_idle_bm+0xc5/0x178)
[<c0024375>] (omap3_enter_idle_bm) from [<c0374e63>] (cpuidle_enter_state+0x77/0x27c)
[<c0374e63>] (cpuidle_enter_state) from [<c00627f1>] (cpu_startup_entry+0x155/0x23c)
[<c00627f1>] (cpu_startup_entry) from [<c06b9a47>] (start_kernel+0x32f/0x338)
[<c06b9a47>] (start_kernel) from [<8000807f>] (0x8000807f)

The power management related assembly on moaps needs to interact with ARM mode bootrom code, so we need to keep most of the related assembly in ARM mode.

Quoted 8 lines

Turns out this error is because of missing ENDPROC for assembly code as suggested by Stephen Boyd <[email protected]>. Let's add the missing ENDPROC in two places to sleep34xx.S, and also remove the earlier mystery code that was probably also caused by missing ENDPROC for earlier kernels.

Reported-by Kevin Hilman <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>

Yup, boot test is now passing for me on omap3-beagle, omap3-beagle-xm, omap3-n900, omap3-overo-tobi, omap3-overo-storm-tobi.

Tested-by Kevin Hilman <[email protected]>

Thanks for the quick fix.

Next step is to look at the exynos4412-odroidu3 failure which probably has a similar cause.

Kevin

#4 lore raw
In reply to Tony Lindgren
Quoted 19 lines · Tony Lindgren

8 <----------------------
From: Tony Lindgren <[email protected]>
Date: Wed, 27 May 2015 15:33:57 -0700
Subject: [PATCH] ARM: OMAP3: Fix booting with thumb2 kernel

We get a NULL pointer dereference on omap3 for thumb2 compiled kernels:

Internal error: Oops: 80000005 [#1] SMP THUMB2

...
[<c046497b>] (_raw_spin_unlock_irqrestore) from [<c0024375>] (omap3_enter_idle_bm+0xc5/0x178)
[<c0024375>] (omap3_enter_idle_bm) from [<c0374e63>] (cpuidle_enter_state+0x77/0x27c)
[<c0374e63>] (cpuidle_enter_state) from [<c00627f1>] (cpu_startup_entry+0x155/0x23c)
[<c00627f1>] (cpu_startup_entry) from [<c06b9a47>] (start_kernel+0x32f/0x338)
[<c06b9a47>] (start_kernel) from [<8000807f>] (0x8000807f)

Quoted 1 line

The power management related assembly on moaps needs to interact with

moaps -> omaps

Gra?vydas

author maintainer
#5 lore raw

RFC 1/1 Fix omap3 booting with thumb2 compiled kernel

Quoted 20 lines · Grazvydas Ignotas
Quoted 19 lines · Tony Lindgren

8 <----------------------
From: Tony Lindgren <[email protected]>
Date: Wed, 27 May 2015 15:33:57 -0700
Subject: [PATCH] ARM: OMAP3: Fix booting with thumb2 kernel

We get a NULL pointer dereference on omap3 for thumb2 compiled kernels:

Internal error: Oops: 80000005 [#1] SMP THUMB2

...
[<c046497b>] (_raw_spin_unlock_irqrestore) from [<c0024375>] (omap3_enter_idle_bm+0xc5/0x178)
[<c0024375>] (omap3_enter_idle_bm) from [<c0374e63>] (cpuidle_enter_state+0x77/0x27c)
[<c0374e63>] (cpuidle_enter_state) from [<c00627f1>] (cpu_startup_entry+0x155/0x23c)
[<c00627f1>] (cpu_startup_entry) from [<c06b9a47>] (start_kernel+0x32f/0x338)
[<c06b9a47>] (start_kernel) from [<8000807f>] (0x8000807f)

Quoted 3 lines
Quoted 1 line

The power management related assembly on moaps needs to interact with

moaps -> omaps

Thanks, fixed now. I also added a link for why ENDPROC is needed to the description. I'll apply the patch below into fixes today.

Regards,

Tony

8< ----------------------
From: Tony Lindgren <[email protected]>
Date: Wed, 27 May 2015 15:33:57 -0700
Subject: [PATCH] ARM: OMAP3: Fix booting with thumb2 kernel

We get a NULL pointer dereference on omap3 for thumb2 compiled kernels:

Internal error: Oops: 80000005 [#1] SMP THUMB2

...
[<c046497b>] (_raw_spin_unlock_irqrestore) from [<c0024375>] (omap3_enter_idle_bm+0xc5/0x178)
[<c0024375>] (omap3_enter_idle_bm) from [<c0374e63>] (cpuidle_enter_state+0x77/0x27c)
[<c0374e63>] (cpuidle_enter_state) from [<c00627f1>] (cpu_startup_entry+0x155/0x23c)
[<c00627f1>] (cpu_startup_entry) from [<c06b9a47>] (start_kernel+0x32f/0x338)
[<c06b9a47>] (start_kernel) from [<8000807f>] (0x8000807f)

The power management related assembly on omaps needs to interact with ARM mode bootrom code, so we need to keep most of the related assembly in ARM mode.

Turns out this error is because of missing ENDPROC for assembly code as suggested by Stephen Boyd <[email protected]>. Let's add the missing ENDPROC in two places to sleep34xx.S, and also remove the earlier mystery code that was probably also caused by missing ENDPROC for earlier kernels.

The reason why ENDPROC makes a difference is it sets .type and then the compiler knows what to do with the thumb bit as explained at:

https://wiki.ubuntu.com/ARM/Thumb2PortingHowto

Reported-by Kevin Hilman <[email protected]>
Tested-by Kevin Hilman <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
arch/arm/mach-omap2/sleep34xx.S +2 -20
@@ -203,23 +203,8 @@ save_context_wfi:
 	 */
 	ldr	r1, kernel_flush
 	blx	r1
-	/*
-	 * The kernel doesn't interwork: v7_flush_dcache_all in particluar will
-	 * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled.
-	 * This sequence switches back to ARM.  Note that .align may insert a
-	 * nop: bx pc needs to be word-aligned in order to work.
-	 */
- THUMB(	.thumb		)
- THUMB(	.align		)
- THUMB(	bx	pc	)
- THUMB(	nop		)
-	.arm
-
 	b	omap3_do_wfi
-
-/*
- * Local variables
- */
+ENDPROC(omap34xx_cpu_suspend)
 omap3_do_wfi_sram_addr:
 	.word omap3_do_wfi_sram
 kernel_flush:

@@ -364,10 +349,7 @@ exit_nonoff_modes:
  * ===================================
  */
 	ldmfd	sp!, {r4 - r11, pc}	@ restore regs and return
-
-/*
- * Local variables
- */
+ENDPROC(omap3_do_wfi)
 sdrc_power:
 	.word	SDRC_POWER_V
 cm_idlest1_core:
#6 lore raw
Quoted 78 lines · Tony Lindgren
Quoted 20 lines · Grazvydas Ignotas
Quoted 19 lines · Tony Lindgren

8 <----------------------
From: Tony Lindgren <[email protected]>
Date: Wed, 27 May 2015 15:33:57 -0700
Subject: [PATCH] ARM: OMAP3: Fix booting with thumb2 kernel

We get a NULL pointer dereference on omap3 for thumb2 compiled kernels:

Internal error: Oops: 80000005 [#1] SMP THUMB2

...
[<c046497b>] (_raw_spin_unlock_irqrestore) from [<c0024375>] (omap3_enter_idle_bm+0xc5/0x178)
[<c0024375>] (omap3_enter_idle_bm) from [<c0374e63>] (cpuidle_enter_state+0x77/0x27c)
[<c0374e63>] (cpuidle_enter_state) from [<c00627f1>] (cpu_startup_entry+0x155/0x23c)
[<c00627f1>] (cpu_startup_entry) from [<c06b9a47>] (start_kernel+0x32f/0x338)
[<c06b9a47>] (start_kernel) from [<8000807f>] (0x8000807f)

Quoted 3 lines
Quoted 1 line

The power management related assembly on moaps needs to interact with

moaps -> omaps

Thanks, fixed now. I also added a link for why ENDPROC is needed to the description. I'll apply the patch below into fixes today.

Regards,

Tony

8< ----------------------
From: Tony Lindgren <[email protected]>
Date: Wed, 27 May 2015 15:33:57 -0700
Subject: [PATCH] ARM: OMAP3: Fix booting with thumb2 kernel

We get a NULL pointer dereference on omap3 for thumb2 compiled kernels:

Internal error: Oops: 80000005 [#1] SMP THUMB2

...
[<c046497b>] (_raw_spin_unlock_irqrestore) from [<c0024375>] (omap3_enter_idle_bm+0xc5/0x178)
[<c0024375>] (omap3_enter_idle_bm) from [<c0374e63>] (cpuidle_enter_state+0x77/0x27c)
[<c0374e63>] (cpuidle_enter_state) from [<c00627f1>] (cpu_startup_entry+0x155/0x23c)
[<c00627f1>] (cpu_startup_entry) from [<c06b9a47>] (start_kernel+0x32f/0x338)
[<c06b9a47>] (start_kernel) from [<8000807f>] (0x8000807f)

The power management related assembly on omaps needs to interact with ARM mode bootrom code, so we need to keep most of the related assembly in ARM mode.

Turns out this error is because of missing ENDPROC for assembly code as suggested by Stephen Boyd <[email protected]>. Let's add the missing ENDPROC in two places to sleep34xx.S, and also remove the earlier mystery code that was probably also caused by missing ENDPROC for earlier kernels.

The reason why ENDPROC makes a difference is it sets .type and then the compiler knows what to do with the thumb bit as explained at:

https://wiki.ubuntu.com/ARM/Thumb2PortingHowto

Reported-by Kevin Hilman <[email protected]>
Tested-by Kevin Hilman <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
arch/arm/mach-omap2/sleep34xx.S +0 -2
@@ -203,23 +203,8 @@ save_context_wfi:
 	 */
 	ldr	r1, kernel_flush
 	blx	r1
-	/*
-	 * The kernel doesn't interwork: v7_flush_dcache_all in particluar will

arch/arm/mach-omap2/sleep34xx.S save_context_wfi: · line 217

Quoted 10 lines
arch/arm/mach-omap2/sleep34xx.S +0 -10
-	 * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled.
-	 * This sequence switches back to ARM.  Note that .align may insert a
-	 * nop: bx pc needs to be word-aligned in order to work.
-	 */
- THUMB(	.thumb		)
- THUMB(	.align		)
- THUMB(	bx	pc	)
- THUMB(	nop		)
-	.arm
-

^ This looks unrelated to the issue? This code is simply unnecessary after Russell introduced the "ret" macro in commit 6ebbf2ce43. That made v7_flush_dcache_all do a proper interworking return.

It should probably be in a separate commit, or at least mentioned explicitly in the commit log.

arch/arm/mach-omap2/sleep34xx.S save_context_wfi: · line 222

Quoted 5 lines
arch/arm/mach-omap2/sleep34xx.S +0 -4
 	b	omap3_do_wfi
-
-/*
- * Local variables
- */

^ Likewise this is just tidyup, not bug fixing.

arch/arm/mach-omap2/sleep34xx.S exit_nonoff_modes: · line 352

Quoted 13 lines
arch/arm/mach-omap2/sleep34xx.S +2 -4
+ENDPROC(omap34xx_cpu_suspend)
 omap3_do_wfi_sram_addr:
 	.word omap3_do_wfi_sram
 kernel_flush:

@@ -364,10 +349,7 @@ exit_nonoff_modes:
  * ===================================
  */
 	ldmfd	sp!, {r4 - r11, pc}	@ restore regs and return
-
-/*
- * Local variables
- */
+ENDPROC(omap3_do_wfi)

The ENDPROC()s look appropriate here.

[...]

Cheers
---Dave

author maintainer
#7 lore raw

RFC 1/1 Fix omap3 booting with thumb2 compiled kernel

Quoted 24 lines · Dave Martin
Quoted 8 lines · Tony Lindgren
arch/arm/mach-omap2/sleep34xx.S +0 -2
@@ -203,23 +203,8 @@ save_context_wfi:
 	 */
 	ldr	r1, kernel_flush
 	blx	r1
-	/*
-	 * The kernel doesn't interwork: v7_flush_dcache_all in particluar will

arch/arm/mach-omap2/sleep34xx.S save_context_wfi: · line 217

Quoted 10 lines
arch/arm/mach-omap2/sleep34xx.S +0 -10
-	 * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled.
-	 * This sequence switches back to ARM.  Note that .align may insert a
-	 * nop: bx pc needs to be word-aligned in order to work.
-	 */
- THUMB(	.thumb		)
- THUMB(	.align		)
- THUMB(	bx	pc	)
- THUMB(	nop		)
-	.arm
-

^ This looks unrelated to the issue? This code is simply unnecessary after Russell introduced the "ret" macro in commit 6ebbf2ce43. That made v7_flush_dcache_all do a proper interworking return.

Quoted 2 lines

It should probably be in a separate commit, or at least mentioned explicitly in the commit log.

Thanks, I've updated the description with the commit info above.

I'll keep the removal of the duplicate code in this patch, as it's all related to the mode switching and we don't want to do it twice.

Quoted 7 lines

arch/arm/mach-omap2/sleep34xx.S save_context_wfi: · line 222

Quoted 5 lines
arch/arm/mach-omap2/sleep34xx.S +0 -4
 	b	omap3_do_wfi
-
-/*
- * Local variables
- */

^ Likewise this is just tidyup, not bug fixing.

Will keep that too, the local variables comments are now just too confusing while reading the code after adding the ENDPROC. And that too is just removal.

Quoted 7 lines
Quoted 7 lines
arch/arm/mach-omap2/sleep34xx.S +1 -0
+ENDPROC(omap34xx_cpu_suspend)
 omap3_do_wfi_sram_addr:
 	.word omap3_do_wfi_sram
 kernel_flush:

@@ -364,10 +349,7 @@ exit_nonoff_modes:
  * ===================================
  */
Quoted 8 lines

arch/arm/mach-omap2/sleep34xx.S exit_nonoff_modes: · line 352

Quoted 6 lines
arch/arm/mach-omap2/sleep34xx.S +1 -4
 	ldmfd	sp!, {r4 - r11, pc}	@ restore regs and return
-
-/*
- * Local variables
- */
+ENDPROC(omap3_do_wfi)

The ENDPROC()s look appropriate here.

OK thanks for looking, I was hoping you'd check it as you did the original thumb2 changes for this file :) Updated patch below.

Regards,

Tony

8< ------------------------
From: Tony Lindgren <[email protected]>
Date: Thu, 28 May 2015 07:22:08 -0700
Subject: [PATCH] ARM: OMAP3: Fix booting with thumb2 kernel

We get a NULL pointer dereference on omap3 for thumb2 compiled kernels:

Internal error: Oops: 80000005 [#1] SMP THUMB2

...
[<c046497b>] (_raw_spin_unlock_irqrestore) from [<c0024375>] (omap3_enter_idle_bm+0xc5/0x178)
[<c0024375>] (omap3_enter_idle_bm) from [<c0374e63>] (cpuidle_enter_state+0x77/0x27c)
[<c0374e63>] (cpuidle_enter_state) from [<c00627f1>] (cpu_startup_entry+0x155/0x23c)
[<c00627f1>] (cpu_startup_entry) from [<c06b9a47>] (start_kernel+0x32f/0x338)
[<c06b9a47>] (start_kernel) from [<8000807f>] (0x8000807f)

The power management related assembly on omaps needs to interact with ARM mode bootrom code, so we need to keep most of the related assembly in ARM mode.

Turns out this error is because of missing ENDPROC for assembly code as suggested by Stephen Boyd <[email protected]>. Let's fix the problem by adding ENDPROC in two places to sleep34xx.S.

Let's also remove the now duplicate custom code for mode switching. This has been unnecessary since commit 6ebbf2ce437b ("ARM: convert all "mov.* pc, reg" to "bx reg" for ARMv6+").

And let's also remove the comments about local variables, they are now just confusing after the ENDPROC.

The reason why ENDPROC makes a difference is it sets .type and then the compiler knows what to do with the thumb bit as explained at:

https://wiki.ubuntu.com/ARM/Thumb2PortingHowto

Reported-by Kevin Hilman <[email protected]>
Tested-by Kevin Hilman <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
arch/arm/mach-omap2/sleep34xx.S +2 -20
@@ -203,23 +203,8 @@ save_context_wfi:
 	 */
 	ldr	r1, kernel_flush
 	blx	r1
-	/*
-	 * The kernel doesn't interwork: v7_flush_dcache_all in particluar will
-	 * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled.
-	 * This sequence switches back to ARM.  Note that .align may insert a
-	 * nop: bx pc needs to be word-aligned in order to work.
-	 */
- THUMB(	.thumb		)
- THUMB(	.align		)
- THUMB(	bx	pc	)
- THUMB(	nop		)
-	.arm
-
 	b	omap3_do_wfi
-
-/*
- * Local variables
- */
+ENDPROC(omap34xx_cpu_suspend)
 omap3_do_wfi_sram_addr:
 	.word omap3_do_wfi_sram
 kernel_flush:

@@ -364,10 +349,7 @@ exit_nonoff_modes:
  * ===================================
  */
 	ldmfd	sp!, {r4 - r11, pc}	@ restore regs and return
-
-/*
- * Local variables
- */
+ENDPROC(omap3_do_wfi)
 sdrc_power:
 	.word	SDRC_POWER_V
 cm_idlest1_core:
#8 lore raw
Quoted 41 lines · Tony Lindgren
Quoted 24 lines · Dave Martin
Quoted 8 lines · Tony Lindgren
arch/arm/mach-omap2/sleep34xx.S +0 -2
@@ -203,23 +203,8 @@ save_context_wfi:
 	 */
 	ldr	r1, kernel_flush
 	blx	r1
-	/*
-	 * The kernel doesn't interwork: v7_flush_dcache_all in particluar will

arch/arm/mach-omap2/sleep34xx.S save_context_wfi: · line 217

Quoted 10 lines
arch/arm/mach-omap2/sleep34xx.S +0 -10
-	 * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled.
-	 * This sequence switches back to ARM.  Note that .align may insert a
-	 * nop: bx pc needs to be word-aligned in order to work.
-	 */
- THUMB(	.thumb		)
- THUMB(	.align		)
- THUMB(	bx	pc	)
- THUMB(	nop		)
-	.arm
-

^ This looks unrelated to the issue? This code is simply unnecessary after Russell introduced the "ret" macro in commit 6ebbf2ce43. That made v7_flush_dcache_all do a proper interworking return.

Quoted 2 lines

It should probably be in a separate commit, or at least mentioned explicitly in the commit log.

Thanks, I've updated the description with the commit info above.

I'll keep the removal of the duplicate code in this patch, as it's all related to the mode switching and we don't want to do it twice.

Quoted 7 lines

arch/arm/mach-omap2/sleep34xx.S save_context_wfi: · line 222

Quoted 5 lines
arch/arm/mach-omap2/sleep34xx.S +0 -4
 	b	omap3_do_wfi
-
-/*
- * Local variables
- */

^ Likewise this is just tidyup, not bug fixing.

Quoted 3 lines

Will keep that too, the local variables comments are now just too confusing while reading the code after adding the ENDPROC. And that too is just removal.

I have no objection to tidyups ;)

So long as the commit message is clear about which parts are bugfix and which parts are tidyup, I'm fine with that.

Cheers
---Dave

#9 lore raw
In reply to Tony Lindgren
Quoted 25 lines · On Thursday 28 May 2015 09:36:05 Tony Lindgren wrote:
Quoted 24 lines · Dave Martin
Quoted 8 lines · Tony Lindgren
arch/arm/mach-omap2/sleep34xx.S +0 -2
@@ -203,23 +203,8 @@ save_context_wfi:
     */
    ldr     r1, kernel_flush
    blx     r1
-   /*
-    * The kernel doesn't interwork: v7_flush_dcache_all in particluar will

arch/arm/mach-omap2/sleep34xx.S save_context_wfi: · line 217

Quoted 10 lines
arch/arm/mach-omap2/sleep34xx.S +0 -10
-    * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled.
-    * This sequence switches back to ARM.  Note that .align may insert a
-    * nop: bx pc needs to be word-aligned in order to work.
-    */
- THUMB(    .thumb          )
- THUMB(    .align          )
- THUMB(    bx      pc      )
- THUMB(    nop             )
-   .arm
-

^ This looks unrelated to the issue? This code is simply unnecessary after Russell introduced the "ret" macro in commit 6ebbf2ce43. That made v7_flush_dcache_all do a proper interworking return.

Quoted 7 lines
Quoted 2 lines

It should probably be in a separate commit, or at least mentioned explicitly in the commit log.

Thanks, I've updated the description with the commit info above.

I'll keep the removal of the duplicate code in this patch, as it's all related to the mode switching and we don't want to do it twice.

I would have thought that the change is actually necessary after 6ebbf2ce43, because it now returns in ARM mode, which will cause the "bx pc; nop" thumb instruction sequence be misinterpreted as an ARM instruction.

Arnd

author maintainer
#10 lore raw
Quoted 31 lines · Arnd Bergmann
Quoted 30 lines · On Thursday 28 May 2015 09:36:05 Tony Lindgren wrote:
Quoted 24 lines · Dave Martin
Quoted 8 lines · Tony Lindgren
arch/arm/mach-omap2/sleep34xx.S +0 -2
@@ -203,23 +203,8 @@ save_context_wfi:
     */
    ldr     r1, kernel_flush
    blx     r1
-   /*
-    * The kernel doesn't interwork: v7_flush_dcache_all in particluar will

arch/arm/mach-omap2/sleep34xx.S save_context_wfi: · line 217

Quoted 10 lines
arch/arm/mach-omap2/sleep34xx.S +0 -10
-    * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled.
-    * This sequence switches back to ARM.  Note that .align may insert a
-    * nop: bx pc needs to be word-aligned in order to work.
-    */
- THUMB(    .thumb          )
- THUMB(    .align          )
- THUMB(    bx      pc      )
- THUMB(    nop             )
-   .arm
-

^ This looks unrelated to the issue? This code is simply unnecessary after Russell introduced the "ret" macro in commit 6ebbf2ce43. That made v7_flush_dcache_all do a proper interworking return.

Quoted 2 lines

It should probably be in a separate commit, or at least mentioned explicitly in the commit log.

Thanks, I've updated the description with the commit info above.

Quoted 7 lines
Quoted 2 lines

I'll keep the removal of the duplicate code in this patch, as it's all related to the mode switching and we don't want to do it twice.

I would have thought that the change is actually necessary after 6ebbf2ce43, because it now returns in ARM mode, which will cause the "bx pc; nop" thumb instruction sequence be misinterpreted as an ARM instruction.

Could be it's necessary.. But alone that change is not enough.

Regards,

Tony

#11 lore raw
In reply to Arnd Bergmann
Quoted 34 lines · Arnd Bergmann
Quoted 25 lines · On Thursday 28 May 2015 09:36:05 Tony Lindgren wrote:
Quoted 24 lines · Dave Martin
Quoted 8 lines · Tony Lindgren
arch/arm/mach-omap2/sleep34xx.S +0 -2
@@ -203,23 +203,8 @@ save_context_wfi:
     */
    ldr     r1, kernel_flush
    blx     r1
-   /*
-    * The kernel doesn't interwork: v7_flush_dcache_all in particluar will

arch/arm/mach-omap2/sleep34xx.S save_context_wfi: · line 217

Quoted 10 lines
arch/arm/mach-omap2/sleep34xx.S +0 -10
-    * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled.
-    * This sequence switches back to ARM.  Note that .align may insert a
-    * nop: bx pc needs to be word-aligned in order to work.
-    */
- THUMB(    .thumb          )
- THUMB(    .align          )
- THUMB(    bx      pc      )
- THUMB(    nop             )
-   .arm
-

^ This looks unrelated to the issue? This code is simply unnecessary after Russell introduced the "ret" macro in commit 6ebbf2ce43. That made v7_flush_dcache_all do a proper interworking return.

Quoted 7 lines
Quoted 2 lines

It should probably be in a separate commit, or at least mentioned explicitly in the commit log.

Thanks, I've updated the description with the commit info above.

I'll keep the removal of the duplicate code in this patch, as it's all related to the mode switching and we don't want to do it twice.

Quoted 6 lines

I would have thought that the change is actually necessary after 6ebbf2ce43, because it now returns in ARM mode, which will cause the "bx pc; nop" thumb instruction sequence be misinterpreted as an ARM instruction.

Arnd

You're right. The THUMB2_KERNEL indeed won't work without deleting this code.

Cheers
---Dave

author maintainer
#12 lore raw
Quoted 40 lines · Dave P Martin
Quoted 39 lines · Arnd Bergmann
Quoted 25 lines · On Thursday 28 May 2015 09:36:05 Tony Lindgren wrote:
Quoted 24 lines · Dave Martin
Quoted 8 lines · Tony Lindgren
arch/arm/mach-omap2/sleep34xx.S +0 -2
@@ -203,23 +203,8 @@ save_context_wfi:
     */
    ldr     r1, kernel_flush
    blx     r1
-   /*
-    * The kernel doesn't interwork: v7_flush_dcache_all in particluar will

arch/arm/mach-omap2/sleep34xx.S save_context_wfi: · line 217

Quoted 10 lines
arch/arm/mach-omap2/sleep34xx.S +0 -10
-    * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled.
-    * This sequence switches back to ARM.  Note that .align may insert a
-    * nop: bx pc needs to be word-aligned in order to work.
-    */
- THUMB(    .thumb          )
- THUMB(    .align          )
- THUMB(    bx      pc      )
- THUMB(    nop             )
-   .arm
-

^ This looks unrelated to the issue? This code is simply unnecessary after Russell introduced the "ret" macro in commit 6ebbf2ce43. That made v7_flush_dcache_all do a proper interworking return.

Quoted 7 lines
Quoted 2 lines

It should probably be in a separate commit, or at least mentioned explicitly in the commit log.

Thanks, I've updated the description with the commit info above.

I'll keep the removal of the duplicate code in this patch, as it's all related to the mode switching and we don't want to do it twice.

I would have thought that the change is actually necessary after 6ebbf2ce43, because it now returns in ARM mode, which will cause the "bx pc; nop" thumb instruction sequence be misinterpreted as an ARM instruction.

Quoted 4 lines
Quoted 1 line

Arnd

You're right. The THUMB2_KERNEL indeed won't work without deleting this code.

OK thanks for checking.

Regards,

Tony