Re: [U-Boot] [PATCH 01/10] ARM Add New Board GEC2410

31 Oct 2009

Dear "Hui.Tang",
In message e934d0d28a38c4eec2b08862b258bf4b1e5e3403.1256898456.git.zetalabs@gmail.com you wrote:
...
New Board GEC2410 Setup.
Please see below for checkpatch.pl output for your patch series
(note that you should have resolved all tehse issues _before_
submitting the patches).
...
Signed-off-by: Hui.Tang zetalabs@gmail.com
board/gec/gec2410/Makefile        |   54 +++++
 board/gec/gec2410/README          |   85 ++++++++
 board/gec/gec2410/config.mk       |   32 +++
 board/gec/gec2410/flash.c         |  417 +++++++++++++++++++++++++++++++++++++
 board/gec/gec2410/gec2410.c       |  150 +++++++++++++
 board/gec/gec2410/lowlevel_init.S |  171 +++++++++++++++
 board/gec/gec2410/u-boot-nand.lds |   61 ++++++
 7 files changed, 970 insertions(+), 0 deletions(-)
 create mode 100644 board/gec/gec2410/Makefile
 create mode 100644 board/gec/gec2410/README
 create mode 100644 board/gec/gec2410/config.mk
 create mode 100644 board/gec/gec2410/flash.c
 create mode 100644 board/gec/gec2410/gec2410.c
 create mode 100644 board/gec/gec2410/lowlevel_init.S
 create mode 100644 board/gec/gec2410/u-boot-nand.lds
For a new board support, entries to the top level Makefile and to the
MAINTAINERS and MAKEALL files are missing.
...

diff --git a/board/gec/gec2410/flash.c b/board/gec/gec2410/flash.c
new file mode 100644
index 0000000..ab418b1
--- /dev/null
+++ b/board/gec/gec2410/flash.c
This looks very much like a CFI compatible flash device. Why do you
think you need a custom flash driver?
...
diff --git a/board/gec/gec2410/gec2410.c b/board/gec/gec2410/gec2410.c
new file mode 100644
index 0000000..543ceeb
--- /dev/null
+++ b/board/gec/gec2410/gec2410.c
@@ -0,0 +1,150 @@
+/*


(C) Copyright 2002



Sysgo Real-Time Solutions, GmbH <www.elinos.com>



Marius Groeger mgroeger@sysgo.de







(C) Copyright 2002



David Mueller, ELSOFT AG, d.mueller@elsoft.ch







(C) Copyright 2009



Hui Tang, zetalabs@gmail.com







See file CREDITS for list of people who contributed to this



project.







This program is free software; you can redistribute it and/or



modify it under the terms of the GNU General Public License as



published by the Free Software Foundation; either version 2 of



the License, or (at your option) any later version.







This program is distributed in the hope that it will be useful,



but WITHOUT ANY WARRANTY; without even the implied warranty of



MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the



GNU General Public License for more details.







You should have received a copy of the GNU General Public License



along with this program; if not, write to the Free Software



Foundation, Inc., 59 Temple Place, Suite 330, Boston,



MA 02111-1307 USA


*/


+#include <common.h>
+#include <netdev.h>
+#include <s3c2410.h>



+DECLARE_GLOBAL_DATA_PTR;



+#define FCLK_SPEED 1



+#if FCLK_SPEED == 0		/* Fout = 203MHz, Fin = 12MHz for Audio */
+#define M_MDIV	0xC3
+#define M_PDIV	0x4
+#define M_SDIV	0x1
+#elif FCLK_SPEED == 1		/* Fout = 202.8MHz */
+#define M_MDIV	0xA1
+#define M_PDIV	0x3
+#define M_SDIV	0x1
+#endif



+#define USB_CLOCK 1



+#if USB_CLOCK == 0
+#define U_M_MDIV	0xA1
+#define U_M_PDIV	0x3
+#define U_M_SDIV	0x1
+#elif USB_CLOCK == 1
+#define U_M_MDIV	0x48
+#define U_M_PDIV	0x3
+#define U_M_SDIV	0x2
+#endif



+static inline void delay(unsigned long loops)
+{

__asm__ volatile ("1:\n"
 "subs %0, %1, #1\n"


 "bne 1b" : "=r" (loops) : "0" (loops));



+}



+/*


Miscellaneous platform dependent initialisations


*/


+int board_init(void)
+{

struct s3c24x0_clock_power * const clk_power =
			s3c24x0_get_base_clock_power();


struct s3c24x0_gpio * const gpio = s3c24x0_get_base_gpio();

/* to reduce PLL lock time, adjust the LOCKTIME register */
clk_power->LOCKTIME = 0xFFFFFF;

/* configure MPLL */
clk_power->MPLLCON = ((M_MDIV << 12) + (M_PDIV << 4) + M_SDIV);

/* some delay between MPLL and UPLL */
delay(4000);

/* configure UPLL */
clk_power->UPLLCON = ((U_M_MDIV << 12) + (U_M_PDIV << 4) + U_M_SDIV);

Please use I/O accessors to access device registers, here and
everwhere else in this patch series.
...

/* set up the I/O ports */
gpio->GPACON = 0x007FFFFF;
gpio->GPBCON = 0x00044555;
gpio->GPBUP = 0x000007FF;
gpio->GPCCON = 0xAAAAAAAA;
gpio->GPCUP = 0x0000FFFF;
gpio->GPDCON = 0xAAAAAAAA;
gpio->GPDUP = 0x0000FFFF;
gpio->GPECON = 0xAAAAAAAA;
gpio->GPEUP = 0x0000FFFF;
gpio->GPFCON = 0x000055AA;
gpio->GPFUP = 0x000000FF;
gpio->GPGCON = 0xFF95FFBA;
gpio->GPGUP = 0x0000FFFF;
gpio->GPHCON = 0x002AFAAA;
gpio->GPHUP = 0x000007FF;

Please define some symbolic constants for these data (in your board
config file), and explain in comments what these mean.
...
+int dram_init(void)
+{

gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
gd->bd->bi_dram[0].size = PHYS_SDRAM_1_SIZE;

Please use get_ram_size() for auto-sizing and memory testing.
...
+ulong board_flash_get_legacy(ulong base, int banknum, flash_info_t *info)
+{

if (banknum == 0) {	/* non-CFI boot flash */
info->portwidth = FLASH_CFI_16BIT;


info->chipwidth = FLASH_CFI_BY16;


info->interface = FLASH_CFI_X16;


return 1;


} else
return 0;



+}
Why would that be needed?
...
diff --git a/board/gec/gec2410/lowlevel_init.S b/board/gec/gec2410/lowlevel_init.S
new file mode 100644
...
...
+#include <config.h>
+#include <version.h>




+/* some parameters for the board */
What is this comment supposed to explain?
...
+/*






Taken from linux/arch/arm/boot/compressed/head-s3c2410.S







Copyright (C) 2002 Samsung Electronics SW.LEE  hitchcar@sec.samsung.com






*/


+#define BWSCON	0x48000000



+/* BWSCON */
+#define DW8			(0x0)
+#define DW16			(0x1)
+#define DW32			(0x2)
+#define WAIT			(0x1<<2)
+#define UBLB			(0x1<<3)



+#define B1_BWSCON		(DW32)
+#define B2_BWSCON		(DW16)
+#define B3_BWSCON		(DW16 + WAIT + UBLB)
+#define B4_BWSCON		(DW16)
+#define B5_BWSCON		(DW16)
+#define B6_BWSCON		(DW32)
+#define B7_BWSCON		(DW32)



+/* BANK0CON */
+#define B0_Tacs			0x0	/*  0clk */
+#define B0_Tcos			0x0	/*  0clk */
+#define B0_Tacc			0x7	/* 14clk */
+#define B0_Tcoh			0x0	/*  0clk */
+#define B0_Tah			0x0	/*  0clk */
+#define B0_Tacp			0x0
+#define B0_PMC			0x0	/* normal */



+/* BANK1CON */
+#define B1_Tacs			0x0	/*  0clk */
+#define B1_Tcos			0x0	/*  0clk */
+#define B1_Tacc			0x7	/* 14clk */
+#define B1_Tcoh			0x0	/*  0clk */
+#define B1_Tah			0x0	/*  0clk */
+#define B1_Tacp			0x0
+#define B1_PMC			0x0



+#define B2_Tacs			0x0
+#define B2_Tcos			0x0
+#define B2_Tacc			0x7
+#define B2_Tcoh			0x0
+#define B2_Tah			0x0
+#define B2_Tacp			0x0
+#define B2_PMC			0x0



+#define B3_Tacs			0x0	/*  0clk */
+#define B3_Tcos			0x3	/*  4clk */
+#define B3_Tacc			0x7	/* 14clk */
+#define B3_Tcoh			0x1	/*  1clk */
+#define B3_Tah			0x0	/*  0clk */
+#define B3_Tacp			0x3     /*  6clk */
+#define B3_PMC			0x0	/* normal */



+#define B4_Tacs			0x0	/*  0clk */
+#define B4_Tcos			0x0	/*  0clk */
+#define B4_Tacc			0x7	/* 14clk */
+#define B4_Tcoh			0x0	/*  0clk */
+#define B4_Tah			0x0	/*  0clk */
+#define B4_Tacp			0x0
+#define B4_PMC			0x0	/* normal */



+#define B5_Tacs			0x0	/*  0clk */
+#define B5_Tcos			0x0	/*  0clk */
+#define B5_Tacc			0x7	/* 14clk */
+#define B5_Tcoh			0x0	/*  0clk */
+#define B5_Tah			0x0	/*  0clk */
+#define B5_Tacp			0x0
+#define B5_PMC			0x0	/* normal */



+#define B6_MT			0x3	/* SDRAM */
+#define B6_Trcd			0x1
+#define B6_SCAN			0x1	/* 9bit */



+#define B7_MT			0x3	/* SDRAM */
+#define B7_Trcd			0x1	/* 3clk */
+#define B7_SCAN			0x1	/* 9bit */



+/* REFRESH parameter */
+#define REFEN			0x1	/* Refresh enable */
+#define TREFMD			0x0	/* CBR(CAS before RAS)/Auto refresh */
+#define Trp			0x0	/* 2clk */
+#define Trc			0x3	/* 7clk */
+#define Tchr			0x2	/* 3clk */
+#define REFCNT			1113	/* period=15.6us, HCLK=60Mhz, (2048+1-15.6*60) */
These #defines belong into some header file.
...
+SMRDATA:

.word (0+(B1_BWSCON<<4)+(B2_BWSCON<<8)+(B3_BWSCON<<12)+(B4_BWSCON<<16)+(B5_BWSCON<<20)+(B6_BWSCON<<24)+(B7_BWSCON<<28))
.word ((B0_Tacs<<13)+(B0_Tcos<<11)+(B0_Tacc<<8)+(B0_Tcoh<<6)+(B0_Tah<<4)+(B0_Tacp<<2)+(B0_PMC))
.word ((B1_Tacs<<13)+(B1_Tcos<<11)+(B1_Tacc<<8)+(B1_Tcoh<<6)+(B1_Tah<<4)+(B1_Tacp<<2)+(B1_PMC))
.word ((B2_Tacs<<13)+(B2_Tcos<<11)+(B2_Tacc<<8)+(B2_Tcoh<<6)+(B2_Tah<<4)+(B2_Tacp<<2)+(B2_PMC))
.word ((B3_Tacs<<13)+(B3_Tcos<<11)+(B3_Tacc<<8)+(B3_Tcoh<<6)+(B3_Tah<<4)+(B3_Tacp<<2)+(B3_PMC))
.word ((B4_Tacs<<13)+(B4_Tcos<<11)+(B4_Tacc<<8)+(B4_Tcoh<<6)+(B4_Tah<<4)+(B4_Tacp<<2)+(B4_PMC))
.word ((B5_Tacs<<13)+(B5_Tcos<<11)+(B5_Tacc<<8)+(B5_Tcoh<<6)+(B5_Tah<<4)+(B5_Tacp<<2)+(B5_PMC))

Lines too long. Please fix globally.
PATCH 01/10:
WARNING: line over 80 characters
#336: FILE: board/gec/gec2410/flash.c:43:
+#define MEM_FLASH_ADDR1		(*(volatile u16 *)(CONFIG_SYS_FLASH_BASE + (0x00000555 << 1)))
WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#336: FILE: board/gec/gec2410/flash.c:43:
+#define MEM_FLASH_ADDR1		(*(volatile u16 *)(CONFIG_SYS_FLASH_BASE + (0x00000555 << 1)))
WARNING: line over 80 characters
#337: FILE: board/gec/gec2410/flash.c:44:
+#define MEM_FLASH_ADDR2		(*(volatile u16 *)(CONFIG_SYS_FLASH_BASE + (0x000002AA << 1)))
WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#337: FILE: board/gec/gec2410/flash.c:44:
+#define MEM_FLASH_ADDR2		(*(volatile u16 *)(CONFIG_SYS_FLASH_BASE + (0x000002AA << 1)))
WARNING: line over 80 characters
#1000: FILE: board/gec/gec2410/lowlevel_init.S:128:
+#define REFCNT			1113	/* period=15.6us, HCLK=60Mhz, (2048+1-15.6*60) */
WARNING: line over 80 characters
#1031: FILE: board/gec/gec2410/lowlevel_init.S:159:
+    .word (0+(B1_BWSCON<<4)+(B2_BWSCON<<8)+(B3_BWSCON<<12)+(B4_BWSCON<<16)+(B5_BWSCON<<20)+(B6_BWSCON<<24)+(B7_BWSCON<<28))
WARNING: line over 80 characters
#1032: FILE: board/gec/gec2410/lowlevel_init.S:160:
+    .word ((B0_Tacs<<13)+(B0_Tcos<<11)+(B0_Tacc<<8)+(B0_Tcoh<<6)+(B0_Tah<<4)+(B0_Tacp<<2)+(B0_PMC))
WARNING: line over 80 characters
#1033: FILE: board/gec/gec2410/lowlevel_init.S:161:
+    .word ((B1_Tacs<<13)+(B1_Tcos<<11)+(B1_Tacc<<8)+(B1_Tcoh<<6)+(B1_Tah<<4)+(B1_Tacp<<2)+(B1_PMC))
WARNING: line over 80 characters
#1034: FILE: board/gec/gec2410/lowlevel_init.S:162:
+    .word ((B2_Tacs<<13)+(B2_Tcos<<11)+(B2_Tacc<<8)+(B2_Tcoh<<6)+(B2_Tah<<4)+(B2_Tacp<<2)+(B2_PMC))
WARNING: line over 80 characters
#1035: FILE: board/gec/gec2410/lowlevel_init.S:163:
+    .word ((B3_Tacs<<13)+(B3_Tcos<<11)+(B3_Tacc<<8)+(B3_Tcoh<<6)+(B3_Tah<<4)+(B3_Tacp<<2)+(B3_PMC))
WARNING: line over 80 characters
#1036: FILE: board/gec/gec2410/lowlevel_init.S:164:
+    .word ((B4_Tacs<<13)+(B4_Tcos<<11)+(B4_Tacc<<8)+(B4_Tcoh<<6)+(B4_Tah<<4)+(B4_Tacp<<2)+(B4_PMC))
WARNING: line over 80 characters
#1037: FILE: board/gec/gec2410/lowlevel_init.S:165:
+    .word ((B5_Tacs<<13)+(B5_Tcos<<11)+(B5_Tacc<<8)+(B5_Tcoh<<6)+(B5_Tah<<4)+(B5_Tacp<<2)+(B5_PMC))
total: 0 errors, 12 warnings, 970 lines checked
PATCH 01/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
PATCH 03/10
WARNING: line over 80 characters
#135: FILE: include/configs/gec2410.h:38:
+#define CONFIG_GEC2410		1	/* on a GD-Embedded Software Center GEC2410 Board  */
WARNING: line over 80 characters
#161: FILE: include/configs/gec2410.h:64:
+#define CONFIG_SYS_GBL_DATA_SIZE	128	/* size in bytes reserved for initial data */
ERROR: spaces required around that ':' (ctx:VxV)
#214: FILE: include/configs/gec2410.h:117:
+#define CONFIG_ETHADDR		08:00:3e:26:0a:5b
                       		  ^
ERROR: spaces required around that ':' (ctx:VxV)
#214: FILE: include/configs/gec2410.h:117:
+#define CONFIG_ETHADDR		08:00:3e:26:0a:5b
                       		     ^
ERROR: spaces required around that ':' (ctx:VxV)
#214: FILE: include/configs/gec2410.h:117:
+#define CONFIG_ETHADDR		08:00:3e:26:0a:5b
                       		        ^
ERROR: spaces required around that ':' (ctx:VxV)
#214: FILE: include/configs/gec2410.h:117:
+#define CONFIG_ETHADDR		08:00:3e:26:0a:5b
                       		           ^
ERROR: spaces required around that ':' (ctx:VxV)
#214: FILE: include/configs/gec2410.h:117:
+#define CONFIG_ETHADDR		08:00:3e:26:0a:5b
                       		              ^
WARNING: line over 80 characters
#222: FILE: include/configs/gec2410.h:125:
+#define CONFIG_KGDB_BAUDRATE	115200		/* speed to run kgdb serial port */
WARNING: line over 80 characters
#230: FILE: include/configs/gec2410.h:133:
+#define CONFIG_SYS_LONGHELP				/* undef to save memory       */
WARNING: line over 80 characters
#231: FILE: include/configs/gec2410.h:134:
+#define CONFIG_SYS_PROMPT		"GEC2410#"	/* Monitor Command Prompt     */
WARNING: line over 80 characters
#232: FILE: include/configs/gec2410.h:135:
+#define CONFIG_SYS_CBSIZE		256		/* Console I/O Buffer Size    */
WARNING: line over 80 characters
#233: FILE: include/configs/gec2410.h:136:
+#define CONFIG_SYS_PBSIZE		384		/* Print Buffer Size          */
WARNING: line over 80 characters
#234: FILE: include/configs/gec2410.h:137:
+#define CONFIG_SYS_MAXARGS		16		/* max number of command args */
WARNING: line over 80 characters
#235: FILE: include/configs/gec2410.h:138:
+#define CONFIG_SYS_BARGSIZE		CONFIG_SYS_CBSIZE	/* Boot Argument Buffer Size  */
WARNING: line over 80 characters
#237: FILE: include/configs/gec2410.h:140:
+#define CONFIG_SYS_MEMTEST_START	CONFIG_SYS_SDRAM_BASE	/* memtest works on	      */
WARNING: line over 80 characters
#238: FILE: include/configs/gec2410.h:141:
+#define CONFIG_SYS_MEMTEST_END		(CONFIG_SYS_SDRAM_BASE + 0x3e00000) /* 62 MB in DRAM	*/
WARNING: line over 80 characters
#240: FILE: include/configs/gec2410.h:143:
+#define CONFIG_SYS_LOAD_ADDR		CONFIG_SYS_SDRAM_BASE	/* default load address	*/
WARNING: line over 80 characters
#264: FILE: include/configs/gec2410.h:167:
+#define PHYS_SDRAM_1		CONFIG_SYS_SDRAM_BASE	/* SDRAM Bank #1	*/
WARNING: line over 80 characters
#275: FILE: include/configs/gec2410.h:178:
+#define CONFIG_SYS_MAX_FLASH_BANKS	1	/* max number of memory banks	*/
WARNING: line over 80 characters
#276: FILE: include/configs/gec2410.h:179:
+#define CONFIG_SYS_MAX_FLASH_SECT	512	/* max number of sectors on one chip */
WARNING: line over 80 characters
#278: FILE: include/configs/gec2410.h:181:
+#define CONFIG_SYS_FLASH_CFI		1	/* Use CFI parameters (needed?) */
WARNING: line over 80 characters
#285: FILE: include/configs/gec2410.h:188:
+#define CONFIG_ENV_ADDR		(CONFIG_SYS_FLASH_BASE + 0x0F0000) /* addr of environment */
WARNING: line over 80 characters
#288: FILE: include/configs/gec2410.h:191:
+#define CONFIG_SYS_FLASH_ERASE_TOUT	(5 * CONFIG_SYS_HZ) /* Timeout for Flash Erase	*/
WARNING: line over 80 characters
#289: FILE: include/configs/gec2410.h:192:
+#define CONFIG_SYS_FLASH_WRITE_TOUT	(5 * CONFIG_SYS_HZ) /* Timeout for Flash Write	*/
WARNING: line over 80 characters
#318: FILE: include/configs/gec2410.h:221:
+#define CONFIG_SYS_UBOOT_BASE		(CONFIG_SYS_MAPPED_RAM_BASE + 0x03e00000)
WARNING: line over 80 characters
#320: FILE: include/configs/gec2410.h:223:
+#define CONFIG_ENV_OFFSET		0x0040000 /* Offset of Environment Sector */
WARNING: line over 80 characters
#324: FILE: include/configs/gec2410.h:227:
+#define CONFIG_SYS_NAND_BASE		0x4e00000c	/* NFDATA 0x4e00000c R/W NAND Flash data register	*/
WARNING: line over 80 characters
#329: FILE: include/configs/gec2410.h:232:
+#define CONFIG_SYS_NAND_U_BOOT_DST	CONFIG_SYS_PHY_UBOOT_BASE	/* NUB load-addr      */
WARNING: line over 80 characters
#330: FILE: include/configs/gec2410.h:233:
+#define CONFIG_SYS_NAND_U_BOOT_START	CONFIG_SYS_NAND_U_BOOT_DST	/* NUB start-addr     */
WARNING: line over 80 characters
#332: FILE: include/configs/gec2410.h:235:
+#define CONFIG_SYS_NAND_U_BOOT_OFFS	(4 * 1024)	/* Offset to RAM U-Boot image */
WARNING: line over 80 characters
#333: FILE: include/configs/gec2410.h:236:
+#define CONFIG_SYS_NAND_U_BOOT_SIZE	(252 * 1024)	/* Size of RAM U-Boot image   */
WARNING: line over 80 characters
#344: FILE: include/configs/gec2410.h:247:
+#define CONFIG_SYS_NAND_4_ADDR_CYCLE	1		/* Fourth addr used (>32MB)	*/
WARNING: line over 80 characters
#352: FILE: include/configs/gec2410.h:255:
+#define CONFIG_SYS_NAND_ECCSTEPS	(CONFIG_SYS_NAND_PAGE_SIZE / CONFIG_SYS_NAND_ECCSIZE)
WARNING: line over 80 characters
#356: FILE: include/configs/gec2410.h:259:
+#define CONFIG_SYS_NAND_ECCTOTAL	(CONFIG_SYS_NAND_ECCBYTES * CONFIG_SYS_NAND_ECCSTEPS)
total: 5 errors, 29 warnings, 275 lines checked
PATCH 03/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
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
Time is fluid ... like a river with currents, eddies, backwash.
    -- Spock, "The City on the Edge of Forever", stardate 3134.0

    