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

[Wolfgang Denk]

Dear Ilya Yanok,

In message <1233589490-14293-4-git-send-email-yanok at emcraft.com> you wrote:
> This patch adds support for Dave/DENX QongEVB-LITE i.MX31-based board.
> 
> Signed-off-by: Ilya Yanok <yanok at 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.