[PATCH v2 0/3] ARM: omap: Thumb-2 kernel compatibility fixes

[PATCH v2 0/3] ARM: omap: Thumb-2 kernel compatibility fixes

[Dave Martin]

These patches enable the omap kernel to build and work in
Thumb-2 (CONFIG_THUMB2_KERNEL).

They may make useful reading for all low-level BSP maintainers,
since in general similar issues tend to crop up whenever migrating
code to support Thumb-2.

Tested on: omap3 (Beagle xM A2)
Not tested on any other platforms yet.

Dave Martin (3):
  ARM: omap: Enable low-level omap3 PM code to work with
    CONFIG_THUMB2_KERNEL
  ARM: omap4: Correct definition of do_wfi() for CONFIG_THUMB2_KERNEL
  ARM: omap4: Convert END() to ENDPROC() for correct linkage with
    CONFIG_THUMB2_KERNEL

 arch/arm/mach-omap2/include/mach/omap4-common.h |    5 +++
 arch/arm/mach-omap2/omap-headsmp.S              |    2 +-
 arch/arm/mach-omap2/omap44xx-smc.S              |    8 ++--
 arch/arm/mach-omap2/pm.h                        |    2 +
 arch/arm/mach-omap2/pm34xx.c                    |   13 ++++++--
 arch/arm/mach-omap2/sleep34xx.S                 |   37 +++++++++++++++++++++--
 arch/arm/mach-omap2/sram34xx.S                  |   34 +++++++++++++++-----
 arch/arm/plat-omap/include/plat/sram.h          |    1 +
 arch/arm/plat-omap/sram.c                       |   10 +++++-
 9 files changed, 90 insertions(+), 22 deletions(-)

[PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to work with CONFIG_THUMB2_KERNEL

[Dave Martin]

sleep34xx.S, sram34xx.S:

  * Added ENDPROC() directives for all exported function symbols.
    Without these, exported function symbols are not correctly
    identified as Thumb by the linker, causing incorrect linkage.
    This is needed to avoid some calls to the functions ending up
    with the CPU in the wrong instruction set.

  * Added .align directives where needed to ensure that .word won't
    be misaligned.  (Note that ENTRY() implies .align; no extra
    .align has been added for these cases.)

  * Exported new "base address" symbols for functions which get
    copied to sram by code outside sleep34xx.S (applies to
    save_secure_ram_context and omap32xx_cpu_suspend), and fix up
    the relevant address arithmetic so that these will be copied
    and called correctly by the Thumb code in the rest of the
    kernel.

  * Explicitly build a few parts of sleep34xx.S as ARM.

      * lock_scratchpad_sem is kept as ARM because of the need to
        synchronise with hardware (?) using the SWP instruction.

      * save_secure_ram_context and omap34xx_cpu_suspend are built
        as ARM in case the Secure World firmware expects to decode
        the comment field from the SMC (aka smi) instructions.

        This can be undone later if the firmware is confirmed as
        able to decode the Thumb SMC encoding (or ignores the
        comment field).

      * es3_sdrc_fix should presumably only be called from the
        low-level wakeup code.  To minimise the diff, switched this
        to ARM and demoted it to be a local symbol, since I believe
        it shouldn't be called from outside anyway.

     To prevent future maintainence problems, it would probably be
     a good idea to demote _all_ functions which aren't called
     externally to local.  (i.e., everything except for
     get_es3_restore_pointer, get_restore_pointer,
     omap34xx_cpu_suspend and save_secure_ram_context).

     For now, I've left these as-is to minimise disruption.

   * Use a separate base register instead of PC-relative stores in
     sram34xx.S.  This isn't permitted in Thumb-2, but at least
     some versions of gas will silently output non-working code,
     leading to unpredictable run-time behaviour.

pm34xx.c, pm.h, sram.c, sram.h:

  * Resolve some memory addressing issues where a function symbol's
    value is assumed to be equal to the start address of the
    function body for the purpose of copying some low-level code
    from sleep34xx.S to SRAM.

    This assumption breaks for Thumb, since Thumb functions symbols
    have bit 0 set to indicate the Thumb-ness.  This would result
    in a non-working, off-by-one copy of the function body.

Tested on Beagle xM A2:
  * CONFIG_THUMB2_KERNEL && !CONFIG_SMP_ON_UP
  * CONFIG_THUMB2_KERNEL && CONFIG_SMP_ON_UP
  * !CONFIG_THUMB2_KERNEL && !CONFIG_SMP_ON_UP
  * !CONFIG_THUMB2_KERNEL && CONFIG_SMP_ON_UP

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
KernelVersion: 2.6.37-rc4

 arch/arm/mach-omap2/pm.h               |    2 +
 arch/arm/mach-omap2/pm34xx.c           |   13 ++++++++--
 arch/arm/mach-omap2/sleep34xx.S        |   37 +++++++++++++++++++++++++++++--
 arch/arm/mach-omap2/sram34xx.S         |   34 +++++++++++++++++++++-------
 arch/arm/plat-omap/include/plat/sram.h |    1 +
 arch/arm/plat-omap/sram.c              |   10 +++++++-
 6 files changed, 80 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 0d75bfd..c333bfd 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -80,7 +80,9 @@ extern void save_secure_ram_context(u32 *addr);
 extern void omap3_save_scratchpad_contents(void);
 
 extern unsigned int omap24xx_idle_loop_suspend_sz;
+extern char *const omap34xx_cpu_suspend_base;
 extern unsigned int omap34xx_suspend_sz;
+extern char *const save_secure_ram_context_base;
 extern unsigned int save_secure_ram_context_sz;
 extern unsigned int omap24xx_cpu_suspend_sz;
 extern unsigned int omap34xx_cpu_suspend_sz;
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 0ec8a04..93f0ee8 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -982,11 +982,18 @@ static int __init clkdms_setup(struct clockdomain *clkdm, void *unused)
 
 void omap_push_sram_idle(void)
 {
-	_omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
+	_omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend_base,
 					omap34xx_cpu_suspend_sz);
-	if (omap_type() != OMAP2_DEVICE_TYPE_GP)
-		_omap_save_secure_sram = omap_sram_push(save_secure_ram_context,
+	_omap_sram_idle += (char *)omap34xx_cpu_suspend -
+		omap34xx_cpu_suspend_base;
+
+	if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
+		_omap_save_secure_sram = omap_sram_push(
+				save_secure_ram_context_base,
 				save_secure_ram_context_sz);
+		_omap_save_secure_sram += (char *)save_secure_ram_context -
+			save_secure_ram_context_base;
+	}
 }
 
 static int __init omap3_pm_init(void)
diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
index 2fb205a..06ae955 100644
--- a/arch/arm/mach-omap2/sleep34xx.S
+++ b/arch/arm/mach-omap2/sleep34xx.S
@@ -61,6 +61,7 @@
 
         .text
 /* Function to acquire the semaphore in scratchpad */
+	.arm			@ Do this in ARM for now, due to use of SWP.
 ENTRY(lock_scratchpad_sem)
 	stmfd	sp!, {lr}	@ save registers on stack
 wait_sem:
@@ -74,6 +75,9 @@ wait_loop:
 	cmp	r2, r0		@ did we succeed ?
 	beq	wait_sem	@ no - try again
 	ldmfd	sp!, {pc}	@ restore regs and return
+ENDPROC(lock_scratchpad_sem)
+ THUMB(	.thumb		)
+	.align
 sdrc_scratchpad_sem:
         .word SDRC_SCRATCHPAD_SEM_V
 ENTRY(lock_scratchpad_sem_sz)
@@ -87,6 +91,7 @@ ENTRY(unlock_scratchpad_sem)
 	mov	r2,#0
 	str	r2,[r3]
 	ldmfd	sp!, {pc}	@ restore regs and return
+ENDPROC(unlock_scratchpad_sem)
 ENTRY(unlock_scratchpad_sem_sz)
         .word   . - unlock_scratchpad_sem
 
@@ -96,6 +101,7 @@ ENTRY(get_restore_pointer)
         stmfd   sp!, {lr}     @ save registers on stack
 	adr	r0, restore
         ldmfd   sp!, {pc}     @ restore regs and return
+ENDPROC(get_restore_pointer)
 ENTRY(get_restore_pointer_sz)
         .word   . - get_restore_pointer
 
@@ -105,10 +111,16 @@ ENTRY(get_es3_restore_pointer)
 	stmfd	sp!, {lr}	@ save registers on stack
 	adr	r0, restore_es3
 	ldmfd	sp!, {pc}	@ restore regs and return
+ENDPROC(get_es3_restore_pointer)
 ENTRY(get_es3_restore_pointer_sz)
 	.word	. - get_es3_restore_pointer
 
-ENTRY(es3_sdrc_fix)
+@ For simplicity, make this ARM so it gets called OK from es3_restore.
+@ Demote to a local symbol, since this gives this function an ARM ABI interface
+@ which won't be callable directly from a Thumb-2 kernel.  This code
+@ shouldn't be called from outside anyway...
+	.arm
+es3_sdrc_fix:
 	ldr	r4, sdrc_syscfg		@ get config addr
 	ldr	r5, [r4]		@ get value
 	tst	r5, #0x100		@ is part access blocked
@@ -134,6 +146,9 @@ ENTRY(es3_sdrc_fix)
 	mov	r5, #0x2		@ autorefresh command
 	str	r5, [r4]		@ kick off refreshes
 	bx	lr
+ENDPROC(es3_sdrc_fix)
+ THUMB(	.thumb		)
+	.align
 sdrc_syscfg:
 	.word	SDRC_SYSCONFIG_P
 sdrc_mr_0:
@@ -150,8 +165,12 @@ sdrc_manual_1:
 	.word	SDRC_MANUAL_1_P
 ENTRY(es3_sdrc_fix_sz)
 	.word	. - es3_sdrc_fix
+ THUMB(	.thumb		)
 
 /* Function to call rom code to save secure ram context */
+	.arm			@ Do this in ARM for now, due to use of SMC,
+				@ in case the Secure World firmware may depends
+				@ on decoding the SMC instruction.
 ENTRY(save_secure_ram_context)
 	stmfd	sp!, {r1-r12, lr}	@ save registers on stack
 save_secure_ram_debug:
@@ -175,6 +194,9 @@ save_secure_ram_debug:
 	nop
 	nop
 	ldmfd	sp!, {r1-r12, pc}
+ENDPROC(save_secure_ram_context)
+ THUMB(	.thumb		)
+	.align
 sram_phy_addr_mask:
 	.word	SRAM_BASE_P
 high_mask:
@@ -183,6 +205,8 @@ api_params:
 	.word	0x4, 0x0, 0x0, 0x1, 0x1
 ENTRY(save_secure_ram_context_sz)
 	.word	. - save_secure_ram_context
+ENTRY(save_secure_ram_context_base)
+	.word	save_secure_ram_context_base
 
 /*
  * Forces OMAP into idle state
@@ -193,6 +217,7 @@ ENTRY(save_secure_ram_context_sz)
  * Note: This code get's copied to internal SRAM at boot. When the OMAP
  *	 wakes up it continues execution at the point it went to sleep.
  */
+	.arm			@ Do this in ARM for now, due to use of SMC.
 ENTRY(omap34xx_cpu_suspend)
 	stmfd	sp!, {r0-r12, lr}		@ save registers on stack
 loop:
@@ -563,10 +588,12 @@ loop2:
 	mov     r9, r4
 	/* create working copy of max way size*/
 loop3:
+	mov     r1, r9, lsl r5
+	mov     r2, r7, lsl r2
 	/* factor way and cache number into r11 */
-	orr     r11, r10, r9, lsl r5
+	orr     r11, r10, r1
 	/* factor index number into r11 */
-	orr     r11, r11, r7, lsl r2
+	orr     r11, r11, r2
 	/*clean & invalidate by set/way */
 	mcr     p15, 0, r11, c7, c10, 2
 	/* decrement the way*/
@@ -631,7 +658,9 @@ wait_dll_lock:
         cmp     r5, #0x4
         bne     wait_dll_lock
         bx      lr
+ENDPROC(omap34xx_cpu_suspend)
 
+	.align
 cm_idlest1_core:
 	.word	CM_IDLEST1_CORE_V
 sdrc_dlla_status:
@@ -670,3 +699,5 @@ control_stat:
 	.word	CONTROL_STAT
 ENTRY(omap34xx_cpu_suspend_sz)
 	.word	. - omap34xx_cpu_suspend
+ENTRY(omap34xx_cpu_suspend_base)
+	.word	omap34xx_cpu_suspend
diff --git a/arch/arm/mach-omap2/sram34xx.S b/arch/arm/mach-omap2/sram34xx.S
index 3637274..65fd54f 100644
--- a/arch/arm/mach-omap2/sram34xx.S
+++ b/arch/arm/mach-omap2/sram34xx.S
@@ -105,29 +105,42 @@
  * can satisfy the above requirement can enable the CONFIG_OMAP3_SDRC_AC_TIMING
  * option.
  */
+__omap3_sram_configure_core_dpll_base:	@ Separate local symbol with the Thumb
+					@ bit _not_ set (for base address when
+					@ copying to sram).
 ENTRY(omap3_sram_configure_core_dpll)
 	stmfd	sp!, {r1-r12, lr}	@ store regs to stack
 
 					@ 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
@@ -264,7 +277,9 @@ configure_sdrc:
 skip_cs1_prog:
 	ldr	r12, [r11]		@ posted-write barrier for SDRC
 	bx	lr
+ENDPROC(omap3_sram_configure_core_dpll)
 
+	.align
 omap3_sdrc_power:
 	.word OMAP34XX_SDRC_REGADDR(SDRC_POWER)
 omap3_cm_clksel1_pll:
@@ -316,4 +331,5 @@ core_m2_mask_val:
 
 ENTRY(omap3_sram_configure_core_dpll_sz)
 	.word	. - omap3_sram_configure_core_dpll
-
+ENTRY(omap3_sram_configure_core_dpll_base)
+	.word	__omap3_sram_configure_core_dpll_base
diff --git a/arch/arm/plat-omap/include/plat/sram.h b/arch/arm/plat-omap/include/plat/sram.h
index 5905100..2f27167 100644
--- a/arch/arm/plat-omap/include/plat/sram.h
+++ b/arch/arm/plat-omap/include/plat/sram.h
@@ -67,6 +67,7 @@ extern u32 omap3_sram_configure_core_dpll(
 			u32 sdrc_rfr_ctrl_1, u32 sdrc_actim_ctrl_a_1,
 			u32 sdrc_actim_ctrl_b_1, u32 sdrc_mr_1);
 extern unsigned long omap3_sram_configure_core_dpll_sz;
+extern char *omap3_sram_configure_core_dpll_base;
 
 #ifdef CONFIG_PM
 extern void omap_push_sram_idle(void);
diff --git a/arch/arm/plat-omap/sram.c b/arch/arm/plat-omap/sram.c
index e2c8eeb..61282f4 100644
--- a/arch/arm/plat-omap/sram.c
+++ b/arch/arm/plat-omap/sram.c
@@ -386,8 +386,11 @@ void omap3_sram_restore_context(void)
 	omap_sram_ceil = omap_sram_base + omap_sram_size;
 
 	_omap3_sram_configure_core_dpll =
-		omap_sram_push(omap3_sram_configure_core_dpll,
+		omap_sram_push(omap3_sram_configure_core_dpll_base,
 			       omap3_sram_configure_core_dpll_sz);
+	_omap3_sram_configure_core_dpll +=
+		(char *)omap3_sram_configure_core_dpll -
+		omap3_sram_configure_core_dpll_base;
 	omap_push_sram_idle();
 }
 #endif /* CONFIG_PM */
@@ -395,8 +398,11 @@ void omap3_sram_restore_context(void)
 static int __init omap34xx_sram_init(void)
 {
 	_omap3_sram_configure_core_dpll =
-		omap_sram_push(omap3_sram_configure_core_dpll,
+		omap_sram_push(omap3_sram_configure_core_dpll_base,
 			       omap3_sram_configure_core_dpll_sz);
+	_omap3_sram_configure_core_dpll +=
+		(char *)omap3_sram_configure_core_dpll -
+		omap3_sram_configure_core_dpll_base;
 	omap_push_sram_idle();
 	return 0;
 }
-- 
1.7.1

[PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to work with CONFIG_THUMB2_KERNEL

[Catalin Marinas]

On 6 December 2010 17:35, Dave Martin <dave.martin@linaro.org> wrote:
> ?* Explicitly build a few parts of sleep34xx.S as ARM.
>
> ? ? ?* lock_scratchpad_sem is kept as ARM because of the need to
> ? ? ? ?synchronise with hardware (?) using the SWP instruction.
>
> ? ? ?* save_secure_ram_context and omap34xx_cpu_suspend are built
> ? ? ? ?as ARM in case the Secure World firmware expects to decode
> ? ? ? ?the comment field from the SMC (aka smi) instructions.
>
> ? ? ? ?This can be undone later if the firmware is confirmed as
> ? ? ? ?able to decode the Thumb SMC encoding (or ignores the
> ? ? ? ?comment field).
>
> ? ? ?* es3_sdrc_fix should presumably only be called from the
> ? ? ? ?low-level wakeup code. ?To minimise the diff, switched this
> ? ? ? ?to ARM and demoted it to be a local symbol, since I believe
> ? ? ? ?it shouldn't be called from outside anyway.

I haven't checked the code but does this always work? The kernel isn't
built with interworking enabled, so it's either ARM or Thumb-2.

-- 
Catalin

[PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to work with CONFIG_THUMB2_KERNEL

[Dave Martin]

Hi,

On Mon, Dec 6, 2010 at 6:17 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On 6 December 2010 17:35, Dave Martin <dave.martin@linaro.org> wrote:
>> ?* Explicitly build a few parts of sleep34xx.S as ARM.
>>
>> ? ? ?* lock_scratchpad_sem is kept as ARM because of the need to
>> ? ? ? ?synchronise with hardware (?) using the SWP instruction.
>>
>> ? ? ?* save_secure_ram_context and omap34xx_cpu_suspend are built
>> ? ? ? ?as ARM in case the Secure World firmware expects to decode
>> ? ? ? ?the comment field from the SMC (aka smi) instructions.
>>
>> ? ? ? ?This can be undone later if the firmware is confirmed as
>> ? ? ? ?able to decode the Thumb SMC encoding (or ignores the
>> ? ? ? ?comment field).
>>
>> ? ? ?* es3_sdrc_fix should presumably only be called from the
>> ? ? ? ?low-level wakeup code. ?To minimise the diff, switched this
>> ? ? ? ?to ARM and demoted it to be a local symbol, since I believe
>> ? ? ? ?it shouldn't be called from outside anyway.
>
> I haven't checked the code but does this always work? The kernel isn't
> built with interworking enabled, so it's either ARM or Thumb-2.

Interworking is mandated by EABI, and when building for EABI there is
no such thing as non-interworking C code IIUC: i.e.,
-mno-thumb-interwork does nothing.  Certainly, calls via a function
pointer are assembled as BX instructions, and the linker fixes up
static function calls with the right instruction (BL/BX).

Provided the affected functions are only called from C code, and
providing that legacy tools/ABI aren't used, it should work.  I've
reviewed to make sure that this is the case, and the code as posted
executes correctly on Beagle xM (not including the omap4-specific
code, for which I have no board to test on).

CONFIG_THUMB2_KERNEL already depends on CONFIG_AEABI, so we should be
relatively safe on this point.  Without CONFIG_THUMB2_KERNEL,
everything is built as ARM, so we have no problem.

The two cases where we have to be careful are:

  * Where code assumes that f == <base address of body of function f>,
which would lead to off-by-one errors by code which tries to copy
function bodies and derive pointers to the copies, due to the setting
of bit 0 of the address to indicate Thumb.  The patch series fix the
instances of this error present in the upstream OMAP BSP, except for
instances in code specific to OMAP2 and earlier which can't usefully
be built for Thumb-2 anyway.

  * Where bootloaders / firmware may jump directly into locations in
the kernel, and assume ARM (or don't interwork correctly), or where
the firmware may implicitly decode instructions from the caller as ARM
(e.g., to examine the SMC instruction comment field).  The most
straightforward way to avoid such problems is to make all such entry
points ARM.  But we could get rid of that if the platform maintainers
believe it's safe.


I did experiment with sticking with pure ARM in the low-level
assembler, with explicit veneers to switch instruction set, but that
gets rather messy and shouldn't really be necessary.  I suspect it
would be harder to maintain also.

Pure Thumb on the other hand is unlikely to be possible in every case:
the OMAP BSP uses SWP, and I've conservatively made the assumption
that bootloaders / firmware may assume they're jumping into ARM code,
and may try to decode SMC instructions as ARM instructions -- these
may not be true in practice, but these are harder to test, and I'm not
in a position to check _every_ bootloader/firmware/kernel combination
...

Cheers
---Dave

[PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to work withCONFIG_THUMB2_KERNEL

[Santosh Shilimkar]

Dave,
> -----Original Message-----
> From: linaro-dev-bounces at lists.linaro.org [mailto:linaro-dev-
> bounces at lists.linaro.org] On Behalf Of Dave Martin
> Sent: Monday, December 06, 2010 11:06 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: Tony Lindgren; Dave Martin; linux-omap at vger.kernel.org; linaro-
> dev at lists.linaro.org
> Subject: [PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to
work
> withCONFIG_THUMB2_KERNEL
>
> sleep34xx.S, sram34xx.S:
>
>   * Added ENDPROC() directives for all exported function symbols.
>     Without these, exported function symbols are not correctly
>     identified as Thumb by the linker, causing incorrect linkage.
>     This is needed to avoid some calls to the functions ending up
>     with the CPU in the wrong instruction set.
>
>   * Added .align directives where needed to ensure that .word won't
>     be misaligned.  (Note that ENTRY() implies .align; no extra
>     .align has been added for these cases.)
>
>   * Exported new "base address" symbols for functions which get
>     copied to sram by code outside sleep34xx.S (applies to
>     save_secure_ram_context and omap32xx_cpu_suspend), and fix up
>     the relevant address arithmetic so that these will be copied
>     and called correctly by the Thumb code in the rest of the
>     kernel.
>
>   * Explicitly build a few parts of sleep34xx.S as ARM.
>
>       * lock_scratchpad_sem is kept as ARM because of the need to
>         synchronise with hardware (?) using the SWP instruction.
>
>       * save_secure_ram_context and omap34xx_cpu_suspend are built
>         as ARM in case the Secure World firmware expects to decode
>         the comment field from the SMC (aka smi) instructions.
>
>         This can be undone later if the firmware is confirmed as
>         able to decode the Thumb SMC encoding (or ignores the
>         comment field).
>
>       * es3_sdrc_fix should presumably only be called from the
>         low-level wakeup code.  To minimise the diff, switched this
>         to ARM and demoted it to be a local symbol, since I believe
>         it shouldn't be called from outside anyway.
>
>      To prevent future maintainence problems, it would probably be
>      a good idea to demote _all_ functions which aren't called
>      externally to local.  (i.e., everything except for
>      get_es3_restore_pointer, get_restore_pointer,
>      omap34xx_cpu_suspend and save_secure_ram_context).
>
>      For now, I've left these as-is to minimise disruption.
>
>    * Use a separate base register instead of PC-relative stores in
>      sram34xx.S.  This isn't permitted in Thumb-2, but at least
>      some versions of gas will silently output non-working code,
>      leading to unpredictable run-time behaviour.
>
> pm34xx.c, pm.h, sram.c, sram.h:
>
>   * Resolve some memory addressing issues where a function symbol's
>     value is assumed to be equal to the start address of the
>     function body for the purpose of copying some low-level code
>     from sleep34xx.S to SRAM.
>
>     This assumption breaks for Thumb, since Thumb functions symbols
>     have bit 0 set to indicate the Thumb-ness.  This would result
>     in a non-working, off-by-one copy of the function body.
>
> Tested on Beagle xM A2:
>   * CONFIG_THUMB2_KERNEL && !CONFIG_SMP_ON_UP
>   * CONFIG_THUMB2_KERNEL && CONFIG_SMP_ON_UP
>   * !CONFIG_THUMB2_KERNEL && !CONFIG_SMP_ON_UP
>   * !CONFIG_THUMB2_KERNEL && CONFIG_SMP_ON_UP
>
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> ---
> KernelVersion: 2.6.37-rc4

No need to mention but this patch changes lot of things around
power management code. You said " Tested on: omap3 (Beagle xM A2)"

What did you test ? Is it just boot test ? For sure just boot
test is not enough for this patch and needs to test the PM.

>
>  arch/arm/mach-omap2/pm.h               |    2 +
>  arch/arm/mach-omap2/pm34xx.c           |   13 ++++++++--
>  arch/arm/mach-omap2/sleep34xx.S        |   37
> +++++++++++++++++++++++++++++--
>  arch/arm/mach-omap2/sram34xx.S         |   34
+++++++++++++++++++++------
> -
>  arch/arm/plat-omap/include/plat/sram.h |    1 +
>  arch/arm/plat-omap/sram.c              |   10 +++++++-
>  6 files changed, 80 insertions(+), 17 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index 0d75bfd..c333bfd 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -80,7 +80,9 @@ extern void save_secure_ram_context(u32 *addr);
>  extern void omap3_save_scratchpad_contents(void);
>
>  extern unsigned int omap24xx_idle_loop_suspend_sz;
> +extern char *const omap34xx_cpu_suspend_base;
>  extern unsigned int omap34xx_suspend_sz;
> +extern char *const save_secure_ram_context_base;
>  extern unsigned int save_secure_ram_context_sz;
>  extern unsigned int omap24xx_cpu_suspend_sz;
>  extern unsigned int omap34xx_cpu_suspend_sz;
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 0ec8a04..93f0ee8 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -982,11 +982,18 @@ static int __init clkdms_setup(struct clockdomain
> *clkdm, void *unused)
>
>  void omap_push_sram_idle(void)
>  {
> -	_omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
> +	_omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend_base,
>  					omap34xx_cpu_suspend_sz);
> -	if (omap_type() != OMAP2_DEVICE_TYPE_GP)
> -		_omap_save_secure_sram =
> omap_sram_push(save_secure_ram_context,
> +	_omap_sram_idle += (char *)omap34xx_cpu_suspend -
> +		omap34xx_cpu_suspend_base;
> +
> +	if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
> +		_omap_save_secure_sram = omap_sram_push(
> +				save_secure_ram_context_base,
>  				save_secure_ram_context_sz);
> +		_omap_save_secure_sram += (char *)save_secure_ram_context
-
> +			save_secure_ram_context_base;
> +	}
>  }
>
>  static int __init omap3_pm_init(void)
> diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-
> omap2/sleep34xx.S
> index 2fb205a..06ae955 100644
> --- a/arch/arm/mach-omap2/sleep34xx.S
> +++ b/arch/arm/mach-omap2/sleep34xx.S
> @@ -61,6 +61,7 @@
>
>          .text
>  /* Function to acquire the semaphore in scratchpad */
> +	.arm			@ Do this in ARM for now, due to use of
SWP.
>  ENTRY(lock_scratchpad_sem)
>  	stmfd	sp!, {lr}	@ save registers on stack
>  wait_sem:
> @@ -74,6 +75,9 @@ wait_loop:
>  	cmp	r2, r0		@ did we succeed ?
>  	beq	wait_sem	@ no - try again
>  	ldmfd	sp!, {pc}	@ restore regs and return
> +ENDPROC(lock_scratchpad_sem)
> + THUMB(	.thumb		)
> +	.align
>  sdrc_scratchpad_sem:
>          .word SDRC_SCRATCHPAD_SEM_V
>  ENTRY(lock_scratchpad_sem_sz)
> @@ -87,6 +91,7 @@ ENTRY(unlock_scratchpad_sem)
>  	mov	r2,#0
>  	str	r2,[r3]
>  	ldmfd	sp!, {pc}	@ restore regs and return
> +ENDPROC(unlock_scratchpad_sem)
>  ENTRY(unlock_scratchpad_sem_sz)
>          .word   . - unlock_scratchpad_sem
>
> @@ -96,6 +101,7 @@ ENTRY(get_restore_pointer)
>          stmfd   sp!, {lr}     @ save registers on stack
>  	adr	r0, restore
>          ldmfd   sp!, {pc}     @ restore regs and return
> +ENDPROC(get_restore_pointer)
>  ENTRY(get_restore_pointer_sz)
>          .word   . - get_restore_pointer
>
> @@ -105,10 +111,16 @@ ENTRY(get_es3_restore_pointer)
>  	stmfd	sp!, {lr}	@ save registers on stack
>  	adr	r0, restore_es3
>  	ldmfd	sp!, {pc}	@ restore regs and return
> +ENDPROC(get_es3_restore_pointer)
>  ENTRY(get_es3_restore_pointer_sz)
>  	.word	. - get_es3_restore_pointer
>
> -ENTRY(es3_sdrc_fix)
> +@ For simplicity, make this ARM so it gets called OK from es3_restore.
> +@ Demote to a local symbol, since this gives this function an ARM ABI
> interface
> +@ which won't be callable directly from a Thumb-2 kernel.  This code
> +@ shouldn't be called from outside anyway...
> +	.arm
> +es3_sdrc_fix:
>  	ldr	r4, sdrc_syscfg		@ get config addr
>  	ldr	r5, [r4]		@ get value
>  	tst	r5, #0x100		@ is part access blocked
> @@ -134,6 +146,9 @@ ENTRY(es3_sdrc_fix)
>  	mov	r5, #0x2		@ autorefresh command
>  	str	r5, [r4]		@ kick off refreshes
>  	bx	lr
> +ENDPROC(es3_sdrc_fix)
> + THUMB(	.thumb		)
> +	.align
>  sdrc_syscfg:
>  	.word	SDRC_SYSCONFIG_P
>  sdrc_mr_0:
> @@ -150,8 +165,12 @@ sdrc_manual_1:
>  	.word	SDRC_MANUAL_1_P
>  ENTRY(es3_sdrc_fix_sz)
>  	.word	. - es3_sdrc_fix
> + THUMB(	.thumb		)
>
>  /* Function to call rom code to save secure ram context */
> +	.arm			@ Do this in ARM for now, due to use of
SMC,
> +				@ in case the Secure World firmware may
depends
> +				@ on decoding the SMC instruction.
>  ENTRY(save_secure_ram_context)
>  	stmfd	sp!, {r1-r12, lr}	@ save registers on stack
>  save_secure_ram_debug:
> @@ -175,6 +194,9 @@ save_secure_ram_debug:
>  	nop
>  	nop
>  	ldmfd	sp!, {r1-r12, pc}
> +ENDPROC(save_secure_ram_context)
> + THUMB(	.thumb		)
> +	.align
>  sram_phy_addr_mask:
>  	.word	SRAM_BASE_P
>  high_mask:
> @@ -183,6 +205,8 @@ api_params:
>  	.word	0x4, 0x0, 0x0, 0x1, 0x1
>  ENTRY(save_secure_ram_context_sz)
>  	.word	. - save_secure_ram_context
> +ENTRY(save_secure_ram_context_base)
> +	.word	save_secure_ram_context_base
>
>  /*
>   * Forces OMAP into idle state
> @@ -193,6 +217,7 @@ ENTRY(save_secure_ram_context_sz)
>   * Note: This code get's copied to internal SRAM at boot. When the OMAP
>   *	 wakes up it continues execution at the point it went to sleep.
>   */
> +	.arm			@ Do this in ARM for now, due to use of
SMC.
>  ENTRY(omap34xx_cpu_suspend)
>  	stmfd	sp!, {r0-r12, lr}		@ save registers on stack
>  loop:
> @@ -563,10 +588,12 @@ loop2:
>  	mov     r9, r4
>  	/* create working copy of max way size*/
>  loop3:
> +	mov     r1, r9, lsl r5
> +	mov     r2, r7, lsl r2
>  	/* factor way and cache number into r11 */
> -	orr     r11, r10, r9, lsl r5
> +	orr     r11, r10, r1
>  	/* factor index number into r11 */
> -	orr     r11, r11, r7, lsl r2
> +	orr     r11, r11, r2
>  	/*clean & invalidate by set/way */
>  	mcr     p15, 0, r11, c7, c10, 2
>  	/* decrement the way*/
> @@ -631,7 +658,9 @@ wait_dll_lock:
>          cmp     r5, #0x4
>          bne     wait_dll_lock
>          bx      lr
> +ENDPROC(omap34xx_cpu_suspend)
>
> +	.align
>  cm_idlest1_core:
>  	.word	CM_IDLEST1_CORE_V
>  sdrc_dlla_status:
> @@ -670,3 +699,5 @@ control_stat:
>  	.word	CONTROL_STAT
>  ENTRY(omap34xx_cpu_suspend_sz)
>  	.word	. - omap34xx_cpu_suspend
> +ENTRY(omap34xx_cpu_suspend_base)
> +	.word	omap34xx_cpu_suspend
> diff --git a/arch/arm/mach-omap2/sram34xx.S b/arch/arm/mach-
> omap2/sram34xx.S
> index 3637274..65fd54f 100644
> --- a/arch/arm/mach-omap2/sram34xx.S
> +++ b/arch/arm/mach-omap2/sram34xx.S
> @@ -105,29 +105,42 @@
>   * can satisfy the above requirement can enable the
> CONFIG_OMAP3_SDRC_AC_TIMING
>   * option.
>   */
> +__omap3_sram_configure_core_dpll_base:	@ Separate local symbol
with the
> Thumb
> +					@ bit _not_ set (for base address
when
> +					@ copying to sram).
>  ENTRY(omap3_sram_configure_core_dpll)
>  	stmfd	sp!, {r1-r12, lr}	@ store regs to stack
>
>  					@ 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
> @@ -264,7 +277,9 @@ configure_sdrc:
>  skip_cs1_prog:
>  	ldr	r12, [r11]		@ posted-write barrier for SDRC
>  	bx	lr
> +ENDPROC(omap3_sram_configure_core_dpll)
>
> +	.align
>  omap3_sdrc_power:
>  	.word OMAP34XX_SDRC_REGADDR(SDRC_POWER)
>  omap3_cm_clksel1_pll:
> @@ -316,4 +331,5 @@ core_m2_mask_val:
>
>  ENTRY(omap3_sram_configure_core_dpll_sz)
>  	.word	. - omap3_sram_configure_core_dpll
> -
> +ENTRY(omap3_sram_configure_core_dpll_base)
> +	.word	__omap3_sram_configure_core_dpll_base
> diff --git a/arch/arm/plat-omap/include/plat/sram.h b/arch/arm/plat-
> omap/include/plat/sram.h
> index 5905100..2f27167 100644
> --- a/arch/arm/plat-omap/include/plat/sram.h
> +++ b/arch/arm/plat-omap/include/plat/sram.h
> @@ -67,6 +67,7 @@ extern u32 omap3_sram_configure_core_dpll(
>  			u32 sdrc_rfr_ctrl_1, u32 sdrc_actim_ctrl_a_1,
>  			u32 sdrc_actim_ctrl_b_1, u32 sdrc_mr_1);
>  extern unsigned long omap3_sram_configure_core_dpll_sz;
> +extern char *omap3_sram_configure_core_dpll_base;
>
>  #ifdef CONFIG_PM
>  extern void omap_push_sram_idle(void);
> diff --git a/arch/arm/plat-omap/sram.c b/arch/arm/plat-omap/sram.c
> index e2c8eeb..61282f4 100644
> --- a/arch/arm/plat-omap/sram.c
> +++ b/arch/arm/plat-omap/sram.c
> @@ -386,8 +386,11 @@ void omap3_sram_restore_context(void)
>  	omap_sram_ceil = omap_sram_base + omap_sram_size;
>
>  	_omap3_sram_configure_core_dpll =
> -		omap_sram_push(omap3_sram_configure_core_dpll,
> +		omap_sram_push(omap3_sram_configure_core_dpll_base,
>  			       omap3_sram_configure_core_dpll_sz);
> +	_omap3_sram_configure_core_dpll +=
> +		(char *)omap3_sram_configure_core_dpll -
> +		omap3_sram_configure_core_dpll_base;
>  	omap_push_sram_idle();
>  }
>  #endif /* CONFIG_PM */
> @@ -395,8 +398,11 @@ void omap3_sram_restore_context(void)
>  static int __init omap34xx_sram_init(void)
>  {
>  	_omap3_sram_configure_core_dpll =
> -		omap_sram_push(omap3_sram_configure_core_dpll,
> +		omap_sram_push(omap3_sram_configure_core_dpll_base,
>  			       omap3_sram_configure_core_dpll_sz);
> +	_omap3_sram_configure_core_dpll +=
> +		(char *)omap3_sram_configure_core_dpll -
> +		omap3_sram_configure_core_dpll_base;
>  	omap_push_sram_idle();
>  	return 0;
>  }
> --
> 1.7.1
>
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

[PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to work withCONFIG_THUMB2_KERNEL

[Dave Martin]

On Tue, Dec 7, 2010 at 6:31 AM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> Dave,
>> -----Original Message-----
>> From: linaro-dev-bounces at lists.linaro.org [mailto:linaro-dev-
>> bounces at lists.linaro.org] On Behalf Of Dave Martin
>> Sent: Monday, December 06, 2010 11:06 PM
>> To: linux-arm-kernel at lists.infradead.org
>> Cc: Tony Lindgren; Dave Martin; linux-omap at vger.kernel.org; linaro-
>> dev at lists.linaro.org
>> Subject: [PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to
> work
>> withCONFIG_THUMB2_KERNEL
>>

[...]

>
> No need to mention but this patch changes lot of things around
> power management code. You said " Tested on: omap3 (Beagle xM A2)"
>
> What did you test ? Is it just boot test ? For sure just boot
> test is not enough for this patch and needs to test the PM.

That's a fair point--- yes, I've only tested boot / reboot / shutdown so far.

If you have any thoughts about how to excercise the power management
functionality more completely, I'd be happy to have a go...

Cheers
---Dave

[PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to work withCONFIG_THUMB2_KERNEL

[Jean Pihet]

Hi Dave,

On Tue, Dec 7, 2010 at 11:16 AM, Dave Martin <dave.martin@linaro.org> wrote:
> On Tue, Dec 7, 2010 at 6:31 AM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
>> Dave,
>>> -----Original Message-----
>>> From: linaro-dev-bounces at lists.linaro.org [mailto:linaro-dev-
>>> bounces at lists.linaro.org] On Behalf Of Dave Martin
>>> Sent: Monday, December 06, 2010 11:06 PM
>>> To: linux-arm-kernel at lists.infradead.org
>>> Cc: Tony Lindgren; Dave Martin; linux-omap at vger.kernel.org; linaro-
>>> dev at lists.linaro.org
>>> Subject: [PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to
>> work
>>> withCONFIG_THUMB2_KERNEL
>>>
>
> [...]
>
>>
>> No need to mention but this patch changes lot of things around
>> power management code. You said " Tested on: omap3 (Beagle xM A2)"
>>
>> What did you test ? Is it just boot test ? For sure just boot
>> test is not enough for this patch and needs to test the PM.
>
> That's a fair point--- yes, I've only tested boot / reboot / shutdown so far.
>
The ASM idle code is being reworked right now, which means the T2
support will need to be reworked on top of the patches. Cf. [1] and
[2].

> If you have any thoughts about how to exercise the power management
> functionality more completely, I'd be happy to have a go...
Cf. [3] for more details on how to exercise the PM on the target. This
wiki page is slightly outdated but the idea is still the same. Note
that only cpuidle is currently supported correctly on l-o master.

[1] http://marc.info/?l=linux-omap&m=129139584919242&w=2
[2] http://marc.info/?l=linux-omap&m=129172034809447&w=2
[3] http://elinux.org/OMAP_Power_Management

>
> Cheers
> ---Dave

Thanks,
Jean

>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>

[PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to work withCONFIG_THUMB2_KERNEL

[Dave Martin]

Hi,

On Tue, Dec 7, 2010 at 11:59 AM, Jean Pihet <jean.pihet@newoldbits.com> wrote:
> Hi Dave,
>
> On Tue, Dec 7, 2010 at 11:16 AM, Dave Martin <dave.martin@linaro.org> wrote:
>> On Tue, Dec 7, 2010 at 6:31 AM, Santosh Shilimkar
>> <santosh.shilimkar@ti.com> wrote:
>>> Dave,
>>>> -----Original Message-----
>>>> From: linaro-dev-bounces at lists.linaro.org [mailto:linaro-dev-
>>>> bounces at lists.linaro.org] On Behalf Of Dave Martin
>>>> Sent: Monday, December 06, 2010 11:06 PM
>>>> To: linux-arm-kernel at lists.infradead.org
>>>> Cc: Tony Lindgren; Dave Martin; linux-omap at vger.kernel.org; linaro-
>>>> dev at lists.linaro.org
>>>> Subject: [PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to
>>> work
>>>> withCONFIG_THUMB2_KERNEL
>>>>
>>
>> [...]
>>
>>>
>>> No need to mention but this patch changes lot of things around
>>> power management code. You said " Tested on: omap3 (Beagle xM A2)"
>>>
>>> What did you test ? Is it just boot test ? For sure just boot
>>> test is not enough for this patch and needs to test the PM.
>>
>> That's a fair point--- yes, I've only tested boot / reboot / shutdown so far.
>>
> The ASM idle code is being reworked right now, which means the T2
> support will need to be reworked on top of the patches. Cf. [1] and
> [2].
[...]
On Tue, Dec 7, 2010 at 12:59 PM, Jean Pihet <jean.pihet@newoldbits.com> wrote:
[...]
> For your information, the on-going changes are:
> - run (most of the) code from the DDR instead of the internal SRAM.
> - convert the ASM idle code to C-code,
>
> I am working on getting those changes submitted, it should be done this week.
>
> Unfortunately those changes have implications on your changes for T2.
> As far as I understood converting the code to C-code should solve the
> T2 problem as well. What do you think?
>
> In any case I will let you know. Could you test the changes when they
> are available?
[...]

In general, converting the code to C will solve a lot of potential
problems.  If you don't copy the code, that also simplifies matters.

Note that converting to C doesn't mean that code which attempts to
copy function bodies will work: you still need to handle the fact that
if f() is a C function symbol, then the value of the symbol f is
actually the function's base address + 1.  See my changes in sram.c,
pm34xx.c etc.

If there are still bits of ARM code (such as the resume re-entry
point?) some care will still be needed there.

Anyone maintaining low-level and assembler code may find the following
useful: https://wiki.ubuntu.com/ARM/Thumb2PortingHowto

>> If you have any thoughts about how to exercise the power management
>> functionality more completely, I'd be happy to have a go...
> Cf. [3] for more details on how to exercise the PM on the target. This
> wiki page is slightly outdated but the idea is still the same. Note
> that only cpuidle is currently supported correctly on l-o master.
>
> [1] http://marc.info/?l=linux-omap&m=129139584919242&w=2
> [2] http://marc.info/?l=linux-omap&m=129172034809447&w=2
> [3] http://elinux.org/OMAP_Power_Management

Thanks for this info

Humm, which reference tree should I be using?  I'm not convinced it
works properly/usefully in the linaro trees ... only OMAP_PM_NOOP is
provided, and when I echo mem >/sys/power/state, the platform does not
crash (at least, it doesn't lock up) but doesn't appear to suspend
properly either.  Also, I'm not seeing your debugs contents.


On a separate topic, I have some firmware questions you might be able to answer:
  * Does the secure firmware attempt to read the comment field from
SMC instructions?  And if so, does it assume the SMC is in ARM code?
  * Is the secure firmware able to cope with a Thumb resume re-entry point?

Currently, I've taken the conservative approach and assume that the
answer to both of these questions is "no".  But this means that a few
bits of code need to be built as ARM which could otherwise be Thumb.
It would be cleaner to get the amount of ARM code down to an absolute
minimum when building a Thumb kernel.



Some specific comments on your patches:

After the label "finished:" in sleep34xx.S
+	ldr r1, kernel_flush
+	mov lr, pc
+	bx  r1

If the code might be built as Thumb-2, this won't work correctly,
because the Thumb bit (bit 0) will not get set properly in lr.
Instead:

+	ldr r1, kernel_flush
+	blx r1

Even if you think this code isn't going to be built for Thumb-2, it's
specific to v7+ platforms, so I recommend you use the above change
anyway: it's more compact and will help avoid maintenance problems
further down the line...


+	str	r4, wait_dll_lock_counter
[...]
+	str	r4, kick_counter

PC-relative stores are illegal in Thumb-2.  Worse, current binutils
will assemble invalid code if you do this in the source and build for
Thumb-2.  For details and my fix, search for "PC-relative stores" on
http://lists.arm.linux.org.uk/lurker/message/20101130.133117.a98bf92f.en.html



+ENTRY(get_omap3630_restore_pointer)
+        stmfd   sp!, {lr}     @ save registers on stack
+	adr	r0, restore_3630

If restore_3630 is a Thumb symbol, this will go wrong, because the
"Thumb bit" won't get set in the address.  Instead, you would need to
use "adr r0, BSYM(restore_3630)", but only if restore_3630 is built as
Thumb-2 when the kernel is built with CONFIG_THUMB2_KERNEL.

Cheers
---Dave

[PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to work withCONFIG_THUMB2_KERNEL

[Dave Martin]

On Tue, Dec 7, 2010 at 2:53 PM, Dave Martin <dave.martin@linaro.org> wrote:
[...]
> Note that converting to C doesn't mean that code which attempts to
> copy function bodies will work: you still need to handle the fact that
> if f() is a C function symbol, then the value of the symbol f is
> actually the function's base address + 1. ?See my changes in sram.c,

To clarify, this applies *if* f is a Thumb symbol.

Cheers
---Dave

[PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to work withCONFIG_THUMB2_KERNEL

[Nicolas Pitre]

On Tue, 7 Dec 2010, Dave Martin wrote:

> On Tue, Dec 7, 2010 at 2:53 PM, Dave Martin <dave.martin@linaro.org> wrote:
> [...]
> > Note that converting to C doesn't mean that code which attempts to
> > copy function bodies will work: you still need to handle the fact that
> > if f() is a C function symbol, then the value of the symbol f is
> > actually the function's base address + 1. ?See my changes in sram.c,
> 
> To clarify, this applies *if* f is a Thumb symbol.

To make it generic, a new macro could be used:

#define SYM_ADDR(x) ((void *)((long)(x) & ~1L))


Nicolas

[PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to work withCONFIG_THUMB2_KERNEL

[Dave Martin]

Hi,

On Tue, Dec 7, 2010 at 7:15 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Tue, 7 Dec 2010, Dave Martin wrote:
>
>> On Tue, Dec 7, 2010 at 2:53 PM, Dave Martin <dave.martin@linaro.org> wrote:
>> [...]
>> > Note that converting to C doesn't mean that code which attempts to
>> > copy function bodies will work: you still need to handle the fact that
>> > if f() is a C function symbol, then the value of the symbol f is
>> > actually the function's base address + 1. ?See my changes in sram.c,
>>
>> To clarify, this applies *if* f is a Thumb symbol.
>
> To make it generic, a new macro could be used:
>
> #define SYM_ADDR(x) ((void *)((long)(x) & ~1L))

Could do ... I wasn't sure if it was useful for just this one case,
but I guess we may encounter others.  And it would make the code a lot
less messy...

if so, a macro for exracting just the Thumb bit, and a macro for
rebasing a symbol while preserving the Thumb bit could also be useful.

#define SYM_STATE(x) ((long)(x) & 1)
#define SYM_REBASE(new_addr, sym) ((void *)((long)SYM_ADDR(new_addr) |
SYM_STATE(sym)))

The relationship could be a bit clearer if we use the name SYM_BASE
instead of SYM_ADDR.

With these, the affected code becomes something like:

memcpy(buffer, SYM_BASE(f), size_of_f);
new_f = SYM_REBASE(f, buffer);

What do you think?

Is there a recommended type for a pointer-sized integer?  I notice
linux/types.h defines uintptr_t (as unsigned long).

Cheers
---Dave

[PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to work withCONFIG_THUMB2_KERNEL

[Nicolas Pitre]

On Wed, 8 Dec 2010, Dave Martin wrote:

> Hi,
> 
> On Tue, Dec 7, 2010 at 7:15 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > On Tue, 7 Dec 2010, Dave Martin wrote:
> >
> >> On Tue, Dec 7, 2010 at 2:53 PM, Dave Martin <dave.martin@linaro.org> wrote:
> >> [...]
> >> > Note that converting to C doesn't mean that code which attempts to
> >> > copy function bodies will work: you still need to handle the fact that
> >> > if f() is a C function symbol, then the value of the symbol f is
> >> > actually the function's base address + 1. ?See my changes in sram.c,
> >>
> >> To clarify, this applies *if* f is a Thumb symbol.
> >
> > To make it generic, a new macro could be used:
> >
> > #define SYM_ADDR(x) ((void *)((long)(x) & ~1L))
> 
> Could do ... I wasn't sure if it was useful for just this one case,
> but I guess we may encounter others.  And it would make the code a lot
> less messy...

That also makes the code self documenting.

> if so, a macro for exracting just the Thumb bit, and a macro for
> rebasing a symbol while preserving the Thumb bit could also be useful.
> 
> #define SYM_STATE(x) ((long)(x) & 1)
> #define SYM_REBASE(new_addr, sym) ((void *)((long)SYM_ADDR(new_addr) |
> SYM_STATE(sym)))
> 
> The relationship could be a bit clearer if we use the name SYM_BASE
> instead of SYM_ADDR.
> 
> With these, the affected code becomes something like:
> 
> memcpy(buffer, SYM_BASE(f), size_of_f);
> new_f = SYM_REBASE(f, buffer);
> 
> What do you think?

Looks fine to me.

> Is there a recommended type for a pointer-sized integer?  I notice
> linux/types.h defines uintptr_t (as unsigned long).

If uintptr_t is already defined then it is probably a good idea to just 
use it.


Nicolas

[PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi() for CONFIG_THUMB2_KERNEL

[Dave Martin]

For the Thumb-2 case, the "wfi" mnemonic is used, since in this
case the tools will necessarily be new enough to support it.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
KernelVersion: 2.6.37-rc4

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

diff --git a/arch/arm/mach-omap2/include/mach/omap4-common.h b/arch/arm/mach-omap2/include/mach/omap4-common.h
index 2744dfe..c6b1320 100644
--- a/arch/arm/mach-omap2/include/mach/omap4-common.h
+++ b/arch/arm/mach-omap2/include/mach/omap4-common.h
@@ -17,8 +17,13 @@
  * 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;
-- 
1.7.1

[PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi() forCONFIG_THUMB2_KERNEL

[Santosh Shilimkar]

Dave,
> -----Original Message-----
> From: linaro-dev-bounces at lists.linaro.org [mailto:linaro-dev-
> bounces at lists.linaro.org] On Behalf Of Dave Martin
> Sent: Monday, December 06, 2010 11:06 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: Tony Lindgren; Dave Martin; linux-omap at vger.kernel.org; linaro-
> dev at lists.linaro.org
> Subject: [PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi()
> forCONFIG_THUMB2_KERNEL
>
> For the Thumb-2 case, the "wfi" mnemonic is used, since in this
> case the tools will necessarily be new enough to support it.
>
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> ---
> KernelVersion: 2.6.37-rc4

The choice of opcode instead of instruction here was not because
of toolchain. The problem was it breaks multi-omap build where
ARMv6 and ARMv7 are build together.

For this reason I NAK this patch.

>
>  arch/arm/mach-omap2/include/mach/omap4-common.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/include/mach/omap4-common.h
> b/arch/arm/mach-omap2/include/mach/omap4-common.h
> index 2744dfe..c6b1320 100644
> --- a/arch/arm/mach-omap2/include/mach/omap4-common.h
> +++ b/arch/arm/mach-omap2/include/mach/omap4-common.h
> @@ -17,8 +17,13 @@
>   * 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;
> --
> 1.7.1
>
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

[PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi() forCONFIG_THUMB2_KERNEL

[Dave Martin]

Hi,

On Tue, Dec 7, 2010 at 6:28 AM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> Dave,
>> -----Original Message-----
>> From: linaro-dev-bounces at lists.linaro.org [mailto:linaro-dev-
>> bounces at lists.linaro.org] On Behalf Of Dave Martin
>> Sent: Monday, December 06, 2010 11:06 PM
>> To: linux-arm-kernel at lists.infradead.org
>> Cc: Tony Lindgren; Dave Martin; linux-omap at vger.kernel.org; linaro-
>> dev at lists.linaro.org
>> Subject: [PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi()
>> forCONFIG_THUMB2_KERNEL
>>
>> For the Thumb-2 case, the "wfi" mnemonic is used, since in this
>> case the tools will necessarily be new enough to support it.
>>
>> Signed-off-by: Dave Martin <dave.martin@linaro.org>
>> ---
>> KernelVersion: 2.6.37-rc4
>
> The choice of opcode instead of instruction here was not because
> of toolchain. The problem was it breaks multi-omap build where
> ARMv6 and ARMv7 are build together.
>
> For this reason I NAK this patch.

You can't built a kernel for pre-v7 platforms with
CONFIG_THUMB2_KERNEL: the code can't run on those platforms because
they don't support Thumb-2.

So anything inside #ifdef CONFIG_THUMB2_KERNEL can assume v7/Thumb-2
capable (and hence reasonably new) tools.

I'll follow up shortly with a patch to the generic ARM Kconfig to make
this explicit, so that ARCH_OMAP2 and THUMB2_KERNEL can't accidentally
be configured together.

Cheers
---Dave

[PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi() forCONFIG_THUMB2_KERNEL

[Russell King - ARM Linux]

On Tue, Dec 07, 2010 at 04:50:50PM +0000, Dave Martin wrote:
> I'll follow up shortly with a patch to the generic ARM Kconfig to make
> this explicit, so that ARCH_OMAP2 and THUMB2_KERNEL can't accidentally
> be configured together.

That's a rubbish dependency.  You may have an ARCH_OMAP2 platform which
is Cortex A9, and you only have Cortex A9 support selected.  In that
case there's no reason to prevent THUMB2_KERNEL being selected.

The right thing to do is to make THUMB2_KERNEL depend on those CPUs
which are not to be selected - not by making it depend on some platform
configuration option(s) which aren't actually relevent.

[PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi() forCONFIG_THUMB2_KERNEL

[Dave Martin]

On Tue, Dec 7, 2010 at 7:29 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Dec 07, 2010 at 04:50:50PM +0000, Dave Martin wrote:
>> I'll follow up shortly with a patch to the generic ARM Kconfig to make
>> this explicit, so that ARCH_OMAP2 and THUMB2_KERNEL can't accidentally
>> be configured together.
>
> That's a rubbish dependency. ?You may have an ARCH_OMAP2 platform which
> is Cortex A9, and you only have Cortex A9 support selected. ?In that
> case there's no reason to prevent THUMB2_KERNEL being selected.
>
> The right thing to do is to make THUMB2_KERNEL depend on those CPUs
> which are not to be selected - not by making it depend on some platform
> configuration option(s) which aren't actually relevent.
>

You're correct on this point-- this was my conclusion also.

If you haven't already, take a look at my other post
http://www.spinics.net/lists/arm-kernel/msg106576.html

Does this address your concern?

Cheers
---Dave

[PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi() forCONFIG_THUMB2_KERNEL

[Santosh Shilimkar]

> -----Original Message-----
> From: Dave Martin [mailto:dave.martin at linaro.org]
> Sent: Tuesday, December 07, 2010 10:21 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
> omap at vger.kernel.org; linaro-dev at lists.linaro.org
> Subject: Re: [PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi()
> forCONFIG_THUMB2_KERNEL
>
> Hi,
>
> On Tue, Dec 7, 2010 at 6:28 AM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
> > Dave,
> >> -----Original Message-----
> >> From: linaro-dev-bounces at lists.linaro.org [mailto:linaro-dev-
> >> bounces at lists.linaro.org] On Behalf Of Dave Martin
> >> Sent: Monday, December 06, 2010 11:06 PM
> >> To: linux-arm-kernel at lists.infradead.org
> >> Cc: Tony Lindgren; Dave Martin; linux-omap at vger.kernel.org; linaro-
> >> dev at lists.linaro.org
> >> Subject: [PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi()
> >> forCONFIG_THUMB2_KERNEL
> >>
> >> For the Thumb-2 case, the "wfi" mnemonic is used, since in this
> >> case the tools will necessarily be new enough to support it.
> >>
> >> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> >> ---
> >> KernelVersion: 2.6.37-rc4
> >
> > The choice of opcode instead of instruction here was not because
> > of toolchain. The problem was it breaks multi-omap build where
> > ARMv6 and ARMv7 are build together.
> >
> > For this reason I NAK this patch.
>
> You can't built a kernel for pre-v7 platforms with
> CONFIG_THUMB2_KERNEL: the code can't run on those platforms because
> they don't support Thumb-2.
>
That's better.

> So anything inside #ifdef CONFIG_THUMB2_KERNEL can assume v7/Thumb-2
> capable (and hence reasonably new) tools.
>
> I'll follow up shortly with a patch to the generic ARM Kconfig to make
> this explicit, so that ARCH_OMAP2 and THUMB2_KERNEL can't accidentally
> be configured together.
>
sure

[PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi() forCONFIG_THUMB2_KERNEL

[Santosh Shilimkar]

Dave,

> -----Original Message-----
> From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
> Sent: Wednesday, December 08, 2010 11:27 AM
> To: Dave Martin
> Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
> omap at vger.kernel.org; linaro-dev at lists.linaro.org
> Subject: RE: [PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi()
> forCONFIG_THUMB2_KERNEL
>
> > -----Original Message-----
> > From: Dave Martin [mailto:dave.martin at linaro.org]
> > Sent: Tuesday, December 07, 2010 10:21 PM
> > To: Santosh Shilimkar
> > Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
> > omap at vger.kernel.org; linaro-dev at lists.linaro.org
> > Subject: Re: [PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi()
> > forCONFIG_THUMB2_KERNEL
> >
> > Hi,
> >
> > On Tue, Dec 7, 2010 at 6:28 AM, Santosh Shilimkar
> > <santosh.shilimkar@ti.com> wrote:
> > > Dave,

[.....]
>
> > So anything inside #ifdef CONFIG_THUMB2_KERNEL can assume v7/Thumb-2
> > capable (and hence reasonably new) tools.
> >
> > I'll follow up shortly with a patch to the generic ARM Kconfig to make
> > this explicit, so that ARCH_OMAP2 and THUMB2_KERNEL can't accidentally
> > be configured together.
> >
> sure

When you are doing the changes can you please check if you could build
the THUMB2 kernel with omap2plus_defconfig. I suspect the build will
fail.

Regards,
Santosh

[PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi() forCONFIG_THUMB2_KERNEL

[Dave Martin]

On Wed, Dec 8, 2010 at 5:59 AM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> Dave,
>
>> -----Original Message-----
>> From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
>> Sent: Wednesday, December 08, 2010 11:27 AM
>> To: Dave Martin
>> Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
>> omap at vger.kernel.org; linaro-dev at lists.linaro.org
>> Subject: RE: [PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi()
>> forCONFIG_THUMB2_KERNEL
>>
>> > -----Original Message-----
>> > From: Dave Martin [mailto:dave.martin at linaro.org]
>> > Sent: Tuesday, December 07, 2010 10:21 PM
>> > To: Santosh Shilimkar
>> > Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
>> > omap at vger.kernel.org; linaro-dev at lists.linaro.org
>> > Subject: Re: [PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi()
>> > forCONFIG_THUMB2_KERNEL
>> >
>> > Hi,
>> >
>> > On Tue, Dec 7, 2010 at 6:28 AM, Santosh Shilimkar
>> > <santosh.shilimkar@ti.com> wrote:
>> > > Dave,
>
> [.....]
>>
>> > So anything inside #ifdef CONFIG_THUMB2_KERNEL can assume v7/Thumb-2
>> > capable (and hence reasonably new) tools.
>> >
>> > I'll follow up shortly with a patch to the generic ARM Kconfig to make
>> > this explicit, so that ARCH_OMAP2 and THUMB2_KERNEL can't accidentally
>> > be configured together.
>> >
>> sure
>
> When you are doing the changes can you please check if you could build
> the THUMB2 kernel with omap2plus_defconfig. I suspect the build will
> fail.

With my Kconfig patch, kconfig won't let you turn on Thumb-2 in that
configuration:

http://lists.arm.linux.org.uk/lurker/message/20101207.165737.0897658f.en.html

If you want to turn on Thumb-2, you must disable ARCH_OMAP2 first.  If
my understanding is correct, this is the right behaviour.


The kernel builds, fine but in ARM.

I hope that clarifies things...

Cheers
---Dave

[PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi() forCONFIG_THUMB2_KERNEL

[Santosh Shilimkar]

> -----Original Message-----
> From: Dave Martin [mailto:dave.martin at linaro.org]
> Sent: Wednesday, December 08, 2010 4:11 PM
> To: Santosh Shilimkar
> Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
> omap at vger.kernel.org; linaro-dev at lists.linaro.org
> Subject: Re: [PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi()
> forCONFIG_THUMB2_KERNEL
>
> On Wed, Dec 8, 2010 at 5:59 AM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
> > Dave,
> >
> >> -----Original Message-----
> >> From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
> >> Sent: Wednesday, December 08, 2010 11:27 AM
> >> To: Dave Martin
> >> Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
> >> omap at vger.kernel.org; linaro-dev at lists.linaro.org
> >> Subject: RE: [PATCH v2 2/3] ARM: omap4: Correct definition of
do_wfi()
> >> forCONFIG_THUMB2_KERNEL
> >>
> >> > -----Original Message-----
> >> > From: Dave Martin [mailto:dave.martin at linaro.org]
> >> > Sent: Tuesday, December 07, 2010 10:21 PM
> >> > To: Santosh Shilimkar
> >> > Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
> >> > omap at vger.kernel.org; linaro-dev at lists.linaro.org
> >> > Subject: Re: [PATCH v2 2/3] ARM: omap4: Correct definition of
> do_wfi()
> >> > forCONFIG_THUMB2_KERNEL
> >> >
> >> > Hi,
> >> >
> >> > On Tue, Dec 7, 2010 at 6:28 AM, Santosh Shilimkar
> >> > <santosh.shilimkar@ti.com> wrote:
> >> > > Dave,
> >
> > [.....]
> >>
> >> > So anything inside #ifdef CONFIG_THUMB2_KERNEL can assume
v7/Thumb-2
> >> > capable (and hence reasonably new) tools.
> >> >
> >> > I'll follow up shortly with a patch to the generic ARM Kconfig to
> make
> >> > this explicit, so that ARCH_OMAP2 and THUMB2_KERNEL can't
> accidentally
> >> > be configured together.
> >> >
> >> sure
> >
> > When you are doing the changes can you please check if you could build
> > the THUMB2 kernel with omap2plus_defconfig. I suspect the build will
> > fail.
>
> With my Kconfig patch, kconfig won't let you turn on Thumb-2 in that
> configuration:
>
>
http://lists.arm.linux.org.uk/lurker/message/20101207.165737.0897658f.en.h
> tml
>
> If you want to turn on Thumb-2, you must disable ARCH_OMAP2 first.  If
> my understanding is correct, this is the right behaviour.
>
>
Ofcourse it will build with ARCH_OMAP2 disabled :) (ARMv6 dependency)
In other words, I wanted to say that "omap2plus_defconfig" can't
be used as is to build THUMB kernel binary.

Thanks for conforming.

[PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi() forCONFIG_THUMB2_KERNEL

[Dave Martin]

On Wed, Dec 8, 2010 at 10:45 AM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
>> -----Original Message-----
>> From: Dave Martin [mailto:dave.martin at linaro.org]
>> Sent: Wednesday, December 08, 2010 4:11 PM
>> To: Santosh Shilimkar
>> Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
>> omap at vger.kernel.org; linaro-dev at lists.linaro.org
>> Subject: Re: [PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi()
>> forCONFIG_THUMB2_KERNEL
>>
>> On Wed, Dec 8, 2010 at 5:59 AM, Santosh Shilimkar
>> <santosh.shilimkar@ti.com> wrote:
>> > Dave,
>> >
>> >> -----Original Message-----
>> >> From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
>> >> Sent: Wednesday, December 08, 2010 11:27 AM
>> >> To: Dave Martin
>> >> Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
>> >> omap at vger.kernel.org; linaro-dev at lists.linaro.org
>> >> Subject: RE: [PATCH v2 2/3] ARM: omap4: Correct definition of
> do_wfi()
>> >> forCONFIG_THUMB2_KERNEL
>> >>
>> >> > -----Original Message-----
>> >> > From: Dave Martin [mailto:dave.martin at linaro.org]
>> >> > Sent: Tuesday, December 07, 2010 10:21 PM
>> >> > To: Santosh Shilimkar
>> >> > Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
>> >> > omap at vger.kernel.org; linaro-dev at lists.linaro.org
>> >> > Subject: Re: [PATCH v2 2/3] ARM: omap4: Correct definition of
>> do_wfi()
>> >> > forCONFIG_THUMB2_KERNEL
>> >> >
>> >> > Hi,
>> >> >
>> >> > On Tue, Dec 7, 2010 at 6:28 AM, Santosh Shilimkar
>> >> > <santosh.shilimkar@ti.com> wrote:
>> >> > > Dave,
>> >
>> > [.....]
>> >>
>> >> > So anything inside #ifdef CONFIG_THUMB2_KERNEL can assume
> v7/Thumb-2
>> >> > capable (and hence reasonably new) tools.
>> >> >
>> >> > I'll follow up shortly with a patch to the generic ARM Kconfig to
>> make
>> >> > this explicit, so that ARCH_OMAP2 and THUMB2_KERNEL can't
>> accidentally
>> >> > be configured together.
>> >> >
>> >> sure
>> >
>> > When you are doing the changes can you please check if you could build
>> > the THUMB2 kernel with omap2plus_defconfig. I suspect the build will
>> > fail.
>>
>> With my Kconfig patch, kconfig won't let you turn on Thumb-2 in that
>> configuration:
>>
>>
> http://lists.arm.linux.org.uk/lurker/message/20101207.165737.0897658f.en.h
>> tml
>>
>> If you want to turn on Thumb-2, you must disable ARCH_OMAP2 first. ?If
>> my understanding is correct, this is the right behaviour.
>>
>>
> Ofcourse it will build with ARCH_OMAP2 disabled :) (ARMv6 dependency)
> In other words, I wanted to say that "omap2plus_defconfig" can't
> be used as is to build THUMB kernel binary.

Well, yes, that's another way of looking at it.  Anyway, I think this
is the intended result -- is it OK for you?

Cheers
---Dave

[PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi() forCONFIG_THUMB2_KERNEL

[Santosh Shilimkar]

> -----Original Message-----
> From: Dave Martin [mailto:dave.martin at linaro.org]
> Sent: Wednesday, December 08, 2010 4:35 PM
> To: Santosh Shilimkar
> Cc: Tony Lindgren; linux-omap at vger.kernel.org; linaro-
> dev at lists.linaro.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi()
> forCONFIG_THUMB2_KERNEL
>
> On Wed, Dec 8, 2010 at 10:45 AM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
> >> -----Original Message-----
> >> From: Dave Martin [mailto:dave.martin at linaro.org]
> >> Sent: Wednesday, December 08, 2010 4:11 PM
> >> To: Santosh Shilimkar
> >> Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
> >> omap at vger.kernel.org; linaro-dev at lists.linaro.org
> >> Subject: Re: [PATCH v2 2/3] ARM: omap4: Correct definition of
do_wfi()
> >> forCONFIG_THUMB2_KERNEL
> >>
> >> On Wed, Dec 8, 2010 at 5:59 AM, Santosh Shilimkar
> >> <santosh.shilimkar@ti.com> wrote:
> >> > Dave,
> >> >
> >> >> -----Original Message-----
> >> >> From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
> >> >> Sent: Wednesday, December 08, 2010 11:27 AM
> >> >> To: Dave Martin
> >> >> Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
> >> >> omap at vger.kernel.org; linaro-dev at lists.linaro.org
> >> >> Subject: RE: [PATCH v2 2/3] ARM: omap4: Correct definition of
> > do_wfi()
> >> >> forCONFIG_THUMB2_KERNEL
> >> >>
> >> >> > -----Original Message-----
> >> >> > From: Dave Martin [mailto:dave.martin at linaro.org]
> >> >> > Sent: Tuesday, December 07, 2010 10:21 PM
> >> >> > To: Santosh Shilimkar
> >> >> > Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
> >> >> > omap at vger.kernel.org; linaro-dev at lists.linaro.org
> >> >> > Subject: Re: [PATCH v2 2/3] ARM: omap4: Correct definition of
> >> do_wfi()
> >> >> > forCONFIG_THUMB2_KERNEL
> >> >> >
> >> >> > Hi,
> >> >> >
> >> >> > On Tue, Dec 7, 2010 at 6:28 AM, Santosh Shilimkar
> >> >> > <santosh.shilimkar@ti.com> wrote:
> >> >> > > Dave,
> >> >
> >> > [.....]
> >> >>
> >> >> > So anything inside #ifdef CONFIG_THUMB2_KERNEL can assume
> > v7/Thumb-2
> >> >> > capable (and hence reasonably new) tools.
> >> >> >
> >> >> > I'll follow up shortly with a patch to the generic ARM Kconfig
to
> >> make
> >> >> > this explicit, so that ARCH_OMAP2 and THUMB2_KERNEL can't
> >> accidentally
> >> >> > be configured together.
> >> >> >
> >> >> sure
> >> >
> >> > When you are doing the changes can you please check if you could
> build
> >> > the THUMB2 kernel with omap2plus_defconfig. I suspect the build
will
> >> > fail.
> >>
> >> With my Kconfig patch, kconfig won't let you turn on Thumb-2 in that
> >> configuration:
> >>
> >>
> >
>
http://lists.arm.linux.org.uk/lurker/message/20101207.165737.0897658f.en.h
> >> tml
> >>
> >> If you want to turn on Thumb-2, you must disable ARCH_OMAP2 first.
?If
> >> my understanding is correct, this is the right behaviour.
> >>
> >>
> > Ofcourse it will build with ARCH_OMAP2 disabled :) (ARMv6 dependency)
> > In other words, I wanted to say that "omap2plus_defconfig" can't
> > be used as is to build THUMB kernel binary.
>
> Well, yes, that's another way of looking at it.  Anyway, I think this
> is the intended result -- is it OK for you?
>
Should be ok considering you can't do much with it. But I let Tony comment
on it.

Regards,
Santosh

[PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi() forCONFIG_THUMB2_KERNEL

[Dave Martin]

On Wed, Dec 8, 2010 at 11:10 AM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
>> -----Original Message-----
>> From: Dave Martin [mailto:dave.martin at linaro.org]
>> Sent: Wednesday, December 08, 2010 4:35 PM
>> To: Santosh Shilimkar
>> Cc: Tony Lindgren; linux-omap at vger.kernel.org; linaro-
>> dev at lists.linaro.org; linux-arm-kernel at lists.infradead.org
>> Subject: Re: [PATCH v2 2/3] ARM: omap4: Correct definition of do_wfi()
>> forCONFIG_THUMB2_KERNEL
>>
>> On Wed, Dec 8, 2010 at 10:45 AM, Santosh Shilimkar
>> <santosh.shilimkar@ti.com> wrote:
>> >> -----Original Message-----
>> >> From: Dave Martin [mailto:dave.martin at linaro.org]
>> >> Sent: Wednesday, December 08, 2010 4:11 PM
>> >> To: Santosh Shilimkar
>> >> Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
>> >> omap at vger.kernel.org; linaro-dev at lists.linaro.org
>> >> Subject: Re: [PATCH v2 2/3] ARM: omap4: Correct definition of
> do_wfi()
>> >> forCONFIG_THUMB2_KERNEL
>> >>
>> >> On Wed, Dec 8, 2010 at 5:59 AM, Santosh Shilimkar
>> >> <santosh.shilimkar@ti.com> wrote:
>> >> > Dave,
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
>> >> >> Sent: Wednesday, December 08, 2010 11:27 AM
>> >> >> To: Dave Martin
>> >> >> Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
>> >> >> omap at vger.kernel.org; linaro-dev at lists.linaro.org
>> >> >> Subject: RE: [PATCH v2 2/3] ARM: omap4: Correct definition of
>> > do_wfi()
>> >> >> forCONFIG_THUMB2_KERNEL
>> >> >>
>> >> >> > -----Original Message-----
>> >> >> > From: Dave Martin [mailto:dave.martin at linaro.org]
>> >> >> > Sent: Tuesday, December 07, 2010 10:21 PM
>> >> >> > To: Santosh Shilimkar
>> >> >> > Cc: linux-arm-kernel at lists.infradead.org; Tony Lindgren; linux-
>> >> >> > omap at vger.kernel.org; linaro-dev at lists.linaro.org
>> >> >> > Subject: Re: [PATCH v2 2/3] ARM: omap4: Correct definition of
>> >> do_wfi()
>> >> >> > forCONFIG_THUMB2_KERNEL
>> >> >> >
>> >> >> > Hi,
>> >> >> >
>> >> >> > On Tue, Dec 7, 2010 at 6:28 AM, Santosh Shilimkar
>> >> >> > <santosh.shilimkar@ti.com> wrote:
>> >> >> > > Dave,
>> >> >
>> >> > [.....]
>> >> >>
>> >> >> > So anything inside #ifdef CONFIG_THUMB2_KERNEL can assume
>> > v7/Thumb-2
>> >> >> > capable (and hence reasonably new) tools.
>> >> >> >
>> >> >> > I'll follow up shortly with a patch to the generic ARM Kconfig
> to
>> >> make
>> >> >> > this explicit, so that ARCH_OMAP2 and THUMB2_KERNEL can't
>> >> accidentally
>> >> >> > be configured together.
>> >> >> >
>> >> >> sure
>> >> >
>> >> > When you are doing the changes can you please check if you could
>> build
>> >> > the THUMB2 kernel with omap2plus_defconfig. I suspect the build
> will
>> >> > fail.
>> >>
>> >> With my Kconfig patch, kconfig won't let you turn on Thumb-2 in that
>> >> configuration:
>> >>
>> >>
>> >
>>
> http://lists.arm.linux.org.uk/lurker/message/20101207.165737.0897658f.en.h
>> >> tml
>> >>
>> >> If you want to turn on Thumb-2, you must disable ARCH_OMAP2 first.
> ?If
>> >> my understanding is correct, this is the right behaviour.
>> >>
>> >>
>> > Ofcourse it will build with ARCH_OMAP2 disabled :) (ARMv6 dependency)
>> > In other words, I wanted to say that "omap2plus_defconfig" can't
>> > be used as is to build THUMB kernel binary.
>>
>> Well, yes, that's another way of looking at it. ?Anyway, I think this
>> is the intended result -- is it OK for you?
>>
> Should be ok considering you can't do much with it. But I let Tony comment
> on it.

Well, it depends on what you're trying to do.  We can't combine
Thumb-2 with a combined omap kernel intended to support omap2.  But in
linaro for example we are targeting v7+ only for the kernel and
userspace, so we already build the kernel for v7 and don't include the
OMAP2 support; instead we target OMAP3/4.  I believe the situation for
Ubuntu is similar.  In our case, including the omap2 support wouldn't
be useful, since the userspace won't run on that platform anyway...

So really, my concerns are that a) the generic rules for
CONFIG_THUMB2_KERNEL are correct, and b) the omap tree works well with
this configuration to the maximum extent possible.  Obviously, mixing
ARCH_OMAP2 with THUMB2_KERNEL _isn't_ possible even in theory, so I
don't consider it a failure if that combination isn't supported.

Cheers
---Dave

[PATCH v2 3/3] ARM: omap4: Convert END() to ENDPROC() for correct linkage with CONFIG_THUMB2_KERNEL

[Dave Martin]

almost all code for v7+ platforms) is deprecated/incorrect.

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.

omap_secondary_startup might still need to be changed to ARM,
depending on the compatibility status of bootloaders.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
KernelVersion: 2.6.37-rc4

 arch/arm/mach-omap2/omap-headsmp.S |    2 +-
 arch/arm/mach-omap2/omap44xx-smc.S |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/omap-headsmp.S b/arch/arm/mach-omap2/omap-headsmp.S
index 6ae937a..4ee6aec 100644
--- a/arch/arm/mach-omap2/omap-headsmp.S
+++ b/arch/arm/mach-omap2/omap-headsmp.S
@@ -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)
 
diff --git a/arch/arm/mach-omap2/omap44xx-smc.S b/arch/arm/mach-omap2/omap44xx-smc.S
index 1980dc3..e69d37d 100644
--- a/arch/arm/mach-omap2/omap44xx-smc.S
+++ b/arch/arm/mach-omap2/omap44xx-smc.S
@@ -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)
-- 
1.7.1

[PATCH v2 3/3] ARM: omap4: Convert END() to ENDPROC() for correctlinkage with CONFIG_THUMB2_KERNEL

[Santosh Shilimkar]

> -----Original Message-----
> From: linaro-dev-bounces at lists.linaro.org [mailto:linaro-dev-
> bounces at lists.linaro.org] On Behalf Of Dave Martin
> Sent: Monday, December 06, 2010 11:06 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: Tony Lindgren; Dave Martin; linux-omap at vger.kernel.org; linaro-
> dev at lists.linaro.org
> Subject: [PATCH v2 3/3] ARM: omap4: Convert END() to ENDPROC() for
> correctlinkage with CONFIG_THUMB2_KERNEL
>
> almost all code for v7+ platforms) is deprecated/incorrect.
>
> 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.
>
> omap_secondary_startup might still need to be changed to ARM,
> depending on the compatibility status of bootloaders.
>
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> ---
> KernelVersion: 2.6.37-rc4

Looks ok to me.
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>
>  arch/arm/mach-omap2/omap-headsmp.S |    2 +-
>  arch/arm/mach-omap2/omap44xx-smc.S |    8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap-headsmp.S b/arch/arm/mach-
> omap2/omap-headsmp.S
> index 6ae937a..4ee6aec 100644
> --- a/arch/arm/mach-omap2/omap-headsmp.S
> +++ b/arch/arm/mach-omap2/omap-headsmp.S
> @@ -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)
>
> diff --git a/arch/arm/mach-omap2/omap44xx-smc.S b/arch/arm/mach-
> omap2/omap44xx-smc.S
> index 1980dc3..e69d37d 100644
> --- a/arch/arm/mach-omap2/omap44xx-smc.S
> +++ b/arch/arm/mach-omap2/omap44xx-smc.S
> @@ -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)
> --
> 1.7.1
>
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev