On Tue, Jun 1, 2010 at 3:38 PM, Wolfgang Denk <w...@denx.de> wrote: > Dear Brian Cavagnolo, > > In message <1275417750-10020-1-git-send-email-br...@cozybit.com> you wrote: >> >> >> Signed-off-by: Brian Cavagnolo <br...@cozybit.com> >> Signed-off-by: Andrey Yurovsky <yurov...@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?
This appears to be the convention in the source tree and it seems straightforward and clean. Is there a different preferred way to add new CPUs? I understand your other comments. Thanks, Brian >> --- /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: w...@denx.de > How much net work could a network work, if a network could net work? > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot