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

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

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.

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.