Re: [loongson-PATCH-v1 22/27] Hibernation Support in mips system

[yanh <yanh@lemote.com>]

在 2009-05-22五的 22:01 +0900，Atsushi Nemoto写道：
> On Thu, 21 May 2009 06:11:11 +0800, wuzhangjin@gmail.com wrote:
> > --- /dev/null
> > +++ b/arch/mips/power/cpu.c
> > @@ -0,0 +1,51 @@
> > +/*
> > + * Suspend support specific for mips.
> > + *
> > + */
> > +#include <linux/mm.h>
> > +#include <asm/mipsregs.h>
> > +#include <asm/page.h>
> > +#include <linux/suspend.h>
> > +
> > +/* References to section boundaries */
> > +extern const void __nosave_begin, __nosave_end;
> > +
> > +static uint32_t saved_status;
> > +unsigned long
> > +	saved_ra,
> > +	saved_sp,
> > +	saved_fp,
> > +	saved_gp,
> > +	saved_s0,
> > +	saved_s1,
> > +	saved_s2,
> > +	saved_s3,
> > +	saved_s4,
> > +	saved_s5,
> > +	saved_s6,
> > +	saved_s7,
> > +	saved_a0,
> > +	saved_a1,
> > +	saved_a2,
> > +	saved_a3,
> > +	saved_v0,
> > +	saved_v1;
> 
> Instead of enumerating them, I would prefer something like "struct
> pt_regs saved_regs" or "unsigned long saved_regs[32]".
This implementation is referencing the x86 platform. 
Not all the 32 reigsters are needed to save. 
Maybe the whole registers needed to save can still be reduced.
> 
> > +void save_processor_state(void)
> > +{
> > +	saved_status = read_c0_status();
> > +}
> 
> No need to save/restore floating point registers?
the floating point registers are not used by kernel, for user part, they
are already saved while entering into kernel mode.
> 
> > +int pfn_is_nosave(unsigned long pfn)
> > +{
> > +	unsigned long nosave_begin_pfn = __pa(&__nosave_begin) >> PAGE_SHIFT;
> > +	unsigned long nosave_end_pfn = \
> > +		PAGE_ALIGN(__pa(&__nosave_end)) >> PAGE_SHIFT;
> 
> Unneeded backslash concatenation.
> PFN_UP(), PFN_DOWN() can be used.
> 
> > +LEAF(swsusp_arch_suspend)
> > +	PTR_LA t0, saved_ra
> > +	PTR_S ra, (t0)
> > +	PTR_LA t0, saved_sp
> > +	PTR_S sp, (t0)
> 
> The MIPS assembly language accepts symbol for PTR_S, i.e.:
> 
> 	PTR_S ra, saved_ra
> 
> Or, if you converted saved_xxx into struct or array, you can do
> something like this:
> 
> 	PTR_LA t0, saved_regs
> 	PTR_S ra, PT_R31(t0)
> 	PTR_S sp, PT_R29(t0)

> 	...
> 
> > +	j swsusp_save
> > +	nop
> 
> This nop is required?
> 
> ---
> Atsushi Nemoto
>