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

[Chin Liang See]

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>