Hi Scott,
On Thu, Mar 15, 2012 at 12:09 PM, Scott Wood scottwood@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