[U-Boot] [PATCH 2/2] ARM: Add support for IGEP COM AQUILA/CYGNUS
Enric Balletbo Serra
eballetbo at gmail.com
Thu Apr 4 09:36:39 CEST 2013
Hi Tom,
Thanks for your comments.
2013/4/4 Tom Rini <trini at ti.com>
> On Wed, Apr 03, 2013 at 03:12:03PM +0200, Enric Balletbo i Serra wrote:
>
> > From: Enric Balletbo i Serra <eballetbo at 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 at 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.
>
No, I'll remove.
>
> > +/* 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?
>
You have reason, has no sense, I'll remove too.
>
> [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 :)
>
>
I'll remove ftd related variables until fdt support is not tested.
> > +/*
> > + * 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)
>
Readed and convinced. Thanks for this point.
> - 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?
>
>
Is not too late, I'll reduce the U-Boot space, do you think 512k is
sufficient ?
> > +#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?
>
We'll change the default bootmode in production boards so has no sense
define this. I'll remove.
I'll send version 2, thanks again.
Cheers,
Enric
>
> --
> Tom
>
More information about the U-Boot
mailing list