[U-Boot] [PATCH V4 3/3] Add support for the LaCie ED Mini V2 board
Prafulla Wadaskar
prafulla at marvell.com
Wed Jan 13 15:04:29 CET 2010
> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.aribaud at free.fr]
> Sent: Wednesday, January 13, 2010 6:08 PM
> To: Prafulla Wadaskar
> Cc: U-Boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH V4 3/3] Add support for the
> LaCie ED Mini V2 board
...snip...
> >> + 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 */
This level documentation is good rather than nothing :-)
...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.
>
> > 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.
edminv2.c is also board specific :-), lets keep least ASM code.
We cannot avoid lowlevel_init otherwise I could have preferred to omit it.
> > 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?
Regards.
Prafulla . .
More information about the U-Boot
mailing list