ARM hibernation / suspend-to-disk support code

Quoted 36 lines · Frank Hofmann

On Fri, 20 May 2011, Dave Martin wrote:
[ ... ]
Quoted 9 lines
Quoted 4 lines
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.
Quoted 5 lines
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.
Quoted 4 lines
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.
[ ... ]
Quoted 4 lines
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.

Quoted 8 lines

Quoted 2 lines
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.

Quoted 58 lines

I.e. state "do not assume persistence of these registers over power mgmt transitions - if that's what you require, implement suspend / resume hooks" ?
Quoted 10 lines
Quoted 7 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
Quoted 3 lines
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 :)
Quoted 3 lines
Cheers
---Dave
Thanks again,
FrankH.

Frank Hofmann

author

2011-05-20 16:24 UTC #8 lore raw

On Fri, 20 May 2011, Dave Martin wrote:

[ ... ]

Quoted 9 lines

Quoted 4 lines
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.

Quoted 5 lines

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.

Quoted 4 lines

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.

[ ... ]

Quoted 6 lines

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" ?

Quoted 10 lines

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

Quoted 7 lines

Quoted 5 lines
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.

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.

Quoted 3 lines

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.

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

Quoted 3 lines

Cheers
---Dave

Thanks again,
FrankH.

Nicolas Pitre

2011-05-20 17:53 UTC #9 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 9 lines · Dave Martin

Quoted 4 lines · Frank Hofmann
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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 1 line

Quoted 1 line
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +1 -0
+	stm	r0!, {r4-r12,lr}	/* nonvolatile regs */

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

Nicolas

Nicolas Pitre

2011-05-20 17:53 UTC #10 lore raw

Quoted 9 lines · Dave Martin

Quoted 4 lines · Frank Hofmann
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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 1 line

Quoted 1 line
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +1 -0
+	stm	r0!, {r4-r12,lr}	/* nonvolatile regs */

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

Nicolas

Dave Martin

2011-05-20 15:03 UTC #11 lore raw

Quoted 18 lines · Frank Hofmann

On Fri, 20 May 2011, Dave Martin wrote:
[ ... ]
Quoted 15 lines
Review
Quoted 10 lines
(quoted diff) +10 -0
+/*
+ * 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

Quoted 8 lines

Quoted 2 lines
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...)

Quoted 5 lines

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.

Quoted 80 lines

Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +1 -0
+	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.
Quoted 1 line
[ ... ]
Quoted 13 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+	bl	__save_processor_state
<aside>
Structurally, we seem to have:
swsusp_arch_suspend {
	/* save some processor state */
	__save_processor_state();

	swsusp_save();
}
Quoted 4 lines
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 ...)
Quoted 13 lines
Review
Quoted 4 lines
(quoted diff) +3 -0
+	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 (?))
Quoted 8 lines
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.

Quoted 7 lines

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

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.

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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

Quoted 185 lines

Quoted 7 lines

Cheers
---Dave

Thanks,
FrankH.

arch/arm/plat-s5p/sleep.S +35 -2

index 2cdae4a..dd9516b 100644
@@ -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

arch/arm/mach-omap2/sleep34xx.S +28 -0

index ea4e498..19ecd8e 100644
@@ -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# */

arch/arm/plat-omap/Kconfig +1 -0

index df5ce56..b4713ba 100644
@@ -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"

Russell King - ARM Linux

2011-05-20 18:07 UTC #12 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 4 lines · Frank Hofmann

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.

Frank Hofmann

author

2011-05-22 06:39 UTC #13 lore raw

Hi Russell / Nicolas,

thanks for the clarification on ARM history; there's a lot to learn for me about that still.

I'll reply to Rafael regarding state save/restore.

FrankH.

-----Original Message-----
From: Russell King - ARM Linux [mailto:[email protected]]
Sent: Fri 20/05/2011 20:07
To: Frank Hofmann
Cc: Dave Martin; [email protected]; [email protected]; [email protected]
Subject: Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code

Quoted 4 lines · Frank Hofmann

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.

Russell King - ARM Linux

2011-05-20 18:07 UTC #14 lore raw

Quoted 4 lines · Frank Hofmann

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.

Rafael J. Wysocki

2011-05-20 22:27 UTC #15 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 109 lines · Frank Hofmann

On Fri, 20 May 2011, Dave Martin wrote:
[ ... ]
Quoted 15 lines
Review
Quoted 10 lines
(quoted diff) +10 -0
+/*
+ * 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
Quoted 2 lines
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.
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +1 -0
+	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.
Quoted 1 line
[ ... ]
Quoted 13 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+	bl	__save_processor_state
<aside>
Structurally, we seem to have:
swsusp_arch_suspend {
	/* save some processor state */
	__save_processor_state();

	swsusp_save();
}
Quoted 4 lines
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 ...)
Quoted 13 lines
Review
Quoted 4 lines
(quoted diff) +3 -0
+	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 (?))
Quoted 8 lines
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.

Quoted 4 lines

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

Frank Hofmann

author

2011-05-22 07:01 UTC #16 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 11 lines

[ ... ]
Quoted 9 lines
Quoted 4 lines
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.

Quoted 8 lines

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

Hi Rafael,

regarding "cannot rely on any state", if that is so then it seems quite unnecessary to save/restore _any_ ARM non-#SYSTEM_MODE state - it'd better be reinitialized by calling cpu_init() after the "core restore" callback. The archives were quite a bit helpful in this context:

http://www.mail-archive.com/linux-omap at vger.kernel.org/msg12671.html

that's the same situation isn't it ?

Regarding FIQ: I agree with Russell and others who previously stated that a driver using FIQ is responsible for implementing suspend/resume hooks itself. But what could be done is to disable/enable it for the snapshot, just in case.

I've also found out that the vmlinux.lds changes don't seem to be necessary - they've been the last remnant of the "old" ARM hibernation patch, but just leaving those out looks to make no difference (the nosave data still ends up in the same place, with the same elf section attributes).

With all this in mind, the core part of the patch becomes somewhat simpler. See attached.

I'll test this on Monday.
FrankH.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110522/71096fef/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hibernation-core-22May2011.patch
Type: text/x-patch
Size: 5391 bytes
Desc: hibernation-core-22May2011.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110522/71096fef/attachment.bin>

Rafael J. Wysocki

2011-05-22 09:54 UTC #17 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 17 lines · Frank Hofmann

Quoted 17 lines
[ ... ]
Quoted 9 lines
Quoted 4 lines
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.

Quoted 8 lines

Quoted 2 lines
Thanks,
Rafael
Hi Rafael,
regarding "cannot rely on any state", if that is so then it seems quite unnecessary to save/restore _any_ ARM non-#SYSTEM_MODE state - it'd better be reinitialized by calling cpu_init() after the "core restore" callback.

If they are always initialized in the same way and if you can do the CPU initialization at this point, I agree.

Quoted 5 lines

The archives were quite a bit helpful in this context:
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg12671.html
that's the same situation isn't it ?

I'm not sure, I'm not an ARM expert, but it seems so.

Quoted 2 lines

Regarding FIQ: I agree with Russell and others who previously stated that a driver using FIQ is responsible for implementing suspend/resume hooks itself.

I agree with that too.

Quoted 1 line

But what could be done is to disable/enable it for the snapshot, just in case.

Quoted 7 lines

I've also found out that the vmlinux.lds changes don't seem to be necessary
- they've been the last remnant of the "old" ARM hibernation patch, but just leaving those out looks to make no difference (the nosave data still ends up in the same place, with the same elf section attributes).
With all this in mind, the core part of the patch becomes somewhat simpler. See attached.

It looks reasonable, but it slightly concerns me that you seem to assume that swapper_pg_dir will be the same in both the boot and the target kernel. We used to make this assumption on x86 too, but it started to cause problems to happen at one point. To address those problems we had to create temporary page tables using page frames that are guaranteed not to be overwritten in the last phase of restore (that would be the for () loop in your __swsusp_arch_restore_image()).

Thanks,
Rafael

Rafael J. Wysocki

2011-05-22 09:54 UTC #18 lore raw

In reply to Rafael J. Wysocki

Quoted 17 lines · Frank Hofmann

Quoted 17 lines
[ ... ]
Quoted 9 lines
Quoted 4 lines
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.

Quoted 8 lines

Quoted 2 lines
Thanks,
Rafael
Hi Rafael,
regarding "cannot rely on any state", if that is so then it seems quite unnecessary to save/restore _any_ ARM non-#SYSTEM_MODE state - it'd better be reinitialized by calling cpu_init() after the "core restore" callback.

If they are always initialized in the same way and if you can do the CPU initialization at this point, I agree.

Quoted 5 lines

The archives were quite a bit helpful in this context:
http://www.mail-archive.com/[email protected]/msg12671.html
that's the same situation isn't it ?

I'm not sure, I'm not an ARM expert, but it seems so.

Quoted 2 lines

Regarding FIQ: I agree with Russell and others who previously stated that a driver using FIQ is responsible for implementing suspend/resume hooks itself.

I agree with that too.

Quoted 1 line

But what could be done is to disable/enable it for the snapshot, just in case.

Quoted 7 lines

I've also found out that the vmlinux.lds changes don't seem to be necessary
- they've been the last remnant of the "old" ARM hibernation patch, but just leaving those out looks to make no difference (the nosave data still ends up in the same place, with the same elf section attributes).
With all this in mind, the core part of the patch becomes somewhat simpler. See attached.

Thanks,
Rafael

Frank Hofmann

author

2011-05-22 07:01 UTC #19 lore raw

Quoted 11 lines

[ ... ]
Quoted 9 lines
Quoted 4 lines
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.

Quoted 8 lines

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

Hi Rafael,

http://www.mail-archive.com/[email protected]/msg12671.html

that's the same situation isn't it ?

With all this in mind, the core part of the patch becomes somewhat simpler. See attached.

I'll test this on Monday.
FrankH.

arch/arm/Kconfig +3 -0

index 44c16f0..f282bac 100644
@@ -207,6 +207,9 @@ config ARM_PATCH_PHYS_VIRT_16BIT
 	def_bool y
 	depends on ARM_PATCH_PHYS_VIRT && ARCH_MSM
 
+config ARCH_HIBERNATION_POSSIBLE
+	def_bool n
+
 source "init/Kconfig"
 
 source "kernel/Kconfig.freezer"

arch/arm/include/asm/memory.h +1 -0

index 431077c..c7ef454 100644
@@ -250,6 +250,7 @@ static inline void *phys_to_virt(phys_addr_t x)
  */
 #define __pa(x)			__virt_to_phys((unsigned long)(x))
 #define __va(x)			((void *)__phys_to_virt((unsigned long)(x)))
+#define __pa_symbol(x)		__pa(RELOC_HIDE((unsigned long)(x),0))
 #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
 
 /*

arch/arm/kernel/Makefile +1 -0

index 8d95446..a1f54cd 100644
@@ -44,6 +44,7 @@ obj-$(CONFIG_ARM_THUMBEE)	+= thumbee.o
 obj-$(CONFIG_KGDB)		+= kgdb.o
 obj-$(CONFIG_ARM_UNWIND)	+= unwind.o
 obj-$(CONFIG_HAVE_TCM)		+= tcm.o
+obj-$(CONFIG_HIBERNATION)	+= cpu.o swsusp.o
 obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
 obj-$(CONFIG_SWP_EMULATE)	+= swp_emulate.o
 CFLAGS_swp_emulate.o		:= -Wa,-march=armv7-a

arch/arm/kernel/cpu.c new file +64 -0

64 lines not on this page · raw message

arch/arm/kernel/swsusp.S new file +68 -0

68 lines not on this page · raw message

Dave Martin

2011-05-23 09:52 UTC #20 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

In reply to Rafael J. Wysocki

Quoted 115 lines · Rafael J. Wysocki

Quoted 109 lines · Frank Hofmann
On Fri, 20 May 2011, Dave Martin wrote:
[ ... ]
Quoted 15 lines
Review
Quoted 10 lines
(quoted diff) +10 -0
+/*
+ * 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
Quoted 2 lines
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.
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +1 -0
+	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.
Quoted 1 line
[ ... ]
Quoted 13 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+	bl	__save_processor_state
<aside>
Structurally, we seem to have:
swsusp_arch_suspend {
	/* save some processor state */
	__save_processor_state();

	swsusp_save();
}
Quoted 4 lines
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 ...)
Quoted 13 lines
Review
Quoted 4 lines
(quoted diff) +3 -0
+	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 (?))
Quoted 8 lines
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.
Quoted 4 lines
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.

Quoted 5 lines

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

author

2011-05-23 13:37 UTC #21 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 2 lines · Dave Martin

Quoted 1 line · Rafael J. Wysocki
On Friday, May 20, 2011, Frank Hofmann wrote:

[ ... ]

Quoted 16 lines

Quoted 10 lines
Quoted 9 lines
Quoted 4 lines
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.
Quoted 5 lines
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.

Quoted 8 lines

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:

Quoted 1 line

From [email protected] Fri May 20 08:44:36 2011

From: [email protected] ()
Date: Fri, 20 May 2011 12:44:36 -0000
Subject: No subject
Message-ID: <[email protected]>

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

2011-05-23 14:32 UTC #22 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 5 lines · Frank Hofmann

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.

Frank Hofmann

author

2011-05-23 15:57 UTC #23 lore raw

RFC PATCH v2 1/1 ARM hibernation / suspend-to-disk support code

Quoted 7 lines · Russell King - ARM Linux

Quoted 5 lines · Frank Hofmann
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.

Quoted 7 lines

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.

Ah, and I had it the other way round ... that's why. Thanks !

I've tried that, saving/restoring just CPSR/SPSR and the reg set - and that seems sufficient, works fine !

All this means that the basic code has again become smaller. Attached is a new version, integrating all the feedback so far:

* save/restore only those parts of the register set that the kernel cannot reinitialize from scratch

* take care of FIQ disable/enable bracketing

* use traditional stmfd/ldmfd instead of push/pop

* don't rely on thread state, current->active_mm, but use global &init_mm

* dump arch_prepare_suspend (skipping ahead of Rafael's suggested fix)

* ditch the vmlinux.lds changes as they're not needed

What other outstanding things are there to address for this ?

All the best,
FrankH.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hibernation-core-23May2011.patch
Type: text/x-diff
Size: 5761 bytes
Desc:
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110523/a8962381/attachment.bin>

Frank Hofmann

author

2011-05-23 15:57 UTC #24 lore raw

RFC PATCH v2 1/1 ARM hibernation / suspend-to-disk support code

Quoted 7 lines · Russell King - ARM Linux

Quoted 5 lines · Frank Hofmann
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.

Quoted 7 lines

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.

Ah, and I had it the other way round ... that's why. Thanks !

I've tried that, saving/restoring just CPSR/SPSR and the reg set - and that seems sufficient, works fine !

All this means that the basic code has again become smaller. Attached is a new version, integrating all the feedback so far:

* save/restore only those parts of the register set that the kernel cannot reinitialize from scratch

* take care of FIQ disable/enable bracketing

* use traditional stmfd/ldmfd instead of push/pop

* don't rely on thread state, current->active_mm, but use global &init_mm

* dump arch_prepare_suspend (skipping ahead of Rafael's suggested fix)

* ditch the vmlinux.lds changes as they're not needed

What other outstanding things are there to address for this ?

All the best,
FrankH.

arch/arm/Kconfig +3 -0

index 6b6786c..859dd86 100644
@@ -198,6 +198,9 @@ config VECTORS_BASE
 config ARCH_HAS_CPU_IDLE_WAIT
 	def_bool y
 
+config ARCH_HIBERNATION_POSSIBLE
+	def_bool n
+
 source "init/Kconfig"
 
 source "kernel/Kconfig.freezer"

arch/arm/include/asm/memory.h +1 -0

index 5421d82..23e93a6 100644
@@ -191,6 +191,7 @@ static inline void *phys_to_virt(unsigned long x)
  */
 #define __pa(x)			__virt_to_phys((unsigned long)(x))
 #define __va(x)			((void *)__phys_to_virt((unsigned long)(x)))
+#define __pa_symbol(x)		__pa(RELOC_HIDE((unsigned long)(x),0))
 #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
 
 /*

arch/arm/kernel/Makefile +1 -0

index c9b00bb..541ac3a 100644
@@ -36,6 +36,7 @@ obj-$(CONFIG_ARM_THUMBEE)	+= thumbee.o
 obj-$(CONFIG_KGDB)		+= kgdb.o
 obj-$(CONFIG_ARM_UNWIND)	+= unwind.o
 obj-$(CONFIG_HAVE_TCM)		+= tcm.o
+obj-$(CONFIG_HIBERNATION)	+= cpu.o swsusp.o
 
 obj-$(CONFIG_CRUNCH)		+= crunch.o crunch-bits.o
 AFLAGS_crunch-bits.o		:= -Wa,-mcpu=ep9312

arch/arm/kernel/cpu.c new file +64 -0

64 lines not on this page · raw message

arch/arm/kernel/swsusp.S new file +72 -0

72 lines not on this page · raw message

Frank Hofmann

author

2011-05-23 13:37 UTC #25 lore raw

In reply to Rafael J. Wysocki

Quoted 2 lines · Dave Martin

Quoted 1 line · Rafael J. Wysocki
On Friday, May 20, 2011, Frank Hofmann wrote:

[ ... ]

Quoted 16 lines

Quoted 10 lines
Quoted 9 lines
Quoted 4 lines
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.
Quoted 5 lines
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.

Quoted 8 lines

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.

(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 ?

Regarding interrupt stacks:

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

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.

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

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();

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

If so:

Quoted 1 line

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()

Thanks,
FrankH.

Dave Martin

2011-05-23 09:52 UTC #26 lore raw

Quoted 115 lines · Rafael J. Wysocki

Quoted 109 lines · Frank Hofmann
On Fri, 20 May 2011, Dave Martin wrote:
[ ... ]
Quoted 15 lines
Review
Quoted 10 lines
(quoted diff) +10 -0
+/*
+ * 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
Quoted 2 lines
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.
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +1 -0
+	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.
Quoted 1 line
[ ... ]
Quoted 13 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+	bl	__save_processor_state
<aside>
Structurally, we seem to have:
swsusp_arch_suspend {
	/* save some processor state */
	__save_processor_state();

	swsusp_save();
}
Quoted 4 lines
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 ...)
Quoted 13 lines
Review
Quoted 4 lines
(quoted diff) +3 -0
+	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 (?))
Quoted 8 lines
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.
Quoted 4 lines
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.

Quoted 5 lines

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?

Cheers
---Dave

Rafael J. Wysocki

2011-05-20 22:27 UTC #27 lore raw

Quoted 109 lines · Frank Hofmann

On Fri, 20 May 2011, Dave Martin wrote:
[ ... ]
Quoted 15 lines
Review
Quoted 10 lines
(quoted diff) +10 -0
+/*
+ * 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
Quoted 2 lines
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.
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +1 -0
+	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.
Quoted 1 line
[ ... ]
Quoted 13 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+	bl	__save_processor_state
<aside>
Structurally, we seem to have:
swsusp_arch_suspend {
	/* save some processor state */
	__save_processor_state();

	swsusp_save();
}
Quoted 4 lines
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 ...)
Quoted 13 lines
Review
Quoted 4 lines
(quoted diff) +3 -0
+	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 (?))
Quoted 8 lines
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.

Quoted 4 lines

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.

Thanks,
Rafael

Russell King - ARM Linux

2011-05-20 18:05 UTC #28 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 4 lines · Dave Martin

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 (?))

There is absolutely no need to save r13 for _any_ of the abort modes. As we have to set them up at boot time to fixed values, we keep that code around, and in the resume paths we re-execute that initialization code. cpu_init().

Out of the list you mention above, the only thing which isn't saved are the FIQ registers, and that's a problem for S2RAM too, and should be done by arch/arm/kernel/fiq.c hooking into the suspend paths.

Dave Martin

2011-05-23 10:01 UTC #29 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 10 lines · Russell King - ARM Linux

Quoted 4 lines · Dave Martin
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 (?))
There is absolutely no need to save r13 for _any_ of the abort modes. As we have to set them up at boot time to fixed values, we keep that code around, and in the resume paths we re-execute that initialization code. cpu_init().

That seemed a sensible possibility, but I'm still not familiar with some of the details. It was, as I suspected, a dumb question from me...

Quoted 3 lines

Out of the list you mention above, the only thing which isn't saved are the FIQ registers, and that's a problem for S2RAM too, and should be done by arch/arm/kernel/fiq.c hooking into the suspend paths.

The alternative view is that the driver using FIQ owns the state in the FIQ mode registers and is therefore responsible for saving and restoring it across suspend/resume. If so, then any additional globally implemented state save/restore of the FIQ mode state is unnecessary.

Do you have an opinion on which is the better model?

Requiring the driver to take responsibility might encourage the driver writers to think about all the implications of save/restore on their
driver: saving/restoring the FIQ mode registers is unlikely to be enough by itself. Depending on the state of the driver, it might also be unnecessary (though it's only a few words of state, so not necessarily worth optimising).

Cheers
---Dave

Russell King - ARM Linux

2011-05-23 10:12 UTC #30 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 9 lines · Dave Martin

Quoted 3 lines · Russell King - ARM Linux
Out of the list you mention above, the only thing which isn't saved are the FIQ registers, and that's a problem for S2RAM too, and should be done by arch/arm/kernel/fiq.c hooking into the suspend paths.
The alternative view is that the driver using FIQ owns the state in the FIQ mode registers and is therefore responsible for saving and restoring it across suspend/resume. If so, then any additional globally implemented state save/restore of the FIQ mode state is unnecessary.

This seems to be most sensible - if the FIQ is being used as a software-DMA, then the hardware side of that needs to be cleanly shutdown (eg, waiting for the DMA to complete before proceeding) to ensure no loss of data. That can't happen from within the FIQ code.

I also wonder about issues of secure/non-secure stuff here too - what that means is that if we have a driver using FIQ mode, then we have FIQ state to save, but if not there's probably no point (or even any way) to save that state ourselves anyway.

Dave Martin

2011-05-23 11:16 UTC #31 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 11 lines · Russell King - ARM Linux

Quoted 9 lines · Dave Martin
Quoted 3 lines · Russell King - ARM Linux
Out of the list you mention above, the only thing which isn't saved are the FIQ registers, and that's a problem for S2RAM too, and should be done by arch/arm/kernel/fiq.c hooking into the suspend paths.
The alternative view is that the driver using FIQ owns the state in the FIQ mode registers and is therefore responsible for saving and restoring it across suspend/resume. If so, then any additional globally implemented state save/restore of the FIQ mode state is unnecessary.

Quoted 4 lines

This seems to be most sensible - if the FIQ is being used as a software-DMA, then the hardware side of that needs to be cleanly shutdown (eg, waiting for the DMA to complete before proceeding) to ensure no loss of data. That can't happen from within the FIQ code.

OK. Frank suggested putting comments to this effect in <asm/fiq.h>.

I think something along these lines might be appropriate:

/*
* The FIQ mode registers are not magically preserved across suspend/resume. *
* Drivers which require these registers to be preserved across power
* management operations must implement appropriate suspend/resume handlers
* to save and restore them.
*/

Is the header file a good place for this, or is there some other better place to put it?

Quoted 4 lines

I also wonder about issues of secure/non-secure stuff here too - what that means is that if we have a driver using FIQ mode, then we have FIQ state to save, but if not there's probably no point (or even any way) to save that state ourselves anyway.

That argument may apply to a ton of state in the Secure World, not just the FIQ registers

The likely model there is that the Secure World must hook somewhere into Linux suspend/resume too, so that its hooks can be called at the appropriate times. This could involve a driver which saves/restores the state of the Secure World, or low-level hooks which get called from the platform power management code.

Either way, Linux doesn't need to be doing anything special except for calling those hooks, just as for any other driver.

Cheers
---Dave

Russell King - ARM Linux

2011-05-23 16:11 UTC #32 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 29 lines · Dave Martin

Quoted 11 lines · Russell King - ARM Linux
Quoted 9 lines · Dave Martin
Quoted 3 lines · Russell King - ARM Linux
Out of the list you mention above, the only thing which isn't saved are the FIQ registers, and that's a problem for S2RAM too, and should be done by arch/arm/kernel/fiq.c hooking into the suspend paths.
The alternative view is that the driver using FIQ owns the state in the FIQ mode registers and is therefore responsible for saving and restoring it across suspend/resume. If so, then any additional globally implemented state save/restore of the FIQ mode state is unnecessary.
Quoted 4 lines
This seems to be most sensible - if the FIQ is being used as a software-DMA, then the hardware side of that needs to be cleanly shutdown (eg, waiting for the DMA to complete before proceeding) to ensure no loss of data. That can't happen from within the FIQ code.
OK. Frank suggested putting comments to this effect in <asm/fiq.h>.
I think something along these lines might be appropriate:
/*
* The FIQ mode registers are not magically preserved across suspend/resume. *
* Drivers which require these registers to be preserved across power
* management operations must implement appropriate suspend/resume handlers
* to save and restore them.
*/

Quoted 2 lines

Is the header file a good place for this, or is there some other better place to put it?

I tend to suggest that the header file is the right place, because that's where the interface is defined. Other people argue that its more likely to be seen in the implementation (fiq.c). So I'm tempted to say both, but lets go with fiq.h for the time being.

Quoted 2 lines

That argument may apply to a ton of state in the Secure World, not just the FIQ registers

It very much does, and I know OMAP has some kind of SMC call to handle this.

Dave Martin

2011-05-23 16:38 UTC #33 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 30 lines · Russell King - ARM Linux

Quoted 29 lines · Dave Martin
Quoted 11 lines · Russell King - ARM Linux
Quoted 9 lines · Dave Martin
Quoted 3 lines · Russell King - ARM Linux
Out of the list you mention above, the only thing which isn't saved are the FIQ registers, and that's a problem for S2RAM too, and should be done by arch/arm/kernel/fiq.c hooking into the suspend paths.
The alternative view is that the driver using FIQ owns the state in the FIQ mode registers and is therefore responsible for saving and restoring it across suspend/resume. If so, then any additional globally implemented state save/restore of the FIQ mode state is unnecessary.
Quoted 4 lines
This seems to be most sensible - if the FIQ is being used as a software-DMA, then the hardware side of that needs to be cleanly shutdown (eg, waiting for the DMA to complete before proceeding) to ensure no loss of data. That can't happen from within the FIQ code.
OK. Frank suggested putting comments to this effect in <asm/fiq.h>.
I think something along these lines might be appropriate:
/*
* The FIQ mode registers are not magically preserved across suspend/resume. *
* Drivers which require these registers to be preserved across power
* management operations must implement appropriate suspend/resume handlers
* to save and restore them.
*/

Quoted 7 lines

Quoted 2 lines
Is the header file a good place for this, or is there some other better place to put it?
I tend to suggest that the header file is the right place, because that's where the interface is defined. Other people argue that its more likely to be seen in the implementation (fiq.c). So I'm tempted to say both, but lets go with fiq.h for the time being.

OK -- the {get,set}_fiq_regs patch is currently in your patch system.

If you have no objection, I'll submit a patch adding the above text to apply on top of the other patch (or, if possible, orthogonally).

---Dave

Quoted 6 lines

Quoted 3 lines
That argument may apply to a ton of state in the Secure World, not just the FIQ registers
It very much does, and I know OMAP has some kind of SMC call to handle this.

Frank Hofmann

author

2011-05-24 12:33 UTC #34 lore raw

RFC PATCH 1/1 ARM hibernation / suspend-to-disk support code

Quoted 1 line · Dave Martin

On Mon, May 23, 2011 at 05:11:49PM +0100, Russell King - ARM Linux wrote:

[ ... ]

Quoted 11 lines

Quoted 4 lines
I tend to suggest that the header file is the right place, because that's where the interface is defined. Other people argue that its more likely to be seen in the implementation (fiq.c). So I'm tempted to say both, but lets go with fiq.h for the time being.
OK -- the {get,set}_fiq_regs patch is currently in your patch system.
If you have no objection, I'll submit a patch adding the above text to apply on top of the other patch (or, if possible, orthogonally).
---Dave

Thanks a lot !

Quoted 8 lines

Quoted 7 lines
Quoted 4 lines
That argument may apply to a ton of state in the Secure World, not just the FIQ registers
It very much does, and I know OMAP has some kind of SMC call to handle this.

Yes, _omap_sram_idle() does that bit, it gives a physical address to the OMAP ROM code to save/restore the "secure state" in, triggered via smc.

Anyway, architecturally it seems to be much cleaner to _allow_ device drivers (or machine-specific hooks) to save/restore _more_ state than whatever the "core suspend code" would do, instead of _forcing_ the core suspend code to do everything-and-the-kitchen-sink.
That's where things like FIQ or secure state would be.

FrankH.

Frank Hofmann

author

2011-05-24 12:33 UTC #35 lore raw

Quoted 1 line · Dave Martin

On Mon, May 23, 2011 at 05:11:49PM +0100, Russell King - ARM Linux wrote:

[ ... ]

Quoted 11 lines

Quoted 4 lines
I tend to suggest that the header file is the right place, because that's where the interface is defined. Other people argue that its more likely to be seen in the implementation (fiq.c). So I'm tempted to say both, but lets go with fiq.h for the time being.
OK -- the {get,set}_fiq_regs patch is currently in your patch system.
If you have no objection, I'll submit a patch adding the above text to apply on top of the other patch (or, if possible, orthogonally).
---Dave

Thanks a lot !

Quoted 8 lines

Quoted 7 lines
Quoted 4 lines
That argument may apply to a ton of state in the Secure World, not just the FIQ registers
It very much does, and I know OMAP has some kind of SMC call to handle this.

Yes, _omap_sram_idle() does that bit, it gives a physical address to the OMAP ROM code to save/restore the "secure state" in, triggered via smc.

FrankH.

Dave Martin

2011-05-23 16:38 UTC #36 lore raw

Quoted 30 lines · Russell King - ARM Linux

Quoted 29 lines · Dave Martin
Quoted 11 lines · Russell King - ARM Linux
Quoted 9 lines · Dave Martin
Quoted 3 lines · Russell King - ARM Linux
Out of the list you mention above, the only thing which isn't saved are the FIQ registers, and that's a problem for S2RAM too, and should be done by arch/arm/kernel/fiq.c hooking into the suspend paths.
The alternative view is that the driver using FIQ owns the state in the FIQ mode registers and is therefore responsible for saving and restoring it across suspend/resume. If so, then any additional globally implemented state save/restore of the FIQ mode state is unnecessary.
Quoted 4 lines
This seems to be most sensible - if the FIQ is being used as a software-DMA, then the hardware side of that needs to be cleanly shutdown (eg, waiting for the DMA to complete before proceeding) to ensure no loss of data. That can't happen from within the FIQ code.
OK. Frank suggested putting comments to this effect in <asm/fiq.h>.
I think something along these lines might be appropriate:
/*
* The FIQ mode registers are not magically preserved across suspend/resume. *
* Drivers which require these registers to be preserved across power
* management operations must implement appropriate suspend/resume handlers
* to save and restore them.
*/

Quoted 7 lines

Quoted 2 lines
Is the header file a good place for this, or is there some other better place to put it?
I tend to suggest that the header file is the right place, because that's where the interface is defined. Other people argue that its more likely to be seen in the implementation (fiq.c). So I'm tempted to say both, but lets go with fiq.h for the time being.

OK -- the {get,set}_fiq_regs patch is currently in your patch system.

If you have no objection, I'll submit a patch adding the above text to apply on top of the other patch (or, if possible, orthogonally).

---Dave

Quoted 6 lines

Quoted 3 lines
That argument may apply to a ton of state in the Secure World, not just the FIQ registers
It very much does, and I know OMAP has some kind of SMC call to handle this.

Russell King - ARM Linux

2011-05-23 16:11 UTC #37 lore raw

Quoted 29 lines · Dave Martin

Quoted 11 lines · Russell King - ARM Linux
Quoted 9 lines · Dave Martin
Quoted 3 lines · Russell King - ARM Linux
Out of the list you mention above, the only thing which isn't saved are the FIQ registers, and that's a problem for S2RAM too, and should be done by arch/arm/kernel/fiq.c hooking into the suspend paths.
The alternative view is that the driver using FIQ owns the state in the FIQ mode registers and is therefore responsible for saving and restoring it across suspend/resume. If so, then any additional globally implemented state save/restore of the FIQ mode state is unnecessary.
Quoted 4 lines
This seems to be most sensible - if the FIQ is being used as a software-DMA, then the hardware side of that needs to be cleanly shutdown (eg, waiting for the DMA to complete before proceeding) to ensure no loss of data. That can't happen from within the FIQ code.
OK. Frank suggested putting comments to this effect in <asm/fiq.h>.
I think something along these lines might be appropriate:
/*
* The FIQ mode registers are not magically preserved across suspend/resume. *
* Drivers which require these registers to be preserved across power
* management operations must implement appropriate suspend/resume handlers
* to save and restore them.
*/

Quoted 2 lines

Is the header file a good place for this, or is there some other better place to put it?

Quoted 2 lines

That argument may apply to a ton of state in the Secure World, not just the FIQ registers

It very much does, and I know OMAP has some kind of SMC call to handle this.

Dave Martin

2011-05-23 11:16 UTC #38 lore raw

Quoted 11 lines · Russell King - ARM Linux

Quoted 9 lines · Dave Martin
Quoted 3 lines · Russell King - ARM Linux
Out of the list you mention above, the only thing which isn't saved are the FIQ registers, and that's a problem for S2RAM too, and should be done by arch/arm/kernel/fiq.c hooking into the suspend paths.
The alternative view is that the driver using FIQ owns the state in the FIQ mode registers and is therefore responsible for saving and restoring it across suspend/resume. If so, then any additional globally implemented state save/restore of the FIQ mode state is unnecessary.

Quoted 4 lines

This seems to be most sensible - if the FIQ is being used as a software-DMA, then the hardware side of that needs to be cleanly shutdown (eg, waiting for the DMA to complete before proceeding) to ensure no loss of data. That can't happen from within the FIQ code.

OK. Frank suggested putting comments to this effect in <asm/fiq.h>.

I think something along these lines might be appropriate:

Is the header file a good place for this, or is there some other better place to put it?

Quoted 4 lines

I also wonder about issues of secure/non-secure stuff here too - what that means is that if we have a driver using FIQ mode, then we have FIQ state to save, but if not there's probably no point (or even any way) to save that state ourselves anyway.

That argument may apply to a ton of state in the Secure World, not just the FIQ registers

Either way, Linux doesn't need to be doing anything special except for calling those hooks, just as for any other driver.

Cheers
---Dave

Russell King - ARM Linux

2011-05-23 10:12 UTC #39 lore raw

Quoted 9 lines · Dave Martin

Quoted 3 lines · Russell King - ARM Linux
Out of the list you mention above, the only thing which isn't saved are the FIQ registers, and that's a problem for S2RAM too, and should be done by arch/arm/kernel/fiq.c hooking into the suspend paths.
The alternative view is that the driver using FIQ owns the state in the FIQ mode registers and is therefore responsible for saving and restoring it across suspend/resume. If so, then any additional globally implemented state save/restore of the FIQ mode state is unnecessary.

Dave Martin

2011-05-23 10:01 UTC #40 lore raw

Quoted 10 lines · Russell King - ARM Linux

Quoted 4 lines · Dave Martin
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 (?))
There is absolutely no need to save r13 for _any_ of the abort modes. As we have to set them up at boot time to fixed values, we keep that code around, and in the resume paths we re-execute that initialization code. cpu_init().

That seemed a sensible possibility, but I'm still not familiar with some of the details. It was, as I suspected, a dumb question from me...

Quoted 3 lines

Out of the list you mention above, the only thing which isn't saved are the FIQ registers, and that's a problem for S2RAM too, and should be done by arch/arm/kernel/fiq.c hooking into the suspend paths.

Do you have an opinion on which is the better model?

Cheers
---Dave

Russell King - ARM Linux

2011-05-20 18:05 UTC #41 lore raw

Quoted 4 lines · Dave Martin

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 (?))

Frank Hofmann

author

2011-05-24 13:27 UTC #42 lore raw

[RFC] ARM hibernation, cpu-type-specific code

http://lists.arm.linux.org.uk/lurker/message/20110211.161754.78371a42.en.html

Hi,

Regarding ARM hibernation / suspend-to-disk, now that the glue part of the code has "settled" it's probably time to think about the gory low-level things, what's been delegated to the __save/__restore_processor_state() hooks.

Russell King did the cpu_suspend / cpu_resume cleanup in Feb/2011,

which cleansed the mach-.../ sleep code in favour of moving these things into processor type support hooks in arch/arm/mm/proc-*.S instead.

The key thing here was the addition of cpu_do_suspend/cpu_do_resume hooks which save/restore state to/from a caller-supplied buffer, and then return to their callers. I.e. pure auxilliary, "no side effects" (other than, for resume, of course, what's desired).
The power-down / enter-low-power parts were separated from that.

Quoted 1 line

From my point of view, all platforms that use these should be able to do

the cpu-specific parts of hibernation via a simple addition, say, to arch/arm/kernel/sleep.S (that's already using <asm/glue-proc.h> so cpu_do_... exists), of something like:

#if defined(CONFIG_HAS_ARM_GENERIC_CPU_SUSPEND_SUPPORT) && defined(CONFIG_HIBERNATION) ENTRY(__save_processor_state)
b cpu_do_suspend
ENDPROC(__save_processor_state)

ENTRY(__restore_processor_state)
stmfd sp!,{r4-r11,lr}
bl cpu_do_resume
ldmfd sp!,{r4-r11,pc}
ENDPROC(__restore_processor_state)
#endif

and "select HAS_ARM_GENERIC_CPU_SUSPEND_SUPPORT if CONFIG_PM" for the CPU architectures that compile suitable mm/proc-*.S in.

That would do because the cpu_do_suspend/resume hooks perform the same CP reg save/restore operations as the "illustration patches" I've posted for OMAP3 and samsung 64xx (in fact, the latter is one of the platforms changed via Russell's patch series).

Quoted 1 line

From my point of view, this assumes the following:

- cpu_do_suspend() takes one argument, a virtual address of the buffer to save the state into. It is a "well-behaved" EABI-compliant func that preserves nonvolatile regs.
It returns to LR.

- cpu_do_resume() assumes it can clobber registers; it takes one argument, the address (not necessarily virtual) of the buffer to restore state from.
The function does not depend on the MMU being OFF (could be called with the MMU on, it'll "just clobber the state").
It also returns to LR.

Do these assumptions hold ?

If so, then would it be ok to create a patch for 2.6.39 onwards (baselined against ARM devel-stable), along the lines just mentioned ?

How would one best test something as sweeping as that ?

The above suggestion still leaves a hole out for machine-specific implementations - unselect the GENERIC config and implement the hook yourself. Seems desireable ?

Regarding pre-2.6.38 kernels, are there any plans to backport the cpu_suspend changes to any -stable series ?

Thanks in advance for comments / suggestions / corrections, FrankH.

Dave Martin

2011-05-20 11:37 UTC #43 lore raw