[U-Boot] [PATCH 2/2] ARM: Add support for IGEP COM AQUILA/CYGNUS

[Tom Rini <trini@ti.com>]

On Wed, Apr 03, 2013 at 03:12:03PM +0200, Enric Balletbo i Serra wrote:

> From: Enric Balletbo i Serra <eballetbo@iseebcn.com>
> 
> The IGEP COM AQUILA and CYGNUS are industrial processors modules with
> following highlights:
> 
>   o AM3352/AM3354 Texas Instruments processor
>   o Cortex-A8 ARM CPU
>   o 3.3 volts Inputs / Outputs use industrial
>   o 256 MB DDR3 SDRAM / 128 Megabytes FLASH
>   o MicroSD card reader on-board
>   o Ethernet controller on-board
>   o JTAG debug connector available
>   o Designed for industrial range purposes
> 
> Signed-off-by: Enric Balletbo i Serra <eballetbo@iseebcn.com>

In general, yay.  But some specific comments that I know you inherited:

[snip]
> +/* Display cpuinfo */
> +#define CONFIG_DISPLAY_CPUINFO
> +/* Add libfdt-based support */
> +#define CONFIG_OF_LIBFDT
> +/* Enable passing of ATAGs */
> +#define CONFIG_CMDLINE_TAG
> +#define CONFIG_SETUP_MEMORY_TAGS
> +#define CONFIG_INITRD_TAG

Do you really have to support ATAGS and FDT?  Just confirming.

> +/* Commands to include */
> +#include <config_cmd_default.h>
> +
> +#define CONFIG_CMD_ASKENV
> +#define CONFIG_CMD_BOOTZ
> +#define CONFIG_CMD_DHCP
> +#define CONFIG_CMD_ECHO
> +#define CONFIG_CMD_EXT2
> +#define CONFIG_CMD_EXT4
> +#define CONFIG_CMD_FAT
> +#define CONFIG_CMD_FS_GENERIC

With CONFIG_CMD_FS_GENERIC and CMD_EXT4 do you really need CMD_EXT2 set?

[snip]
> +#define CONFIG_ENV_VARS_UBOOT_CONFIG
> +#define CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
> +#define CONFIG_EXTRA_ENV_SETTINGS \
> +	"loadaddr=0x80200000\0" \
> +	"fdtaddr=0x80F80000\0" \
> +	"fdt_high=0xffffffff\0" \
> +	"rdaddr=0x81000000\0" \
> +	"bootfile=/boot/uImage\0" \
> +	"fdtfile=\0" \

You're setting the config options to get an easy run-time way to set
fdtfile but not providing a script command to set it nor a C function.
If you don't need run-time detection, just set fdtfile :)

> +/*
> + * memtest works on 8 MB in DRAM after skipping 32MB from
> + * start addr of ram disk
> + */
> +#define CONFIG_SYS_MEMTEST_START	(PHYS_DRAM_1 + (64 * 1024 * 1024))
> +#define CONFIG_SYS_MEMTEST_END		(CONFIG_SYS_MEMTEST_START \
> +					+ (8 * 1024 * 1024))

The comment is wrong, and you can do any of:
- Opt-out of mtest (and see Wolfgang's readme on why that's probably a
  good idea)
- Correct this to be all of RAM, minus a bit for a reasonably-sized
  U-Boot to be running in.

[snip]
> +#define MTDIDS_DEFAULT			"nand0=nand"
> +#define MTDPARTS_DEFAULT		"mtdparts=nand:512k(SPL),"\
> +					"1920k(U-Boot),128k(U-Boot Env),"\
> +					"5m(Kernel),-(File System)"

Setting aside such a large space for U-Boot is something else you
inherited, do you want to re-evaluate this or too late?

> +#define CONFIG_SPL_NET_SUPPORT
> +#define CONFIG_SPL_NET_VCI_STRING	"AM335x U-Boot SPL"
> +#define CONFIG_SPL_ETH_SUPPORT

Keeping in mind the errata involved, does your board support CPSW SPL
without needed the erratad-out mode?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130403/e4bccc70/attachment.pgp>