From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Wed, 26 Jun 2013 14:33:28 -0500
Subject: [U-Boot] [PATCH 2/2] powerpc/c29xpcie: add support for C29XPCIE
 board
In-Reply-To: <1372231410-23939-2-git-send-email-Po.Liu@freescale.com> (from
	Po.Liu@freescale.com on Wed Jun 26 02:23:30 2013)
References: <1372231410-23939-1-git-send-email-Po.Liu@freescale.com>
	<1372231410-23939-2-git-send-email-Po.Liu@freescale.com>
Message-ID: <1372275208.8183.36@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/26/2013 02:23:30 AM, Po Liu wrote:
> +	set_tlb(1, flashbase, CONFIG_SYS_FLASH_BASE_PHYS,
> +			MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> +			0, flash_esel, BOOKE_PAGESZ_64M, 1);

Don't set MAS3_SX on I/O mappings.

> +	/* *I*G - Board CPLD */
> +	SET_TLB_ENTRY(1, CONFIG_SYS_CPLD_BASE,  
> CONFIG_SYS_CPLD_BASE_PHYS,
> +			MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> +			0, 5, BOOKE_PAGESZ_4K, 1),
> +
> +	SET_TLB_ENTRY(1, CONFIG_SYS_NAND_BASE,  
> CONFIG_SYS_NAND_BASE_PHYS,
> +			MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> +			0, 6, BOOKE_PAGESZ_1M, 1),

Don't set MAS3_SX on I/O mappings.

> +	/* *I*G - platform SRAM */
> +	SET_TLB_ENTRY(1, CONFIG_SYS_PLATFORM_SRAM_BASE,
> +			CONFIG_SYS_PLATFORM_SRAM_BASE_PHYS,
> +			MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> +			0, 7, BOOKE_PAGESZ_256K, 1),
> +	SET_TLB_ENTRY(1, CONFIG_SYS_PLATFORM_SRAM_BASE + 0x40000,
> +			CONFIG_SYS_PLATFORM_SRAM_BASE_PHYS + 0x40000,
> +			MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> +			0, 8, BOOKE_PAGESZ_256K, 1),

Why is this I+G?

> +#ifndef __CONFIG_H
> +#define __CONFIG_H
> +
> +#ifdef CONFIG_36BIT
> +#define CONFIG_PHYS_64BIT
> +#endif

Why are you adding non-36bit support that will not even get build
testing?

> +/*
> + * Internal Definitions
> + *
> + * Boot Flags
> + */
> +#define BOOTFLAG_COLD	0x01		/* Normal Power-On:  
> Boot from FLASH */
> +#define BOOTFLAG_WARM	0x02		/* Software reboot */

These are not used anywhere.  Please go through this file and only  
retain
things which actually continue to make sense.

> +/*
> + * For booting Linux, the board info and command line data
> + * have to be in the first 64 MB of memory, since this is
> + * the maximum mapped by the Linux kernel during initialization.
> + */
> +#define CONFIG_SYS_BOOTMAPSZ	(64 << 20) /* Initial Memory map for  
> Linux */
> +#define CONFIG_SYS_BOOTM_LEN	(64 << 20) /* Increase max gunzip size  
> */
> +
> +#ifdef CONFIG_CMD_KGDB
> +#define CONFIG_KGDB_BAUDRATE	230400	/* speed to run kgdb serial  
> port */
> +#define CONFIG_KGDB_SER_INDEX	2	/* which serial port to  
> use */
> +#endif

Likewise, this KGDB stuff is bad copy-and-paste.

> +
> +/*
> + * Environment Configuration
> + */
> +
> +#ifdef CONFIG_TSEC_ENET
> +#define CONFIG_HAS_ETH0
> +#define CONFIG_HAS_ETH1
> +#endif
> +
> +#define CONFIG_ROOTPATH		"/opt/nfsroot"
> +#define CONFIG_BOOTFILE		"uImage"
> +#define CONFIG_UBOOTPATH	u-boot.bin/* U-Boot image on TFTP  
> server */
> +
> +/* default location for tftp and bootm */
> +#define CONFIG_LOADADDR		1000000
> +
> +#define CONFIG_BOOTDELAY	10	/* -1 disables auto-boot */
> +
> +#define CONFIG_BAUDRATE		115200
> +
> +#define	CONFIG_EXTRA_ENV_SETTINGS				 
> \
> +	"hwconfig=" __stringify(CONFIG_DEF_HWCONFIG)  "\0"	\
> +	"netdev=eth0\0"						\
> +	"uboot=" __stringify(CONFIG_UBOOTPATH) "\0"		\
> +	"loadaddr=1000000\0"				\
> +	"consoledev=ttyS0\0"				\
> +	"ramdiskaddr=2000000\0"				\
> +	"ramdiskfile=rootfs.ext2.gz.uboot\0"		\
> +	"fdtaddr=c00000\0"				\
> +	"fdtfile=c293pcie.dtb\0"			\

Why c293?  Doesn't this support c291 and c292 as well?

Better to not have any default, or an obviously non-working placeholder
like fdtfile=name/of/device-tree.dtb

-Scott