Patchwork [RFC] ARM hibernation / suspend-to-disk support code

login
register
mail settings
Submitter Frank Hofmann
Date May 20, 2011, 12:39 p.m.
Message ID <alpine.DEB.2.00.1105201255260.8018@localhost6.localdomain6>
Download mbox | patch
Permalink /patch/96598/
State New
Headers show

Comments

Frank Hofmann - May 20, 2011, 12:39 p.m.
On Fri, 20 May 2011, Dave Martin wrote:

[ ... ]
>> +/*
>> + * Save the current CPU state before suspend / poweroff.
>> + */
>> +ENTRY(swsusp_arch_suspend)
>> +	ldr	r0, =__swsusp_arch_ctx
>> +	mrs	r1, cpsr
>> +	str	r1, [r0], #4		/* CPSR */
>> +ARM(	msr	cpsr_c, #SYSTEM_MODE					)
>> +THUMB(	mov	r2, #SYSTEM_MODE					)
>> +THUMB(	msr	cpsr_c, r2						)
>
> For Thumb-2 kernels, you can use the cps instruction to change the CPU
> mode:
> 	cps	#SYSTEM_MODE
>
> For ARM though, this instruction is only supported for ARMv6 and above,
> so it's best to keep the msr form for compatibility for that case.

Ah, ok, no problem will make that change, looks good.

Do all ARMs have cpsr / spsr as used here ? Or is that code restricted to 
ARMv5+ ? I don't have the CPU evolution history there, only got involved 
with ARM when armv6 already was out.

I've simply done there what the "setmode" macro from <asm/assembler.h> 
is doing, have chosen not to include that file because it overrides "push" 
on a global scale for no good reason and that sort of thing makes me 
cringe.


>
>> +	stm	r0!, {r4-r12,lr}	/* nonvolatile regs */
>
> Since r12 is allowed to be corrupted across a function call, we
> probably don't need to save it.

ah ok thx for clarification. Earlier iterations of the patch just saved 
r0-r14 there, "just to have it all", doing it right is best as always.

>
[ ... ]
>> +	bl	__save_processor_state
>
> <aside>
>
> Structurally, we seem to have:
>
> swsusp_arch_suspend {
> 	/* save some processor state */
> 	__save_processor_state();
>
> 	swsusp_save();
> }
>
> Is __save_processor_state() intended to encapsulate all the CPU-model-
> specific state saving?  Excuse my ignorance of previous conversations...
>
> </aside>

You're right.

I've attached the codechanges which implement __save/__restore... for 
TI OMAP3 and Samsung S5P64xx, to illustrate, again (that's the stuff 
referred to in the earlier mail I mentioned in first post; beware of code 
churn in there, those diffs don't readily apply to 'just any' kernel).

These hooks are essentially the same as the machine-specific cpu_suspend() 
except that we substitute "r0" (the save context after the generic part) 
as target for where-to-save-the-state, and we make the funcs return 
instead of switching to OFF mode.

That's what I meant with "backdoorish". A better way would be to change 
the cpu_suspend interface so that it returns instead of directly switching 
to off mode / powering down.

Russell has lately been making changes in this area; the current kernels 
are a bit different here since the caller already supplies the 
state-save-buffer, while the older ones used to choose in some 
mach-specific way where to store that state.

(one of the reasons why these mach-dependent enablers aren't part of this 
patch suggestion ...)


>
>> +	pop	{lr}
>> +	b	swsusp_save
>> +ENDPROC(swsusp_arch_suspend)
>
> I'm not too familiar with the suspend/resume stuff, so I may be asking
> a dumb question here, but:
>
> Where do we save/restore r8_FIQ..r13_FIQ, r13_IRQ, r13_UND and r13_ABT?
> (I'm assuming there's no reason to save/restore r14 or SPSR for any
> exception mode, since we presumably aren't going to suspend/resume
> from inside an exception handler (?))
>
> The exception stack pointers might conceivably be reinitialised to
> sane values on resume, without necessarily needing to save/restore
> them, providing my assumption in the previous paragraph is correct.
>
> r8_FIQ..r12_FIQ can store arbitrary state used by the FIQ handler,
> if FIQ is in use.  Can we expect any driver using FIQ to save/restore
> this state itself, rather than doing it globally?  This may be
> reasonable.

We don't need to save/restore those, because at the time the snapshot is 
taken interrupts are off and we cannot be in any trap handler either. On 
resume, the kernel that has been loaded has initialized them properly 
already.

If there's really any driver out there which uses FIQ in such an exclusive 
manner that it expects FIQ register bank contents to remain the same 
across multiple FIQ events then it's rather that driver's responsibility 
to perform the save/restore via suspend/resume callbacks.

See also Russell's mails about that, for previous attempts a year and half 
a year ago to get "something for ARM hibernation" in:

https://lists.linux-foundation.org/pipermail/linux-pm/2010-July/027571.html
https://lists.linux-foundation.org/pipermail/linux-pm/2010-December/029665.html

The kernel doesn't do IRQ/FIQ/ABT/UND save / restore for suspend-to-ram 
either. CPU hotplug support (re)initializes those. I believe we're fine.


>
> Cheers
> ---Dave
>
>

Thanks,
FrankH.
Dave Martin - May 20, 2011, 3:03 p.m.
On Fri, May 20, 2011 at 01:39:37PM +0100, Frank Hofmann wrote:
> On Fri, 20 May 2011, Dave Martin wrote:
> 
> [ ... ]
> >>+/*
> >>+ * Save the current CPU state before suspend / poweroff.
> >>+ */
> >>+ENTRY(swsusp_arch_suspend)
> >>+	ldr	r0, =__swsusp_arch_ctx
> >>+	mrs	r1, cpsr
> >>+	str	r1, [r0], #4		/* CPSR */
> >>+ARM(	msr	cpsr_c, #SYSTEM_MODE					)
> >>+THUMB(	mov	r2, #SYSTEM_MODE					)
> >>+THUMB(	msr	cpsr_c, r2						)
> >
> >For Thumb-2 kernels, you can use the cps instruction to change the CPU
> >mode:
> >	cps	#SYSTEM_MODE
> >
> >For ARM though, this instruction is only supported for ARMv6 and above,
> >so it's best to keep the msr form for compatibility for that case.
> 
> Ah, ok, no problem will make that change, looks good.
> 
> Do all ARMs have cpsr / spsr as used here ? Or is that code
> restricted to ARMv5+ ? I don't have the CPU evolution history there,
> only got involved with ARM when armv6 already was out.

CPSR/SPSR exist from ARMv3 onwards.  I think for linux we can just assume
that they are there (unless someone knows better...)

> 
> I've simply done there what the "setmode" macro from
> <asm/assembler.h> is doing, have chosen not to include that file
> because it overrides "push" on a global scale for no good reason and
> that sort of thing makes me cringe.

Actually, I guess you should be using that header, from a general
standardisation point of view.  In particular, it would be nicer to
use setmode than to copy it.

The setmode macro itself could be improved to use cps for Thumb-2,
but that would be a separate (and low-priority) patch.


Re the push/pop macros, I'm not sure of the history for those.

In the kernel, it seems customary to write stmfd sp!, / ldmfd sp!,
instead of push / pop, so you could always use those mnemonics instead.
They're equivalent.

> 
> 
> >
> >>+	stm	r0!, {r4-r12,lr}	/* nonvolatile regs */
> >
> >Since r12 is allowed to be corrupted across a function call, we
> >probably don't need to save it.
> 
> ah ok thx for clarification. Earlier iterations of the patch just
> saved r0-r14 there, "just to have it all", doing it right is best as
> always.
> 
> >
> [ ... ]
> >>+	bl	__save_processor_state
> >
> ><aside>
> >
> >Structurally, we seem to have:
> >
> >swsusp_arch_suspend {
> >	/* save some processor state */
> >	__save_processor_state();
> >
> >	swsusp_save();
> >}
> >
> >Is __save_processor_state() intended to encapsulate all the CPU-model-
> >specific state saving?  Excuse my ignorance of previous conversations...
> >
> ></aside>
> 
> You're right.
> 
> I've attached the codechanges which implement __save/__restore...
> for TI OMAP3 and Samsung S5P64xx, to illustrate, again (that's the
> stuff referred to in the earlier mail I mentioned in first post;
> beware of code churn in there, those diffs don't readily apply to
> 'just any' kernel).
> 
> These hooks are essentially the same as the machine-specific
> cpu_suspend() except that we substitute "r0" (the save context after
> the generic part) as target for where-to-save-the-state, and we make
> the funcs return instead of switching to OFF mode.
> 
> That's what I meant with "backdoorish". A better way would be to
> change the cpu_suspend interface so that it returns instead of
> directly switching to off mode / powering down.
> 
> Russell has lately been making changes in this area; the current
> kernels are a bit different here since the caller already supplies
> the state-save-buffer, while the older ones used to choose in some
> mach-specific way where to store that state.
> 
> (one of the reasons why these mach-dependent enablers aren't part of
> this patch suggestion ...)
> 
> 
> >
> >>+	pop	{lr}
> >>+	b	swsusp_save
> >>+ENDPROC(swsusp_arch_suspend)
> >
> >I'm not too familiar with the suspend/resume stuff, so I may be asking
> >a dumb question here, but:
> >
> >Where do we save/restore r8_FIQ..r13_FIQ, r13_IRQ, r13_UND and r13_ABT?
> >(I'm assuming there's no reason to save/restore r14 or SPSR for any
> >exception mode, since we presumably aren't going to suspend/resume
> >from inside an exception handler (?))
> >
> >The exception stack pointers might conceivably be reinitialised to
> >sane values on resume, without necessarily needing to save/restore
> >them, providing my assumption in the previous paragraph is correct.
> >
> >r8_FIQ..r12_FIQ can store arbitrary state used by the FIQ handler,
> >if FIQ is in use.  Can we expect any driver using FIQ to save/restore
> >this state itself, rather than doing it globally?  This may be
> >reasonable.
> 
> We don't need to save/restore those, because at the time the
> snapshot is taken interrupts are off and we cannot be in any trap
> handler either. On resume, the kernel that has been loaded has
> initialized them properly already.
> 
> If there's really any driver out there which uses FIQ in such an
> exclusive manner that it expects FIQ register bank contents to

This is exactly how FIQ may be used: by preloading the data for the next
I/O operation in the FIQ banked registers, you can issue the next
operation to the peripheral on the first instruction after taking the
interrupt, without having any delay for accessing memory or executing
other instructions, so:

my_fiq_handler:
	str	r8, [r9]
	/* ... */
	/* set up next transaction in r8,r9 */
	/* return */

This can make a significant difference to throughput when streaming
data to certain types of serial peripheral.

It's mostly of historical interest though, since that advantage
is likely to be swamped by cache effects on modern platforms, plus
it's hard to use FIQ to service more than one device without losing
the advantages.

> remain the same across multiple FIQ events then it's rather that
> driver's responsibility to perform the save/restore via
> suspend/resume callbacks.

I think I agree.  Few drivers use FIQ, and if there are drivers
which use FIQ but don't implement suspend/resume hooks, that sounds
like a driver bug.

Assuming nobody disagrees with that conclusion, it would be a good
idea to stick a comment somewhere explaining that policy.

> 
> See also Russell's mails about that, for previous attempts a year
> and half a year ago to get "something for ARM hibernation" in:
> 
> https://lists.linux-foundation.org/pipermail/linux-pm/2010-July/027571.html
> https://lists.linux-foundation.org/pipermail/linux-pm/2010-December/029665.html

Relying on drivers to save/restore the FIQ state if necessary seems
to correspond to what Russell is saying in his second mail.

> 
> The kernel doesn't do IRQ/FIQ/ABT/UND save / restore for
> suspend-to-ram either. CPU hotplug support (re)initializes those. I
> believe we're fine.

OK, just wanted to make sure I understood that right.

I'll leave it to others to comment on the actual SoC-specific routines,
but thanks for the illustration.

Cheers
---Dave

> 
> 
> >
> >Cheers
> >---Dave
> >
> >
> 
> Thanks,
> FrankH.

> diff --git a/arch/arm/plat-s5p/sleep.S b/arch/arm/plat-s5p/sleep.S
> index 2cdae4a..dd9516b 100644
> --- a/arch/arm/plat-s5p/sleep.S
> +++ b/arch/arm/plat-s5p/sleep.S
> @@ -44,14 +44,32 @@
>  */
>  	.text
>  
> +#ifdef CONFIG_HIBERNATION
> +ENTRY(__save_processor_state)
> +	mov	r1, #0
> +	b	.Ls3c_cpu_save_internal
> +ENDPROC(__save_processor_state)
> +
> +ENTRY(__restore_processor_state)
> +	stmfd	sp!, { r3 - r12, lr }
> +	ldr	r2, =.Ls3c_cpu_resume_internal
> +	mov	r1, #1
> +	str	sp, [r0, #40]		@ fixup sp in restore context
> +	mov	pc, r2
> +ENDPROC(__restore_processor_state)
> +#endif
> +
>  	/* s3c_cpu_save
>  	 *
>  	 * entry:
>  	 *	r0 = save address (virtual addr of s3c_sleep_save_phys)
> -	*/
> +	 *	r1 (_internal_ only) = CPU sleep trampoline (if any)
> +	 */
>  
>  ENTRY(s3c_cpu_save)
>  
> +	ldr	r1, =pm_cpu_sleep	@ set trampoline
> +.Ls3c_cpu_save_internal:
>  	stmfd	sp!, { r3 - r12, lr }
>  
>  	mrc	p15, 0, r4, c13, c0, 0	@ FCSE/PID
> @@ -67,11 +85,15 @@ ENTRY(s3c_cpu_save)
>  
>  	stmia	r0, { r3 - r13 }
>  
> +	mov	r4, r1
>  	@@ write our state back to RAM
>  	bl	s3c_pm_cb_flushcache
>  
> +	movs	r0, r4
> +#ifdef CONFIG_HIBERNATION
> +	ldmeqfd	sp!, { r3 - r12, pc }	@ if there was no trampoline, return
> +#endif
>  	@@ jump to final code to send system to sleep
> -	ldr	r0, =pm_cpu_sleep
>  	@@ldr	pc, [ r0 ]
>  	ldr	r0, [ r0 ]
>  	mov	pc, r0
> @@ -86,6 +108,7 @@ resume_with_mmu:
>  	str	r12, [r4]
>  
>  	ldmfd	sp!, { r3 - r12, pc }
> +ENDPROC(s3c_cpu_save)
>  
>  	.ltorg
>  
> @@ -131,6 +154,7 @@ ENTRY(s3c_cpu_resume)
>  	mcr	p15, 0, r1, c7, c5, 0		@@ invalidate I Cache
>  
>  	ldr	r0, s3c_sleep_save_phys	@ address of restore block
> +.Ls3c_cpu_resume_internal:
>  	ldmia	r0, { r3 - r13 }
>  
>  	mcr	p15, 0, r4, c13, c0, 0	@ FCSE/PID
> @@ -152,6 +176,11 @@ ENTRY(s3c_cpu_resume)
>  	mcr	p15, 0, r12, c10, c2, 0	@ write PRRR
>  	mcr	p15, 0, r3, c10, c2, 1	@ write NMRR
>  
> +#ifdef CONFIG_HIBERNATION
> +	cmp	r1, #0
> +	bne	0f			@ only do MMU phys init
> +					@ not called by __restore_processor_state
> +#endif
>  	/* calculate first section address into r8 */
>  	mov	r4, r6
>  	ldr	r5, =0x3fff
> @@ -175,6 +204,7 @@ ENTRY(s3c_cpu_resume)
>  	str	r10, [r4]
>  
>  	ldr	r2, =resume_with_mmu
> +0:
>  	mcr	p15, 0, r9, c1, c0, 0		@ turn on MMU, etc
>  
>          nop
> @@ -183,6 +213,9 @@ ENTRY(s3c_cpu_resume)
>          nop
>          nop					@ second-to-last before mmu
>  
> +#ifdef CONFIG_HIBERNATION
> +	ldmnefd	sp!, { r3 - r12, pc }
> +#endif
>  	mov	pc, r2				@ go back to virtual address
>  
>  	.ltorg

> diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
> index ea4e498..19ecd8e 100644
> --- a/arch/arm/mach-omap2/sleep34xx.S
> +++ b/arch/arm/mach-omap2/sleep34xx.S
> @@ -358,6 +358,7 @@ logic_l1_restore:
>  	ldr	r4, scratchpad_base
>  	ldr	r3, [r4,#0xBC]
>  	adds	r3, r3, #16
> +.Llogic_l1_restore_internal:
>  	ldmia	r3!, {r4-r6}
>  	mov	sp, r4
>  	msr	spsr_cxsf, r5
> @@ -433,6 +434,10 @@ ttbr_error:
>  	*/
>  	b	ttbr_error
>  usettbr0:
> +#ifdef CONFIG_HIBERNATION
> +	cmp	r1, #1
> +	ldmeqfd	sp!, { r0 - r12, pc }	@ early return from __restore_processor_state
> +#endif
>  	mrc	p15, 0, r2, c2, c0, 0
>  	ldr	r5, ttbrbit_mask
>  	and	r2, r5
> @@ -471,6 +476,25 @@ usettbr0:
>  	mcr	p15, 0, r4, c1, c0, 0
>  
>  	ldmfd	sp!, {r0-r12, pc}		@ restore regs and return
> +
> +#ifdef CONFIG_HIBERNATION
> +ENTRY(__save_processor_state)
> +	stmfd	sp!, {r0-r12, lr}
> +	mov	r1, #0x4
> +	mov	r8, r0
> +	b	l1_logic_lost
> +ENDPROC(__save_processor_state)
> +
> +ENTRY(__restore_processor_state)
> +	stmfd	sp!, { r0 - r12, lr }
> +	str	sp, [r0]		@ fixup saved stack pointer
> +	str	lr, [r0, #8]		@ fixup saved link register
> +	mov	r3, r0
> +	mov	r1, #1
> +	b	.Llogic_l1_restore_internal
> +ENDPROC(__restore_processor_state)
> +#endif
> +
>  save_context_wfi:
>  	/*b	save_context_wfi*/	@ enable to debug save code
>  	mov	r8, r0 /* Store SDRAM address in r8 */
> @@ -545,6 +569,10 @@ l1_logic_lost:
>  	mrc	p15, 0, r4, c1, c0, 0
>  	/* save control register */
>  	stmia	r8!, {r4}
> +#ifdef CONFIG_HIBERNATION
> +	cmp	r1, #4
> +	ldmeqfd	sp!, {r0-r12, pc}	@ early return from __save_processor_state
> +#endif
>  clean_caches:
>  	/* Clean Data or unified cache to POU*/
>  	/* How to invalidate only L1 cache???? - #FIX_ME# */
> diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
> index df5ce56..b4713ba 100644
> --- a/arch/arm/plat-omap/Kconfig
> +++ b/arch/arm/plat-omap/Kconfig
> @@ -23,6 +23,7 @@ config ARCH_OMAP3
>  	select CPU_V7
>  	select COMMON_CLKDEV
>  	select OMAP_IOMMU
> +	select ARCH_HIBERNATION_POSSIBLE
>  
>  config ARCH_OMAP4
>  	bool "TI OMAP4"
Frank Hofmann - May 20, 2011, 4:24 p.m.
On Fri, 20 May 2011, Dave Martin wrote:

[ ... ]
>> I've simply done there what the "setmode" macro from
>> <asm/assembler.h> is doing, have chosen not to include that file
>> because it overrides "push" on a global scale for no good reason and
>> that sort of thing makes me cringe.
>
> Actually, I guess you should be using that header, from a general
> standardisation point of view.  In particular, it would be nicer to
> use setmode than to copy it.
>
> The setmode macro itself could be improved to use cps for Thumb-2,
> but that would be a separate (and low-priority) patch.
>
>
> Re the push/pop macros, I'm not sure of the history for those.

arch/arm/lib are the only users.

>
> In the kernel, it seems customary to write stmfd sp!, / ldmfd sp!,
> instead of push / pop, so you could always use those mnemonics instead.
> They're equivalent.

The "customary" seems largely a consequence of having the #define in 
<asm/assembler.h> as you get a compile error if you use push/pop as 
instruction.

I've made the change to "setmode" and stmfd/ldmfd, ok.

[ ... ]
> I think I agree.  Few drivers use FIQ, and if there are drivers
> which use FIQ but don't implement suspend/resume hooks, that sounds
> like a driver bug.
>
> Assuming nobody disagrees with that conclusion, it would be a good
> idea to stick a comment somewhere explaining that policy.

<speak up or remain silent forever> ;-)

Since the change which moved get/set_fiq_regs() to pure assembly has just 
gone in, would a simple comment update in the header file along those 
lines be sufficient ?

I.e. state "do not assume persistence of these registers over power mgmt 
transitions - if that's what you require, implement suspend / resume 
hooks" ?

>
>>
>> See also Russell's mails about that, for previous attempts a year
>> and half a year ago to get "something for ARM hibernation" in:
>>
>> https://lists.linux-foundation.org/pipermail/linux-pm/2010-July/027571.html
>> https://lists.linux-foundation.org/pipermail/linux-pm/2010-December/029665.html
>
> Relying on drivers to save/restore the FIQ state if necessary seems
> to correspond to what Russell is saying in his second mail.

Agreed, and largely why I haven't bothered touching FIQ.

>
>>
>> The kernel doesn't do IRQ/FIQ/ABT/UND save / restore for
>> suspend-to-ram either. CPU hotplug support (re)initializes those. I
>> believe we're fine.
>
> OK, just wanted to make sure I understood that right.

By and large, "if suspend-to-ram works, suspend-to-disk should too" seems 
a good idea; if the former doesn't (need to) save/restore such state even 
though it's not off-mode-persistent, then the latter doesn't need either.

That said, I've seen (out-of-tree) SoC-specific *suspend() code that 
simply blotted everything out. Seems the most trivial way to go, but not 
necessarily the best.

>
> I'll leave it to others to comment on the actual SoC-specific routines,
> but thanks for the illustration.

To make this clear, I'm not personally considering these parts optimal as 
the attached patch is doing them.

That's why preferrably, only the "enabler" code should go in first.

I do wonder, though, whether the machine maintainers are willing to make 
the change to these low-level suspend parts that'd split chip state 
save/restore from the actual power transition. That'd allow to turn this 
from a "backdoor" into a proper use of the interface.

Russell has made the change to pass the context buffer as argument, but 
not split the power transition off; things are getting there, eventually 
:)

>
> Cheers
> ---Dave

Thanks again,
FrankH.
Nicolas Pitre - May 20, 2011, 5:53 p.m.
On Fri, 20 May 2011, Dave Martin wrote:

> On Fri, May 20, 2011 at 01:39:37PM +0100, Frank Hofmann wrote:
> > I've simply done there what the "setmode" macro from
> > <asm/assembler.h> is doing, have chosen not to include that file
> > because it overrides "push" on a global scale for no good reason and
> > that sort of thing makes me cringe.
> 
> Actually, I guess you should be using that header, from a general
> standardisation point of view.  In particular, it would be nicer to
> use setmode than to copy it.

Absolutely.  If there are issues with the generic infrastructure 
provided to you, by all means let's find a way to fix them instead of 
locally sidestepping them.

> The setmode macro itself could be improved to use cps for Thumb-2,
> but that would be a separate (and low-priority) patch.
> 
> Re the push/pop macros, I'm not sure of the history for those.

I'm responsible for those, from many years ago (November 2005 according 
to Git) when push didn't even exist as an official ARM mnemonic.  BTW 
the converse macro is pull not pop.  Those are used to write endian 
independent assembly string copy routines.

> In the kernel, it seems customary to write stmfd sp!, / ldmfd sp!,
> instead of push / pop, so you could always use those mnemonics instead.
> They're equivalent.

Yes, and more importantly they're backward compatible with older 
binutils version with no knowledge of the latest "unified" syntax.  We 
preferably don't want to break compilation with those older tools, which 
is why we stick to ldmfd/stmfd.

> > >>+	stm	r0!, {r4-r12,lr}	/* nonvolatile regs */

This asks to be written as "stmia r0!, ..." as well for the same 
reasons.


Nicolas
Russell King - ARM Linux - May 20, 2011, 6:07 p.m.
On Fri, May 20, 2011 at 01:39:37PM +0100, Frank Hofmann wrote:
> I've simply done there what the "setmode" macro from <asm/assembler.h>  
> is doing, have chosen not to include that file because it overrides 
> "push" on a global scale for no good reason and that sort of thing makes 
> me cringe.

"push" was never an ARM instruction until someone decided to make r13
"special".  As our macros there pre-date the invention of the new ARM
instruction neumonics, it takes precident _especially_ as there's
perfectly good alternative ways to say "push" to the assembler.
Rafael J. Wysocki - May 20, 2011, 10:27 p.m.
On Friday, May 20, 2011, Frank Hofmann wrote:
> On Fri, 20 May 2011, Dave Martin wrote:
> 
> [ ... ]
> >> +/*
> >> + * Save the current CPU state before suspend / poweroff.
> >> + */
> >> +ENTRY(swsusp_arch_suspend)
> >> +	ldr	r0, =__swsusp_arch_ctx
> >> +	mrs	r1, cpsr
> >> +	str	r1, [r0], #4		/* CPSR */
> >> +ARM(	msr	cpsr_c, #SYSTEM_MODE					)
> >> +THUMB(	mov	r2, #SYSTEM_MODE					)
> >> +THUMB(	msr	cpsr_c, r2						)
> >
> > For Thumb-2 kernels, you can use the cps instruction to change the CPU
> > mode:
> > 	cps	#SYSTEM_MODE
> >
> > For ARM though, this instruction is only supported for ARMv6 and above,
> > so it's best to keep the msr form for compatibility for that case.
> 
> Ah, ok, no problem will make that change, looks good.
> 
> Do all ARMs have cpsr / spsr as used here ? Or is that code restricted to 
> ARMv5+ ? I don't have the CPU evolution history there, only got involved 
> with ARM when armv6 already was out.
> 
> I've simply done there what the "setmode" macro from <asm/assembler.h> 
> is doing, have chosen not to include that file because it overrides "push" 
> on a global scale for no good reason and that sort of thing makes me 
> cringe.
> 
> 
> >
> >> +	stm	r0!, {r4-r12,lr}	/* nonvolatile regs */
> >
> > Since r12 is allowed to be corrupted across a function call, we
> > probably don't need to save it.
> 
> ah ok thx for clarification. Earlier iterations of the patch just saved 
> r0-r14 there, "just to have it all", doing it right is best as always.
> 
> >
> [ ... ]
> >> +	bl	__save_processor_state
> >
> > <aside>
> >
> > Structurally, we seem to have:
> >
> > swsusp_arch_suspend {
> > 	/* save some processor state */
> > 	__save_processor_state();
> >
> > 	swsusp_save();
> > }
> >
> > Is __save_processor_state() intended to encapsulate all the CPU-model-
> > specific state saving?  Excuse my ignorance of previous conversations...
> >
> > </aside>
> 
> You're right.
> 
> I've attached the codechanges which implement __save/__restore... for 
> TI OMAP3 and Samsung S5P64xx, to illustrate, again (that's the stuff 
> referred to in the earlier mail I mentioned in first post; beware of code 
> churn in there, those diffs don't readily apply to 'just any' kernel).
> 
> These hooks are essentially the same as the machine-specific cpu_suspend() 
> except that we substitute "r0" (the save context after the generic part) 
> as target for where-to-save-the-state, and we make the funcs return 
> instead of switching to OFF mode.
> 
> That's what I meant with "backdoorish". A better way would be to change 
> the cpu_suspend interface so that it returns instead of directly switching 
> to off mode / powering down.
> 
> Russell has lately been making changes in this area; the current kernels 
> are a bit different here since the caller already supplies the 
> state-save-buffer, while the older ones used to choose in some 
> mach-specific way where to store that state.
> 
> (one of the reasons why these mach-dependent enablers aren't part of this 
> patch suggestion ...)
> 
> 
> >
> >> +	pop	{lr}
> >> +	b	swsusp_save
> >> +ENDPROC(swsusp_arch_suspend)
> >
> > I'm not too familiar with the suspend/resume stuff, so I may be asking
> > a dumb question here, but:
> >
> > Where do we save/restore r8_FIQ..r13_FIQ, r13_IRQ, r13_UND and r13_ABT?
> > (I'm assuming there's no reason to save/restore r14 or SPSR for any
> > exception mode, since we presumably aren't going to suspend/resume
> > from inside an exception handler (?))
> >
> > The exception stack pointers might conceivably be reinitialised to
> > sane values on resume, without necessarily needing to save/restore
> > them, providing my assumption in the previous paragraph is correct.
> >
> > r8_FIQ..r12_FIQ can store arbitrary state used by the FIQ handler,
> > if FIQ is in use.  Can we expect any driver using FIQ to save/restore
> > this state itself, rather than doing it globally?  This may be
> > reasonable.
> 
> We don't need to save/restore those, because at the time the snapshot is 
> taken interrupts are off and we cannot be in any trap handler either. On 
> resume, the kernel that has been loaded has initialized them properly 
> already.

I'm not sure if this is a safe assumption in general.  We may decide to
switch to loading hibernate images from boot loaders, for example, and
it may not hold any more.  Generally, please don't assume _anything_ has
been properly initialized during resume, before the image is loaded.
This has already beaten us a couple of times.

Thanks,
Rafael
Dave Martin - May 23, 2011, 9:42 a.m.
On Fri, May 20, 2011 at 05:24:05PM +0100, Frank Hofmann wrote:
> On Fri, 20 May 2011, Dave Martin wrote:
> 
> [ ... ]
> >>I've simply done there what the "setmode" macro from
> >><asm/assembler.h> is doing, have chosen not to include that file
> >>because it overrides "push" on a global scale for no good reason and
> >>that sort of thing makes me cringe.
> >
> >Actually, I guess you should be using that header, from a general
> >standardisation point of view.  In particular, it would be nicer to
> >use setmode than to copy it.
> >
> >The setmode macro itself could be improved to use cps for Thumb-2,
> >but that would be a separate (and low-priority) patch.
> >
> >
> >Re the push/pop macros, I'm not sure of the history for those.
> 
> arch/arm/lib are the only users.
> 
> >
> >In the kernel, it seems customary to write stmfd sp!, / ldmfd sp!,
> >instead of push / pop, so you could always use those mnemonics instead.
> >They're equivalent.
> 
> The "customary" seems largely a consequence of having the #define in
> <asm/assembler.h> as you get a compile error if you use push/pop as
> instruction.
> 
> I've made the change to "setmode" and stmfd/ldmfd, ok.
> 
> [ ... ]
> >I think I agree.  Few drivers use FIQ, and if there are drivers
> >which use FIQ but don't implement suspend/resume hooks, that sounds
> >like a driver bug.
> >
> >Assuming nobody disagrees with that conclusion, it would be a good
> >idea to stick a comment somewhere explaining that policy.
> 
> <speak up or remain silent forever> ;-)
> 
> Since the change which moved get/set_fiq_regs() to pure assembly has
> just gone in, would a simple comment update in the header file along
> those lines be sufficient ?

Gone in where?

I haven't submitted my patch to Russell's patch system yet, but it
takes into account earlier discussions and there have been no major
disagreements, so I will submit it if it is not superseded.

> I.e. state "do not assume persistence of these registers over power
> mgmt transitions - if that's what you require, implement suspend /
> resume hooks" ?
> 
> >
> >>
> >>See also Russell's mails about that, for previous attempts a year
> >>and half a year ago to get "something for ARM hibernation" in:
> >>
> >>https://lists.linux-foundation.org/pipermail/linux-pm/2010-July/027571.html
> >>https://lists.linux-foundation.org/pipermail/linux-pm/2010-December/029665.html
> >
> >Relying on drivers to save/restore the FIQ state if necessary seems
> >to correspond to what Russell is saying in his second mail.
> 
> Agreed, and largely why I haven't bothered touching FIQ.
> 
> >
> >>
> >>The kernel doesn't do IRQ/FIQ/ABT/UND save / restore for
> >>suspend-to-ram either. CPU hotplug support (re)initializes those. I
> >>believe we're fine.
> >
> >OK, just wanted to make sure I understood that right.
> 
> By and large, "if suspend-to-ram works, suspend-to-disk should too"
> seems a good idea; if the former doesn't (need to) save/restore such
> state even though it's not off-mode-persistent, then the latter
> doesn't need either.
> 
> That said, I've seen (out-of-tree) SoC-specific *suspend() code that
> simply blotted everything out. Seems the most trivial way to go, but
> not necessarily the best.
> 
> >
> >I'll leave it to others to comment on the actual SoC-specific routines,
> >but thanks for the illustration.
> 
> To make this clear, I'm not personally considering these parts
> optimal as the attached patch is doing them.
> 
> That's why preferrably, only the "enabler" code should go in first.
> 
> I do wonder, though, whether the machine maintainers are willing to
> make the change to these low-level suspend parts that'd split chip
> state save/restore from the actual power transition. That'd allow to
> turn this from a "backdoor" into a proper use of the interface.
> 
> Russell has made the change to pass the context buffer as argument,
> but not split the power transition off; things are getting there,
> eventually :)
> 
> >
> >Cheers
> >---Dave
> 
> Thanks again,
> FrankH.
Dave Martin - May 23, 2011, 9:52 a.m.
On Sat, May 21, 2011 at 12:27:16AM +0200, Rafael J. Wysocki wrote:
> On Friday, May 20, 2011, Frank Hofmann wrote:
> > On Fri, 20 May 2011, Dave Martin wrote:
> > 
> > [ ... ]
> > >> +/*
> > >> + * Save the current CPU state before suspend / poweroff.
> > >> + */
> > >> +ENTRY(swsusp_arch_suspend)
> > >> +	ldr	r0, =__swsusp_arch_ctx
> > >> +	mrs	r1, cpsr
> > >> +	str	r1, [r0], #4		/* CPSR */
> > >> +ARM(	msr	cpsr_c, #SYSTEM_MODE					)
> > >> +THUMB(	mov	r2, #SYSTEM_MODE					)
> > >> +THUMB(	msr	cpsr_c, r2						)
> > >
> > > For Thumb-2 kernels, you can use the cps instruction to change the CPU
> > > mode:
> > > 	cps	#SYSTEM_MODE
> > >
> > > For ARM though, this instruction is only supported for ARMv6 and above,
> > > so it's best to keep the msr form for compatibility for that case.
> > 
> > Ah, ok, no problem will make that change, looks good.
> > 
> > Do all ARMs have cpsr / spsr as used here ? Or is that code restricted to 
> > ARMv5+ ? I don't have the CPU evolution history there, only got involved 
> > with ARM when armv6 already was out.
> > 
> > I've simply done there what the "setmode" macro from <asm/assembler.h> 
> > is doing, have chosen not to include that file because it overrides "push" 
> > on a global scale for no good reason and that sort of thing makes me 
> > cringe.
> > 
> > 
> > >
> > >> +	stm	r0!, {r4-r12,lr}	/* nonvolatile regs */
> > >
> > > Since r12 is allowed to be corrupted across a function call, we
> > > probably don't need to save it.
> > 
> > ah ok thx for clarification. Earlier iterations of the patch just saved 
> > r0-r14 there, "just to have it all", doing it right is best as always.
> > 
> > >
> > [ ... ]
> > >> +	bl	__save_processor_state
> > >
> > > <aside>
> > >
> > > Structurally, we seem to have:
> > >
> > > swsusp_arch_suspend {
> > > 	/* save some processor state */
> > > 	__save_processor_state();
> > >
> > > 	swsusp_save();
> > > }
> > >
> > > Is __save_processor_state() intended to encapsulate all the CPU-model-
> > > specific state saving?  Excuse my ignorance of previous conversations...
> > >
> > > </aside>
> > 
> > You're right.
> > 
> > I've attached the codechanges which implement __save/__restore... for 
> > TI OMAP3 and Samsung S5P64xx, to illustrate, again (that's the stuff 
> > referred to in the earlier mail I mentioned in first post; beware of code 
> > churn in there, those diffs don't readily apply to 'just any' kernel).
> > 
> > These hooks are essentially the same as the machine-specific cpu_suspend() 
> > except that we substitute "r0" (the save context after the generic part) 
> > as target for where-to-save-the-state, and we make the funcs return 
> > instead of switching to OFF mode.
> > 
> > That's what I meant with "backdoorish". A better way would be to change 
> > the cpu_suspend interface so that it returns instead of directly switching 
> > to off mode / powering down.
> > 
> > Russell has lately been making changes in this area; the current kernels 
> > are a bit different here since the caller already supplies the 
> > state-save-buffer, while the older ones used to choose in some 
> > mach-specific way where to store that state.
> > 
> > (one of the reasons why these mach-dependent enablers aren't part of this 
> > patch suggestion ...)
> > 
> > 
> > >
> > >> +	pop	{lr}
> > >> +	b	swsusp_save
> > >> +ENDPROC(swsusp_arch_suspend)
> > >
> > > I'm not too familiar with the suspend/resume stuff, so I may be asking
> > > a dumb question here, but:
> > >
> > > Where do we save/restore r8_FIQ..r13_FIQ, r13_IRQ, r13_UND and r13_ABT?
> > > (I'm assuming there's no reason to save/restore r14 or SPSR for any
> > > exception mode, since we presumably aren't going to suspend/resume
> > > from inside an exception handler (?))
> > >
> > > The exception stack pointers might conceivably be reinitialised to
> > > sane values on resume, without necessarily needing to save/restore
> > > them, providing my assumption in the previous paragraph is correct.
> > >
> > > r8_FIQ..r12_FIQ can store arbitrary state used by the FIQ handler,
> > > if FIQ is in use.  Can we expect any driver using FIQ to save/restore
> > > this state itself, rather than doing it globally?  This may be
> > > reasonable.
> > 
> > We don't need to save/restore those, because at the time the snapshot is 
> > taken interrupts are off and we cannot be in any trap handler either. On 
> > resume, the kernel that has been loaded has initialized them properly 
> > already.
> 
> I'm not sure if this is a safe assumption in general.  We may decide to
> switch to loading hibernate images from boot loaders, for example, and
> it may not hold any more.  Generally, please don't assume _anything_ has
> been properly initialized during resume, before the image is loaded.
> This has already beaten us a couple of times.

Surely when resuming via the bootloader, the bootloader must still
branch to some resume entry point in the kernel?

That resume code would be responsible for doing any OS-specific
initialisation and waking up the kernel; plus, the kernel should
not re-enable interrupts until the kernel state has been restored
anyway.  It wouldn't be the responsibility of the bootloader to
set up the relevant state.

Cheers
---Dave
Frank Hofmann - May 23, 2011, 1:37 p.m.
On Mon, 23 May 2011, Dave Martin wrote:

> On Sat, May 21, 2011 at 12:27:16AM +0200, Rafael J. Wysocki wrote:
>> On Friday, May 20, 2011, Frank Hofmann wrote:
[ ... ]
>>>> r8_FIQ..r12_FIQ can store arbitrary state used by the FIQ handler,
>>>> if FIQ is in use.  Can we expect any driver using FIQ to save/restore
>>>> this state itself, rather than doing it globally?  This may be
>>>> reasonable.
>>>
>>> We don't need to save/restore those, because at the time the snapshot is
>>> taken interrupts are off and we cannot be in any trap handler either. On
>>> resume, the kernel that has been loaded has initialized them properly
>>> already.
>>
>> I'm not sure if this is a safe assumption in general.  We may decide to
>> switch to loading hibernate images from boot loaders, for example, and
>> it may not hold any more.  Generally, please don't assume _anything_ has
>> been properly initialized during resume, before the image is loaded.
>> This has already beaten us a couple of times.
>
> Surely when resuming via the bootloader, the bootloader must still
> branch to some resume entry point in the kernel?
>
> That resume code would be responsible for doing any OS-specific
> initialisation and waking up the kernel; plus, the kernel should
> not re-enable interrupts until the kernel state has been restored
> anyway.  It wouldn't be the responsibility of the bootloader to
> set up the relevant state.

What is whose' responsibility ...

It's as easy to say "you can't assume anything" as it is "you must assume 
that ...". Hopefully, this isn't going to become philosophical ;-)

There are two things in the context of hibernation that were mentioned:

a) CPU interrupt stacks (FIQ/IRQ/UND/ABT stacks and reg sets)
b) swapper_pg_dir

Regarding the latter:

As far as swsusp_arch_resume() is concerned, by the time that function is 
called one thing definitely has happened - the loading of the image.

That on the other hand requires some form of MMU setup having happened 
before, because the resume image metadata (restore_pglist and the virtual 
addresses used in the linked "struct pbe") have been created.
Also, since the code somehow ended up in swsusp_arch_resume, that part of 
the kernel text must have been loaded, and mapped.

(If it weren't so, then how did whichever entity loaded the image manage 
to create the list linkage ? How did it enter swsusp_arch_resume ?)

Am I understanding that part correctly ?

If so, then that part at least concedes that on ARM, one can safely switch 
to swapper_pg_dir. Because on ARM, those are the tables which are supplied 
by bootloader and/or kernel decompressor anyway.

That's why I consider it safe to switch to swapper_pg_dir mappings. Note 
the second arg to cpu_switch_mm() is an "output context", I've moved that 
now from using the current thread's (current->active_mm) to the temporary 
global (init_mm), so there's no longer any reliance on having a kernel 
thread context at the time swsusp_arch_resume() is entered.



Regarding interrupt stacks:

cpu_init() looks like the way to go for those. That takes care of them.

Seems better to do that as well than to enforce saving this context over a 
power transition; if the kernel might find a good reason to have different 
FIQ/IRQ/UND/ABT stacks after power transitions (just hypothetically) the 
hibernate code should get out of its way.

I've added the call to cpu_init() at the end of swsusp_arch_resume(); that 
again brings it in line with the existing cpu idle code for various ARM 
incarnations.

I've also, for good measure, added a local_fiq_disable() / enable() 
bracked within save/restore_processor_state(). Again, no more than the 
cpu_idle code does, so hibernation should as well.


What I've found necessary to save/restore via swsusp_arch_suspend/resume 
are the SYSTEM_MODE and SVC_MODE registers.
Yesterday, I had thought that cpu_init() resets SVC_MODE sufficiently but 
that doesn't seem to be the case, if I leave that out, resume-from-disk 
doesn't work anymore.




Regarding other state:

In the first email on this thread, I've said "let's talk the generic code 
only (and ignore what's in __save/__restore_processor_state for now)".

Maybe it's a good idea to look into the machine type code again at this 
point ...
The CPU idle code for various ARM incarnations does state saves for 
subsystems beyond just the core; e.g. omap_sram_idle() does:

 	omap3_core_restore_context();
 	omap3_prcm_restore_context();
 	omap3_sram_restore_context();
 	omap2_sms_restore_context();

Samsung ARM11's do in their cpu_idle:

 	s3c_pm_restore_core();
 	s3c_pm_restore_uarts();
 	s3c_pm_restore_gpios();

and so on; this is currently not covered by the resume-from-disk code that 
I have; the lack of that might be a possible cause for e.g. the omap video 
restore issues I've seen. That's speculation, though.


Is this stuff needed (for suspend-to/resume-from-disk) ?

If so:
>From the way the the suspend-to-ram code is currently modeled, such state 
is saved before the "SoC ARM context" parts, and restored after that and 
the cpu_init() call. All ARM machines seem to do like:

...pm_enter():

 	local_irq_disable()
 	local_fiq_disable();
 	... maybe: save "machine-specific" stuff ...

 	... enter idle
 		==> save SoC state (CP regs)
 		<== restore
 	cpu_init()

 	... maybe: restore "machine-specific" stuff ...
 	local_fiq_enable()
 	local_irq_enable()

Anyway, that given, I wonder whether it makes sense to give the machines a 
hook into save/restore_processor_state.
The other option of course it to be lazy (call it "flexible" if you 
wish) and leave the cpu_init() call to the (single) existing machine-hook.


Thanks,
FrankH.
Russell King - ARM Linux - May 23, 2011, 2:32 p.m.
On Mon, May 23, 2011 at 02:37:19PM +0100, Frank Hofmann wrote:
> What I've found necessary to save/restore via swsusp_arch_suspend/resume  
> are the SYSTEM_MODE and SVC_MODE registers.
> Yesterday, I had thought that cpu_init() resets SVC_MODE sufficiently but 
> that doesn't seem to be the case, if I leave that out, resume-from-disk  
> doesn't work anymore.

You will be running in SVC mode, so the SVC mode registers are your
current register set.  At some point you need to do an effective
"context switch" between the kernel doing the resume and the kernel
which was running.  That involves restoring the saved register state.

System mode on the other hand is unused by the kernel.

Patch

diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S

index ea4e498..19ecd8e 100644

--- a/arch/arm/mach-omap2/sleep34xx.S

+++ b/arch/arm/mach-omap2/sleep34xx.S

@@ -358,6 +358,7 @@  logic_l1_restore:

 	ldr	r4, scratchpad_base
 	ldr	r3, [r4,#0xBC]
 	adds	r3, r3, #16
+.Llogic_l1_restore_internal:

 	ldmia	r3!, {r4-r6}
 	mov	sp, r4
 	msr	spsr_cxsf, r5
@@ -433,6 +434,10 @@  ttbr_error:

 	*/
 	b	ttbr_error
 usettbr0:
+#ifdef CONFIG_HIBERNATION

+	cmp	r1, #1

+	ldmeqfd	sp!, { r0 - r12, pc }	@ early return from __restore_processor_state

+#endif

 	mrc	p15, 0, r2, c2, c0, 0
 	ldr	r5, ttbrbit_mask
 	and	r2, r5
@@ -471,6 +476,25 @@  usettbr0:

 	mcr	p15, 0, r4, c1, c0, 0
 
 	ldmfd	sp!, {r0-r12, pc}		@ restore regs and return
+

+#ifdef CONFIG_HIBERNATION

+ENTRY(__save_processor_state)

+	stmfd	sp!, {r0-r12, lr}

+	mov	r1, #0x4

+	mov	r8, r0

+	b	l1_logic_lost

+ENDPROC(__save_processor_state)

+

+ENTRY(__restore_processor_state)

+	stmfd	sp!, { r0 - r12, lr }

+	str	sp, [r0]		@ fixup saved stack pointer

+	str	lr, [r0, #8]		@ fixup saved link register

+	mov	r3, r0

+	mov	r1, #1

+	b	.Llogic_l1_restore_internal

+ENDPROC(__restore_processor_state)

+#endif

+

 save_context_wfi:
 	/*b	save_context_wfi*/	@ enable to debug save code
 	mov	r8, r0 /* Store SDRAM address in r8 */
@@ -545,6 +569,10 @@  l1_logic_lost:

 	mrc	p15, 0, r4, c1, c0, 0
 	/* save control register */
 	stmia	r8!, {r4}
+#ifdef CONFIG_HIBERNATION

+	cmp	r1, #4

+	ldmeqfd	sp!, {r0-r12, pc}	@ early return from __save_processor_state

+#endif

 clean_caches:
 	/* Clean Data or unified cache to POU*/
 	/* How to invalidate only L1 cache???? - #FIX_ME# */
diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig

index df5ce56..b4713ba 100644

--- a/arch/arm/plat-omap/Kconfig

+++ b/arch/arm/plat-omap/Kconfig

@@ -23,6 +23,7 @@  config ARCH_OMAP3

 	select CPU_V7
 	select COMMON_CLKDEV
 	select OMAP_IOMMU
+	select ARCH_HIBERNATION_POSSIBLE

 
 config ARCH_OMAP4
 	bool "TI OMAP4"