From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean-Christophe PLAGNIOL-VILLARD Date: Sat, 18 Apr 2009 09:23:41 +0200 Subject: [U-Boot] [PATCH v2] Marvell Kirkwood family SOC support In-Reply-To: <73173D32E9439E4ABB5151606C3E19E201C4497983@SC-VEXCH1.marvell.com> References: <20090417092229.GC29071@game.jcrosoft.org> <73173D32E9439E4ABB5151606C3E19E201C4497983@SC-VEXCH1.marvell.com> Message-ID: <20090418072341.GA1413@game.jcrosoft.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 23:44 Fri 17 Apr , Prafulla Wadaskar wrote: > Hi Jean > > Thanks for your review comments. > > > -----Original Message----- > > From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj at jcrosoft.com] > > Sent: Friday, April 17, 2009 2:52 PM > > To: Prafulla Wadaskar > > Cc: u-boot at lists.denx.de; Ashish Karkare; Ronen Shitrit > > Subject: Re: [U-Boot] [PATCH v2] Marvell Kirkwood family SOC support > > > > On 17:33 Wed 08 Apr , Prafulla Wadaskar wrote: > > > Kirkwood family controllers are highly integrated SOCs based on > > > Feroceon-88FR131/Sheeva-88SV131 cpu core. > > > > > > SOC versions supported:- > > > 1) 88F6281-Z0 define CONFIG_KW88F6281_Z0 > > > 2) 88F6281-A0 define CONFIG_KW88F6281_A0 > > > 3) 88F6192-A0 define CONFIG_KW88F6192_A0 > > > > > > Other supported features:- > > > 1) get_random_hex() fucntion > > > 2) SPI port controller driver > > > 3) PCI Express port initialization > > > +/* > > > + * kw_sdram_bar - reads SDRAM Base Address Register */ > > > +u32 kw_sdram_bar(MEMORY_BANK bank) > > > +{ > > > + u32 result = 0; > > > + u32 enable = (0x01 & KW_REG_READ((0x1504 + bank * 8))); > > please create macro for these registers > This will be taken care.... > > please use readx/writex (madatory) > You mean instead of KW_REG_READ to be used readx() ? readl I guess take a look in include/arm-asm/io.h > > > > + > > > +void reset_cpu(unsigned long ignored) { > > > + KW_REG_BITS_SET(KW_REG_CPU_RSTOUTN_MASK, BIT2); > > > + KW_REG_BITS_SET(KW_REG_CPU_SYS_SOFT_RST, BIT0); > > plase use readx/writex > > everywhere > Okay... > > > > + > > > +#if defined (CONFIG_KW88F6281_Z0) > > > + KW_REG_BITS_SET(0x1478, BIT7); > > > +#elif defined (CONFIG_KW88F6281_A0) || defined > > (CONFIG_KW88F6192_A0) > > could you detect it > I should be able to detect it, I will check and update the same > > > > + /* > > > + * in case of 88F6281/88F6192 A0, > > > + * BIT7 need to reset to generate random values in 0x1470 > > > + */ > > > + KW_REG_BITS_RESET(0x1478, BIT7); > > please use macro everywhere instead of hardcode value > Okay.. > > > > + > > > +/* > > > + * kw_window_ctrl_reg_init - Mbus-L to Mbus Bridge Registers init. > > > + */ > > > +int kw_window_ctrl_reg_init(void) > > > +{ > > coudl explain a few more what you do and try use macro > > instead of hardcode value > Macros will be used.. > I will put the explaination in the comment for this function > > > > + KW_REG_WRITE(KW_REG_WIN_CTRL(0), 0x0fffe841); > > > + KW_REG_WRITE(KW_REG_WIN_BASE(0), 0x90000000); > > > + KW_REG_WRITE(KW_REG_WIN_REMAP_LOW(0), 0x90000000); > > > + KW_REG_WRITE(KW_REG_WIN_REMAP_HIGH(0), 0x00000000); > > > + > > > > +#ifndef __ASSEMBLY__ > > > +void reset_cpu(unsigned long ignored); unsigned char > > > +get_random_hex(void); typedef enum _memory_bank { BANK0, BANK1, > > > +BANK2, BANK3 } MEMORY_BANK; > > please do not use upper case > You mean MEMORY_BANK, right? I will change it yes > > > > > + (TICK_SMPL_1x3 << CCR_CPU_2_MBUSL_TICK_SMPL_OFFS)) > > > +#define CPU_2_MBUSL_DDR_CLK_1x4 > > \ > > > + ((TICK_DRV_1x4 << > > CCR_CPU_2_MBUSL_TICK_DRV_OFFS) | \ > > > + (TICK_SMPL_1x4 << CCR_CPU_2_MBUSL_TICK_SMPL_OFFS)) > > > + > > please move all this define to header corresponding of the > > functionallity/IP > I thought these are only settings limited to this file only, any way I will create and move them to header file > > > > + > > > + .globl kw_cpu_if_pre_init > > > +kw_cpu_if_pre_init: > > do you really need to do this before relocation > Yes.. could add comment to explain why > > > > + > > > + mov r11, LR /* Save link register */ > > will you call a sub routine? > I will do it? > > > > + .globl kw_enable_invalidate_l2_cache > > > +kw_enable_invalidate_l2_cache: > > > + mov r11, LR /* Save link register */ > > > + > > > + /* Enable L2 cache in write through mode */ > > > + KW_REG_READ_ASM(r4, r1, KW_REG_CPU_L2_CONFIG) > > please remove thhis KW_RED_READ_ASM > > or do it via asm macro as we have done for sh2/3/4 > I will check this... attached a first version of macro.h file for arm > > > > > + > > > + /* invalidate L2 cache */ > > > + mov r0, #0 > > > + mcr p15, 1, r0, c15, c11, 0 > > please create a macro for this > Okay... > > > > + > > > + mov PC, r11 /* r11 is saved link register */ > > > + > > > + .globl lowlevel_init > > > > in this case call it arch_lowlevel_init > > > > and update start.S to call it just be the lowlevel_init > You mean to add support for arch_lowlevel_init, using some CONFIG_ option ? yes CONFIG_ARCH_LOWLEVEL_INIT Best Regards, J. -------------- next part -------------- A non-text attachment was scrubbed... Name: macro.h Type: text/x-chdr Size: 1374 bytes Desc: not available Url : http://lists.denx.de/pipermail/u-boot/attachments/20090418/9678bb62/attachment-0001.h