Hi Wolfgang ----- Original Message ----- From: "Wolfgang Denk" <w...@denx.de> To: "Vivek" <v.da...@samsung.com> Cc: "Scott Wood" <scottw...@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 <b887d7d8b9d94a0f80640358260fa...@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.da...@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: w...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot