* [U-Boot] [PATCH] drivers/net/tsec.c - mii_parse_sr does not wait for auto-negotiation completion bug fix @ 2009-03-24 17:57 Michael Zaidman 2009-03-24 19:01 ` Peter Tyser 0 siblings, 1 reply; 5+ messages in thread From: Michael Zaidman @ 2009-03-24 17:57 UTC (permalink / raw) To: u-boot drivers/net/tsec.c - mii_parse_sr does not wait for auto-negotiation completion bug fix In the case when the MIIM_STATUS_LINK is 0 i.e. link is down and this is the situation immediately after power up, the code of awaiting for auto-negotiation completion now will be executed. Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com> --- drivers/net/tsec.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index 399116f..54279ca 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -363,7 +363,7 @@ 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; ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] drivers/net/tsec.c - mii_parse_sr does not wait for auto-negotiation completion bug fix 2009-03-24 17:57 [U-Boot] [PATCH] drivers/net/tsec.c - mii_parse_sr does not wait for auto-negotiation completion bug fix Michael Zaidman @ 2009-03-24 19:01 ` Peter Tyser 2009-03-26 15:21 ` Michael Zaidman 0 siblings, 1 reply; 5+ messages in thread From: Peter Tyser @ 2009-03-24 19:01 UTC (permalink / raw) To: u-boot Hi Michael, On Tue, 2009-03-24 at 19:57 +0200, Michael Zaidman wrote: > drivers/net/tsec.c - mii_parse_sr does not wait for auto-negotiation > completion bug fix > > In the case when the MIIM_STATUS_LINK is 0 i.e. link is down > and this is the situation immediately after power up, > the code of awaiting for auto-negotiation completion now will be executed. > > Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com> > --- > drivers/net/tsec.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c > index 399116f..54279ca 100644 > --- a/drivers/net/tsec.c > +++ b/drivers/net/tsec.c > @@ -363,7 +363,7 @@ 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; This issue has been brought up once before: http://lists.denx.de/pipermail/u-boot/2009-February/046829.html There was some concern about adding delay to the boot process in the case that the primary ethernet port(s) did not achieve link. There had also been some discussion about performing PHY auto-negotiation in parallel, but this seems like a much larger can of worms. I had proposed this: http://lists.denx.de/pipermail/u-boot/2009-February/048489.html Andy/Ben, any feedback on getting a patch such as Michael's or mine merged in? Thanks, Peter ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] drivers/net/tsec.c - mii_parse_sr does not wait for auto-negotiation completion bug fix 2009-03-24 19:01 ` Peter Tyser @ 2009-03-26 15:21 ` Michael Zaidman 2009-03-26 16:26 ` Peter Tyser 0 siblings, 1 reply; 5+ messages in thread From: Michael Zaidman @ 2009-03-26 15:21 UTC (permalink / raw) To: u-boot On Tue, Mar 24, 2009 at 9:01 PM, Peter Tyser <ptyser@xes-inc.com> wrote: > Hi Michael, > > On Tue, 2009-03-24 at 19:57 +0200, Michael Zaidman wrote: >> drivers/net/tsec.c - mii_parse_sr does not wait for auto-negotiation >> completion bug fix >> >> In the case when the MIIM_STATUS_LINK is 0 i.e. link is down >> and this is the situation immediately after power up, >> the code of awaiting for auto-negotiation completion now will be executed. >> >> Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com> >> --- >> ?drivers/net/tsec.c | ? ?2 +- >> ?1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c >> index 399116f..54279ca 100644 >> --- a/drivers/net/tsec.c >> +++ b/drivers/net/tsec.c >> @@ -363,7 +363,7 @@ 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; > > This issue has been brought up once before: > http://lists.denx.de/pipermail/u-boot/2009-February/046829.html > > There was some concern about adding delay to the boot process in the > case that the primary ethernet port(s) did not achieve link. > > There had also been some discussion about performing PHY > auto-negotiation in parallel, but this seems like a much larger can of > worms. > > I had proposed this: > http://lists.denx.de/pipermail/u-boot/2009-February/048489.html > > Andy/Ben, any feedback on getting a patch such as Michael's or mine > merged in? > > Thanks, > Peter > > 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. So I reduced the PHY_AUTONEGOTIATE_TIMEOUT to be 4000 ms. Here is the updated patch Signed-off-by: Michael Zaidman <michael.zaidman@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)) { /* * 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) */ } 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 */ /* TBI register addresses */ #define TBI_CR 0x00 Thanks, Michael ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] drivers/net/tsec.c - mii_parse_sr does not wait for auto-negotiation completion bug fix 2009-03-26 15:21 ` Michael Zaidman @ 2009-03-26 16:26 ` Peter Tyser 2009-03-26 22:48 ` Michael Zaidman 0 siblings, 1 reply; 5+ messages in thread From: Peter Tyser @ 2009-03-26 16:26 UTC (permalink / raw) To: u-boot 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@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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] drivers/net/tsec.c - mii_parse_sr does not wait for auto-negotiation completion bug fix 2009-03-26 16:26 ` Peter Tyser @ 2009-03-26 22:48 ` Michael Zaidman 0 siblings, 0 replies; 5+ messages in thread From: Michael Zaidman @ 2009-03-26 22:48 UTC (permalink / raw) To: u-boot Hi Peter, On Thu, Mar 26, 2009 at 6:26 PM, Peter Tyser <ptyser@xes-inc.com> wrote: > 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:) Sure, you can download it from http://standards.ieee.org/getieee802/802.3.html All auto-negotiation stuff is located in the "IEEE 802.3-2005 -- Section Two" > >> ? ? So I reduced the PHY_AUTONEGOTIATE_TIMEOUT to be 4000 ms. >> >> ? ? Here is the updated patch >> >> Signed-off-by: Michael Zaidman <michael.zaidman@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. I am not sure if this is the possible situation. As I understand the auto-negotiation process is prerequisite for the link state assignment. And generally both indications should come closely coupled. Probably I miss-interpreted the link_status primitive in the state diagram. It gets three values - FAIL, READY and OK, whereas the Link status bit in the Status register (1.2) gets UP and Down. And timing relationships between them are unclear. (See Figure 24?15 Link Monitor state diagram) > ?According to the spec, we > shouldn't leave this loop until auto-negotiation is done. We are interesting in the link status. This is what the routine returns. The link status UP indicates that a valid link is established and reliable reception of signals transmitted from the remote PHY is possible. > > 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"? I assume it would work also. I just did not try it because I was not aware about it. :-) > 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()? It will be interesting to know an assertion order of link up and auto-negotiation to complete for another PHY. > >> ? ? ? ? ? ? ? ? ? ? ? ? /* >> ? ? ? ? ? ? ? ? ? ? ? ? ?* 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. Agree. > > Best, > Peter > > > Thanks, Michael ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-03-26 22:48 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-03-24 17:57 [U-Boot] [PATCH] drivers/net/tsec.c - mii_parse_sr does not wait for auto-negotiation completion bug fix Michael Zaidman 2009-03-24 19:01 ` Peter Tyser 2009-03-26 15:21 ` Michael Zaidman 2009-03-26 16:26 ` Peter Tyser 2009-03-26 22:48 ` Michael Zaidman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox