* [U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs cpu_eth_init() on AMCC boards
@ 2009-02-11 8:52 Stefan Roese
2009-02-11 12:19 ` Wolfgang Denk
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Roese @ 2009-02-11 8:52 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs cpu_eth_init() on AMCC boards
2009-02-11 8:52 [U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs cpu_eth_init() on AMCC boards Stefan Roese
@ 2009-02-11 12:19 ` Wolfgang Denk
2009-02-11 12:52 ` Stefan Roese
0 siblings, 1 reply; 7+ messages in thread
From: Wolfgang Denk @ 2009-02-11 12:19 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs cpu_eth_init() on AMCC boards
2009-02-11 12:19 ` Wolfgang Denk
@ 2009-02-11 12:52 ` Stefan Roese
2009-02-11 13:02 ` Wolfgang Denk
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Roese @ 2009-02-11 12:52 UTC (permalink / raw)
To: u-boot
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
=====================================================================
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs cpu_eth_init() on AMCC boards
2009-02-11 12:52 ` Stefan Roese
@ 2009-02-11 13:02 ` Wolfgang Denk
2009-02-11 13:36 ` Stefan Roese
2009-02-11 14:53 ` Ben Warren
0 siblings, 2 replies; 7+ messages in thread
From: Wolfgang Denk @ 2009-02-11 13:02 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs cpu_eth_init() on AMCC boards
2009-02-11 13:02 ` Wolfgang Denk
@ 2009-02-11 13:36 ` Stefan Roese
2009-02-11 16:22 ` Wolfgang Denk
2009-02-11 14:53 ` Ben Warren
1 sibling, 1 reply; 7+ messages in thread
From: Stefan Roese @ 2009-02-11 13:36 UTC (permalink / raw)
To: u-boot
(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
=====================================================================
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs cpu_eth_init() on AMCC boards
2009-02-11 13:02 ` Wolfgang Denk
2009-02-11 13:36 ` Stefan Roese
@ 2009-02-11 14:53 ` Ben Warren
1 sibling, 0 replies; 7+ messages in thread
From: Ben Warren @ 2009-02-11 14:53 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs cpu_eth_init() on AMCC boards
2009-02-11 13:36 ` Stefan Roese
@ 2009-02-11 16:22 ` Wolfgang Denk
0 siblings, 0 replies; 7+ messages in thread
From: Wolfgang Denk @ 2009-02-11 16:22 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-02-11 16:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-02-11 8:52 [U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs cpu_eth_init() on AMCC boards Stefan Roese
2009-02-11 12:19 ` Wolfgang Denk
2009-02-11 12:52 ` Stefan Roese
2009-02-11 13:02 ` Wolfgang Denk
2009-02-11 13:36 ` Stefan Roese
2009-02-11 16:22 ` Wolfgang Denk
2009-02-11 14:53 ` Ben Warren
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.