From mboxrd@z Thu Jan  1 00:00:00 1970
From: Kyungmin Park <kmpark@infradead.org>
Date: Fri, 4 Sep 2009 20:09:10 +0900
Subject: [U-Boot] [PATCH 4/4] s5pc1xx: add support SMDKC100 board
In-Reply-To: <20090904105629.03583832E8DE@gemini.denx.de>
References: <4AA0CE4A.3060209@samsung.com>
	<20090904105629.03583832E8DE@gemini.denx.de>
Message-ID: <9c9fda240909040409u576a7149kd4a4666148bfafa7@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Fri, Sep 4, 2009 at 7:56 PM, Wolfgang Denk<wd@denx.de> wrote:
> 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?

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: 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
>