[U-Boot] [PATCH] ARMV7: Add support for TRATS board

[Wolfgang Denk]

Dear "HeungJun, Kim",

In message <1325751248-23125-1-git-send-email-riverful.kim at samsung.com> you wrote:
> This patch adds support for Samsung TRATS board
> 
> Signed-off-by: HeungJun, Kim <riverful.kim at samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a56ca10..c856c59 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -700,6 +700,10 @@ Minkyu Kang <mk7.kang at samsung.com>
>  	s5p_goni		ARM ARMV7 (S5PC110 SoC)
>  	s5pc210_universal	ARM ARMV7 (EXYNOS4210 SoC)
>  
> +Heungjun Kim <riverful.kim at samsung.com>
> +
> +	trats			ARM ARMV7 (EXYNOS4210 SoC)
> +
>  Chander Kashyap <k.chander at samsung.com>

Please keep list sorted ("Ki..." goes after "Ka...").

> +++ b/board/samsung/trats/Makefile
> @@ -0,0 +1,44 @@
> +#
> +# Copyright (C) 2010 Samsung Electronics
> +# Heungjun Kim <riverful.kim at samsung.com>
> +#
> +# See file CREDITS for list of people who contributed to this
> +# project.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License version 2 as
> +# published by the Free Software Foundation.

Please see http://www.denx.de/wiki/view/U-Boot/Patches#Notes

We need GPLv2+ for all new files.  Please fix globally.

> + */
> +uart_asm_init:
...
> +system_clock_init:
...
> +system_power_init:
...

What are the compelling reasons to write thes functionms in assembler,
and not in C?

> +int board_init(void)
> +{
> +	gpio1 = (struct exynos4_gpio_part1 *) EXYNOS4_GPIO_PART1_BASE;
> +	gpio2 = (struct exynos4_gpio_part2 *) EXYNOS4_GPIO_PART2_BASE;
> +
> +	gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100;
> +
> +	check_hw_revision();
> +	printf("HW Revision:\t0x%x\n", board_rev);
> +
> +#if defined(CONFIG_PMIC)
> +	pmic_init();
> +#endif
> +
> +	return 0;
> +}

Does this code build free wof warnings?  I would expect to see
warnings here because of unused variables gpio1 and gpio2?

...
> +/*
> + * High Level Configuration Options
> + * (easy to change)
> + */
> +#define CONFIG_SAMSUNG		1	/* in a SAMSUNG core */
> +#define CONFIG_S5P		1	/* which is in a S5P Family */
> +#define CONFIG_EXYNOS4210	1	/* which is in a EXYNOS4210 */
> +#define CONFIG_TRATS		1	/* working with TRATS */

Please do not define values for variables that select features only.
Please fix globally.

> +#include <asm/arch/cpu.h>		/* get chip and board defs */
> +
> +#define CONFIG_ARCH_CPU_INIT
> +#define CONFIG_DISPLAY_CPUINFO
> +#define CONFIG_DISPLAY_BOARDINFO
> +
> +/* Keep L2 Cache Disabled */
> +#define CONFIG_SYS_L2CACHE_OFF		1
> +
> +#define CONFIG_SYS_SDRAM_BASE		0x40000000
> +#define CONFIG_SYS_TEXT_BASE		0x44800000
> +
> +/* input clock of PLL: TRATS has 24MHz input clock at EXYNOS4210 */
> +#define CONFIG_SYS_CLK_FREQ_C210	24000000
> +
> +#define CONFIG_SETUP_MEMORY_TAGS
> +#define CONFIG_CMDLINE_TAG
> +#define CONFIG_INITRD_TAG
> +#define CONFIG_REVISION_TAG
> +#define CONFIG_CMDLINE_EDITING
> +
> +/* MACH_TYPE_TRATS macro will be removed once added to mach-types */
> +#define MACH_TYPE_TRATS			3928
> +#define CONFIG_MACH_TYPE		MACH_TYPE_TRATS
> +
> +/* Size of malloc() pool */
> +#define CONFIG_SYS_MALLOC_LEN		(CONFIG_ENV_SIZE + (1 << 20))
> +
> +/* select serial console configuration */
> +#define CONFIG_SERIAL_MULTI	1
> +#define CONFIG_SERIAL2		1	/* use SERIAL 2 */
> +#define CONFIG_BAUDRATE		115200
> +
> +/* MMC */
> +#define CONFIG_GENERIC_MMC	1
> +#define CONFIG_MMC		1
> +#define CONFIG_S5P_MMC		1
> +
> +/* PWM */
> +#define CONFIG_PWM			1
...
> +#define MBRPARTS_DEFAULT	"20M(permanent)"\
> +				",20M(boot)"\
> +				",1G(system)"\
> +				",100M(swap)"\
> +				",-(UMS)\0"
...
> +	"mbrparts=" MBRPARTS_DEFAULT \

MBR ??? Is this a typo and you actually mean MTD ?


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
Es ist nicht genug zu wissen, man muß auch anwenden; es ist nicht ge-
nug zu wollen, man muß auch tun.   -- Goethe, Maximen und Reflexionen