[U-Boot] [PATCHv1] ARM: Add Altera SOCFPGA Cyclone5

Chin Liang See
2012-09-03T02:59:52Z

Hi Pavel,

On the constant 0x3, its to enable the timer plus configure the timer mode to user-defined count mode.
We can covert them to constant as below:

#define TIMER_ENABLE (1<<0)
#define TIMER_MODE_USERDEF (1<<1)

writel((readl((&timer_base->ctrl)) | TIMER_ENABLE | TIMER_MODE_USERDEF )

Thanks and have a nice day!

Chin Liang
Direct Line: +604 - 636 8776

-----Original Message-----
From: Pavel Machek [mailto:pavel at denx.de]
Sent: Friday, August 31, 2012 1:00 AM
To: Marek Vasut
Cc: Tom Rini; albert.u.boot at aribaud.net; ZY - u-boot; dinh.linux at gmail.com; Chin Liang See
Subject: Re: [U-Boot] [PATCHv1] ARM: Add Altera SOCFPGA Cyclone5

Hi!

> > I took oportunity to cleanup whitespace in
> > arch/arm/cpu/armv7/omap-common/u-boot-spl.lds. Perhaps someone can
> > merge that...
>
> Argh ... what about using git send-email for the patch submission please?

I'm not supposed to submit patches here, these are just requests for review ;-).

> NOTE: I really have a great deal of respect for Pavel, so i do have
> trouble stepping on him properly during the patch review ;-)

;-).

> > + ldr r0, =CONFIG_SYS_INIT_RAM_ADDR
> > + ldr r1, =CONFIG_SYS_INIT_RAM_SIZE
> > + add r0, r0, r1
>
> Won't the preprocessor compute (CONFIG_SYS_INIT_RAM_ADDR +
> CONFIG_SYS_INIT_RAM_SIZE) for you? Then you'd only have to do one LDR
> and no ADD instruction, saving two ticks of CPU :-)

:-). And opportunity for me to try my luck at arm assembly. Does this look correct?

ldr r0, =(CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE)
ldr r1, =IRQ_STACK_START_IN
str r0, [r1]

bx lr

> > +static const struct socfpga_reset_manager *reset_manager_base =
> > + (void *)SOCFPGA_RSTMGR_ADDRESS;
> > +
> > +/*
> > + * Write the reset manager register to cause reset */
>
> You might want to add __attribute__((noreturn)) to this function :-)

Hmm... I guess that should go to the central header file? No need to sprinkle it all through the C files. (Done).

> > + writel(TIMER_LOAD_VAL, &timer_base->load_val);
> > + writel(TIMER_LOAD_VAL, &timer_base->curr_val);
> > + writel((readl((&timer_base->ctrl)) | 0x3),
>
> I think you should stick to programming in C here, not ((((LISP)))),
> so try cutting down on the ((braces)) :-)

Some braces have been harmed by creation of this patch.

> btw. what's this 0x3 magic constant ?

Dinh has to help here :-(.

> > +static inline void delay(unsigned long loops) {
> > + __asm__ volatile ("1:\n"
> > + "subs %0, %1, #1\n"
> > + "bne 1b" : "=r" (loops) : "0" (loops)); }
>
> Am I flat blind or is this not used here?

Killed, thanks. Lets see what the compiler thinks.

Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

-------------- next part --------------
A non-text attachment was scrubbed...
Name: winmail.dat
Type: application/ms-tnef
Size: 6008 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120903/95f407e3/attachment.bin>