[U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs cpu_eth_init() on AMCC boards

[Stefan Roese]

(Added Ben to CC)

On Wednesday 11 February 2009, Wolfgang Denk wrote:
> > > > +	cpu_eth_init(bis);
> > > > +	pci_eth_init(bis);
> > > > +
> > > > +	/*
> > > > +	 * Return 0 so that cpu_eth_init() won't get executed again
> > > > +	 */
> > > > +	return 0;
> > >
> > > What happens in case of errors? This looks broken to me, or I
> > > misinderstand the comment.
> >
> > This is the code calling board_eth_init() from net/eth.c:
> >
> >         /* Try board-specific initialization first.  If it fails or isn't
> >          * present, try the cpu-specific initialization */
> >         if (board_eth_init(bis) < 0)
> >                 cpu_eth_init(bis);
> >
> > So if we return with an error in board_eth_init(), cpu_eth_init() will
> > get called again. Another way to fix this problem would be this
> > implementation:
>
> I consider this a buggy design that should be fixed. It should be
> possible to handle the situation that pci_eth_init() returns an error
> code.

pci_eth_init() is called to add additional *optional* network interfaces. 
Since PCI boards may or may not exist, I think that a non existant PCI 
ethernet device should not result in an error.

What sort of error handling do you have in mind here?

> > board_eth_init()
> > {
> > 	pci_eth_init(bis);
> >
> > 	/*
> > 	 * Return -1 so that cpu_eth_init() will get executed in net/eth.c
> > 	 */
> > 	return -1;
> > }
> >
> > But I like the former implementation better, since the cpu internal
> > ethernet interfaces are added first to the network devices list.
>
> That would be as bad as the previous solution, or actually worse as it
> looks as if board_eth_init() was always failing.

Right. That's also one reason why I implemented the first version.

> I think the key problems is here:
> >         /* Try board-specific initialization first.  If it fails or isn't
> >          * present, try the cpu-specific initialization */
> >         if (board_eth_init(bis) < 0)
> >                 cpu_eth_init(bis);
>
> I think we must differentiate between board_eth_init() not existing
> and a failure in board_eth_init(); these are two very different
> situations.

board_eth_init() not existing is the default. We have a weak implementation 
for board_eth_init() in eth.c:

/*
 * CPU and board-specific Ethernet initializations.  Aliased function
 * signals caller to move on
 */
static int __def_eth_init(bd_t *bis)
{
        return -1;
}
int cpu_eth_init(bd_t *bis) __attribute((weak, alias("__def_eth_init")));
int board_eth_init(bd_t *bis) __attribute((weak, alias("__def_eth_init")));


What change do you have in mind here?

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================