[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 :

>> diff --git a/board/LaCie/edminiv2/config.mk

>> +TEXT_BASE = 0x00600000
> 
> ??? As reported earlier, is this okay for you? And do not want to lower it?

Fixed for good this time. :)

>> diff --git a/board/LaCie/edminiv2/edminiv2.c

>> +     info->flash_id                  = 0x01000000;
>> +     info->portwidth = FLASH_CFI_8BIT;
>> +     info->chipwidth = FLASH_CFI_BY8;
>> +     info->buffer_size = 0;
>> +     info->erase_blk_tout = 1000;
>> +     info->write_tout = 10;
>> +     info->buffer_write_tout = 300;
>> +     info->vendor = CFI_CMDSET_AMD_LEGACY;
>> +     info->cmd_reset = 0xF0;
>> +     info->interface = FLASH_CFI_X8;
>> +     info->legacy_unlock = 0;
>> +     info->manufacturer_id = 0x22;
>> +     info->device_id = 0xBA;
>> +     info->device_id2 = 0;
>> +     info->ext_addr = 0;
>> +     info->cfi_version = 0x3133;
>> +     info->cfi_offset = 0x0000;
>> +     info->addr_unlock1 = 0x00000aaa;
>> +     info->addr_unlock2 = 0x00000555;
>> +     info->name = "MX29LV400CB";
> 
> initialization with magic numbers should be provided with some comments for better understanding.

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 */

... won't add much to readability. For anyone interested in this part of 
the code, for the most part field names and assigned values are all the 
information needed. How about a global comment before the whole lot of 
assignments?

>> +++ b/board/LaCie/edminiv2/edminiv2.h

>> +#define EDMINIV2_MPP0_7              0x00000003
>> +#define EDMINIV2_MPP8_15     0x55550000
>> +#define EDMINIV2_MPP16_23    0x00000000

>> +++ b/board/LaCie/edminiv2/lowlevel_init.S

>> +#define EDMINIV2_MPP0_7                      0x00000003
>> +#define EDMINIV2_MPP8_15             0x55550000
>> +#define EDMINIV2_MPP16_19            0x00005555
> 
> Code repeated, this is already defined in edminiv2.h (reported earlier for V3)

Dupe removed.

> Please defined all GPIO stuff at one place

Ditto, as above.

>> +
>> +/*
>> + * Low-level init happens right after start.S has switched to SVC32,
>> + * flushed and disabled caches and disabled MMU. We're still running
>> + * from the boot chip select, so the first thing we should do is set
>> + * up RAM for us to relocate into.
>> + */
>> +
>> +.globl lowlevel_init
>> +
>> +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.

> MPP and GPIO inits should be moved to edminv2.c.

Why move them? They must be board-specific, and they are indeed in 
board-specific code.

> 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?

> Regards..
> Prafulla . .

Thanks Prafulla for your comments.

Amicalement,
-- 
Albert.