diff mbox

OMAP3: PM: Fix freeze when scaling CORE dpll to < 83Mhz

Message ID DF4DCBD3DB270B419320B577AC0C3C8602E2EB04@zmy16exm62.ds.mot.com (mailing list archive)
State Awaiting Upstream, archived
Delegated to: Kevin Hilman
Headers show

Commit Message

Wang Limei-E12499 July 9, 2009, 11:49 p.m. UTC
Paul,

Thank you very much for copying me!   

During the investigation, Richard and Girish from TI helped, the mail
is attached. 

Rajendra, 

Yesterday night, I sent mail to paul and kevin, I also submmited below
patch along with detailed issue report and analysis. 

-#define FIXEDDELAY_MASK   (0xff << FIXEDDELAY_SHIFT)
+#define FIXEDDELAY_MASK   0x00FFFFFF


Thanks,
limei

-----Original Message-----
From: Paul Walmsley [mailto:paul@pwsan.com] 
Sent: Thursday, July 09, 2009 3:44 PM
To: Rajendra Nayak
Cc: Wang Limei-E12499; khilman@deeprootsystems.com;
linux-omap@vger.kernel.org
Subject: Re: [PATCH] OMAP3: PM: Fix freeze when scaling CORE dpll to <
83Mhz

Hi Rajendra,

On Thu, 9 Jul 2009, Rajendra Nayak wrote:

> This patch fixes a bug in the CORE dpll scaling sequence which was 
> errouneously clearing some bits in the SDRC DLLA CTRL register and 
> hence causing a freeze.
> The issue was observed only on platforms which scale CORE dpll to < 
> 83Mhz and hence program the DLL in fixed delay mode.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>

Thanks, looks good, I'll queue this up for a fixes series to rmk.

One question: I got an E-mail on this earlier today from Limei Wang
about this same issue.  Looks like we should credit Limei also in this
patch with finding the bug.  Is this acceptable to you?

- Paul

> ---
>  arch/arm/mach-omap2/sram34xx.S |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/sram34xx.S 
> b/arch/arm/mach-omap2/sram34xx.S index 481f912..9be09a7 100644
> --- a/arch/arm/mach-omap2/sram34xx.S
> +++ b/arch/arm/mach-omap2/sram34xx.S
> @@ -113,7 +113,7 @@ return_to_sdram:
>  unlock_dll:
>  	ldr	r11, omap3_sdrc_dlla_ctrl
>  	ldr	r12, [r11]
> -	and	r12, r12, #FIXEDDELAY_MASK
> +	bic	r12, r12, #FIXEDDELAY_MASK
>  	orr	r12, r12, #FIXEDDELAY_DEFAULT
>  	orr	r12, r12, #DLLIDLE_MASK
>  	str	r12, [r11]		@ (no OCP barrier needed)
> --
> 1.5.4.7
> 
> --
> 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
>
Good finding.

 

I’m not sure what motivated Paul to make this change.  Perhaps we should inquire on list.  He is an open source contractor who Nokia has under contract to make code ‘ready’ for open source.  I suspect he felt it was an optimization.  I am aware he filed some question to TI about the fixed delay value but not about the enable/disable sequence.   It has been my experience that he is quick to make changes with code purity in mind.  He does gladly take back bug fixes for those changes.  There are many cases that the TI code has arrived at a working sequence which is re-coded in open source ‘clean ups’.  Generally the code cleanness does improve but functional regressions are not uncommon.   This is part of open source maturity.

 

I’m not sure of answer to sequencing issue you raise.  Some research is necessary to find the answer.  Maybe Girish or Rajendra know off hand.

 

Regards,

Richard W.

Comments

Paul Walmsley July 10, 2009, 12:17 a.m. UTC | #1
Hello Limei,

On Fri, 10 Jul 2009, Wang Limei-E12499 wrote:

> Thank you very much for copying me!   

You're welcome.

> During the investigation, Richard and Girish from TI helped, the mail
> is attached.

We should probably add them in the patch description too.

> Rajendra, 
> 
> Yesterday night, I sent mail to paul and kevin, I also submmited below
> patch along with detailed issue report and analysis. 
> 
> -#define FIXEDDELAY_MASK   (0xff << FIXEDDELAY_SHIFT)
> +#define FIXEDDELAY_MASK   0x00FFFFFF

I prefer Rajendra's patch -- it fixes a bug accidentally 
introduced in commit 102bfe0bcb4e5985322b5f30b1321f2be1ad1c20.


- Paul

> 
> 
> Thanks,
> limei
> 
> -----Original Message-----
> From: Paul Walmsley [mailto:paul@pwsan.com] 
> Sent: Thursday, July 09, 2009 3:44 PM
> To: Rajendra Nayak
> Cc: Wang Limei-E12499; khilman@deeprootsystems.com;
> linux-omap@vger.kernel.org
> Subject: Re: [PATCH] OMAP3: PM: Fix freeze when scaling CORE dpll to <
> 83Mhz
> 
> Hi Rajendra,
> 
> On Thu, 9 Jul 2009, Rajendra Nayak wrote:
> 
> > This patch fixes a bug in the CORE dpll scaling sequence which was 
> > errouneously clearing some bits in the SDRC DLLA CTRL register and 
> > hence causing a freeze.
> > The issue was observed only on platforms which scale CORE dpll to < 
> > 83Mhz and hence program the DLL in fixed delay mode.
> > 
> > Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> 
> Thanks, looks good, I'll queue this up for a fixes series to rmk.
> 
> One question: I got an E-mail on this earlier today from Limei Wang
> about this same issue.  Looks like we should credit Limei also in this
> patch with finding the bug.  Is this acceptable to you?
> 
> - Paul
> 
> > ---
> >  arch/arm/mach-omap2/sram34xx.S |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/sram34xx.S 
> > b/arch/arm/mach-omap2/sram34xx.S index 481f912..9be09a7 100644
> > --- a/arch/arm/mach-omap2/sram34xx.S
> > +++ b/arch/arm/mach-omap2/sram34xx.S
> > @@ -113,7 +113,7 @@ return_to_sdram:
> >  unlock_dll:
> >  	ldr	r11, omap3_sdrc_dlla_ctrl
> >  	ldr	r12, [r11]
> > -	and	r12, r12, #FIXEDDELAY_MASK
> > +	bic	r12, r12, #FIXEDDELAY_MASK
> >  	orr	r12, r12, #FIXEDDELAY_DEFAULT
> >  	orr	r12, r12, #DLLIDLE_MASK
> >  	str	r12, [r11]		@ (no OCP barrier needed)
> > --
> > 1.5.4.7
> > 
> > --
> > 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
> > 
> 
> 
> 


- Paul
--
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
diff mbox

Patch

diff --git a/arch/arm/mach-omap2/sram34xx.S b/arch/arm/mach-omap2/sram34xx.S
index 91e3213..860a252 100644
--- a/arch/arm/mach-omap2/sram34xx.S
+++ b/arch/arm/mach-omap2/sram34xx.S
@@ -41,8 +41,7 @@ 
 #define SDRC_UNLOCK_DLL                        0x1

 

 /* SDRC_DLLA_CTRL bit settings */
-#define FIXEDDELAY_SHIFT               24
-#define FIXEDDELAY_MASK                        (0xff << FIXEDDELAY_SHIFT)
+#define FIXEDDELAY_MASK                        0x00FFFFFF
 #define DLLIDLE_MASK                   0x4

 

Richard and Girish, 

 

Do you agree with this? Any comments?

 

Thanks,

limei

 

________________________________

From: Wang Limei-E12499 
Sent: Wednesday, July 08, 2009 1:45 PM
To: Wang Limei-E12499; 'Woodruff, Richard'; 'Hunter, Jon'; 'Ghongdemath, Girish'; 'Nayak, Rajendra'; 'mikechan@google.com'
Cc: Sripathi Srinivas-A14759; Wang Sawsd-A24013; Falempe Jocelyn-XHP836; De Chanterac Cyril-cdlc01; WIDZER NOAH-KFQG76; Liu Haiyang-DGRW68
Subject: RE: Mem freeze at omap3_sram_configure_core_dpll in K29 open source PM

 

Girish and Ricard,

 

when unlock dll, clear SDRC_DLLA_CTRL. LOCKDLL , is DLL supposed to be disabled(SDRC_DLLA_CTRL.ENADLL) ? if it is disabled, what will happen?

 

I think when dll is unlocked, DLL should still be enabled, right?

 

thanks,

limei

 

________________________________

From: Wang Limei-E12499 
Sent: Tuesday, July 07, 2009 6:59 PM
To: Woodruff, Richard; Hunter, Jon; Ghongdemath, Girish; Nayak, Rajendra; mikechan@google.com
Cc: Sripathi Srinivas-A14759; Wang Sawsd-A24013; Falempe Jocelyn-XHP836; De Chanterac Cyril-cdlc01; WIDZER NOAH-KFQG76; Liu Haiyang-DGRW68; Wang Limei-E12499
Subject: RE: Mem freeze at omap3_sram_configure_core_dpll in K29 open source PM

 

 

Ricarhd and Girish,

 

I think you refer to  fix_dll_freeze, correct ?   It is already in sleep34xx.S.  I attached the the sleep34xx.s and sram34xx.s I am using.  will you take a close look at them? 

 

And I set the breakpoint at the return address of omap3_sram_configure_core_dpll 0xC0047FAC, and found that mem content is corrupted after returning from it. pls see attached snapshot before and after omap3_sram_configure_core_dpll is run.  Seems like unlock DLL lead to mem corruption. 

 

Again, if does not unlock DLL, no this problem, that ist the only difference between good and bad. 

 

Thanks,

limei

 

 

________________________________

From: Woodruff, Richard [mailto:r-woodruff2@ti.com] 
Sent: Monday, July 06, 2009 9:33 PM
To: Wang Limei-E12499; Hunter, Jon; Ghongdemath, Girish; Nayak, Rajendra; mikechan@google.com
Cc: Sripathi Srinivas-A14759; Wang Sawsd-A24013; Falempe Jocelyn-XHP836; De Chanterac Cyril-cdlc01; WIDZER NOAH-KFQG76; Liu Haiyang-DGRW68
Subject: RE: Mem freeze at omap3_sram_configure_core_dpll in K29 open source PM

Did you apply my force DLL to lock patch?  I think Mike did.  I didn’t see it in your code.

 

rkw

 

________________________________

From: Wang Limei-E12499 [mailto:E12499@motorola.com] 
Sent: Monday, July 06, 2009 9:23 PM
To: Wang Limei-E12499; Woodruff, Richard; Hunter, Jon; Ghongdemath, Girish; Nayak, Rajendra; mikechan@google.com
Cc: Sripathi Srinivas-A14759; Wang Sawsd-A24013; Falempe Jocelyn-XHP836; De Chanterac Cyril-cdlc01; WIDZER NOAH-KFQG76; Wang Limei-E12499; Liu Haiyang-DGRW68
Subject: RE: Mem freeze at omap3_sram_configure_core_dpll in K29 open source PM

 

More update about this issue:

 

omap3_sdrc_actim_ctrla,omap3_sdrc_actim_ctrlb,omap3_sdrc_rfr_ctrl  are not re-configured when L3 clk is changed from 160.5 to 80.25, the reason is in  current sram34xx.c/configure_sdrc, a hack was added to skip ddr timing configure, when hit this function, it returns instead of setting timing registers.  Mike, will you expain what is the problem with these code?

 

but even after removing the hack, freeze still happen. when it occur, attach LB, find that dataabt happened. pls see attached snapshot. sdrc iclk is enabled and status is accessed....  Does anybody can check if the register setting is correct after freeze happens?  In another two instances when occur happened, attached LB, PC stop at FFFFxxxx,seems around exception hander/vector, pls check the snapshot. Any idea about the data abort and unexpected instructions?  

 

BTW, Hacking unlock_dll to zero still works with below patch.

 

Cyril or Noah,

 

Will you pls spend sometime with me tomorrow to debug this issue?

 

Below is the patch I applied. 

 

diff --git a/arch/arm/mach-omap2/sram34xx.S b/arch/arm/mach-omap2/sram34xx.S
index 91e3213..9831981 100644
--- a/arch/arm/mach-omap2/sram34xx.S
+++ b/arch/arm/mach-omap2/sram34xx.S
@@ -151,11 +151,15 @@  configure_core_dpll:
        and     r12, r12, r10
        orr     r12, r12, r3, lsl #CORE_DPLL_CLKOUT_DIV_SHIFT
        str     r12, [r11]
-       ldr     r12, [r11]              @ posted-write barrier for CM
-       bx      lr
+    ldr r12, clk_stabilize_delay    @ wait for the clock to stabilise
 wait_clk_stable:
        subs    r12, r12, #1
        bne     wait_clk_stable
+    nop
+    nop
+    nop
+    nop
+    nop
        bx      lr
 enable_sdrc:
        ldr     r11, omap3_cm_iclken1_core
@@ -187,7 +191,6 @@  wait_dll_unlock:
        bne     wait_dll_unlock
        bx      lr
 configure_sdrc:
-       bx      lr /* Skip ddr timing configurations, these values are bogus */
        ldr     r11, omap3_sdrc_rfr_ctrl
        str     r0, [r11]
        ldr     r11, omap3_sdrc_actim_ctrla
@@ -221,6 +224,8 @@  omap3_sdrc_dlla_ctrl:
        .word OMAP34XX_SDRC_REGADDR(SDRC_DLLA_CTRL)
 core_m2_mask_val:
        .word 0x07FFFFFF
+clk_stabilize_delay:
+    .word 0x000000C8

 

 ENTRY(omap3_sram_configure_core_dpll_sz)
        .word   . - omap3_sram_configure_core_dpll

 

thanks,

limei

 

________________________________

From: Wang Limei-E12499 
Sent: Monday, July 06, 2009 1:09 PM
To: Woodruff, Richard; Hunter, Jon; Ghongdemath, Girish; Nayak, Rajendra; mikechan@google.com
Cc: Sripathi Srinivas-A14759; Wang Sawsd-A24013; Falempe Jocelyn-XHP836; De Chanterac Cyril-cdlc01; WIDZER NOAH-KFQG76; Wang Limei-E12499
Subject: RE: Mem freeze at omap3_sram_configure_core_dpll in K29 open source PM

Richard,

 

Thanks for the info!

 

I already applied the patch before, it did not resolve the problem.    Remove the posted-write barrier, still does not work. 

 

will you pls check the attached sram34xx.S?  what is the delay to program for clk stablization?  The clk stable delay is 103 mpu cycles when change freq from 160.5 to 80.25M, it is calculated in clock34xx.c.  Also tried 200(0xC8), same thing. 

 

/*
  * XXX This only needs to be done when the CPU frequency changes
  */
 mpurate = arm_fck.rate / CYCLES_PER_MHZ;
 c = (mpurate << SDRC_MPURATE_SCALE) >> SDRC_MPURATE_BASE_SHIFT;
 c += 1;  /* for safety */
 c *= SDRC_MPURATE_LOOPS;
 c >>= SDRC_MPURATE_SCALE;
 if (c == 0)
  c = 1;

 

diff --git a/arch/arm/mach-omap2/sram34xx.S b/arch/arm/mach-omap2/sram34xx.S
index 91e3213..db7fef5 100644
--- a/arch/arm/mach-omap2/sram34xx.S
+++ b/arch/arm/mach-omap2/sram34xx.S
@@ -97,6 +97,8 @@  ENTRY(omap3_sram_configure_core_dpll)
        blne    lock_dll
        bl      sdram_in_selfrefresh    @ put SDRAM in self refresh, idle SDRC
        bl      configure_core_dpll     @ change the DPLL3 M2 divider
+    mov r12, r5
+    bl  wait_clk_stable     @ wait for SDRC to stabilize
        bl      enable_sdrc             @ take SDRC out of idle
        cmp     r4, #SDRC_UNLOCK_DLL    @ wait for DLL status to change
        bleq    wait_dll_unlock
@@ -104,8 +106,6 @@  ENTRY(omap3_sram_configure_core_dpll)
        cmp     r7, #1                  @ if increasing SDRC clk rate,
        beq     return_to_sdram         @ return to SDRAM code, otherwise,
        bl      configure_sdrc          @ reprogram SDRC regs now
-       mov     r12, r5
-       bl      wait_clk_stable         @ wait for SDRC to stabilize
 return_to_sdram:
        isb                             @ prevent speculative exec past here
        mov     r0, #0                  @ return value
@@ -151,7 +151,6 @@  configure_core_dpll:
        and     r12, r12, r10
        orr     r12, r12, r3, lsl #CORE_DPLL_CLKOUT_DIV_SHIFT
        str     r12, [r11]
-       ldr     r12, [r11]              @ posted-write barrier for CM
        bx      lr
 wait_clk_stable:
        subs    r12, r12, #1