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

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

[Stefan Roese]

Some AMCC eval boards do have a board_eth_init() function calling
pci_eth_init(). These boards need to call cpu_eth_init() explicitly now
with the new eth_init rework.

Signed-off-by: Stefan Roese <sr@denx.de>
---
 board/amcc/katmai/katmai.c   |    8 +++++++-
 board/amcc/taihu/taihu.c     |    8 +++++++-
 board/amcc/taishan/taishan.c |    8 +++++++-
 board/amcc/yucca/yucca.c     |    8 +++++++-
 4 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/board/amcc/katmai/katmai.c b/board/amcc/katmai/katmai.c
index b6c0c11..cace93f 100644
--- a/board/amcc/katmai/katmai.c
+++ b/board/amcc/katmai/katmai.c
@@ -451,5 +451,11 @@ int post_hotkeys_pressed(void)
 
 int board_eth_init(bd_t *bis)
 {
-	return pci_eth_init(bis);
+	cpu_eth_init(bis);
+	pci_eth_init(bis);
+
+	/*
+	 * Return 0 so that cpu_eth_init() won't get executed again
+	 */
+	return 0;
 }
diff --git a/board/amcc/taihu/taihu.c b/board/amcc/taihu/taihu.c
index 5224378..8e74b3c 100644
--- a/board/amcc/taihu/taihu.c
+++ b/board/amcc/taihu/taihu.c
@@ -195,5 +195,11 @@ int pci_pre_init(struct pci_controller *hose)
 
 int board_eth_init(bd_t *bis)
 {
-	return pci_eth_init(bis);
+	cpu_eth_init(bis);
+	pci_eth_init(bis);
+
+	/*
+	 * Return 0 so that cpu_eth_init() won't get executed again
+	 */
+	return 0;
 }
diff --git a/board/amcc/taishan/taishan.c b/board/amcc/taishan/taishan.c
index 28bdab5..0b5cdcc 100644
--- a/board/amcc/taishan/taishan.c
+++ b/board/amcc/taishan/taishan.c
@@ -315,5 +315,11 @@ int post_hotkeys_pressed(void)
 
 int board_eth_init(bd_t *bis)
 {
-	return pci_eth_init(bis);
+	cpu_eth_init(bis);
+	pci_eth_init(bis);
+
+	/*
+	 * Return 0 so that cpu_eth_init() won't get executed again
+	 */
+	return 0;
 }
diff --git a/board/amcc/yucca/yucca.c b/board/amcc/yucca/yucca.c
index c805568..6036b29 100644
--- a/board/amcc/yucca/yucca.c
+++ b/board/amcc/yucca/yucca.c
@@ -956,5 +956,11 @@ int onboard_pci_arbiter_selected(int core_pci)
 
 int board_eth_init(bd_t *bis)
 {
-	return pci_eth_init(bis);
+	cpu_eth_init(bis);
+	pci_eth_init(bis);
+
+	/*
+	 * Return 0 so that cpu_eth_init() won't get executed again
+	 */
+	return 0;
 }
-- 
1.6.1.3

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

[Wolfgang Denk]

Dear Stefan Roese,

In message <1234342325-28950-1-git-send-email-sr@denx.de> you wrote:
> Some AMCC eval boards do have a board_eth_init() function calling
> pci_eth_init(). These boards need to call cpu_eth_init() explicitly now
> with the new eth_init rework.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> ---
>  board/amcc/katmai/katmai.c   |    8 +++++++-
>  board/amcc/taihu/taihu.c     |    8 +++++++-
>  board/amcc/taishan/taishan.c |    8 +++++++-
>  board/amcc/yucca/yucca.c     |    8 +++++++-
>  4 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/board/amcc/katmai/katmai.c b/board/amcc/katmai/katmai.c
> index b6c0c11..cace93f 100644
> --- a/board/amcc/katmai/katmai.c
> +++ b/board/amcc/katmai/katmai.c
> @@ -451,5 +451,11 @@ int post_hotkeys_pressed(void)
>  
>  int board_eth_init(bd_t *bis)
>  {
> -	return pci_eth_init(bis);
> +	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.

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
Generally speaking, there are other ways to accomplish whatever it is
that you think you need ...                               - Doug Gwyn

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

[Stefan Roese]

On Wednesday 11 February 2009, Wolfgang Denk wrote:
> > +++ b/board/amcc/katmai/katmai.c
> > @@ -451,5 +451,11 @@ int post_hotkeys_pressed(void)
> >
> >  int board_eth_init(bd_t *bis)
> >  {
> > -	return pci_eth_init(bis);
> > +	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:

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.

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@denx.de
=====================================================================

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

[Wolfgang Denk]

Dear Stefan,

In message <200902111352.04598.sr@denx.de> you 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.

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

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.

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
"Well I don't see why I have to make one man  miserable  when  I  can
make so many men happy."              - Ellyn Mustard, about marriage

[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
=====================================================================

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

[Ben Warren]

Hi Stefan/Wolfgang,

On Wed, Feb 11, 2009 at 5:02 AM, Wolfgang Denk <wd@denx.de> wrote:

> Dear Stefan,
>
> In message <200902111352.04598.sr@denx.de> you 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.
> >
>
I think this is right, although the undocumented return code of 0 may not be
(more on this later in this e-mail)

>
> > 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.
>
What handling should there be in the case of a device initialization error?
Drivers currently printf something and don't register the device.  What else
is appropriate?

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

Sorry this is confusing. The -1 return code currently means "does not
exist", although there are a few individual drivers that return -1 on
failure, and should be cleaned up.  All of the PCI drivers return the number
of interfaces found (i.e. return 0 if nothing is found or there's an
error).  While this flaunts UNIX convention, it has the advantage of letting
us count the interfaces at initialization without stepping through the
list.  I was thinking of taking advantage of this to get rid of the
CONFIG_NET_MULTI thing some time in the future.

>
> Best regards,
>
> Wolfgang Denk
>

regards,
Ben

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

[Wolfgang Denk]

Dear Stefan Roese,

In message <200902111436.38210.sr@denx.de> you wrote:
> 
> 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?

I don;t know. I just see that pci_eth_init() is declared to return
"int". If the return value is not used, we should make the function
void.

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

I don't understand the intended logic. As far as I can see,
pci_eth_init() will return the number of initialized boards, i. e. a
non-negative value. Then having board_eth_init() reuturn the same
return code should be no problem - you get -1 (i. e. < 0) if
board_eth_init() does not exist, and >= 0 else.

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
No journaling file system can recover your data if the disk dies.
                                 - Steve Rago in <D4Cw1p.L9E@plc.com>