[U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs cpu_eth_init() on AMCC boards
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; }
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
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 =====================================================================
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
(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@denx.de =====================================================================
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
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
participants (3)
-
Ben Warren -
Stefan Roese -
Wolfgang Denk