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

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

[Frank Hofmann]

Hi,

/me again ...

Sorry that this took a little ... holidays. And work. And distractions...

Anyway, here we go again, basic code to enable hibernation 
(suspend-to-disk) on ARM platforms.

Any comments highly welcome.



To use this, you need sleep.S modifications for your SoC type (to get 
__save/__restore_processor_state hooks). I've sent some of those for 
illustration earlier, they haven't changed, I've not included them here, 
so pick these changes up from:

http://68.183.106.108/lists/linux-pm/msg24020.html

The patch below only contains the _generic_ code.


This is tested on S5P6450 and OMAP3, with the sleep...S changes just 
mentioned - check the archives for those. Works both with normal swsusp and 
tuxonice (again, check the archives for the TOI modification needed).



Previously, I've reported OMAP3 video issues, after resume-from-disk. That 
isn't fully solved (it's a driver issue) but I've found a workaround: 
Trigger the resume from initramfs, after loading a logo image into the 
framebuffer and switching it on. That gets everything back without 
corruptions / wrong LCD reinitialization.

The OMAP video seems a bit of a diva; I've got one board type on which 
suspend/resume work perfectly but the omapdss driver spits out thousands 
of error interrupts during system startup (before the image is loaded), 
and the other board where all that is fine but the restore somehow garbles 
the LCD clocking (but the driver's sysfs files claim it's the same).


In short: This stuff really works now, for all I can say. And adding 
support for new type of ARM SoC doesn't touch the basic / generic code at 
all anymore either.




Anyway ...
About the patch, changes vs. all previous suggestions:

* Made the assembly sources as small as I responsibly could ;-)
   They compile for thumb2 (but I haven't tested that yet) as well.

* The page copy loop is now a C function. That also has the advantage
   that one can use cpu_switch_mm() - a macro - there for swapper_pg_dir,
   which makes resume via uswsusp ioctl or /sys/power/tuxonice/do_resume
   possible (only tested the latter, though).

* The SoC state save/restore is made to (re-)use the existing code in
   sleep....S  for the particular chip.
   OMAP3 and S5P64xx are provided as examples of that.

* The save/restore_processor_state() hooks are now used in the same way
   as e.g. existing powerpc code uses them (to trigger lazy saves before
   and perform cache flushes after).


Things that probably aren't perfect yet:

* The code currently reserves a full page for the saved "core" state.
   This is more than absolutely necessary; anyone think it's a problem ?

* it sets aside another half a page of __nosavedata page for use as
   temporary stack during the image copy (so that funcs can be called).

   Right now on ARM, that's not an issue because even with TuxOnIce in,
   there's less than 20 bytes of nosave stuff, so can as well put the
   rest of that page to good use ;-)

* I'd love to get rid of the include/asm-generic/vmlinux.lds.h change,
   as it seems that's not necessary in other architectures.
   Without that, the code gives a link error when building vmlinux
   though, and I'm unsure how to address that.

* The "integration" with the CPU sleep code is rather "backdoorish".
   While the hooks into ..._cpu_suspend aren't massive, and there's no
   code duplication, it'd be nicer to eventually have a cleaner way.

* An OMAPDSS restore troubleshooting HOWTO would be helpful ;-)


* The patch needs to be rebaselined against a current kernel;
   any preferences which tree to base this on ?



Thanks for all help with the little nits !
FrankH.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hibernate-19May2011.patch
Type: text/x-diff
Size: 7512 bytes
Desc: 
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110519/1d69d23c/attachment.bin>

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

[Dave Martin]

On Thu, May 19, 2011 at 06:31:28PM +0100, Frank Hofmann wrote:
> Hi,
> 
> /me again ...
> 
> Sorry that this took a little ... holidays. And work. And distractions...
> 
> Anyway, here we go again, basic code to enable hibernation
> (suspend-to-disk) on ARM platforms.
> 
> Any comments highly welcome.
> 
> 
> 
> To use this, you need sleep.S modifications for your SoC type (to
> get __save/__restore_processor_state hooks). I've sent some of those
> for illustration earlier, they haven't changed, I've not included
> them here, so pick these changes up from:
> 
> http://68.183.106.108/lists/linux-pm/msg24020.html
> 
> The patch below only contains the _generic_ code.
> 
> 
> This is tested on S5P6450 and OMAP3, with the sleep...S changes just
> mentioned - check the archives for those. Works both with normal
> swsusp and tuxonice (again, check the archives for the TOI
> modification needed).
> 
> 
> 
> Previously, I've reported OMAP3 video issues, after
> resume-from-disk. That isn't fully solved (it's a driver issue) but
> I've found a workaround: Trigger the resume from initramfs, after
> loading a logo image into the framebuffer and switching it on. That
> gets everything back without corruptions / wrong LCD
> reinitialization.
> 
> The OMAP video seems a bit of a diva; I've got one board type on
> which suspend/resume work perfectly but the omapdss driver spits out
> thousands of error interrupts during system startup (before the
> image is loaded), and the other board where all that is fine but the
> restore somehow garbles the LCD clocking (but the driver's sysfs
> files claim it's the same).
> 
> 
> In short: This stuff really works now, for all I can say. And adding
> support for new type of ARM SoC doesn't touch the basic / generic
> code at all anymore either.
> 
> 
> 
> 
> Anyway ...
> About the patch, changes vs. all previous suggestions:
> 
> * Made the assembly sources as small as I responsibly could ;-)
>   They compile for thumb2 (but I haven't tested that yet) as well.
> 
> * The page copy loop is now a C function. That also has the advantage
>   that one can use cpu_switch_mm() - a macro - there for swapper_pg_dir,
>   which makes resume via uswsusp ioctl or /sys/power/tuxonice/do_resume
>   possible (only tested the latter, though).
> 
> * The SoC state save/restore is made to (re-)use the existing code in
>   sleep....S  for the particular chip.
>   OMAP3 and S5P64xx are provided as examples of that.
> 
> * The save/restore_processor_state() hooks are now used in the same way
>   as e.g. existing powerpc code uses them (to trigger lazy saves before
>   and perform cache flushes after).
> 
> 
> Things that probably aren't perfect yet:
> 
> * The code currently reserves a full page for the saved "core" state.
>   This is more than absolutely necessary; anyone think it's a problem ?
> 
> * it sets aside another half a page of __nosavedata page for use as
>   temporary stack during the image copy (so that funcs can be called).
> 
>   Right now on ARM, that's not an issue because even with TuxOnIce in,
>   there's less than 20 bytes of nosave stuff, so can as well put the
>   rest of that page to good use ;-)
> 
> * I'd love to get rid of the include/asm-generic/vmlinux.lds.h change,
>   as it seems that's not necessary in other architectures.
>   Without that, the code gives a link error when building vmlinux
>   though, and I'm unsure how to address that.
> 
> * The "integration" with the CPU sleep code is rather "backdoorish".
>   While the hooks into ..._cpu_suspend aren't massive, and there's no
>   code duplication, it'd be nicer to eventually have a cleaner way.
> 
> * An OMAPDSS restore troubleshooting HOWTO would be helpful ;-)
> 
> 
> * The patch needs to be rebaselined against a current kernel;
>   any preferences which tree to base this on ?
> 
> 
> 
> Thanks for all help with the little nits !
> FrankH.

>  arch/arm/Kconfig                  |    3 +
>  arch/arm/include/asm/memory.h     |    1 +
>  arch/arm/include/asm/suspend.h    |    6 ++
>  arch/arm/kernel/cpu.c             |   65 ++++++++++++++++++++++++++
>  arch/arm/kernel/swsusp.S          |   92 +++++++++++++++++++++++++++++++++++++
>  arch/arm/kernel/vmlinux.lds.S     |    3 +-
>  include/asm-generic/vmlinux.lds.h |    2 +-
>  7 files changed, 170 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 6b6786c..859dd86 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -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"
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index 5421d82..23e93a6 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -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)
>  
>  /*
> diff --git a/arch/arm/include/asm/suspend.h b/arch/arm/include/asm/suspend.h
> new file mode 100644
> index 0000000..7ab1fd2
> --- /dev/null
> +++ b/arch/arm/include/asm/suspend.h
> @@ -0,0 +1,6 @@
> +#ifndef __ASM_ARM_SUSPEND_H
> +#define __ASM_ARM_SUSPEND_H
> +
> +static inline int arch_prepare_suspend(void) { return 0; }
> +
> +#endif /* __ASM_ARM_SUSPEND_H */
> diff --git a/arch/arm/kernel/cpu.c b/arch/arm/kernel/cpu.c
> new file mode 100644
> index 0000000..764c8fa
> --- /dev/null
> +++ b/arch/arm/kernel/cpu.c
> @@ -0,0 +1,65 @@
> +/*
> + * Hibernation support specific for ARM
> + *
> + * Derived from work on ARM hibernation support by:
> + *
> + * Ubuntu project, hibernation support for mach-dove
> + * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu)
> + * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.)
> + *	https://lkml.org/lkml/2010/6/18/4
> + *	https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html
> + *	https://patchwork.kernel.org/patch/96442/
> + *
> + * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl>
> + *
> + * License terms: GNU General Public License (GPL) version 2
> + */
> +
> +#include <linux/mm.h>
> +#include <linux/sched.h>
> +#include <linux/suspend.h>
> +#include <asm/tlbflush.h>
> +
> +extern const void __nosave_begin, __nosave_end;
> +
> +int pfn_is_nosave(unsigned long pfn)
> +{
> +	unsigned long nosave_begin_pfn = __pa_symbol(&__nosave_begin) >> PAGE_SHIFT;
> +	unsigned long nosave_end_pfn = PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT;
> +
> +	return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
> +}
> +
> +void save_processor_state(void)
> +{
> +	flush_thread();
> +}
> +
> +void restore_processor_state(void)
> +{
> +	local_flush_tlb_all();
> +}
> +
> +u8 __swsusp_arch_ctx[PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
> +u8 __swsusp_resume_stk[PAGE_SIZE/2] __nosavedata;
> +
> +/*
> + * The framework loads the hibernation image into this linked list,
> + * for swsusp_arch_resume() to copy back to the proper destinations.
> + *
> + * To make this work if resume is triggered from initramfs, the
> + * pagetables need to be switched to allow writes to kernel mem.
> + */
> +void notrace __swsusp_arch_restore_prepare(void)
> +{
> +	cpu_switch_mm(__virt_to_phys(swapper_pg_dir), current->active_mm);
> +}
> +
> +void notrace __swsusp_arch_restore_image(void)
> +{
> +	extern struct pbe *restore_pblist;
> +	struct pbe *pbe;
> +
> +	for (pbe = restore_pblist; pbe; pbe = pbe->next)
> +		copy_page(pbe->orig_address, pbe->address);
> +}
> diff --git a/arch/arm/kernel/swsusp.S b/arch/arm/kernel/swsusp.S
> new file mode 100644
> index 0000000..fb260a7
> --- /dev/null
> +++ b/arch/arm/kernel/swsusp.S
> @@ -0,0 +1,92 @@
> +/*
> + * Hibernation support specific for ARM
> + *
> + * Based on work by:
> + *
> + * Ubuntu project, hibernation support for mach-dove,
> + * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu)
> + * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.)
> + *	https://lkml.org/lkml/2010/6/18/4
> + *	https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html
> + *	https://patchwork.kernel.org/patch/96442/
> + *
> + * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl>
> + *
> + * License terms: GNU General Public License (GPL) version 2
> + */
> +
> +#include <linux/linkage.h>
> +#include <asm/memory.h>
> +#include <asm/page.h>
> +#include <asm/cache.h>
> +#include <asm/ptrace.h>
> +
> +/*
> + * Save the current CPU state before suspend / poweroff.
> + */
> +ENTRY(swsusp_arch_suspend)
> +	ldr	r0, =__swsusp_arch_ctx
> +	mrs	r1, cpsr
> +	str	r1, [r0], #4		/* CPSR */
> +ARM(	msr	cpsr_c, #SYSTEM_MODE					)
> +THUMB(	mov	r2, #SYSTEM_MODE					)
> +THUMB(	msr	cpsr_c, r2						)

For Thumb-2 kernels, you can use the cps instruction to change the CPU
mode:
	cps	#SYSTEM_MODE

For ARM though, this instruction is only supported for ARMv6 and above,
so it's best to keep the msr form for compatibility for that case.

> +	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.

> +	str	sp, [r0], #4
> +ARM(	msr	cpsr_c, #SVC_MODE					)
> +THUMB(	mov	r2, #SVC_MODE						)
> +THUMB(	msr	cpsr_c, r2						)
> +	mrs	r2, spsr
> +	stm	r0!, {r2,lr}		/* SVC SPSR, SVC regs */
> +	str	sp, [r0], #4
> +	msr	cpsr, r1		/* restore mode at entry */
> +	push	{lr}
> +	bl	__save_processor_state

<aside>

Structurally, we seem to have:

swsusp_arch_suspend {
	/* save some processor state */
	__save_processor_state();

	swsusp_save();
}

Is __save_processor_state() intended to encapsulate all the CPU-model-
specific state saving?  Excuse my ignorance of previous conversations...

</aside>

> +	pop	{lr}
> +	b	swsusp_save
> +ENDPROC(swsusp_arch_suspend)

I'm not too familiar with the suspend/resume stuff, so I may be asking
a dumb question here, but:

Where do we save/restore r8_FIQ..r13_FIQ, r13_IRQ, r13_UND and r13_ABT?
(I'm assuming there's no reason to save/restore r14 or SPSR for any
exception mode, since we presumably aren't going to suspend/resume
from inside an exception handler (?))

The exception stack pointers might conceivably be reinitialised to
sane values on resume, without necessarily needing to save/restore
them, providing my assumption in the previous paragraph is correct.

r8_FIQ..r12_FIQ can store arbitrary state used by the FIQ handler,
if FIQ is in use.  Can we expect any driver using FIQ to save/restore
this state itself, rather than doing it globally?  This may be
reasonable.

Cheers
---Dave

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

[Frank Hofmann]

On Fri, 20 May 2011, Dave Martin wrote:

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

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

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

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


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

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

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

You're right.

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

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

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

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

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


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

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

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

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

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

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


>
> Cheers
> ---Dave
>
>

Thanks,
FrankH.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: s5p-hibernate-hook.patch
Type: text/x-diff
Size: 2451 bytes
Desc: 
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110520/2da942b0/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: omap3-hibernate-hook.patch
Type: text/x-diff
Size: 2034 bytes
Desc: 
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110520/2da942b0/attachment-0001.bin>

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

[Dave Martin]

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

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

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

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

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


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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Cheers
---Dave

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

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

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

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

[Frank Hofmann]

On Fri, 20 May 2011, Dave Martin wrote:

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

arch/arm/lib are the only users.

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

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

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

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

<speak up or remain silent forever> ;-)

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

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

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

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

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

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

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

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

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

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

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

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

>
> Cheers
> ---Dave

Thanks again,
FrankH.

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

[Nicolas Pitre]

On Fri, 20 May 2011, Dave Martin wrote:

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

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

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

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

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

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

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

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


Nicolas

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

[Russell King - ARM Linux]

On Fri, May 20, 2011 at 12:37:58PM +0100, Dave Martin wrote:
> 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.

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

[Russell King - ARM Linux]

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

"push" was never an ARM instruction until someone decided to make r13
"special".  As our macros there pre-date the invention of the new ARM
instruction neumonics, it takes precident _especially_ as there's
perfectly good alternative ways to say "push" to the assembler.

[linux-pm] [RFC PATCH] ARM hibernation / suspend-to-disk support code

[Rafael J. Wysocki]

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

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

Thanks,
Rafael

[linux-pm] [RFC PATCH] ARM hibernation / suspend-to-disk support code

[Frank Hofmann]

> [ ... ]
> > > r8_FIQ..r12_FIQ can store arbitrary state used by the FIQ handler,
> > > if FIQ is in use.  Can we expect any driver using FIQ to save/restore
> > > this state itself, rather than doing it globally?  This may be
> > > reasonable.
> > 
> > We don't need to save/restore those, because at the time the snapshot is 
> > taken interrupts are off and we cannot be in any trap handler either. On 
> > resume, the kernel that has been loaded has initialized them properly 
> > already.
> 
> I'm not sure if this is a safe assumption in general.  We may decide to
> switch to loading hibernate images from boot loaders, for example, and
> it may not hold any more.  Generally, please don't assume _anything_ has
> been properly initialized during resume, before the image is loaded.
> This has already beaten us a couple of times.
> 
> Thanks,
> Rafael

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>

[linux-pm] [RFC PATCH] ARM hibernation / suspend-to-disk support code

[Rafael J. Wysocki]

On Sunday, May 22, 2011, Frank Hofmann wrote:
> > [ ... ]
> > > > r8_FIQ..r12_FIQ can store arbitrary state used by the FIQ handler,
> > > > if FIQ is in use.  Can we expect any driver using FIQ to save/restore
> > > > this state itself, rather than doing it globally?  This may be
> > > > reasonable.
> > > 
> > > We don't need to save/restore those, because at the time the snapshot is 
> > > taken interrupts are off and we cannot be in any trap handler either. On 
> > > resume, the kernel that has been loaded has initialized them properly 
> > > already.
> > 
> > I'm not sure if this is a safe assumption in general.  We may decide to
> > switch to loading hibernate images from boot loaders, for example, and
> > it may not hold any more.  Generally, please don't assume _anything_ has
> > been properly initialized during resume, before the image is loaded.
> > This has already beaten us a couple of times.
> > 
> > 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.

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

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

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

OK

> 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

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

[Dave Martin]

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

Gone in where?

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

> I.e. state "do not assume persistence of these registers over power
> mgmt transitions - if that's what you require, implement suspend /
> resume hooks" ?
> 
> >
> >>
> >>See also Russell's mails about that, for previous attempts a year
> >>and half a year ago to get "something for ARM hibernation" in:
> >>
> >>https://lists.linux-foundation.org/pipermail/linux-pm/2010-July/027571.html
> >>https://lists.linux-foundation.org/pipermail/linux-pm/2010-December/029665.html
> >
> >Relying on drivers to save/restore the FIQ state if necessary seems
> >to correspond to what Russell is saying in his second mail.
> 
> Agreed, and largely why I haven't bothered touching FIQ.
> 
> >
> >>
> >>The kernel doesn't do IRQ/FIQ/ABT/UND save / restore for
> >>suspend-to-ram either. CPU hotplug support (re)initializes those. I
> >>believe we're fine.
> >
> >OK, just wanted to make sure I understood that right.
> 
> By and large, "if suspend-to-ram works, suspend-to-disk should too"
> seems a good idea; if the former doesn't (need to) save/restore such
> state even though it's not off-mode-persistent, then the latter
> doesn't need either.
> 
> That said, I've seen (out-of-tree) SoC-specific *suspend() code that
> simply blotted everything out. Seems the most trivial way to go, but
> not necessarily the best.
> 
> >
> >I'll leave it to others to comment on the actual SoC-specific routines,
> >but thanks for the illustration.
> 
> To make this clear, I'm not personally considering these parts
> optimal as the attached patch is doing them.
> 
> That's why preferrably, only the "enabler" code should go in first.
> 
> I do wonder, though, whether the machine maintainers are willing to
> make the change to these low-level suspend parts that'd split chip
> state save/restore from the actual power transition. That'd allow to
> turn this from a "backdoor" into a proper use of the interface.
> 
> Russell has made the change to pass the context buffer as argument,
> but not split the power transition off; things are getting there,
> eventually :)
> 
> >
> >Cheers
> >---Dave
> 
> Thanks again,
> FrankH.

[linux-pm] [RFC PATCH] ARM hibernation / suspend-to-disk support code

[Dave Martin]

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

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

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

Cheers
---Dave

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

[Dave Martin]

On Fri, May 20, 2011 at 07:05:10PM +0100, Russell King - ARM Linux wrote:
> On Fri, May 20, 2011 at 12:37:58PM +0100, Dave Martin wrote:
> > 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...

> 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

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

[Russell King - ARM Linux]

On Mon, May 23, 2011 at 11:01:32AM +0100, Dave Martin wrote:
> On Fri, May 20, 2011 at 07:05:10PM +0100, Russell King - ARM Linux wrote:
> > 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.

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

[Dave Martin]

On Mon, May 23, 2011 at 11:12:20AM +0100, Russell King - ARM Linux wrote:
> On Mon, May 23, 2011 at 11:01:32AM +0100, Dave Martin wrote:
> > On Fri, May 20, 2011 at 07:05:10PM +0100, Russell King - ARM Linux wrote:
> > > 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.

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?

> 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

[linux-pm] [RFC PATCH] ARM hibernation / suspend-to-disk support code

[Frank Hofmann]

On Mon, 23 May 2011, Dave Martin wrote:

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

What is whose' responsibility ...

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

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

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

Regarding the latter:

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

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

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

Am I understanding that part correctly ?

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

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



Regarding interrupt stacks:

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

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

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

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


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




Regarding other state:

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

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

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

Samsung ARM11's do in their cpu_idle:

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

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


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

If so:

>From bogus@does.not.exist.com  Fri May 20 08:44:36 2011
From: bogus@does.not.exist.com ()
Date: Fri, 20 May 2011 12:44:36 -0000
Subject: No subject
Message-ID: <mailman.2.1306157846.23796.linux-arm-kernel@lists.infradead.org>

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.

[linux-pm] [RFC PATCH] ARM hibernation / suspend-to-disk support code

[Russell King - ARM Linux]

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

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

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

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

[Frank Hofmann]

On Mon, 23 May 2011, Russell King - ARM Linux wrote:

> On Mon, May 23, 2011 at 02:37:19PM +0100, Frank Hofmann wrote:
>> What I've found necessary to save/restore via swsusp_arch_suspend/resume
>> are the SYSTEM_MODE and SVC_MODE registers.
>> Yesterday, I had thought that cpu_init() resets SVC_MODE sufficiently but
>> that doesn't seem to be the case, if I leave that out, resume-from-disk
>> doesn't work anymore.
>
> You will be running in SVC mode, so the SVC mode registers are your
> current register set.  At some point you need to do an effective
> "context switch" between the kernel doing the resume and the kernel
> which was running.  That involves restoring the saved register state.
>
> System mode on the other hand is unused by the kernel.
>

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>

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

[Russell King - ARM Linux]

On Mon, May 23, 2011 at 12:16:31PM +0100, Dave Martin wrote:
> On Mon, May 23, 2011 at 11:12:20AM +0100, Russell King - ARM Linux wrote:
> > On Mon, May 23, 2011 at 11:01:32AM +0100, Dave Martin wrote:
> > > On Fri, May 20, 2011 at 07:05:10PM +0100, Russell King - ARM Linux wrote:
> > > > 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.
> 
> 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?

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.

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

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

[Dave Martin]

On Mon, May 23, 2011 at 05:11:49PM +0100, Russell King - ARM Linux wrote:
> On Mon, May 23, 2011 at 12:16:31PM +0100, Dave Martin wrote:
> > On Mon, May 23, 2011 at 11:12:20AM +0100, Russell King - ARM Linux wrote:
> > > On Mon, May 23, 2011 at 11:01:32AM +0100, Dave Martin wrote:
> > > > On Fri, May 20, 2011 at 07:05:10PM +0100, Russell King - ARM Linux wrote:
> > > > > 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.
> > 
> > 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?
> 
> 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

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

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

[Frank Hofmann]

On Mon, 23 May 2011, Dave Martin wrote:

> On Mon, May 23, 2011 at 05:11:49PM +0100, Russell King - ARM Linux wrote:
[ ... ]
>> 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 !

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