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

Marek Vasut
2012-08-30T18:02:30Z

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