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

[Chenhui Zhao <chenhui.zhao@freescale.com>]

On Fri, Mar 14, 2014 at 06:18:27PM -0500, Scott Wood wrote:
> 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@freescale.com>
> > > > 
> > > > 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@freescale.com>
> > > > ---
> > > >  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.

Will use a better name.

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

fsl_booke_entry_mapping.S is for 32-bit. 64-bit calls
initial_tlb_book3e() in exceptions-64e.S.

> > 
> > > > +#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?

I mean a far jump which can jump to an address which has not been prefetched in
advance. I wish the code is executed in a restricted environment (predictable code
and address).

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

No.

-Chenhui