Hi Jean
Thanks for your review comments.
-----Original Message----- From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj@jcrosoft.com] Sent: Friday, April 17, 2009 2:52 PM To: Prafulla Wadaskar Cc: u-boot@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:-
- 88F6281-Z0 define CONFIG_KW88F6281_Z0
- 88F6281-A0 define CONFIG_KW88F6281_A0
- 88F6192-A0 define CONFIG_KW88F6192_A0
Other supported features:-
- get_random_hex() fucntion
- SPI port controller driver
- 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_initin 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.