Re: [PATCH v4 2/7] OMAP3: PM: Erratum i581 support: dll kick strategy

["Peter 'p2' De Schrijver" <peter.de-schrijver@nokia.com>]

On Mon, Dec 20, 2010 at 11:23:27AM +0100, ext Jean Pihet wrote:
> On Sat, Dec 18, 2010 at 11:53 PM, Nishanth Menon <nm@ti.com> wrote:
> > From: Peter 'p2' De Schrijver <peter.de-schrijver@nokia.com>
> >
> > Erratum i581 impacts OMAP3 platforms.
> > PRCM DPLL control FSM removes SDRC_IDLEREQ before DPLL3 locks causing
> > the DPLL not to be locked at times.
> >
> > IMPORTANT:
> > *) This is not a complete workaround implementation as recommended
> > by the silicon erratum. this is a support logic for detecting lockups and
> > attempting to recover where possible and is known to provide stability
> > in multiple platforms.
> > *) This code is mostly important for inactive and retention. The ROM code
> > waits for the maximum dll lock time when resuming from off mode. So for
> > off mode this code isn't really needed.
> >
> > This should eventually get refactored as part of cleanups to sleep34xx.S
> >
> > Cc: Kevin Hilman <khilman@deeprootsystems.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> >
> > Signed-off-by: Peter 'p2' De Schrijver <peter.de-schrijver@nokia.com>
> > ---
> > (no change done, posting for completeness of the series)
> > v2: https://patchwork.kernel.org/patch/365252/
> >        typo correction- erratum, support, added comment from Peter from the
> >        thread to commit message
> > v1: http://marc.info/?l=linux-omap&m=129013172525234&w=2
> >  arch/arm/mach-omap2/sleep34xx.S |   52 +++++++++++++++++++++++++++++++++++---
> >  1 files changed, 47 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
> > index 2c20fcf..3fbd1e5 100644
> > --- a/arch/arm/mach-omap2/sleep34xx.S
> > +++ b/arch/arm/mach-omap2/sleep34xx.S
> > @@ -42,6 +42,7 @@
> >                                OMAP3430_PM_PREPWSTST)
> >  #define PM_PWSTCTRL_MPU_P      OMAP3430_PRM_BASE + MPU_MOD + OMAP2_PM_PWSTCTRL
> >  #define CM_IDLEST1_CORE_V      OMAP34XX_CM_REGADDR(CORE_MOD, CM_IDLEST1)
> > +#define CM_IDLEST_CKGEN_V      OMAP34XX_CM_REGADDR(PLL_MOD, CM_IDLEST)
> >  #define SRAM_BASE_P            0x40200000
> >  #define CONTROL_STAT           0x480022F0
> >  #define SCRATCHPAD_MEM_OFFS    0x310 /* Move this as correct place is
> > @@ -554,31 +555,67 @@ skip_l2_inval:
> >
> >  /* Make sure SDRC accesses are ok */
> >  wait_sdrc_ok:
> > +
> > +/* DPLL3 must be locked before accessing the SDRC. Maybe the HW ensures this. */
> > +       ldr     r4, cm_idlest_ckgen
> > +wait_dpll3_lock:
> > +       ldr     r5, [r4]
> > +       tst     r5, #1
> > +       beq     wait_dpll3_lock
> > +
> >         ldr     r4, cm_idlest1_core
> > +wait_sdrc_ready:
> >         ldr     r5, [r4]
> > -        and     r5, r5, #0x2
> > -        cmp     r5, #0
> > -        bne     wait_sdrc_ok
> > +        tst     r5, #0x2
> > +        bne     wait_sdrc_ready
> > +       /* allow DLL powerdown upon hw idle req */
> >         ldr     r4, sdrc_power
> >         ldr     r5, [r4]
> >         bic     r5, r5, #0x40
> >         str     r5, [r4]
> > -wait_dll_lock:
> > +is_dll_in_lock_mode:
> > +
> >         /* Is dll in lock mode? */
> >         ldr     r4, sdrc_dlla_ctrl
> >         ldr     r5, [r4]
> >         tst     r5, #0x4
> >         bxne    lr
> >         /* wait till dll locks */
> > -        ldr     r4, sdrc_dlla_status
> > +wait_dll_lock_timed:
> > +       ldr     r4, wait_dll_lock_counter
> > +       add     r4, r4, #1
> > +       str     r4, wait_dll_lock_counter
> > +       ldr     r4, sdrc_dlla_status
> > +        mov    r6, #8          /* Wait 20uS for lock */
> > +wait_dll_lock:
> > +       subs    r6, r6, #0x1
> > +       beq     kick_dll
> 
> It would be good to have more comments on the code flow here:
> - what are wait_dll_lock_counter and kick_counter used for?

For debugging and statistics. So you can find out how many times a
'kick' was needed.

> - what is the timing based on? Why 20uS for the wait time?

This is the maximum lock time of the dll according to TI for OMAP3430.

> - jumping back and forth to kick_dll and wait_dll_lock_timed is confusing.
> 
> >         ldr     r5, [r4]
> >         and     r5, r5, #0x4
> >         cmp     r5, #0x4
> >         bne     wait_dll_lock
> >         bx      lr
> >
> > +       /* disable/reenable DLL if not locked */
> > +kick_dll:
> > +       ldr     r4, sdrc_dlla_ctrl
> > +       ldr     r5, [r4]
> > +       mov     r6, r5
> > +       bic     r6, #(1<<3)     /* disable dll */
> > +       str     r6, [r4]
> > +       dsb
> > +       orr     r6, r6, #(1<<3) /* enable dll */
> > +       str     r6, [r4]
> > +       dsb
> > +       ldr     r4, kick_counter
> > +       add     r4, r4, #1
> > +       str     r4, kick_counter
> > +       b       wait_dll_lock_timed
> > +
> >  cm_idlest1_core:
> >        .word   CM_IDLEST1_CORE_V
> > +cm_idlest_ckgen:
> > +       .word   CM_IDLEST_CKGEN_V
> >  sdrc_dlla_status:
> >        .word   SDRC_DLLA_STATUS_V
> >  sdrc_dlla_ctrl:
> > @@ -615,5 +652,10 @@ control_stat:
> >        .word   CONTROL_STAT
> >  kernel_flush:
> >        .word v7_flush_dcache_all
> > +       /* these 2 words need to be at the end !!! */
> > +kick_counter:
> > +       .word   0
> > +wait_dll_lock_counter:
> > +       .word   0
> Why do they need to be at the end? Also, at the end of what do they need to be?
> 

At the end of omap34xx_cpu_suspend. As we don't know where in SRAM the
counters will be, the code accessing those counters addresses them
relative from (_omap_sram_idle + omap34xx_cpu_suspend_sz). Not sure if
this part of the code made it to linux-omap though.

Cheers,

Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html