On Friday, May 20, 2011, Frank Hofmann wrote:
> On Fri, 20 May 2011, Dave Martin wrote:
>
> [ ... ]
> >> +/*
> >> + * Save the current CPU state before suspend / poweroff.
> >> + */
> >> +ENTRY(swsusp_arch_suspend)
> >> + ldr r0, =__swsusp_arch_ctx
> >> + mrs r1, cpsr
> >> + str r1, [r0], #4 /* CPSR */
> >> +ARM( msr cpsr_c, #SYSTEM_MODE )
> >> +THUMB( mov r2, #SYSTEM_MODE )
> >> +THUMB( msr cpsr_c, r2 )
> >
> > For Thumb-2 kernels, you can use the cps instruction to change the CPU
> > mode:
> > cps #SYSTEM_MODE
> >
> > For ARM though, this instruction is only supported for ARMv6 and above,
> > so it's best to keep the msr form for compatibility for that case.
>
> Ah, ok, no problem will make that change, looks good.
>
> Do all ARMs have cpsr / spsr as used here ? Or is that code restricted to
> ARMv5+ ? I don't have the CPU evolution history there, only got involved
> with ARM when armv6 already was out.
>
> I've simply done there what the "setmode" macro from <asm/assembler.h>
> is doing, have chosen not to include that file because it overrides "push"
> on a global scale for no good reason and that sort of thing makes me
> cringe.
>
>
> >
> >> + stm r0!, {r4-r12,lr} /* nonvolatile regs */
> >
> > Since r12 is allowed to be corrupted across a function call, we
> > probably don't need to save it.
>
> ah ok thx for clarification. Earlier iterations of the patch just saved
> r0-r14 there, "just to have it all", doing it right is best as always.
>
> >
> [ ... ]
> >> + bl __save_processor_state
> >
> > <aside>
> >
> > Structurally, we seem to have:
> >
> > swsusp_arch_suspend {
> > /* save some processor state */
> > __save_processor_state();
> >
> > swsusp_save();
> > }
> >
> > Is __save_processor_state() intended to encapsulate all the CPU-model-
> > specific state saving? Excuse my ignorance of previous conversations...
> >
> > </aside>
>
> You're right.
>
> I've attached the codechanges which implement __save/__restore... for
> TI OMAP3 and Samsung S5P64xx, to illustrate, again (that's the stuff
> referred to in the earlier mail I mentioned in first post; beware of code
> churn in there, those diffs don't readily apply to 'just any' kernel).
>
> These hooks are essentially the same as the machine-specific cpu_suspend()
> except that we substitute "r0" (the save context after the generic part)
> as target for where-to-save-the-state, and we make the funcs return
> instead of switching to OFF mode.
>
> That's what I meant with "backdoorish". A better way would be to change
> the cpu_suspend interface so that it returns instead of directly switching
> to off mode / powering down.
>
> Russell has lately been making changes in this area; the current kernels
> are a bit different here since the caller already supplies the
> state-save-buffer, while the older ones used to choose in some
> mach-specific way where to store that state.
>
> (one of the reasons why these mach-dependent enablers aren't part of this
> patch suggestion ...)
>
>
> >
> >> + pop {lr}
> >> + b swsusp_save
> >> +ENDPROC(swsusp_arch_suspend)
> >
> > I'm not too familiar with the suspend/resume stuff, so I may be asking
> > a dumb question here, but:
> >
> > Where do we save/restore r8_FIQ..r13_FIQ, r13_IRQ, r13_UND and r13_ABT?
> > (I'm assuming there's no reason to save/restore r14 or SPSR for any
> > exception mode, since we presumably aren't going to suspend/resume
> > from inside an exception handler (?))
> >
> > The exception stack pointers might conceivably be reinitialised to
> > sane values on resume, without necessarily needing to save/restore
> > them, providing my assumption in the previous paragraph is correct.
> >
> > r8_FIQ..r12_FIQ can store arbitrary state used by the FIQ handler,
> > if FIQ is in use. Can we expect any driver using FIQ to save/restore
> > this state itself, rather than doing it globally? This may be
> > reasonable.
>
> We don't need to save/restore those, because at the time the snapshot is
> taken interrupts are off and we cannot be in any trap handler either. On
> resume, the kernel that has been loaded has initialized them properly
> already.
I'm not sure if this is a safe assumption in general. We may decide to
switch to loading hibernate images from boot loaders, for example, and
it may not hold any more. Generally, please don't assume _anything_ has
been properly initialized during resume, before the image is loaded.
This has already beaten us a couple of times.
Thanks,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
