[U-Boot] [PATCH 1/3 v2] ppc4xx: Add AMCC Arches board support (dual 460GT)

[Wolfgang Denk]

Dear Adam Graham,

In message <1222897538-18473-1-git-send-email-agraham at amcc.com> you wrote:
> The Arches Evaluation board is based on the AMCC 460GT SoC chip.  This board is a dual processor board with each processor providing independent resources for Rapid IO, Gigabit Ethernet, and serial communications.  Each 460GT has it's own 512MB DDR2 memory, 32MB NOR FLASH, UART, EEPROM and temperature sensor, along with a shared debug port.  The two 460GT's will communicate with each other via shared memory, Gigabit Ethernet and x1 PCI-Express.

Wow. Line length 448, that's more than 6 times the allowed max.

Please restrict your line length to < 70 characters.

...
> +int misc_init_r(void)
> +{
...
> +	/* reset all SGMII interfaces */
> +	mfsdr(SDR0_SRST1,   reg);
> +	reg |= (SDR0_SRST1_SGMII0 | SDR0_SRST1_SGMII1 | SDR0_SRST1_SGMII2);
> +	mtsdr(SDR0_SRST1, reg);
> +	mtsdr(SDR0_ETH_STS, 0xFFFFFFFF);
> +	mtsdr(SDR0_SRST1,   0x00000000);
> +
> +	for (timeout = 60; timeout > 0; timeout--) {
> +		udelay(1000);
> +		mfsdr(SDR0_ETH_PLL, eth_pll);
> +		if ((eth_pll & SDR0_ETH_PLL_PLLLOCK) != 0)
> +			break;
> +	}

Where is the 60 ms timeout coming from?

> --- a/include/configs/amcc-common.h
> +++ b/include/configs/amcc-common.h
> @@ -55,6 +55,13 @@
>  #endif
>  
>  /*
> + * Only very few boards have default netdev not set to eth0 (like Arches)
> + */
> +#if !defined(CONFIG_NETDEV)
> +#define CONFIG_NETDEV		eth0
> +#endif

Please don't add this to common code; please handle it locally in the
arches configuration.

> @@ -147,9 +154,11 @@
>  /*
>   * Booting and default environment
>   */
> +#if !defined(CONFIG_PREBOOT)
>  #define CONFIG_PREBOOT	"echo;"	\
>  	"echo Type \"run flash_nfs\" to mount root filesystem over NFS;" \
>  	"echo"
> +#endif

Ditto. [Also: what's the reason for this change? ]

> +	"net_self_load=tftp ${kernel_addr_r} ${bootfile};"		\
> +		"tftp ${fdt_addr_r} ${fdt_file};"			\
> +		"tftp ${ramdisk_addr_r} ${ramdisk_file};\0"		\

What is this needed for?

> diff --git a/include/configs/canyonlands.h b/include/configs/canyonlands.h
> index 2f162e1..acad9b3 100644
> --- a/include/configs/canyonlands.h
> +++ b/include/configs/canyonlands.h
...
> +#define CONFIG_PREBOOT \
> +	"setenv ethact ppc_4xx_eth1;" \

This should not be done in the preboot command, but as part of the
default config instead.

> -#define CFG_AHB_BASE		0xE2000000	/* internal AHB peripherals	*/
> +#define CFG_AHB_BASE		0xE2000000	/* internal AHB peripherals */

Is this change an improvement?

> @@ -223,6 +268,7 @@
>   * DDR SDRAM
>   *----------------------------------------------------------------------------*/
>  #if !defined(CONFIG_NAND_U_BOOT)
> +#if !defined(CONFIG_ARCHES)
>  /*
>   * NAND booting U-Boot version uses a fixed initialization, since the whole
>   * I2C SPD DIMM autodetection/calibration doesn't fit into the 4k of boot

Are you sure this is correct?

> +#if !defined(CONFIG_ARCHES)
>  /* Only Canyonlands (460EX) has USB */
>  #ifdef CONFIG_460EX

I think it would make more sense to move the "!defined(CONFIG_ARCHES)"
_after_ the "#ifdef CONFIG_460EX".

> +#else	/* defined(CONFIG_ARCHES) */
> +
> +/* Arches board does not have USB connectivity */
> +
> +/*
> + * Default environment variables
> + */
> +#define	CONFIG_EXTRA_ENV_SETTINGS					\

Ah... but these definitely do not belong into the "USB connectivity"
block...

>   */
> +#if !defined(CONFIG_ARCHES)
>  #define CONFIG_CMD_DATE
> -#define CONFIG_CMD_DTT
>  #define CONFIG_CMD_NAND
> +#define CONFIG_CMD_SNTP
> +#endif
> +#define CONFIG_CMD_DTT
>  #define CONFIG_CMD_PCI
>  #define CONFIG_CMD_SDRAM
> -#define CONFIG_CMD_SNTP
>  #ifdef CONFIG_460EX
>  #define CONFIG_CMD_EXT2
>  #define CONFIG_CMD_FAT

I think we should separate these tlistrs of commands, so we can keep
the lists sorted.

> +/*
> + * Arches has 32MBytes of NOR FLASH (Spansion 29GL256) so remap FLASH to
> + * EBC address which accepts bigger regions:
> + *   0xfe00.0000 -> 4.ce00.0000
> + */

Cannot we do this automatically, dependent on the actual size of flash
as determined by the code?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Q: What do you get when you cross an ethernet with an income statement?
A: A local area networth.