[U-Boot-Users] [PATCH V2] PPC4xx: Unify ECC Initialization Routines

[Stefan Roese <sr@denx.de>]

On Saturday 17 May 2008, Grant Erickson wrote:
> This patch continues laying the ground work for moving out-of-assembly
> and unifying the SDRAM initialization code for PowerPC 405EX[r]-based
> boards.
>
> To do so, this deduces by one the number of nearly-identical DRAM ECC
> initialization implementations for PowerPC 4xx processors utilizing a
> DDR/DDR2 SDRAM controller by merging two of them into a common, shared
> implementation.

Good idea. Thanks. Please find some comments below.

> The last revision of this patch failed to compile on 440ERX/GPX
> systems, so the appropriate preprocessor wrappers were added to
> address it.
>
> Signed-off-by: Grant Erickson <gerickson@nuovations.com>
> ---
>  cpu/ppc4xx/44x_spd_ddr.c |   51 ++------------------
>  cpu/ppc4xx/Makefile      |    1 +
>  cpu/ppc4xx/ecc.c         |  119
> ++++++++++++++++++++++++++++++++++++++++++++++ cpu/ppc4xx/ecc.h         |  
> 42 ++++++++++++++++
>  cpu/ppc4xx/sdram.c       |   46 +-----------------
>  5 files changed, 170 insertions(+), 89 deletions(-)
>  create mode 100644 cpu/ppc4xx/ecc.c
>  create mode 100644 cpu/ppc4xx/ecc.h
>
> diff --git a/cpu/ppc4xx/44x_spd_ddr.c b/cpu/ppc4xx/44x_spd_ddr.c
> index b9cf5cb..7f04ca6 100644
> --- a/cpu/ppc4xx/44x_spd_ddr.c
> +++ b/cpu/ppc4xx/44x_spd_ddr.c
> @@ -53,6 +53,10 @@
>  #include <ppc4xx.h>
>  #include <asm/mmu.h>
>
> +#ifdef CONFIG_DDR_ECC
> +#include "ecc.h"
> +#endif

It should be save to include this header unconditionally. Please remove the 
#ifdef here.

>  #if defined(CONFIG_SPD_EEPROM) &&					\
>  	(defined(CONFIG_440GP) || defined(CONFIG_440GX) ||		\
>  	 defined(CONFIG_440EP) || defined(CONFIG_440GR))
> @@ -296,10 +300,6 @@ static void program_tr0(unsigned long *dimm_populated,
>  			unsigned long num_dimm_banks);
>  static void program_tr1(void);
>
> -#ifdef CONFIG_DDR_ECC
> -static void program_ecc(unsigned long num_bytes);
> -#endif
> -
>  static unsigned long program_bxcr(unsigned long *dimm_populated,
>  				  unsigned char *iic0_dimm_addr,
>  				  unsigned long num_dimm_banks);
> @@ -418,7 +418,7 @@ long int spd_sdram(void) {
>  	/*
>  	 * If ecc is enabled, initialize the parity bits.
>  	 */
> -	program_ecc(total_size);
> +	ecc_init(CFG_SDRAM_BASE, total_size);
>  #endif
>
>  	return total_size;
> @@ -1402,45 +1402,4 @@ static unsigned long program_bxcr(unsigned long
> *dimm_populated,
>
>  	return(bank_base_addr);
>  }
> -
> -#ifdef CONFIG_DDR_ECC
> -static void program_ecc(unsigned long num_bytes)
> -{
> -	unsigned long bank_base_addr;
> -	unsigned long current_address;
> -	unsigned long end_address;
> -	unsigned long address_increment;
> -	unsigned long cfg0;
> -
> -	/*
> -	 * get Memory Controller Options 0 data
> -	 */
> -	mfsdram(mem_cfg0, cfg0);
> -
> -	/*
> -	 * reset the bank_base address
> -	 */
> -	bank_base_addr = CFG_SDRAM_BASE;
> -
> -	if ((cfg0 & SDRAM_CFG0_MCHK_MASK) != SDRAM_CFG0_MCHK_NON) {
> -		mtsdram(mem_cfg0, (cfg0 & ~SDRAM_CFG0_MCHK_MASK) | SDRAM_CFG0_MCHK_GEN);
> -
> -		if ((cfg0 & SDRAM_CFG0_DMWD_MASK) == SDRAM_CFG0_DMWD_32)
> -			address_increment = 4;
> -		else
> -			address_increment = 8;
> -
> -		current_address = (unsigned long)(bank_base_addr);
> -		end_address = (unsigned long)(bank_base_addr) + num_bytes;
> -
> -		while (current_address < end_address) {
> -			*((unsigned long*)current_address) = 0x00000000;
> -			current_address += address_increment;
> -		}
> -
> -		mtsdram(mem_cfg0, (cfg0 & ~SDRAM_CFG0_MCHK_MASK) |
> -			SDRAM_CFG0_MCHK_CHK);
> -	}
> -}
> -#endif /* CONFIG_DDR_ECC */
>  #endif /* CONFIG_SPD_EEPROM */
> diff --git a/cpu/ppc4xx/Makefile b/cpu/ppc4xx/Makefile
> index 178c5c6..800bb41 100644
> --- a/cpu/ppc4xx/Makefile
> +++ b/cpu/ppc4xx/Makefile
> @@ -45,6 +45,7 @@ COBJS	+= cpu.o
>  COBJS	+= cpu_init.o
>  COBJS	+= denali_data_eye.o
>  COBJS	+= denali_spd_ddr2.o
> +COBJS	+= ecc.o
>  COBJS	+= fdt.o
>  COBJS	+= gpio.o
>  COBJS	+= i2c.o
> diff --git a/cpu/ppc4xx/ecc.c b/cpu/ppc4xx/ecc.c
> new file mode 100644
> index 0000000..dd45b50
> --- /dev/null
> +++ b/cpu/ppc4xx/ecc.c
> @@ -0,0 +1,119 @@
> +/*
> + *    Copyright (c) 2008 Nuovation System Designs, LLC
> + *	Grant Erickson <gerickson@nuovations.com>
> + *
> + *    Copyright (c) 2005-2007 DENX Software Engineering, GmbH
> + *	Stefan Roese <sr@denx.de>
> + *
> + *    Copyright (c) 2002 Artesyn Technology
> + *	Jun Gu <jung@artesyncp.com>
> + *
> + *    Copyright (c) 2001 Wave 7 Optics
> + *	Bill Hunter <williamhunter@attbi.com>x
> + *
> + *    See file CREDITS for list of people who contributed to this
> + *    project.
> + *
> + *    This program is free software; you can redistribute it and/or
> + *    modify it under the terms of the GNU General Public License as
> + *    published by the Free Software Foundation; either version 2 of
> + *    the License, or (at your option) any later version.
> + *
> + *    This program is distributed in the hope that it will abe useful,
> + *    but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + *    GNU General Public License for more details.
> + *
> + *    You should have received a copy of the GNU General Public License
> + *    along with this program; if not, write to the Free Software
> + *    Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + *    MA 02111-1307 USA
> + *
> + *    Description:
> + *	This file implements generic DRAM ECC initialization for
> + *	PowerPC processors using a SDRAM DDR/DDR2 controller,
> + *	including the 405EX(r), 440GP/GX/EP/GR, 440SP(E), and
> + *	460EX/GT.
> + */
> +
> +#include <common.h>
> +#include <ppc4xx.h>
> +#include <ppc_asm.tmpl>
> +#include <ppc_defs.h>
> +#include <asm/processor.h>
> +
> +#include "ecc.h"
> +
> +#if !defined(CONFIG_440EPX) && !defined(CONFIG_440GRX)
> +#if defined(CONFIG_DDR_ECC) || defined(CONFIG_SDRAM_ECC)
> +/*
> + *  void ecc_init()
> + *
> + *  Description:
> + *    This routine initializes a range of DRAM ECC memory with known
> + *    data and enables ECC checking.
> + *
> + *  TO DO:
> + *    Improve performance by utilizing cache.

Yes. And please add something like this here:

* Make is more generally usable for other 4xx PPC variants (like 440EPx...).

> + *
> + *  Input(s):
> + *    start - A pointer to the start of memory covered by ECC requiring
> + *	      initialization.
> + *    size  - The size, in bytes, of the memory covered by ECC requiring
> + *	      initialization.
> + *
> + *  Output(s):
> + *    start - A pointer to the start of memory covered by ECC with
> + *	      CFG_ECC_PATTERN written to all locations and ECC data
> + *	      primed.
> + *
> + *  Returns:
> + *    N/A
> + */
> +void ecc_init(unsigned long * const start, unsigned long size)
> +{
> +	const unsigned long pattern = CFG_ECC_PATTERN;
> +	unsigned * const end = (unsigned long * const)((long)start + size);
> +	unsigned long * current = start;
> +	unsigned long mcopt1;
> +	long increment;
> +
> +	if (start >= end)
> +		return;
> +
> +	mfsdram(SDRAM_MCOPT1, mcopt1);
> +
> +	/* Enable ECC generation without checking or reporting */
> +
> +	mtsdram(SDRAM_MCOPT1, ((mcopt1 & ~SDRAM_MCOPT1_MCHK_MASK) |
> +			       SDRAM_MCOPT1_MCHK_GEN));
> +
> +	increment = sizeof(u32);
> +
> +#if defined(CONFIG_440)
> +	/*
> +	 * Look at the geometry of SDRAM (data width) to determine whether we
> +	 * can skip words when writing.
> +	 */
> +
> +	if ((mcopt1 & SDRAM_MCOPT1_DMWD_MASK) != SDRAM_MCOPT1_DMWD_32)
> +		increment = sizeof(u64);
> +#endif /* defined(CONFIG_440) */
> +
> +	while (current < end) {
> +		*current = pattern;
> +		 current = (unsigned long *)((long)current + increment);
> +	}
> +
> +	/* Wait until the writes are finished. */
> +
> +	sync();
> +	eieio();

Please drop the eieio(). It's not needed since we have the sync().

> +	/* Enable ECC generation with checking and no reporting */
> +
> +	mtsdram(SDRAM_MCOPT1, ((mcopt1 & ~SDRAM_MCOPT1_MCHK_MASK) |
> +			       SDRAM_MCOPT1_MCHK_CHK));
> +}
> +#endif /* defined(CONFIG_DDR_ECC) || defined(CONFIG_SDRAM_ECC) */
> +#endif /* !defined(CONFIG_440EPX) && !defined(CONFIG_440GRX) */
> diff --git a/cpu/ppc4xx/ecc.h b/cpu/ppc4xx/ecc.h
> new file mode 100644
> index 0000000..da1c4fd
> --- /dev/null
> +++ b/cpu/ppc4xx/ecc.h
> @@ -0,0 +1,42 @@
> +/*
> + *    Copyright (c) 2008 Nuovation System Designs, LLC
> + *	Grant Erickson <gerickson@nuovations.com>
> + *
> + *    Copyright (c) 2007 DENX Software Engineering, GmbH
> + *	Stefan Roese <sr@denx.de>
> + *
> + *    See file CREDITS for list of people who contributed to this
> + *    project.
> + *
> + *    This program is free software; you can redistribute it and/or
> + *    modify it under the terms of the GNU General Public License as
> + *    published by the Free Software Foundation; either version 2 of
> + *    the License, or (at your option) any later version.
> + *
> + *    This program is distributed in the hope that it will abe useful,
> + *    but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + *    GNU General Public License for more details.
> + *
> + *    You should have received a copy of the GNU General Public License
> + *    along with this program; if not, write to the Free Software
> + *    Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + *    MA 02111-1307 USA
> + *
> + *    Description:
> + *	This file implements ECC initialization for PowerPC processors
> + *	using the SDRAM DDR2 controller, including the 405EX(r),
> + *	440SP(E), 460EX and 460GT.
> + *
> + */
> +
> +#ifndef _ECC_H_
> +#define _ECC_H_
> +
> +#if !defined(CFG_ECC_PATTERN)
> +#define	CFG_ECC_PATTERN	0x00000000
> +#endif /* !defined(CFG_ECC_PATTERN) */
> +
> +extern void ecc_init(unsigned long * const start, unsigned long size);
> +
> +#endif /* _ECC_H_ */
> diff --git a/cpu/ppc4xx/sdram.c b/cpu/ppc4xx/sdram.c
> index 2724d91..bd0a827 100644
> --- a/cpu/ppc4xx/sdram.c
> +++ b/cpu/ppc4xx/sdram.c
> @@ -31,6 +31,9 @@
>  #include <ppc4xx.h>
>  #include <asm/processor.h>
>  #include "sdram.h"
> +#ifdef CONFIG_SDRAM_ECC
> +#include "ecc.h"
> +#endif

Again, please make it unconditional.

Thanks a lot.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================