[U-Boot] [PATCH 1/3] add support for arm926ejs-based pollux CPU
Brian Cavagnolo
brian at cozybit.com
Mon Jun 21 14:25:37 CEST 2010
On Tue, Jun 1, 2010 at 3:38 PM, Wolfgang Denk <wd at denx.de> wrote:
> 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?
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 at denx.de
> How much net work could a network work, if a network could net work?
>
More information about the U-Boot
mailing list