[U-Boot] [PATCH] Adding support for DevKit8000

[Wolfgang Denk <wd@denx.de>]

Dear Frederik Kriewitz,

In message <1250500736-20034-1-git-send-email-frederik@kriewitz.eu> you wrote:
> This patch adds support for the DevKit8000 board.
> 
> Signed-off-by: Frederik Kriewitz <frederik@kriewitz.eu>
> ---
>  Makefile                            |    3 +
>  board/omap3/devkit8000/Makefile     |   51 +++++
>  board/omap3/devkit8000/config.mk    |   34 +++
>  board/omap3/devkit8000/devkit8000.c |  117 +++++++++++
>  board/omap3/devkit8000/devkit8000.h |  376 +++++++++++++++++++++++++++++++++++
>  doc/README.omap3                    |   11 +
>  include/asm-arm/mach-types.h        |   13 ++
>  include/configs/omap3_devkit8000.h  |  365 ++++++++++++++++++++++++++++++++++
>  8 files changed, 970 insertions(+), 0 deletions(-)
>  create mode 100644 board/omap3/devkit8000/Makefile
>  create mode 100644 board/omap3/devkit8000/config.mk
>  create mode 100644 board/omap3/devkit8000/devkit8000.c
>  create mode 100644 board/omap3/devkit8000/devkit8000.h
>  create mode 100644 include/configs/omap3_devkit8000.h

There are some Coding Style issues like trailing white space, vertical
alignment by spaces instead of TAB as required, C++ comments etc.

Entry to MAINTAINERS missing.

> diff --git a/Makefile b/Makefile
> index 329e0f5..a040249 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3084,6 +3084,9 @@ omap3_zoom1_config :	unconfig
>  omap3_zoom2_config :	unconfig
>  	@$(MKCONFIG) $(@:_config=) arm arm_cortexa8 zoom2 omap3 omap3
>  
> +omap3_devkit8000_config : unconfig
> +	@$(MKCONFIG) $(@:_config=) arm arm_cortexa8 devkit8000 omap3 omap3

Please keep list sorted.

> diff --git a/board/omap3/devkit8000/Makefile b/board/omap3/devkit8000/Makefile
> new file mode 100644
> index 0000000..63e16b0
> --- /dev/null
> +++ b/board/omap3/devkit8000/Makefile
> @@ -0,0 +1,51 @@
> +#
> +# (C) Copyright 2000, 2001, 2002
> +# Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> +#
> +# Modified by Frederik Kriewitz <frederik@kriewitz.eu>

Please add Copyright and year, then (applies globally).

> diff --git a/doc/README.omap3 b/doc/README.omap3
> index 6227151..e24531c 100644
> --- a/doc/README.omap3
> +++ b/doc/README.omap3
> @@ -21,6 +21,8 @@ Currently the following boards are supported:
>  
>  * TI/Logic PD Zoom 2 [7]
>  
> +* DevKit8000 [9]
> +
>  Toolchain
>  =========
>  
> @@ -61,6 +63,11 @@ make
>  make omap3_zoom2_config
>  make
>  
> +* DevKit8000:
> +
> +make omap3_devkit8000_config
> +make

Please keep list sorted.

> diff --git a/include/asm-arm/mach-types.h b/include/asm-arm/mach-types.h
> index 5293d67..adacc90 100644
> --- a/include/asm-arm/mach-types.h
> +++ b/include/asm-arm/mach-types.h
> @@ -2241,6 +2241,7 @@ extern unsigned int __machine_arch_type;
>  #define MACH_TYPE_OMAP3_WL_FF          2258
>  #define MACH_TYPE_SIMCOM               2259
>  #define MACH_TYPE_MCWEBIO              2260
> +#define MACH_TYPE_OMAP3_DEVKIT8000     2330

Please don't do this. Instead, send a request for an update of the
mach-types.h file to the ARM custodian, please.

> diff --git a/include/configs/omap3_devkit8000.h b/include/configs/omap3_devkit8000.h
> new file mode 100644
> index 0000000..b2871c8
> --- /dev/null
> +++ b/include/configs/omap3_devkit8000.h
...
> +#define CONFIG_SYS_MALLOC_LEN		(CONFIG_ENV_SIZE + SZ_128K)
> +#define CONFIG_SYS_GBL_DATA_SIZE	128	/* bytes reserved for */
> +						/* initial data */
> +

One blank line is enough.


> +/* commands to include */
> +#include <config_cmd_default.h>
> +
> +#define CONFIG_CMD_EXT2			/* EXT2 Support			*/
> +#define CONFIG_CMD_FAT			/* FAT support			*/
> +#define CONFIG_CMD_JFFS2		/* JFFS2 Support		*/
> +#define CONFIG_CMD_MTDPARTS		/* Enable MTD parts commands    */
> +#define CONFIG_MTD_DEVICE		/* needed for mtdparts commands */ 
> +#define MTDIDS_DEFAULT			"nand0=nand"
> +#define MTDPARTS_DEFAULT		"mtdparts=nand:512k(x-loader),"\
> +					"1920k(u-boot),128k(u-boot-env),"\
> +					"4m(kernel),-(fs)"

I recommend not to mix the list of command definitions with other
defines. 

> +#define CONFIG_CMD_I2C			/* I2C serial bus support	*/
> +#define CONFIG_CMD_MMC			/* MMC support			*/
> +#define CONFIG_CMD_NAND			/* NAND support			*/
> +#define CONFIG_CMD_NAND_LOCK_UNLOCK	/* nand (un)lock commands   */

Also, it may make sense to kepp such lists sorted, too.

> +#define CONFIG_BOOTCOMMAND \
> +	"if mmc init 0; then " \
> +		"if run loadbootscript; then " \
> +			"run bootscript; " \
> +		"else " \
> +			"if run loaduimage; then " \
> +				"run mmcboot; " \
> +			"else run nandboot; " \
> +			"fi; " \
> +		"fi; " \
> +	"else run nandboot; fi"

In my opinion it makes little sense to define such a complex boot
command - you will lose the definition if you redefine it for
testing. Instead, define a variable containing this script, and use
"run variable" as boot command.

> +#define CONFIG_SYS_MEMTEST_START	(OMAP34XX_SDRC_CS0)	/* memtest */
> +								/* works on */
> +#define CONFIG_SYS_MEMTEST_END		(OMAP34XX_SDRC_CS0 + \
> +					0x01F00000) /* 31MB */

Did you test that these settings work?

..
> +#define CONFIG_SYS_MAX_FLASH_SECT	520	/* max number of sectors on */
> +						/* one chip */
> +#define CONFIG_SYS_MAX_FLASH_BANKS	2	/* max number of flash banks */
> +#define CONFIG_SYS_MONITOR_LEN		SZ_256K	/* Reserve 2 sectors */
> +
> +#define CONFIG_SYS_FLASH_BASE		boot_flash_base
> +
> +/* Monitor at start of flash */
> +#define CONFIG_SYS_MONITOR_BASE		CONFIG_SYS_FLASH_BASE

You seem to have NOR flash, but define NO_FLSH eralier. Is this
intentional? What is the rationale behind that?

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
If programming was easy, they wouldn't need something as  complicated
as a human being to do it, now would they?
                       - L. Wall & R. L. Schwartz, _Programming Perl_