[U-Boot] [PATCH V4 3/3] Add support for the LaCie ED Mini V2 board

[Albert ARIBAUD <albert.aribaud@free.fr>]

Prafulla Wadaskar a ?crit :

>> I can add some comments, although here most of the comments 
>> would simply 
>> paraphrase the code one way or the other, e.g.
>>
>>>> +     info->device_id = 0xBA; /* device ID of the 
>> MX29LV400CB is 0xBA */
> 
> This level documentation is good rather than nothing :-)

Ok, but I prefer to summarize it above the assignments, e.g. 'commands 
and unlock addresses are AMD-compliant for an 8-bit mode, 8-bit bus 
device' is enough to let the reader understand the assignments to 
portwidth, chipwidth, vendor, cmd_reset, interface, addr_unlock1 and 
addr_unlock2; I'll make sure all fields are covered.

> ...snip...
>>>> +
>>>> +lowlevel_init:
>>>> +
>>>> +     /* Use 'r4 as the base for internal register accesses */
>>>> +     ldr     r4, =EDMINIV2_INTERNAL_BASE
>>>> +
>>>> +     /* move internal registers from the default 0xD0000000
>>>> +      * to their intended location of 0xf1000000 */
>>>> +     ldr     r3, =0xD0000000
>>>> +     add     r3, r3, #0x20000
>>>> +        str  r4, [r3, #0x80]
>>>> +
>>>> +     /* Use R3 as the base for Device Bus registers */
>>>> +     add     r3, r4, #0x10000
>>>> +
>>>> +     /* init MPPs */
>>>> +     ldr     r6, =EDMINIV2_MPP0_7
>>>> +     str     r6, [r3, #0x000]
>>>> +     ldr     r6, =EDMINIV2_MPP8_15
>>>> +     str     r6, [r3, #0x004]
>>>> +     ldr     r6, =EDMINIV2_MPP16_23
>>>> +     str     r6, [r3, #0x050]
>>>> +
>>>> +     /* init GPIOs */
>>>> +     ldr     r6, =EDMINIV2_GPIO_OUT_ENABLE
>>>> +     str     r6, [r3, #0x104]
>>>> +
>>>> +     /* Use R3 as the base for DRAM registers */
>>>> +     add     r3, r4, #0x01000
>>>> +
>>>> +     /*DDR SDRAM Initialization Control */
>>>> +     ldr     r6, =0x00000001
>>>> +     str     r6, [r3, #0x480]
>>>> +
>>>> +     /* Use R3 as the base for PCI registers */
>>>> +     add     r3, r4, #0x31000
>>>> +
>>>> +     /* Disable arbiter */
>>>> +     ldr     r6, =0x00000030
>>>> +     str     r6, [r3, #0xd00]
>>>> +
>>>> +     /* Use R3 as the base for DRAM registers */
>>>> +     add     r3, r4, #0x01000
>>>> +
>>>> +     /* set all dram windows to 0 */
>>>> +     mov     r6, #0
>>>> +     str     r6, [r3, #0x504]
>>>> +     str     r6, [r3, #0x50C]
>>>> +     str     r6, [r3, #0x514]
>>>> +     str     r6, [r3, #0x51C]
>>>> +
>>>> +     /* 1) Configure SDRAM  */
>>>> +     ldr     r6, =SDRAM_CONFIG
>>>> +     str     r6, [r3, #0x400]
>>>> +
>>>> +     /* 2) Set SDRAM Control reg */
>>>> +     ldr     r6, =SDRAM_CONTROL
>>>> +     str     r6, [r3, #0x404]
>>>> +
>>>> +        /* 3) Write SDRAM address control register */
>>>> +     ldr     r6, =SDRAM_ADDR_CTRL
>>>> +     str     r6, [r3, #0x410]
>>>> +
>>>> +        /* 4) Write SDRAM bank 0 size register */
>>>> +     ldr     r6, =SDRAM_BANK0_SIZE
>>>> +     str     r6, [r3, #0x504]
>>>> +     /* keep other banks disabled */
>>>> +
>>>> +        /* 5) Write SDRAM open pages control register */
>>>> +     ldr     r6, =SDRAM_OPEN_PAGE_EN
>>>> +     str     r6, [r3, #0x414]
>>>> +
>>>> +        /* 6) Write SDRAM timing Low register */
>>>> +     ldr     r6, =SDRAM_TIME_CTRL_LOW
>>>> +     str     r6, [r3, #0x408]
>>>> +
>>>> +        /* 7) Write SDRAM timing High register */
>>>> +     ldr     r6, =SDRAM_TIME_CTRL_HI
>>>> +     str     r6, [r3, #0x40C]
>>>> +
>>>> +        /* 8) Write SDRAM mode register */
>>>> +        /* The CPU must not attempt to change the SDRAM Mode
>>>> register setting */
>>>> +        /* prior to DRAM controller completion of the DRAM
>>>> initialization     */
>>>> +        /* sequence. To guarantee this restriction, it is
>>>> recommended that    */
>>>> +        /* the CPU sets the SDRAM Operation register to NOP
>>>> command, performs */
>>>> +        /* read polling until the register is back in Normal
>>>> operation value, */
>>>> +        /* and then sets SDRAM Mode register to its new
>>>> value.                */
>>>> +
>>>> +     /* 8.1 write 'nop' to SDRAM operation */
>>>> +        ldr  r6, =SDRAM_OP_NOP
>>>> +     str     r6, [r3, #0x418]
>>>> +
>>>> +        /* 8.2 poll SDRAM operation until back in 
>> 'normal' mode.  */
>>>> +1:
>>>> +     ldr     r6, [r3, #0x418]
>>>> +     cmp     r6, #0
>>>> +     bne     1b
>>>> +
>>>> +        /* 8.3 Now its safe to write new value to SDRAM Mode
>>>> register         */
>>>> +     ldr     r6, =SDRAM_MODE
>>>> +     str     r6, [r3, #0x41C]
>>>> +
>>>> +        /* 8.4 Set new mode */
>>>> +        ldr  r6, =SDRAM_OP_SETMODE
>>>> +     str     r6, [r3, #0x418]
>>>> +
>>>> +        /* 8.5 poll SDRAM operation until back in 
>> 'normal' mode.  */
>>>> +2:
>>>> +     ldr     r6, [r3, #0x418]
>>>> +     cmp     r6, #0
>>>> +     bne     2b
>>>> +
>>>> +        /* DDR SDRAM Address/Control Pads Calibration */
>>>> +     ldr     r6, [r3, #0x4C0]
>>>> +
>>>> +        /* Set Bit [31] to make the register writable
>>>>            */
>>>> +     orr     r6, r6, #SDRAM_PAD_CTRL_WR_EN
>>>> +     str     r6, [r3, #0x4C0]
>>>> +
>>>> +     bic     r6, r6, #SDRAM_PAD_CTRL_WR_EN
>>>> +     bic     r6, r6, #SDRAM_PAD_CTRL_TUNE_EN
>>>> +     bic     r6, r6, #SDRAM_PAD_CTRL_DRVN_MASK
>>>> +     bic     r6, r6, #SDRAM_PAD_CTRL_DRVP_MASK
>>>> +
>>>> +        /* Get the final N locked value of driving strength
>>>> [22:17]     */
>>>> +        mov   r1, r6
>>>> +        mov   r1, r1, LSL #9
>>>> +        mov   r1, r1, LSR #26    /* r1[5:0]<DrvN>  =
>>>> r3[22:17]<LockN>   */
>>>> +        orr   r1, r1, r1, LSL #6 /* r1[11:6]<DrvP> =
>>>> r1[5:0]<DrvN>      */
>>>> +
>>>> +        /* Write to both <DrvN> bits [5:0] and <DrvP> bits
>>>> [11:6]       */
>>>> +     orr     r6, r6, r1
>>>> +     str     r6, [r3, #0x4C0]
>>>> +
>>>> +        /* DDR SDRAM Data Pads Calibration
>>>>               */
>>>> +     ldr     r6, [r3, #0x4C4]
>>>> +
>>>> +        /* Set Bit [31] to make the register writable
>>>>            */
>>>> +     orr     r6, r6, #SDRAM_PAD_CTRL_WR_EN
>>>> +     str     r6, [r3, #0x4C4]
>>>> +
>>>> +     bic     r6, r6, #SDRAM_PAD_CTRL_WR_EN
>>>> +     bic     r6, r6, #SDRAM_PAD_CTRL_TUNE_EN
>>>> +     bic     r6, r6, #SDRAM_PAD_CTRL_DRVN_MASK
>>>> +     bic     r6, r6, #SDRAM_PAD_CTRL_DRVP_MASK
>>>> +
>>>> +        /* Get the final N locked value of driving strength
>>>> [22:17]     */
>>>> +        mov   r1, r6
>>>> +        mov   r1, r1, LSL #9
>>>> +        mov   r1, r1, LSR #26
>>>> +        orr   r1, r1, r1, LSL #6 /* r1[5:0] = r3[22:17]<LockN>  */
>>>> +
>>>> +        /* Write to both <DrvN> bits [5:0] and <DrvP> bits
>>>> [11:6]       */
>>>> +     orr     r6, r6, r1
>>>> +
>>>> +     str     r6, [r3, #0x4C4]
>>>> +
>>>> +        /* Implement Guideline (GL# MEM-3) Drive Strength
>>>> Value         */
>>>> +        /* Relevant for: 88F5181-A1/B0/B1 and 88F5281-A0/B0
>>>>            */
>>>> +
>>>> +        ldr     r1, =DDR1_PAD_STRENGTH_DEFAULT
>>>> +
>>>> +     /* Enable writes to DDR SDRAM Addr/Ctrl Pads
>>>> Calibration register */
>>>> +     ldr     r6, [r3, #0x4C0]
>>>> +     orr     r6, r6, #SDRAM_PAD_CTRL_WR_EN
>>>> +     str     r6, [r3, #0x4C0]
>>>> +
>>>> +     /* Correct strength and disable writes again */
>>>> +     bic     r6, r6, #SDRAM_PAD_CTRL_WR_EN
>>>> +     bic     r6, r6, #SDRAM_PAD_CTRL_DRV_STR_MASK
>>>> +     orr     r6, r6, r1
>>>> +     str     r6, [r3, #0x4C0]
>>>> +
>>>> +     /* Enable writes to DDR SDRAM Data Pads Calibration 
>> register */
>>>> +     ldr     r6, [r3, #0x4C4]
>>>> +     orr     r6, r6, #SDRAM_PAD_CTRL_WR_EN
>>>> +     str     r6, [r3, #0x4C4]
>>>> +
>>>> +     /* Correct strength and disable writes again */
>>>> +     bic     r6, r6, #SDRAM_PAD_CTRL_DRV_STR_MASK
>>>> +     bic     r6, r6, #SDRAM_PAD_CTRL_WR_EN
>>>> +     orr     r6, r6, r1
>>>> +     str     r6, [r3, #0x4C4]
>>>> +
>>>> +        /* Implement Guideline (GL# MEM-4) DQS Reference
>>>> Delay Tuning   */
>>>> +        /* Relevant for: 88F5181-A1/B0/B1 and 88F5281-A0/B0
>>>>            */
>>>> +
>>>> +        /* Get the "sample on reset" register for the DDR
>>>> frequancy     */
>>>> +     ldr     r3, =0x10000
>>>> +        ldr  r6, [r3, #0x010]
>>>> +        ldr  r1, =MSAR_ARMDDRCLCK_MASK
>>>> +        and  r1, r6, r1
>>>> +
>>>> +        ldr  r6, =FTDLL_DDR1_166MHZ
>>>> +        cmp  r1, #MSAR_ARMDDRCLCK_333_167
>>>> +        beq  3f
>>>> +        cmp  r1, #MSAR_ARMDDRCLCK_500_167
>>>> +        beq  3f
>>>> +        cmp  r1, #MSAR_ARMDDRCLCK_667_167
>>>> +        beq  3f
>>>> +
>>>> +        ldr  r6, =FTDLL_DDR1_200MHZ
>>>> +        cmp  r1, #MSAR_ARMDDRCLCK_400_200_1
>>>> +        beq  3f
>>>> +        cmp  r1, #MSAR_ARMDDRCLCK_400_200
>>>> +        beq  3f
>>>> +        cmp  r1, #MSAR_ARMDDRCLCK_600_200
>>>> +        beq  3f
>>>> +        cmp  r1, #MSAR_ARMDDRCLCK_800_200
>>>> +        beq  3f
>>>> +
>>> As reported earlier comment for v3,
>>> this should only have simple DRAM initialization, which is 
>> only dependency to copy and start binary.
>>
>> Hmm... Those are fixes to allow/ensure DDRAM access, so I'd 
>> say this is 
>> a dependency to copy and start the binary.
> 
> The code here looks very long
> It's purpose is to initialize certain CPU registers.
> For specific board there is no need of any conditional initializations.
> 
> Similar to board/Marvell/Sheevaplug/kwbimage.cfg, can you abstract a data for CPU registers and values to be initialized through some data structures and a small function to read and copy them to respective registers?
>
> That will give better readability and easy updates for future users.

It's not only setting registers, there are some loops, so several 
register+value tables would be needed, but yes, I can give it a shot.

> lets keep least ASM code.

Ok.

> We cannot avoid lowlevel_init otherwise I could have preferred to omit it.

I would have too, but there's no choice there.

>>> Common to SoC stuff to be moved to cpu.c/h
>> This I can agree upon, however I don't see much that is common across 
>> SoCs here. Does that warrant creating a function at SoC level 
>> which will 
>> do practically nothing?
> 
> For ex. Mpp_init, GPIO_init and other init can go in cpu.c,
> You can declare respective init macros in edminv2.h and function calls in edminv2.c
> Thus those will be re-usable for other orion5X boards.
> 
> Also you can do basic CPU registers initialization as suggested above and
> Further tuning like reading "sample on reset" and updating some specific registers can be pushed in cpu.c
> Because this will be a standard need to all board using this SoC
> 
> What do you think?

I'll give a shot at moving as much code from lowlevel_init as I can into 
cpu.c with constants in edminiv2.h.

> Regards.
> Prafulla . .

Amicalement,
-- 
Albert.