[U-Boot] [PATCH] Add basic support for FriendlyARM MINI6410 development board

Tue Sep 7 16:55:58 CEST 2010

Dear Alex Ling,

In message <1283869706-18506-1-git-send-email-kasimling at 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 <kasimling at 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 <kasimling at 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: wd at denx.de
Too many people are ready to carry the stool when the piano needs  to
be moved.