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

[Chander Kashyap]

Dear Wolfgang Denk,

On 3 August 2011 09:17, Chander Kashyap <chander.kashyap at linaro.org> wrote:

> Dear Wolfgang Denk,
>
> On 31 July 2011 15:30, Wolfgang Denk <wd at denx.de> wrote:
>
>> 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.
>>
>
> 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