Hi Wolfgang
----- Original Message ----- From: "Wolfgang Denk" wd@denx.de To: "Vivek" v.dalal@samsung.com Cc: "Scott Wood" scottwood@freescale.com; u-boot@lists.denx.de Sent: Friday, July 31, 2009 12:34 AM Subject: Re: [U-Boot] [RESEND][PATCH 3/6]POSEIDON Board Support
Dear Vivek,
In message B887D7D8B9D94A0F80640358260FA1FB@sisodomain.com you wrote:
Removed code referring Legacy NAND and did some code cleanup.
Such version comments should go below the --- line, not above.
OK...
Signed-off-by: Vivek Dalal v.dalal@samsung.com
Consider using git-format-patch to create the patches.
OK, I will use it.
MAINAINERS entry is missing.
MAKEALL entry is missing.
Top level Makefile entry is missing.
diff --git a/board/poseidon/Makefile b/board/poseidon/Makefile index e69de29..edbc696 100644 --- a/board/poseidon/Makefile +++ b/board/poseidon/Makefile @@ -0,0 +1,48 @@ +# +# (C) Copyright 2009-2010
Hey. PLease tell me where you got that time machine from. You're already living in 2010 ?
Please fix globally.
OK..Will modify
...
+include $(TOPDIR)/config.mk
+LIB = lib$(BOARD).a
+OBJS := poseidon.o mem.o sys_info.o +SOBJS := lowlevel_init.o load.o
+$(LIB): $(OBJS) $(SOBJS)
- $(AR) crv $@ $^
+clean:
- rm -f $(SOBJS) $(OBJS)
+distclean: clean
- rm -f $(LIB) core *.bak .depend
+#########################################################################
+.depend: Makefile $(SOBJS:.o=.S) $(OBJS:.o=.c)
- $(CC) -M $(CPPFLAGS) $(SOBJS:.o=.S) $(OBJS:.o=.c) > $@
+-include .depend
This Makefile does not support out-of-tree building. Please fix.
OK
diff --git a/board/poseidon/config.mk b/board/poseidon/config.mk index e69de29..f05593c 100644 --- a/board/poseidon/config.mk +++ b/board/poseidon/config.mk
...
+# For use with external or internal boots. +TEXT_BASE = 0x83e80000
+# Handy to get symbols to debug ROM version. +#TEXT_BASE = 0x0 +#TEXT_BASE = 0x08000000 +#TEXT_BASE = 0x04000000
Please do not add dead code. Remove this.
OK, Will do that.
diff --git a/board/poseidon/lowlevel_init.S b/board/poseidon/lowlevel_init.S index e69de29..9052f71 100644 --- a/board/poseidon/lowlevel_init.S +++ b/board/poseidon/lowlevel_init.S
...
- *************************************************************************/
+.global cpy_clk_code
- cpy_clk_code:
/* Copy DPLL code into SRAM */adr r0, go_to_speed /* get addr of clock setting code */mov r2, #384 /* r2 size to copy (div by 32 bytes) */mov r1, r1 /* r1 <- dest address (passed in) */add r2, r2, r0 /* r2 <- source end address */+next2:
ldmia r0!, {r3-r10} /* copy from source address [r0] */stmia r1!, {r3-r10} /* copy to target address [r1] */cmp r0, r2 /* until source end address [r2] */bne next2- mov pc, lr /* back to caller */
Here and everywhere else: indentation by TAB only, please.
Already taken care indentation but missed at some places. For checking codng gudilines I used checkpatch.pl script of linux. Will look with more care.
+/*****************************************************************************
- go_to_speed: -Moves to bypass, -Commits clock dividers, -puts dpll at speed
-executed from SRAM.
- R0 = PRCM_CLKCFG_CTRL - addr of valid reg
- R1 = CM_CLKEN_PLL - addr dpll ctlr reg
- R2 = dpll value
- R3 = CM_IDLEST_CKGEN - addr dpll lock wait
- ******************************************************************************/
Incorrect multiline comment style.
- /* now prepare GPMC (flash) for new dpll speed */
- /* flash needs to be stable when we jump back to it */
Incorrect multiline comment style.
- /* setup to 2x loop though code. The first loop pre-loads the
- icache, the 2nd commits the prcm config, and locks the dpll
- */
Incorrect multiline comment style.
Please fix globally.
Will do for all cases. ACK
diff --git a/board/poseidon/mem.c b/board/poseidon/mem.c index e69de29..8f7dc65 100644 --- a/board/poseidon/mem.c +++ b/board/poseidon/mem.c
...
+/* Board CS Organization - Poseidon */ +static const unsigned char chip_sel_sdp[][GPMC_MAX_CS] = {
- /* GPMC CS Indices */
- /* S8- 1 2 3 IDX CS0, CS1, CS2 .. CS7 */
- /* 0 OFF OFF OFF */ {0, 0, 0, 0, 0, 0, 0, 0},
- /* 0 OFF OFF OFF */ {0, 0, 0, 0, 0, 0, 0, 0},
- /* 0 OFF OFF OFF */ {0, 0, 0, 0, 0, 0, 0, 0},
- /* 0 OFF OFF OFF */ {0, 0, 0, 0, 0, 0, 0, 0},
- /* 0 OFF OFF OFF */ {0, 0, 0, 0, 0, 0, 0, 0},
- /* 0 OFF OFF OFF */ {0, 0, 0, 0, 0, 0, 0, 0},
- /* 0 OFF OFF OFF */ {0, 0, 0, 0, 0, 0, 0, 0},
- /* 7 ON ON ON */
- {PROC_ONENAND, PROC_NAND, PISMO_CS0, 0, 0, DBG_MPDB, 0, PISMO_CS1},
+};
Only one blank line here.
OK..and will take care in other cases too.
+/* Values for each of the chips */ +static u32 gpmc_mpdb[GPMC_MAX_REG] = {
- MPDB_GPMC_CONFIG1,
- MPDB_GPMC_CONFIG2,
- MPDB_GPMC_CONFIG3,
- MPDB_GPMC_CONFIG4,
- MPDB_GPMC_CONFIG5,
- MPDB_GPMC_CONFIG6, 0
+}; +static u32 gpmc_onenand[GPMC_MAX_REG] = {
- ONENAND_GPMC_CONFIG1,
- ONENAND_GPMC_CONFIG2,
- ONENAND_GPMC_CONFIG3,
- ONENAND_GPMC_CONFIG4,
- ONENAND_GPMC_CONFIG5,
- ONENAND_GPMC_CONFIG6, 0
+};
Only one blank line here. Check globally, please.
...
- __raw_writel((((size & 0xF) << 8) | ((base >> 24) & 0x3F) |
- (1 << 6)), GPMC_CONFIG7 + gpmc_base);
- sdelay(2000);
Delete this blank line, please. And do so globally.
...
+int board_init(void) +{
- DECLARE_GLOBAL_DATA_PTR;
- gpmc_init(); /* in SRAM or SDRM, finish GPMC */
- (*(volatile unsigned int*)0x49002030) |= (0x18<<16);
- *(volatile unsigned int*)0x49006200 |= (1<<17);
- *(volatile unsigned int*)0x49006210 |= (1<<17);
- *(volatile unsigned int*)0x490062A0 |= (1<<17);
- __raw_writeb(0x1b, 0x4900211a);
Please do not access device regiosters through pointers. Please use proper I/O accessor functions. Declare C structs where needed so you can use these without casts.
This comment applies to all your code.
OK, Will do that.
+/*******************************************************
- Routine: misc_init_r
- Description: Init ethernet(done here so udelay works)
- ********************************************************/
What has Ethernet to do with trhe functioning of udelay() ??
+#ifdef CONFIG_2430
This is not a legal name for a CONFIG_ variable.
OK. Will check..
+u32 get_cpu_type(void) +{
- u32 v;
- v = __raw_readl(TAP_IDCODE_REG);
- v &= CPU_24XX_ID_MASK;
- if (v == CPU_2430_CHIPID) {
- return CPU_2430;
- } else
No braces for single line statements, please.
...
OK...
+/***********************************************************************
- get_cs0_size() - get size of chip select 0/1
- ************************************************************************/
A chip select probably cannot have a size, or can it?
What is it - the diameter of the ball? ;-)
...
Will Check ....
+/*********************************************************************
- display_board_info() - print banner with board info.
- *********************************************************************/
Please be terse with your output.
+void display_board_info(u32 btype) +{
- char *bootmode[] = {
- "ONND",
- "SIB1",
- "SIB0",
- "NAND",
- "SIB1",
- "SIB0",
- "NOR",
- "OneNAND",
- };
- u32 brev = get_board_rev();
- char cpu_2430s[] = "2430C";
- char db_ver[] = "0.0"; /* board type */
- char mem_sdr[] = "mSDR"; /* memory type */
- char mem_ddr[] = "mDDR";
- char t_tst[] = "TST"; /* security level */
- char t_emu[] = "EMU";
- char t_hs[] = "HS";
- char t_gp[] = "GP";
- char unk[] = "?";
- char t_poseidon[] = "POSEIDON";
+#ifdef CONFIG_LED_INFO
- char led_string[CONFIG_LED_LEN] = { 0 };
+#endif
+#if defined(PRCM_CONFIG_I)
- char prcm[] = "I";
+#elif defined(PRCM_CONFIG_II)
- char prcm[] = "II";
+#endif
- char *cpu_s, *db_s, *mem_s, *sec_s, *sdp;
- u32 cpu, rev, sec;
Indentation wrong. Please check globally.
OK, will check it.
+/********************************************************
- get_base(); get upper addr of current execution
- *******************************************************/
+u32 get_base(void) +{
- u32 val;
- __asm__ __volatile__("mov %0, pc \n" : "=r"(val) : : "memory");
- val &= 0xF0000000;
- val >>= 28;
- return val;
+}
Hm... Instead of using magic constants here please use #defines from your board config file so this code remains correct even if someone changes the memory map.
+/********************************************************
- running_in_flash() - tell if currently running in
- flash.
- *******************************************************/
+u32 running_in_flash(void) +{
- if (get_base() < 4)
- return 1; /* in flash */
- return 0; /* running in SRAM or SDRAM */
ditto.
+/********************************************************
- running_in_sram() - tell if currently running in
- sram.
- *******************************************************/
+u32 running_in_sram(void) +{
- if (get_base() == 4)
- return 1; /* in SRAM */
- return 0; /* running in FLASH or SDRAM */
ditto.
+/********************************************************
- running_in_sdram() - tell if currently running in
- flash.
- *******************************************************/
+u32 running_in_sdram(void) +{
- if (get_base() > 4)
- return 1; /* in sdram */
- return 0; /* running in SRAM or FLASH */
ditto.
Will check ..
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "There is nothing new under the sun, but there are lots of old things we don't know yet." - Ambrose Bierce
Sorry for late reply. I was on vacation. Will be releasing v2 of all the 6 patches soon with all the comments incorporated.
Best regards Vivek Dalal