Messages in this thread | | | Date | Mon, 17 Mar 2014 18:50:32 +0800 | From | Chenhui Zhao <> | Subject | Re: [PATCH 8/9] powerpc/85xx: add save/restore functions for core registers |
| |
On Fri, Mar 14, 2014 at 06:01:45PM -0500, Scott Wood wrote: > On Wed, 2014-03-12 at 17:42 +0800, Chenhui Zhao wrote: > > On Tue, Mar 11, 2014 at 07:45:14PM -0500, Scott Wood wrote: > > > On Fri, 2014-03-07 at 12:58 +0800, Chenhui Zhao wrote: > > > > From: Wang Dongsheng <dongsheng.wang@freescale.com> > > > > > > > > Add booke_cpu_state_save() and booke_cpu_state_restore() functions which can be > > > > used to save/restore CPU's registers in the case of deep sleep and hibernation. > > > > > > > > Supported processors: E6500, E5500, E500MC, E500v2 and E500v1. > > > > > > > > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com> > > > > Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com> > > > > --- > > > > arch/powerpc/include/asm/booke_save_regs.h | 96 ++++++++ > > > > arch/powerpc/kernel/Makefile | 1 + > > > > arch/powerpc/kernel/booke_save_regs.S | 361 ++++++++++++++++++++++++++++ > > > > 3 files changed, 458 insertions(+), 0 deletions(-) > > > > create mode 100644 arch/powerpc/include/asm/booke_save_regs.h > > > > create mode 100644 arch/powerpc/kernel/booke_save_regs.S > > > > > > > > diff --git a/arch/powerpc/include/asm/booke_save_regs.h b/arch/powerpc/include/asm/booke_save_regs.h > > > > new file mode 100644 > > > > index 0000000..87c357a > > > > --- /dev/null > > > > +++ b/arch/powerpc/include/asm/booke_save_regs.h > > > > @@ -0,0 +1,96 @@ > > > > +/* > > > > + * Save/restore e500 series core registers > > > > > > Filename says booke, comment says e500. > > > > > > Filename and comment also fail to point out that this is specifically > > > for standby/suspend, not for hibernate which is implemented in > > > swsusp_booke.S/swsusp_asm64.S. > > > > Sorry for inconsistency. Will changes e500 to booke. > > Hibernation and suspend can share the code. > > Maybe they could, but AFAICT this patchset doesn't make that happen -- > and I'm not convinced that the churn would be worthwhile. Note that > swsusp_asm64.S is not just for booke, so most of that file would not be > going away if you did make such a change.
OK. Let's put Hibernation aside, and change the code just for suspend.
> > I also don't like the way it looks like booke_save_regs.S is a booke > version of ppc_save_regs.S, even though they serve different purposes > and ppc_save_regs.S is still relevant to booke. > > > > > + * Software-Use Registers > > > > + * SPRG1 0x260 (dw * 76), 64-bit need to save. > > > > + * SPRG3 0x268 (dw * 77), 32-bit need to save. > > > > > > What about "CPU and NUMA node for VDSO getcpu" on 64-bit? Currently > > > SPRG3, but it will need to change for critical interrupt support. > > > > > > > + * MMU Registers > > > > + * PID0 - PID2 0x270 ~ 0x280 (dw * 78 ~ dw * 80) > > > > > > PID1/PID2 are e500v1/v2 only -- and Linux doesn't use them outside of > > > KVM (and you're not in KVM when you're running this code). > > > > > > Are we ever going to have a non-zero PID at this point? > > > > I incline to the view that saving all registers regardless of used or > > unused. The good point is that it can be compliant to the future > > changes of the usage of registers. > > > > What do you think? > > I agree to a certain extent, but balance it with the complexity of > dealing with registers that don't exist on all booke chips. If they > don't really need to be saved, why go through the hassle of conditional > code?
I agree. For these registers, I'll check if they are really needed.
-Chenhui
| |