[U-Boot] [PATCH 1/4] ARM Add New Board GEC2410

[Wolfgang Denk]

Dear "Hui.Tang",

In message <1257324286-19359-1-git-send-email-zetalabs at gmail.com> you wrote:
> This patch add a new ARM board GEC2410.
> checkpatch.pl shows that all things are fine.
> 
> total: 0 errors, 0 warnings, 1268 lines checked
> 
> 0001-ARM-Add-New-Board-GEC2410.patch has no obvious style problems and is ready for submission.

As mentioned before by others: this does noty belong into tht ecommit
message.


> diff --git a/Makefile b/Makefile
> index bcb3fe9..2de0b1d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2951,6 +2951,13 @@ davinci_dm365evm_config :	unconfig
>  davinci_dm6467evm_config :	unconfig
>  	@$(MKCONFIG) $(@:_config=) arm arm926ejs dm6467evm davinci davinci
>  
> +gec2410_config	:	unconfig
> +	@mkdir -p $(obj)include $(obj)board/gec/gec2410
> +	@mkdir -p $(obj)nand_spl/board/gec/gec2410
> +	@echo "RAM_TEXT = 0x33e00000" >> $(obj)board/gec/gec2410/config.tmp
> +	@$(MKCONFIG) $(@:_config=) arm arm920t gec2410 gec s3c24x0
> +	@echo "CONFIG_NAND_U_BOOT = y" >> $(obj)include/config.mk

Please so not put such scripting into the top level Makefile.
Especially since this is all unconditional. Please move to board
config file.

> diff --git a/board/gec/gec2410/config.mk b/board/gec/gec2410/config.mk
> new file mode 100644
> index 0000000..f32ee2e
> --- /dev/null
> +++ b/board/gec/gec2410/config.mk
...
> +sinclude $(OBJTREE)/board/$(BOARDDIR)/config.tmp
> +
> +ifndef CONFIG_NAND_SPL
> +TEXT_BASE = $(RAM_TEXT)
> +else
> +TEXT_BASE = 0
> +endif

There is nothing in the code that would set CONFIG_NAND_SPL, so please
get rid of the dead code.

> +LDSCRIPT := $(SRCTREE)/board/$(BOARDDIR)/u-boot-nand.lds

u-boot-nand.lds? Sure?

> +PLATFORM_CPPFLAGS += -I$(TOPDIR)/board/gec/gec2410

Why is this needed?

> diff --git a/board/gec/gec2410/gec2410.c b/board/gec/gec2410/gec2410.c
> new file mode 100644
> index 0000000..a9b6290
> --- /dev/null
> +++ b/board/gec/gec2410/gec2410.c
> @@ -0,0 +1,313 @@
...
> +#define FCLK_SPEED 1
> +
> +#if FCLK_SPEED == 0		/* Fout = 203MHz, Fin = 12MHz for Audio */
> +#define M_MDIV	0xC3
> +#define M_PDIV	0x4
> +#define M_SDIV	0x1
> +#elif FCLK_SPEED == 1		/* Fout = 202.8MHz */
> +#define M_MDIV	0xA1
> +#define M_PDIV	0x3
> +#define M_SDIV	0x1
> +#endif
> +
> +#define USB_CLOCK 1
> +
> +#if USB_CLOCK == 0
> +#define U_M_MDIV	0xA1
> +#define U_M_PDIV	0x3
> +#define U_M_SDIV	0x1
> +#elif USB_CLOCK == 1
> +#define U_M_MDIV	0x48
> +#define U_M_PDIV	0x3
> +#define U_M_SDIV	0x2
> +#endif

Please move to some header file.

> +static inline void delay(unsigned long loops)
> +{
> +	__asm__ volatile ("1:\n"
> +	  "subs %0, %1, #1\n"
> +	  "bne 1b" : "=r" (loops) : "0" (loops));
> +}

Is there really no better way to implement a delay? For example in a
way that does not depend on the system clock?

> --- a/cpu/arm920t/s3c24x0/timer.c
> +++ b/cpu/arm920t/s3c24x0/timer.c
> @@ -188,7 +188,8 @@ ulong get_tbclk(void)
>  	tbclk = timer_load_val * 100;
>  #elif defined(CONFIG_SBC2410X) || \
>        defined(CONFIG_SMDK2410) || \
> -      defined(CONFIG_VCMA9)
> +      defined(CONFIG_VCMA9) || \
> +      defined(CONFIG_GEC2410)
>  	tbclk = CONFIG_SYS_HZ;

Please keep list sorted.

And maybe we can use a feature-specific define instead of adding more
and more boards here?


> diff --git a/include/configs/gec2410.h b/include/configs/gec2410.h
> new file mode 100644
> index 0000000..8279e36
> --- /dev/null
> +++ b/include/configs/gec2410.h
> @@ -0,0 +1,262 @@
...
> +#if 0
> +#undef CONFIG_USE_IRQ			/* we don't need IRQ/FIQ stuff */
> +
> +#undef CONFIG_SKIP_RELOCATE_UBOOT
> +#endif

Please do not add dead code.

> +#include <config_cmd_default.h>
> +
> +#define CONFIG_CMD_CACHE
> +#define CONFIG_CMD_SAVEENV
> +#define CONFIG_CMD_NAND
> +#define CONFIG_CMD_PING
> +#define CONFIG_CMD_ELF
> +#define CONFIG_CMD_FAT

You may want to keep such lists sorted.

> diff --git a/nand_spl/board/gec/gec2410/u-boot.lds b/nand_spl/board/gec/gec2410/u-boot.lds
> new file mode 100644
> index 0000000..5871f7e
> --- /dev/null
> +++ b/nand_spl/board/gec/gec2410/u-boot.lds

This file seems to be unused. Why add it?


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
"A verbal contract isn't worth the paper it's printed on."
- Samuel Goldwyn