[U-Boot-Users] [PATCH V2] Add the Harris QUAD100HD AMCC 405EP-based board.

[Gary Jennejohn <garyj@denx.de>]

On Sat, 26 Apr 2008 08:22:02 +0200
Stefan Roese <sr@denx.de> wrote:

Hi Stefan,

> Looks much better now. Just some minor issues (nitpicking) left. See comments 
> below.
> 
> > +	/* taken from PPCBoot */
> > +	mtdcr(uicsr, 0xFFFFFFFF);	/* clear all ints */
> > +	mtdcr(uicer, 0x00000000);	/* disable all ints */
> > +	mtdcr(uiccr, 0x00000000);
> > +	mtdcr(uicpr, 0xFFFF7FFE);	/* set int polarities */
> > +	mtdcr(uictr, 0x00000000);	/* set int trigger levels */
> > +	mtdcr(uicsr, 0xFFFFFFFF);	/* clear all ints */
> > +	mtdcr(uicvcr, 0x00000001);	/* set vect base=0,INT0 highest priority */
> > +
> > +    	mtdcr (CPC0_SRR, 0x00040000);   /* Hold PCI bridge in reset */
> 
> Please use only one coding style in one file. So either:
> 
> 	func();
> 
> or
> 
> 	func ();
> 
> Since this file mostly consists of the func() style I suggest you switch to 
> this one completely.
> 

Ah yes.  I copied parts of this from PPCBoot and failed to notice the
different formatting in the various places.  Thanks!

> > +#define CFG_MONITOR_LEN		(256 * 1024)	/* Reserve 256 kB for Monitor	*/
> > +#define CFG_MALLOC_LEN		(128 * 1024)	/* Reserve 128 kB for malloc()	*/
> > +#define CFG_MONITOR_BASE	0xFFF80000
> 
> CFG_MONITOR_BASE at 0xfff80000 makes me think that the U-Boot image has a size 
> of 512k. But CFG_MONITOR_LEN is 256k. Which one is correct? I would think 
> that 256k should be enough for this image.
> 

That's a valid point.  Actually u-boot.bin is 512kB in length, but only
because there's a reset vector at 0xFFFFFFFC.  u-boot itself is really
only 256kB in size.

So now I wonder - what's correct?  If CFG_MONITOR_LEN applies to RAM,
which it seems to since it only really affects TOTAL_MALLOC_LEN, then
256kB is correct and no change is required.

> Starting here
> 
> > +#define CFG_FLASH_WORD_SIZE	unsigned short
> > +#define CFG_FLASH_ADDR0		0x0555
> > +#define CFG_FLASH_ADDR1		0x02aa
> > +#define FLASH_BASE0_PRELIM	CFG_FLASH_BASE	/* FLASH bank #0	*/
> 
> till here. These defines are most likely not needed for the CFI driver. Please 
> remove them.
> 

OK, I'll look into it.

---
Gary Jennejohn
*********************************************************************
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
*********************************************************************