[U-Boot] [PATCH 4/4] s5pc1xx: add support SMDKC100 board

[Wolfgang Denk]

Dear Minkyu Kang,

In message <4AA0CE4A.3060209 at samsung.com> you wrote:
> Add new board SMDKC100 that uses s5pc100 SoC
...
> diff --git a/board/samsung/smdkc100/mem_setup.S b/board/samsung/smdkc100/mem_setup.S
> new file mode 100644
> index 0000000..a3e692f
> --- /dev/null
> +++ b/board/samsung/smdkc100/mem_setup.S

Why is this all written in assembly code? 

Cannot we use C instead?


> diff --git a/board/samsung/smdkc100/onenand.c b/board/samsung/smdkc100/onenand.c
> new file mode 100644
> index 0000000..75bb8a9
> --- /dev/null
> +++ b/board/samsung/smdkc100/onenand.c
...
> +static inline int onenand_read_reg(int offset)
> +{
> +	return readl(CONFIG_SYS_ONENAND_BASE + offset);
> +}
> +
> +static inline void onenand_write_reg(int value, int offset)
> +{
> +	writel(value, CONFIG_SYS_ONENAND_BASE + offset);
> +}

See previous comments about not using base address plus offset for
register accesses. Please change to use C structs.

...
> diff --git a/board/samsung/smdkc100/smdkc100.c b/board/samsung/smdkc100/smdkc100.c
> new file mode 100644
> index 0000000..4539ced
> --- /dev/null
> +++ b/board/samsung/smdkc100/smdkc100.c
...
> +#include <common.h>
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +int board_init(void)
> +{
> +	gd->bd->bi_arch_number = MACH_TYPE;

Please don;t hide this information - use MACH_TYPE_SMDKC100 directly.

> diff --git a/include/configs/smdkc100.h b/include/configs/smdkc100.h
> new file mode 100644
> index 0000000..ec0fd1d
> --- /dev/null
> +++ b/include/configs/smdkc100.h
...
> +/*
> + * Architecture magic and machine type
> + */
> +#define MACH_TYPE		1826

Please do not do this. I recommend to omit this completely, and use
the MACH_TYPE_SMDKC100 defintion from include/asm-arm/mach-types.h
instead.

> +/***********************************************************
> + * Command definition
> + ***********************************************************/
> +#include <config_cmd_default.h>
> +
> +#undef CONFIG_CMD_LOADB
> +#undef CONFIG_CMD_LOADS
> +#undef CONFIG_CMD_BOOTD
> +#undef CONFIG_CMD_FPGA
> +#undef CONFIG_CMD_XIMG
> +#undef CONFIG_CMD_NAND
> +#undef CONFIG_CMD_IMLS
> +#undef CONFIG_CMD_FLASH
> +#undef CONFIG_CMD_IMLS
> +#undef CONFIG_CMD_NET

Is there any specific reason for disabling these commands? Some of
these are extremely useful to the end user?

Also, you might want to sort such lists, and remove duplicate entries.

> +#if 1
> +#define CONFIG_BOOTCOMMAND	"run ubifsboot"
> +#else
> +#define CONFIG_BOOTCOMMAND	"bootm 0x31008000"
> +#endif

Please do not add dead code.

...
> +#define CONFIG_SYS_HZ		2085900		/* at PCLK 66.75MHz */

NAK. It is a mandatory requirement that CONFIG_SYS_HZ must be 1000.


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
Do not underestimate the value of print statements for debugging.
Don't have aesthetic convulsions when using them, either.