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

Wolfgang Denk
2009-08-17T09:50:33Z

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_