[U-Boot] [PATCH 5/9] apf9328: add default board configuration file

[Stefano Babic <sbabic@denx.de>]

On 08/10/2011 10:33 PM, Eric Jarrige wrote:
> Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>
> ---
>  include/configs/apf9328.h | 1034 +++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 1034 insertions(+), 0 deletions(-)
>  create mode 100644 include/configs/apf9328.h

Hi Eric,

> +
> +#ifndef __CONFIG_H
> +#define __CONFIG_H
> +
> +#define CONFIG_VERSION_VARIABLE
> +#define CONFIG_ENV_VERSION 	"4.0"
> +#define CONFIG_IDENT_STRING	" apf9328 patch 4.0"

You add CONFIG_ without documenting them. If they are specific for your
board, you should use another name, or please add documentation to make
them available for other boards, too.

> +
> +#define CONFIG_ARM920T		1	/* this is an ARM920T CPU */

It is enough to #define a switch, without setting the value. This should
be only:

#define CONFIG_ARM920T	/* this is an ARM920T CPU */

This must be fixed globally.

> +#define CONFIG_IMX		1	/* in a Motorola MC9328MXL Chip */

Ditto, and so on..

> +/*
> + * Definition of u-boot build in commands. Check out CONFIG_CMD_DFL if
> + * neccessary in include/cmd_confdefs.h file. (Un)comment for getting
> + * functionality or size of u-boot code.
> + */
> +
> +#include <config_cmd_default.h>
> +
> +#define CONFIG_CMD_ASKENV	/* ask for env variable		*/
> +#define CONFIG_CMD_BSP		/* Board Specific functions	*/
> +#define CONFIG_CMD_CACHE	/* icache, dcache		*/
> +#define CONFIG_CMD_DATE		/* support for RTC, date/time.. */
> +#define CONFIG_CMD_DHCP		/* DHCP Support			*/
> +#define CONFIG_CMD_DIAG		/* Diagnostics			*/
> +#define CONFIG_CMD_EEPROM	/* EEPROM read/write support	*/
> +#define CONFIG_CMD_FLASH	/* flinfo, erase, protect	*/
> +#define CONFIG_CMD_I2C		/* I2C serial bus support	*/
> +#define CONFIG_CMD_IMLS		/* List all found images	*/

IMLS is already part of default commands

> +#define CONFIG_CMD_IMMAP	/* IMMR dump support		*/

Is this not specific for PowerQuick processor ?

> +#define CONFIG_EXTRA_ENV_SETTINGS \
> +	"env_version="		CONFIG_ENV_VERSION		"\0"	\
> +	"fileaddr="		MK_STR(CONFIG_SYS_LOAD_ADDR)	"\0"	\
> +	"filesize="		MK_STR(CONFIG_SYS_MONITOR_LEN)	"\0"	\

filesize is dynamically computed, you should not add it

> +	"ntpserverip=217.147.208.1\0"					\

No fixed IP address, please, even if they are related to known NTP servers.

> +#define CONFIG_MACH_TYPE MACH_TYPE_APF9328

Why do you need it ? Is it not enough to use directly MACH_TYPE_APF9328 ?

> +#define CONFIG_ETHADDR

Not required, right ?

> +#define CONFIG_NETMASK		255.255.255.0
> +#define CONFIG_IPADDR		192.168.000.10

Please drop fix ip address. They should not be part of mainline,

> +#define CONFIG_BOARD_NAME	"apf9328"
> +#define CONFIG_HOSTNAME		"apf9328"
> +#define CONFIG_GATEWAYIP	192.168.000.1
> +#define CONFIG_SERVERIP		192.168.000.2

Ditto

> +/*
> + * Malloc pool need to host env + 128 Kb reserve for other allocations.
> + */
> +#define CONFIG_SYS_MALLOC_LEN		(CONFIG_ENV_SIZE + (128<<10))

I Think checkpatch will complain about missing spaces before and after <<

> +#define CONFIG_STACKSIZE	(120<<10)	/* stack size	*/
> +
> +#ifdef CONFIG_USE_IRQ
> +#define CONFIG_STACKSIZE_IRQ	(4<<10)	/* IRQ stack	*/
> +#define CONFIG_STACKSIZE_FIQ	(4<<10)	/* FIQ stack	*/
> +#endif

Do you need IRQ ? It is undefined, you can drop this part


> +
> +/*
> +* Clocks configuration
> +*/
> +/*
> + * PLL configuration
> + *
> + * f_{dpll}=2*f{ref}*(MFI+MFN/(MFD+1))/(PD+1)
> + * f_ref=16,777216MHz
> + * 32768 Hz xtal
> + * 0x07B32DA5: 192.0000173
> + * 0x002a141f: 191,9944MHz
> + * 0x040b2007: 144MHz
> + * 0x0FB32DA5: 96.00000864 MHz
> + * 0x042a141f: 96MHz
> + * 0x0811140d: 64MHz
> + * 0x040e200e: 150MHz
> + * 0x00321431: 200MHz
> + *
> + *16 MHz xtal
> + * 0x08001800: 64MHz mit 16er Quarz
> + * 0x04001800: 96MHz mit 16er Quarz
> + * 0x04002400: 144MHz mit 16er Quarz
> + *
> + * 31	|x x x x|x x x x|x x x x|x x x x|x x x x|x x x x|x x x x|x x x x| 0
> + *	|XXX|--PD---|-------MFD---------|XXX|--MFI--|-----MFN-----------|
> + */
> +
> +#define CONFIG_SYS_OSC32	32768	/* 32768 or 32000 Hz crystal */
> +#undef	CONFIG_SYS_OSC16	/* there is no external 16MHz external clock */
> +
> +/* MPU CLOCK source before PLL	(should be named CONFIG_SYS_SYS_CLK_FREQ) */
> +#define CONFIG_SYS_CLK_FREQ	(512*CONFIG_SYS_OSC32)
> +#define CONFIG_SYS_MPCTL0_VAL		0x07B32DA5	/* 192.000017 MHz */
> +#define CONFIG_SYS_MPCTL1_VAL		0
> +
> +/* system clock source before PLL (should be named CONFIG_SYS_SYSPLL_CLK_FREQ)*/
> +#ifndef CONFIG_SYS_OSC16
> +#define CONFIG_SYSPLL_CLK_FREQ		(512*CONFIG_SYS_OSC32)
> +#if (CONFIG_SYS_OSC32 == 32000)
> +#define CONFIG_SYS_SPCTL0_VAL		0x043F1437	/* 96 MHz */
> +#define CONFIG_SYS_SPCTL1_VAL		0
> +#define CONFIG_SYS_FREQ			96	/* MHz */
> +#else /* CONFIG_SYS_OSC32 == 32768 */
> +#define CONFIG_SYS_SPCTL0_VAL		0x0FB32DA5	/* 96.000009 MHz */
> +#define CONFIG_SYS_SPCTL1_VAL		0
> +#define CONFIG_SYS_FREQ			96	/* MHz */
> +#endif /* CONFIG_SYS_OSC32 */
> +#else /* CONFIG_SYS_OSC16 in use */
> +#define CONFIG_SYSPLL_CLK_FREQ		CONFIG_SYS_OSC16
> +#define CONFIG_SYS_SPCTL0_VAL		0x04001401	/* 96 MHz */
> +#define CONFIG_SYS_SPCTL1_VAL		0x0C000040
> +#define CONFIG_SYS_FREQ			96	/* MHz */
> +#endif /* CONFIG_SYS_OSC16 */
> +
> +/* external bus frequency (have to be a CONFIG_SYS_FREQ ratio) */
> +#define CONFIG_SYS_BUS_FREQ 	96	/* 96|48... MHz (BCLOCK and HCLOCK) */
> +#define CONFIG_USB_FREQ		48	/* 48 MHz */
> +#define CONFIG_PERIF1_FREQ	16	/* 16 MHz UART, Timer PWM */
> +#define CONFIG_PERIF2_FREQ 	48	/* 48 MHz LCD SD SPI */
> +#define CONFIG_PERIF3_FREQ	16	/* 16 MHz SSI */
> +
> +/*
> + * SDRAM definition parameter
> + */
> +/* we have and support only 1 bank of SDRAM */
> +#define CONFIG_NR_DRAM_BANKS 1
> +
> +#define CONFIG_SYS_SDRAM_MBYTE_SYZE 16
> +
> +/*
> + * CONFIG_SYS_SDRAM_NUM_COL 8, 9, 10 or 11 column address bits
> + * CONFIG_SYS_SDRAM_NUM_ROW 11, 12 or 13 row address bits
> + * CONFIG_SYS_SDRAM_REFRESH 0=OFF 1=2048 2=4096 3=8192 refresh
> + * CONFIG_SYS_SDRAM_CLOCK_CYCLE_CL_1 X ns clock cycle time when CL=1
> + * CONFIG_SYS_SDRAM_ROW_PRECHARGE_DELAY X ns SRP
> + * CONFIG_SYS_SDRAM_ROW_2_COL_DELAY X ns SRCD
> + * CONFIG_SYS_SDRAM_ROW_CYCLE_DELAY X ns SRC
> + * CONFIG_SYS_SDRAM_BURST_LENGTH 2^N BYTES (N=0..3)
> + * CONFIG_SYS_SDRAM_SINGLE_ACCESS 1= single access; 0 = Burst mode
> + */
> +
> +#if (CONFIG_SYS_SDRAM_MBYTE_SYZE == 8)

Why do we need such a stuff when your configuration is fixed at compile
time with CONFIG_SYS_SDRAM_MBYTE_SYZE 16 ?

> +/* lowlevel_linit copy U-Boot at CONFIG_SYS_INIT_SP_ADDR  (here in RAM) */
> +/* making u-boot runnable from flash and also RAM that is usefull to boot */
> +/* from serial port and from flash with only one version of U-Boot */
> +#ifndef CONFIG_SYS_TEXT_BASE
> +#define CONFIG_SYS_TEXT_BASE	0x08000000
> +#endif
> +
> +#define CONFIG_SYS_INIT_SP_ADDR	(CONFIG_SYS_SDRAM_BASE	\
> +		+ CONFIG_SYS_SDRAM_1_SIZE - 0x0100000)

You do not use U_Boot macro here (GENERATED_GBL_DATA_SIZE), as it is
usual. Stack pointer is already in RAM before relocation. Is it correct ?

> +/*#define CONFIG_MMC		1
> +*/

Dead code

> +/*
> + * External interfaces module
> + *
> + * CSxU_VAL:
> + * 63|    x    |x|x x|x x x x|x x| x | x  |x x x x|48
> + *   |DTACK_SEL|0|BCD|  BCS  |PSZ|PME|SYNC|  DOL  |
> + *
> + * 47| x x  | x x x x x x | x | x x x x | x x x x |32
> + *   | CNC  |     WSC     | 0 |   WWS   |   EDC   |
> + *
> + * CSxL_VAL:
> + * 31|  x x x x  | x x x x  | x x x x  | x x x x  |24
> + *   |    OEA    |   OEN    |   WEA    |   WEN    |
> + * 23|x x x x| x | x x x | x x  x x |x x| x |  x  | 0
> + *   |  CSA  |EBC|  DSZ  | 0|SP|0|WP|0 0|PA |CSEN |
> + */
> +
> +/* CS0 configuration for flash memory Micron MT28F128J3-150 */
> +#define CONFIG_SYS_CS0_CHIP_SELECT_ENABLE	1 /* 1 : enable CS0 */
> +#define CONFIG_SYS_CS0_DATA_PORT_SIZE		0x5 /* 5=16 bits on D[15:0] */
> +					/* 3=8bits on D[7:0] 6=32 bits..*/
> +#define CONFIG_SYS_CS0_SUPERVISOR_PROTECT	0 /* 1: user access prohibited*/
> +#define CONFIG_SYS_CS0_WRITE_PROTECT		0 /* 1: write prohibited*/
> +#define CONFIG_SYS_CS0_EB_SIGNAL_CONTROL_WRITE	1 /* 1 EB is as write signal */
> +#define CONFIG_SYS_CS0_READ_CYC_LGTH		150	/* ns */
> +#define CONFIG_SYS_CS0_OE_ASSERT_DLY		0	/* ns */
> +#define CONFIG_SYS_CS0_OE_NEG_DLY		0	/* ns */
> +
> +#define CONFIG_SYS_CS0_CS_NEG_LGTH 	0	/* ns CS HIGH to CS LOW : tCWH*/
> +#define CONFIG_SYS_CS0_XTRA_DEAD_CYC	35	/* ns from CS HIGH to tristate*/
> +
> +#define CONFIG_SYS_CS0_WRITE_XTRA_LGTH		0	/* ns */
> +#define CONFIG_SYS_CS0_EB_ASSERT_DLY		0	/* ns */
> +#define CONFIG_SYS_CS0_EB_NEG_DLY		0	/* ns */
> +#define CONFIG_SYS_CS0_CS_ASSERT_NEG_DLY	0	/* ns */
> +

All this stuff seems to me not general, that is they should not have a
CONFIG_SYS_* name. Else they must be documented.

Really I think that all this defines should be put in the code that use
them, and not in the configuration file (if they are not spread with
other boards).

> +#define CONFIG_SYS_CSCR_VAL\
> +	(CSCR_MASK 						\
> +	|((((CONFIG_SYS_FREQ/CONFIG_USB_FREQ)-1)&0x07)<<26)	\
> +	|((((CONFIG_SYS_FREQ/CONFIG_SYS_BUS_FREQ)-1)&0x0F)<<10))
> +

This macro (and the following ones) are related to the IMX processor and
are not specific to the board. They should be available for all IMX
boards, and the right place is arch/arm/include/asm/arch-imx/.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================