linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] Current status, suspend-to-disk support on ARM
@ 2011-04-04 14:47 Frank Hofmann
  2011-04-04 15:06 ` Frank Hofmann
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Frank Hofmann @ 2011-04-04 14:47 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

some status on making hibernation (suspend-to-disk) work on ARM.

I've simplified an updated patch set again, to make the pieces obvious. 
The attached patch set also cleanly compiles when adding TuxOnIce to it.


Pls. don't take this as a formal patch submission; this is discussion 
material and hence not currently based on any specific kernel revision.


The code essentially splits into two - a generic bit supplying the glue 
code that the framework needs, and SoC-specific code to do the core state 
suspend/resume.

The generic bits do two things:

 	* implement some glue the suspend-to-disk framework requires:

 		- pfn_is_nosave
 		- save/restore_processor_state
 		- swsusp_arch_suspend/resume entrypoints

 	* ARM assembly for the "guts" of swsusp_arch_suspend/resume

 		- save/restore current regset, CPSR and svc stack/lr
 		- page restore loop to copy the pageset back
 		- redirect to SoC-specific code for core suspend/resume

Hopefully what's in there is actually agnostic enough to qualify as "ARM 
generic". This stuff is quite clean by now.

There's one ugly thing in this set - I've changed a generic kernel header, 
<linux/suspend.h> to #define save/restore_processor_state() on ARM so that 
it only does preempt_disable/enable(). It's surprising that this isn't the 
default behaviour; all platforms need swsusp_arch_suspend/resume anyway so 
why force the existance of _two_ arch-specific hooks ?



In addition to this glue, one needs:

 	* a SoC-dependent __save/__restore_processor_state.
           There's two examples attached for that, OMAP3 and Samsung 6450.

This bit is the "hacky" part of the patch; on ARM, the platform code is 
blissfully unaware of suspend-to-disk while suspend-to-ram is partially 
very complex code.

The shown diffs hook into the "inner guts" of existing suspend-to-ram on 
ARM, and while that looks like it does a large part of the job, there's 
surely a better way.

I've supplied those as merely illustrative... to show that the inline 
assembly orgies from previous patches are just unnecessarily duplicating 
already-existing functionality. The way this is reused here might not be 
the perfect way - intent only was to show how much code re-use is 
actually possible in the area.

Whoever wishes can instead substitute the __save/__restore_processor_state 
inline assembly orgies from previously posted patches here - it all hooks 
in very cleanly.



To test / extend this code on an ARM architecture other than the two 
shown, one can start with the generic bits and simply make NOP-impls for 
the SoC-specific bits; that should at the very least allow a snapshot to 
be created, and so validate that device tree quiesce/freeze resume/thaw 
work correctly. Resume from such an image wouldn't work, though. At least 
the MMU state is absolutely critical.




Re, how (if at all) to get this towards mainline:

It looks like the various ARM boards have _very_ different views towards 
what suspend_ops.enter() will have to do; some are quite simple there, 
others are exceedingly complex.

One would ultimately hope for as much code sharing between hibernation and 
suspend-to-mem as possible, for sure, but the current code isn't aware of 
this; in many cases, saving/restoring state is done all over the place in 
the "arch framework" bits of suspend_ops.enter(), before the actual CPU 
core state is saved/resumed. ARM is heavily biased towards optimizing the 
hell out of suspend-to-mem, and a "grand central store" for system state 
isn't really there, all boards have their own strategy for this, and their 
own assumptions on what the CPU state is when resumed from RAM. This is 
even harder to do for "secure" SoCs, where part of the functionality is 
handled by (non-kernel) internal ROM code.


Russell King has recently done a change to the CPU suspend core code to 
make that use a "more generic" interface (have them all dump state into a 
caller-supplied buffer, at the least). The attached patches aren't yet 
fully aware of that because of the need _not_ to suspend (but return after 
state save) when called through the hibernation code. My brain hasn't yet 
grown big enough to solve this well ...



Re, success with this patch: On OMAP3, I've got issues; the console 
doesn't properly suspend/resume and using no_console_suspend keeps the 
messages but looses console input after resume. Also, graphics doesn't 
come back, and successive hibernation attempts cause crashes in the USB 
stack. It works much better on the Samsung 64xx boards, for me at least. 
But then, quite a few people reported success with the older patches on 
OMAP, hence I wonder, who's got it "fully working" ?



Any comments ? What can be improved ?


Have fun,
FrankH.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hibernate-core-04Apr2011.patch
Type: text/x-diff
Size: 9586 bytes
Desc: 
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110404/10ee57ea/attachment-0001.bin>
-------------- next part --------------
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"
diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
index ea4e498..fd48417 100644
--- a/arch/arm/mach-omap2/sleep34xx.S
+++ b/arch/arm/mach-omap2/sleep34xx.S
@@ -328,6 +328,17 @@ restore:
 	.word	0xE1600071		@ call SMI monitor (smi #1)
 #endif
 	b	logic_l1_restore
+#ifdef CONFIG_HIBERNATION
+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
+
 l2_inv_api_params:
 	.word   0x1, 0x00
 l2_inv_gp:
@@ -358,6 +369,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 +445,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 +487,16 @@ 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)
+#endif
+
 save_context_wfi:
 	/*b	save_context_wfi*/	@ enable to debug save code
 	mov	r8, r0 /* Store SDRAM address in r8 */
@@ -545,6 +571,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# */
-------------- next part --------------
diff --git a/arch/arm/plat-s5p/sleep.S b/arch/arm/plat-s5p/sleep.S
index 2cdae4a..fd2b0a1 100644
--- a/arch/arm/plat-s5p/sleep.S
+++ b/arch/arm/plat-s5p/sleep.S
@@ -48,10 +48,17 @@
 	 *
 	 * entry:
 	 *	r0 = save address (virtual addr of s3c_sleep_save_phys)
-	*/
+	 *	r1 (_internal_ only) = CPU sleep trampoline (if any)
+	 */
 
-ENTRY(s3c_cpu_save)
+ENTRY(__save_processor_state)
+	mov	r1, #0
+	b	.Ls3c_cpu_save_internal
+ENDPROC(__save_processor_state)
 
+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 +74,13 @@ ENTRY(s3c_cpu_save)
 
 	stmia	r0, { r3 - r13 }
 
+	mov	r4, r1
 	@@ write our state back to RAM
 	bl	s3c_pm_cb_flushcache
 
+	mov	r0, r4
+	ldmeqfd	sp!, { r3 - r12, pc }	@ if there was no trampoline, return
 	@@ jump to final code to send system to sleep
-	ldr	r0, =pm_cpu_sleep
 	@@ldr	pc, [ r0 ]
 	ldr	r0, [ r0 ]
 	mov	pc, r0
@@ -86,9 +95,19 @@ resume_with_mmu:
 	str	r12, [r4]
 
 	ldmfd	sp!, { r3 - r12, pc }
+ENDPROC(s3c_cpu_save)
+
+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)
 
 	.ltorg
 
+
 	@@ the next bits sit in the .data segment, even though they
 	@@ happen to be code... the s5pv210_sleep_save_phys needs to be
 	@@ accessed by the resume code before it can restore the MMU.
@@ -131,6 +150,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 +172,9 @@ ENTRY(s3c_cpu_resume)
 	mcr	p15, 0, r12, c10, c2, 0	@ write PRRR
 	mcr	p15, 0, r3, c10, c2, 1	@ write NMRR
 
+	cmp	r1, #0
+	bne	0f			@ only do MMU phys init
+					@ not called by __restore_processor_state
 	/* calculate first section address into r8 */
 	mov	r4, r6
 	ldr	r5, =0x3fff
@@ -175,6 +198,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 +207,7 @@ ENTRY(s3c_cpu_resume)
         nop
         nop					@ second-to-last before mmu
 
+	ldmnefd	sp!, { r3 - r12, pc }
 	mov	pc, r2				@ go back to virtual address
 
 	.ltorg

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH] Current status, suspend-to-disk support on ARM
  2011-04-04 14:47 [RFC PATCH] Current status, suspend-to-disk support on ARM Frank Hofmann
@ 2011-04-04 15:06 ` Frank Hofmann
  2011-04-05 12:22 ` [RFC PATCH v2] " Frank Hofmann
  2011-04-12  5:59 ` [RFC PATCH] " Pavel Machek
  2 siblings, 0 replies; 13+ messages in thread
From: Frank Hofmann @ 2011-04-04 15:06 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

sorry for double posting, the mailing list processor tells me "a non-text 
attachment was scrubbed"; just for the critical piece ...

Hopefully this one goes through. It's only "git diff" output; what could 
it have tripped over ?

Thanks,
FrankH.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hibernate-core-04Apr2011.patch
Type: text/x-diff
Size: 9586 bytes
Desc: 
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110404/bae266a3/attachment.bin>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [RFC PATCH v2] Current status, suspend-to-disk support on ARM
  2011-04-04 14:47 [RFC PATCH] Current status, suspend-to-disk support on ARM Frank Hofmann
  2011-04-04 15:06 ` Frank Hofmann
@ 2011-04-05 12:22 ` Frank Hofmann
  2011-04-12  5:59 ` [RFC PATCH] " Pavel Machek
  2 siblings, 0 replies; 13+ messages in thread
From: Frank Hofmann @ 2011-04-05 12:22 UTC (permalink / raw)
  To: linux-arm-kernel


Updated version of yesterday's core ARM hibernation code bits.

I've found that the use of cpu_switch_mm() breaks resume on OMAP3; leaving 
it out for now. It's unnecessary for direct resume (via kernel command 
line or TuxOnIce), only needed for uswsusp.

Also, me bad, apologies for posting refactored code one too early ... I've 
tried many ways of getting the actual swsusp_arch_suspend/resume 
entrypoints to be C code (and only the absolute essentials assembly), but 
for the case of swsusp_arch_suspend, assembly entry is mandatory because 
the framework relies on resuming into to the caller (i.e. no change can be 
made to the return address / LR reg before storing it, that makes it 
impossible to use a C wrapper function).

Thanks for any thoughts, feedback, reviews on this.

FrankH.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hibernate-core-05Apr2011.patch
Type: text/x-diff
Size: 9758 bytes
Desc: 
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110405/ed330412/attachment-0001.bin>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [RFC PATCH] Current status, suspend-to-disk support on ARM
  2011-04-04 14:47 [RFC PATCH] Current status, suspend-to-disk support on ARM Frank Hofmann
  2011-04-04 15:06 ` Frank Hofmann
  2011-04-05 12:22 ` [RFC PATCH v2] " Frank Hofmann
@ 2011-04-12  5:59 ` Pavel Machek
  2011-04-12 16:30   ` Frank Hofmann
  2 siblings, 1 reply; 13+ messages in thread
From: Pavel Machek @ 2011-04-12  5:59 UTC (permalink / raw)
  To: linux-arm-kernel

Hi!

> There's one ugly thing in this set - I've changed a generic kernel
> header, <linux/suspend.h> to #define save/restore_processor_state()
> on ARM so that it only does preempt_disable/enable(). It's
> surprising that this isn't the default behaviour; all platforms need
> swsusp_arch_suspend/resume anyway so why force the existance of
> _two_ arch-specific hooks ?

Can you submit separate patch cleaning it up?

> @@ -195,6 +195,14 @@ config VECTORS_BASE
>  	help
>  	  The base address of exception vectors.
>  
> +config ARCH_HIBERNATION_POSSIBLE
> +	bool
> +	help
> +	  If the machine architecture supports suspend-to-disk
> +	  it should select this automatically for you.
> +	  Otherwise, say 'Y' at your own peril.
> +

Good for debugging, but not good for mainline.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [RFC PATCH] Current status, suspend-to-disk support on ARM
  2011-04-12  5:59 ` [RFC PATCH] " Pavel Machek
@ 2011-04-12 16:30   ` Frank Hofmann
  2011-04-12 18:32     ` Pavel Machek
  0 siblings, 1 reply; 13+ messages in thread
From: Frank Hofmann @ 2011-04-12 16:30 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, 12 Apr 2011, Pavel Machek wrote:

> Hi!
>
>> There's one ugly thing in this set - I've changed a generic kernel
>> header, <linux/suspend.h> to #define save/restore_processor_state()
>> on ARM so that it only does preempt_disable/enable(). It's
>> surprising that this isn't the default behaviour; all platforms need
>> swsusp_arch_suspend/resume anyway so why force the existance of
>> _two_ arch-specific hooks ?
>
> Can you submit separate patch cleaning it up?

How about the attached ?

It's for discussion, and hence not tested (admittedly, I need an x86 test 
system ...).

The diff is against RMK's devel-stable tree, as of commit 
5f183860d5007ec76ea36bfa6c36d66e37f0dbcf


There's a few hurdles here:

a) who knows all assembly calling conventions for all architectures
    supported by linux ?

    This applies to SH and S390; save/restore_processor_state on those
    are primitive and should be inlined within swsusp_arch_suspend/resume,
    just like the attached patch does for the powerpc variants.

    I've done a bounce call within S390 but don't know enough about SH3
    for arch/sh/kernel/cpu/sh3/swsusp.S - i.e. add a call in assembly to
    init_cpu(current), and then ditch save_processor_state.

b) some architectures do things _beyond_ register/state saving in those
    two functions, and hence rely on them being called paired.

c) CONFIG_KEXEC_JUMP (ab-)uses them. I don't know that subsystem at all,
    but this sort of re-use means the symbols can't just be ditched.
    They become arch-dependent though, no non-arch/ code calls them
    anymore.

What I've attached might break SH3 swsusp, as save_processor_state 
is no longer called there (it stores FPU state).




I've also been wondering:
The comments in kernel/power/hibernate.c mention the need to get preempt 
counts "right" as major motivator to call save/restore_processor_state.
effect" of save/restore_processor_state).

But then on all architectures in kernels as far back as 2.6.32 it doesn't 
look like save/restore_processor state are actually adjusting the preempt 
count; nowhere do they call preempt_enable/disable.

What is and what is not necessary, really ?


Finally, on things like e.g. the floating point context saves: Wouldn't 
this happen as part of freezing processes (in the early stages of 
suspend), and/or as part of device quiesce ?
I wonder why e.g. powerpc does it explicitly; not doing so on ARM doesn't 
seem to cause problems.



>
>> @@ -195,6 +195,14 @@ config VECTORS_BASE
>>  	help
>>  	  The base address of exception vectors.
>>
>> +config ARCH_HIBERNATION_POSSIBLE
>> +	bool
>> +	help
>> +	  If the machine architecture supports suspend-to-disk
>> +	  it should select this automatically for you.
>> +	  Otherwise, say 'Y' at your own peril.
>> +
>
> Good for debugging, but not good for mainline.

How would this be done better ?

FrankH.

>
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: remove-save_processor_state.patch
Type: text/x-diff
Size: 10417 bytes
Desc: 
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110412/6de02f2b/attachment.bin>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [RFC PATCH] Current status, suspend-to-disk support on ARM
  2011-04-12 16:30   ` Frank Hofmann
@ 2011-04-12 18:32     ` Pavel Machek
  2011-04-13 13:36       ` Frank Hofmann
  0 siblings, 1 reply; 13+ messages in thread
From: Pavel Machek @ 2011-04-12 18:32 UTC (permalink / raw)
  To: linux-arm-kernel

Hi!

> >>There's one ugly thing in this set - I've changed a generic kernel
> >>header, <linux/suspend.h> to #define save/restore_processor_state()
> >>on ARM so that it only does preempt_disable/enable(). It's
> >>surprising that this isn't the default behaviour; all platforms need
> >>swsusp_arch_suspend/resume anyway so why force the existance of
> >>_two_ arch-specific hooks ?
> >
> >Can you submit separate patch cleaning it up?
> 
> How about the attached ?
> 
> It's for discussion, and hence not tested (admittedly, I need an x86
> test system ...).
...

> I've also been wondering:
> The comments in kernel/power/hibernate.c mention the need to get
> preempt counts "right" as major motivator to call
> save/restore_processor_state.
> effect" of save/restore_processor_state).
> 
> But then on all architectures in kernels as far back as 2.6.32 it
> doesn't look like save/restore_processor state are actually
> adjusting the preempt count; nowhere do they call
> preempt_enable/disable.

You might need to dig a bit more. IIRC it manipulated FPU or something
long time ago.

> Finally, on things like e.g. the floating point context saves:
> Wouldn't this happen as part of freezing processes (in the early
> stages of suspend), and/or as part of device quiesce ?

Are you sure? I thought we have (had?) concept of lazy FPU
saving... and occassionaly kernel uses FPU too (with some
precautions).

> >>@@ -195,6 +195,14 @@ config VECTORS_BASE
> >> 	help
> >> 	  The base address of exception vectors.
> >>
> >>+config ARCH_HIBERNATION_POSSIBLE
> >>+	bool
> >>+	help
> >>+	  If the machine architecture supports suspend-to-disk
> >>+	  it should select this automatically for you.
> >>+	  Otherwise, say 'Y' at your own peril.
> >>+
> >
> >Good for debugging, but not good for mainline.
> 
> How would this be done better ?

Just set the ARCH_HIBERNATION_POSSIBLE=y in the ARM code now that ARM
can do it. No need to ask user.

> @@ -412,8 +413,7 @@ static int resume_target_kernel(bool platform_mode)
>  	if (error)
>  		goto Enable_irqs;
>  
> -	/* We'll ignore saved state, but this gets preempt count (etc) right */
> -	save_processor_state();
> +	preempt_disable();
>  	error = restore_highmem();
>  	if (!error) {
>  		error = swsusp_arch_resume();

Yep. save_processor_state() on x86 does kernel_fpu_begin(), and than
one needs to be balanced IIRC.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [RFC PATCH] Current status, suspend-to-disk support on ARM
  2011-04-12 18:32     ` Pavel Machek
@ 2011-04-13 13:36       ` Frank Hofmann
  2011-04-13 21:38         ` Rafael J. Wysocki
  0 siblings, 1 reply; 13+ messages in thread
From: Frank Hofmann @ 2011-04-13 13:36 UTC (permalink / raw)
  To: linux-arm-kernel



On Tue, 12 Apr 2011, Pavel Machek wrote:

> Hi!
[ ... ]
>> But then on all architectures in kernels as far back as 2.6.32 it
>> doesn't look like save/restore_processor state are actually
>> adjusting the preempt count; nowhere do they call
>> preempt_enable/disable.
>
> You might need to dig a bit more. IIRC it manipulated FPU or something
> long time ago.

Actually, it's the other way round - kernel_fpu_begin/end are users of 
preempt_disable/enable, quote Documentation/preempt-locking.txt:

 	Note, some FPU functions are already explicitly preempt safe.  For example,
 	kernel_fpu_begin and kernel_fpu_end will disable and enable preemption.
 	However, math_state_restore must be called with preemption disabled.

But kernel_fpu_begin/end functions are x86-specific. At best, that makes 
the comment in hibernate.c (which refers to "needing" preempt_dis/enable) 
misleading.

x86, in this, is an odd one out, though, in ; On ARM, for example, saving 
the FP context happens through a PM notifier.

In addition, it seems that the code (at least in hibernate.c, 
create_image) does anyway:

 	- go single-cpu (disables all but current)
 	- switch interrupts off (local_irq_disable)

before going into save_processor_state - wouldn't that mean all conditions 
for nonpreemptable (or all conditions for a stable FP state) are already 
met by that time ?



>
>> Finally, on things like e.g. the floating point context saves:
>> Wouldn't this happen as part of freezing processes (in the early
>> stages of suspend), and/or as part of device quiesce ?
>
> Are you sure? I thought we have (had?) concept of lazy FPU
> saving... and occassionaly kernel uses FPU too (with some
> precautions).

speaking in particular for ARM, I'd deflect the answer to:

 	http://www.spinics.net/lists/linux-pm/msg22839.html

It's lazy but it does happen ... without any specific need to have the 
"bracketing" that save/restore_processor_state are doing.


Food for thought:

I mean, the code in hibernate.c really looks like this (quote):


static int create_image(int platform_mode)
{
         int error;

         error = arch_prepare_suspend();				<===== platform hook 1
[ ... ]
         error = dpm_suspend_noirq(PMSG_FREEZE);			<===== platform hook 2
[ ... ]
         error = platform_pre_snapshot(platform_mode);		<===== platform hook 3
[ ... ]
         error = disable_nonboot_cpus();				<===== platform hook 4
[ ... ]
         error = sysdev_suspend(PMSG_FREEZE);			<===== ...
         if (!error)
                 error = syscore_suspend();			<===== ...
[ ... ]
         in_suspend = 1;
         save_processor_state();					<===== platform hook N-1
         error = swsusp_arch_suspend();				<===== platform hook N
[ ... ]
         restore_processor_state();				<===== platform hook N+1
[ ... ]

And that's _without_ the platform hibernation ops hooks code in 
hibernate(), which calls this one.

This code is full of places where to plug save/restore code (or any sort 
of "bracketing" begin/finish hooks) into.
It's surprising that with all these hooks existing already, "bracketing" 
of swsusp_arch_suspend() by save/restore_processor state is so sacrosanct.

These are _three_ arch-dependent hooks in three consecutive lines of code.

If a platform requires the bracketing, why can it not do that part in 
syscore_suspend() ? Or why not simply embed it in swsusp_arch_suspend() ?

As said, wrt. to save/restore_processor_state, x86 is definitely the odd 
one out (and not a good example) with the monstrosity of code; all other 
architectures either have trivial or even completely empty code for
save/restore_processor_state().

Should we really force everyone to provide an (as good as) empty hook just 
because x86@one point had chosen to have it ?

>
>>>> @@ -195,6 +195,14 @@ config VECTORS_BASE
>>>> 	help
>>>> 	  The base address of exception vectors.
>>>>
>>>> +config ARCH_HIBERNATION_POSSIBLE
>>>> +	bool
>>>> +	help
>>>> +	  If the machine architecture supports suspend-to-disk
>>>> +	  it should select this automatically for you.
>>>> +	  Otherwise, say 'Y' at your own peril.
>>>> +
>>>
>>> Good for debugging, but not good for mainline.
>>
>> How would this be done better ?
>
> Just set the ARCH_HIBERNATION_POSSIBLE=y in the ARM code now that ARM
> can do it. No need to ask user.

I.e. preferrably:

 	config ARCH_HIBERNATION_POSSIBLE
 		def_bool n

plus the "select" within the actual platform config subsection for those 
that have it.

To stress the point: "ARM can do it" would be an exaggeration even if 
those changes went in, because only some ARM types will have it.

The situation on ARM is identical to e.g. SuperH, see arch/sh/Kconfig,

config SUPERH32
         def_bool ARCH = "sh"
[ ... various ... ]
         select ARCH_HIBERNATION_POSSIBLE if MMU

config ARCH_HIBERNATION_POSSIBLE
         def_bool n


>
>> @@ -412,8 +413,7 @@ static int resume_target_kernel(bool platform_mode)
>>  	if (error)
>>  		goto Enable_irqs;
>>
>> -	/* We'll ignore saved state, but this gets preempt count (etc) right */
>> -	save_processor_state();
>> +	preempt_disable();
>>  	error = restore_highmem();
>>  	if (!error) {
>>  		error = swsusp_arch_resume();
>
> Yep. save_processor_state() on x86 does kernel_fpu_begin(), and than
> one needs to be balanced IIRC.


The needs of x86 force generic kernel changes ;-)

FrankH.

>
> 									Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [RFC PATCH] Current status, suspend-to-disk support on ARM
  2011-04-13 13:36       ` Frank Hofmann
@ 2011-04-13 21:38         ` Rafael J. Wysocki
  2011-04-14 17:06           ` Frank Hofmann
  0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2011-04-13 21:38 UTC (permalink / raw)
  To: linux-arm-kernel

On Wednesday, April 13, 2011, Frank Hofmann wrote:
> 
> On Tue, 12 Apr 2011, Pavel Machek wrote:
> 
> > Hi!
> [ ... ]
> >> But then on all architectures in kernels as far back as 2.6.32 it
> >> doesn't look like save/restore_processor state are actually
> >> adjusting the preempt count; nowhere do they call
> >> preempt_enable/disable.
> >
> > You might need to dig a bit more. IIRC it manipulated FPU or something
> > long time ago.
> 
> Actually, it's the other way round - kernel_fpu_begin/end are users of 
> preempt_disable/enable, quote Documentation/preempt-locking.txt:
> 
>  	Note, some FPU functions are already explicitly preempt safe.  For example,
>  	kernel_fpu_begin and kernel_fpu_end will disable and enable preemption.
>  	However, math_state_restore must be called with preemption disabled.
> 
> But kernel_fpu_begin/end functions are x86-specific. At best, that makes 
> the comment in hibernate.c (which refers to "needing" preempt_dis/enable) 
> misleading.

This comment is outright wrong in fact, sorry about that.  Evidently,
restore_processor_state() is called right after the "restore control flow
magically appears here" comment and it _does_ use the saved state.

I'll remove that comment.

> x86, in this, is an odd one out, though, in ; On ARM, for example, saving 
> the FP context happens through a PM notifier.
> 
> In addition, it seems that the code (at least in hibernate.c, 
> create_image) does anyway:
> 
>  	- go single-cpu (disables all but current)
>  	- switch interrupts off (local_irq_disable)
> 
> before going into save_processor_state - wouldn't that mean all conditions 
> for nonpreemptable (or all conditions for a stable FP state) are already 
> met by that time ?

Yes, it does.

> >> Finally, on things like e.g. the floating point context saves:
> >> Wouldn't this happen as part of freezing processes (in the early
> >> stages of suspend), and/or as part of device quiesce ?
> >
> > Are you sure? I thought we have (had?) concept of lazy FPU
> > saving... and occassionaly kernel uses FPU too (with some
> > precautions).
> 
> speaking in particular for ARM, I'd deflect the answer to:
> 
>  	http://www.spinics.net/lists/linux-pm/msg22839.html
> 
> It's lazy but it does happen ... without any specific need to have the 
> "bracketing" that save/restore_processor_state are doing.

save/restore_processor_state() are arch-specific and do whatever is necessary
or useful for the given architecture.  The saving/restoring of the FPU state
at those points happens to be useful for x86 IIRC.

> Food for thought:
> 
> I mean, the code in hibernate.c really looks like this (quote):
> 
> 
> static int create_image(int platform_mode)
> {
>          int error;
> 
>          error = arch_prepare_suspend();				<===== platform hook 1

Architecture hook rather and it would have been one if there had been any
architecture actually providing anything different from a NOP.  IOW, to be
removed (forgot about it last time I did a code cleanup).

> [ ... ]
>          error = dpm_suspend_noirq(PMSG_FREEZE);			<===== platform hook 2

Not really.  This is a device core callback for the last stage of device freeze.

> [ ... ]
>          error = platform_pre_snapshot(platform_mode);		<===== platform hook 3

This is a platform hook.

> [ ... ]
>          error = disable_nonboot_cpus();				<===== platform hook 4

Not a platform hook.  This one disables the nonboot CPUs using hotplug and this
mechanism is supposed to work that way on all platforms.

> [ ... ]
>          error = sysdev_suspend(PMSG_FREEZE);			<===== ...
>          if (!error)
>                  error = syscore_suspend();			<===== ...
> [ ... ]

These suspend things that are "below" devices (like interrupt controllers
and such stuff) and by no means are platform hooks.

>          in_suspend = 1;
>          save_processor_state();					<===== platform hook N-1

This is an architecture hook for saving the state of the boot CPU.

>          error = swsusp_arch_suspend();				<===== platform hook N

This one is needed on x86 so that we can point the CPU to the original
page tables after we've restored the target kernel from the image.

> [ ... ]
>          restore_processor_state();				<===== platform hook N+1
> [ ... ]
> 
> And that's _without_ the platform hibernation ops hooks code in 
> hibernate(), which calls this one.

Actually not without, platform_pre_snapshot() is one of those.

> This code is full of places where to plug save/restore code (or any sort 
> of "bracketing" begin/finish hooks) into.
> It's surprising that with all these hooks existing already, "bracketing" 
> of swsusp_arch_suspend() by save/restore_processor state is so sacrosanct.
> 
> These are _three_ arch-dependent hooks in three consecutive lines of code.
> 
> If a platform requires the bracketing, why can it not do that part in 
> syscore_suspend() ? Or why not simply embed it in swsusp_arch_suspend() ?

save/restore_processor_state() are also used by the x86's suspend to RAM code.
Of course, you can argue that they should be kept x86-specific, but since
those features were originally developed on x86, the code still remains
x86-centric in that respect.

> As said, wrt. to save/restore_processor_state, x86 is definitely the odd 
> one out (and not a good example) with the monstrosity of code; all other 
> architectures either have trivial or even completely empty code for
> save/restore_processor_state().
> 
> Should we really force everyone to provide an (as good as) empty hook just 
> because x86 at one point had chosen to have it ?

I don't see much reason for that, but since none of the guys who implemented
those empty hooks has ever complained and/or attempted to change things, there
has not been much reason to work in that direction. :-)

> >>>> @@ -195,6 +195,14 @@ config VECTORS_BASE
> >>>> 	help
> >>>> 	  The base address of exception vectors.
> >>>>
> >>>> +config ARCH_HIBERNATION_POSSIBLE
> >>>> +	bool
> >>>> +	help
> >>>> +	  If the machine architecture supports suspend-to-disk
> >>>> +	  it should select this automatically for you.
> >>>> +	  Otherwise, say 'Y' at your own peril.
> >>>> +
> >>>
> >>> Good for debugging, but not good for mainline.
> >>
> >> How would this be done better ?
> >
> > Just set the ARCH_HIBERNATION_POSSIBLE=y in the ARM code now that ARM
> > can do it. No need to ask user.
> 
> I.e. preferrably:
> 
>  	config ARCH_HIBERNATION_POSSIBLE
>  		def_bool n

You don't need to use "def_bool n", "bool" alone will do just fine.

> plus the "select" within the actual platform config subsection for those 
> that have it.
> 
> To stress the point: "ARM can do it" would be an exaggeration even if 
> those changes went in, because only some ARM types will have it.
> 
> The situation on ARM is identical to e.g. SuperH, see arch/sh/Kconfig,
> 
> config SUPERH32
>          def_bool ARCH = "sh"
> [ ... various ... ]
>          select ARCH_HIBERNATION_POSSIBLE if MMU
> 
> config ARCH_HIBERNATION_POSSIBLE
>          def_bool n
> 

Yes, something like this.

> >> @@ -412,8 +413,7 @@ static int resume_target_kernel(bool platform_mode)
> >>  	if (error)
> >>  		goto Enable_irqs;
> >>
> >> -	/* We'll ignore saved state, but this gets preempt count (etc) right */
> >> -	save_processor_state();
> >> +	preempt_disable();

Why do you need that preempt_disable() here?

> >>  	error = restore_highmem();
> >>  	if (!error) {
> >>  		error = swsusp_arch_resume();
> >
> > Yep. save_processor_state() on x86 does kernel_fpu_begin(), and than
> > one needs to be balanced IIRC.
> 
> 
> The needs of x86 force generic kernel changes ;-)

Not necessarily.  In fact, implementing that feature on different architectures
is a good opportunity to clean up the code if necessary.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [RFC PATCH] Current status, suspend-to-disk support on ARM
  2011-04-13 21:38         ` Rafael J. Wysocki
@ 2011-04-14 17:06           ` Frank Hofmann
  2011-04-18  5:48             ` [TuxOnIce-devel] " Matt Hsu
  0 siblings, 1 reply; 13+ messages in thread
From: Frank Hofmann @ 2011-04-14 17:06 UTC (permalink / raw)
  To: linux-arm-kernel


Hi Rafael, hi Pavel,


thanks for your comments, this makes history and purpose of the 
save/restore_processor_functions a lot clearer.
Yes, x86 does a lot more in those than other architectures, but even on 
non-x86, they have a purpose even if one decides to do the "core" work 
inside swsusp_arch_suspend/resume().


I'll go back to the drawing board for the ARM patch again for a bit.

It might be useful/necessary to delegate some of the functionality actually 
into save/restore_processor_state, like powerpc does (call flush_thread() 
to store lazy-save state, and maybe, if necessary, do some of the things 
the cpu_idle code on ARM does before going down into cpu_suspend).

John reported a crash on resume (on OMAP4) related to FP state; I've done 
some tracing here to see whether the PM notifiers for storing the VFP/FPU 
state on ARM are actually triggering with the suggested change and they're 
not, which means there's still some work to do here.


The last thing here in my setup that's obviously not correctly suspending 
/ resuming is the OMAP display driver; I'm not the only one with that 
problem, Matt's previous report:

http://blog.gmane.org/gmane.linux.swsusp.devel/month=20101201

has the same thing, "omapdss DISPC error: SYNC_LOST, disabling LCD".


I'll provide an update before Easter.

Thanks for the help so far!
FrankH.



On Wed, 13 Apr 2011, Rafael J. Wysocki wrote:

> On Wednesday, April 13, 2011, Frank Hofmann wrote:
>>
>> On Tue, 12 Apr 2011, Pavel Machek wrote:
>>
>>> Hi!
>> [ ... ]
>>>> But then on all architectures in kernels as far back as 2.6.32 it
>>>> doesn't look like save/restore_processor state are actually
>>>> adjusting the preempt count; nowhere do they call
>>>> preempt_enable/disable.
>>>
>>> You might need to dig a bit more. IIRC it manipulated FPU or something
>>> long time ago.
>>
>> Actually, it's the other way round - kernel_fpu_begin/end are users of
>> preempt_disable/enable, quote Documentation/preempt-locking.txt:
>>
>>  	Note, some FPU functions are already explicitly preempt safe.  For example,
>>  	kernel_fpu_begin and kernel_fpu_end will disable and enable preemption.
>>  	However, math_state_restore must be called with preemption disabled.
>>
>> But kernel_fpu_begin/end functions are x86-specific. At best, that makes
>> the comment in hibernate.c (which refers to "needing" preempt_dis/enable)
>> misleading.
>
> This comment is outright wrong in fact, sorry about that.  Evidently,
> restore_processor_state() is called right after the "restore control flow
> magically appears here" comment and it _does_ use the saved state.
>
> I'll remove that comment.
>
>> x86, in this, is an odd one out, though, in ; On ARM, for example, saving
>> the FP context happens through a PM notifier.
>>
>> In addition, it seems that the code (at least in hibernate.c,
>> create_image) does anyway:
>>
>>  	- go single-cpu (disables all but current)
>>  	- switch interrupts off (local_irq_disable)
>>
>> before going into save_processor_state - wouldn't that mean all conditions
>> for nonpreemptable (or all conditions for a stable FP state) are already
>> met by that time ?
>
> Yes, it does.
>
>>>> Finally, on things like e.g. the floating point context saves:
>>>> Wouldn't this happen as part of freezing processes (in the early
>>>> stages of suspend), and/or as part of device quiesce ?
>>>
>>> Are you sure? I thought we have (had?) concept of lazy FPU
>>> saving... and occassionaly kernel uses FPU too (with some
>>> precautions).
>>
>> speaking in particular for ARM, I'd deflect the answer to:
>>
>>  	http://www.spinics.net/lists/linux-pm/msg22839.html
>>
>> It's lazy but it does happen ... without any specific need to have the
>> "bracketing" that save/restore_processor_state are doing.
>
> save/restore_processor_state() are arch-specific and do whatever is necessary
> or useful for the given architecture.  The saving/restoring of the FPU state
> at those points happens to be useful for x86 IIRC.
>
>> Food for thought:
>>
>> I mean, the code in hibernate.c really looks like this (quote):
>>
>>
>> static int create_image(int platform_mode)
>> {
>>          int error;
>>
>>          error = arch_prepare_suspend();				<===== platform hook 1
>
> Architecture hook rather and it would have been one if there had been any
> architecture actually providing anything different from a NOP.  IOW, to be
> removed (forgot about it last time I did a code cleanup).
>
>> [ ... ]
>>          error = dpm_suspend_noirq(PMSG_FREEZE);			<===== platform hook 2
>
> Not really.  This is a device core callback for the last stage of device freeze.
>
>> [ ... ]
>>          error = platform_pre_snapshot(platform_mode);		<===== platform hook 3
>
> This is a platform hook.
>
>> [ ... ]
>>          error = disable_nonboot_cpus();				<===== platform hook 4
>
> Not a platform hook.  This one disables the nonboot CPUs using hotplug and this
> mechanism is supposed to work that way on all platforms.
>
>> [ ... ]
>>          error = sysdev_suspend(PMSG_FREEZE);			<===== ...
>>          if (!error)
>>                  error = syscore_suspend();			<===== ...
>> [ ... ]
>
> These suspend things that are "below" devices (like interrupt controllers
> and such stuff) and by no means are platform hooks.
>
>>          in_suspend = 1;
>>          save_processor_state();					<===== platform hook N-1
>
> This is an architecture hook for saving the state of the boot CPU.
>
>>          error = swsusp_arch_suspend();				<===== platform hook N
>
> This one is needed on x86 so that we can point the CPU to the original
> page tables after we've restored the target kernel from the image.
>
>> [ ... ]
>>          restore_processor_state();				<===== platform hook N+1
>> [ ... ]
>>
>> And that's _without_ the platform hibernation ops hooks code in
>> hibernate(), which calls this one.
>
> Actually not without, platform_pre_snapshot() is one of those.
>
>> This code is full of places where to plug save/restore code (or any sort
>> of "bracketing" begin/finish hooks) into.
>> It's surprising that with all these hooks existing already, "bracketing"
>> of swsusp_arch_suspend() by save/restore_processor state is so sacrosanct.
>>
>> These are _three_ arch-dependent hooks in three consecutive lines of code.
>>
>> If a platform requires the bracketing, why can it not do that part in
>> syscore_suspend() ? Or why not simply embed it in swsusp_arch_suspend() ?
>
> save/restore_processor_state() are also used by the x86's suspend to RAM code.
> Of course, you can argue that they should be kept x86-specific, but since
> those features were originally developed on x86, the code still remains
> x86-centric in that respect.
>
>> As said, wrt. to save/restore_processor_state, x86 is definitely the odd
>> one out (and not a good example) with the monstrosity of code; all other
>> architectures either have trivial or even completely empty code for
>> save/restore_processor_state().
>>
>> Should we really force everyone to provide an (as good as) empty hook just
>> because x86 at one point had chosen to have it ?
>
> I don't see much reason for that, but since none of the guys who implemented
> those empty hooks has ever complained and/or attempted to change things, there
> has not been much reason to work in that direction. :-)
>
>>>>>> @@ -195,6 +195,14 @@ config VECTORS_BASE
>>>>>> 	help
>>>>>> 	  The base address of exception vectors.
>>>>>>
>>>>>> +config ARCH_HIBERNATION_POSSIBLE
>>>>>> +	bool
>>>>>> +	help
>>>>>> +	  If the machine architecture supports suspend-to-disk
>>>>>> +	  it should select this automatically for you.
>>>>>> +	  Otherwise, say 'Y' at your own peril.
>>>>>> +
>>>>>
>>>>> Good for debugging, but not good for mainline.
>>>>
>>>> How would this be done better ?
>>>
>>> Just set the ARCH_HIBERNATION_POSSIBLE=y in the ARM code now that ARM
>>> can do it. No need to ask user.
>>
>> I.e. preferrably:
>>
>>  	config ARCH_HIBERNATION_POSSIBLE
>>  		def_bool n
>
> You don't need to use "def_bool n", "bool" alone will do just fine.
>
>> plus the "select" within the actual platform config subsection for those
>> that have it.
>>
>> To stress the point: "ARM can do it" would be an exaggeration even if
>> those changes went in, because only some ARM types will have it.
>>
>> The situation on ARM is identical to e.g. SuperH, see arch/sh/Kconfig,
>>
>> config SUPERH32
>>          def_bool ARCH = "sh"
>> [ ... various ... ]
>>          select ARCH_HIBERNATION_POSSIBLE if MMU
>>
>> config ARCH_HIBERNATION_POSSIBLE
>>          def_bool n
>>
>
> Yes, something like this.
>
>>>> @@ -412,8 +413,7 @@ static int resume_target_kernel(bool platform_mode)
>>>>  	if (error)
>>>>  		goto Enable_irqs;
>>>>
>>>> -	/* We'll ignore saved state, but this gets preempt count (etc) right */
>>>> -	save_processor_state();
>>>> +	preempt_disable();
>
> Why do you need that preempt_disable() here?
>
>>>>  	error = restore_highmem();
>>>>  	if (!error) {
>>>>  		error = swsusp_arch_resume();
>>>
>>> Yep. save_processor_state() on x86 does kernel_fpu_begin(), and than
>>> one needs to be balanced IIRC.
>>
>>
>> The needs of x86 force generic kernel changes ;-)
>
> Not necessarily.  In fact, implementing that feature on different architectures
> is a good opportunity to clean up the code if necessary.
>
> Thanks,
> Rafael
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [TuxOnIce-devel] [RFC PATCH] Current status, suspend-to-disk support on ARM
  2011-04-14 17:06           ` Frank Hofmann
@ 2011-04-18  5:48             ` Matt Hsu
  2011-04-19 18:13               ` Frank Hofmann
  0 siblings, 1 reply; 13+ messages in thread
From: Matt Hsu @ 2011-04-18  5:48 UTC (permalink / raw)
  To: linux-arm-kernel

>
>
> The last thing here in my setup that's obviously not correctly suspending /
> resuming is the OMAP display driver; I'm not the only one with that problem,
> Matt's previous report:
>
> http://blog.gmane.org/gmane.linux.swsusp.devel/month=20101201
>
> has the same thing, "omapdss DISPC error: SYNC_LOST, disabling LCD".
>
>
>     Hi FrankH,

    Above issue could be resolved by:

   - CONFIG_FB_OMAP2_FORCE_AUTO_UPDATE=y
   - the following patch

diff --git a/drivers/video/omap2/dss/dispc.c
b/drivers/video/omap2/dss/dispc.c
index 8b3ac80..a188732 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -2784,7 +2784,6 @@ static void dispc_error_worker(struct work_struct
*work)
                bool enable = false;

                DSSERR("SYNC_LOST, disabling LCD\n");

                for (i = 0; i < omap_dss_get_num_overlay_managers(); ++i) {
                        struct omap_overlay_manager *mgr;
                        mgr = omap_dss_get_overlay_manager(i);
@@ -2815,6 +2814,9 @@ static void dispc_error_worker(struct work_struct
*work)
                        if (enable)
                                manager->device->enable(manager->device);
                }
+
+               dispc_enable_lcd_out(false);
+               dispc_enable_lcd_out(true);

     Although this patch is somehow nasty, but it work anyway.

- Matt
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110418/b205e44d/attachment.html>

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [TuxOnIce-devel] [RFC PATCH] Current status, suspend-to-disk support on ARM
  2011-04-18  5:48             ` [TuxOnIce-devel] " Matt Hsu
@ 2011-04-19 18:13               ` Frank Hofmann
  2011-04-20  7:14                 ` Matt Hsu
  0 siblings, 1 reply; 13+ messages in thread
From: Frank Hofmann @ 2011-04-19 18:13 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, 18 Apr 2011, Matt Hsu wrote:

>>
>>
>> The last thing here in my setup that's obviously not correctly suspending /
>> resuming is the OMAP display driver; I'm not the only one with that problem,
>> Matt's previous report:
>>
>> http://blog.gmane.org/gmane.linux.swsusp.devel/month=20101201
>>
>> has the same thing, "omapdss DISPC error: SYNC_LOST, disabling LCD".
>>
>>
>>     Hi FrankH,
>
>    Above issue could be resolved by:

Thanks Matt.

It didn't do the trick for me, but then a solution was found; we were 
using the omap dss vrfb component and that didn't resume properly (not 
from ram, nor from disk). We've disabled it and behold, screen comes back 
now ;-)


New patch set once I've done a bit more testing. I've found a way to use 
swapper_pg_dir (i.e. should allow for uswsusp - how would I test that ?) 
that seems to do the job on OMAP as well, but I'd rather validate 
thoroughly.


Also, I'd like to investigate whether ARM will require a set of 
platform_hibernation_ops - Samsung's patch from last December,

http://www.spinics.net/lists/arm-kernel/msg108565.html

seems to indicate that they believe it's necessary at least on some ARM 
hardware. OMAP has similar functions to that, it'd be great to unify.


FrankH.


>
>   - CONFIG_FB_OMAP2_FORCE_AUTO_UPDATE=y
>   - the following patch
>
> diff --git a/drivers/video/omap2/dss/dispc.c
> b/drivers/video/omap2/dss/dispc.c
> index 8b3ac80..a188732 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -2784,7 +2784,6 @@ static void dispc_error_worker(struct work_struct
> *work)
>                bool enable = false;
>
>                DSSERR("SYNC_LOST, disabling LCD\n");
>
>                for (i = 0; i < omap_dss_get_num_overlay_managers(); ++i) {
>                        struct omap_overlay_manager *mgr;
>                        mgr = omap_dss_get_overlay_manager(i);
> @@ -2815,6 +2814,9 @@ static void dispc_error_worker(struct work_struct
> *work)
>                        if (enable)
>                                manager->device->enable(manager->device);
>                }
> +
> +               dispc_enable_lcd_out(false);
> +               dispc_enable_lcd_out(true);
>
>     Although this patch is somehow nasty, but it work anyway.
>
> - Matt
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [TuxOnIce-devel] [RFC PATCH] Current status, suspend-to-disk support on ARM
  2011-04-19 18:13               ` Frank Hofmann
@ 2011-04-20  7:14                 ` Matt Hsu
  2011-04-21  4:55                   ` [linux-pm] " MyungJoo Ham
  0 siblings, 1 reply; 13+ messages in thread
From: Matt Hsu @ 2011-04-20  7:14 UTC (permalink / raw)
  To: linux-arm-kernel

>
> Also, I'd like to investigate whether ARM will require a set of
> platform_hibernation_ops - Samsung's patch from last December,
>
> http://www.spinics.net/lists/arm-kernel/msg108565.html
>

    Hi FrankH,

     The patch seems has connection with this

     www.kernel.org/doc/ols/2010/ols2010-pages-9-18.pdf

    You can see the section 4.3 fast device reactivation.
    It does the similar thing  as STR, but drivers status are kept in
hibernation image instead of main memory.
    I guess this is why this patch comes out.

    In the current device driver model, there are no appropriate handling
specific to
    hibernation_suspend/hibernation_resume ops.

    So they make their own interface/platform drivers.


> seems to indicate that they believe it's necessary at least on some ARM
> hardware. OMAP has similar functions to that, it'd be great to unify.
>
>
> FrankH.
>
>
> --
- Matt
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110420/37618720/attachment-0001.html>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [linux-pm] [TuxOnIce-devel] [RFC PATCH] Current status, suspend-to-disk support on ARM
  2011-04-20  7:14                 ` Matt Hsu
@ 2011-04-21  4:55                   ` MyungJoo Ham
  0 siblings, 0 replies; 13+ messages in thread
From: MyungJoo Ham @ 2011-04-21  4:55 UTC (permalink / raw)
  To: linux-arm-kernel

Hello Matt,

On Wed, Apr 20, 2011 at 4:14 PM, Matt Hsu <matt@0xlab.org> wrote:
>
>>
>> Also, I'd like to investigate whether ARM will require a set of
>> platform_hibernation_ops - Samsung's patch from last December,
>>
>> http://www.spinics.net/lists/arm-kernel/msg108565.html
>
> ??? Hi FrankH,
>
> ???? The patch seems has connection with this
>
> ???? www.kernel.org/doc/ols/2010/ols2010-pages-9-18.pdf

Actually, those patches had no direct connections with that anyway. :)

>
> ? ? You can see the section 4.3 fast device reactivation.
> ? ? It does the similar thing? as STR, but drivers status are kept in
> hibernation image instead of main memory.
> ??? I guess this is why this patch comes out.

Yes, this SoC has registers that require STR/Hibernation to save the
status. They include power, clock, memory-control, gpio, uart related
registers.

>
> ??? In the current device driver model, there are no appropriate handling
> specific to
> ??? hibernation_suspend/hibernation_resume ops.
>
> ??? So they make their own interface/platform drivers.

Correct.

If the interface looks like dev_pm_ops, it would be virtually perfect. :)

>
>>
>> seems to indicate that they believe it's necessary at least on some ARM
>> hardware. OMAP has similar functions to that, it'd be great to unify.
>>
>>
>> FrankH.
>>
> --
> - Matt
>
> _______________________________________________
> linux-pm mailing list
> linux-pm at lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm
>



-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2011-04-21  4:55 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-04-04 14:47 [RFC PATCH] Current status, suspend-to-disk support on ARM Frank Hofmann
2011-04-04 15:06 ` Frank Hofmann
2011-04-05 12:22 ` [RFC PATCH v2] " Frank Hofmann
2011-04-12  5:59 ` [RFC PATCH] " Pavel Machek
2011-04-12 16:30   ` Frank Hofmann
2011-04-12 18:32     ` Pavel Machek
2011-04-13 13:36       ` Frank Hofmann
2011-04-13 21:38         ` Rafael J. Wysocki
2011-04-14 17:06           ` Frank Hofmann
2011-04-18  5:48             ` [TuxOnIce-devel] " Matt Hsu
2011-04-19 18:13               ` Frank Hofmann
2011-04-20  7:14                 ` Matt Hsu
2011-04-21  4:55                   ` [linux-pm] " MyungJoo Ham

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).