Thanks. I'll fix them and resubmit the patch
On Tuesday, September 7, 2010, Wolfgang Denk <w...@denx.de> wrote:
> Dear Alex Ling,
>
> In message <1283869706-18506-1-git-send-email-kasiml...@gmail.com> you wrote:
>> This patch adds basic support for FriendlyARM MINI6410 development board (a
>> Chinese clone of Samsung SMDK6410)
>
> Please restrict your line length to < 70 characters.
>
>
> General note: please rebase your code on top of Heiko's ARm rework
> patches (enable caches, add relocation). These will go in before your
> board support.
>
>> Signed-off-by: Alex Ling <kasiml...@gmail.com>
>> ---
>> MAINTAINERS | 4 +
>> MAKEALL | 1 +
>> Makefile | 14 ++
>> board/samsung/mini6410/.gitignore | 5 +
>> board/samsung/mini6410/Makefile | 57 +++++
>> board/samsung/mini6410/config.mk | 35 +++
>> board/samsung/mini6410/lowlevel_init.S | 310
>> ++++++++++++++++++++++++++++
>> board/samsung/mini6410/mini6410.c | 86 ++++++++
>> board/samsung/mini6410/u-boot-nand.lds | 62 ++++++
>> include/configs/mini6410.h | 296 ++++++++++++++++++++++++++
>> nand_spl/board/samsung/mini6410/Makefile | 112 ++++++++++
>> nand_spl/board/samsung/mini6410/config.mk | 41 ++++
>> nand_spl/board/samsung/mini6410/u-boot.lds | 61 ++++++
>> 13 files changed, 1084 insertions(+), 0 deletions(-)
>> create mode 100644 board/samsung/mini6410/.gitignore
>> create mode 100644 board/samsung/mini6410/Makefile
>> create mode 100644 board/samsung/mini6410/config.mk
>> create mode 100644 board/samsung/mini6410/lowlevel_init.S
>> create mode 100644 board/samsung/mini6410/mini6410.c
>> create mode 100644 board/samsung/mini6410/u-boot-nand.lds
>> create mode 100644 include/configs/mini6410.h
>> create mode 100644 nand_spl/board/samsung/mini6410/Makefile
>> create mode 100644 nand_spl/board/samsung/mini6410/config.mk
>> create mode 100644 nand_spl/board/samsung/mini6410/u-boot.lds
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 4b91b0f..e858c5c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -815,6 +815,10 @@ Alex Z
>> lart SA1100
>> dnp1110 SA1110
>>
>> +Alex Ling <kasiml...@gmail.com>
>> +
>> + MINI6410 ARM1176JZF-S (S3C6410)
>
> Please keep list sorted.
>
>> diff --git a/MAKEALL b/MAKEALL
>> index b34ae33..e3e3def 100755
>> --- a/MAKEALL
>> +++ b/MAKEALL
>> @@ -644,6 +644,7 @@ LIST_ARM11=" \
>> qong \
>> smdk6400 \
>> tnetv107x_evm \
>> + mini6410 \
>
> Please keep list sorted.
>
>> diff --git a/Makefile b/Makefile
>> index 4f1cb1b..200a365 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2235,6 +2235,20 @@ smdk6400_config : unconfig
>> @$(MKCONFIG) smdk6400 arm arm1176 smdk6400 samsung s3c64xx
>> @echo "CONFIG_NAND_U_BOOT = y" >> $(obj)include/config.mk
>>
>> +mini6410_noUSB_config \
>> +mini6410_config : unconfig
>> + @mkdir -p $(obj)include $(obj)board/samsung/mini6410
>> + @mkdir -p $(obj)nand_spl/board/samsung/mini6410
>> + @echo "#define CONFIG_NAND_U_BOOT" > $(obj)include/config.h
>> + @echo "CONFIG_NAND_U_BOOT = y" >> $(obj)include/config.mk
>> + @if [ -z "$(findstring mini6410_noUSB_config,$@)" ]; then
>> \
>> + echo "RAM_TEXT = 0x57e00000" >>
>> $(obj)board/samsung/mini6410/config.tmp;\
>> + else
>> \
>> + echo "RAM_TEXT = 0xc7e00000" >>
>> $(obj)board/samsung/mini6410/config.tmp;\
>> + fi
>> + @$(MKCONFIG) mini6410 arm arm1176 mini6410 samsung s3c64xx
>> + @echo "CONFIG_NAND_U_BOOT = y" >> $(obj)include/config.mk
>
> NAK. We don't allow such changes any more. Please see previous
> discussions about this topic and configure using boards.cfg instead.
>
>> diff --git a/board/samsung/mini6410/.gitignore
>> b/board/samsung/mini6410/.gitignore
>> new file mode 100644
>> index 0000000..25ab492
>> --- /dev/null
>> +++ b/board/samsung/mini6410/.gitignore
>> @@ -0,0 +1,5 @@
>> +#
>> +# Generated files
>> +#
>> +
>> +/config.tmp
>
> Try to avoid such a file.
>
>> diff --git a/board/samsung/mini6410/Makefile
>> b/board/samsung/mini6410/Makefile
>> new file mode 100644
>> index 0000000..ef86f48
>> --- /dev/null
>> +++ b/board/samsung/mini6410/Makefile
> ...
>> +LIB = $(obj)lib$(BOARD).a
>> +
>> +COBJS-y := mini6410.o
>
> There is no configuration doen here, so rename into plain COBJS.
>
>> diff --git a/board/samsung/mini6410/lowlevel_init.S
>> b/board/samsung/mini6410/lowlevel_init.S
>> new file mode 100644
>> index 0000000..c00d0ab
> ...
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>> + */
>> +
>> +
>
> One blank line should be sufficient.
>
>> +#ifdef CONFIG_SERIAL1
>> +#define ELFIN_UART_CONSOLE_BASE (ELFIN_UART_BASE + ELFIN_UART0_OFFSET)
>> +#elif defined(CONFIG_SERIAL2)
>> +#define ELFIN_UART_CONSOLE_BASE (ELFIN_UART_BASE + ELFIN_UART1_OFFSET)
>> +#else
>> +#define ELFIN_UART_CONSOLE_BASE (ELFIN_UART_BASE + ELFIN_UART2_OFFSET)
>> +#endif
>
> These seem to be unused. Please remove dead code.
>
>> + /*
>> + * This was unconditional in original Samsung sources, but it doesn't
>> + * seem to make much sense on S3C6400.
>> + */
>> +#ifndef CONFIG_S3C6400
>
> So does this make sense on your board? Please remove dead code.
>
> ...
>> diff --git a/board/samsung/mini6410/mini6410.c
>> b/board/samsung/mini6410/mini6410.c
>> new file mode 100644
>> index 0000000..0db6f76
>> --- /dev/null
>> +++ b/board/samsung/mini6410/mini6410.c
> ...
>> +static inline void delay(unsigned long loops)
>> +{
>> + __asm__ volatile ("1:\n" "subs %0, %1, #1\n"
>> + "bne 1b"
>> + : "=r" (loops) : "0" (loops));
>> +}
>
> This is probably NOT what you want to do.
>
>> +int board_init(void)
>> +{
>> + DECLARE_GLOBAL_DATA_PTR;
>
> DECLARE_GLOBAL_DATA_PTR must be declared on file scope, not on
> function scope, or incorrect code may result. Please fix globally.
>
>> diff --git a/include/configs/mini6410.h b/include/configs/mini6410.h
>> new file mode 100644
>> index 0000000..3995970
>> --- /dev/null
>> +++ b/include/configs/mini6410.h
> ...
>> +#define CONFIG_SYS_HUSH_PARSER /* use "hush" command
>> parser */
>
> Line too long. Please fix globally.
>
>> +#define CONFIG_CMD_CACHE
>> +#define CONFIG_CMD_REGINFO
>> +#define CONFIG_CMD_LOADS
>> +#define CONFIG_CMD_LOADB
>> +#define CONFIG_CMD_SAVEENV
>> +#define CONFIG_CMD_NAND
>> +#if defined(CONFIG_BOOT_ONENAND)
>> +#define CONFIG_CMD_ONENAND
>> +#endif
>> +#define CONFIG_CMD_PING
>> +#define CONFIG_CMD_ELF
>> +#define CONFIG_CMD_FAT
>> +#define CONFIG_CMD_EXT2
>
> This would be easier to read if the list was sorted.
>
>> +#define CONFIG_SYS_MEMTEST_START CONFIG_SYS_SDRAM_BASE /* memtest
>> works on */
>> +#define CONFIG_SYS_MEMTEST_END (CONFIG_SYS_SDRAM_BASE +
>> 0x7e00000) /* 126MB in DRAM */
>
> Have you really tested these settings?
>
>
>> +/*-----------------------------------------------------------------------
>> + * Stack sizes
>> + *
>> + * The stack sizes are set up in start.S using the settings below
>> + */
>
> Incorrect multiline comment style.
>
>> +/**********************************
>> + Support Clock Settings
>> + **********************************
>
> Ditto. Please fix globally.
>
>> +/*#define CONFIG_CLK_667_133_66*/
>> +#define CONFIG_CLK_533_133_66
>> +/*
>> +#define CONFIG_CLK_400_100_50
>> +#define CONFIG_CLK_400_133_66
>> +#define CONFIG_SYNC_MODE
>> +*/
>
> Please remove dead code.
>
>> +/* SMDK6400 has 2 banks of DRAM, but we use only one in U-Boot */
>
> Why?
>
>> +/* None of these are currently implemented. Left from the original Samsung
>> + * version for reference
>> +#define CONFIG_BOOT_NOR
>> +#define CONFIG_BOOT_MOVINAND
>> +#define CONFIG_BOOT_ONENAND
>> +*/
>
> Please remove dead code. 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: w...@denx.de
> Too many people are ready to carry the stool when the piano needs to
> be moved.
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
