* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) @ 2011-06-13 12:04 Frank Hofmann 2011-06-13 12:26 ` Russell King - ARM Linux 2011-09-30 7:48 ` Barry Song 0 siblings, 2 replies; 21+ messages in thread From: Frank Hofmann @ 2011-06-13 12:04 UTC (permalink / raw) To: linux-arm-kernel Hi, to make one thing clear to start with: This is not a proposal to integrate the hibernation feature into mainline. It's not ripe for that because it's unclear what exactly the differences are between what the hibernation / S2RAM codepaths have to do, and/or how much code sharing between these codepaths is advisable. Therefore, I'm merely putting this out as-is as a reference / base for those who want to do their own experiments with the feature. Prerequisites: * The patch requires a working cpu_reset(); for v6/v7, that's in Will Deacon's patch series from last week. * It also requires being able to take the address of cpu_reset and on MULTI_CPU configs therefore, again see Will Deacon's patch series. * It uses on cpu_suspend() / cpu_resume(), therefore a recent-enough kernel (or a port of those to whatever you're using) is required. Important / Limitations: A) For platforms that currently use cpu_suspend/cpu_resume in their s2ram codepaths AND DO NOTHING ELSE BEFORE AND NOTHING ELSE AFTER THAT BUT TO "ENTER LOW POWER", the way the hibernation state snapshot for the CPU core is done by this patch is sufficient. To make it clear: IF AND ONLY IF your suspend(-to-ram) func looks like: ENTRY(acmeSoC_cpu_suspend) stmfd sp!, {r4-r12,lr} ldr r3, resume_mmu_done bl cpu_suspend resume_mmu_done: ldmfd sp!, {r3-r12,pc} ENDPROC(acmeSoC_cpu_suspend) then this patch is ok to provide hibernation support for your CPU core. In all other cases, item B). IN ALL CASES, read item C) below; there's more than the core to the SoC. B) If there's any cpu-specific / SoC-specific state that needs (re)init over a suspend-to-disk/resume-from-disk cycle, this patch is incomplete because it provides no hooks/code for that. This is the case e.g. for "secure" SoCs that have different sets of control registers and/or different CR reg access patterns. It's also the case e.g. for SoCs with L2 caches as the activation sequence there is SoC-dependent; a full off-on cycle for L2 is not done by the hibernation support code. It's also the case if the SoC requires steps on wakeup _before_ the "generic" parts done by cpu_suspend / cpu_resume can work correctly. (OMAP is an example of such a SoC; the patch "works" on OMAP in the sense that it gets you a non-secure OMAP back from hibernation but as mentioned, your mileage may vary; I for example don't know what the consequences of not disabling / reenabling the L2 cache over cpu_reset are) C) The current low-power handling may perform SoC-specific tasks such as GPIO state handling, which is currently also not addressed by this patch; the assumption is rather that device suspend/resume deals with this. That is not guaranteed to be complete / sufficient - if your SoC and/or your device drivers have particular needs in this area, you can put hooks performing these actions into save_/restore_processor_state(). D) If you wish to extend this to a SoC for which the generic CPU core hooks (i.e. what cpu_suspend/resume provide) are either insufficient or unavailable, hook your own state snapshot mechanism into the places where this code currently calls cpu_suspend and cpu_resume. The main assumption by the assembly code within swusp_arch_resume() is that __swsusp_arch_restore_image() returns to its caller with the stack restored - how you achieve that is up to you, the patch as-is does it by cpu_reset(cpu_resume) but your mileage may vary. I will, at this point, not send further iterations of this patch. I'll answer questions about it though, if there are any. Thanks for all the comments during the previous threads - learned a lot ! Best regards, FrankH. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-13 12:04 [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) Frank Hofmann @ 2011-06-13 12:26 ` Russell King - ARM Linux 2011-06-13 12:40 ` Frank Hofmann 2011-06-13 13:20 ` Frank Hofmann 2011-09-30 7:48 ` Barry Song 1 sibling, 2 replies; 21+ messages in thread From: Russell King - ARM Linux @ 2011-06-13 12:26 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jun 13, 2011 at 01:04:02PM +0100, Frank Hofmann wrote: > To make it clear: IF AND ONLY IF your suspend(-to-ram) func looks like: > > ENTRY(acmeSoC_cpu_suspend) > stmfd sp!, {r4-r12,lr} > ldr r3, resume_mmu_done > bl cpu_suspend > resume_mmu_done: > ldmfd sp!, {r3-r12,pc} > ENDPROC(acmeSoC_cpu_suspend) Nothing has that - because you can't execute that ldmfd after the call to cpu_suspend returns. I don't think you've understood what I said on that subject in the previous thread. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-13 12:26 ` Russell King - ARM Linux @ 2011-06-13 12:40 ` Frank Hofmann 2011-06-13 13:20 ` Frank Hofmann 1 sibling, 0 replies; 21+ messages in thread From: Frank Hofmann @ 2011-06-13 12:40 UTC (permalink / raw) To: linux-arm-kernel On Mon, 13 Jun 2011, Russell King - ARM Linux wrote: > On Mon, Jun 13, 2011 at 01:04:02PM +0100, Frank Hofmann wrote: >> To make it clear: IF AND ONLY IF your suspend(-to-ram) func looks like: >> >> ENTRY(acmeSoC_cpu_suspend) >> stmfd sp!, {r4-r12,lr} >> ldr r3, resume_mmu_done >> bl cpu_suspend >> resume_mmu_done: >> ldmfd sp!, {r3-r12,pc} >> ENDPROC(acmeSoC_cpu_suspend) > > Nothing has that - because you can't execute that ldmfd after the call > to cpu_suspend returns. I don't think you've understood what I said on > that subject in the previous thread. > Sorry typo. Missed the critical lines: ENTRY(acmeSoC_cpu_suspend) stmfd sp!, {r3-r12,lr} ldr r3, =resume_mmu_done [ load r1 with v:p, whichever way - either you or your caller ] bl cpu_suspend [ ... do whatever low power entry stuff ... ] [ function effectively ENDS HERE ... ] resume_mmu_done: ldmfd sp!, {r3-r12,pc} That's the key bits. DO NOTHING OF RELEVANCE BEFORE cpu_suspend AND DO NOTHING APART FROM ENTERING LOW POWER AFTERWARDS. I _have_ understood what you've said. cpu_suspend is not made for this purpose, and hence I'm not claiming it should be. The use of it to create the core state snapshot for hibernation is merely something that, right now, works _by accident_ (or sheer luck, if you want to say that) rather than by intent / design. Since there _is no design_ for what should work / how it should work in this context, I'm leaving it as-is, merely stating the constraints. Roll your own if you need / want to. Does that clarify ? FrankH. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-13 12:26 ` Russell King - ARM Linux 2011-06-13 12:40 ` Frank Hofmann @ 2011-06-13 13:20 ` Frank Hofmann 2011-06-13 13:56 ` Dave Martin 2011-06-13 16:44 ` Russell King - ARM Linux 1 sibling, 2 replies; 21+ messages in thread From: Frank Hofmann @ 2011-06-13 13:20 UTC (permalink / raw) To: linux-arm-kernel On Mon, 13 Jun 2011, Russell King - ARM Linux wrote: > On Mon, Jun 13, 2011 at 01:04:02PM +0100, Frank Hofmann wrote: >> To make it clear: IF AND ONLY IF your suspend(-to-ram) func looks like: >> >> ENTRY(acmeSoC_cpu_suspend) >> stmfd sp!, {r4-r12,lr} >> ldr r3, resume_mmu_done >> bl cpu_suspend >> resume_mmu_done: >> ldmfd sp!, {r3-r12,pc} >> ENDPROC(acmeSoC_cpu_suspend) > > Nothing has that - because you can't execute that ldmfd after the call > to cpu_suspend returns. I don't think you've understood what I said on > that subject in the previous thread. > Ok, to illustrate a bit more, what is ok and what not. >From bogus@does.not.exist.com Wed Jun 1 12:03:18 2011 From: bogus@does.not.exist.com () Date: Wed, 01 Jun 2011 16:03:18 -0000 Subject: No subject Message-ID: <mailman.22.1307971213.24103.linux-arm-kernel@lists.infradead.org> a) a case where the hibernation patch as I posted it is ok for core state: arch/arm/mach-exynos4/sleep.S has: ENTRY(s3c_cpu_save) stmfd sp!, { r3 - r12, lr } ldr r3, =resume_with_mmu bl cpu_suspend ldr r0, =pm_cpu_sleep ldr r0, [ r0 ] mov pc, r0 resume_with_mmu: ldmfd sp!, { r3 - r12, pc } ENTRY(s3c_cpu_resume) b cpu_resume I.e. it does nothing before but set up the arguments for cpu_suspend, and does nothing afterwards but redirect to a function that enters low power (and switches the CPU off). Likewise, all it does for resume is redirect to cpu_resume which will ultimately end up jumping to resume_with_mmu: as requested. b) a case where the hibernation patch is insufficient even though the code in the soc suspend func uses cpu_suspend: arch/arm/mach-pxa/sleep.S has: /* * pxa27x_cpu_suspend() * * Forces CPU into sleep state. * * r0 = value for PWRMODE M field for desired sleep state * r1 = v:p offset */ ENTRY(pxa27x_cpu_suspend) #ifndef CONFIG_IWMMXT mra r2, r3, acc0 #endif stmfd sp!, {r2 - r12, lr} @ save registers on stack mov r4, r0 @ save sleep mode ldr r3, =pxa_cpu_resume @ resume function bl cpu_suspend [ ... some stuff ... ] ldr r6, =CCCR ldr r8, [r6] @ keep original value for resume ldr r7, =CCCR_SLEEP @ prepare CCCR sleep value mov r0, #0x2 @ prepare value for CLKCFG @ align execution to a cache line b pxa_cpu_do_suspend #endif [ ... ] pxa_cpu_do_suspend: @ All needed values are now in registers. @ These last instructions should be in cache @ initiate the frequency change... str r7, [r6] mcr p14, 0, r0, c6, c0, 0 @ restore the original cpu speed value for resume str r8, [r6] [ ... ] /* * pxa_cpu_resume() * * entry point from bootloader into kernel during resume */ .align 5 pxa_cpu_resume: ldmfd sp!, {r2, r3} #ifndef CONFIG_IWMMXT mar acc0, r2, r3 #endif ldmfd sp!, {r4 - r12, pc} @ return to caller In this case, there's _MORE_ state saved (the CCCR / CCCR_SLEEP accesses) after the call to cpu_suspend things that aren't dealt with by the way the hibernation support patch currently operates. There also is _more_ restored at resume than what the generic part does for you. Whether these things (for example) are relevant wrt. to a hibernation resume is something I simply do not know. A third example of a soc suspend func, even more complex, with more constraints, would be as already mentioned, OMAP. On that as well the caveat applies, "what works empirically might not be correct in all cases". This hopefully illustrates what someone who tries to make suspend-to-disk work for a particular ARM SoC has to look out for, and why the patch shouldn't be blindly adopted ?! Again, it's only a starting point. An improved one compared to having ARCH-#ifdefs and huge code bloat all over swsusp.S, but not sufficient for the general case quite yet. FrankH. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-13 13:20 ` Frank Hofmann @ 2011-06-13 13:56 ` Dave Martin 2011-06-13 16:44 ` Russell King - ARM Linux 1 sibling, 0 replies; 21+ messages in thread From: Dave Martin @ 2011-06-13 13:56 UTC (permalink / raw) To: Santosh Shilimkar, Frank Hofmann Cc: Russell King - ARM Linux, Colin Cross, linux-kernel, linux-arm-kernel On Mon, Jun 13, 2011 at 02:20:12PM +0100, Frank Hofmann wrote: > > > On Mon, 13 Jun 2011, Russell King - ARM Linux wrote: > > >On Mon, Jun 13, 2011 at 01:04:02PM +0100, Frank Hofmann wrote: > >> To make it clear: IF AND ONLY IF your suspend(-to-ram) func looks like: > >> > >> ENTRY(acmeSoC_cpu_suspend) > >> stmfd sp!, {r4-r12,lr} > >> ldr r3, resume_mmu_done > >> bl cpu_suspend > >> resume_mmu_done: > >> ldmfd sp!, {r3-r12,pc} > >> ENDPROC(acmeSoC_cpu_suspend) > > > >Nothing has that - because you can't execute that ldmfd after the call > >to cpu_suspend returns. I don't think you've understood what I said on > >that subject in the previous thread. > > > > Ok, to illustrate a bit more, what is ok and what not. > > > From the current code ... > > a) a case where the hibernation patch as I posted it is ok for core state: > > arch/arm/mach-exynos4/sleep.S has: > > ENTRY(s3c_cpu_save) > > stmfd sp!, { r3 - r12, lr } > ldr r3, =resume_with_mmu > bl cpu_suspend > > ldr r0, =pm_cpu_sleep > ldr r0, [ r0 ] > mov pc, r0 > > resume_with_mmu: > ldmfd sp!, { r3 - r12, pc } > > ENTRY(s3c_cpu_resume) > b cpu_resume > > > I.e. it does nothing before but set up the arguments for > cpu_suspend, and does nothing afterwards but redirect to a function > that enters low power (and switches the CPU off). > Likewise, all it does for resume is redirect to cpu_resume which > will ultimately end up jumping to resume_with_mmu: as requested. > > > b) a case where the hibernation patch is insufficient even though the > code in the soc suspend func uses cpu_suspend: > > arch/arm/mach-pxa/sleep.S has: > > /* > * pxa27x_cpu_suspend() > * > * Forces CPU into sleep state. > * > * r0 = value for PWRMODE M field for desired sleep state > * r1 = v:p offset > */ > ENTRY(pxa27x_cpu_suspend) > > #ifndef CONFIG_IWMMXT > mra r2, r3, acc0 > #endif > stmfd sp!, {r2 - r12, lr} @ save registers on stack > mov r4, r0 @ save sleep mode > ldr r3, =pxa_cpu_resume @ resume function > bl cpu_suspend > [ ... some stuff ... ] > ldr r6, =CCCR > ldr r8, [r6] @ keep original value for resume > > ldr r7, =CCCR_SLEEP @ prepare CCCR sleep value > mov r0, #0x2 @ prepare value for CLKCFG > > @ align execution to a cache line > b pxa_cpu_do_suspend > #endif > [ ... ] > pxa_cpu_do_suspend: > > @ All needed values are now in registers. > @ These last instructions should be in cache > > @ initiate the frequency change... > str r7, [r6] > mcr p14, 0, r0, c6, c0, 0 > > @ restore the original cpu speed value for resume > str r8, [r6] > [ ... ] > /* > * pxa_cpu_resume() > * > * entry point from bootloader into kernel during resume > */ > .align 5 > pxa_cpu_resume: > ldmfd sp!, {r2, r3} > #ifndef CONFIG_IWMMXT > mar acc0, r2, r3 > #endif > ldmfd sp!, {r4 - r12, pc} @ return to caller > > > > In this case, there's _MORE_ state saved (the CCCR / CCCR_SLEEP > accesses) after the call to cpu_suspend things that aren't dealt > with by the way the hibernation support patch currently operates. > > There also is _more_ restored at resume than what the generic part > does for you. > > Whether these things (for example) are relevant wrt. to a > hibernation resume is something I simply do not know. > > > A third example of a soc suspend func, even more complex, with more > constraints, would be as already mentioned, OMAP. On that as well > the caveat applies, "what works empirically might not be correct in > all cases". Santosh, Frank: to what extent do you think the OMAP suspend code could be abstracted using something like Colin's CPU pm notifier framework? I'd hope that at least some stuff can be abstracted out, but I don't understand the OMAP code intimately enough to be certain of that... We shouldn't expect to remove absolutely all the SoC specifics from suspend code, but the more we can hook into a generic framework, the better for everyone. Cheers ---Dave ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) @ 2011-06-13 13:56 ` Dave Martin 0 siblings, 0 replies; 21+ messages in thread From: Dave Martin @ 2011-06-13 13:56 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jun 13, 2011 at 02:20:12PM +0100, Frank Hofmann wrote: > > > On Mon, 13 Jun 2011, Russell King - ARM Linux wrote: > > >On Mon, Jun 13, 2011 at 01:04:02PM +0100, Frank Hofmann wrote: > >> To make it clear: IF AND ONLY IF your suspend(-to-ram) func looks like: > >> > >> ENTRY(acmeSoC_cpu_suspend) > >> stmfd sp!, {r4-r12,lr} > >> ldr r3, resume_mmu_done > >> bl cpu_suspend > >> resume_mmu_done: > >> ldmfd sp!, {r3-r12,pc} > >> ENDPROC(acmeSoC_cpu_suspend) > > > >Nothing has that - because you can't execute that ldmfd after the call > >to cpu_suspend returns. I don't think you've understood what I said on > >that subject in the previous thread. > > > > Ok, to illustrate a bit more, what is ok and what not. > > > From the current code ... > > a) a case where the hibernation patch as I posted it is ok for core state: > > arch/arm/mach-exynos4/sleep.S has: > > ENTRY(s3c_cpu_save) > > stmfd sp!, { r3 - r12, lr } > ldr r3, =resume_with_mmu > bl cpu_suspend > > ldr r0, =pm_cpu_sleep > ldr r0, [ r0 ] > mov pc, r0 > > resume_with_mmu: > ldmfd sp!, { r3 - r12, pc } > > ENTRY(s3c_cpu_resume) > b cpu_resume > > > I.e. it does nothing before but set up the arguments for > cpu_suspend, and does nothing afterwards but redirect to a function > that enters low power (and switches the CPU off). > Likewise, all it does for resume is redirect to cpu_resume which > will ultimately end up jumping to resume_with_mmu: as requested. > > > b) a case where the hibernation patch is insufficient even though the > code in the soc suspend func uses cpu_suspend: > > arch/arm/mach-pxa/sleep.S has: > > /* > * pxa27x_cpu_suspend() > * > * Forces CPU into sleep state. > * > * r0 = value for PWRMODE M field for desired sleep state > * r1 = v:p offset > */ > ENTRY(pxa27x_cpu_suspend) > > #ifndef CONFIG_IWMMXT > mra r2, r3, acc0 > #endif > stmfd sp!, {r2 - r12, lr} @ save registers on stack > mov r4, r0 @ save sleep mode > ldr r3, =pxa_cpu_resume @ resume function > bl cpu_suspend > [ ... some stuff ... ] > ldr r6, =CCCR > ldr r8, [r6] @ keep original value for resume > > ldr r7, =CCCR_SLEEP @ prepare CCCR sleep value > mov r0, #0x2 @ prepare value for CLKCFG > > @ align execution to a cache line > b pxa_cpu_do_suspend > #endif > [ ... ] > pxa_cpu_do_suspend: > > @ All needed values are now in registers. > @ These last instructions should be in cache > > @ initiate the frequency change... > str r7, [r6] > mcr p14, 0, r0, c6, c0, 0 > > @ restore the original cpu speed value for resume > str r8, [r6] > [ ... ] > /* > * pxa_cpu_resume() > * > * entry point from bootloader into kernel during resume > */ > .align 5 > pxa_cpu_resume: > ldmfd sp!, {r2, r3} > #ifndef CONFIG_IWMMXT > mar acc0, r2, r3 > #endif > ldmfd sp!, {r4 - r12, pc} @ return to caller > > > > In this case, there's _MORE_ state saved (the CCCR / CCCR_SLEEP > accesses) after the call to cpu_suspend things that aren't dealt > with by the way the hibernation support patch currently operates. > > There also is _more_ restored at resume than what the generic part > does for you. > > Whether these things (for example) are relevant wrt. to a > hibernation resume is something I simply do not know. > > > A third example of a soc suspend func, even more complex, with more > constraints, would be as already mentioned, OMAP. On that as well > the caveat applies, "what works empirically might not be correct in > all cases". Santosh, Frank: to what extent do you think the OMAP suspend code could be abstracted using something like Colin's CPU pm notifier framework? I'd hope that at least some stuff can be abstracted out, but I don't understand the OMAP code intimately enough to be certain of that... We shouldn't expect to remove absolutely all the SoC specifics from suspend code, but the more we can hook into a generic framework, the better for everyone. Cheers ---Dave ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-13 13:56 ` Dave Martin @ 2011-06-13 15:34 ` Frank Hofmann -1 siblings, 0 replies; 21+ messages in thread From: Frank Hofmann @ 2011-06-13 15:34 UTC (permalink / raw) To: Dave Martin Cc: Santosh Shilimkar, Frank Hofmann, Russell King - ARM Linux, Colin Cross, linux-kernel, linux-arm-kernel On Mon, 13 Jun 2011, Dave Martin wrote: [ ... ] > Santosh, Frank: to what extent do you think the OMAP suspend code could > be abstracted using something like Colin's CPU pm notifier framework? > > I'd hope that at least some stuff can be abstracted out, but I don't > understand the OMAP code intimately enough to be certain of that... > > We shouldn't expect to remove absolutely all the SoC specifics from > suspend code, but the more we can hook into a generic framework, the > better for everyone. > > Cheers > ---Dave > Hi Dave, you mean the patch set from this weekend ? I've still got to read through all of that in detail (currently not getting the digest mails). There's this thing which Russell keeps on pointing out, that a SoC is more than a CPU core and even a CPU core is more than just the ARM itself ... and power management somehow needs dealing with all... To me, the picture looks a bit like: - the "inner" thing which is the ARMv[4567] control register set - the "semi-inner" thing which are SoC core control regs outside of what is generic ARMv[4567], and/or which behave differently from that due to SoC spec / errata / ... "Secure state" probably fits amongst this as well. - the "semi-outer" parts like optional ARM features (cache, VFP, Neon, ...) which still have a generic spec - the "outer" parts - builtin devices and their state, interrupt controller, clocks and regulator stuff, ... This is unfortunately simplified and the separations between these aren't quite as clean. It used to be so that as far as device drivers were dealing with suspend/resume the latter bit was done via sysdev/syscore and anything else delegated to a (SoC-specific) platform_suspend_ops entry. Russell's changes for "generic CPU suspend/resume" get the innermost bits into common code, while Colin's changes allow for more of the outer parts within common code ? Assuming I'm reading these things right, then the place where OMAP could possibly use Colin's CPU PM notifier changes would be to convert some of the code in omap_sram_idle() (the backbone for platform_suspend_ops on OMAP) to use it. To use the OMAP code for illustration (could've quoted other SoC code as well, the kind of work to be done is similar but the details of what and how it's done are very different ...): void omap_sram_idle(void) { [ ... ] omap3_per_save_context(); [ ... ] omap3_core_save_context(); omap3_cm_save_context(); [ ... ] omap3_intc_prepare_idle(); [ ... ] _omap_sram_idle(omap3_arm_context, save_state); cpu_init(); [ ... ] omap3_core_restore_context(); omap3_cm_restore_context(); omap3_sram_restore_context(); omap2_sms_restore_context(); [ ... ] omap3_intc_resume_idle(); [ ... ] omap3_per_restore_context(); [ ... ] } Aren't things like these potential candidates to convert to Colin's framework, if one chooses so ? While the place where OMAP might possibly be using Russell's generic cpu_suspend/resume were deeper down - within _omap_sram_idle. Note the _ - it's a function pointer actually evaluating to omap????_cpu_suspend. So it's kind of a two-pronged attack to minimize the SoC-specific code, Colin's framework to extract common code from the "outer" parts and Russell's cpu_suspend/resume to extract common code from the "inner" parts. Orthogonal problems / solutions ? FrankH. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) @ 2011-06-13 15:34 ` Frank Hofmann 0 siblings, 0 replies; 21+ messages in thread From: Frank Hofmann @ 2011-06-13 15:34 UTC (permalink / raw) To: linux-arm-kernel On Mon, 13 Jun 2011, Dave Martin wrote: [ ... ] > Santosh, Frank: to what extent do you think the OMAP suspend code could > be abstracted using something like Colin's CPU pm notifier framework? > > I'd hope that at least some stuff can be abstracted out, but I don't > understand the OMAP code intimately enough to be certain of that... > > We shouldn't expect to remove absolutely all the SoC specifics from > suspend code, but the more we can hook into a generic framework, the > better for everyone. > > Cheers > ---Dave > Hi Dave, you mean the patch set from this weekend ? I've still got to read through all of that in detail (currently not getting the digest mails). There's this thing which Russell keeps on pointing out, that a SoC is more than a CPU core and even a CPU core is more than just the ARM itself ... and power management somehow needs dealing with all... To me, the picture looks a bit like: - the "inner" thing which is the ARMv[4567] control register set - the "semi-inner" thing which are SoC core control regs outside of what is generic ARMv[4567], and/or which behave differently from that due to SoC spec / errata / ... "Secure state" probably fits amongst this as well. - the "semi-outer" parts like optional ARM features (cache, VFP, Neon, ...) which still have a generic spec - the "outer" parts - builtin devices and their state, interrupt controller, clocks and regulator stuff, ... This is unfortunately simplified and the separations between these aren't quite as clean. It used to be so that as far as device drivers were dealing with suspend/resume the latter bit was done via sysdev/syscore and anything else delegated to a (SoC-specific) platform_suspend_ops entry. Russell's changes for "generic CPU suspend/resume" get the innermost bits into common code, while Colin's changes allow for more of the outer parts within common code ? Assuming I'm reading these things right, then the place where OMAP could possibly use Colin's CPU PM notifier changes would be to convert some of the code in omap_sram_idle() (the backbone for platform_suspend_ops on OMAP) to use it. To use the OMAP code for illustration (could've quoted other SoC code as well, the kind of work to be done is similar but the details of what and how it's done are very different ...): void omap_sram_idle(void) { [ ... ] omap3_per_save_context(); [ ... ] omap3_core_save_context(); omap3_cm_save_context(); [ ... ] omap3_intc_prepare_idle(); [ ... ] _omap_sram_idle(omap3_arm_context, save_state); cpu_init(); [ ... ] omap3_core_restore_context(); omap3_cm_restore_context(); omap3_sram_restore_context(); omap2_sms_restore_context(); [ ... ] omap3_intc_resume_idle(); [ ... ] omap3_per_restore_context(); [ ... ] } Aren't things like these potential candidates to convert to Colin's framework, if one chooses so ? While the place where OMAP might possibly be using Russell's generic cpu_suspend/resume were deeper down - within _omap_sram_idle. Note the _ - it's a function pointer actually evaluating to omap????_cpu_suspend. So it's kind of a two-pronged attack to minimize the SoC-specific code, Colin's framework to extract common code from the "outer" parts and Russell's cpu_suspend/resume to extract common code from the "inner" parts. Orthogonal problems / solutions ? FrankH. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-13 15:34 ` Frank Hofmann @ 2011-06-13 16:11 ` Frank Hofmann -1 siblings, 0 replies; 21+ messages in thread From: Frank Hofmann @ 2011-06-13 16:11 UTC (permalink / raw) To: Frank Hofmann Cc: Dave Martin, Santosh Shilimkar, Russell King - ARM Linux, Colin Cross, linux-kernel, linux-arm-kernel On Mon, 13 Jun 2011, Frank Hofmann wrote: [ ... ] > So it's kind of a two-pronged attack to minimize the SoC-specific code, > Colin's framework to extract common code from the "outer" parts and Russell's > cpu_suspend/resume to extract common code from the "inner" parts. Also, to clarify on the hibernation side: The biggest shortfall of the hibernation support patch that I've posted is that it currently has no awareness of the "peripheral" side of things at all, i.e. all the stuff done by the platform_suspend_ops->begin/enter/end for the suspend-to-mem case isn't replicated sufficiently by the hibernation codepath. If you like, swsusp_arch_suspend / resume are on the same level of complexity as are the functions in platform_suspend_ops. But the current hibernation patch does only parts of that. It gets away with this largely because it "resumes from ON" ... As far as that is concerned, Russell really has a point, to do all this properly / well for the hibernation case, a hibernation-specific approach on the platform_ops abstraction level would be better than just to hack low-level generics together for the sake of hacking low-level generics together. If just doing per-SoC hibernation, then the code bloat problem remains, on ARM there's already platform_suspend_ops per SoC; if one adds a full set of platform_hibernation_ops per SoC without thinking where/how to consolidate, it'd only create another mess. Beware the beginnings ... I had thought the idea of using cpu_suspend / resume actually made it clear that my main point for all this "use generics wherever imaginable" is to never allow for unnecessary bloat in the first place and hence opt for generics even if somewhat imperfect / shoehorned for the general case. I'd like to apologize if that didn't come over. I'm following the work in this area; compared to where things were a year ago a lot of things have happened already. In a way, my hope is that part of all this blah of mine is helping to identify areas where code might be shared even if that starts out as coincidental (as is the current use of cpu_suspend / resume inside the hibernation patch). FrankH. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) @ 2011-06-13 16:11 ` Frank Hofmann 0 siblings, 0 replies; 21+ messages in thread From: Frank Hofmann @ 2011-06-13 16:11 UTC (permalink / raw) To: linux-arm-kernel On Mon, 13 Jun 2011, Frank Hofmann wrote: [ ... ] > So it's kind of a two-pronged attack to minimize the SoC-specific code, > Colin's framework to extract common code from the "outer" parts and Russell's > cpu_suspend/resume to extract common code from the "inner" parts. Also, to clarify on the hibernation side: The biggest shortfall of the hibernation support patch that I've posted is that it currently has no awareness of the "peripheral" side of things at all, i.e. all the stuff done by the platform_suspend_ops->begin/enter/end for the suspend-to-mem case isn't replicated sufficiently by the hibernation codepath. If you like, swsusp_arch_suspend / resume are on the same level of complexity as are the functions in platform_suspend_ops. But the current hibernation patch does only parts of that. It gets away with this largely because it "resumes from ON" ... As far as that is concerned, Russell really has a point, to do all this properly / well for the hibernation case, a hibernation-specific approach on the platform_ops abstraction level would be better than just to hack low-level generics together for the sake of hacking low-level generics together. If just doing per-SoC hibernation, then the code bloat problem remains, on ARM there's already platform_suspend_ops per SoC; if one adds a full set of platform_hibernation_ops per SoC without thinking where/how to consolidate, it'd only create another mess. Beware the beginnings ... I had thought the idea of using cpu_suspend / resume actually made it clear that my main point for all this "use generics wherever imaginable" is to never allow for unnecessary bloat in the first place and hence opt for generics even if somewhat imperfect / shoehorned for the general case. I'd like to apologize if that didn't come over. I'm following the work in this area; compared to where things were a year ago a lot of things have happened already. In a way, my hope is that part of all this blah of mine is helping to identify areas where code might be shared even if that starts out as coincidental (as is the current use of cpu_suspend / resume inside the hibernation patch). FrankH. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-13 13:20 ` Frank Hofmann 2011-06-13 13:56 ` Dave Martin @ 2011-06-13 16:44 ` Russell King - ARM Linux 2011-06-15 13:35 ` Frank Hofmann 1 sibling, 1 reply; 21+ messages in thread From: Russell King - ARM Linux @ 2011-06-13 16:44 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jun 13, 2011 at 02:20:12PM +0100, Frank Hofmann wrote: > > > On Mon, 13 Jun 2011, Russell King - ARM Linux wrote: > >> On Mon, Jun 13, 2011 at 01:04:02PM +0100, Frank Hofmann wrote: >>> To make it clear: IF AND ONLY IF your suspend(-to-ram) func looks like: >>> >>> ENTRY(acmeSoC_cpu_suspend) >>> stmfd sp!, {r4-r12,lr} >>> ldr r3, resume_mmu_done >>> bl cpu_suspend >>> resume_mmu_done: >>> ldmfd sp!, {r3-r12,pc} >>> ENDPROC(acmeSoC_cpu_suspend) >> >> Nothing has that - because you can't execute that ldmfd after the call >> to cpu_suspend returns. I don't think you've understood what I said on >> that subject in the previous thread. >> > > Ok, to illustrate a bit more, what is ok and what not. Actually, we can do something about cpu_suspend. Currently cpu_suspend is not like a normal C function - when it's called it returns normally to a bunch of code which is not expected to return. The return path is via code pointed to by 'r3'. It also corrupts a bunch of registers in ways which make it non-compliant with a C API. If we do make this complaint as a normal C-like function, it eliminates this register saving. We also swap 'lr' and 'r3', so cpu_suspend effectively only returns to following code on resume - and r3 points to the suspend code. So, this becomes: ENTRY(acmeSoC_cpu_suspend) stmfd sp!, {lr} adr r3, soc_finish_suspend bl cpu_suspend ldmfd sp!, {pc} ENDPROC(acmeSoC_cpu_suspend) soc_finish_suspend: blah blah put soc to sleep never return or even: static void soc_suspend(void) { [soc specific preparation] cpu_suspend(0, PLAT_PHYS_OFFSET - PAGE_OFFSET, soc_suspend_arg, soc_suspend_fn); [soc specific cleanup ] } where soc_suspend_fn can be either assembly or C code - but must never return. Patches will follow this evening. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-13 16:44 ` Russell King - ARM Linux @ 2011-06-15 13:35 ` Frank Hofmann 2011-06-16 21:31 ` Russell King - ARM Linux ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Frank Hofmann @ 2011-06-15 13:35 UTC (permalink / raw) To: linux-arm-kernel On Mon, 13 Jun 2011, Russell King - ARM Linux wrote: > On Mon, Jun 13, 2011 at 02:20:12PM +0100, Frank Hofmann wrote: >> >> >> On Mon, 13 Jun 2011, Russell King - ARM Linux wrote: >> >>> On Mon, Jun 13, 2011 at 01:04:02PM +0100, Frank Hofmann wrote: >>>> To make it clear: IF AND ONLY IF your suspend(-to-ram) func looks like: >>>> >>>> ENTRY(acmeSoC_cpu_suspend) >>>> stmfd sp!, {r4-r12,lr} >>>> ldr r3, resume_mmu_done >>>> bl cpu_suspend >>>> resume_mmu_done: >>>> ldmfd sp!, {r3-r12,pc} >>>> ENDPROC(acmeSoC_cpu_suspend) >>> >>> Nothing has that - because you can't execute that ldmfd after the call >>> to cpu_suspend returns. I don't think you've understood what I said on >>> that subject in the previous thread. >>> >> >> Ok, to illustrate a bit more, what is ok and what not. > > Actually, we can do something about cpu_suspend. > > Currently cpu_suspend is not like a normal C function - when it's called > it returns normally to a bunch of code which is not expected to return. > The return path is via code pointed to by 'r3'. > > It also corrupts a bunch of registers in ways which make it non-compliant > with a C API. > > If we do make this complaint as a normal C-like function, it eliminates > this register saving. We also swap 'lr' and 'r3', so cpu_suspend > effectively only returns to following code on resume - and r3 points > to the suspend code. Hi Russell, this change is perfect; with this, the hibernation support code turns into the attached. That's both better and simpler to perform a full suspend/resume cycle (via resetting in the cpu_suspend "finisher") after the snapshot image has been created, instead of shoehorning a return into this. I've tested the attached on v6 (samsung) and v7 (omap3 nonsecure - yes I know ...) kernels that have the cpu_suspend changes in you've posted Monday. It does what it's supposed to do. And yes, aware that's not enough yet, this is only an attempt to address the "how to snapshot the core state", other outstanding issues will be addressed later. It'd be great to know why this makes your toenails curl so much ... Anyway, this again shows that as far as the core SoC state is concerned, the only difference between the hibernation snapshot and the s2ram soc suspend side are what the "finisher" does: hibernate: s2ram: swsusp_save() low power self-refresh settings, ... <reset> <poweroff> Why would calling swsusp_save() not be acceptable in this context ? It's supposed to run in a restricted env (no ints, no scheduling, ...) anyway. FrankH. -------------- next part -------------- A non-text attachment was scrubbed... Name: hibernation-15Jun2011.patch Type: text/x-diff Size: 5726 bytes Desc: URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110615/b7790fe8/attachment-0001.bin> ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-15 13:35 ` Frank Hofmann @ 2011-06-16 21:31 ` Russell King - ARM Linux 2011-06-20 12:32 ` Frank Hofmann 2011-06-29 14:52 ` Matthieu CASTET 2011-07-05 12:37 ` Matthieu CASTET 2 siblings, 1 reply; 21+ messages in thread From: Russell King - ARM Linux @ 2011-06-16 21:31 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jun 15, 2011 at 02:35:26PM +0100, Frank Hofmann wrote: > this change is perfect; with this, the hibernation support code turns > into the attached. Should I assume that's an ack for all the patches? > That's both better and simpler to perform a full suspend/resume cycle > (via resetting in the cpu_suspend "finisher") after the snapshot image > has been created, instead of shoehorning a return into this. It's now not soo difficult to have an error code returned from the finisher function - the only thing we have to make sure is that the cpu_do_suspend helper just saves state and does not make any state changes. We can then do this, which makes it possible for the finisher to return, and we propagate its return value. We also ensure that a path through from cpu_resume will result in a zero return value from cpu_suspend. This isn't an invitation for people to make the S2RAM path return after they time out waiting for suspend to happen - that's potentially dangerous because in that case the suspend may happen while we're resuming devices which wouldn't be nice. diff -u b/arch/arm/kernel/sleep.S b/arch/arm/kernel/sleep.S --- b/arch/arm/kernel/sleep.S +++ b/arch/arm/kernel/sleep.S @@ -12,7 +12,6 @@ * r1 = v:p offset * r2 = suspend function arg0 * r3 = suspend function - * Note: does not return until system resumes */ ENTRY(cpu_suspend) stmfd sp!, {r4 - r11, lr} @@ -26,7 +25,7 @@ #endif mov r6, sp @ current virtual SP sub sp, sp, r5 @ allocate CPU state on stack - mov r0, sp @ save pointer + mov r0, sp @ save pointer to CPU save block add ip, ip, r1 @ convert resume fn to phys stmfd sp!, {r1, r6, ip} @ save v:p, virt SP, phys resume fn ldr r5, =sleep_save_sp @@ -55,10 +54,17 @@ #else bl __cpuc_flush_kern_all #endif + adr lr, BSYM(cpu_suspend_abort) ldmfd sp!, {r0, pc} @ call suspend fn ENDPROC(cpu_suspend) .ltorg +cpu_suspend_abort: + ldmia sp!, {r1 - r3} @ pop v:p, virt SP, phys resume fn + mov sp, r2 + ldmfd sp!, {r4 - r11, pc} +ENDPROC(cpu_suspend_abort) + /* * r0 = control register value * r1 = v:p offset (preserved by cpu_do_resume) @@ -88,6 +94,7 @@ cpu_resume_after_mmu: str r5, [r2, r4, lsl #2] @ restore old mapping mcr p15, 0, r0, c1, c0, 0 @ turn on D-cache + mov r0, #0 @ return zero on success ldmfd sp!, {r4 - r11, pc} ENDPROC(cpu_resume_after_mmu) ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-16 21:31 ` Russell King - ARM Linux @ 2011-06-20 12:32 ` Frank Hofmann 2011-06-21 14:35 ` Russell King - ARM Linux 0 siblings, 1 reply; 21+ messages in thread From: Frank Hofmann @ 2011-06-20 12:32 UTC (permalink / raw) To: linux-arm-kernel On Thu, 16 Jun 2011, Russell King - ARM Linux wrote: > On Wed, Jun 15, 2011 at 02:35:26PM +0100, Frank Hofmann wrote: >> this change is perfect; with this, the hibernation support code turns >> into the attached. > > Should I assume that's an ack for all the patches? Sorry the late reply, been away. I've only tested on s3c64xx (for s2ram and s2disk) and omap3 (s2disk only due to the omap code not having been "genericized" yet), the changes are ok there. The remainder apply and compile ok. If that's sufficient, then: Acked-by: Frank Hofmann <frank.hofmann@tomtom.com> > >> That's both better and simpler to perform a full suspend/resume cycle >> (via resetting in the cpu_suspend "finisher") after the snapshot image >> has been created, instead of shoehorning a return into this. > > It's now not soo difficult to have an error code returned from the > finisher function - the only thing we have to make sure is that > the cpu_do_suspend helper just saves state and does not make any > state changes. > > We can then do this, which makes it possible for the finisher to > return, and we propagate its return value. We also ensure that a > path through from cpu_resume will result in a zero return value > from cpu_suspend. > > This isn't an invitation for people to make the S2RAM path return > after they time out waiting for suspend to happen - that's potentially > dangerous because in that case the suspend may happen while we're > resuming devices which wouldn't be nice. You're right there's some risk that the ability to return an error here is misinterpreted as an invitation to use error returns for indicating state machine conditions. What I'm wondering about is the usecase for having an error return opportunity in this case (beyond reporting it as such). Isn't it rather so that most finishers would succeed and at best do a "BUG_ON()" at failure, because system state then isn't such to make it trivially recoverable ? For hibernation, the ability to force the entire down/up transition before writing the snapshot image out is actually very beneficial, for reliability - one knows that the device/cpu side of suspend/resume has already worked when the snapshot is being written, without having to wait for reboot/image load/resume to test that. One would want to go through suspend/resume even if the memory snapshot operation, swsusp_save, errors. A failure of swsusp_save doesn't make suspend/resume itself a failure, therefore it's desirable to propagate that return code in other ways (and keep the finisher "unconditional"). I'm not opposed to this addition as such, but I'm curious: * If an error occurs, how can the caller of cpu_suspend recover from it ? * What's the state the system is in after an error in this codepath ? * What subsystems / users would do anything else with it than BUG_ON() ? Also, consider possible errors in the SoC-specific code on the resume side; situations like a failure to perform SoC-iROM-calls for e.g. an attempted secure state restore would result in errors that can't be propagated by this mechanism; i.e. there are still failure modes which require platform-specific intervention of sorts, and platform-specific error propagation/handling, even were cpu_suspend to return error codes. FrankH. > > diff -u b/arch/arm/kernel/sleep.S b/arch/arm/kernel/sleep.S > --- b/arch/arm/kernel/sleep.S > +++ b/arch/arm/kernel/sleep.S > @@ -12,7 +12,6 @@ > * r1 = v:p offset > * r2 = suspend function arg0 > * r3 = suspend function > - * Note: does not return until system resumes > */ > ENTRY(cpu_suspend) > stmfd sp!, {r4 - r11, lr} > @@ -26,7 +25,7 @@ > #endif > mov r6, sp @ current virtual SP > sub sp, sp, r5 @ allocate CPU state on stack > - mov r0, sp @ save pointer > + mov r0, sp @ save pointer to CPU save block > add ip, ip, r1 @ convert resume fn to phys > stmfd sp!, {r1, r6, ip} @ save v:p, virt SP, phys resume fn > ldr r5, =sleep_save_sp > @@ -55,10 +54,17 @@ > #else > bl __cpuc_flush_kern_all > #endif > + adr lr, BSYM(cpu_suspend_abort) > ldmfd sp!, {r0, pc} @ call suspend fn > ENDPROC(cpu_suspend) > .ltorg > > +cpu_suspend_abort: > + ldmia sp!, {r1 - r3} @ pop v:p, virt SP, phys resume fn > + mov sp, r2 > + ldmfd sp!, {r4 - r11, pc} > +ENDPROC(cpu_suspend_abort) > + > /* > * r0 = control register value > * r1 = v:p offset (preserved by cpu_do_resume) > @@ -88,6 +94,7 @@ > cpu_resume_after_mmu: > str r5, [r2, r4, lsl #2] @ restore old mapping > mcr p15, 0, r0, c1, c0, 0 @ turn on D-cache > + mov r0, #0 @ return zero on success > ldmfd sp!, {r4 - r11, pc} > ENDPROC(cpu_resume_after_mmu) > > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-20 12:32 ` Frank Hofmann @ 2011-06-21 14:35 ` Russell King - ARM Linux 0 siblings, 0 replies; 21+ messages in thread From: Russell King - ARM Linux @ 2011-06-21 14:35 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jun 20, 2011 at 01:32:47PM +0100, Frank Hofmann wrote: > I've only tested on s3c64xx (for s2ram and s2disk) and omap3 (s2disk only > due to the omap code not having been "genericized" yet), the changes are > ok there. > The remainder apply and compile ok. > > If that's sufficient, then: > Acked-by: Frank Hofmann <frank.hofmann@tomtom.com> I've added that to the core stuff and s3c64xx. > You're right there's some risk that the ability to return an error here > is misinterpreted as an invitation to use error returns for indicating > state machine conditions. Well, an error return undoes (and probably corrupts) the state which cpu_suspend saved, so it wouldn't be useful for state machine stuff - I wonder whether we should poison the saved pointers just to reinforce this point. > What I'm wondering about is the usecase for having an error return > opportunity in this case (beyond reporting it as such). Isn't it rather > so that most finishers would succeed and at best do a "BUG_ON()" at > failure, because system state then isn't such to make it trivially > recoverable ? For s2ram, there's no reason at this point to fail. The only thing left to do at this point is to do whatever is required to cause the system to enter the low power mode. In the case where you need to place the SDRAM into self refresh mode manually, the timing of stuff after self refresh mode has been entered to the point where power is cut to the CPU is normally well defined, and if for whatever reason power doesn't get cut, there's very little which can be done to re-awake the system (you'd have to ensure that the code to do that was already in the I-cache and whatever data was required was already in registers.) And as I mentioned, if you timeout waiting for the system to power off, and you've done everything you should have done, are you sure that the system won't power off on you unexpectedly if you return with an error, which would then mean you have corrupted state saved (and possibly corrupted filesystems.) So, I don't think S2RAM would have any practical use for an error return path. It's more for hibernation. > For hibernation, the ability to force the entire down/up transition > before writing the snapshot image out is actually very beneficial, for > reliability - one knows that the device/cpu side of suspend/resume has > already worked when the snapshot is being written, without having to wait > for reboot/image load/resume to test that. It's not a test of the suspend/resume path though - device state is very different between suspending in some way followed by an immediate resume without resetting, and a real suspend, power off, resume cycle. The former doesn't really test the resume paths - a register which has been forgotten won't be found by any other way than cutting the power, and you'll only ever find that on a proper resume-from-power-off. > I'm not opposed to this addition as such, but I'm curious: > * If an error occurs, how can the caller of cpu_suspend recover from it ? If an error occurs from cpu_suspend(), then the caller needs to undo any modification to state which it did while saving, and return an error. > * What's the state the system is in after an error in this codepath ? We can arrange for another callback into CPU specific code to sort out anything they've done - iow, an error return from cpu_suspend should ensure that we have the same state present as we had at the point it was called. We currently do not need that as cpu_suspend() just saves state without modifying anything, but if we ended up with a CPU which required stuff to be modified, we'd have to undo those modifications before returning an error. > * What subsystems / users would do anything else with it than BUG_ON() ? The error code from the platform_suspend_ops ->enter method is already dealt with - by going through the resume steps before returning the error code. Whether hibernate has similar functionality I don't know. What I do think is that if hibernate needs to return an error code, we'd better do that without doing things like taking the MMU down to run the resume paths in order to do that. If we have some error to propagate, we want to propagate it back as easily as possible. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-15 13:35 ` Frank Hofmann 2011-06-16 21:31 ` Russell King - ARM Linux @ 2011-06-29 14:52 ` Matthieu CASTET 2011-06-29 15:14 ` Frank Hofmann 2011-07-05 12:37 ` Matthieu CASTET 2 siblings, 1 reply; 21+ messages in thread From: Matthieu CASTET @ 2011-06-29 14:52 UTC (permalink / raw) To: linux-arm-kernel Frank Hofmann a ?crit : > On Mon, 13 Jun 2011, Russell King - ARM Linux wrote: > > Hi Russell, > > > this change is perfect; with this, the hibernation support code turns into > the attached. > > That's both better and simpler to perform a full suspend/resume cycle (via > resetting in the cpu_suspend "finisher") after the snapshot image has been > created, instead of shoehorning a return into this. > > FrankH. > > + > +u8 __swsusp_resume_stk[PAGE_SIZE/2] __nosavedata; It look like dangerous : there is no alignment constraint, but the stack should be aligned on a 8 Bytes. Matthieu ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-29 14:52 ` Matthieu CASTET @ 2011-06-29 15:14 ` Frank Hofmann 2011-06-29 20:08 ` Will Deacon 0 siblings, 1 reply; 21+ messages in thread From: Frank Hofmann @ 2011-06-29 15:14 UTC (permalink / raw) To: linux-arm-kernel On Wed, 29 Jun 2011, Matthieu CASTET wrote: > Frank Hofmann a ?crit : >> On Mon, 13 Jun 2011, Russell King - ARM Linux wrote: >> >> Hi Russell, >> >> >> this change is perfect; with this, the hibernation support code turns into >> the attached. >> >> That's both better and simpler to perform a full suspend/resume cycle (via >> resetting in the cpu_suspend "finisher") after the snapshot image has been >> created, instead of shoehorning a return into this. >> >> FrankH. > > >> >> + >> +u8 __swsusp_resume_stk[PAGE_SIZE/2] __nosavedata; > It look like dangerous : there is no alignment constraint, but the stack should > be aligned on a 8 Bytes. Uh - sorry. I used to have both the __nosavedata and __attribute__((__aligned__(PAGE_SIZE/2))) attributes there. That must've gone lost at one point. It's an artifact of the build that things turn out ok by default; the __nosavedata forces a separate section (page), and arch/arm is linked before kernel/power (the other user of __nosavedata), hence this block, due to the way the kernel build works, ends up just fine. But as you say, not by intent / declaration. Have you seen Will Deacon's suggested kexec changes ? That keeps a "reset stack" page around, _elsewhere_, and I've been considering using that. In the end, all swsusp_arch_resume() really requires is a stack page that's guaranteed to be outside the target kernel data, thereby left alone by the restore. __nosavedata is merely one way. > > > Matthieu > Thanks, FrankH. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-29 15:14 ` Frank Hofmann @ 2011-06-29 20:08 ` Will Deacon 0 siblings, 0 replies; 21+ messages in thread From: Will Deacon @ 2011-06-29 20:08 UTC (permalink / raw) To: linux-arm-kernel Hi Frank, On Wed, Jun 29, 2011 at 04:14:23PM +0100, Frank Hofmann wrote: > Have you seen Will Deacon's suggested kexec changes ? That keeps a "reset > stack" page around, _elsewhere_, and I've been considering using that. In > the end, all swsusp_arch_resume() really requires is a stack page that's > guaranteed to be outside the target kernel data, thereby left alone by the > restore. __nosavedata is merely one way. For what it's worth, in v4 of that series (hopefully I'll post it soon) I'm starting to think about SMP kexec which necessitates that the reserved stack page is placed in a fixed location. I'm planning to memblock_reserve the page immediately below swapper for this. Will ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-15 13:35 ` Frank Hofmann 2011-06-16 21:31 ` Russell King - ARM Linux 2011-06-29 14:52 ` Matthieu CASTET @ 2011-07-05 12:37 ` Matthieu CASTET [not found] ` <2C577202CB5719438D4E9608C565CB2C01B69D7F@NL-EXC-07.intra.local> 2 siblings, 1 reply; 21+ messages in thread From: Matthieu CASTET @ 2011-07-05 12:37 UTC (permalink / raw) To: linux-arm-kernel Frank Hofmann a ?crit : > On Mon, 13 Jun 2011, Russell King - ARM Linux wrote: > >> On Mon, Jun 13, 2011 at 02:20:12PM +0100, Frank Hofmann wrote: >>> >>> On Mon, 13 Jun 2011, Russell King - ARM Linux wrote: >>> >>>> On Mon, Jun 13, 2011 at 01:04:02PM +0100, Frank Hofmann wrote: >>>>> To make it clear: IF AND ONLY IF your suspend(-to-ram) func looks like: >>>>> >>>>> ENTRY(acmeSoC_cpu_suspend) >>>>> stmfd sp!, {r4-r12,lr} >>>>> ldr r3, resume_mmu_done >>>>> bl cpu_suspend >>>>> resume_mmu_done: >>>>> ldmfd sp!, {r3-r12,pc} >>>>> ENDPROC(acmeSoC_cpu_suspend) >>>> Nothing has that - because you can't execute that ldmfd after the call >>>> to cpu_suspend returns. I don't think you've understood what I said on >>>> that subject in the previous thread. >>>> >>> Ok, to illustrate a bit more, what is ok and what not. >> Actually, we can do something about cpu_suspend. >> >> Currently cpu_suspend is not like a normal C function - when it's called >> it returns normally to a bunch of code which is not expected to return. >> The return path is via code pointed to by 'r3'. >> >> It also corrupts a bunch of registers in ways which make it non-compliant >> with a C API. >> >> If we do make this complaint as a normal C-like function, it eliminates >> this register saving. We also swap 'lr' and 'r3', so cpu_suspend >> effectively only returns to following code on resume - and r3 points >> to the suspend code. > > Hi Russell, > > > this change is perfect; with this, the hibernation support code turns into > the attached. > That's both better and simpler to perform a full suspend/resume cycle (via > resetting in the cpu_suspend "finisher") after the snapshot image has been > created, instead of shoehorning a return into this. > > > +static void notrace __swsusp_arch_restore_image(void) > +{ > + extern struct pbe *restore_pblist; > + struct pbe *pbe; > + > + cpu_switch_mm(swapper_pg_dir, &init_mm); > + > + for (pbe = restore_pblist; pbe; pbe = pbe->next) > + copy_page(pbe->orig_address, pbe->address); > + One question : isn't dangerous to modify the code where we are running ? I believe the code shouldn't change too much between the kernel that do the resume and the resumed kernel and the copy routine should fit in the instruction cache, but I want to be sure it doesn't cause any problem on recent arm cores (instruction prefetching , ...) Matthieu ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <2C577202CB5719438D4E9608C565CB2C01B69D7F@NL-EXC-07.intra.local>]
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) [not found] ` <2C577202CB5719438D4E9608C565CB2C01B69D7F@NL-EXC-07.intra.local> @ 2011-07-05 17:09 ` Matthieu CASTET 0 siblings, 0 replies; 21+ messages in thread From: Matthieu CASTET @ 2011-07-05 17:09 UTC (permalink / raw) To: linux-arm-kernel Frank Hofmann a ?crit : > -----Original Message----- > From: Matthieu CASTET [mailto:matthieu.castet at parrot.com] > Frank Hofmann a ?crit : >> [ ... ] >> > +static void notrace __swsusp_arch_restore_image(void) >> > +{ >> > + extern struct pbe *restore_pblist; >> > + struct pbe *pbe; >> > + >> > + cpu_switch_mm(swapper_pg_dir, &init_mm); >> > + >> > + for (pbe = restore_pblist; pbe; pbe = pbe->next) >> > + copy_page(pbe->orig_address, pbe->address); >> > + >> >> One question : isn't dangerous to modify the code where we are running ? >> >> I believe the code shouldn't change too much between the kernel that > do the >> resume and the resumed kernel and the copy routine should fit in the > instruction >> cache, but I want to be sure it doesn't cause any problem on recent > arm cores >> (instruction prefetching , ...) >> >> >> Matthieu > > Hi Matthieu, > > this isn't new behaviour to _this_ rev of the patch ... yes > > and yes, it is dangerous to modify code where you're running. Except > that this isn't happening in the current environment; You are modifying it by putting the same code (modulo dynamic patching on the code (ftrace, kprobe, ...)). > If you're resuming via some other > mechanism but the kernel's snapshot image loading code (and only jump > into swsusp_arch_resume to kickstart resume) then it's up to you how you > get the kernel text into place. Yes. > I've not experimented with resuming "foreign" images; how would one > create such, and bypass the checks on load ? I wasn't suggesting that. > > It's less of a problem for the copy loop itself, really, as that's > definitely cache-hot. But it's an issue for what happens _after_ the > copy loop. If the code for cpu_resume / cpu_reset is not at the places > where the resum-ing code expects it to be (i.e. if there's a mismatch in > the resuming and to-be-resumed kernels wrt. to that), then things will > jump to code nirvana. > > Why are you asking about this ? While reading the code I was surprised of that. And that there weren't any comment about it. Looking at other implementations, only x86_64 seems to need to relocate the copy code. Matthieu ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) 2011-06-13 12:04 [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) Frank Hofmann 2011-06-13 12:26 ` Russell King - ARM Linux @ 2011-09-30 7:48 ` Barry Song 1 sibling, 0 replies; 21+ messages in thread From: Barry Song @ 2011-09-30 7:48 UTC (permalink / raw) To: linux-arm-kernel Hi Frank, 2011/6/13 Frank Hofmann <frank.hofmann@tomtom.com>: > Hi, > > to make one thing clear to start with: > > This is not a proposal to integrate the hibernation feature into mainline. > > It's not ripe for that because it's unclear what exactly the differences are > between what the hibernation / S2RAM codepaths have to do, and/or how much > code sharing between these codepaths is advisable. > > > Therefore, I'm merely putting this out as-is as a reference / base for those > who want to do their own experiments with the feature. > > Prerequisites: > > * The patch requires a working cpu_reset(); for v6/v7, that's in Will > ?Deacon's patch series from last week. > > * It also requires being able to take the address of cpu_reset and > ?on MULTI_CPU configs therefore, again see Will Deacon's patch series. > > * It uses on cpu_suspend() / cpu_resume(), therefore a recent-enough > ?kernel (or a port of those to whatever you're using) is required. > > > Important / Limitations: > > A) For platforms that currently use cpu_suspend/cpu_resume in their s2ram > ?codepaths AND DO NOTHING ELSE BEFORE AND NOTHING ELSE AFTER THAT BUT TO > ?"ENTER LOW POWER", the way the hibernation state snapshot for the CPU > ?core is done by this patch is sufficient. > > ?To make it clear: IF AND ONLY IF your suspend(-to-ram) func looks like: > > ? ? ? ?ENTRY(acmeSoC_cpu_suspend) > ? ? ? ? ? ? ? ?stmfd ? sp!, {r4-r12,lr} > ? ? ? ? ? ? ? ?ldr ? ? r3, resume_mmu_done > ? ? ? ? ? ? ? ?bl ? ? ?cpu_suspend > ? ? ? ?resume_mmu_done: > ? ? ? ? ? ? ? ?ldmfd ? sp!, {r3-r12,pc} > ? ? ? ?ENDPROC(acmeSoC_cpu_suspend) > > ?then this patch is ok to provide hibernation support for your CPU core. > > ?In all other cases, item B). > > ?IN ALL CASES, read item C) below; there's more than the core to the SoC. > > B) If there's any cpu-specific / SoC-specific state that needs (re)init > ?over a suspend-to-disk/resume-from-disk cycle, this patch is incomplete > ?because it provides no hooks/code for that. > > ?This is the case e.g. for "secure" SoCs that have different sets of > ?control registers and/or different CR reg access patterns. > > ?It's also the case e.g. for SoCs with L2 caches as the activation > ?sequence there is SoC-dependent; a full off-on cycle for L2 is not done > ?by the hibernation support code. > > ?It's also the case if the SoC requires steps on wakeup _before_ the > ?"generic" parts done by cpu_suspend / cpu_resume can work correctly. > > ?(OMAP is an example of such a SoC; the patch "works" on OMAP in the > ?sense that it gets you a non-secure OMAP back from hibernation but as > ?mentioned, your mileage may vary; I for example don't know what the > ?consequences ?of not disabling / reenabling the L2 cache over cpu_reset > ?are) > > C) The current low-power handling may perform SoC-specific tasks such > ?as GPIO state handling, which is currently also not addressed by this > ?patch; the assumption is rather that device suspend/resume deals with > ?this. That is not guaranteed to be complete / sufficient - if your SoC > ?and/or your device drivers have particular needs in this area, you can > ?put hooks performing these actions into save_/restore_processor_state(). > > D) If you wish to extend this to a SoC for which the generic CPU core > ?hooks (i.e. what cpu_suspend/resume provide) are either insufficient > ?or unavailable, hook your own state snapshot mechanism into the places > ?where this code currently calls cpu_suspend and cpu_resume. The main > ?assumption by the assembly code within swusp_arch_resume() is that > ?__swsusp_arch_restore_image() returns to its caller with the stack > ?restored - how you achieve that is up to you, the patch as-is does it > ?by cpu_reset(cpu_resume) but your mileage may vary. > > > I will, at this point, not send further iterations of this patch. > > I'll answer questions about it though, if there are any. i did bring-up prima2(cortex-a9) swsusp hibernation support based on your patch. i am considering whether we can have some ways to make the resume faster. Now the resume flow is: [1] poweron -> bootloader -> kernel -> all boot and device probed -> late_init() -> loading snapshot from disk to memory -> resume devices and software Is it possible we make it: [2] poweron -> bootloader -> loading snapshot from disk to memory -> resume devices and software Then the sub-process "bootloader -> kernel -> all boot and device probed -> late_init()" will be eliminated and time to boot kernel will not be needed too. the flow [2] is more platform-related. so it is probably we can use platform_hibernation_ops to handle. but it can be generic as well. > > > Thanks for all the comments during the previous threads - learned a lot ! > > > Best regards, > FrankH. -barry ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2011-09-30 7:48 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-06-13 12:04 [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd) Frank Hofmann 2011-06-13 12:26 ` Russell King - ARM Linux 2011-06-13 12:40 ` Frank Hofmann 2011-06-13 13:20 ` Frank Hofmann 2011-06-13 13:56 ` Dave Martin 2011-06-13 13:56 ` Dave Martin 2011-06-13 15:34 ` Frank Hofmann 2011-06-13 15:34 ` Frank Hofmann 2011-06-13 16:11 ` Frank Hofmann 2011-06-13 16:11 ` Frank Hofmann 2011-06-13 16:44 ` Russell King - ARM Linux 2011-06-15 13:35 ` Frank Hofmann 2011-06-16 21:31 ` Russell King - ARM Linux 2011-06-20 12:32 ` Frank Hofmann 2011-06-21 14:35 ` Russell King - ARM Linux 2011-06-29 14:52 ` Matthieu CASTET 2011-06-29 15:14 ` Frank Hofmann 2011-06-29 20:08 ` Will Deacon 2011-07-05 12:37 ` Matthieu CASTET [not found] ` <2C577202CB5719438D4E9608C565CB2C01B69D7F@NL-EXC-07.intra.local> 2011-07-05 17:09 ` Matthieu CASTET 2011-09-30 7:48 ` Barry Song
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.