On Fri, Sep 4, 2009 at 7:56 PM, Wolfgang Denk<w...@denx.de> wrote:
> Dear Minkyu Kang,
>
> In message <4aa0ce4a.3060...@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?
Since it is used at OneNAND IPL. It has size limitation.
>
>
>> 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.
Agreed.
>
>> 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?
Since now we only tested on OneNAND. If someone or who want to use
these feature just remove it at that time.
>
> 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.
Okay.
Thank you,
Kyungmin Park
>
> ...
>> +#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: w...@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@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
