* [RFC PATCH] ARM hibernation / suspend-to-disk support code
@ 2011-05-20 12:39 ` Frank Hofmann
0 siblings, 0 replies; 46+ messages in thread
From: Frank Hofmann @ 2011-05-20 12:39 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 12:39 ` Frank Hofmann
(?)
@ 2011-05-20 15:03 ` Dave Martin
-1 siblings, 0 replies; 46+ messages in thread
From: Dave Martin @ 2011-05-20 15:03 UTC (permalink / raw)
To: Frank Hofmann; +Cc: linux-pm, tuxonice-devel, linux-arm-kernel
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"
^ permalink raw reply [flat|nested] 46+ messages in thread* [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 12:39 ` Frank Hofmann
(?)
(?)
@ 2011-05-20 15:03 ` Dave Martin
2011-05-20 16:24 ` Frank Hofmann
` (3 more replies)
-1 siblings, 4 replies; 46+ messages in thread
From: Dave Martin @ 2011-05-20 15:03 UTC (permalink / raw)
To: linux-arm-kernel
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"
^ permalink raw reply [flat|nested] 46+ messages in thread* [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 15:03 ` Dave Martin
@ 2011-05-20 16:24 ` Frank Hofmann
2011-05-23 9:42 ` Dave Martin
2011-05-23 9:42 ` Dave Martin
2011-05-20 16:24 ` Frank Hofmann
` (2 subsequent siblings)
3 siblings, 2 replies; 46+ messages in thread
From: Frank Hofmann @ 2011-05-20 16:24 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 16:24 ` Frank Hofmann
@ 2011-05-23 9:42 ` Dave Martin
2011-05-23 9:42 ` Dave Martin
1 sibling, 0 replies; 46+ messages in thread
From: Dave Martin @ 2011-05-23 9:42 UTC (permalink / raw)
To: Frank Hofmann; +Cc: linux-pm, tuxonice-devel, linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 16:24 ` Frank Hofmann
2011-05-23 9:42 ` Dave Martin
@ 2011-05-23 9:42 ` Dave Martin
1 sibling, 0 replies; 46+ messages in thread
From: Dave Martin @ 2011-05-23 9:42 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 15:03 ` Dave Martin
2011-05-20 16:24 ` Frank Hofmann
@ 2011-05-20 16:24 ` Frank Hofmann
2011-05-20 17:53 ` Nicolas Pitre
2011-05-20 17:53 ` Nicolas Pitre
3 siblings, 0 replies; 46+ messages in thread
From: Frank Hofmann @ 2011-05-20 16:24 UTC (permalink / raw)
To: Dave Martin; +Cc: Frank Hofmann, linux-pm, tuxonice-devel, linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 15:03 ` Dave Martin
2011-05-20 16:24 ` Frank Hofmann
2011-05-20 16:24 ` Frank Hofmann
@ 2011-05-20 17:53 ` Nicolas Pitre
2011-05-20 17:53 ` Nicolas Pitre
3 siblings, 0 replies; 46+ messages in thread
From: Nicolas Pitre @ 2011-05-20 17:53 UTC (permalink / raw)
To: Dave Martin; +Cc: Frank Hofmann, linux-pm, tuxonice-devel, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 46+ messages in thread* [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 15:03 ` Dave Martin
` (2 preceding siblings ...)
2011-05-20 17:53 ` Nicolas Pitre
@ 2011-05-20 17:53 ` Nicolas Pitre
3 siblings, 0 replies; 46+ messages in thread
From: Nicolas Pitre @ 2011-05-20 17:53 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 12:39 ` Frank Hofmann
` (2 preceding siblings ...)
(?)
@ 2011-05-20 18:07 ` Russell King - ARM Linux
-1 siblings, 0 replies; 46+ messages in thread
From: Russell King - ARM Linux @ 2011-05-20 18:07 UTC (permalink / raw)
To: Frank Hofmann; +Cc: Dave Martin, linux-pm, linux-arm-kernel, tuxonice-devel
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.
^ permalink raw reply [flat|nested] 46+ messages in thread* [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 12:39 ` Frank Hofmann
` (3 preceding siblings ...)
(?)
@ 2011-05-20 18:07 ` Russell King - ARM Linux
2011-05-22 6:39 ` Frank Hofmann
-1 siblings, 1 reply; 46+ messages in thread
From: Russell King - ARM Linux @ 2011-05-20 18:07 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 18:07 ` Russell King - ARM Linux
@ 2011-05-22 6:39 ` Frank Hofmann
0 siblings, 0 replies; 46+ messages in thread
From: Frank Hofmann @ 2011-05-22 6:39 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Dave Martin, linux-pm, linux-arm-kernel, tuxonice-devel
[-- Attachment #1.1: Type: text/plain, Size: 1107 bytes --]
Hi Russell / Nicolas,
thanks for the clarification on ARM history; there's a lot to learn for me about that still.
I'll reply to Rafael regarding state save/restore.
FrankH.
-----Original Message-----
From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
Sent: Fri 20/05/2011 20:07
To: Frank Hofmann
Cc: Dave Martin; linux-pm@lists.linux-foundation.org; tuxonice-devel@tuxonice.net; linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code
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.
[-- Attachment #1.2: Type: text/html, Size: 1742 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [linux-pm] [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 12:39 ` Frank Hofmann
` (4 preceding siblings ...)
(?)
@ 2011-05-20 22:27 ` Rafael J. Wysocki
2011-05-22 7:01 ` Frank Hofmann
` (3 more replies)
-1 siblings, 4 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2011-05-20 22:27 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 46+ messages in thread* [linux-pm] [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 22:27 ` [linux-pm] " Rafael J. Wysocki
@ 2011-05-22 7:01 ` Frank Hofmann
2011-05-22 9:54 ` Rafael J. Wysocki
2011-05-22 9:54 ` Rafael J. Wysocki
2011-05-22 7:01 ` Frank Hofmann
` (2 subsequent siblings)
3 siblings, 2 replies; 46+ messages in thread
From: Frank Hofmann @ 2011-05-22 7:01 UTC (permalink / raw)
To: linux-arm-kernel
> [ ... ]
> > > 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>
^ permalink raw reply [flat|nested] 46+ messages in thread* [linux-pm] [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-22 7:01 ` Frank Hofmann
@ 2011-05-22 9:54 ` Rafael J. Wysocki
2011-05-22 9:54 ` Rafael J. Wysocki
1 sibling, 0 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2011-05-22 9:54 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-22 7:01 ` Frank Hofmann
2011-05-22 9:54 ` Rafael J. Wysocki
@ 2011-05-22 9:54 ` Rafael J. Wysocki
1 sibling, 0 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2011-05-22 9:54 UTC (permalink / raw)
To: Frank Hofmann; +Cc: linux-pm, Dave Martin, linux-arm-kernel, tuxonice-devel
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@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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 22:27 ` [linux-pm] " Rafael J. Wysocki
2011-05-22 7:01 ` Frank Hofmann
@ 2011-05-22 7:01 ` Frank Hofmann
2011-05-23 9:52 ` Dave Martin
2011-05-23 9:52 ` [linux-pm] " Dave Martin
3 siblings, 0 replies; 46+ messages in thread
From: Frank Hofmann @ 2011-05-22 7:01 UTC (permalink / raw)
To: Rafael J. Wysocki, linux-pm; +Cc: Dave Martin, tuxonice-devel, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1945 bytes --]
> [ ... ]
> > > 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@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.
[-- Attachment #1.2: Type: text/html, Size: 2732 bytes --]
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: hibernation-core-22May2011.patch --]
[-- Type: text/x-patch; name="hibernation-core-22May2011.patch", Size: 5391 bytes --]
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 44c16f0..f282bac 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -207,6 +207,9 @@ config ARM_PATCH_PHYS_VIRT_16BIT
def_bool y
depends on ARM_PATCH_PHYS_VIRT && ARCH_MSM
+config ARCH_HIBERNATION_POSSIBLE
+ def_bool n
+
source "init/Kconfig"
source "kernel/Kconfig.freezer"
diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index 431077c..c7ef454 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -250,6 +250,7 @@ static inline void *phys_to_virt(phys_addr_t x)
*/
#define __pa(x) __virt_to_phys((unsigned long)(x))
#define __va(x) ((void *)__phys_to_virt((unsigned long)(x)))
+#define __pa_symbol(x) __pa(RELOC_HIDE((unsigned long)(x),0))
#define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT)
/*
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 8d95446..a1f54cd 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_ARM_THUMBEE) += thumbee.o
obj-$(CONFIG_KGDB) += kgdb.o
obj-$(CONFIG_ARM_UNWIND) += unwind.o
obj-$(CONFIG_HAVE_TCM) += tcm.o
+obj-$(CONFIG_HIBERNATION) += cpu.o swsusp.o
obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
obj-$(CONFIG_SWP_EMULATE) += swp_emulate.o
CFLAGS_swp_emulate.o := -Wa,-march=armv7-a
diff --git a/arch/arm/kernel/cpu.c b/arch/arm/kernel/cpu.c
new file mode 100644
index 0000000..0f1c31f
--- /dev/null
+++ b/arch/arm/kernel/cpu.c
@@ -0,0 +1,64 @@
+/*
+ * 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();
+ local_fiq_disable();
+}
+
+void restore_processor_state(void)
+{
+ local_flush_tlb_all();
+ local_fiq_enable();
+}
+
+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_image(void)
+{
+ extern struct pbe *restore_pblist;
+ struct pbe *pbe;
+
+ cpu_switch_mm(__virt_to_phys(swapper_pg_dir), &init_mm);
+
+ 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..d7ebbe9
--- /dev/null
+++ b/arch/arm/kernel/swsusp.S
@@ -0,0 +1,68 @@
+/*
+ * 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
+ stm r0!, {r1-r12,lr} /* CPSR, nonvolatile regs */
+ stmfd sp!, {lr}
+ bl __save_processor_state
+ ldmfd sp!, {lr}
+ b swsusp_save
+ENDPROC(swsusp_arch_suspend)
+
+/*
+ * Restore the memory image from the pagelists, and load the CPU registers
+ * from saved state.
+ */
+ENTRY(swsusp_arch_resume)
+ /*
+ * Switch stack to a nosavedata region to make sure image restore
+ * doesn't clobber it underneath itself.
+ */
+ ldr sp, =(__swsusp_resume_stk + PAGE_SIZE / 2)
+ bl __swsusp_arch_restore_image
+
+ /*
+ * Restore the CPU registers.
+ */
+ ldr r0, =__swsusp_arch_ctx
+ ldr r1, [r0], #4
+ msr cpsr, r1
+ ldm r0!, {r2-r12,lr}
+ /*
+ * From here on we have a valid stack again. Core state is
+ * not restored yet, redirect to the machine-specific
+ * implementation to get that done.
+ * Resume has succeeded at this point; if the machine-specific
+ * code wants to fail it needs to panic.
+ */
+ mov r1, #0
+ push {r1,lr}
+ bl __restore_processor_state /* restore core state */
+ bl cpu_init
+ pop {r0,pc}
+ENDPROC(swsusp_arch_resume)
[-- Attachment #3: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 22:27 ` [linux-pm] " Rafael J. Wysocki
2011-05-22 7:01 ` Frank Hofmann
2011-05-22 7:01 ` Frank Hofmann
@ 2011-05-23 9:52 ` Dave Martin
2011-05-23 9:52 ` [linux-pm] " Dave Martin
3 siblings, 0 replies; 46+ messages in thread
From: Dave Martin @ 2011-05-23 9:52 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: frank.hofmann, linux-pm, tuxonice-devel, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 46+ messages in thread* [linux-pm] [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 22:27 ` [linux-pm] " Rafael J. Wysocki
` (2 preceding siblings ...)
2011-05-23 9:52 ` Dave Martin
@ 2011-05-23 9:52 ` Dave Martin
2011-05-23 13:37 ` Frank Hofmann
2011-05-23 13:37 ` [RFC PATCH] " Frank Hofmann
3 siblings, 2 replies; 46+ messages in thread
From: Dave Martin @ 2011-05-23 9:52 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 46+ messages in thread* [linux-pm] [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-23 9:52 ` [linux-pm] " Dave Martin
@ 2011-05-23 13:37 ` Frank Hofmann
2011-05-23 14:32 ` [linux-pm] " Russell King - ARM Linux
2011-05-23 13:37 ` [RFC PATCH] " Frank Hofmann
1 sibling, 1 reply; 46+ messages in thread
From: Frank Hofmann @ 2011-05-23 13:37 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-23 13:37 ` Frank Hofmann
@ 2011-05-23 14:32 ` Russell King - ARM Linux
0 siblings, 0 replies; 46+ messages in thread
From: Russell King - ARM Linux @ 2011-05-23 14:32 UTC (permalink / raw)
To: Frank Hofmann; +Cc: Dave Martin, linux-pm, linux-arm-kernel, tuxonice-devel
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.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [linux-pm] [RFC PATCH] ARM hibernation / suspend-to-disk support code
@ 2011-05-23 14:32 ` Russell King - ARM Linux
0 siblings, 0 replies; 46+ messages in thread
From: Russell King - ARM Linux @ 2011-05-23 14:32 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC PATCH v2] ARM hibernation / suspend-to-disk support code
2011-05-23 14:32 ` [linux-pm] " Russell King - ARM Linux
(?)
@ 2011-05-23 15:57 ` Frank Hofmann
-1 siblings, 0 replies; 46+ messages in thread
From: Frank Hofmann @ 2011-05-23 15:57 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC PATCH v2] ARM hibernation / suspend-to-disk support code
2011-05-23 14:32 ` [linux-pm] " Russell King - ARM Linux
(?)
(?)
@ 2011-05-23 15:57 ` Frank Hofmann
-1 siblings, 0 replies; 46+ messages in thread
From: Frank Hofmann @ 2011-05-23 15:57 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Dave Martin, tuxonice-devel, Frank Hofmann, linux-pm, linux-arm-kernel
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1550 bytes --]
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.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: TEXT/x-diff; name=hibernation-core-23May2011.patch, Size: 5761 bytes --]
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/kernel/Makefile b/arch/arm/kernel/Makefile
index c9b00bb..541ac3a 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_ARM_THUMBEE) += thumbee.o
obj-$(CONFIG_KGDB) += kgdb.o
obj-$(CONFIG_ARM_UNWIND) += unwind.o
obj-$(CONFIG_HAVE_TCM) += tcm.o
+obj-$(CONFIG_HIBERNATION) += cpu.o swsusp.o
obj-$(CONFIG_CRUNCH) += crunch.o crunch-bits.o
AFLAGS_crunch-bits.o := -Wa,-mcpu=ep9312
diff --git a/arch/arm/kernel/cpu.c b/arch/arm/kernel/cpu.c
new file mode 100644
index 0000000..0f1c31f
--- /dev/null
+++ b/arch/arm/kernel/cpu.c
@@ -0,0 +1,64 @@
+/*
+ * 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();
+ local_fiq_disable();
+}
+
+void restore_processor_state(void)
+{
+ local_flush_tlb_all();
+ local_fiq_enable();
+}
+
+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_image(void)
+{
+ extern struct pbe *restore_pblist;
+ struct pbe *pbe;
+
+ cpu_switch_mm(__virt_to_phys(swapper_pg_dir), &init_mm);
+
+ 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..1fc0e33
--- /dev/null
+++ b/arch/arm/kernel/swsusp.S
@@ -0,0 +1,72 @@
+/*
+ * 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>
+
+/*
+ * Save the current CPU state before suspend / poweroff.
+ */
+ENTRY(swsusp_arch_suspend)
+ ldr r0, =__swsusp_arch_ctx
+ mrs r1, cpsr
+ mrs r2, spsr
+ stmia r0!, {r1-r11,lr} @ CPSR, SPSR, nonvolatile regs
+ str sp, [r0], #4 @ stack
+ stmfd sp!, {lr}
+ bl __save_processor_state @ machine-specific state
+ ldmfd sp!, {lr}
+ b swsusp_save @ let framework write snapshot out
+ENDPROC(swsusp_arch_suspend)
+
+/*
+ * Restore the memory image from the pagelists, and load the CPU registers
+ * from saved state.
+ */
+ENTRY(swsusp_arch_resume)
+ /*
+ * Switch stack to a nosavedata region to make sure image restore
+ * doesn't clobber it underneath itself.
+ */
+ ldr sp, =(__swsusp_resume_stk + PAGE_SIZE / 2)
+ bl __swsusp_arch_restore_image
+
+ /*
+ * Restore the CPU registers.
+ */
+ ldr r0, =__swsusp_arch_ctx
+ ldmia r0!, {r1,r2} @ CPSR / SPSR
+ msr cpsr, r1
+ msr spsr, r2
+ ldr r0, =__swsusp_arch_ctx @ reload in case regset switched
+ ldmia r0!, {r1-r11,lr} @ nonvolatile regs
+ ldr sp, [r0], #4 @ stack
+
+ /*
+ * From here on we have a valid stack again. Core state is
+ * not restored yet, redirect to the machine-specific
+ * implementation to get that done.
+ * Resume has succeeded at this point; if the machine-specific
+ * code wants to fail it needs to panic.
+ */
+ mov r1, #0
+ stmfd sp!, {r1,lr}
+ bl __restore_processor_state @ machine-specific state
+ bl cpu_init @ reinitialize other modes
+ ldmfd sp!, {r0,pc}
+ENDPROC(swsusp_arch_resume)
[-- Attachment #3: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-23 9:52 ` [linux-pm] " Dave Martin
2011-05-23 13:37 ` Frank Hofmann
@ 2011-05-23 13:37 ` Frank Hofmann
1 sibling, 0 replies; 46+ messages in thread
From: Frank Hofmann @ 2011-05-23 13:37 UTC (permalink / raw)
To: Dave Martin; +Cc: frank.hofmann, linux-pm, tuxonice-devel, linux-arm-kernel
On Mon, 23 May 2011, Dave Martin wrote:
> On Sat, May 21, 2011 at 12:27:16AM +0200, Rafael J. Wysocki wrote:
>> On Friday, May 20, 2011, Frank Hofmann wrote:
[ ... ]
>>>> r8_FIQ..r12_FIQ can store arbitrary state used by the FIQ handler,
>>>> if FIQ is in use. Can we expect any driver using FIQ to save/restore
>>>> this state itself, rather than doing it globally? This may be
>>>> reasonable.
>>>
>>> We don't need to save/restore those, because at the time the snapshot is
>>> taken interrupts are off and we cannot be in any trap handler either. On
>>> resume, the kernel that has been loaded has initialized them properly
>>> already.
>>
>> I'm not sure if this is a safe assumption in general. We may decide to
>> switch to loading hibernate images from boot loaders, for example, and
>> it may not hold any more. Generally, please don't assume _anything_ has
>> been properly initialized during resume, before the image is loaded.
>> This has already beaten us a couple of times.
>
> Surely when resuming via the bootloader, the bootloader must still
> branch to some resume entry point in the kernel?
>
> That resume code would be responsible for doing any OS-specific
> initialisation and waking up the kernel; plus, the kernel should
> not re-enable interrupts until the kernel state has been restored
> anyway. It wouldn't be the responsibility of the bootloader to
> set up the relevant state.
What is whose' responsibility ...
It's as easy to say "you can't assume anything" as it is "you must assume
that ...". Hopefully, this isn't going to become philosophical ;-)
There are two things in the context of hibernation that were mentioned:
a) CPU interrupt stacks (FIQ/IRQ/UND/ABT stacks and reg sets)
b) swapper_pg_dir
Regarding the latter:
As far as swsusp_arch_resume() is concerned, by the time that function is
called one thing definitely has happened - the loading of the image.
That on the other hand requires some form of MMU setup having happened
before, because the resume image metadata (restore_pglist and the virtual
addresses used in the linked "struct pbe") have been created.
Also, since the code somehow ended up in swsusp_arch_resume, that part of
the kernel text must have been loaded, and mapped.
(If it weren't so, then how did whichever entity loaded the image manage
to create the list linkage ? How did it enter swsusp_arch_resume ?)
Am I understanding that part correctly ?
If so, then that part at least concedes that on ARM, one can safely switch
to swapper_pg_dir. Because on ARM, those are the tables which are supplied
by bootloader and/or kernel decompressor anyway.
That's why I consider it safe to switch to swapper_pg_dir mappings. Note
the second arg to cpu_switch_mm() is an "output context", I've moved that
now from using the current thread's (current->active_mm) to the temporary
global (init_mm), so there's no longer any reliance on having a kernel
thread context at the time swsusp_arch_resume() is entered.
Regarding interrupt stacks:
cpu_init() looks like the way to go for those. That takes care of them.
Seems better to do that as well than to enforce saving this context over a
power transition; if the kernel might find a good reason to have different
FIQ/IRQ/UND/ABT stacks after power transitions (just hypothetically) the
hibernate code should get out of its way.
I've added the call to cpu_init() at the end of swsusp_arch_resume(); that
again brings it in line with the existing cpu idle code for various ARM
incarnations.
I've also, for good measure, added a local_fiq_disable() / enable()
bracked within save/restore_processor_state(). Again, no more than the
cpu_idle code does, so hibernation should as well.
What I've found necessary to save/restore via swsusp_arch_suspend/resume
are the SYSTEM_MODE and SVC_MODE registers.
Yesterday, I had thought that cpu_init() resets SVC_MODE sufficiently but
that doesn't seem to be the case, if I leave that out, resume-from-disk
doesn't work anymore.
Regarding other state:
In the first email on this thread, I've said "let's talk the generic code
only (and ignore what's in __save/__restore_processor_state for now)".
Maybe it's a good idea to look into the machine type code again at this
point ...
The CPU idle code for various ARM incarnations does state saves for
subsystems beyond just the core; e.g. omap_sram_idle() does:
omap3_core_restore_context();
omap3_prcm_restore_context();
omap3_sram_restore_context();
omap2_sms_restore_context();
Samsung ARM11's do in their cpu_idle:
s3c_pm_restore_core();
s3c_pm_restore_uarts();
s3c_pm_restore_gpios();
and so on; this is currently not covered by the resume-from-disk code that
I have; the lack of that might be a possible cause for e.g. the omap video
restore issues I've seen. That's speculation, though.
Is this stuff needed (for suspend-to/resume-from-disk) ?
If so:
>From the way the the suspend-to-ram code is currently modeled, such state
is saved before the "SoC ARM context" parts, and restored after that and
the cpu_init() call. All ARM machines seem to do like:
...pm_enter():
local_irq_disable()
local_fiq_disable();
... maybe: save "machine-specific" stuff ...
... enter idle
==> save SoC state (CP regs)
<== restore
cpu_init()
... maybe: restore "machine-specific" stuff ...
local_fiq_enable()
local_irq_enable()
Anyway, that given, I wonder whether it makes sense to give the machines a
hook into save/restore_processor_state.
The other option of course it to be lazy (call it "flexible" if you
wish) and leave the cpu_init() call to the (single) existing machine-hook.
Thanks,
FrankH.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH] ARM hibernation / suspend-to-disk support code
2011-05-20 12:39 ` Frank Hofmann
` (5 preceding siblings ...)
(?)
@ 2011-05-20 22:27 ` Rafael J. Wysocki
-1 siblings, 0 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2011-05-20 22:27 UTC (permalink / raw)
To: linux-pm, frank.hofmann; +Cc: Dave Martin, tuxonice-devel, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 46+ messages in thread