[U-Boot] [PATCH 0/3 v2] DMA ECC update

[U-Boot] [PATCH 0/3 v2] DMA ECC update

[Peter Tyser]

Changes since v1:
- Enable instruction cache during 83xx sdram init

These changes bring the 83xx SDRAM ECC initialization in line
with the 85xx/86xx boards and also fixes a minor bug in fsl_dma.c.

I don't have any 83xx boards to test on, so it would be appreciated
if someone with 83xx hardware that uses ECC could give the patches
a shot.

It'd be nice if SDRAM could be initialized via the DDR controller
with CONFIG_ECC_INIT_VIA_DDRCONTROLLER on the 83xx platform too,
but I'm not going to tackle it:)

The patches also resolve the compile error Stefan brought up with
non-freescale boards with CONFIG_ECC.

Peter Tyser (3):
  83xx: Default to using DMA to initialize SDRAM
  83xx: Added CONFIG_MEM_INIT_VALUE for boards with ECC
  fsl_dma: Fix SDRAM initial value

 cpu/mpc83xx/spd_sdram.c       |   57 +++--------------------------------------
 drivers/dma/fsl_dma.c         |   18 ++++++++----
 include/asm-ppc/config.h      |    7 ++---
 include/configs/MPC8349EMDS.h |    1 +
 include/configs/MPC8360EMDS.h |    1 +
 include/configs/MPC8360ERDK.h |    1 +
 include/configs/MPC837XEMDS.h |    1 +
 include/configs/MPC837XERDB.h |    1 +
 include/configs/TQM834x.h     |    1 +
 include/configs/kmeter1.h     |    1 +
 include/configs/sbc8349.h     |    1 +
 11 files changed, 27 insertions(+), 63 deletions(-)

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initialize SDRAM

[Peter Tyser]

When SDRAM ECC is enabled and CONFIG_ECC_INIT_VIA_DDRCONTROLLER is not
defined use DMA to set SDRAM to a known state.  Previously a sequence of
64-bit stores was used.

Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
---
 cpu/mpc83xx/spd_sdram.c  |   57 +++------------------------------------------
 drivers/dma/fsl_dma.c    |   16 +++++++++----
 include/asm-ppc/config.h |    7 ++---
 3 files changed, 18 insertions(+), 62 deletions(-)

diff --git a/cpu/mpc83xx/spd_sdram.c b/cpu/mpc83xx/spd_sdram.c
index 0f61180..8a09a7d 100644
--- a/cpu/mpc83xx/spd_sdram.c
+++ b/cpu/mpc83xx/spd_sdram.c
@@ -825,67 +825,18 @@ long int spd_sdram()
 
 #if defined(CONFIG_DDR_ECC) && !defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER)
 /*
- * Use timebase counter, get_timer() is not availabe
- * at this point of initialization yet.
- */
-static __inline__ unsigned long get_tbms (void)
-{
-	unsigned long tbl;
-	unsigned long tbu1, tbu2;
-	unsigned long ms;
-	unsigned long long tmp;
-
-	ulong tbclk = get_tbclk();
-
-	/* get the timebase ticks */
-	do {
-		asm volatile ("mftbu %0":"=r" (tbu1):);
-		asm volatile ("mftb %0":"=r" (tbl):);
-		asm volatile ("mftbu %0":"=r" (tbu2):);
-	} while (tbu1 != tbu2);
-
-	/* convert ticks to ms */
-	tmp = (unsigned long long)(tbu1);
-	tmp = (tmp << 32);
-	tmp += (unsigned long long)(tbl);
-	ms = tmp/(tbclk/1000);
-
-	return ms;
-}
-
-/*
  * Initialize all of memory for ECC, then enable errors.
  */
 void ddr_enable_ecc(unsigned int dram_size)
 {
 	volatile immap_t *immap = (immap_t *)CONFIG_SYS_IMMR;
 	volatile ddr83xx_t *ddr= &immap->ddr;
-	unsigned long t_start, t_end;
-	register u64 *p;
-	register uint size;
-	unsigned int pattern[2];
-
-	icache_enable();
-	t_start = get_tbms();
-	pattern[0] = 0xdeadbeef;
-	pattern[1] = 0xdeadbeef;
-
-#if defined(CONFIG_DDR_ECC_INIT_VIA_DMA)
-	dma_meminit(pattern[0], dram_size);
-#else
-	debug("ddr init: CPU FP write method\n");
-	size = dram_size;
-	for (p = 0; p < (u64*)(size); p++) {
-		ppcDWstore((u32*)p, pattern);
-	}
-	__asm__ __volatile__ ("sync");
-#endif
 
-	t_end = get_tbms();
-	icache_disable();
+	debug("\nInitializing ECC!\n");
+
+	dma_meminit(0xdeadbeef, dram_size);
 
-	debug("\nREADY!!\n");
-	debug("ddr init duration: %ld ms\n", t_end - t_start);
+	debug("\nREADY!\n");
 
 	/* Clear All ECC Errors */
 	if ((ddr->err_detect & ECC_ERROR_DETECT_MME) == ECC_ERROR_DETECT_MME)
diff --git a/drivers/dma/fsl_dma.c b/drivers/dma/fsl_dma.c
index df33e7a..90a96dd 100644
--- a/drivers/dma/fsl_dma.c
+++ b/drivers/dma/fsl_dma.c
@@ -140,17 +140,19 @@ int dmacpy(phys_addr_t dest, phys_addr_t src, phys_size_t count) {
 }
 
 /*
- * 85xx/86xx use dma to initialize SDRAM when !CONFIG_ECC_INIT_VIA_DDRCONTROLLER
- * while 83xx uses dma to initialize SDRAM when CONFIG_DDR_ECC_INIT_VIA_DMA
+ * Use dma to initialize SDRAM when !CONFIG_ECC_INIT_VIA_DDRCONTROLLER
  */
-#if ((!defined CONFIG_MPC83xx && defined(CONFIG_DDR_ECC) &&	\
-	!defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER)) ||		\
-	(defined(CONFIG_MPC83xx) && defined(CONFIG_DDR_ECC_INIT_VIA_DMA)))
+#if (defined(CONFIG_DDR_ECC) && !defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER))
 void dma_meminit(uint val, uint size)
 {
 	uint *p = 0;
 	uint i = 0;
 
+#ifdef CONFIG_MPC83xx
+	/* Enabling the instruction cache greatly improves SDRAM init speed */
+	icache_enable();
+#endif
+
 	for (*p = 0; p < (uint *)(8 * 1024); p++) {
 		if (((uint)p & 0x1f) == 0)
 			ppcDcbz((ulong)p);
@@ -174,5 +176,9 @@ void dma_meminit(uint val, uint size)
 
 	for (i = 1; i < size / 0x800000; i++)
 		dmacpy((0x800000 * i), 0, 0x800000);
+
+#ifdef CONFIG_MPC83xx
+	icache_disable();
+#endif
 }
 #endif
diff --git a/include/asm-ppc/config.h b/include/asm-ppc/config.h
index ca143c7..01668a9 100644
--- a/include/asm-ppc/config.h
+++ b/include/asm-ppc/config.h
@@ -29,10 +29,9 @@
 #endif
 #endif
 
-#ifndef CONFIG_FSL_DMA
-#if ((!defined CONFIG_MPC83xx && defined(CONFIG_DDR_ECC) &&	\
-	!defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER)) ||		\
-	(defined(CONFIG_MPC83xx) && defined(CONFIG_DDR_ECC_INIT_VIA_DMA)))
+#if (defined(CONFIG_MPC83xx) || defined(CONFIG_MPC85xx) || \
+	defined(CONFIG_MPC86xx)) && !defined CONFIG_FSL_DMA
+#if (defined(CONFIG_DDR_ECC) && !defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER))
 #define CONFIG_FSL_DMA
 #endif
 #endif
-- 
1.6.2.1

[U-Boot] [PATCH 2/3 v2] 83xx: Add CONFIG_MEM_INIT_VALUE for boards with ECC

[Peter Tyser]

Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
---
 cpu/mpc83xx/spd_sdram.c       |    2 +-
 include/configs/MPC8349EMDS.h |    1 +
 include/configs/MPC8360EMDS.h |    1 +
 include/configs/MPC8360ERDK.h |    1 +
 include/configs/MPC837XEMDS.h |    1 +
 include/configs/MPC837XERDB.h |    1 +
 include/configs/TQM834x.h     |    1 +
 include/configs/kmeter1.h     |    1 +
 include/configs/sbc8349.h     |    1 +
 9 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/cpu/mpc83xx/spd_sdram.c b/cpu/mpc83xx/spd_sdram.c
index 8a09a7d..ab6a2bb 100644
--- a/cpu/mpc83xx/spd_sdram.c
+++ b/cpu/mpc83xx/spd_sdram.c
@@ -834,7 +834,7 @@ void ddr_enable_ecc(unsigned int dram_size)
 
 	debug("\nInitializing ECC!\n");
 
-	dma_meminit(0xdeadbeef, dram_size);
+	dma_meminit(CONFIG_MEM_INIT_VALUE, dram_size);
 
 	debug("\nREADY!\n");
 
diff --git a/include/configs/MPC8349EMDS.h b/include/configs/MPC8349EMDS.h
index ea5fbff..35e6e84 100644
--- a/include/configs/MPC8349EMDS.h
+++ b/include/configs/MPC8349EMDS.h
@@ -76,6 +76,7 @@
  */
 #define CONFIG_DDR_ECC			/* support DDR ECC function */
 #define CONFIG_DDR_ECC_CMD		/* use DDR ECC user commands */
+#define CONFIG_MEM_INIT_VALUE		0xDeadBeef
 #define CONFIG_SPD_EEPROM		/* use SPD EEPROM for DDR setup*/
 
 /*
diff --git a/include/configs/MPC8360EMDS.h b/include/configs/MPC8360EMDS.h
index 3497ba0..27b5a58 100644
--- a/include/configs/MPC8360EMDS.h
+++ b/include/configs/MPC8360EMDS.h
@@ -109,6 +109,7 @@
 
 #define CONFIG_DDR_ECC		/* support DDR ECC function */
 #define CONFIG_DDR_ECC_CMD	/* Use DDR ECC user commands */
+#define CONFIG_MEM_INIT_VALUE	0xDeadBeef
 
 /*
  * DDRCDR - DDR Control Driver Register
diff --git a/include/configs/MPC8360ERDK.h b/include/configs/MPC8360ERDK.h
index f584435..c2ff098 100644
--- a/include/configs/MPC8360ERDK.h
+++ b/include/configs/MPC8360ERDK.h
@@ -91,6 +91,7 @@
 
 #define CONFIG_DDR_ECC		/* support DDR ECC function */
 #define CONFIG_DDR_ECC_CMD	/* Use DDR ECC user commands */
+#define CONFIG_MEM_INIT_VALUE	0xDeadBeef
 
 /*
  * DDRCDR - DDR Control Driver Register
diff --git a/include/configs/MPC837XEMDS.h b/include/configs/MPC837XEMDS.h
index 4befcab..aa17220 100644
--- a/include/configs/MPC837XEMDS.h
+++ b/include/configs/MPC837XEMDS.h
@@ -129,6 +129,7 @@
 
 #undef CONFIG_DDR_ECC		/* support DDR ECC function */
 #undef CONFIG_DDR_ECC_CMD	/* Use DDR ECC user commands */
+#undef CONFIG_MEM_INIT_VALUE
 
 #define CONFIG_SPD_EEPROM	/* Use SPD EEPROM for DDR setup */
 #define CONFIG_NEVER_ASSERT_ODT_TO_CPU /* Never assert ODT to internal IOs */
diff --git a/include/configs/MPC837XERDB.h b/include/configs/MPC837XERDB.h
index 2b7d629..6a91395 100644
--- a/include/configs/MPC837XERDB.h
+++ b/include/configs/MPC837XERDB.h
@@ -156,6 +156,7 @@
 
 #undef CONFIG_DDR_ECC		/* support DDR ECC function */
 #undef CONFIG_DDR_ECC_CMD	/* Use DDR ECC user commands */
+#undef CONFIG_MEM_INIT_VALUE
 
 #undef CONFIG_NEVER_ASSERT_ODT_TO_CPU	/* Never assert ODT to internal IOs */
 
diff --git a/include/configs/TQM834x.h b/include/configs/TQM834x.h
index efade69..fbb2e48 100644
--- a/include/configs/TQM834x.h
+++ b/include/configs/TQM834x.h
@@ -68,6 +68,7 @@
 #define CONFIG_SYS_DDR_SDRAM_BASE	CONFIG_SYS_DDR_BASE
 #define DDR_CASLAT_25				/* CASLAT set to 2.5 */
 #undef CONFIG_DDR_ECC				/* only for ECC DDR module */
+#undef CONFIG_MEM_INIT_VALUE
 #undef CONFIG_SPD_EEPROM			/* do not use SPD EEPROM for DDR setup */
 
 #undef CONFIG_SYS_DRAM_TEST				/* memory test, takes time */
diff --git a/include/configs/kmeter1.h b/include/configs/kmeter1.h
index 19da133..d9c88f2 100644
--- a/include/configs/kmeter1.h
+++ b/include/configs/kmeter1.h
@@ -84,6 +84,7 @@
 #define CFG_83XX_DDR_USES_CS0
 
 #undef CONFIG_DDR_ECC
+#undef CONFIG_MEM_INIT_VALUE
 
 /*
  * DDRCDR - DDR Control Driver Register
diff --git a/include/configs/sbc8349.h b/include/configs/sbc8349.h
index 84a251a..5a62c69 100644
--- a/include/configs/sbc8349.h
+++ b/include/configs/sbc8349.h
@@ -74,6 +74,7 @@
  */
 #undef CONFIG_DDR_ECC			/* only for ECC DDR module */
 #undef CONFIG_DDR_ECC_CMD		/* use DDR ECC user commands */
+#undef CONFIG_MEM_INIT_VALUE
 #define CONFIG_SPD_EEPROM		/* use SPD EEPROM for DDR setup*/
 #define CONFIG_SYS_83XX_DDR_USES_CS0		/* WRS; Fsl board uses CS2/CS3 */
 
-- 
1.6.2.1

[U-Boot] [PATCH 3/3 v2] fsl_dma: Fix SDRAM initial value

[Peter Tyser]

The initial SDRAM value was being hardcoded to CONFIG_MEM_INIT_VALUE
instead of the value passed in 'val'.

Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
---
 drivers/dma/fsl_dma.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/fsl_dma.c b/drivers/dma/fsl_dma.c
index 90a96dd..9ad07fe 100644
--- a/drivers/dma/fsl_dma.c
+++ b/drivers/dma/fsl_dma.c
@@ -157,7 +157,7 @@ void dma_meminit(uint val, uint size)
 		if (((uint)p & 0x1f) == 0)
 			ppcDcbz((ulong)p);
 
-		*p = (uint)CONFIG_MEM_INIT_VALUE;
+		*p = val;
 
 		if (((uint)p & 0x1c) == 0x1c)
 			ppcDcbf((ulong)p);
-- 
1.6.2.1

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initializeSDRAM

[Liu Dave-R63238]

> When SDRAM ECC is enabled and CONFIG_ECC_INIT_VIA_DDRCONTROLLER is not
> defined use DMA to set SDRAM to a known state.  Previously a 
> sequence of
> 64-bit stores was used.

IIRC, the DMA init SDRAM is slower than the 64bit stores.
It is why I added these code here.

I suggest to keep the way. 

> 
> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
> ---
>  cpu/mpc83xx/spd_sdram.c  |   57 
> +++------------------------------------------
>  drivers/dma/fsl_dma.c    |   16 +++++++++----
>  include/asm-ppc/config.h |    7 ++---
>  3 files changed, 18 insertions(+), 62 deletions(-)
> 
> diff --git a/cpu/mpc83xx/spd_sdram.c b/cpu/mpc83xx/spd_sdram.c
> index 0f61180..8a09a7d 100644
> --- a/cpu/mpc83xx/spd_sdram.c
> +++ b/cpu/mpc83xx/spd_sdram.c
> @@ -825,67 +825,18 @@ long int spd_sdram()
>  
>  #if defined(CONFIG_DDR_ECC) && 
> !defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER)
>  /*
> - * Use timebase counter, get_timer() is not availabe
> - * at this point of initialization yet.
> - */
> -static __inline__ unsigned long get_tbms (void)
> -{
> -	unsigned long tbl;
> -	unsigned long tbu1, tbu2;
> -	unsigned long ms;
> -	unsigned long long tmp;
> -
> -	ulong tbclk = get_tbclk();
> -
> -	/* get the timebase ticks */
> -	do {
> -		asm volatile ("mftbu %0":"=r" (tbu1):);
> -		asm volatile ("mftb %0":"=r" (tbl):);
> -		asm volatile ("mftbu %0":"=r" (tbu2):);
> -	} while (tbu1 != tbu2);
> -
> -	/* convert ticks to ms */
> -	tmp = (unsigned long long)(tbu1);
> -	tmp = (tmp << 32);
> -	tmp += (unsigned long long)(tbl);
> -	ms = tmp/(tbclk/1000);
> -
> -	return ms;
> -}
> -
> -/*
>   * Initialize all of memory for ECC, then enable errors.
>   */
>  void ddr_enable_ecc(unsigned int dram_size)
>  {
>  	volatile immap_t *immap = (immap_t *)CONFIG_SYS_IMMR;
>  	volatile ddr83xx_t *ddr= &immap->ddr;
> -	unsigned long t_start, t_end;
> -	register u64 *p;
> -	register uint size;
> -	unsigned int pattern[2];
> -
> -	icache_enable();
> -	t_start = get_tbms();
> -	pattern[0] = 0xdeadbeef;
> -	pattern[1] = 0xdeadbeef;
> -
> -#if defined(CONFIG_DDR_ECC_INIT_VIA_DMA)
> -	dma_meminit(pattern[0], dram_size);
> -#else
> -	debug("ddr init: CPU FP write method\n");
> -	size = dram_size;
> -	for (p = 0; p < (u64*)(size); p++) {
> -		ppcDWstore((u32*)p, pattern);
> -	}
> -	__asm__ __volatile__ ("sync");
> -#endif
>  
> -	t_end = get_tbms();
> -	icache_disable();
> +	debug("\nInitializing ECC!\n");
> +
> +	dma_meminit(0xdeadbeef, dram_size);
>  
> -	debug("\nREADY!!\n");
> -	debug("ddr init duration: %ld ms\n", t_end - t_start);
> +	debug("\nREADY!\n");
>  
>  	/* Clear All ECC Errors */
>  	if ((ddr->err_detect & ECC_ERROR_DETECT_MME) == 
> ECC_ERROR_DETECT_MME)
> diff --git a/drivers/dma/fsl_dma.c b/drivers/dma/fsl_dma.c
> index df33e7a..90a96dd 100644
> --- a/drivers/dma/fsl_dma.c
> +++ b/drivers/dma/fsl_dma.c
> @@ -140,17 +140,19 @@ int dmacpy(phys_addr_t dest, 
> phys_addr_t src, phys_size_t count) {
>  }
>  
>  /*
> - * 85xx/86xx use dma to initialize SDRAM when 
> !CONFIG_ECC_INIT_VIA_DDRCONTROLLER
> - * while 83xx uses dma to initialize SDRAM when 
> CONFIG_DDR_ECC_INIT_VIA_DMA
> + * Use dma to initialize SDRAM when 
> !CONFIG_ECC_INIT_VIA_DDRCONTROLLER
>   */
> -#if ((!defined CONFIG_MPC83xx && defined(CONFIG_DDR_ECC) &&	\
> -	!defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER)) ||		\
> -	(defined(CONFIG_MPC83xx) && 
> defined(CONFIG_DDR_ECC_INIT_VIA_DMA)))
> +#if (defined(CONFIG_DDR_ECC) && 
> !defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER))
>  void dma_meminit(uint val, uint size)
>  {
>  	uint *p = 0;
>  	uint i = 0;
>  
> +#ifdef CONFIG_MPC83xx
> +	/* Enabling the instruction cache greatly improves 
> SDRAM init speed */
> +	icache_enable();
> +#endif
> +
>  	for (*p = 0; p < (uint *)(8 * 1024); p++) {
>  		if (((uint)p & 0x1f) == 0)
>  			ppcDcbz((ulong)p);
> @@ -174,5 +176,9 @@ void dma_meminit(uint val, uint size)
>  
>  	for (i = 1; i < size / 0x800000; i++)
>  		dmacpy((0x800000 * i), 0, 0x800000);
> +
> +#ifdef CONFIG_MPC83xx
> +	icache_disable();
> +#endif
>  }
>  #endif
> diff --git a/include/asm-ppc/config.h b/include/asm-ppc/config.h
> index ca143c7..01668a9 100644
> --- a/include/asm-ppc/config.h
> +++ b/include/asm-ppc/config.h
> @@ -29,10 +29,9 @@
>  #endif
>  #endif
>  
> -#ifndef CONFIG_FSL_DMA
> -#if ((!defined CONFIG_MPC83xx && defined(CONFIG_DDR_ECC) &&	\
> -	!defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER)) ||		\
> -	(defined(CONFIG_MPC83xx) && 
> defined(CONFIG_DDR_ECC_INIT_VIA_DMA)))
> +#if (defined(CONFIG_MPC83xx) || defined(CONFIG_MPC85xx) || \
> +	defined(CONFIG_MPC86xx)) && !defined CONFIG_FSL_DMA
> +#if (defined(CONFIG_DDR_ECC) && 
> !defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER))
>  #define CONFIG_FSL_DMA
>  #endif
>  #endif
> -- 
> 1.6.2.1
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initializeSDRAM

[Peter Tyser]

On Thu, 2009-07-09 at 08:24 +0800, Liu Dave-R63238 wrote:
> > When SDRAM ECC is enabled and CONFIG_ECC_INIT_VIA_DDRCONTROLLER is not
> > defined use DMA to set SDRAM to a known state.  Previously a 
> > sequence of
> > 64-bit stores was used.
> 
> IIRC, the DMA init SDRAM is slower than the 64bit stores.
> It is why I added these code here.
> 
> I suggest to keep the way. 

According to Ira, the DMA method was faster than the cpu method:
"It makes the DMA initialization normal speed again. The DMA in the for
loop takes the longest (as expected).

So yes, strangely it (enabling the icache) makes a HUGE difference. The
total time is <3 seconds now. It is now faster than the previous CPU
method."


Logically the DMA method should be faster, and Ira's results seem to
reinforce this.  I don't have an 83xx board to test on, so let me know
if others have different results than Ira.

Best,
Peter

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initializeSDRAM

[Liu Dave-R63238]

> According to Ira, the DMA method was faster than the cpu method:
> "It makes the DMA initialization normal speed again. The DMA 
> in the for loop takes the longest (as expected).
> 
> So yes, strangely it (enabling the icache) makes a HUGE 
> difference. The total time is <3 seconds now. It is now 
> faster than the previous CPU method."
> 
> 
> Logically the DMA method should be faster, and Ira's results 
> seem to reinforce this.  I don't have an 83xx board to test 
> on, so let me know if others have different results than Ira.

I did the DMA init test with the orignal DMA code. At that time
The DMA method is slower than CPU method. Strange why the
result is different.

Not sure what is the new DMA code?

DMA method needs read and write bus transaction, but CPU doesn't
need the read transaction. Alougth the DMA may be burst read/write.

Anyway, the test result determine all.

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initializeSDRAM

[Ira W. Snyder]

On Wed, Jul 08, 2009 at 07:32:26PM -0500, Peter Tyser wrote:
> On Thu, 2009-07-09 at 08:24 +0800, Liu Dave-R63238 wrote:
> > > When SDRAM ECC is enabled and CONFIG_ECC_INIT_VIA_DDRCONTROLLER is not
> > > defined use DMA to set SDRAM to a known state.  Previously a 
> > > sequence of
> > > 64-bit stores was used.
> > 
> > IIRC, the DMA init SDRAM is slower than the 64bit stores.
> > It is why I added these code here.
> > 
> > I suggest to keep the way. 
> 
> According to Ira, the DMA method was faster than the cpu method:
> "It makes the DMA initialization normal speed again. The DMA in the for
> loop takes the longest (as expected).
> 
> So yes, strangely it (enabling the icache) makes a HUGE difference. The
> total time is <3 seconds now. It is now faster than the previous CPU
> method."
> 
> 
> Logically the DMA method should be faster, and Ira's results seem to
> reinforce this.  I don't have an 83xx board to test on, so let me know
> if others have different results than Ira.
> 

I didn't check the SDRAM init time with code, only by a rough estimate.
(Counting "one one-thousand, two one-thousand...").  I can add back the
time measuring code, and be really sure which one is faster.

Both the DMA and CPU methods are definitely on the same order of
magnitude. The time taken by the CFI flash driver is MUCH longer than
the SDRAM initialization. I wonder, should the icache be enabled for
that as well?

I'll do some more testing when I get back to the office tomorrow.

Ira

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initializeSDRAM

[Jerry Van Baren]

Liu Dave-R63238 wrote:
>> According to Ira, the DMA method was faster than the cpu method:
>> "It makes the DMA initialization normal speed again. The DMA 
>> in the for loop takes the longest (as expected).
>>
>> So yes, strangely it (enabling the icache) makes a HUGE 
>> difference. The total time is <3 seconds now. It is now 
>> faster than the previous CPU method."
>>
>> Logically the DMA method should be faster, and Ira's results 
>> seem to reinforce this.  I don't have an 83xx board to test 
>> on, so let me know if others have different results than Ira.
> 
> I did the DMA init test with the orignal DMA code. At that time
> The DMA method is slower than CPU method. Strange why the
> result is different.

Instruction cache enabled/disabled is the difference - it eliminates 
instruction fetches from competing with the DMA engine on the SDRAM bus. 
  See my recent message:
<http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/63152/focus=63308>

FWIIW, in our experience with various PowerPC processors, using the 
CPU's "dcbz" (data cache block set to zero) instruction is also very 
fast, but it only works if you want to initialize memory to zero.  You 
need to enable data caching (obviously) and not having instruction cache 
enabled would cripple that method too.

[snip]

Best regards,
gvb

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initializeSDRAM

[Jerry Van Baren]

Jerry Van Baren wrote:
> Liu Dave-R63238 wrote:
>>> According to Ira, the DMA method was faster than the cpu method:
>>> "It makes the DMA initialization normal speed again. The DMA 
>>> in the for loop takes the longest (as expected).
>>>
>>> So yes, strangely it (enabling the icache) makes a HUGE 
>>> difference. The total time is <3 seconds now. It is now 
>>> faster than the previous CPU method."
>>>
>>> Logically the DMA method should be faster, and Ira's results 
>>> seem to reinforce this.  I don't have an 83xx board to test 
>>> on, so let me know if others have different results than Ira.
>> I did the DMA init test with the orignal DMA code. At that time
>> The DMA method is slower than CPU method. Strange why the
>> result is different.
> 
> Instruction cache enabled/disabled is the difference - it eliminates 
> instruction fetches from competing with the DMA engine on the SDRAM bus. 
>   See my recent message:
> <http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/63152/focus=63308>

Oops, that isn't going to be exactly correct since we aren't running out 
of RAM yet so instruction fetching isn't going to compete on the SDRAM 
bus directly.  I stand by the general theory, however, that it is bus 
contention of some sort - I would speculate the arbitration of the local 
bus to flash vs. the DMA engine to SDRAM.  Logically they should be 
independent, but they may not be due to internal interference (both 
using the same internal bus?) or simplifying assumptions in the 
arbitrator implementation.

[snip]

Best regards,
gvb

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initialize SDRAM

[Ira W. Snyder]

On Wed, Jul 08, 2009 at 05:58:50PM -0700, Ira W. Snyder wrote:
> On Wed, Jul 08, 2009 at 07:32:26PM -0500, Peter Tyser wrote:
> > On Thu, 2009-07-09 at 08:24 +0800, Liu Dave-R63238 wrote:
> > > > When SDRAM ECC is enabled and CONFIG_ECC_INIT_VIA_DDRCONTROLLER is not
> > > > defined use DMA to set SDRAM to a known state.  Previously a 
> > > > sequence of
> > > > 64-bit stores was used.
> > > 
> > > IIRC, the DMA init SDRAM is slower than the 64bit stores.
> > > It is why I added these code here.
> > > 
> > > I suggest to keep the way. 
> > 
> > According to Ira, the DMA method was faster than the cpu method:
> > "It makes the DMA initialization normal speed again. The DMA in the for
> > loop takes the longest (as expected).
> > 
> > So yes, strangely it (enabling the icache) makes a HUGE difference. The
> > total time is <3 seconds now. It is now faster than the previous CPU
> > method."
> > 
> > 
> > Logically the DMA method should be faster, and Ira's results seem to
> > reinforce this.  I don't have an 83xx board to test on, so let me know
> > if others have different results than Ira.
> > 
> 
> I didn't check the SDRAM init time with code, only by a rough estimate.
> (Counting "one one-thousand, two one-thousand...").  I can add back the
> time measuring code, and be really sure which one is faster.
> 
> Both the DMA and CPU methods are definitely on the same order of
> magnitude. The time taken by the CFI flash driver is MUCH longer than
> the SDRAM initialization. I wonder, should the icache be enabled for
> that as well?
> 
> I'll do some more testing when I get back to the office tomorrow.
> 

Ok, I've added back the get_tbms() code, and created a routine to
initialize ECC with the CPU. I've inlined my patch below, just for
reference.

DMA 945ms
CPU 581ms

As an interesting comparison, I also benchmarked the method of using the
DDR controller to initialize ECC.

DDRC 129ms

So there you have it. Dave Liu is correct, the CPU method is faster,
though definitely on the same order of magnitude with the
icache_enable()/icache_disable() patch.

I may stick with the DDRC method on my board, the difference is just
amazing. The flash init still takes up most of the boot process,
however.

Peter, feel free to fold the patch below into your own work.
Ira

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initialize SDRAM

[Jerry Van Baren]

Ira W. Snyder wrote:
> On Wed, Jul 08, 2009 at 05:58:50PM -0700, Ira W. Snyder wrote:
>> On Wed, Jul 08, 2009 at 07:32:26PM -0500, Peter Tyser wrote:
>>> On Thu, 2009-07-09 at 08:24 +0800, Liu Dave-R63238 wrote:

[snip]

>> Both the DMA and CPU methods are definitely on the same order of
>> magnitude. The time taken by the CFI flash driver is MUCH longer than
>> the SDRAM initialization. I wonder, should the icache be enabled for
>> that as well?
>>
>> I'll do some more testing when I get back to the office tomorrow.
>>
> 
> Ok, I've added back the get_tbms() code, and created a routine to
> initialize ECC with the CPU. I've inlined my patch below, just for
> reference.
> 
> DMA 945ms
> CPU 581ms
> 
> As an interesting comparison, I also benchmarked the method of using the
> DDR controller to initialize ECC.
> 
> DDRC 129ms
> 
> So there you have it. Dave Liu is correct, the CPU method is faster,
> though definitely on the same order of magnitude with the
> icache_enable()/icache_disable() patch.

Thanks for the measurements and the update.

> I may stick with the DDRC method on my board, the difference is just
> amazing. The flash init still takes up most of the boot process,
> however.
> 
> Peter, feel free to fold the patch below into your own work.
> Ira

[snip]

FWIIW, and it doesn't materially change your results, but it would have 
been better to duplicate the get_tbms() call and put it directly around 
the *_meminit() calls rather than having the print statements inside the 
timed part.  Since U-Boot printing is a blocking call (generally), you 
are adding the UART Tx time to your measurement.

#if 0
	debug("\nInitializing ECC (using DMA)!\n");
	s = get_tbms();
  	dma_meminit(CONFIG_MEM_INIT_VALUE, dram_size);
#else
	debug("\nInitializing ECC (using CPU)!\n");
	s = get_tbms();
	cpu_meminit(CONFIG_MEM_INIT_VALUE, dram_size);
#endif
	e = get_tbms();


> +	s = get_tbms();
> +#if 0
> +	debug("\nInitializing ECC (using DMA)!\n");
>  	dma_meminit(CONFIG_MEM_INIT_VALUE, dram_size);
> +#else
> +	debug("\nInitializing ECC (using CPU)!\n");
> +	cpu_meminit(CONFIG_MEM_INIT_VALUE, dram_size);
> +#endif
> +	e = get_tbms();
>  
>  	debug("\nREADY!\n");
> +	debug("ddr init duration: %ld ms\n", e - s);
>  
>  	/* Clear All ECC Errors */
>  	if ((ddr->err_detect & ECC_ERROR_DETECT_MME) == ECC_ERROR_DETECT_MME)

Thanks,
gvb

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initialize SDRAM

[Wolfgang Denk]

Dear Kim,

In message <1247098309-29122-2-git-send-email-ptyser@xes-inc.com> Peter Tyse wrote:
> When SDRAM ECC is enabled and CONFIG_ECC_INIT_VIA_DDRCONTROLLER is not
> defined use DMA to set SDRAM to a known state.  Previously a sequence of
> 64-bit stores was used.
> 
> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
> ---
>  cpu/mpc83xx/spd_sdram.c  |   57 +++------------------------------------------
>  drivers/dma/fsl_dma.c    |   16 +++++++++----
>  include/asm-ppc/config.h |    7 ++---
>  3 files changed, 18 insertions(+), 62 deletions(-)

It seems this patch (especially the include/asm-ppc/config.h changes)
fix build problems for some other (non-FSL) boards, so I would like to
get this in quickly.

Do you plan to send a pull request including these patches any time
soon?

Alternatively, should I go ahead and apply just this patch (1/3)
directly?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Keep your head and your heart going in the right  direction  and  you
will not have to worry about your feet.

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initialize SDRAM

[Kumar Gala]

On Jul 14, 2009, at 4:22 AM, Wolfgang Denk wrote:

> Dear Kim,
>
> In message <1247098309-29122-2-git-send-email-ptyser@xes-inc.com>  
> Peter Tyse wrote:
>> When SDRAM ECC is enabled and CONFIG_ECC_INIT_VIA_DDRCONTROLLER is  
>> not
>> defined use DMA to set SDRAM to a known state.  Previously a  
>> sequence of
>> 64-bit stores was used.
>>
>> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
>> ---
>> cpu/mpc83xx/spd_sdram.c  |   57 ++ 
>> +------------------------------------------
>> drivers/dma/fsl_dma.c    |   16 +++++++++----
>> include/asm-ppc/config.h |    7 ++---
>> 3 files changed, 18 insertions(+), 62 deletions(-)
>
> It seems this patch (especially the include/asm-ppc/config.h changes)
> fix build problems for some other (non-FSL) boards, so I would like to
> get this in quickly.
>
> Do you plan to send a pull request including these patches any time
> soon?
>
> Alternatively, should I go ahead and apply just this patch (1/3)
> directly?
>
> Best regards,
>
> Wolfgang Denk

I'll poke Kim on these and resolve this one way or another today.

- k

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initialize SDRAM

[Peter Tyser]

On Tue, 2009-07-14 at 11:22 +0200, Wolfgang Denk wrote:
> Dear Kim,
> 
> In message <1247098309-29122-2-git-send-email-ptyser@xes-inc.com> Peter Tyse wrote:
> > When SDRAM ECC is enabled and CONFIG_ECC_INIT_VIA_DDRCONTROLLER is not
> > defined use DMA to set SDRAM to a known state.  Previously a sequence of
> > 64-bit stores was used.
> > 
> > Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
> > ---
> >  cpu/mpc83xx/spd_sdram.c  |   57 +++------------------------------------------
> >  drivers/dma/fsl_dma.c    |   16 +++++++++----
> >  include/asm-ppc/config.h |    7 ++---
> >  3 files changed, 18 insertions(+), 62 deletions(-)
> 
> It seems this patch (especially the include/asm-ppc/config.h changes)
> fix build problems for some other (non-FSL) boards, so I would like to
> get this in quickly.
> 
> Do you plan to send a pull request including these patches any time
> soon?
> 
> Alternatively, should I go ahead and apply just this patch (1/3)
> directly?

There was some discussion about whether to use DMA to initialize SDRAM
on the 83xx architecture (which this patch does).  After some
benchmarking by Ira Synder it turns out that using DMA is actually
slower than using the initial method of a series of 64-bit writes.  Thus
I'm assuming Kim would prefer not to use DMA and thus this patch should
be discarded.

Kim, let me know if this is the case and I'll resubmit a patch just to
fix the build error.  Patches 2 and 3 are still applicable even if we
discard patch 1.

Best,
Peter

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initialize SDRAM

[Wolfgang Denk]

Dear Peter Tyser,

In message <1247582520.30723.4.camel@localhost.localdomain> you wrote:
>
> There was some discussion about whether to use DMA to initialize SDRAM
> on the 83xx architecture (which this patch does).  After some
> benchmarking by Ira Synder it turns out that using DMA is actually
> slower than using the initial method of a series of 64-bit writes.  Thus
> I'm assuming Kim would prefer not to use DMA and thus this patch should
> be discarded.
> 
> Kim, let me know if this is the case and I'll resubmit a patch just to
> fix the build error.  Patches 2 and 3 are still applicable even if we
> discard patch 1.

We need at least the include/asm-ppc/config.h changes as these fix a
real build problem.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If you believe that feeling bad or worrying long enough will change a
past or future event, then you are residing on another planet with  a
different reality system.

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initialize SDRAM

[Kim Phillips]

On Tue, 14 Jul 2009 09:42:00 -0500
Peter Tyser <ptyser@xes-inc.com> wrote:


> I'm assuming Kim would prefer not to use DMA and thus this patch should
> be discarded.

yeah, I prefer speed :)

> Kim, let me know if this is the case and I'll resubmit a patch just to
> fix the build error.  Patches 2 and 3 are still applicable even if we
> discard patch 1.

ok

WD, please feel free to apply the fix, otherwise I'll have a pull
request for you coming up.

Kim

[U-Boot] [PATCH 1/3 v2] 83xx: Default to using DMA to initialize SDRAM

[Peter Tyser]

On Tue, 2009-07-14 at 12:32 -0500, Kim Phillips wrote:
> On Tue, 14 Jul 2009 09:42:00 -0500
> Peter Tyser <ptyser@xes-inc.com> wrote:
> 
> 
> > I'm assuming Kim would prefer not to use DMA and thus this patch should
> > be discarded.
> 
> yeah, I prefer speed :)

It looked like Ira tested using the DDR controller for initialization
and it was ~4x as fast as the CPU method.  If someone out there in 83xx
land is looking to shave a few ms off their boot time (and they use ECC)
it shouldn't be that hard to add support for the
CONFIG_ECC_INIT_VIA_DDRCONTROLLER similar to the 85xx and 86xx
architectures.

> > Kim, let me know if this is the case and I'll resubmit a patch just to
> > fix the build error.  Patches 2 and 3 are still applicable even if we
> > discard patch 1.
> 
> ok
> 
> WD, please feel free to apply the fix, otherwise I'll have a pull
> request for you coming up.

I'll submit a patch just fixing the build error shortly.

Peter