[U-Boot] [PATCH] 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).
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net --- drivers/net/kirkwood_egiga.c | 17 ++++++++++++----- 1 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c index f31fefc..9ac9c1f 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -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; }
Simon,
Simon Kagstrom wrote:
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 simon.kagstrom@netinsight.net
drivers/net/kirkwood_egiga.c | 17 ++++++++++++----- 1 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c index f31fefc..9ac9c1f 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -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);
Please use __func__ instead. It's defined in C99, while __FUNCTION__ isn't (or so I've read)
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
miiphy_read(dev->name, 0xEE, 0xEE, &phyadr);
What does '0xEE' mean? I know you didn't write it, but magic numbers are bad.
/* 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
Dear Ben Warren,
In message 4A8C3172.70001@gmail.com you wrote:
- 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.
- /* 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
Wolfgang Denk wrote:
Dear Ben Warren,
<snip>
- /* 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.
It seems your coding standards have changed, which everybody knows is impossible. Where's you put the old Wolfgang, doppleganger? :)
Best regards,
Wolfgang Denk
regards, Ben
Dear Ben Warren,
In message 4A8C5288.5020308@gmail.com you wrote:
- /* 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.
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
On Wed, 19 Aug 2009 10:08:02 -0700 Ben Warren biggerbadderben@gmail.com wrote:
- 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.
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
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
participants (3)
-
Ben Warren -
Simon Kagstrom -
Wolfgang Denk