On Tue, Jun 1, 2010 at 3:38 PM, Wolfgang Denk wd@denx.de wrote:
Dear Brian Cavagnolo,
In message 1275417750-10020-1-git-send-email-brian@cozybit.com you wrote:
Signed-off-by: Brian Cavagnolo brian@cozybit.com Signed-off-by: Andrey Yurovsky yurovsky@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: wd@denx.de How much net work could a network work, if a network could net work?