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
