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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ1ngBc4SdJbiemQu2znprZ6iwjVRotGMcdVKhdgDSr7tQ at mail.gmail.com> you wrote:
> 
> >> +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.

s/could/should/

This interface is realy ugly crap, and doesn't scale at all.

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

Passing it as arguments will make the code size grow and the code
harder to read.

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

Yes.  Eventually only ARM has this.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The language provides a programmer with a set of conceptual tools; if
these are inadequate for the task, they will simply be  ignored.  For
example, seriously restricting the concept of a pointer simply forces
the  programmer  to use a vector plus integer arithmetic to implement
structures, pointer, etc. Good  design  and  the  absence  of  errors
cannot  be guaranteed by mere language features. - Bjarne Stroustrup,
"The C++ Programming Language"