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

[Marek Vasut]

Dear Pavel Machek,

> 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

Yes

> > > +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.

At least one more pair must be killed on the above statement :)

> > btw. what's this 0x3 magic constant ?
> 
> Dinh has to help here :-(.

Also, use setbits_le32() there instead anyway.

> > > +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

Best regards,
Marek Vasut