Dear Matthias Kaehlcke, In message <20091206145444.ge22...@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., <ctu...@videon-central.com> > +# > +# Copyright (C) 2006 > +# Dominic Rath <dominic.r...@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: w...@denx.de Save energy: Drive a smaller shell. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot