[U-Boot] [PATCH] arm:kirkwood Wait for the link to come up on kirkwood network init

[U-Boot] [PATCH] arm:kirkwood Wait for the link to come up on kirkwood network init

[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 <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;
 }
-- 
1.6.0.4

[U-Boot] [PATCH] arm:kirkwood Wait for the link to come up on kirkwood network init

[Ben Warren]

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

[U-Boot] [PATCH] arm:kirkwood Wait for the link to come up on kirkwood network init

[Wolfgang Denk]

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

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

[U-Boot] [PATCH] arm:kirkwood Wait for the link to come up on kirkwood network init

[Ben Warren]

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

[U-Boot] [PATCH] arm:kirkwood Wait for the link to come up on kirkwood network init

[Wolfgang Denk]

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

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

[U-Boot] [PATCH] arm:kirkwood Wait for the link to come up on kirkwood network init

[Simon Kagstrom]

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