Hi Tom, Thanks for your comments.
2013/4/4 Tom Rini <tr...@ti.com> > On Wed, Apr 03, 2013 at 03:12:03PM +0200, Enric Balletbo i Serra wrote: > > > From: Enric Balletbo i Serra <eballe...@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 <eballe...@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 >
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
