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.
> Signed-off-by: Vivek Dalal <v.da...@samsung.com>
> ---
Consider using git-format-patch to create the patches.
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.
...
> +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.
> 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.
> 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.
> +/*****************************************************************************
> + * 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.
> 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.
> +/* 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.
> +/*******************************************************
> + * 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.
> +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.
...
> +/***********************************************************************
> + * 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? ;-)
...
> +/*********************************************************************
> + * 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.
> +/********************************************************
> + * 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.
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
