[U-Boot-Users] [RFC][PATCH 1/1] Add board_eth_init() function

Joakim Tjernlund
2008-03-25T16:59:32Z

On Tue, 2008-03-25 at 11:56 -0400, Jerry Van Baren wrote:
> Joakim Tjernlund wrote:
> > On Tue, 2008-03-25 at 10:57 -0400, Ben Warren wrote:
> >> Joakim Tjernlund wrote:
> >>> On Tue, 2008-03-25 at 10:22 -0400, Ben Warren wrote:
> >>>
> >>>> Stefan Roese wrote:
> >>>>
> >>>>> On Saturday 22 March 2008, Ben Warren wrote:
>
> [snip]
>
> >>>>> Using Markus's idea, why not use a cpu (platform) specific *and* a board
> >>>>> specific init function, both with an empty weak alias in the common eth.c
> >>>>> code:
> >>>>>
> >>>>> cpu_eth_init(bis);
> >>>>> board_eth_init(bis);
> >>>>>
> >>>> I thought about this some more, and the problem is that cpu_eth_init() and board_eth_init()
> >>>> are mutually exclusive, with board_eth_init() having a higher priority.
> >>>> I think the following will work, but would appreciate some feedback.
> >>>>
> >>>> -----
> >>>>
> >>>> int board_eth_init(bd_t *bis) __attribute(weak);
> >>>> int cpu_eth_init(bd_t *bis) __attribute(weak);
> >>>>
> >>>> .
> >>>> .
> >>>> .
> >>>>
> >>>> if (board_eth_init)
> >>>> board_eth_init(bis);
> >>>> else if (cpu_eth_init)
> >>>> cpu_eth_init(bis);
> >>>>
> >>>> -----
> >>>>
> >>>> This gets rid of the pointless aliases and gives precedence to the board-specific initialization.
> >>>>
> >>>>
> >>>> regards,
> >>>> Ben
> >>>>
> >>> I think you must enable full relocation, CONFIG_RELOC_FIXUP_WORKS, to
> >>> make the "if (board_eth_init)" work. This is just a guess though.
> >>>
> >>> Jocke
> >>>
> >>>
> >> Nothing a little testing can't figure out.
> >>
> >> thanks,
> >> Ben
> >
> > You could do too:
> > if (!board_eth_init(bis))
> > cpu_eth_init(bis);
> >
> > Jocke
>
> Per an earlier discussion on how weak functions are implemented, these
> are not equivalent. Functions marked "weak" *without* a weak
> implementation become NULL pointers. The code
> if (board_eth_init)
> board_eth_init(bis);
> else if (cpu_eth_init)
> cpu_eth_init(bis);
> uses that knowledge to see if the weak function board_eth_init() exists
> and then calls it if it does. If it doesn't exist, it sees if
> cpu_eth_init() exists and calls it if it does.
>
> Your counter proposal assumes that a weak function board_eth_init()
> *does* exist and uses the returned result as the condition of executing
> cpu_eth_init() (assuming it also exists).
>
> If you define a weak function that simply returns failure, your
> alternative is close, but still not the same because an overridden
> (*real*) board_eth_init() could return failure too, in which case it

Yes, see below for fix

>
> will (probably erroneously) execute cpu_eth_init(). Beyond that, if
> cpu_eth_init() doesn't exist (doesn't have a default weak function
> defined), the call to it will go *SPLAT*.
>
> HTH,
> gvb
>

All true, I was thinking that there would be default weak dummy versions
of both board_eth_init() and cpu_eth_init() that would do the
right thing. To address the weakness you spotted one would need three
return values, something like:
-1 fail
0 not impl.
1 success

Jocke