[U-Boot] [RESEND][PATCH 3/6]POSEIDON Board Support

[Wolfgang Denk]

Dear Vivek,

In message <B887D7D8B9D94A0F80640358260FA1FB at 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.dalal at 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: wd at denx.de
"There is nothing new under the sun, but there are lots of old things
we don't know yet."                                  - Ambrose Bierce