Re: [PATCH 9/9] powerpc/pm: support deep sleep feature on T1040

[Scott Wood]

On Wed, 2014-03-12 at 18:40 +0800, Chenhui Zhao wrote:
> On Tue, Mar 11, 2014 at 08:10:24PM -0500, Scott Wood wrote:
> > On Fri, 2014-03-07 at 12:58 +0800, Chenhui Zhao wrote:
> > > From: Zhao Chenhui <chenhui.zhao@xxxxxxxxxxxxx>
> > > 
> > > T1040 supports deep sleep feature, which can switch off most parts of
> > > the SoC when it is in deep sleep mode. This way, it becomes more
> > > energy-efficient.
> > > 
> > > The DDR controller will also be powered off in deep sleep. Therefore,
> > > the last stage (the latter part of fsl_dp_enter_low) will run without DDR
> > > access. This piece of code and related TLBs will be prefetched.
> > > 
> > > Due to the different initialization code between 32-bit and 64-bit, they
> > > have seperate resume entry and precedure.
> > > 
> > > The feature supports 32-bit and 64-bit kernel mode.
> > > 
> > > Signed-off-by: Zhao Chenhui <chenhui.zhao@xxxxxxxxxxxxx>
> > > ---
> > >  arch/powerpc/include/asm/booke_save_regs.h |    3 +
> > >  arch/powerpc/kernel/cpu_setup_fsl_booke.S  |   17 ++
> > >  arch/powerpc/kernel/head_fsl_booke.S       |   30 +++
> > >  arch/powerpc/platforms/85xx/Makefile       |    2 +-
> > >  arch/powerpc/platforms/85xx/deepsleep.c    |  201 +++++++++++++++++++
> > >  arch/powerpc/platforms/85xx/qoriq_pm.c     |   38 ++++
> > >  arch/powerpc/platforms/85xx/sleep.S        |  295 ++++++++++++++++++++++++++++
> > >  arch/powerpc/sysdev/fsl_soc.h              |    7 +
> > >  8 files changed, 592 insertions(+), 1 deletions(-)
> > >  create mode 100644 arch/powerpc/platforms/85xx/deepsleep.c
> > >  create mode 100644 arch/powerpc/platforms/85xx/sleep.S
> > > 
> > > diff --git a/arch/powerpc/include/asm/booke_save_regs.h b/arch/powerpc/include/asm/booke_save_regs.h
> > > index 87c357a..37c1f6c 100644
> > > --- a/arch/powerpc/include/asm/booke_save_regs.h
> > > +++ b/arch/powerpc/include/asm/booke_save_regs.h
> > > @@ -88,6 +88,9 @@
> > >  #define HIBERNATION_FLAG	1
> > >  #define DEEPSLEEP_FLAG		2
> > >  
> > > +#define CPLD_FLAG	1
> > > +#define FPGA_FLAG	2
> > 
> > What is this?
> 
> We have two kind of boards, QDS and RDB.
> They have different register map. Use the flag to indicate the current board is using which kind
> of register map.

CPLD versus FPGA is not a meaningful difference.  We don't care what
technology is used to implement programmable logic -- we care what
programming interface is exposed.  Customers will have their own boards
that will likely not imitate either of these programming interfaces, but
what they do have will still probably be implemented in a CPLD or FPGA.
Likewise, Freescale may have future reference boards whose CPLD/FPGA is
not compatible.

So use better naming, and structure the code so it's easy to plug in
implementations for new or custom boards.
 
> > > diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S
> > > index 20204fe..3285752 100644
> > > --- a/arch/powerpc/kernel/head_fsl_booke.S
> > > +++ b/arch/powerpc/kernel/head_fsl_booke.S
> > > @@ -162,6 +162,19 @@ _ENTRY(__early_start)
> > >  #include "fsl_booke_entry_mapping.S"
> > >  #undef ENTRY_MAPPING_BOOT_SETUP
> > >  
> > > +#if defined(CONFIG_SUSPEND) && defined(CONFIG_FSL_CORENET_RCPM)
> > > +	/* if deep_sleep_flag != 0, jump to the deep sleep resume entry */
> > > +	LOAD_REG_ADDR(r4, deep_sleep_flag)
> > > +	lwz	r3, 0(r4)
> > > +	cmpwi	r3, 0
> > > +	beq	11f
> > > +	/* clear deep_sleep_flag */
> > > +	li	r3, 0
> > > +	stw	r3, 0(r4)
> > > +	b	fsl_deepsleep_resume
> > > +11:
> > > +#endif
> > 
> > Why do you come in via the normal kernel entry, versus specifying a
> > direct entry point for deep sleep resume?  How does U-Boot even know
> > what the normal entry is when resuming?
> 
> I wish to return to a specified point (like 64-bit mode), but the code in
> fsl_booke_entry_mapping.S only can run in the first page. Because it
> only setups a temp mapping of 4KB.

Why do you need the entry mapping on 32-bit but not 64-bit?
> 
> > > +#if defined(CONFIG_SUSPEND) && defined(CONFIG_FSL_CORENET_RCPM)
> > > +_ENTRY(__entry_deep_sleep)
> > > +/*
> > > + * Bootloader will jump to here when resuming from deep sleep.
> > > + * After executing the init code in fsl_booke_entry_mapping.S,
> > > + * will jump to the real resume entry.
> > > + */
> > > +	li	r8, 1
> > > +	bl	12f
> > > +12:	mflr	r9
> > > +	addi	r9, r9, (deep_sleep_flag - 12b)
> > > +	stw	r8, 0(r9)
> > > +	b __early_start
> > > +deep_sleep_flag:
> > > +	.long	0
> > > +#endif
> > 
> > It's a bit ambiguous to say "entry_deep_sleep" when it's resuming rather
> > than entering...
> 
> How about __fsl_entry_resume?

fsl_booke_deep_sleep_resume

> > > +#define FSLDELAY(count)		\
> > > +	li	r3, (count)@l;	\
> > > +	slwi	r3, r3, 10;	\
> > > +	mtctr	r3;		\
> > > +101:	nop;			\
> > > +	bdnz	101b;
> > 
> > You don't need a namespace prefix on local macros in a non-header file.
> > 
> > Is the timebase stopped where you're calling this from?
> 
> No. My purpose is to avoid jump in the last stage of entering deep sleep.
> Jump may cause problem at that time.

"bdnz" is a jump.

What problems do you think a jump will cause?

> > You also probably want to do a "sync, readback, data dependency, isync"
> > sequence to make sure that the store has hit CCSR before you begin your
> > delay (or is a delay required at all if you do that?).
> 
> Yes. It is safer with a sync sequence.
> 
> The DDR controller need some time to signal the external DDR modules to
> enter self refresh mode.

Is it documented how much time it requires?

-Scott


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/