[U-Boot-Users] [PATCH 1/2 v2] PPC4xx: Enable Primordial Stack for 40x and Unify ECC Handling

Stefan Roese sr at denx.de
Thu May 22 17:37:05 CEST 2008


Grant,

On Wednesday 21 May 2008, Grant Erickson wrote:
> This patch (Part 1 of 2):

Again, please find some comments below.

<snip>

> diff --git a/cpu/ppc4xx/start.S b/cpu/ppc4xx/start.S
> index a513b45..e43da7b 100644
> --- a/cpu/ppc4xx/start.S
> +++ b/cpu/ppc4xx/start.S
> @@ -3,6 +3,8 @@
>   *  Copyright (C) 1999	Magnus Damm <kieraypc01.p.y.kie.era.ericsson.se>
>   *  Copyright (C) 2000,2001,2002 Wolfgang Denk <wd at denx.de>
>   *  Copyright (C) 2007 Stefan Roese <sr at denx.de>, DENX Software
> Engineering + *  Copyright (c) 2008 Nuovation System Designs, LLC
> + *    Grant Erickson <gerickson at nuovations.com>
>   *
>   * See file CREDITS for list of people who contributed to this
>   * project.
> @@ -79,34 +81,100 @@
>  # if (CFG_INIT_DCACHE_CS == 0)
>  #  define PBxAP pb0ap
>  #  define PBxCR pb0cr
> +#  if (defined(CFG_EBC_PB0AP) && defined(CFG_EBC_PB0CR))
> +#   define PBxAP_VAL CFG_EBC_PB0AP
> +#   define PBxCR_VAL CFG_EBC_PB0CR
> +#  endif
>  # endif
>  # if (CFG_INIT_DCACHE_CS == 1)
>  #  define PBxAP pb1ap
>  #  define PBxCR pb1cr
> +#  if (defined(CFG_EBC_PB1AP) && defined(CFG_EBC_PB1CR))
> +#   define PBxAP_VAL CFG_EBC_PB1AP
> +#   define PBxCR_VAL CFG_EBC_PB1CR
> +#  endif
>  # endif
>  # if (CFG_INIT_DCACHE_CS == 2)
>  #  define PBxAP pb2ap
>  #  define PBxCR pb2cr
> +#  if (defined(CFG_EBC_PB2AP) && defined(CFG_EBC_PB2CR))
> +#   define PBxAP_VAL CFG_EBC_PB2AP
> +#   define PBxCR_VAL CFG_EBC_PB2CR
> +#  endif
>  # endif
>  # if (CFG_INIT_DCACHE_CS == 3)
>  #  define PBxAP pb3ap
>  #  define PBxCR pb3cr
> +#  if (defined(CFG_EBC_PB3AP) && defined(CFG_EBC_PB3CR))
> +#   define PBxAP_VAL CFG_EBC_PB3AP
> +#   define PBxCR_VAL CFG_EBC_PB3CR
> +#  endif
>  # endif
>  # if (CFG_INIT_DCACHE_CS == 4)
>  #  define PBxAP pb4ap
>  #  define PBxCR pb4cr
> +#  if (defined(CFG_EBC_PB4AP) && defined(CFG_EBC_PB4CR))
> +#   define PBxAP_VAL CFG_EBC_PB4AP
> +#   define PBxCR_VAL CFG_EBC_PB4CR
> +#  endif
>  # endif
>  # if (CFG_INIT_DCACHE_CS == 5)
>  #  define PBxAP pb5ap
>  #  define PBxCR pb5cr
> +#  if (defined(CFG_EBC_PB5AP) && defined(CFG_EBC_PB5CR))
> +#   define PBxAP_VAL CFG_EBC_PB5AP
> +#   define PBxCR_VAL CFG_EBC_PB5CR
> +#  endif
>  # endif
>  # if (CFG_INIT_DCACHE_CS == 6)
>  #  define PBxAP pb6ap
>  #  define PBxCR pb6cr
> +#  if (defined(CFG_EBC_PB6AP) && defined(CFG_EBC_PB6CR))
> +#   define PBxAP_VAL CFG_EBC_PB6AP
> +#   define PBxCR_VAL CFG_EBC_PB6CR
> +#  endif
>  # endif
>  # if (CFG_INIT_DCACHE_CS == 7)
>  #  define PBxAP pb7ap
>  #  define PBxCR pb7cr
> +#  if (defined(CFG_EBC_PB7AP) && defined(CFG_EBC_PB7CR))
> +#   define PBxAP_VAL CFG_EBC_PB7AP
> +#   define PBxCR_VAL CFG_EBC_PB7CR
> +#  endif
> +# endif
> +# ifndef PBxAP_VAL
> +#  define PBxAP_VAL	0
> +# endif
> +# ifndef PBxCR_VAL
> +#  define PBxCR_VAL	0
> +# endif
> +/*
> + * Memory Bank x (nothingness) initialization CFG_INIT_RAM_ADDR + 64 MiB
> + * used as temporary stack pointer for the primordial stack
> + */
> +# ifndef CFG_INIT_DCACHE_PBxAR
> +#  define CFG_INIT_DCACHE_PBxAR	(EBC_BXAP_BME_DISABLED			| \
> +				 EBC_BXAP_TWT_ENCODE(7)			| \
> +				 EBC_BXAP_BCE_DISABLE			| \
> +				 EBC_BXAP_BCT_2TRANS			| \
> +				 EBC_BXAP_CSN_ENCODE(0)			| \
> +				 EBC_BXAP_OEN_ENCODE(0)			| \
> +				 EBC_BXAP_WBN_ENCODE(0)			| \
> +				 EBC_BXAP_WBF_ENCODE(0)			| \
> +				 EBC_BXAP_TH_ENCODE(2)			| \
> +				 EBC_BXAP_RE_DISABLED			| \
> +				 EBC_BXAP_SOR_NONDELAYED		| \
> +				 EBC_BXAP_BEM_WRITEONLY			| \
> +				 EBC_BXAP_PEN_DISABLED)
> +# endif /* CFG_INIT_DCACHE_PBxAR */
> +# ifndef CFG_INIT_DCACHE_PBxCR
> +#  define CFG_INIT_DCACHE_PBxCR	(EBC_BXCR_BAS_ENCODE(CFG_INIT_RAM_ADDR)	|
> \ +				 EBC_BXCR_BS_64MB			| \
> +				 EBC_BXCR_BU_RW				| \
> +				 EBC_BXCR_BW_16BIT)
> +# endif /* CFG_INIT_DCACHE_PBxCR */
> +# ifndef CFG_INIT_RAM_PATTERN
> +#  define CFG_INIT_RAM_PATTERN	0xDEADDEAD
>  # endif
>  #endif /* CFG_INIT_DCACHE_CS */
>
> @@ -840,15 +908,25 @@ _start:
>  	/* make sure above stores all comlete before going on */
>  	sync
>
> -	/*-----------------------------------------------------------------------
> */ -	/* Enable two 128MB cachable regions. */
> -	/*-----------------------------------------------------------------------
> */ -	addis	r1,r0,0xc000
> -	addi	r1,r1,0x0001
> +	/*---------------------------------------------------------------------
> +	 * Enable two 128MB cachable instruction regions at CFG_SDRAM_BASE
> +	 * and another 128MB cacheable instruction region covering NOR flash
> +	 * at CFG_FLASH_BASE. Disbale all cacheable data regions.

"Disable" instead of "Disbale".

> +	 *------------------------------------------------------------------ */
> +
> +#define ICSACRVAL	(PPC_128MB_SACR_VALUE(CFG_SDRAM_BASE + (  0 << 20)) | \
> +			 PPC_128MB_SACR_VALUE(CFG_SDRAM_BASE + (128 << 20)) | \
> +			 PPC_128MB_SACR_VALUE(CFG_FLASH_BASE))

I don't like these #defines within the code. Please move them to the top of 
the file.

> +	lis	r1, ICSACRVAL at h
> +	ori	r1, r1, ICSACRVAL at l
>  	mticcr	r1			/* instruction cache */
> +	isync
>
> -	addis	r1,r0,0x0000
> -	addi	r1,r1,0x0000
> +#define DCSACRVAL	0x00000000

Same here.

> +	lis	r1, DCSACRVAL at h
> +	ori	r1, r1, DCSACRVAL at l
>  	mtdccr	r1			/* data cache */
>
>  	addis	r1,r0,CFG_INIT_RAM_ADDR at h
> @@ -902,16 +980,25 @@ _start:
>  	bl	invalidate_icache
>  	bl	invalidate_dcache
>
> -	/*-----------------------------------------------------------------------
> */ -	/* Enable two 128MB cachable regions. */
> -	/*-----------------------------------------------------------------------
> */ -	lis	r4,0xc000
> -	ori	r4,r4,0x0001
> +	/*---------------------------------------------------------------------
> +	 * Enable two 128MB cachable instruction regions at CFG_SDRAM_BASE
> +	 * and another 128MB cacheable instruction region covering NOR flash
> +	 * at CFG_FLASH_BASE. Disbale all cacheable data regions.
> +	 *------------------------------------------------------------------ */
> +
> +#define ICSACRVAL	(PPC_128MB_SACR_VALUE(CFG_SDRAM_BASE + (  0 << 20)) | \
> +			 PPC_128MB_SACR_VALUE(CFG_SDRAM_BASE + (128 << 20)) | \
> +			 PPC_128MB_SACR_VALUE(CFG_FLASH_BASE))

And now it's defined twice. Please remove.

> +	lis	r4, ICSACRVAL at h
> +	ori	r4, r4, ICSACRVAL at l
>  	mticcr	r4			/* instruction cache */
>  	isync
>
> -	lis	r4,0x0000
> -	ori	r4,r4,0x0000
> +#define DCSACRVAL	0x00000000

Here too.

Please fix and resubmit. Thanks.

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




More information about the U-Boot mailing list