[U-Boot] [PATCH v2] s5pc1xx: Add support for Samsung Goni board

[Wolfgang Denk]

Dear Minkyu Kang,

In message <4C0309E2.2030206 at samsung.com> you wrote:
> This patch adds support for the Samsung Goni board (S5PC110 SoC)
> 
> Signed-off-by: Minkyu Kang <mk7.kang at samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>

> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -628,6 +628,7 @@ Simon Kagstrom <simon.kagstrom at netinsight.net>
>  
>  Minkyu Kang <mk7.kang at samsung.com>
>  
> +	s5p_goni	ARM CORTEX-A8 (S5PC110 SoC)
>  	SMDKC100	ARM CORTEX-A8 (S5PC100 SoC)
>  
>  Nishant Kamat <nskamat at ti.com>

Please keep lists sorted (kang > Kamat).

...
> +#ifdef CONFIG_DISPLAY_BOARDINFO
> +int checkboard(void)
> +{
> +	printf("Board:\tGoni\n");

Please consider using puts() when you don't do any print formatting.

> index 0000000..4b72992
> --- /dev/null
> +++ b/board/samsung/goni/lowlevel_init.S
> @@ -0,0 +1,585 @@
> +/*
> + * Memory Setup stuff - taken from blob memsetup.S

Is it really necessary to code all this in assembler?

> +/*
> + * uart_asm_init: Initialize UART's pins
> + */
> +uart_asm_init:

For example, why cannot this be part of the UART driver and written in
C instead?


> index 0000000..6239444
> --- /dev/null
> +++ b/include/configs/s5p_goni.h
...
> +#undef CONFIG_CMD_BOOTD
> +#undef CONFIG_CMD_CONSOLE
> +#undef CONFIG_CMD_ECHO
> +#undef CONFIG_CMD_FPGA
> +#undef CONFIG_CMD_ITEST
> +#undef CONFIG_CMD_FLASH
> +#undef CONFIG_CMD_IMLS
> +#undef CONFIG_CMD_LOADB
> +#undef CONFIG_CMD_LOADS
> +#undef CONFIG_CMD_NAND
> +#undef CONFIG_CMD_MISC
> +#undef CONFIG_CMD_NFS
> +#undef CONFIG_CMD_SOURCE
> +#undef CONFIG_CMD_XIMG
> +#undef CONFIG_CMD_NET
> +#undef CONFIG_XYZMODEM

Many of these commands are pretty useful to the end user. Is there a
special reason for disabling these here? It seems you are not
especially restricted in terms of resources, so I don't understand
why you disable these.

> +/*-----------------------------------------------------------------------
> + * Stack sizes
> + *
> + * The stack sizes are set up in start.S using the settings below
> + */

Incorrect multiline comment style; please fix globally.


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
"...and the fully armed nuclear warheads, are, of  course,  merely  a
courtesy detail."