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

[Matthias Kaehlcke]

hi wolfgang,

thanks a lot for you review, below some comments and questions

El Mon, Dec 07, 2009 at 08:46:29PM +0100 Wolfgang Denk ha dit:

> 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!

ok

> > +	@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.

i got the inspiration to handle it this way from the U-Boot Makefile
(Total5100, TQM5200, et al). could you please point me to an example
of a well done multi-board configuration file?

> > +	@$(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 ;

same here, further i don't understand how to set the TEXT_BASE from
the board configuration file. any pointer?

> > +	/* 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.

ok

> > +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.

ok

> > 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.

ok

> > 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?

ok

> > 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?

ignorance ...

> 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.

i'm just starting to get my feet wet with low level initialization and
supposed sdram setup is always done in assembler.

> > 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?

the ep93xx expects to find the pattern 'CRUS' (in ASCII) at position
0x1000. is there a more canonical way to achieve this?

> > 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?

as i stated in the patch description this patch is based on a patch
(http://article.gmane.org/gmane.linux.ports.arm.cirrus/722) from
Dominic Rath.

my contribution consists mainly in porting the patch to the current
U-Boot version, add support for more boards, tweak pll settings,
introduce the use of i/o accessors, adding comments and trying to
bring the patch in a shape ready for mainline.

as the header files contain a GPL header i interpreted that it's ok to
submit them without signed-off-by lines from the original authors (what
doesn't mean that i claim authorship on their work):

"The contribution is based upon previous work that, to the best
 of my knowledge, is covered under an appropriate open source
 license and I have the right under that license to submit
 that work with modifications, whether created in whole or in
 part by me, under the same open source license (unless I am
 permitted to submit under a different license), as indicated
 in the file"

(Developer's Certificate of Origin 1.1, point b)

> > 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.

agreed

> > +/* 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.

ok

> > +/* 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.

ok

> > +
> > +/* 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.

ok

> > +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.

ok

> > +	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.

ok

> > +/**
> > + * 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.

ok

> > 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.

ok

> > 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?

bad comment, will fix

> > +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!

ok

> > +void udelay(unsigned long usec)
> 
> This needs to be adapted to the current implementation. Make this
> __udelay() here.

ok

> > 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.

ok

> > 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.

hm, a whole bunch of boards do this, so i thought that's the way to
go. what is the correct thing, not defining this values at all?

> 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.

good point

i'm not working for cirrus, so i think getting my own set of MAC
address is not really what i'm supposed to do. any further advice?

> > +#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.

ok

> > +#define CONFIG_SYS_CLKS_IN_HZ			/* Everything in Hz            */
> 
> This was removed a long time ago. Don't re-introduce it.

ok

> > +#if 0
> > +#undef CONFIG_CMD_BDI
> > +#endif
> 
> See above - don;t add dead code.

ok

> > +#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.

ok

> > +#define CONFIG_EDB93XX_SDCS0
> > +#define CONFIG_SYS_MEMTEST_START	0xc0000000
> > +#define CONFIG_SYS_MEMTEST_END		0xc0800000
> 
> Has this actually been tested?

gotcha, will fix

> > +#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.

i'm not sure if i understand what you mean bt simplifying in this case

do you propose something like

#ifdef CONFIG_EDB9301
#define CONFIG_ENV_SECT_SIZE	0x00020000
#elif defined(CONFIG_EDB9302)
#define CONFIG_ENV_SECT_SIZE	0x00020000
#elif ...

#ifdef CONFIG_EDB9301
#define CONFIG_SYS_JFFS2_FIRST_SECTOR	28
#elif defined(CONFIG_EDB9302)
#define CONFIG_SYS_JFFS2_FIRST_SECTOR	28
#elif ...

or even

#ifdef CONFIG_EDB9301
#define CONFIG_ENV_SECT_SIZE	0x00020000
#define CONFIG_SYS_JFFS2_FIRST_SECTOR	28
#elif ...

though the settings aren't related?

with the current approach i tried to avoid redundancy in the
declarations, with the result of more complex conditions. please give
me a pointer to the preferred way to do this kind of stuff.

thanks again for your time and for helping me to get the patch into
the right shape.

i'll address the issues you and tom brought up and submit a second
revision of the patch at a later time

best regards

-- 
Matthias Kaehlcke
Embedded Linux Developer
Barcelona

       The yellow ships hung in the air just like bricks dont do
                 (The Hitch-Hiker's Guide to the Galaxy)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-