From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chander Kashyap Date: Wed, 3 Aug 2011 09:17:17 +0530 Subject: [U-Boot] [PATCH v4 1/2] ARMV7: Add support for Samsung ORIGEN board In-Reply-To: <20110731100009.E4D6812B7AE4@gemini.denx.de> References: <1311914519-10531-1-git-send-email-chander.kashyap@linaro.org> <1311914519-10531-2-git-send-email-chander.kashyap@linaro.org> <20110731100009.E4D6812B7AE4@gemini.denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Dear Wolfgang Denk, On 31 July 2011 15:30, Wolfgang Denk wrote: > Dear Chander Kashyap, > > In message <1311914519-10531-2-git-send-email-chander.kashyap@linaro.org> > you wrote: > > Origen board is based upon S5PV310 SoC which is similiar to > > S5PC210 SoC. > > > > Signed-off-by: Chander Kashyap > ... > > + /* 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. > I will change hard coded values to symbolic names. > > ... > > --- /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.] I will use symbolic names. > > > +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? > get_ram_size will take care for it. > > 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. > > These are used in spl code. I will move them to spl 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. > -- with warm regards, Chander Kashyap