From mboxrd@z Thu Jan  1 00:00:00 1970
From: Pavel Machek <pavel@denx.de>
Date: Thu, 30 Aug 2012 18:59:51 +0200
Subject: [U-Boot] [PATCHv1] ARM: Add Altera SOCFPGA Cyclone5
In-Reply-To: <201208300156.32100.marex@denx.de>
References: <1345157139-19585-1-git-send-email-dinguyen@altera.com>
	<20120829182645.GB3450@bill-the-cat>
	<20120829233412.GB23417@elf.ucw.cz>
	<201208300156.32100.marex@denx.de>
Message-ID: <20120830165951.GC5980@elf.ucw.cz>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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