[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