| Message ID | alpine.DEB.2.00.1105201255260.8018@localhost6.localdomain6 |
|---|---|
| State | New |
| Headers | show |
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"
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.
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
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.
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
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.
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
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.
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.
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"