Dear Brian Cavagnolo,
In message <[email protected]> you wrote:
>
>
> Signed-off-by: Brian Cavagnolo <[email protected]>
> Signed-off-by: Andrey Yurovsky <[email protected]>
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: [email protected]
How much net work could a network work, if a network could net work?
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
