[PATCH 5/7] OMAP3: rework of the ASM sleep code execution paths

Nishanth Menon nm at ti.com
Fri Dec 17 10:35:52 EST 2010


jean.pihet at newoldbits.com had written, on 12/17/2010 04:08 AM, the 
following:
> From: Jean Pihet <j-pihet at ti.com>
> 
> - Reworked and simplified the execution paths for better
>   readability and to avoid duplication of code,
is it possible to split this good rewrite into logical chunks for better 
and easier reviewability?

my suggestion clean each of the paths independently - like the code upto 
wfi.. and then we can clean up the resume path as well separately.

> - Added comments on the entry and exit points and the interaction
>   with the ROM code for OFF mode restore,
> - Reworked the existing comments for better readability.
thanks for doing this, but, just my suggestion, looking at the amount of 
changes done in this patch -> if you can keep one patch for functional 
change which is separate from cosmetic change (such as comment cleanup 
and addition), it makes a reviewer's life easier :)

> 
> Tested on N900 and Beagleboard with full RET and OFF modes,
> using cpuidle and suspend.
> 
> Signed-off-by: Jean Pihet <j-pihet at ti.com>
> ---
>  arch/arm/mach-omap2/control.c   |    9 +-
>  arch/arm/mach-omap2/sleep34xx.S |  313 +++++++++++++++++++++++----------------
>  2 files changed, 191 insertions(+), 131 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
> index 728f268..f4b19ed 100644
> --- a/arch/arm/mach-omap2/control.c
> +++ b/arch/arm/mach-omap2/control.c
> @@ -239,7 +239,14 @@ void omap3_save_scratchpad_contents(void)
>  	struct omap3_scratchpad_prcm_block prcm_block_contents;
>  	struct omap3_scratchpad_sdrc_block sdrc_block_contents;
>  
> -	/* Populate the Scratchpad contents */
> +	/*
> +	 * Populate the Scratchpad contents
> +	 *
> +	 * The "get_*restore_pointer" functions are used to provide a
> +	 * physical restore address where the ROM code jumps while waking
> +	 * up from MPU OFF/OSWR state.
Just curious: we dont have OSWR(open switch retention) in l-o unless I 
am mistaken.. we have cswr (closed switch retention).

> +	 * The restore pointer is stored into the scratchpad.
> +	 */
>  	scratchpad_contents.boot_config_ptr = 0x0;
>  	if (cpu_is_omap3630())
>  		scratchpad_contents.public_restore_ptr =
> diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
> index beeb682..12061fd 100644
> --- a/arch/arm/mach-omap2/sleep34xx.S
> +++ b/arch/arm/mach-omap2/sleep34xx.S
> @@ -71,6 +71,13 @@
>   * API functions
>   */
>  
> +/*
> + * The "get_*restore_pointer" functions are used to provide a
> + * physical restore address where the ROM code jumps while waking
> + * up from MPU OFF/OSWR state.
> + * The restore pointer is stored into the scratchpad.
> + */
> +
we need to cleanup comments such as those used below:

>  	.text
>  /* Function call to get the restore pointer for resume from OFF */
^^^^
may be make it explicit what the difference for get_restore_pointer is etc..

>  ENTRY(get_restore_pointer)
> @@ -102,7 +109,7 @@ ENTRY(get_es3_restore_pointer_sz)
>  /*
>   * L2 cache needs to be toggled for stable OFF mode functionality on 3630.
>   * This function sets up a flag that will allow for this toggling to take
> - * place on 3630. Hopefully some version in the future maynot need this
> + * place on 3630. Hopefully some version in the future may not need this.
I think we should split the patch up for the comment cleanup -> it is a 
little nuisance trying to review the actual functional change mixed with 
comment cleanups together..

>   */
>  ENTRY(enable_omap3630_toggle_l2_on_restore)
>          stmfd   sp!, {lr}     @ save registers on stack
> @@ -144,34 +151,158 @@ ENTRY(save_secure_ram_context_sz)
>  	.word	. - save_secure_ram_context
>  
>  /*
> + * ======================
> + * == Idle entry point ==
> + * ======================
> + */
> +
> +/*
>   * Forces OMAP into idle state
>   *
> - * omap34xx_suspend() - This bit of code just executes the WFI
> - * for normal idles.
> + * omap34xx_suspend() - This bit of code saves the CPU context if needed
> + * and executes the WFI instruction
since we are cleaning up, I might as well suggest - what condition do we 
save the CPU context might be worth mentioning. is this called during 
RET transition or OFF transition is not clear..
>   *
> - * 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.
> + * Notes:
> + * - this code gets copied to internal SRAM at boot.
> + * - when the OMAP wakes up it continues at different execution points
> + *   depending on the low power mode (non-OFF vs OFF modes),
> + *   cf. 'Resume path for xxx mode' comments.
Further, might as well help people and say that this is the call 
_omap_sram_idle actually takes ;)

Also might be a little indepth pseudo code overview of this complex code 
flow might help for code maintainers later on..

omap34xx_cpu_suspend
    |- if Context save not required -> omap3_do_wfi -> wfi
    \- if context save required: -> save_context_wfi
                                       |
				cache maintenance:
cache maintenance:
	- what happens when l1 is lost, what happens when l2 is lost..

>   */
>  ENTRY(omap34xx_cpu_suspend)
>  	stmfd	sp!, {r0-r12, lr}		@ save registers on stack
>  
> -	/* r0 contains restore pointer in sdram */
> -	/* r1 contains information about saving context */
> -	ldr     r4, sdrc_power          @ read the SDRC_POWER register
> -	ldr     r5, [r4]                @ read the contents of SDRC_POWER
> -	orr     r5, r5, #0x40           @ enable self refresh on idle req
> -	str     r5, [r4]                @ write back to SDRC_POWER register
> +	/*
> +	 * r0 contains restore pointer in sdram
> +	 * r1 contains information about saving context:
> +	 *   0 - No context lost
> +	 *   1 - Only L1 and logic lost
> +	 *   2 - Only L2 lost
> +	 *   3 - Both L1 and L2 lost
Since we are cleaning up, one generic Suggestion:
since these 0 1 2 3 are used by pm34xx.c and this code alone, we might 
as well define it in a common header and save ourselves the pain of 
defining this in 2 places and some one screwing things up later on..

looking at pm34xx.c, I dont see 2 being defined at all :( unless I 
missed something, pm34xx.c:
  switch (mpu_next_state) {
  case PWRDM_POWER_ON:
  case PWRDM_POWER_RET:
          /* No need to save context */
Which is kind of funny for me. since we use this parameter to determine 
to clean cache or not :D - another misinterpretation of using values 
instead of readable macros ;)
          save_state = 0;
          break;
  case PWRDM_POWER_OFF:
          save_state = 3;
          break;

> +	 */
>  
> +	/* Save context only if required */
>  	cmp	r1, #0x0
> -	/* If context save is required, do that and execute wfi */
> -	bne	save_context_wfi
> +	beq	omap3_do_wfi
> +
Add comment to fall through here.

> +save_context_wfi:
> +	mov	r8, r0			@ Store SDRAM address in r8
> +	mrc	p15, 0, r5, c1, c0, 1	@ Read Auxiliary Control Register
> +	mov	r4, #0x1		@ Number of parameters for restore call
> +	stmia	r8!, {r4-r5}		@ Push parameters for restore call
> +	mrc	p15, 1, r5, c9, c0, 2	@ Read L2 AUX ctrl register
> +	stmia	r8!, {r4-r5}		@ Push parameters for restore call
> +
> +        /* Check what that target sleep state is from r1 */
> +	cmp	r1, #0x2		@ Only L2 lost, no need to save context
> +	beq	clean_caches

one suggestion here:
how about:
	and rn, r1, #L2_LOGIC_LOST
	cmp rn, #L2_LOGIC_LOST
	bleq l2_cleanup
	and rn, r1, #L1_LOGIC_LOST
	cmp rn, #L1_LOGIC_LOST
	bleq l1_cleanup
	move the omap3_do_wfi code here

l1_cleanup:
	...
	save_context
	...
	return back

l2_cleanup:
	...
	...
	return back

or am I simplifying things too much here?

> +
> +l1_logic_lost:
> +	/* Store sp and spsr to SDRAM */
> +	mov	r4, sp
> +	mrs	r5, spsr
> +	mov	r6, lr
> +	stmia	r8!, {r4-r6}
> +	/* Save all ARM registers */
> +	/* Coprocessor access control register */
> +	mrc	p15, 0, r6, c1, c0, 2
> +	stmia	r8!, {r6}
> +	/* TTBR0, TTBR1 and Translation table base control */
> +	mrc	p15, 0, r4, c2, c0, 0
> +	mrc	p15, 0, r5, c2, c0, 1
> +	mrc	p15, 0, r6, c2, c0, 2
> +	stmia	r8!, {r4-r6}
> +	/*
> +	 * Domain access control register, data fault status register,
> +	 * and instruction fault status register
> +	 */
> +	mrc	p15, 0, r4, c3, c0, 0
> +	mrc	p15, 0, r5, c5, c0, 0
> +	mrc	p15, 0, r6, c5, c0, 1
> +	stmia	r8!, {r4-r6}
> +	/*
> +	 * Data aux fault status register, instruction aux fault status,
> +	 * data fault address register and instruction fault address register
> +	 */
> +	mrc	p15, 0, r4, c5, c1, 0
> +	mrc	p15, 0, r5, c5, c1, 1
> +	mrc	p15, 0, r6, c6, c0, 0
> +	mrc	p15, 0, r7, c6, c0, 2
> +	stmia	r8!, {r4-r7}
> +	/*
> +	 * user r/w thread and process ID, user r/o thread and process ID,
> +	 * priv only thread and process ID, cache size selection
> +	 */
> +	mrc	p15, 0, r4, c13, c0, 2
> +	mrc	p15, 0, r5, c13, c0, 3
> +	mrc	p15, 0, r6, c13, c0, 4
> +	mrc	p15, 2, r7, c0, c0, 0
> +	stmia	r8!, {r4-r7}
> +	/* Data TLB lockdown, instruction TLB lockdown registers */
> +	mrc	p15, 0, r5, c10, c0, 0
> +	mrc	p15, 0, r6, c10, c0, 1
> +	stmia	r8!, {r5-r6}
> +	/* Secure or non secure vector base address, FCSE PID, Context PID*/
> +	mrc	p15, 0, r4, c12, c0, 0
> +	mrc	p15, 0, r5, c13, c0, 0
> +	mrc	p15, 0, r6, c13, c0, 1
> +	stmia	r8!, {r4-r6}
> +	/* Primary remap, normal remap registers */
> +	mrc	p15, 0, r4, c10, c2, 0
> +	mrc	p15, 0, r5, c10, c2, 1
> +	stmia	r8!,{r4-r5}
> +
> +	/* Store current cpsr*/
> +	mrs	r2, cpsr
> +	stmia	r8!, {r2}
> +
> +	mrc	p15, 0, r4, c1, c0, 0
> +	/* save control register */
> +	stmia	r8!, {r4}
> +
> +clean_caches:
> +	/*
> +	 * Clean Data or unified cache to POU
> +	 * How to invalidate only L1 cache???? - #FIX_ME#
> +	 * mcr	p15, 0, r11, c7, c11, 1
> +	 */
> +	cmp	r1, #0x1 		@ Check whether L2 inval is required
> +	beq	omap3_do_wfi
> +
> +clean_l2:
> +	/*
> +	 * jump out to kernel flush routine
> +	 *  - reuse that code is better
> +	 *  - it executes in a cached space so is faster than refetch per-block
> +	 *  - should be faster and will change with kernel
> +	 *  - 'might' have to copy address, load and jump to it
> +	 */
> +	ldr r1, kernel_flush
> +	mov lr, pc
> +	bx  r1
> +
> +omap3_do_wfi:
recommend changing the name -> basically the flow is split into two:
wfi with save of context: save_context_wfi
wfi without save of context: omap3_do_wfi (I suppose the naming is 
because it is being used in multiple paths)



> +	ldr	r4, sdrc_power		@ read the SDRC_POWER register
> +	ldr	r5, [r4]		@ read the contents of SDRC_POWER
> +	orr	r5, r5, #0x40		@ enable self refresh on idle req
> +	str	r5, [r4]		@ write back to SDRC_POWER register
> +
we just changed the functional sequence of when we enable self refresh 
on idle req (previously - this was the first step on entry, now, we do 
this after context save) -> this should be a separate patch of it's own 
for tracking any issues that may be detected by this change i suppose.

>  	/* Data memory barrier and Data sync barrier */
>  	mov	r1, #0
>  	mcr	p15, 0, r1, c7, c10, 4
>  	mcr	p15, 0, r1, c7, c10, 5
>  
> +/*
> + * ===================================
> + * == WFI instruction => Enter idle ==
idle is a bit confusing -> cpu idle can hit both RET and OFF..

> + * ===================================
> + */
>  	wfi				@ wait for interrupt
>  
> +/*
> + * ===================================
> + * == Resume path for non-OFF modes ==
> + * ===================================
> + */
>  	nop
>  	nop
>  	nop
> @@ -184,7 +315,29 @@ ENTRY(omap34xx_cpu_suspend)
>  	nop
>  	bl wait_sdrc_ok
>  
> -	ldmfd	sp!, {r0-r12, pc}		@ restore regs and return
> +/*
> + * ===================================
> + * == Exit point from non-OFF modes ==
> + * ===================================
> + */
> +	ldmfd	sp!, {r0-r12, pc}	@ restore regs and return
cosmetic changes like this belong it's own patch to help the reviewer.

> +
> +
additional EOL?
> +/*
> + * ==============================
> + * == Resume path for OFF mode ==
> + * ==============================
> + */
> +
> +/*
> + * The restore_* functions are called by the ROM code
> + *  when back from WFI in OFF mode.
> + * Cf. the get_*restore_pointer functions.
> + *
> + *  restore_es3: applies to 34xx >= ES3.0
> + *  restore_3630: applies to 36xx
> + *  restore: common code for 3xxx
> + */
>  restore_es3:
>  	ldr	r5, pm_prepwstst_core_p
>  	ldr	r4, [r5]
> @@ -214,12 +367,17 @@ restore_3630:
>  	ldr	r1, control_mem_rta
>  	mov	r2, #OMAP36XX_RTA_DISABLE
>  	str	r2, [r1]
> -	/* Fall thru for the remaining logic */
> +
> +	/* Fall thru to common code for the remaining logic */
might as well replace thru with through ;)

> +
>  restore:
> -        /* Check what was the reason for mpu reset and store the reason in r9*/
> -        /* 1 - Only L1 and logic lost */
> -        /* 2 - Only L2 lost - In this case, we wont be here */
> -        /* 3 - Both L1 and L2 lost */
> +        /*
> +	 * Check what was the reason for mpu reset and store the reason in r9:
> +	 *  0 - No context lost
> +         *  1 - Only L1 and logic lost
> +         *  2 - Only L2 lost - In this case, we wont be here
> +         *  3 - Both L1 and L2 lost
> +	 */
again something we can skip if we were to use #defines.

>  	ldr     r1, pm_pwstctrl_mpu
>  	ldr	r2, [r1]
>  	and     r2, r2, #0x3
> @@ -422,118 +580,12 @@ usettbr0:
>  	and	r4, r2
>  	mcr	p15, 0, r4, c1, c0, 0
>  
> +/*
> + * ==============================
> + * == Exit point from OFF mode ==
> + * ==============================
> + */
>  	ldmfd	sp!, {r0-r12, pc}		@ restore regs and return
> -save_context_wfi:
> -	mov	r8, r0 /* Store SDRAM address in r8 */
> -	mrc	p15, 0, r5, c1, c0, 1	@ Read Auxiliary Control Register
> -	mov	r4, #0x1		@ Number of parameters for restore call
> -	stmia	r8!, {r4-r5}		@ Push parameters for restore call
> -	mrc	p15, 1, r5, c9, c0, 2	@ Read L2 AUX ctrl register
> -	stmia	r8!, {r4-r5}		@ Push parameters for restore call
> -        /* Check what that target sleep state is:stored in r1*/
> -        /* 1 - Only L1 and logic lost */
> -        /* 2 - Only L2 lost */
> -        /* 3 - Both L1 and L2 lost */
> -	cmp	r1, #0x2 /* Only L2 lost */
> -	beq	clean_l2
> -	cmp	r1, #0x1 /* L2 retained */
> -	/* r9 stores whether to clean L2 or not*/
> -	moveq	r9, #0x0 /* Dont Clean L2 */
> -	movne	r9, #0x1 /* Clean L2 */
> -l1_logic_lost:
> -	/* Store sp and spsr to SDRAM */
> -	mov	r4, sp
> -	mrs	r5, spsr
> -	mov	r6, lr
> -	stmia	r8!, {r4-r6}
> -	/* Save all ARM registers */
> -	/* Coprocessor access control register */
> -	mrc	p15, 0, r6, c1, c0, 2
> -	stmia	r8!, {r6}
> -	/* TTBR0, TTBR1 and Translation table base control */
> -	mrc	p15, 0, r4, c2, c0, 0
> -	mrc	p15, 0, r5, c2, c0, 1
> -	mrc	p15, 0, r6, c2, c0, 2
> -	stmia	r8!, {r4-r6}
> -	/* Domain access control register, data fault status register,
> -	and instruction fault status register */
> -	mrc	p15, 0, r4, c3, c0, 0
> -	mrc	p15, 0, r5, c5, c0, 0
> -	mrc	p15, 0, r6, c5, c0, 1
> -	stmia	r8!, {r4-r6}
> -	/* Data aux fault status register, instruction aux fault status,
> -	datat fault address register and instruction fault address register*/
> -	mrc	p15, 0, r4, c5, c1, 0
> -	mrc	p15, 0, r5, c5, c1, 1
> -	mrc	p15, 0, r6, c6, c0, 0
> -	mrc	p15, 0, r7, c6, c0, 2
> -	stmia	r8!, {r4-r7}
> -	/* user r/w thread and process ID, user r/o thread and process ID,
> -	priv only thread and process ID, cache size selection */
> -	mrc	p15, 0, r4, c13, c0, 2
> -	mrc	p15, 0, r5, c13, c0, 3
> -	mrc	p15, 0, r6, c13, c0, 4
> -	mrc	p15, 2, r7, c0, c0, 0
> -	stmia	r8!, {r4-r7}
> -	/* Data TLB lockdown, instruction TLB lockdown registers */
> -	mrc	p15, 0, r5, c10, c0, 0
> -	mrc	p15, 0, r6, c10, c0, 1
> -	stmia	r8!, {r5-r6}
> -	/* Secure or non secure vector base address, FCSE PID, Context PID*/
> -	mrc	p15, 0, r4, c12, c0, 0
> -	mrc	p15, 0, r5, c13, c0, 0
> -	mrc	p15, 0, r6, c13, c0, 1
> -	stmia	r8!, {r4-r6}
> -	/* Primary remap, normal remap registers */
> -	mrc	p15, 0, r4, c10, c2, 0
> -	mrc	p15, 0, r5, c10, c2, 1
> -	stmia	r8!,{r4-r5}
> -
> -	/* Store current cpsr*/
> -	mrs	r2, cpsr
> -	stmia	r8!, {r2}
> -
> -	mrc	p15, 0, r4, c1, c0, 0
> -	/* save control register */
> -	stmia	r8!, {r4}
> -clean_caches:
> -	/* Clean Data or unified cache to POU*/
> -	/* How to invalidate only L1 cache???? - #FIX_ME# */
> -	/* mcr	p15, 0, r11, c7, c11, 1 */
> -	cmp	r9, #1 /* Check whether L2 inval is required or not*/
> -	bne	skip_l2_inval
> -clean_l2:
> -	/*
> -	 * Jump out to kernel flush routine
> -	 *  - reuse that code is better
> -	 *  - it executes in a cached space so is faster than refetch per-block
> -	 *  - should be faster and will change with kernel
> -	 *  - 'might' have to copy address, load and jump to it
> -	 */
> -	ldr r1, kernel_flush
> -	mov lr, pc
> -	bx  r1
> -
> -skip_l2_inval:
> -	/* Data memory barrier and Data sync barrier */
> -	mov     r1, #0
> -	mcr     p15, 0, r1, c7, c10, 4
> -	mcr     p15, 0, r1, c7, c10, 5
> -
> -	wfi                             @ wait for interrupt
> -	nop
> -	nop
> -	nop
> -	nop
> -	nop
> -	nop
> -	nop
> -	nop
> -	nop
> -	nop
> -	bl wait_sdrc_ok
> -	/* restore regs and return */
> -	ldmfd   sp!, {r0-r12, pc}
>  
>  
>  /*
> @@ -683,5 +735,6 @@ kick_counter:
>  	.word	0
>  wait_dll_lock_counter:
>  	.word	0
> +
spurious change.

>  ENTRY(omap34xx_cpu_suspend_sz)
>  	.word	. - omap34xx_cpu_suspend

as such, this patch:

Tested-by: Nishanth Menon <nm at ti.com>
Tested on:
SDP3630
SDP3430
Test script:
http://pastebin.mozilla.org/889933


-- 
Regards,
Nishanth Menon



More information about the linux-arm-kernel mailing list