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

[Minkyu Kang <promsoft@gmail.com>]

Dear Wolfgang,

2009/9/4 Wolfgang Denk <wd@denx.de>

> Dear Minkyu Kang,
>
> In message <4AA0CE4A.3060209@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?
>

Because of this function called by lowlevel_init (before jumping to C code).
Any problem?
If so, we can merge to lowlevel_init.S.


>
>
> > 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.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

Thanks for review.
Minkyu Kang.

-- 
from. prom.
www.promsoft.net