[U-Boot] [PATCH v4 14/27] Introduce generic pre-relocation board_f.c

[Simon Glass]

Hi Scott,

On Thu, Mar 15, 2012 at 12:09 PM, Scott Wood <scottwood at freescale.com> wrote:
> On 03/14/2012 09:16 PM, Simon Glass wrote:
>> +/*
>> + * sjg: IMO this code should be
>> + * refactored to a single function, something like:
>> + *
>> + * void led_set_state(enum led_colour_t colour, int on);
>> + */
>> +/************************************************************************
>> + * Coloured LED functionality
>> + ************************************************************************
>> + * May be supplied by boards if desired
>> + */
>> +inline void __coloured_LED_init(void) {}
>> +void coloured_LED_init(void)
>> +     __attribute__((weak, alias("__coloured_LED_init")));
>> +inline void __red_led_on(void) {}
>> +void red_led_on(void) __attribute__((weak, alias("__red_led_on")));
>> +inline void __red_led_off(void) {}
>> +void red_led_off(void) __attribute__((weak, alias("__red_led_off")));
>> +inline void __green_led_on(void) {}
>> +void green_led_on(void) __attribute__((weak, alias("__green_led_on")));
>> +inline void __green_led_off(void) {}
>> +void green_led_off(void) __attribute__((weak, alias("__green_led_off")));
>> +inline void __yellow_led_on(void) {}
>> +void yellow_led_on(void) __attribute__((weak, alias("__yellow_led_on")));
>> +inline void __yellow_led_off(void) {}
>> +void yellow_led_off(void) __attribute__((weak, alias("__yellow_led_off")));
>> +inline void __blue_led_on(void) {}
>> +void blue_led_on(void) __attribute__((weak, alias("__blue_led_on")));
>> +inline void __blue_led_off(void) {}
>> +void blue_led_off(void) __attribute__((weak, alias("__blue_led_off")));
>
> Is this really the right file for this?

Not in my opinion, but it comes from arch/arm/lib/board.c, which is
why it is here. I have already mentioned my thoughts on this API on
the list, and maybe this is something that could be tidied up.

>
>> +/*
>> + * Why is gd allocated a register? Prior to reloc it might be better to
>> + * just pass it around to each function in this file?
>
> You're assuming that this is the only file that needs gd.

Not quite - I am wondering whether we should just pass it as an
argument whichever file it is in. I think this was in fact discussed
on the list and it is better to keep it the way it is. I may look at
this later.

>
>> +static int reserve_stacks(void)
>> +{
>> +     /* setup stack pointer for exceptions */
>> +     gd->irq_sp = gd->dest_addr_sp;
>> +#ifdef CONFIG_USE_IRQ
>> +     gd->dest_addr_sp -= (CONFIG_STACKSIZE_IRQ + CONFIG_STACKSIZE_FIQ);
>> +     debug("Reserving %zu Bytes for IRQ stack at: %08lx\n",
>> +             CONFIG_STACKSIZE_IRQ + CONFIG_STACKSIZE_FIQ, gd->dest_addr_sp);
>> +
>> +     /* 8-byte alignment for ARM ABI compliance */
>> +     gd->dest_addr_sp &= ~0x07;
>> +#endif
>> +     /* leave 3 words for abort-stack, plus 1 for alignment */
>> +     gd->dest_addr_sp -= 16;
>> +
>> +     return 0;
>> +}
>
> What does "leave 3 words for abort-stack, plus 1 for alignment" mean in
> a generic context?  Certainly we shouldn't have references to things
> like FIQ or ARM ABI.

This is limited to code which has CONFIG_USE_IRQ in it. Maybe this
function will have to be per-architecture?

>
> Do all architectures U-Boot supports have a stack that grows downward?

So far I have included ARM, x86 and PowerPC. If we add other archs to
generic board init, we will need to look at this.

>
> PowerPC requires 16-byte stack alignment, not 8-byte.

OK I will update this.

>
> -Scott
>

Thanks for reviewing this.

Regards,
Simon