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