[PATCH] OMAP3: DVFS: No sdrc AC timing changes during core dvfs

[PATCH] OMAP3: DVFS: No sdrc AC timing changes during core dvfs

[Rajendra Nayak]

This patch removes the SDRC AC timings changes done during core dvfs.
Updating AC timing CTRL values for SDRC during DVFS is seen to be a risk,
while the RFR CTRL value is safe to be updated.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/clock34xx.c        |   17 +++++++-----
 arch/arm/mach-omap2/sram34xx.S         |   44 +------------------------------
 arch/arm/plat-omap/include/mach/sram.h |    6 +---
 arch/arm/plat-omap/sram.c              |   18 ++++--------
 4 files changed, 20 insertions(+), 65 deletions(-)

diff --git a/arch/arm/mach-omap2/clock34xx.c b/arch/arm/mach-omap2/clock34xx.c
index d6b7eb6..6210200 100644
--- a/arch/arm/mach-omap2/clock34xx.c
+++ b/arch/arm/mach-omap2/clock34xx.c
@@ -893,19 +893,22 @@ static int omap3_core_dpll_m2_set_rate(struct clk *clk, unsigned long rate)
 		 sdrc_cs1->rfr_ctrl, sdrc_cs1->actim_ctrla,
 		 sdrc_cs1->actim_ctrlb, sdrc_cs1->mr);
 
+	/*
+	 * Only the SDRC RFRCTRL value is seen to be safe to be
+	 * changed during dvfs.
+	 * The ACTiming values are left unchanged and should be
+	 * the ones programmed by the bootloader for higher OPP.
+	 */
 	if (sdrc_cs1)
 		omap3_configure_core_dpll(
 				  new_div, unlock_dll, c, rate > clk->rate,
-				  sdrc_cs0->rfr_ctrl, sdrc_cs0->actim_ctrla,
-				  sdrc_cs0->actim_ctrlb, sdrc_cs0->mr,
-				  sdrc_cs1->rfr_ctrl, sdrc_cs1->actim_ctrla,
-				  sdrc_cs1->actim_ctrlb, sdrc_cs1->mr);
+				  sdrc_cs0->rfr_ctrl, sdrc_cs0->mr,
+				  sdrc_cs1->rfr_ctrl, sdrc_cs1->mr);
 	else
 		omap3_configure_core_dpll(
 				  new_div, unlock_dll, c, rate > clk->rate,
-				  sdrc_cs0->rfr_ctrl, sdrc_cs0->actim_ctrla,
-				  sdrc_cs0->actim_ctrlb, sdrc_cs0->mr,
-				  0, 0, 0, 0);
+				  sdrc_cs0->rfr_ctrl, sdrc_cs0->mr,
+				  0, 0);
 
 	return 0;
 }
diff --git a/arch/arm/mach-omap2/sram34xx.S b/arch/arm/mach-omap2/sram34xx.S
index 82aa4a3..fc84801 100644
--- a/arch/arm/mach-omap2/sram34xx.S
+++ b/arch/arm/mach-omap2/sram34xx.S
@@ -83,12 +83,8 @@
  *  before use by the code in SRAM (SDRAM is not accessible during SDRC
  *  reconfiguration):
  *  new SDRC_RFR_CTRL_0 register contents
- *  new SDRC_ACTIM_CTRL_A_0 register contents
- *  new SDRC_ACTIM_CTRL_B_0 register contents
  *  new SDRC_MR_0 register value
  *  new SDRC_RFR_CTRL_1 register contents
- *  new SDRC_ACTIM_CTRL_A_1 register contents
- *  new SDRC_ACTIM_CTRL_B_1 register contents
  *  new SDRC_MR_1 register value
  *
  * If the param SDRC_RFR_CTRL_1 is 0, the parameters
@@ -102,20 +98,12 @@ ENTRY(omap3_sram_configure_core_dpll)
 	ldr	r4, [sp, #52]
 	str     r4, omap_sdrc_rfr_ctrl_0_val
 	ldr	r4, [sp, #56]
-	str     r4, omap_sdrc_actim_ctrl_a_0_val
-	ldr	r4, [sp, #60]
-	str     r4, omap_sdrc_actim_ctrl_b_0_val
-	ldr	r4, [sp, #64]
 	str     r4, omap_sdrc_mr_0_val
-	ldr	r4, [sp, #68]
+	ldr	r4, [sp, #60]
 	str     r4, omap_sdrc_rfr_ctrl_1_val
 	cmp	r4, #0			@ if SDRC_RFR_CTRL_1 is 0,
 	beq	skip_cs1_params		@  do not use cs1 params
-	ldr	r4, [sp, #72]
-	str     r4, omap_sdrc_actim_ctrl_a_1_val
-	ldr	r4, [sp, #76]
-	str     r4, omap_sdrc_actim_ctrl_b_1_val
-	ldr	r4, [sp, #80]
+	ldr	r4, [sp, #64]
 	str     r4, omap_sdrc_mr_1_val
 skip_cs1_params:
 	dsb				@ flush buffered writes to interconnect
@@ -219,12 +207,6 @@ configure_sdrc:
 	ldr	r12, omap_sdrc_rfr_ctrl_0_val	@ fetch value from SRAM
 	ldr	r11, omap3_sdrc_rfr_ctrl_0	@ fetch addr from SRAM
 	str	r12, [r11]			@ store
-	ldr	r12, omap_sdrc_actim_ctrl_a_0_val
-	ldr	r11, omap3_sdrc_actim_ctrl_a_0
-	str	r12, [r11]
-	ldr	r12, omap_sdrc_actim_ctrl_b_0_val
-	ldr	r11, omap3_sdrc_actim_ctrl_b_0
-	str	r12, [r11]
 	ldr	r12, omap_sdrc_mr_0_val
 	ldr	r11, omap3_sdrc_mr_0
 	str	r12, [r11]
@@ -233,12 +215,6 @@ configure_sdrc:
 	beq	skip_cs1_prog		@  do not program cs1 params
 	ldr	r11, omap3_sdrc_rfr_ctrl_1
 	str	r12, [r11]
-	ldr	r12, omap_sdrc_actim_ctrl_a_1_val
-	ldr	r11, omap3_sdrc_actim_ctrl_a_1
-	str	r12, [r11]
-	ldr	r12, omap_sdrc_actim_ctrl_b_1_val
-	ldr	r11, omap3_sdrc_actim_ctrl_b_1
-	str	r12, [r11]
 	ldr	r12, omap_sdrc_mr_1_val
 	ldr	r11, omap3_sdrc_mr_1
 	str	r12, [r11]
@@ -259,14 +235,6 @@ omap3_sdrc_rfr_ctrl_0:
 	.word OMAP34XX_SDRC_REGADDR(SDRC_RFR_CTRL_0)
 omap3_sdrc_rfr_ctrl_1:
 	.word OMAP34XX_SDRC_REGADDR(SDRC_RFR_CTRL_1)
-omap3_sdrc_actim_ctrl_a_0:
-	.word OMAP34XX_SDRC_REGADDR(SDRC_ACTIM_CTRL_A_0)
-omap3_sdrc_actim_ctrl_a_1:
-	.word OMAP34XX_SDRC_REGADDR(SDRC_ACTIM_CTRL_A_1)
-omap3_sdrc_actim_ctrl_b_0:
-	.word OMAP34XX_SDRC_REGADDR(SDRC_ACTIM_CTRL_B_0)
-omap3_sdrc_actim_ctrl_b_1:
-	.word OMAP34XX_SDRC_REGADDR(SDRC_ACTIM_CTRL_B_1)
 omap3_sdrc_mr_0:
 	.word OMAP34XX_SDRC_REGADDR(SDRC_MR_0)
 omap3_sdrc_mr_1:
@@ -275,14 +243,6 @@ omap_sdrc_rfr_ctrl_0_val:
 	.word 0xDEADBEEF
 omap_sdrc_rfr_ctrl_1_val:
 	.word 0xDEADBEEF
-omap_sdrc_actim_ctrl_a_0_val:
-	.word 0xDEADBEEF
-omap_sdrc_actim_ctrl_a_1_val:
-	.word 0xDEADBEEF
-omap_sdrc_actim_ctrl_b_0_val:
-	.word 0xDEADBEEF
-omap_sdrc_actim_ctrl_b_1_val:
-	.word 0xDEADBEEF
 omap_sdrc_mr_0_val:
 	.word 0xDEADBEEF
 omap_sdrc_mr_1_val:
diff --git a/arch/arm/plat-omap/include/mach/sram.h b/arch/arm/plat-omap/include/mach/sram.h
index 16a1b45..52e3cec 100644
--- a/arch/arm/plat-omap/include/mach/sram.h
+++ b/arch/arm/plat-omap/include/mach/sram.h
@@ -23,10 +23,8 @@ extern u32 omap2_set_prcm(u32 dpll_ctrl_val, u32 sdrc_rfr_val, int bypass);
 
 extern u32 omap3_configure_core_dpll(
 			u32 m2, u32 unlock_dll, u32 f, u32 inc,
-			u32 sdrc_rfr_ctrl_0, u32 sdrc_actim_ctrl_a_0,
-			u32 sdrc_actim_ctrl_b_0, u32 sdrc_mr_0,
-			u32 sdrc_rfr_ctrl_1, u32 sdrc_actim_ctrl_a_1,
-			u32 sdrc_actim_ctrl_b_1, u32 sdrc_mr_1);
+			u32 sdrc_rfr_ctrl_0, u32 sdrc_mr_0,
+			u32 sdrc_rfr_ctrl_1, u32 sdrc_mr_1);
 extern void omap3_sram_restore_context(void);
 
 /* Do not use these */
diff --git a/arch/arm/plat-omap/sram.c b/arch/arm/plat-omap/sram.c
index f2b0fa6..3705387 100644
--- a/arch/arm/plat-omap/sram.c
+++ b/arch/arm/plat-omap/sram.c
@@ -375,24 +375,18 @@ static inline int omap243x_sram_init(void)
 
 static u32 (*_omap3_sram_configure_core_dpll)(
 			u32 m2, u32 unlock_dll, u32 f, u32 inc,
-			u32 sdrc_rfr_ctrl_0, u32 sdrc_actim_ctrl_a_0,
-			u32 sdrc_actim_ctrl_b_0, u32 sdrc_mr_0,
-			u32 sdrc_rfr_ctrl_1, u32 sdrc_actim_ctrl_a_1,
-			u32 sdrc_actim_ctrl_b_1, u32 sdrc_mr_1);
+			u32 sdrc_rfr_ctrl_0, u32 sdrc_mr_0,
+			u32 sdrc_rfr_ctrl_1, u32 sdrc_mr_1);
 
 u32 omap3_configure_core_dpll(u32 m2, u32 unlock_dll, u32 f, u32 inc,
-			u32 sdrc_rfr_ctrl_0, u32 sdrc_actim_ctrl_a_0,
-			u32 sdrc_actim_ctrl_b_0, u32 sdrc_mr_0,
-			u32 sdrc_rfr_ctrl_1, u32 sdrc_actim_ctrl_a_1,
-			u32 sdrc_actim_ctrl_b_1, u32 sdrc_mr_1)
+			u32 sdrc_rfr_ctrl_0, u32 sdrc_mr_0,
+			u32 sdrc_rfr_ctrl_1, u32 sdrc_mr_1)
 {
 	BUG_ON(!_omap3_sram_configure_core_dpll);
 	return _omap3_sram_configure_core_dpll(
 			m2, unlock_dll, f, inc,
-			sdrc_rfr_ctrl_0, sdrc_actim_ctrl_a_0,
-			sdrc_actim_ctrl_b_0, sdrc_mr_0,
-			sdrc_rfr_ctrl_1, sdrc_actim_ctrl_a_1,
-			sdrc_actim_ctrl_b_1, sdrc_mr_1);
+			sdrc_rfr_ctrl_0, sdrc_mr_0,
+			sdrc_rfr_ctrl_1, sdrc_mr_1);
 }
 
 #ifdef CONFIG_PM
-- 
1.5.4.7

Re: [PATCH] OMAP3: DVFS: No sdrc AC timing changes during core dvfs

[Paul Walmsley]

Hi,

On Tue, 6 Oct 2009, Rajendra Nayak wrote:

> This patch removes the SDRC AC timings changes done during core dvfs.
> Updating AC timing CTRL values for SDRC during DVFS is seen to be a risk,
> while the RFR CTRL value is safe to be updated.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>

just wanted to let you know, I haven't forgotten about this one. Am trying 
to get an answer from others as to whether AC timing register changes are 
completely unsafe, in which case your patch is probably the best way to 
move forward; or whether we can make the changes when no initiators are 
touching SDRAM.


- Paul

RE: [PATCH] OMAP3: DVFS: No sdrc AC timing changes during core dvfs

[Woodruff, Richard]

> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Paul Walmsley
> Sent: Tuesday, October 20, 2009 8:21 PM

> > This patch removes the SDRC AC timings changes done during core dvfs.
> > Updating AC timing CTRL values for SDRC during DVFS is seen to be a risk,
> > while the RFR CTRL value is safe to be updated.
> >
> > Signed-off-by: Rajendra Nayak <rnayak@ti.com>
>
> just wanted to let you know, I haven't forgotten about this one. Am trying
> to get an answer from others as to whether AC timing register changes are
> completely unsafe, in which case your patch is probably the best way to
> move forward; or whether we can make the changes when no initiators are
> touching SDRAM.

It is not guaranteed safe to write actim registers on the fly to an active part.  It is safe to do RFR as it is buffered and sent out at a safe time.

A few years back before omap3 as part of omap2 lessons learned this issue was highlighted but design was not able to change.  Recently implementation review at rtl provided the above points.

I'm not aware of anyone seeing a crash but recommendation for safety is to not.

Regards,
Richard W.

RE: [PATCH] OMAP3: DVFS: No sdrc AC timing changes during core dvfs

[Cousson, Benoit]

Hi Richard,

>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>owner@vger.kernel.org] On Behalf Of Woodruff, Richard
>
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Paul Walmsley
>> Sent: Tuesday, October 20, 2009 8:21 PM
>
>> > This patch removes the SDRC AC timings changes done during core dvfs.
>> > Updating AC timing CTRL values for SDRC during DVFS is seen to be a
>risk,
>> > while the RFR CTRL value is safe to be updated.
>> >
>> > Signed-off-by: Rajendra Nayak <rnayak@ti.com>
>>
>> just wanted to let you know, I haven't forgotten about this one. Am
>trying
>> to get an answer from others as to whether AC timing register changes are
>> completely unsafe, in which case your patch is probably the best way to
>> move forward; or whether we can make the changes when no initiators are
>> touching SDRAM.
>
>It is not guaranteed safe to write actim registers on the fly to an active
>part.  It is safe to do RFR as it is buffered and sent out at a safe time.

I think that what Paul is suggesting is to change the ACTIM after ensuring that the DDR is in self-refresh. In that case it is perfectly valid; it is just tricky to ensure that all initiators are in mute before doing that.

Regards,
Benoit

>A few years back before omap3 as part of omap2 lessons learned this issue
>was highlighted but design was not able to change.  Recently implementation
>review at rtl provided the above points.
>
>I'm not aware of anyone seeing a crash but recommendation for safety is to
>not.
>
>Regards,
>Richard W.
>
>--
>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


Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

RE: [PATCH] OMAP3: DVFS: No sdrc AC timing changes during core dvfs

[Woodruff, Richard]

Hi Beonit,

> From: Cousson, Benoit
> Sent: Thursday, October 22, 2009 3:59 AM
> To: Woodruff, Richard; Paul Walmsley; Nayak, Rajendra

> >It is not guaranteed safe to write actim registers on the fly to an active
> >part.  It is safe to do RFR as it is buffered and sent out at a safe time.
>
> I think that what Paul is suggesting is to change the ACTIM after ensuring
> that the DDR is in self-refresh. In that case it is perfectly valid; it is
> just tricky to ensure that all initiators are in mute before doing that.

Yes that is true, however, ...

That is not the way the code is setup and it’s a long way from that.  Run time pause of drivers is a minefield.

With out a broad notifier they best you would hope for is some opportunistic change time (against full system).  You wouldn't have a guaranteed way to speed them up again where you need them most at high opp.

Some minimal kernel not using dma and the like could do it but that is probably more misleading to put in the tree than something useful for most.

Regards,
Richard W.

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

RE: [PATCH] OMAP3: DVFS: No sdrc AC timing changes during core dvfs

[Tero.Kristo]

Hi,

Is there any update on this chain seeing this has been pending for a month now?

-Tero 

>-----Original Message-----
>From: linux-omap-owner@vger.kernel.org 
>[mailto:linux-omap-owner@vger.kernel.org] On Behalf Of ext 
>Woodruff, Richard
>Sent: 22 October, 2009 15:20
>To: Cousson, Benoit; Paul Walmsley; Nayak, Rajendra
>Cc: linux-omap@vger.kernel.org
>Subject: RE: [PATCH] OMAP3: DVFS: No sdrc AC timing changes 
>during core dvfs
>
>Hi Beonit,
>
>> From: Cousson, Benoit
>> Sent: Thursday, October 22, 2009 3:59 AM
>> To: Woodruff, Richard; Paul Walmsley; Nayak, Rajendra
>
>> >It is not guaranteed safe to write actim registers on the 
>fly to an active
>> >part.  It is safe to do RFR as it is buffered and sent out 
>at a safe time.
>>
>> I think that what Paul is suggesting is to change the ACTIM 
>after ensuring
>> that the DDR is in self-refresh. In that case it is 
>perfectly valid; it is
>> just tricky to ensure that all initiators are in mute before 
>doing that.
>
>Yes that is true, however, ...
>
>That is not the way the code is setup and it's a long way from 
>that.  Run time pause of drivers is a minefield.
>
>With out a broad notifier they best you would hope for is some 
>opportunistic change time (against full system).  You wouldn't 
>have a guaranteed way to speed them up again where you need 
>them most at high opp.
>
>Some minimal kernel not using dma and the like could do it but 
>that is probably more misleading to put in the tree than 
>something useful for most.
>
>Regards,
>Richard W.
>
>--
>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
>

[PATCH] OMAP3: SDRC: Comment out SDRC AC timing and MR changes in CORE DVFS SRAM code

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3720 bytes --]


The code that reprograms the SDRC memory controller during CORE DVFS,
mach-omap2/sram34xx.S:omap3_sram_configure_core_dpll(), does not
ensure that all L3 initiators are prevented from accessing the SDRAM
before modifying the SDRC AC timing and MR registers.  This can cause
memory to be corrupted or cause the SDRC to enter an unpredictable
state.  This patch comments out that code for now and adds a note
explaining what is going on.  Ideally it can be added back in once
supporting code is present to ensure that other initiators aren't
touching the SDRAM.  At the very least, these registers should be
reprogrammable during kernel init to deal with buggy bootloaders.

This is a modification of a patch originally written by Rajendra Nayak
<rnayak@ti.com> (the original is at http://patchwork.kernel.org/patch/51927/).
Rather than removing the code completely, this patch just comments it out.

Thanks to Benoît Cousson <b-cousson@ti.com> and Christophe Sucur
<c-sucur@ti.com> for explaining the technical basis for this and for
explaining what can be done to make this path work in future code.
Thanks to Richard Woodruff <r-woodruff2@ti.com> for his comments.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Christophe Sucur <c-sucur@ti.com>
Cc: Benoît Cousson <b-cousson@ti.com>
Cc: Richard Woodruff <r-woodruff2@ti.com>
---
 arch/arm/mach-omap2/sram34xx.S |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/sram34xx.S b/arch/arm/mach-omap2/sram34xx.S
index 82aa4a3..8fa8955 100644
--- a/arch/arm/mach-omap2/sram34xx.S
+++ b/arch/arm/mach-omap2/sram34xx.S
@@ -91,8 +91,18 @@
  *  new SDRC_ACTIM_CTRL_B_1 register contents
  *  new SDRC_MR_1 register value
  *
- * If the param SDRC_RFR_CTRL_1 is 0, the parameters
- *  are not programmed into the SDRC CS1 registers
+ * If the param SDRC_RFR_CTRL_1 is 0, the parameters are not programmed into
+ * the SDRC CS1 registers
+ *
+ * NOTE: This code no longer attempts to program the SDRC AC timing and MR
+ * registers.  This is because the code currently cannot ensure that all
+ * L3 initiators (e.g., sDMA, IVA, DSS DISPC, etc.) are not accessing the
+ * SDRAM when the registers are written.  If the registers are changed while
+ * an initiator is accessing SDRAM, memory can be corrupted and/or the SDRC
+ * may enter an unpredictable state.  The code to reprogram the registers,
+ * however, has been left in -- commented out in "#if 0" .. "#endif" blocks --
+ * since in the future, the intent is to re-enable this code in cases where we
+ * can ensure that no initiators are touching the SDRAM.
  */
 ENTRY(omap3_sram_configure_core_dpll)
 	stmfd	sp!, {r1-r12, lr}	@ store regs to stack
@@ -219,6 +229,7 @@ configure_sdrc:
 	ldr	r12, omap_sdrc_rfr_ctrl_0_val	@ fetch value from SRAM
 	ldr	r11, omap3_sdrc_rfr_ctrl_0	@ fetch addr from SRAM
 	str	r12, [r11]			@ store
+#if 0
 	ldr	r12, omap_sdrc_actim_ctrl_a_0_val
 	ldr	r11, omap3_sdrc_actim_ctrl_a_0
 	str	r12, [r11]
@@ -228,11 +239,13 @@ configure_sdrc:
 	ldr	r12, omap_sdrc_mr_0_val
 	ldr	r11, omap3_sdrc_mr_0
 	str	r12, [r11]
+#endif
 	ldr	r12, omap_sdrc_rfr_ctrl_1_val
 	cmp	r12, #0			@ if SDRC_RFR_CTRL_1 is 0,
 	beq	skip_cs1_prog		@  do not program cs1 params
 	ldr	r11, omap3_sdrc_rfr_ctrl_1
 	str	r12, [r11]
+#if 0
 	ldr	r12, omap_sdrc_actim_ctrl_a_1_val
 	ldr	r11, omap3_sdrc_actim_ctrl_a_1
 	str	r12, [r11]
@@ -242,6 +255,7 @@ configure_sdrc:
 	ldr	r12, omap_sdrc_mr_1_val
 	ldr	r11, omap3_sdrc_mr_1
 	str	r12, [r11]
+#endif
 skip_cs1_prog:
 	ldr	r12, [r11]		@ posted-write barrier for SDRC
 	bx	lr
-- 
1.6.5.GIT

Re: [PATCH] OMAP3: SDRC: Comment out SDRC AC timing and MR changes in CORE DVFS SRAM code

[Menon, Nishanth]

Paul Walmsley said the following on 12/02/2009 03:33 AM:
> The code that reprograms the SDRC memory controller during CORE DVFS,
> mach-omap2/sram34xx.S:omap3_sram_configure_core_dpll(), does not
> ensure that all L3 initiators are prevented from accessing the SDRAM
> before modifying the SDRC AC timing and MR registers.  This can cause
> memory to be corrupted or cause the SDRC to enter an unpredictable
> state.  This patch comments out that code for now and adds a note
> explaining what is going on.  Ideally it can be added back in once
> supporting code is present to ensure that other initiators aren't
> touching the SDRAM.  At the very least, these registers should be
> reprogrammable during kernel init to deal with buggy bootloaders.
>
> This is a modification of a patch originally written by Rajendra Nayak
> <rnayak@ti.com> (the original is at http://patchwork.kernel.org/patch/51927/).
> Rather than removing the code completely, this patch just comments it out.
>   
why not make this a #ifdef instead if we need it some other time, a #if
0 and it's intention might not be readable without doing a git annotate
in a few months time..
> Thanks to Benoît Cousson <b-cousson@ti.com> and Christophe Sucur
> <c-sucur@ti.com> for explaining the technical basis for this and for
> explaining what can be done to make this path work in future code.
> Thanks to Richard Woodruff <r-woodruff2@ti.com> for his comments.
>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Rajendra Nayak <rnayak@ti.com>
> Cc: Christophe Sucur <c-sucur@ti.com>
> Cc: Benoît Cousson <b-cousson@ti.com>
> Cc: Richard Woodruff <r-woodruff2@ti.com>
> ---
>  arch/arm/mach-omap2/sram34xx.S |   18 ++++++++++++++++--
>  1 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/sram34xx.S b/arch/arm/mach-omap2/sram34xx.S
> index 82aa4a3..8fa8955 100644
> --- a/arch/arm/mach-omap2/sram34xx.S
> +++ b/arch/arm/mach-omap2/sram34xx.S
> @@ -91,8 +91,18 @@
>   *  new SDRC_ACTIM_CTRL_B_1 register contents
>   *  new SDRC_MR_1 register value
>   *
> - * If the param SDRC_RFR_CTRL_1 is 0, the parameters
> - *  are not programmed into the SDRC CS1 registers
> + * If the param SDRC_RFR_CTRL_1 is 0, the parameters are not programmed into
> + * the SDRC CS1 registers
> + *
> + * NOTE: This code no longer attempts to program the SDRC AC timing and MR
> + * registers.  This is because the code currently cannot ensure that all
> + * L3 initiators (e.g., sDMA, IVA, DSS DISPC, etc.) are not accessing the
> + * SDRAM when the registers are written.  If the registers are changed while
> + * an initiator is accessing SDRAM, memory can be corrupted and/or the SDRC
> + * may enter an unpredictable state.  The code to reprogram the registers,
> + * however, has been left in -- commented out in "#if 0" .. "#endif" blocks --
> + * since in the future, the intent is to re-enable this code in cases where we
> + * can ensure that no initiators are touching the SDRAM.
>   */
>  ENTRY(omap3_sram_configure_core_dpll)
>  	stmfd	sp!, {r1-r12, lr}	@ store regs to stack
> @@ -219,6 +229,7 @@ configure_sdrc:
>  	ldr	r12, omap_sdrc_rfr_ctrl_0_val	@ fetch value from SRAM
>  	ldr	r11, omap3_sdrc_rfr_ctrl_0	@ fetch addr from SRAM
>  	str	r12, [r11]			@ store
> +#if 0
>  	ldr	r12, omap_sdrc_actim_ctrl_a_0_val
>  	ldr	r11, omap3_sdrc_actim_ctrl_a_0
>  	str	r12, [r11]
> @@ -228,11 +239,13 @@ configure_sdrc:
>  	ldr	r12, omap_sdrc_mr_0_val
>  	ldr	r11, omap3_sdrc_mr_0
>  	str	r12, [r11]
> +#endif
>  	ldr	r12, omap_sdrc_rfr_ctrl_1_val
>  	cmp	r12, #0			@ if SDRC_RFR_CTRL_1 is 0,
>  	beq	skip_cs1_prog		@  do not program cs1 params
>  	ldr	r11, omap3_sdrc_rfr_ctrl_1
>  	str	r12, [r11]
> +#if 0
>  	ldr	r12, omap_sdrc_actim_ctrl_a_1_val
>  	ldr	r11, omap3_sdrc_actim_ctrl_a_1
>  	str	r12, [r11]
> @@ -242,6 +255,7 @@ configure_sdrc:
>  	ldr	r12, omap_sdrc_mr_1_val
>  	ldr	r11, omap3_sdrc_mr_1
>  	str	r12, [r11]
> +#endif
>  skip_cs1_prog:
>  	ldr	r12, [r11]		@ posted-write barrier for SDRC
>  	bx	lr
>   

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

Re: [PATCH] OMAP3: SDRC: Comment out SDRC AC timing and MR changes in CORE DVFS SRAM code

[Olof Johansson]

On Wed, Dec 02, 2009 at 08:12:34AM +0200, Menon, Nishanth wrote:

> why not make this a #ifdef instead if we need it some other time, a #if
> 0 and it's intention might not be readable without doing a git annotate
> in a few months time..

Just remove the code. The old implementation is still there in git and
can be brought back if need be. It's the purpose of having revision
control in the first place.


-Olof

Re: [PATCH] OMAP3: SDRC: Comment out SDRC AC timing and MR changes in CORE DVFS SRAM code

[Paul Walmsley]

Hello Olof, Nishanth,

thanks for the comments,

On Wed, 2 Dec 2009, Olof Johansson wrote:

> On Wed, Dec 02, 2009 at 08:12:34AM +0200, Menon, Nishanth wrote:
> 
> > why not make this a #ifdef instead if we need it some other time, a #if
> > 0 and it's intention might not be readable without doing a git annotate
> > in a few months time..
> 
> Just remove the code. The old implementation is still there in git and
> can be brought back if need be. It's the purpose of having revision
> control in the first place.

I'm leaning towards Nishanth's suggestion and making it a Kconfig option. 
That way, users who know that other system initiators will be inactive at 
CORE DVFS time will be able to use optimal SDRC timings in CORE DVFS.

The other issue with removing it completely is that I'm holding out the 
(admittedly slim) hope that someone else will be enticed into helping out 
with this issue, and so I'd reason that since very few people will look at 
older revisions, removing it will drastically reduce the chances that 
others will help.


- Paul

[PATCH v2] OMAP3: SDRC: Comment out SDRC AC timing and MR changes in CORE DVFS SRAM code

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4752 bytes --]


The code that reprograms the SDRC memory controller during CORE DVFS,
mach-omap2/sram34xx.S:omap3_sram_configure_core_dpll(), does not
ensure that all L3 initiators are prevented from accessing the SDRAM
before modifying the SDRC AC timing and MR registers.  This can cause
memory to be corrupted or cause the SDRC to enter an unpredictable
state.  This patch places that code behind a Kconfig option,
CONFIG_OMAP3_SDRC_AC_TIMING for now, and adds a note explaining what
is going on.  Ideally the code can be added back in once supporting
code is present to ensure that other initiators aren't touching the
SDRAM.  At the very least, these registers should be reprogrammable
during kernel init to deal with buggy bootloaders.  Users who know
that all other system initiators will not be touching the SDRAM can
also re-enable this Kconfig option.

This is a modification of a patch originally written by Rajendra Nayak
<rnayak@ti.com> (the original is at http://patchwork.kernel.org/patch/51927/).
Rather than removing the code completely, this patch just comments it out.

Thanks to Benoît Cousson <b-cousson@ti.com> and Christophe Sucur
<c-sucur@ti.com> for explaining the technical basis for this and for
explaining what can be done to make this path work in future code.
Thanks to Richard Woodruff <r-woodruff2@ti.com> and Nishanth Menon
<nm@ti.com> for their comments.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Christophe Sucur <c-sucur@ti.com>
Cc: Benoît Cousson <b-cousson@ti.com>
Cc: Richard Woodruff <r-woodruff2@ti.com>
Cc: Nishanth Menon <nm@ti.com>

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index aad194f..a8d2610 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -100,3 +100,16 @@ config MACH_OMAP_ZOOM2
 config MACH_OMAP_4430SDP
 	bool "OMAP 4430 SDP board"
 	depends on ARCH_OMAP4
+
+config OMAP3_SDRC_AC_TIMING
+	bool "Enable SDRC AC timing register changes"
+	depends on ARCH_OMAP3 && ARCH_OMAP34XX
+	default n
+	help
+	  If you know that none of your system initiators will attempt to
+	  access SDRAM during CORE DVFS, select Y here.  This should boost
+	  SDRAM performance at lower CORE OPPs.  There are relatively few
+	  users who will wish to say yes at this point - almost everyone will
+	  wish to say no.  Selecting yes without understanding what is
+	  going on could result in system crashes;
+
diff --git a/arch/arm/mach-omap2/sram34xx.S b/arch/arm/mach-omap2/sram34xx.S
index 82aa4a3..de99ba2 100644
--- a/arch/arm/mach-omap2/sram34xx.S
+++ b/arch/arm/mach-omap2/sram34xx.S
@@ -91,8 +91,19 @@
  *  new SDRC_ACTIM_CTRL_B_1 register contents
  *  new SDRC_MR_1 register value
  *
- * If the param SDRC_RFR_CTRL_1 is 0, the parameters
- *  are not programmed into the SDRC CS1 registers
+ * If the param SDRC_RFR_CTRL_1 is 0, the parameters are not programmed into
+ * the SDRC CS1 registers
+ *
+ * NOTE: This code no longer attempts to program the SDRC AC timing and MR
+ * registers.  This is because the code currently cannot ensure that all
+ * L3 initiators (e.g., sDMA, IVA, DSS DISPC, etc.) are not accessing the
+ * SDRAM when the registers are written.  If the registers are changed while
+ * an initiator is accessing SDRAM, memory can be corrupted and/or the SDRC
+ * may enter an unpredictable state.  In the future, the intent is to
+ * re-enable this code in cases where we can ensure that no initiators are
+ * touching the SDRAM.  Until that time, users who know that their use case
+ * can satisfy the above requirement can enable the CONFIG_OMAP3_SDRC_AC_TIMING
+ * option.
  */
 ENTRY(omap3_sram_configure_core_dpll)
 	stmfd	sp!, {r1-r12, lr}	@ store regs to stack
@@ -219,6 +230,7 @@ configure_sdrc:
 	ldr	r12, omap_sdrc_rfr_ctrl_0_val	@ fetch value from SRAM
 	ldr	r11, omap3_sdrc_rfr_ctrl_0	@ fetch addr from SRAM
 	str	r12, [r11]			@ store
+#ifdef CONFIG_OMAP3_SDRC_AC_TIMING
 	ldr	r12, omap_sdrc_actim_ctrl_a_0_val
 	ldr	r11, omap3_sdrc_actim_ctrl_a_0
 	str	r12, [r11]
@@ -228,11 +240,13 @@ configure_sdrc:
 	ldr	r12, omap_sdrc_mr_0_val
 	ldr	r11, omap3_sdrc_mr_0
 	str	r12, [r11]
+#endif
 	ldr	r12, omap_sdrc_rfr_ctrl_1_val
 	cmp	r12, #0			@ if SDRC_RFR_CTRL_1 is 0,
 	beq	skip_cs1_prog		@  do not program cs1 params
 	ldr	r11, omap3_sdrc_rfr_ctrl_1
 	str	r12, [r11]
+#ifdef CONFIG_OMAP3_SDRC_AC_TIMING
 	ldr	r12, omap_sdrc_actim_ctrl_a_1_val
 	ldr	r11, omap3_sdrc_actim_ctrl_a_1
 	str	r12, [r11]
@@ -242,6 +256,7 @@ configure_sdrc:
 	ldr	r12, omap_sdrc_mr_1_val
 	ldr	r11, omap3_sdrc_mr_1
 	str	r12, [r11]
+#endif
 skip_cs1_prog:
 	ldr	r12, [r11]		@ posted-write barrier for SDRC
 	bx	lr