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

[Joakim Tjernlund]

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