[U-Boot] [PATCH] ARM: Add support for edb93xx boards

[Wolfgang Denk]

Dear Matthias Kaehlcke,

In message <20091206145444.GE22689 at darwin> you wrote:
> Add support for the Cirrus EP93xx platform and EDB93xx boards

In addition to Tom's comments:

> @@ -2429,6 +2429,42 @@ TQM834x_config:	unconfig
>  vme8349_config:		unconfig
>  	@$(MKCONFIG) $(@:_config=) ppc mpc83xx vme8349 esd
>  
> +edb93xx_config \
> +edb9301_config \
> +edb9302_config \
> +edb9302a_config \
> +edb9307_config \
> +edb9307a_config \
> +edb9315a_config: unconfig

Keep all such lists sorted!

> +	@if [ "$(findstring 01_,$@)" ] ; then \
> +		echo "#define        CONFIG_EDB9301" >> $(obj)include/config.h ; \
> +	elif [ "$(findstring 02_,$@)" ] ; then \
> +		echo "#define        CONFIG_EDB9302"  >> $(obj)include/config.h ; \
> +	elif [ "$(findstring 02a_,$@)" ] ; then \
> +		echo "#define        CONFIG_EDB9302A" >> $(obj)include/config.h ; \
> +	elif [ "$(findstring 07_,$@)" ] ; then \
> +		echo "#define        CONFIG_EDB9307"  >> $(obj)include/config.h ; \
> +	elif [ "$(findstring 07a_,$@)" ] ; then \
> +		echo "#define        CONFIG_EDB9307A" >> $(obj)include/config.h ; \
> +	elif [ "$(findstring 12_,$@)" ] ; then \
> +		echo "#define        CONFIG_EDB9312"  >> $(obj)include/config.h ; \
> +	elif [ "$(findstring 15_,$@)" ] ; then \
> +		echo "#define        CONFIG_EDB9315"  >> $(obj)include/config.h ; \
> +	elif [ "$(findstring 15a_,$@)" ] ; then \
> +		echo "#define        CONFIG_EDB9315A" >> $(obj)include/config.h ; \
> +	fi ;

Please don't do scripting in the Makefile. Move this logic into your
board config file instead.

> +	@$(MKCONFIG) -a edb93xx arm arm920t edb93xx NULL ep93xx
> +	@if [ "$(findstring 01_,$@)" ] || [ "$(findstring 02_,$@)" ]; then \
> +		echo "TEXT_BASE = 0x05400000" >> $(obj)include/config.mk ; \
> +	elif [ "$(findstring 02a_,$@)" ]; then \
> +		echo "TEXT_BASE = 0xc5400000" >> $(obj)include/config.mk ; \
> +	elif [ "$(findstring 07_,$@)" ] || [ "$(findstring 12_,$@)" ] || [ "$(findstring 15_,$@)" ]; then \
> +		echo "TEXT_BASE = 0x01f00000" >> $(obj)include/config.mk ; \
> +	elif [ "$(findstring 07a_,$@)" ] || [ "$(findstring 15a_,$@)" ]; then \
> +		echo "TEXT_BASE = 0xc1f00000" >> $(obj)include/config.mk ; \
> +	fi ;

Ditto.

> +	/* Machine number, as defined in linux/arch/arm/tools/mach-types
> +	 */
> +#ifdef CONFIG_EDB9301
> +	gd->bd->bi_arch_number = 462;
> +#elif defined(CONFIG_EDB9302)
> +	gd->bd->bi_arch_number = 538;
> +#elif defined(CONFIG_EDB9302A)
> +	gd->bd->bi_arch_number = 1127;
> +#elif (defined CONFIG_EDB9307)
> +	gd->bd->bi_arch_number = 607;
> +#elif (defined CONFIG_EDB9307A)
> +	gd->bd->bi_arch_number = 1128;
> +#elif defined(CONFIG_EDB9312)
> +	gd->bd->bi_arch_number = 451;
> +#elif defined(CONFIG_EDB9315)
> +	gd->bd->bi_arch_number = 463;
> +#elif defined(CONFIG_EDB9315A)
> +	gd->bd->bi_arch_number = 772;
> +#endif

Please use symbolic constants from <asm/mach-types.h>, and move the
logic into your board config file.

> +int dram_init(void)
> +{
> +	DECLARE_GLOBAL_DATA_PTR;
> +	unsigned int *src, *dst;
> +	int i;
> +
> +	gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
> +	gd->bd->bi_dram[0].size = PHYS_SDRAM_SIZE_1;
> +
> +#ifdef PHYS_SDRAM_2
> +	gd->bd->bi_dram[1].start = PHYS_SDRAM_2;
> +	gd->bd->bi_dram[1].size = PHYS_SDRAM_SIZE_2;
> +#endif
> +
> +#ifdef PHYS_SDRAM_3
> +	gd->bd->bi_dram[2].start = PHYS_SDRAM_3;
> +	gd->bd->bi_dram[2].size = PHYS_SDRAM_SIZE_3;
> +#endif
> +
> +#ifdef PHYS_SDRAM_4
> +	gd->bd->bi_dram[3].start = PHYS_SDRAM_4;
> +	gd->bd->bi_dram[3].size = PHYS_SDRAM_SIZE_4;
> +#endif

Make sure to use get_ram_size() on all banks for checking and
auto-sizing.

> diff --git a/board/edb93xx/flash_cfg.S b/board/edb93xx/flash_cfg.S
> new file mode 100644
> index 0000000..aa61a3c
> --- /dev/null
> +++ b/board/edb93xx/flash_cfg.S
...
> +.globl flash_cfg
> +flash_cfg:
> +	/* configure smc bank 6 (Intel TE28F128J3D75 Strata Flash)
> +	 * ebibrkdis: 0, mw: 0x1 (16 bit), pme: 0, wp: 0, wst2: 2(+1),
> +	 * wst1: 5(+1), rble: 1, idcy: 2(+1)
> +	 * TODO: we don't enable page mode for now
> +	 */
> +	ldr r0, =SMC_BCR6
> +
> +	ldr r1, =(2 << SMC_BCR_IDCY_SHIFT | 5 << SMC_BCR_WST1_SHIFT | \
> +		  SMC_BCR_BLE | 2 << SMC_BCR_WST2_SHIFT | 1 << SMC_BCR_MW_SHIFT)
> +
> +	str r1, [r0]
> +
> +	mov pc, lr

Why is this written in assembler? Please rewrite in C.

> diff --git a/board/edb93xx/pll_cfg.S b/board/edb93xx/pll_cfg.S
> new file mode 100644
> index 0000000..ae69a7d
> --- /dev/null
> +++ b/board/edb93xx/pll_cfg.S
> @@ -0,0 +1,93 @@
...
> +	/* the user's guide recommends to wait at least 1 ms for PLL2 to
> +	 * stabilize, but Cirrus' Redboot doesn't do that, either
> +	 */
> +
> +	mov pc, lr

Why don't you write this as C code? And why do you copy bad (or at
least not recommended behaviour, even if it seems to work for you?
Adding a "udelay(1000);" seems so trivial that I wonder why you don;t
follow the docs?

> diff --git a/board/edb93xx/sdram_cfg.S b/board/edb93xx/sdram_cfg.S
> new file mode 100644
> index 0000000..752bb72
> --- /dev/null
> +++ b/board/edb93xx/sdram_cfg.S

Stop here. Why again assembler?

U-Boot uses assembler only when ther eis no reasonable way to
implement the code in C, and I don't see any such justification here.
Please rewrite all this in C.

> diff --git a/board/edb93xx/u-boot.lds b/board/edb93xx/u-boot.lds
> new file mode 100644
> index 0000000..76caef3
> --- /dev/null
> +++ b/board/edb93xx/u-boot.lds


This looks pretty generic. Please explain why exactly you think you
need a board specific linker script here, instead of using the generic
one?

> diff --git a/cpu/arm920t/ep93xx/Makefile b/cpu/arm920t/ep93xx/Makefile
> new file mode 100644
> index 0000000..9763e15
> --- /dev/null
> +++ b/cpu/arm920t/ep93xx/Makefile
> @@ -0,0 +1,55 @@
> +# vim: set ts=8 sw=8 noet:
> +#
> +# Cirrus Logic EP93xx CPU-specific Makefile
> +#
> +# Copyright (C) 2004, 2005
> +# Cory T. Tusar, Videon Central, Inc., <ctusar at videon-central.com>
> +#
> +# Copyright (C) 2006
> +# Dominic Rath <Dominic.Rath at gmx.de>

Hm... Your signed-off-by: line does not mention Cory Tusar nor Dominic
Rath, so which part exactly do these have in the code you are
submitting here?

> diff --git a/cpu/arm920t/ep93xx/cpu.c b/cpu/arm920t/ep93xx/cpu.c
> new file mode 100644
> index 0000000..3989ee8
> --- /dev/null
> +++ b/cpu/arm920t/ep93xx/cpu.c
...
> +#include <common.h>
> +
> +#if defined(CONFIG_EP93XX)

In which cases would this #define be needed?  Your Makefiles should
only build files which are needed in the first place - such a #define
here seems wrong to me.

> +/* All EP93xx variants have 16 KiB I-cache. */
> +extern int checkicache(void)
> +{
> +	return 16 << 10;
> +}
> +
> +
> +/* All EP93xx variants have 16 KiB D-cache. */
> +extern int checkdcache(void)
> +{
> +	return 16 << 10;
> +}

If you need something like this, then provide it as static inline
functions in the respective header file.

> +/* This is a nop on ARM, and is included here for completeness only. */
> +extern void upmconfig(unsigned int upm, unsigned int *table, unsigned int size)
> +{
> +	/* nop */
> +}

Get rid of unused crap, please.

> +
> +/* We reset the CPU by generating a 1-->0 transition on DeviceCfg bit 31. */
> +extern void reset_cpu(ulong addr)
> +{
> +	uint32_t value;
> +
> +	/* Unlock DeviceCfg and set SWRST */
> +	writel(0xAA, SYSCON_SYSSWLOCK);
> +	value = readl(SYSCON_DEVICECFG);
> +	value |= SYSCON_DEVICECFG_SWRST;
> +	writel(value, SYSCON_DEVICECFG);
> +
> +	/* Unlock DeviceCfg and clear SWRST */
> +	writel(0xAA, SYSCON_SYSSWLOCK);
> +	value = readl(SYSCON_DEVICECFG);
> +	value &= ~SYSCON_DEVICECFG_SWRST;
> +	writel(value, SYSCON_DEVICECFG);
> +
> +	/* Dying... */
> +	while (1)
> +		; /* nop */
> +}
> +
> +
> +#endif	/* defined(CONFIG_EP93XX) */
> diff --git a/cpu/arm920t/ep93xx/eth.c b/cpu/arm920t/ep93xx/eth.c
> new file mode 100644
> index 0000000..513c221
> --- /dev/null
> +++ b/cpu/arm920t/ep93xx/eth.c
...
> +/**
> + * Send a trace message to the terminal.
> + */
> +#if 0
...

Here and elsewhere: please do not add dead code.

> +struct rx_status {
> +	union {
> +		uint32_t word1;
> +
> +		struct {
> +			unsigned:8;
> +			unsigned hti:6;
> +			unsigned:1;
> +			unsigned crci:1;
> +			unsigned crce:1;
> +			unsigned edata:1;
> +			unsigned runt:1;
> +			unsigned fe:1;
> +			unsigned oe:1;
> +			unsigned rx_err:1;
> +			unsigned am:2;
> +			unsigned:4;
> +			unsigned eob:1;
> +			unsigned eof:1;
> +			unsigned rwe:1;
> +			unsigned rfp:1;
> +		};
> +	};

Do not use bit fields. These are unportable and a major PITA in any
case.

> +	union {
> +		uint32_t word2;
> +
> +		struct {
> +			unsigned frame_length:16;
> +			unsigned buffer_index:15;
> +			unsigned rfp:1;
> +		};
> +	};
> +} __attribute__((packed));
> +
> +
> +/**
> + * Transmit descriptor queue entry
> + */
> +struct tx_descriptor {


I think this whole declarations stuff belongs to some separate header
file.


> +/**
> + * Dump ep93xx_mac values to the terminal.
> + */
> +inline void dump_dev(void)
> +{
> +#if defined(EP93XX_MAC_DEBUG)

Don't compile the function at all when debug is not selected. Ditto
for the other debug functions.

> diff --git a/cpu/arm920t/ep93xx/speed.c b/cpu/arm920t/ep93xx/speed.c
> new file mode 100644
> index 0000000..cc32ec7
> --- /dev/null
> +++ b/cpu/arm920t/ep93xx/speed.c
...
> +/* ------------------------------------------------------------------------- */
> +/* NOTE: This describes the proper use of this file.

This line just adds bloat.

> diff --git a/cpu/arm920t/ep93xx/timer.c b/cpu/arm920t/ep93xx/timer.c
> new file mode 100644
> index 0000000..28660d0
> --- /dev/null
> +++ b/cpu/arm920t/ep93xx/timer.c
> @@ -0,0 +1,163 @@
> +/* vim: set ts=8 sw=8 noet:
> + *
> + * Cirrus Logic EP93xx interrupt support.

Why is the file name "timer.c" when it's interrupt support actually?

> +int timer_init(void)
> +{
> +	/* use timer 1 with 2KHz and free running */
> +	writel(0x00, TIMER1_CONTROL);
> +	if (timer_load_val == 0) {
> +		/*
> +		 * for 10 ms clock period @ PCLK with 4 bit divider = 1/2
> +		 * (default) and prescaler = 16. Should be 10390
> +		 * @33.25MHz and 15625 @ 50 MHz
> +		 */
> +
> +		/* set to constant just now, until I resolve clocking issues */
> +		timer_load_val = 21;
> +	}
> +	/* auto load, manual update of Timer 1 */
> +	lastdec = timer_load_val;
> +	writel(timer_load_val, TIMER1_LOAD);
> +
> +	/* Enable the timer and periodic mode */
> +	writel(0xC0, TIMER1_CONTROL);
> +
> +	return 0;
> +}
> +
> +/*
> + * timer without interrupts
> + */
> +
> +void reset_timer(void)
> +{
> +	reset_timer_masked();
> +}
> +
> +ulong get_timer(ulong base)
> +{
> +	return get_timer_masked() - base;
> +}

Hm.... does not seem to be too much of interrupt support, so please
fix the comments!

> +void udelay(unsigned long usec)

This needs to be adapted to the current implementation. Make this
__udelay() here.


> diff --git a/include/asm-arm/arch-ep93xx/ep93xx.h b/include/asm-arm/arch-ep93xx/ep93xx.h
> new file mode 100644
> index 0000000..47e2a21
> --- /dev/null
> +++ b/include/asm-arm/arch-ep93xx/ep93xx.h
...
> +#define DMAMP_TX_0_CONTROL		(DMA_BASE + 0x0000)
> +#define DMAMP_TX_0_INTERRUPT		(DMA_BASE + 0x0004)
> +#define DMAMP_TX_0_PPALLOC		(DMA_BASE + 0x0008)
> +#define DMAMP_TX_0_STATUS		(DMA_BASE + 0x000C)
> +#define DMAMP_TX_0_REMAIN		(DMA_BASE + 0x0014)
> +#define DMAMP_TX_0_MAXCNT0		(DMA_BASE + 0x0020)
> +#define DMAMP_TX_0_BASE0		(DMA_BASE + 0x0024)
> +#define DMAMP_TX_0_CURRENT0		(DMA_BASE + 0x0028)
> +#define DMAMP_TX_0_MAXCNT1		(DMA_BASE + 0x0030)
> +#define DMAMP_TX_0_BASE1		(DMA_BASE + 0x0034)
> +#define DMAMP_TX_0_CURRENT1		(DMA_BASE + 0x0038)

Get rid of all this address / offset based register accesses. Turn
this into a C struct instead. Please fix globally.


> diff --git a/include/configs/edb93xx.h b/include/configs/edb93xx.h
> new file mode 100644
> index 0000000..6c4576b
> --- /dev/null
> +++ b/include/configs/edb93xx.h
...
> +/* Initial environment and monitor configuration options. */
> +#define CONFIG_ETHADDR			08:00:3E:26:0A:5B
> +#define CONFIG_NETMASK			255.255.255.0
> +#define CONFIG_IPADDR			192.168.99.225
> +#define CONFIG_SERVERIP			192.168.99.1
> +#define CONFIG_GATEWAYIP		192.168.99.1

NAK. We don't allow board specific settings like these.

Also note that the MAC address seems to be hijacked - it actually
belongs to the codex corporation. Please make sure to get your own set
of MAC addresses.


> +#define CONFIG_SYS_CLK_FREQ	14745600	/* EP93xx has a 14.7456 clock  */
> +#define CONFIG_SYS_HZ		2048		/* Timer 3 set for 2KHz        */

CONFIG_SYS_HZ must always and everywhere be 1000.

> +#define CONFIG_SYS_CLKS_IN_HZ			/* Everything in Hz            */

This was removed a long time ago. Don't re-introduce it.

> +#if 0
> +#undef CONFIG_CMD_BDI
> +#endif

See above - don;t add dead code.

> +#elif defined(CONFIG_EDB9302A)
> +#define CONFIG_NR_DRAM_BANKS	4		/* EDB9302a has 4 banks of SDRAM  */
> +#define PHYS_SDRAM_1		0xc0000000	/* consisting of 1x Samsung       */
> +#define PHYS_SDRAM_SIZE_1	0x00800000	/* K4S561632E-TC75 256 Mbit       */
> +#define PHYS_SDRAM_2		0xc1000000	/* SDRAM on a 16-bit data bus,    */
> +#define PHYS_SDRAM_SIZE_2	0x00800000	/* for a total of 32MB of SDRAM.  */
> +#define PHYS_SDRAM_3		0xc4000000	/* We set the SROMLL bit on the   */
> +#define PHYS_SDRAM_SIZE_3	0x00800000	/* processor, resulting in this   */
> +#define PHYS_SDRAM_4		0xc5000000	/* non-contiguous memory map.	  */

The comment is all single line comments, so it is supposed to refer to
the actual code in the same line. This is obviously not the case.
Please fix.

> +#define CONFIG_EDB93XX_SDCS0
> +#define CONFIG_SYS_MEMTEST_START	0xc0000000
> +#define CONFIG_SYS_MEMTEST_END		0xc0800000

Has this actually been tested?

> +#define CONFIG_ENV_ADDR		0x60040000
> +#if (defined(CONFIG_EDB9301) || defined(CONFIG_EDB9302) ||\
> +     defined(CONFIG_EDB9302A))
> +#define CONFIG_ENV_SECT_SIZE	0x00020000
> +#elif (defined(CONFIG_EDB9307) || defined(CONFIG_EDB9307A) ||\
> +       defined(CONFIG_EDB9312) || defined(CONFIG_EDB9315) ||\
> +       defined(CONFIG_EDB9315A))
> +#define CONFIG_ENV_SECT_SIZE	0x00040000
> +#endif
> +#define CONFIG_ENV_ADDR_REDUND	(CONFIG_ENV_ADDR + CONFIG_ENV_SECT_SIZE)
> +
> +#define CONFIG_ENV_SIZE		CONFIG_ENV_SECT_SIZE
> +#define CONFIG_ENV_SIZE_REDUND	CONFIG_ENV_SIZE
> +
> +#define CONFIG_SYS_JFFS2_FIRST_BANK	0
> +#if (defined(CONFIG_EDB9301) || defined(CONFIG_EDB9302) ||\
> +     defined(CONFIG_EDB9302A))
> +#define CONFIG_SYS_JFFS2_FIRST_SECTOR	28
> +#elif (defined(CONFIG_EDB9307) || defined(CONFIG_EDB9307A) ||\
> +       defined(CONFIG_EDB9312) || defined(CONFIG_EDB9315) ||\
> +       defined(CONFIG_EDB9315A))
> +#define CONFIG_SYS_JFFS2_FIRST_SECTOR	14
> +#endif

Hm... this looks very much like being completely unmaintainable.

Please simplify the code - in your own interest.

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
Save energy:  Drive a smaller shell.