[U-Boot] [PATCH 1/3] add support for arm926ejs-based pollux CPU

[Wolfgang Denk]

Dear Brian Cavagnolo,

In message <1275417750-10020-1-git-send-email-brian at cozybit.com> you wrote:
> 
> 
> Signed-off-by: Brian Cavagnolo <brian at cozybit.com>
> Signed-off-by: Andrey Yurovsky <yurovsky at gmail.com>

Please be a bit more verbose - who is manufacturing this pollux
thingy, who will be maintaining the code, etc.

>  arch/arm/cpu/arm926ejs/pollux/Makefile    |   51 ++++++++
>  arch/arm/cpu/arm926ejs/pollux/reset.S     |   49 ++++++++
>  arch/arm/cpu/arm926ejs/pollux/timer.c     |  190 +++++++++++++++++++++++++++++

Why exactly do we need a new directory for it?


> --- /dev/null
> +++ b/arch/arm/cpu/arm926ejs/pollux/reset.S
> @@ -0,0 +1,49 @@
...
> +	.align	5
> +.globl reset_cpu
> +reset_cpu:
> +	ldr	r1, rstctl1	/* get clkm1 reset ctl */
> +	mov	r3, #0x0
> +	strh	r3, [r1]	/* clear it */
> +	mov	r3, #0x8
> +	strh	r3, [r1]	/* force dsp+arm reset */
> +_loop_forever:
> +	b	_loop_forever
> +
> +rstctl1:
> +	.word	0xfffece10

This seems identical to /arm/cpu/arm926ejs/omap/reset.S and
arch/arm/cpu/arm926ejs/versatile/reset.S to me.  Why do we need a 3rd
copy of the same code?

Please factor out common code.

> diff --git a/arch/arm/cpu/arm926ejs/pollux/timer.c b/arch/arm/cpu/arm926ejs/pollux/timer.c
> new file mode 100644
> index 0000000..fc6c699
> --- /dev/null
> +++ b/arch/arm/cpu/arm926ejs/pollux/timer.c
...
> +#ifndef CONFIG_SYS_TIMERBASE
> +#error "Please define CONFIG_SYS_TIMERBASE to a suitable TIMERx_BASE"
> +#endif
> +#define TIMERBASE CONFIG_SYS_TIMERBASE
> +
> +#define TIMER_LOAD_VAL 0xffffffff
> +
> +static ulong inline read_timer(void)
> +{
> +	REG32(TIMERBASE + TMRCONTROL) |= (1<<LDCNT);
> +	return REG32(TIMERBASE + TMRMATCH);
> +}

We don;t allow register accesses through base address + offset any
more. Please declare porper C structs and use proper I/O accessors.
Please fix globally.

> +	if (lastdec >= now) {		/* normal mode (non roll) */
> +		/* normal mode */
> +		timestamp += lastdec - now; /* move stamp fordward with absoulte diff ticks */

LIne too long. Please fix globally.

> +	} else {			/* we have overflow of the count down timer */
> +		/* nts = ts + ld + (TLV - now)
> +		 * ts=old stamp, ld=time that passed before passing through -1
> +		 * (TLV-now) amount of time after passing though -1
> +		 * nts = new "advancing time stamp"...it could also roll and cause problems.
> +		 */

Incorrect multiline comment style. Please fix globally.

> +/* Clock and Power Control Registers */
> +#define CLKPWR_BASE		0xC000F000
> +#define CLKMODEREG		(CLKPWR_BASE + 0x000)
> +#define PLLSETREG0		(CLKPWR_BASE + 0x004)
> +#define PLLSETREG1		(CLKPWR_BASE + 0x008)
> +#define GPIOWAKEUPENB		(CLKPWR_BASE + 0x040)
> +#define RTCWAKEUPENB		(CLKPWR_BASE + 0x044)
> +#define GPIOWAKEUPRISEENB	(CLKPWR_BASE + 0x048)
> +#define GPIOWAKEUPFALLENB	(CLKPWR_BASE + 0x04C)
> +#define GPIOPEND		(CLKPWR_BASE + 0x050)
> +#define INTPENDSPAD		(CLKPWR_BASE + 0x058)
> +#define PWRRSTSTATUS		(CLKPWR_BASE + 0x05C)
> +#define INTENB			(CLKPWR_BASE + 0x060)
> +#define PWRMODE			(CLKPWR_BASE + 0x07C)
> +#define PADSTRENGTHGPIOAL	(CLKPWR_BASE + 0x100)
> +#define PADSTRENGTHGPIOAH	(CLKPWR_BASE + 0x104)
> +#define PADSTRENGTHGPIOBL	(CLKPWR_BASE + 0x108)
> +#define PADSTRENGTHGPIOBH	(CLKPWR_BASE + 0x10C)
> +#define PADSTRENGTHGPIOCL	(CLKPWR_BASE + 0x110)
> +#define PADSTRENGTHGPIOCH	(CLKPWR_BASE + 0x114)
> +#define PADSTRENGTHBUS		(CLKPWR_BASE + 0x118)

NAK!  See above - please declare proper C structs instead.

> diff --git a/arch/arm/include/asm/arch-pollux/gpio.h b/arch/arm/include/asm/arch-pollux/gpio.h
> new file mode 100644
> index 0000000..f6ddd1b
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-pollux/gpio.h
...
> +/* GPIO registers */
> +#define GPIO_BASE		0xC000A000
> +
> +#define GPIOAOUT		(GPIO_BASE + 0x00)
> +#define GPIOAOUTENB		(GPIO_BASE + 0x04)
> +#define GPIOADETMODE0		(GPIO_BASE + 0x08)
> +#define GPIOADETMODE1		(GPIO_BASE + 0x0C)
> +#define GPIOAINTENB		(GPIO_BASE + 0x10)
> +#define GPIOADET		(GPIO_BASE + 0x14)
> +#define GPIOAPAD		(GPIO_BASE + 0x18)
> +#define GPIOAPUENB		(GPIO_BASE + 0x1C)
> +#define GPIOAALTFN0		(GPIO_BASE + 0x20)
> +#define GPIOAALTFN1		(GPIO_BASE + 0x24)

NAK again.

> diff --git a/arch/arm/include/asm/arch-pollux/reg.h b/arch/arm/include/asm/arch-pollux/reg.h
> new file mode 100644
> index 0000000..fba6216
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-pollux/reg.h
...
> +/* register access and manipulation helper macros */
> +#define REG8(addr)  (*((volatile unsigned char *)(addr)))
> +#define REG16(addr) (*((volatile unsigned short *)(addr)))
> +#define REG32(addr) (*((volatile unsigned long *)(addr)))

NAK again! Please use proper I/O accessors instead.

> +/* UART register offsets */
> +#define LCON			0x00
> +#define UCON			0x02
> +#define FCON			0x04
> +#define MCON			0x06
> +#define TRSTATUS		0x08
> +#define ESTATUS			0x0A
> +#define FSTATUS			0x0C
> +#define MSTATUS			0x0E
> +#define THB			0x10
> +#define RHB			0x12
> +#define BRD			0x14
> +#define TIMEOUTREG		0x16
> +#define INTSTATUSREG		0x18
> +#define UARTCLKENB		0x40
> +#define UARTCLKGEN		0x44

Is this really such a new UART that we need new code for it?

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
How much net work could a network work, if a network could net work?