Tom, One more question...
2013/4/4 Enric Balletbo Serra <eballe...@gmail.com> > 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. > As explained in Wolfgang's readme shouldn't remove CONFIG_CMD_MEMTEST from config_cmd_default.h ? > > >> - 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
