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

[Enric Balletbo Serra]

Tom,

One more question...


2013/4/4 Enric Balletbo Serra <eballetbo at gmail.com>

> 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.
>

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
>>
>
>