From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chander Kashyap Date: Tue, 9 Aug 2011 16:02:59 +0530 Subject: [U-Boot] [PATCH v4 1/2] ARMV7: Add support for Samsung ORIGEN board In-Reply-To: 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 3 August 2011 09:17, Chander Kashyap wrote: > 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 > While doing this, I find that the values written to the registers are used just once. So do you still prefer to have them as macros ? I did convert the register offsets and addresses to macros, but did not find it right to have macros for register values that are used just once. Please advise. >> ... >> > --- /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 > -- with warm regards, Chander Kashyap