[U-Boot] [PATCH 3/3] qong: support for Dave/DENX QongEVB-LITE board

Wolfgang Denk
2009-02-02T20:43:35Z

Dear Ilya Yanok,

In message <1233589490-14293-4-git-send-email-yanok@emcraft.com> you wrote:
> This patch adds support for Dave/DENX QongEVB-LITE i.MX31-based board.
>
> Signed-off-by: Ilya Yanok <yanok@emcraft.com>
...
> board/dave/qong/Makefile | 51 +++++++++
> board/dave/qong/config.mk | 1 +
> board/dave/qong/lowlevel_init.S | 170 +++++++++++++++++++++++++++++++
> board/dave/qong/qong.c | 167 ++++++++++++++++++++++++++++++
> board/dave/qong/qong_fpga.h | 41 ++++++++
> board/dave/qong/u-boot.lds | 59 +++++++++++

The vendor name should be "davedenx", thus the directory name should
be board/davedenx/qong/, please.

> diff --git a/board/dave/qong/qong.c b/board/dave/qong/qong.c
> new file mode 100644
> index 0000000..5d12a13
> --- /dev/null
> +++ b/board/dave/qong/qong.c
> @@ -0,0 +1,167 @@
...
> +int dram_init (void)
> +{
> + gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
> + gd->bd->bi_dram[0].size = PHYS_SDRAM_1_SIZE;
> +
> + return 0;
> +}

Why do we not auto-size the RAM here like U-Boot is supposed to do?

> diff --git a/board/dave/qong/u-boot.lds b/board/dave/qong/u-boot.lds
> new file mode 100644
> index 0000000..1460adc
> --- /dev/null
> +++ b/board/dave/qong/u-boot.lds
> @@ -0,0 +1,59 @@
> +/*
> + * January 2004 - Changed to support H4 device
> + * Copyright (c) 2004 Texas Instruments

Are you sure this is an appropriate comment for this file?

> diff --git a/include/configs/qong.h b/include/configs/qong.h
> new file mode 100644
> index 0000000..8fae342
> --- /dev/null
> +++ b/include/configs/qong.h
...
> +/*
> + * Reducing the ARP timeout from default 5 seconds to 200ms we speed up the
> + * initial TFTP transfer, should the user wish one, significantly.
> + */
> +#define CONFIG_ARP_TIMEOUT 200UL

Is this really necessary on this hardware?

> +/* allow to overwrite serial and ethaddr */
> +#define CONFIG_ENV_OVERWRITE

Please don't.

> +/***********************************************************
> + * Command definition
> + ***********************************************************/
> +
> +#include <config_cmd_default.h>
> +
> +#define CONFIG_CMD_PING
> +#define CONFIG_CMD_DHCP
> +#define CONFIG_CMD_NET
> +#define CONFIG_CMD_MII
> +
> +#define CONFIG_ETHADDR 00:50:c2:1e:af:e7
> +#define CONFIG_NETMASK 255.255.0.0
> +#define CONFIG_IPADDR 192.168.0.81
> +#define CONFIG_SERVERIP 192.168.1.1
> +#define CONFIG_GATEWAYIP 192.168.1.1

Please never, never ever hardcode any network configuration into
U-Boot. Delete this, please.

And please add image timestamp support.

> +#define CONFIG_BOOTDELAY 3

Make this standard 5 seconds, please.

> +#define CONFIG_EXTRA_ENV_SETTINGS \
> + "netdev=eth0\0" \
> + "nfsargs=setenv bootargs root=/dev/nfs rw " \
> + "nfsroot=${serverip}:${rootpath}\0" \
> + "ramargs=setenv bootargs root=/dev/ram rw\0" \
> + "addip=setenv bootargs ${bootargs} " \
> + "ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}" \
> + ":${hostname}:${netdev}:off panic=1\0" \
> + "addtty=setenv bootargs ${bootargs}" \
> + " console=ttymxc0,${baudrate}\0" \
> + "addmisc=setenv bootargs ${bootargs}\0" \
> + "uboot_addr=0xa0000000\0" \
--------------------^^
> + "uboot=qong/u-boot.bin\0" \
> + "kernel_addr_r=0x80800000\0" \
-----------------------^^

No need for 0x prefix.

> + "hostname=qong\0" \
> + "bootfile=qong/uImage\0" \
> + "rootpath=/opt/eldk-4.2-arm/armVFP\0" \
> + "flash_self=run ramargs addip addtty addmisc;" \
> + "bootm ${kernel_addr} ${ramdisk_addr}\0" \
> + "flash_nfs=run nfsargs addip addtty addmisc;" \
> + "bootm ${kernel_addr}\0" \
> + "net_nfs=tftp ${kernel_addr_r} ${bootfile};" \
> + "run nfsargs addip addtty addmisc;" \
> + "bootm ${kernel_addr_r}\0" \

Pleain "bootm" would do as well.

> + "load=tftp ${loadaddr} ${uboot}\0" \
> + "update=protect off " xstr(CONFIG_SYS_MONITOR_BASE) " +" \
> + xstr(CONFIG_SYS_MONITOR_LEN)";" \
----------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> + "era " xstr(CONFIG_SYS_MONITOR_BASE) " +" \
> + xstr(CONFIG_SYS_MONITOR_LEN)";" \
----------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Please use "+${filesize}" instead.

> +#define CONFIG_SYS_PBSIZE (CONFIG_SYS_CBSIZE + \
> + sizeof(CONFIG_SYS_PROMPT) + 16)
> +#define CONFIG_SYS_MAXARGS 16 /* max number of command args */

32 ?

> +#define CONFIG_SYS_MEMTEST_START 0 /* memtest works on */
> +#define CONFIG_SYS_MEMTEST_END 0x10000

Hm... tha RAM test operates on the first 64 kB of flash memory? Or am
I missing something?

> +#define CONFIG_ENV_IS_IN_FLASH 1
> +#define CONFIG_ENV_SECT_SIZE SZ_128K

Please do not use any of these funny "SZ_" definitions. These shall be
removed ASAP.

> +#define CONFIG_ENV_SIZE CONFIG_ENV_SECT_SIZE
> +#define CONFIG_ENV_ADDR (CONFIG_SYS_FLASH_BASE+(2*SZ_128K))

Ditto.

> +/*
> + * JFFS2 partitions
> + */
> +#undef CONFIG_JFFS2_CMDLINE

Why?

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Of all the things I've lost, I miss my mind the most.