[U-Boot] [PATCH v4 1/2] ARMV7: Add support for Samsung ORIGEN board

[Wolfgang Denk]

Dear Chander Kashyap,

In message <1311914519-10531-2-git-send-email-chander.kashyap at linaro.org> you wrote:
> Origen board is based upon S5PV310 SoC which is similiar to
> S5PC210 SoC.
> 
> Signed-off-by: Chander Kashyap <chander.kashyap at linaro.org>
...
> +	/* APLL(1), MPLL(1), CORE(0), HPM(0) */
> +	ldr	r1, =0x0101
> +	ldr	r2, =0x14200			@CLK_SRC_CPU
...
> +	mov	r1, #0x10000
...
> +	ldr	r1, =0x00
> +	ldr	r2, =0x0C210			@CLK_SRC_TOP0
...
> +	ldr	r1, =0x00
> +	ldr	r2, =0x0C214			@CLK_SRC_TOP1_OFFSET
...
> +	ldr	r1, =0x10000
> +	ldr	r2, =0x10200			@CLK_SRC_DMC_OFFSET
...

..
> +	/*
> +	 * CLK_DIV_DMC0:
> +	 *
> +	 * CORE_TIMERS_RATIO[28]	0x1
> +	 * COPY2_RATIO[24]		0x3
> +	 * DMCP_RATIO[20]		0x1
> +	 * DMCD_RATIO[16]		0x1
> +	 * DMC_RATIO[12]		0x1
> +	 * DPHY_RATIO[8]		0x1
> +	 * ACP_PCLK_RATIO[4]		0x1
> +	 * ACP_RATIO[0]			0x3
> +	 */
> +	ldr	r1, =0x13111113
> +	ldr	r2, =0x010500			@CLK_DIV_DMC0_OFFSET
> +	str	r1, [r0, r2]


lease get rid of all these magic hard coded constants.  Use symbolic
names instead. If needed, auto-generate these from the respective C
structs.  If needed, create the C structs.

...
> --- /dev/null
> +++ b/board/samsung/origen/mem_setup.S
> @@ -0,0 +1,359 @@

What exactly is the reeason for not coding this in C ?

...
> +#ifdef CONFIG_MIU_1BIT_12_INTERLEAVED
> +	ldr	r1, =0x0000000c
> +	str	r1, [r0, #0x400]
> +	ldr	r1, =0x00000001
> +	str	r1, [r0, #0xc00]
> +#elif defined(CONFIG_MIU_1BIT_7_INTERLEAVED)
> +	ldr	r1, =0x00000007
> +	str	r1, [r0, #0x400]
> +	ldr	r1, =0x00000001
> +	str	r1, [r0, #0xc00]
> +#elif defined(CONFIG_MIU_2BIT_21_12_INTERLEAVED)
> +	ldr	r1, =0x2000150c
> +	str	r1, [r0, #0x400]
> +	ldr	r1, =0x00000001
> +	str	r1, [r0, #0xc00]
> +#elif defined(CONFIG_MIU_2BIT_21_7_INTERLEAVED)
> +	ldr	r1, =0x20001507
> +	str	r1, [r0, #0x400]
> +	ldr	r1, =0x00000001
> +	str	r1, [r0, #0xc00]
> +#elif defined(CONFIG_MIU_2BIT_31_INTERLEAVED)
> +	ldr	r1, =0x0000001d
> +	str	r1, [r0, #0x400]
> +	ldr	r1, =0x00000001
> +	str	r1, [r0, #0xc00]

See above.  Please get rid of all these hard-ciooded magic numbers.
[Eventually this happens automativcally when you rewrite this in C.]

> +int dram_init(void)
> +{
> +	gd->ram_size	= get_ram_size((long *)PHYS_SDRAM_1, PHYS_SDRAM_1_SIZE)
> +			+ get_ram_size((long *)PHYS_SDRAM_2, PHYS_SDRAM_2_SIZE)
> +			+ get_ram_size((long *)PHYS_SDRAM_3, PHYS_SDRAM_3_SIZE)
> +			+ get_ram_size((long *)PHYS_SDRAM_4, PHYS_SDRAM_4_SIZE);
> +
> +	return 0;
> +}
> +
> +void dram_init_banksize(void)
> +{
> +	gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
> +	gd->bd->bi_dram[0].size = PHYS_SDRAM_1_SIZE;
> +	gd->bd->bi_dram[1].start = PHYS_SDRAM_2;
> +	gd->bd->bi_dram[1].size = PHYS_SDRAM_2_SIZE;
> +	gd->bd->bi_dram[2].start = PHYS_SDRAM_3;
> +	gd->bd->bi_dram[2].size = PHYS_SDRAM_3_SIZE;
> +	gd->bd->bi_dram[3].start = PHYS_SDRAM_4;
> +	gd->bd->bi_dram[3].size = PHYS_SDRAM_4_SIZE;
> +}

How do you detect memory issues in one of the memory banks, say, a
memory error in bank 3?

> diff --git a/include/configs/origen.h b/include/configs/origen.h
> new file mode 100644
> index 0000000..3cd9bfe
> --- /dev/null
> +++ b/include/configs/origen.h
...
> +#define COPY_BL2_SIZE		0x80000
> +#define BL2_START_OFFSET	((CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE)/512)
> +#define BL2_SIZE_BLOC_COUNT	(COPY_BL2_SIZE/512)

Some of the defines in this file are not used anywhere in the code
(for example, COPY_BL2_SIZE, COPY_BL2_SIZEBL2_START_OFFSET or
BL2_SIZE_BLOC_COUNT.  Please remove unused defines from this patch.

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
Don't hit a man when he's down - kick him; it's easier.