From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Mon, 24 Jun 2013 12:35:17 -0500
Subject: [U-Boot] [PATCH v2] powerpc/85xx: Add TWR-P10xx board support
In-Reply-To: <1372057290-23862-1-git-send-email-X.Xie@freescale.com> (from
	X.Xie@freescale.com on Mon Jun 24 02:01:30 2013)
Message-ID: <1372095317.8183.0@snotra>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 06/24/2013 02:01:30 AM, Xie Xiaobo wrote:
> TWR-P1025 Specification:

Please CC Andy Fleming on mpc85xx patches.

> +	/* W**G* - Flash, localbus */
> +	/* This will be changed to *I*G* after relocation to RAM. */
> +	SET_TLB_ENTRY(1, CONFIG_SYS_SSD_BASE, CONFIG_SYS_SSD_BASE_PHYS,
> +			MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> +			0, 5, BOOKE_PAGESZ_1M, 1),

Inappropriate comment (it's already *I*G*) and don't set MAS3_SX on I/O
regions.

> +#ifdef CONFIG_SYS_RAMBOOT
> +	/* *I*G - eSDHC boot */
> +	SET_TLB_ENTRY(1, CONFIG_SYS_DDR_SDRAM_BASE,  
> CONFIG_SYS_DDR_SDRAM_BASE,
> +			MAS3_SX|MAS3_SW|MAS3_SR, 0,
> +			0, 8, BOOKE_PAGESZ_1G, 1),
> +#endif

Comment says *I*G but it's actually ****.

> +/*
> + * Memory map
> + *
> + * 0x0000_0000 0x1fff_ffff	DDR		Up to 512MB cacheable
> + * 0x8000_0000 0xdfff_ffff	PCI Express Mem	1.5G non-cacheable(PCIe  
> * 3)
> + * 0xffc0_0000 0xffc3_ffff	PCI IO range	256k non-cacheable
> + *
> + * Localbus
> + * 0xe000_0000 0xe002_0000	SSD1289		128K non-cacheable

This says 128K, but elsewhere you map the LAW and TLB as 1M, and you
configure it as 64K in eLBC.

> + * 0xec00_0000 0xefff_ffff	FLASH		Up to 64M non-cacheable
> + *
> + * 0xff90_0000 0xff97_ffff	L2 SRAM		Up to 512K cacheable
> + * 0xffd0_0000 0xffd0_3fff	init ram	16K Cacheable
> + * 0xffe0_0000 0xffef_ffff	CCSR		1M non-cacheable

L2 SRAM, init ram, and CCSR do not belong under "Localbus".

> +/* Serial Port
> + * open - index 2
> + * shorted - index 1
> + */
> +#define CONFIG_CONS_INDEX		1
> +#undef CONFIG_SERIAL_SOFTWARE_FIFO

Don't undef things that were never defined in the first place.  The only
place in the entire tree that CONFIG_SERIAL_SOFTWARE_FIFO exists at all
is a few similar undefs in other Freescale boards.

> +/* Use the HUSH parser */
> +#define CONFIG_SYS_HUSH_PARSER
> +#ifdef CONFIG_SYS_HUSH_PARSER
> +#define CONFIG_SYS_PROMPT_HUSH_PS2 "> "
> +#endif

This is a pointless ifdef, and a pointless setting of PS2 since that's
already the default.

> +#define CONFIG_BOOTDELAY 10	/* -1 disables auto-boot */

Please consider defaulting to -1 here, now that it no longer disables
bootdelay support at compile-time.

-Scott