Re: [PATCH 11/12] arm: omap3: am35x: Add do_wfi routine for EMIF4 submodules

["Mark A. Greer" <mgreer@animalcreek.com>]

On Wed, Apr 11, 2012 at 04:36:25PM -0600, Paul Walmsley wrote:
> Hi
> 
> just a few comments based on a quick look
> 
> On Wed, 11 Apr 2012, Mark A. Greer wrote:

> > diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
> > index 1f62f23..22aac4c 100644
> > --- a/arch/arm/mach-omap2/sleep34xx.S
> > +++ b/arch/arm/mach-omap2/sleep34xx.S

> > @@ -391,6 +401,52 @@ wait_dll_lock_counter:
> >  ENTRY(omap3_do_wfi_sz)
> >  	.word	. - omap3_do_wfi
> >  
> > +/*
> > + * Do WFI instruction (SDRC module has EMIF4 submodule)
> > + *
> > + * This code gets copied to internal SRAM and is accessible
> > + * from both SDRAM and SRAM:
> > + * - executed from SRAM for non-off modes (omap3_emif4_do_wfi_sram),
> > + * - executed from SDRAM for OFF mode (omap3_emif4_do_wfi).
> > + */
> > +	.align	3
> > +ENTRY(omap3_emif4_do_wfi)
> > +	/* Put EMIF in self-refresh */
> 
> Hmm, I guess this comment isn't quite right; it just tells it to go to 
> self-refresh when it's "idle" ? (see below)
> 
> > +	ldr     r4, pwr_mgmt_ctrl
> > +	ldr     r5, [r4]
> > +	orr     r5, r5, #0x200
> 
> Maybe use a macro here in place of the #0x200 to indicate what it's trying 
> to set...
> 
> Do you happen to know what "idle" means in the context of SPRUGR0B Section 
> 9.2.3.4.5.1 "SDRAM Self-Refresh Mode" ?  Is it referring to 
> target module-level idle, e.g. SIdleReq; or is it referring to 
> an internal definition of idle, e.g., something like "no reads or writes 
> from/to the SDRAM" ?  Am wondering if this should just be set once during 
> EMIF initialization.

I can't tell from tne manual.  I assumed it meant the latter of the choices
you listed.  I just tested a kernel with it set up during EMIF init (value
of 0x80000200) and removed the related bits from omap3_emif4_do_wfi() and
it booted and returned from suspend-to-RAM just fine.  So, it seems that
it can be moved to EMIF init.

[I swear I tested this some time ago and discovered that it was needed
in omap3_emif4_do_wfi().  Oh well...]

> 
> > +	str     r5, [r4]
> 
> Might be good to do a readback after this to ensure that the store has 
> made it to the EMIF.
> 
> Also, scanning SPRUGR0B, it looks like this setting is dependent partially 
> on the REG_PM_TIM field in the same register.  Might be good to set 
> REG_PM_TIM during EMIF init to avoid any bootloader dependency.

True but moot now since I'll move the code to EMIF init.

> > +
> > +	dsb
> > +	dmb
> > +
> > +	wfi
> > +
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> 
> Are these NOPs necessary?  Is the number of NOPs dependent on CPU or bus 
> speed or some ARM parameter?

After looking through the am35x errata, I don't think so.  AFAICT,
they are there to work around--from omap3535 errata (sprz278f.pdf)--
Advisory 3.1.1.75, "IVA2: CAM/SGX Dependencies (OMAP3530/25 only)".
I don't see that or any other errata that requires a nop in the
am35x errata so I'll get rid of them.

[As I went through the errata again, I noticed talk about the device
going into RET state.  Sigh...]

Mark
--