Tom,
One more question...
2013/4/4 Enric Balletbo Serra eballetbo@gmail.com
Hi Tom,
Thanks for your comments.
2013/4/4 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.
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