[U-Boot] [PATCH v2] Marvell Kirkwood family SOC support

[Prafulla Wadaskar]

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

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


> > +		  (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..

> > +
> > +        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...


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

> 
> which is board lowlevel init
> 
> > +lowlevel_init:
> 	if you have multple sub call use the stack will be beter
That's good idea, I will do it


> > +/*
> > + * ARM Timers Registers Map
> > + */
> > +#define CNTMR_CTRL_REG			KW_REG_TMR_CTRL
> > +#define CNTMR_RELOAD_REG(tmrNum)	(KW_REG_TMR_RELOAD + tmrNum*8)
> > +#define CNTMR_VAL_REG(tmrNum)		(KW_REG_TMR_VAL 
> + tmrNum*8)
> please no uppercase in the var name
Okay..


> > +/*
> > + * ARM Timer\Watchdog Register
> > + * CNTMR_VAL_REG (TVRG)
> > + */
> > +#define TVR_ARM_TIMER_OFFS			0
> > +#define TVR_ARM_TIMER_MASK			0xffffffff
> > +#define TVR_ARM_TIMER_MAX			0xffffffff
> please move all this define to a header
Okay...

Thanks and regards....
Prafulla . .

> 
> and nearly all precedent comment can be apply to the rest of the patch
> 
> Best Regards,
> J.
>