[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Frank Hofmann]

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.

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Russell King - ARM Linux]

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.

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Frank Hofmann]

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.

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Frank Hofmann]

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.

Re: [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Dave Martin]

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

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Dave Martin]

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

Re: [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Frank Hofmann]

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.

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Frank Hofmann]

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.

Re: [RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Frank Hofmann]

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.

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Frank Hofmann]

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.

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Russell King - ARM Linux]

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.

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Frank Hofmann]

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>

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Russell King - ARM Linux]

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)
 

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Frank Hofmann]

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

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Russell King - ARM Linux]

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.

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Matthieu CASTET]

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

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Frank Hofmann]

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.

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Will Deacon]

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

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Matthieu CASTET]

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

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Matthieu CASTET]

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

[RFC PATCH v5] ARM hibernation / suspend-to-disk (fwd)

[Barry Song]

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