[U-Boot] [PATCH] drivers/net/tsec.c - mii_parse_sr does not wait for auto-negotiation completion bug fix

[Peter Tyser]

Hi Michael,
> Hi Peter,
> 
> Thanks for the reference. I found this thread very interesting.
> 
>     Before I submitted the patch I also wondered if it is possible for
>     the link status to be OK while auto-negotiation has not been completed yet.
>     So I dived into the IEEE-802.3 spec (Clause 28) and figured out that
>     according to the Arbitration state diagram (Figure 28-16) the
> auto-negotiation
>     completed indication is asserted after the link status became OK.
>     It contradicts with Marvell’s 88E1111 phy behavior on my mpc8349 based board
>     where link status OK (register 1.2) always comes at least with 1 tbclk delay
>     after auto-negotiation completion indication (register 1.5).
>     It probably explains why the delay of 500 ms “results in faster booting”
>     as stated in the code - without this delay the CPU leaves the routine with
>     MIIM_STATUS_LINK still being in the down state.
>     By replacing the PHY_BMSR_AUTN_CMP with MIIM_STATUS_LINK in loop exiting
>     condition we eliminate the 500 ms delay.
> 
>     The spec also specifies auto-negotiation timeout in table 28-9.
>     Its worst case value can be calculated as:
> 
>     break_link_timer + autoneg_wait_timer + link_fai_inhibit_timer =
>         1500 ms + 1000 ms + 1000 ms = 3500 ms.

Thanks for the info.  Any chance there's a public link to the IEEE spec
you're referencing?  I've looked for the info you provided relatively
hard, it'd be great to finally lay my eyes on something concrete:)

>     So I reduced the PHY_AUTONEGOTIATE_TIMEOUT to be 4000 ms.
> 
>     Here is the updated patch
> 
> Signed-off-by: Michael Zaidman <michael.zaidman at gmail.com>
> ---
> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
> index 399116f..28e9cc7 100644
> --- a/drivers/net/tsec.c
> +++ b/drivers/net/tsec.c
> @@ -363,12 +363,12 @@ uint mii_parse_sr(uint mii_reg, struct
> tsec_private * priv)
>          * (ie - we're capable and it's not done)
>          */
>         mii_reg = read_phy_reg(priv, MIIM_STATUS);
> -       if ((mii_reg & MIIM_STATUS_LINK) && (mii_reg & PHY_BMSR_AUTN_ABLE)
> +       if (!(mii_reg & MIIM_STATUS_LINK) && (mii_reg & PHY_BMSR_AUTN_ABLE)
>             && !(mii_reg & PHY_BMSR_AUTN_COMP)) {
>                 int i = 0;
> 
>                 puts("Waiting for PHY auto negotiation to complete");
> -               while (!(mii_reg & PHY_BMSR_AUTN_COMP)) {
> +               while (!(mii_reg & MIIM_STATUS_LINK)) {

Won't this be a problem for PHYs which do adhere to the spec you
mentioned above?  If a PHY has achieved link, but auto-negotiation
hasn't completed, we'll return from this function with auto-negotiation
still not complete, which doesn't seem right.  According to the spec, we
shouldn't leave this loop until auto-negotiation is done.

It sounds like the reason that my original patch doesn't work for you is
due to the fact that the 88e1111 PHY doesn't adhere to the spec as far
as the ordering of "link up" and "auto-negotation complete"?  If that's
the case, perhaps we should either wait for both link up and
auto-negotiation to complete, or the 88e1111 (or Marvel PHYs in general)
should have its own mii_parse_sr()?

>                         /*
>                          * Timeout reached ?
>                          */
> @@ -386,7 +386,6 @@ uint mii_parse_sr(uint mii_reg, struct tsec_private * priv)
>                 }
>                 puts(" done\n");
>                 priv->link = 1;
> -               udelay(500000); /* another 500 ms (results in faster booting) */

Agreed that it would be great to get rid of this "magic" delay.

>         } else {
>                 if (mii_reg & MIIM_STATUS_LINK)
>                         priv->link = 1;
> diff --git a/include/tsec.h b/include/tsec.h
> index 7b52e06..ed4c855 100644
> --- a/include/tsec.h
> +++ b/include/tsec.h
> @@ -56,7 +56,7 @@
>  #define TSEC_TIMEOUT 1000
>  #define TOUT_LOOP      1000000
> 
> -#define PHY_AUTONEGOTIATE_TIMEOUT      5000 /* in ms */
> +#define PHY_AUTONEGOTIATE_TIMEOUT      4000 /* in ms */

I'd vote for breaking this out into a separate patch with an explanation
from the IEEE spec you referenced above.

Best,
Peter