arm:kirkwood Wait for the link to come up on kirkwood network init
PATCH 1/1 arm:kirkwood Wait for the link to come up on kirkwood network init
Wait for the link to come up on kirkwood network init
This patch makes the device wait for up to 5 seconds for the link to come up, similar to what many of the other network drivers do. This avoids confusing situations where, e.g., a tftp fails when initiated early after U-boot has started (before the link has come up).
--- drivers/net/kirkwood_egiga.c | 17 ++++++++++++----- 1 files changed, 12 insertions(+), 5 deletions(-)
drivers/net/kirkwood_egiga.c +12 -5
@@ -396,6 +396,7 @@ static int kwgbe_init(struct eth_device *dev) {struct kwgbe_device *dkwgbe =to_dkwgbe (dev);struct kwgbe_registers *regs = dkwgbe->regs; +int i;/* setup RX rings */ kwgbe_init_rx_desc_ring (dkwgbe);
@@ -443,12 +444,18 @@ static int kwgbe_init(struct eth_device *dev)#if (defined (CONFIG_MII ) || defined (CONFIG_CMD_MII )) \ && defined (CONFIG_SYS_FAULT_ECHO_LINK_DOWN ) -u16 phyadr; -miiphy_read (dev->name,0xEE ,0xEE , &phyadr); -if (!miiphy_link (dev->name, phyadr)) { -printf ("%s: No link on %s\n" , __FUNCTION__, dev->name); -return -1 ; +/* Wait up to 5s for the link status */ +for (i =0 ; i <5 ; i++) { +u16 phyadr; +miiphy_read (dev->name,0xEE ,0xEE , &phyadr); +/* Return if we get link up */ +if (miiphy_link (dev->name, phyadr)) +return 0 ; +udelay (1000000 ); } + +printf ("%s: No link on %s\n" , __FUNCTION__, dev->name); +return -1 ;#endif return 0 ; }
Signature · 1 line
1.6.0.4
PATCH 1/1 arm:kirkwood Wait for the link to come up on kirkwood network init
Simon,
Quoted 25 lines · Simon Kagstrom
Wait for the link to come up on kirkwood network init
This patch makes the device wait for up to 5 seconds for the link to come up, similar to what many of the other network drivers do. This avoids confusing situations where, e.g., a tftp fails when initiated early after U-boot has started (before the link has come up).
Signed-off-by Simon Kagstrom <[email protected]>--- drivers/net/kirkwood_egiga.c | 17 ++++++++++++----- 1 files changed, 12 insertions(+), 5 deletions(-)drivers/net/kirkwood_egiga.c +1
-0@@ -396,6 +396,7 @@ static int kwgbe_init(struct eth_device *dev) {struct kwgbe_device *dkwgbe =to_dkwgbe (dev);struct kwgbe_registers *regs = dkwgbe->regs; +int i;/* setup RX rings */ kwgbe_init_rx_desc_ring (dkwgbe);
drivers/net/kirkwood_egiga.c static int kwgbe_init(struct eth_device *dev) · line 447
Quoted 8 lines
drivers/net/kirkwood_egiga.c +0
-4@@ -443,12 +444,18 @@ static int kwgbe_init(struct eth_device *dev)#if (defined (CONFIG_MII ) || defined (CONFIG_CMD_MII )) \ && defined (CONFIG_SYS_FAULT_ECHO_LINK_DOWN ) -u16 phyadr; -miiphy_read (dev->name,0xEE ,0xEE , &phyadr); -if (!miiphy_link (dev->name, phyadr)) { -printf ("%s: No link on %s\n" , __FUNCTION__, dev->name);
Please use __func__ instead. It's defined in C99, while __FUNCTION__ isn't (or so I've read)
drivers/net/kirkwood_egiga.c static int kwgbe_init(struct eth_device *dev)
Quoted 5 lines
drivers/net/kirkwood_egiga.c +3
-1-return -1 ; +/* Wait up to 5s for the link status */ +for (i =0 ; i <5 ; i++) { +u16 phyadr;
Please put this variable declaration outside of the 'for' loop
drivers/net/kirkwood_egiga.c static int kwgbe_init(struct eth_device *dev) · line 453
Quoted 2 lines
drivers/net/kirkwood_egiga.c +1
-0+miiphy_read (dev->name,0xEE ,0xEE , &phyadr);
What does '0xEE' mean? I know you didn't write it, but magic numbers are bad.
drivers/net/kirkwood_egiga.c static int kwgbe_init(struct eth_device *dev) · line 461
Quoted 13 lines
drivers/net/kirkwood_egiga.c +7
-0+/* Return if we get link up */ +if (miiphy_link (dev->name, phyadr)) +return 0 ; + udelay(1000000); } + +printf ("%s: No link on %s\n" , __FUNCTION__, dev->name); +return -1 ;#endif return 0 ; }
regards,
Ben
PATCH 1/1 arm:kirkwood Wait for the link to come up on kirkwood network init
Dear Ben Warren,
Quoted 6 lines · In message <[email protected]> you wrote:
drivers/net/kirkwood_egiga.c static int kwgbe_init(struct eth_device *dev) · line 449
Quoted 4 lines
drivers/net/kirkwood_egiga.c +0
-2-if (!miiphy_link (dev->name, phyadr)) { -printf ("%s: No link on %s\n" , __FUNCTION__, dev->name);Please use
__func__instead. It's defined in C99, while__FUNCTION__isn't (or so I've read)
Do we really need the function name at all? This sounds more like a debug message for the developper than a standard error message.
Quoted 5 lines
drivers/net/kirkwood_egiga.c static int kwgbe_init(struct eth_device *dev) · line 449
Quoted 4 lines
drivers/net/kirkwood_egiga.c +3
-0+/* Wait up to 5s for the link status */ +for (i =0 ; i <5 ; i++) { +u16 phyadr;Please put this variable declaration outside of the 'for' loop
Why? If it's only used in this block it's actually a good thing to restrict it to this scope.
Best regards,
Wolfgang Denk
Signature · 6 lines
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
Man is the best computer we can put aboard a spacecraft ... and the
only one that can be mass produced with unskilled labor.
-- Wernher von BraunPATCH 1/1 arm:kirkwood Wait for the link to come up on kirkwood network init
Quoted 3 lines · Wolfgang Denk
Dear Ben Warren,
<snip>
Quoted 9 lines
Quoted 8 lines
drivers/net/kirkwood_egiga.c static int kwgbe_init(struct eth_device *dev) · line 449
Quoted 6 lines
drivers/net/kirkwood_egiga.c +3
-0+/* Wait up to 5s for the link status */ +for (i =0 ; i <5 ; i++) { +u16 phyadr;Please put this variable declaration outside of the 'for' loop
Quoted 4 lines
Why? If it's only used in this block it's actually a good thing to restrict it to this scope.
It seems your coding standards have changed, which everybody knows is impossible. Where's you put the old Wolfgang, doppleganger? :)
Quoted 5 lines
Best regards,
Wolfgang Denk
regards,
Ben
PATCH 1/1 arm:kirkwood Wait for the link to come up on kirkwood network init
Dear Ben Warren,
Quoted 8 lines · In message <[email protected]> you wrote:
Quoted 8 lines
Quoted 7 lines
drivers/net/kirkwood_egiga.c static int kwgbe_init(struct eth_device *dev) · line 449
Quoted 5 lines
drivers/net/kirkwood_egiga.c +3
-0+/* Wait up to 5s for the link status */ +for (i =0 ; i <5 ; i++) { +u16 phyadr;Please put this variable declaration outside of the 'for' loop
Quoted 5 lines
Quoted 3 lines
Why? If it's only used in this block it's actually a good thing to restrict it to this scope.
It seems your coding standards have changed, which everybody knows is impossible. Where's you put the old Wolfgang, doppleganger? :)
Oops? I think you must be wrong. I have always been a friend of declaring variables such that they have minimal scope. Back in the old days when I started to learn C this helped to keep the stack size low.
Did I really ever say anything against such practice?
I'm always fighting against variable declarations mixed in the middle of code, of course, but this here is done right: at the begin of a new block.
Best regards,
Wolfgang Denk
Signature · 6 lines
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 There's an old story about the person who wished his computer were as easy to use as his telephone. That wish has come true, since I no longer know how to use my telephone.
PATCH 1/1 arm:kirkwood Wait for the link to come up on kirkwood network init
In reply to Ben WarrenQuoted 7 lines · On Wed, 19 Aug 2009 10:08:02 -0700 Ben Warren <[email protected]> wrote:
drivers/net/kirkwood_egiga.c static int kwgbe_init(struct eth_device *dev) · line 449
Quoted 5 lines
drivers/net/kirkwood_egiga.c +0
-4-u16 phyadr; -miiphy_read (dev->name,0xEE ,0xEE , &phyadr); -if (!miiphy_link (dev->name, phyadr)) { -printf ("%s: No link on %s\n" , __FUNCTION__, dev->name);Please use
__func__instead. It's defined in C99, while__FUNCTION__isn't (or so I've read)
I'll remove the function name part completely.
Quoted 10 lines
drivers/net/kirkwood_egiga.c static int kwgbe_init(struct eth_device *dev) · line 449
Quoted 5 lines
drivers/net/kirkwood_egiga.c +3
-1-return -1 ; +/* Wait up to 5s for the link status */ +for (i =0 ; i <5 ; i++) { +u16 phyadr;Please put this variable declaration outside of the 'for' loop
drivers/net/kirkwood_egiga.c static int kwgbe_init(struct eth_device *dev) · line 450
Quoted 2 lines
drivers/net/kirkwood_egiga.c +1
-0+miiphy_read (dev->name,0xEE ,0xEE , &phyadr);What does '
0xEE' mean? I know you didn't write it, but magic numbers are bad.
Good question. After looking around a bit, I end up in smi_reg_read in the same file:
static int smi_reg_read(char *devname, u8 phy_adr, u8 reg_ofs, u16 * data)
{
[...]
/* Phyadr read request */
if (phy_adr == 0xEE && reg_ofs == 0xEE) {
/* */
*data = (u16) (KWGBEREG_RD(regs->phyadr) & PHYADR_MASK);
return 0;
[...]}
which is registered for the PHY reads with miiphy_register. So it's a file-local magic number. I'll cook up another patch which adresses this.
// Simon