Re: [PATCH net] at803x: insure minimum delay for SGMII link AN completion ckeck
On Tue, Feb 14, 2017 at 02:31:53PM +, Claudiu Manoil wrote: > >-Original Message- > >From: Andrew Lunn [mailto:and...@lunn.ch] > >Sent: Monday, February 13, 2017 7:31 PM > >Subject: Re: [PATCH net] at803x: insure minimum delay for SGMII link AN > >completion ckeck > > > > [...] > > >> > >> I can confirm that link status changes are signaled via interrupts > >> ("phy_interrupt") > >> in this case. > > > >Is there a way to enable an interrupt when SGMII signalled link > >changes has happened? Maybe another interrupt enable bit somewhere? > >That would avoid having to sleep(). > > > > No, except for the interrupt lines coming from the external AR8033 PHYs > there's > nothing else (documentation on the SoC and board is available on nxp.com). > I think this question hints to the actual problem, that the internal SGMII > link > should have a separate state (and state machine) apart from the external > link's > state. Yes. I think this is something Russell Kings phylink patchset tried to address. He has something similar. SGMII link to an SFP cage, which could have a copper module plugged in. So again, multiple PHYs in a chain. Andrew
RE: [PATCH net] at803x: insure minimum delay for SGMII link AN completion ckeck
>-Original Message- >From: Andrew Lunn [mailto:and...@lunn.ch] >Sent: Monday, February 13, 2017 7:31 PM >Subject: Re: [PATCH net] at803x: insure minimum delay for SGMII link AN >completion ckeck > [...] >> >> I can confirm that link status changes are signaled via interrupts >> ("phy_interrupt") >> in this case. > >Is there a way to enable an interrupt when SGMII signalled link >changes has happened? Maybe another interrupt enable bit somewhere? >That would avoid having to sleep(). > No, except for the interrupt lines coming from the external AR8033 PHYs there's nothing else (documentation on the SoC and board is available on nxp.com). I think this question hints to the actual problem, that the internal SGMII link should have a separate state (and state machine) apart from the external link's state. PHYLIB currently handles the state of the external link of a PHY, hence the aneg_done() hook should reflect the status of the external link. So, I think there'll always be issues if the external PHY device's aneg_done() returns the AN status of the internal SGMII link. Actually, the internal link is owned by an internal (SoC) PHY device, TBI in this case (or something similar for other SoCs I'd expect), so, by a different phy device. Moreover, the auto-negotiation on the external link is different from the AN on the internal SGMII link (different IEEE clauses: clause 27 vs clause 37). So, I think the way to go would be to drop the SGMII link state check from AR803x's aneg_done() for now and try to address SGMII internal link potential issues at SoC/MAC level, on a case by case basis. Claudiu
Re: [PATCH net] at803x: insure minimum delay for SGMII link AN completion ckeck
> Yes, the phy is operating in interrupt mode. > The phy nodes from the board's device tree have their interrupt properties > set: > http://lxr.free-electrons.com/source/arch/powerpc/boot/dts/fsl/p1010rdb-pb.dts > > I can confirm that link status changes are signaled via interrupts > ("phy_interrupt") > in this case. Is there a way to enable an interrupt when SGMII signalled link changes has happened? Maybe another interrupt enable bit somewhere? That would avoid having to sleep(). Andrew
Re: [PATCH net] at803x: insure minimum delay for SGMII link AN completion ckeck
On 02/13/2017 02:15 PM, Claudiu Manoil wrote: >> -Original Message- >> From: Zefir Kurtisi [mailto:zefir.kurt...@neratec.com] >> Sent: Monday, February 13, 2017 12:16 PM >> To: Claudiu Manoil <claudiu.man...@nxp.com> >> Cc: netdev@vger.kernel.org; David S. Miller <da...@davemloft.net>; Florian >> Fainelli <f.faine...@gmail.com> >> Subject: Re: [PATCH net] at803x: insure minimum delay for SGMII link AN >> completion ckeck >> >> On 02/10/2017 05:42 PM, Claudiu Manoil wrote: >>> Commit: f62265b "at803x: double check SGMII side autoneg" >>> introduced a regression for the p1010rdb board which has >>> two of the ethernet controllers (eTSEC) connected through >>> SGMII links to external Atheros SGMII AR8033 PHYs. >>> The issue consists in a dead link for these ports, and is >>> 100% reproducible on kernel 4.9 (and later): >>> > > [...] > >>> >> >> Could you confirm that you are using PHY_HAS_INTERRUPT? In polling mode the >> effect would be very unlikely to happen, since you'd need to run the state >> machine >> exactly between the two AN stages. >> > > Hi Zefir. Thanks for having a look at this issue. > Yes, the phy is operating in interrupt mode. > The phy nodes from the board's device tree have their interrupt properties > set: > http://lxr.free-electrons.com/source/arch/powerpc/boot/dts/fsl/p1010rdb-pb.dts > > I can confirm that link status changes are signaled via interrupts > ("phy_interrupt") > in this case. And this is always desirable, right? Why would one want to > waste CPU > cycles by polling for link status changes periodically, if it can work with > interrupts. > Sure, only wanted to double check, since we are polling and with that never run (or at least never observed) into a situation where a first read fails but subsequent ones succeed. >> As for the 100us delay proposed, is this something Atheros suggested or is it >> based on empirical considerations? Since ending up in a situation where the >> double-check fails might left you with a permanent link loss, having a >> reliable >> minimum required delay between first and second stage AN is essential - to me >> 100us look quite short. >> > > The value was chosen based on experiments, so yes, it's empirical. I don't > think > that detailed documentation for this phy is publicly available. I was > expecting a > small delay and I was looking for the smallest power of 10 (10 us doesn't > work). > But I'm also expecting that the SGMII specification is imposing a minimum > delay > between AN completion on the copper side and AN completion on the SGMII side. > My bad, I assumed NXP is already part of Qualcomm and with that you had better access to Atheros internal documentation than the typical open-source developer - realized the merger is yet to come. >> Same goes for the readout polling proposed: given that reading an MDIO >> register >> takes ~16us (at assumed 2.5MHz MDC), delaying for 1us between reads is kind >> of >> useless and ends up in storm-reading the register (and also extends the >> wait-time >> by the same factor). Imo, it won't hurt to sleep for milliseconds between >> reads >> here (see phy_poll_reset() for reference). >> > > I'm not fond of using udelay either, I'm also aware that the timeout loop is > not > exactly 100 us, given the delay involved by the phy register reads, but I > wanted > to emphasize that there needs to be a minimum delay before establishing that > the SGMII AN is done. > Would you mind sending a v2 patch using msleep() instead of udelay()? > You have an idea now of the minimum delay needed in my case. > Would make sense to me. It won't hurt to provide a significantly higher wait time (e.g. 50ms) and sleep for 4ms between register reads. Since polling happens only in the failure case, it is cheap for the typical one. @Florian: sleeping in .aneg_done is ok, right? > Thanks, > Claudiu > Thanks and great to hear that you are working with P1010 - that's maybe the root cause for the workaround we are dealing with here, since the eTSEC has a known problem with stalling SGMII link (errata A-004187). At least one more party that might observe the issue.
RE: [PATCH net] at803x: insure minimum delay for SGMII link AN completion ckeck
>-Original Message- >From: Zefir Kurtisi [mailto:zefir.kurt...@neratec.com] >Sent: Monday, February 13, 2017 12:16 PM >To: Claudiu Manoil <claudiu.man...@nxp.com> >Cc: netdev@vger.kernel.org; David S. Miller <da...@davemloft.net>; Florian >Fainelli <f.faine...@gmail.com> >Subject: Re: [PATCH net] at803x: insure minimum delay for SGMII link AN >completion ckeck > >On 02/10/2017 05:42 PM, Claudiu Manoil wrote: >> Commit: f62265b "at803x: double check SGMII side autoneg" >> introduced a regression for the p1010rdb board which has >> two of the ethernet controllers (eTSEC) connected through >> SGMII links to external Atheros SGMII AR8033 PHYs. >> The issue consists in a dead link for these ports, and is >> 100% reproducible on kernel 4.9 (and later): >> [...] >> > >Could you confirm that you are using PHY_HAS_INTERRUPT? In polling mode the >effect would be very unlikely to happen, since you'd need to run the state >machine >exactly between the two AN stages. > Hi Zefir. Thanks for having a look at this issue. Yes, the phy is operating in interrupt mode. The phy nodes from the board's device tree have their interrupt properties set: http://lxr.free-electrons.com/source/arch/powerpc/boot/dts/fsl/p1010rdb-pb.dts I can confirm that link status changes are signaled via interrupts ("phy_interrupt") in this case. And this is always desirable, right? Why would one want to waste CPU cycles by polling for link status changes periodically, if it can work with interrupts. >As for the 100us delay proposed, is this something Atheros suggested or is it >based on empirical considerations? Since ending up in a situation where the >double-check fails might left you with a permanent link loss, having a reliable >minimum required delay between first and second stage AN is essential - to me >100us look quite short. > The value was chosen based on experiments, so yes, it's empirical. I don't think that detailed documentation for this phy is publicly available. I was expecting a small delay and I was looking for the smallest power of 10 (10 us doesn't work). But I'm also expecting that the SGMII specification is imposing a minimum delay between AN completion on the copper side and AN completion on the SGMII side. >Same goes for the readout polling proposed: given that reading an MDIO register >takes ~16us (at assumed 2.5MHz MDC), delaying for 1us between reads is kind of >useless and ends up in storm-reading the register (and also extends the >wait-time >by the same factor). Imo, it won't hurt to sleep for milliseconds between reads >here (see phy_poll_reset() for reference). > I'm not fond of using udelay either, I'm also aware that the timeout loop is not exactly 100 us, given the delay involved by the phy register reads, but I wanted to emphasize that there needs to be a minimum delay before establishing that the SGMII AN is done. Would you mind sending a v2 patch using msleep() instead of udelay()? You have an idea now of the minimum delay needed in my case. Thanks, Claudiu
Re: [PATCH net] at803x: insure minimum delay for SGMII link AN completion ckeck
On 02/10/2017 05:42 PM, Claudiu Manoil wrote: > Commit: f62265b "at803x: double check SGMII side autoneg" > introduced a regression for the p1010rdb board which has > two of the ethernet controllers (eTSEC) connected through > SGMII links to external Atheros SGMII AR8033 PHYs. > The issue consists in a dead link for these ports, and is > 100% reproducible on kernel 4.9 (and later): > > root@p1010rdb-pb:~# ifconfig eth2 172.16.1.1 > [ 203.274263] IPv6: ADDRCONF(NETDEV_UP): eth2: link is not ready > root@p1010rdb-pb:~# [ 206.408255] 803x_aneg_done: SGMII link is not ok > > root@p1010rdb-pb:~# ethtool eth2 > Settings for eth2: > Supported ports: [ MII ] > Supported link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > 1000baseT/Full > Supported pause frame use: Symmetric Receive-only > Supports auto-negotiation: Yes > Advertised link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > 1000baseT/Full > Advertised pause frame use: No > Advertised auto-negotiation: Yes > Link partner advertised link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > 1000baseT/Half 1000baseT/Full > Link partner advertised pause frame use: Symmetric Receive-only > Link partner advertised auto-negotiation: Yes > Speed: 1000Mb/s > Duplex: Full > Port: MII > PHYAD: 2 > Transceiver: internal > Auto-negotiation: on > Supports Wake-on: g > Wake-on: d > Current message level: 0x003f (63) >drv probe link timer ifdown ifup > Link detected: no > > Insuring up to 100 usecs for the SGMII link side AN to complete > proves to be enough to have a working SGMII link, for this board. > The need for a delay for the SGMII link side may be explained by > the fact that there are two levels of auto-negotiation (AN) for a > SGMII link. First the PHY autonegotiates the link parameters w/ > its link partner over the copper link. In the second stage, the > AN results are then passed to the eTSEC MAC over the SGMII link > using the Clause 37 auto-negotiation functionality. While the > aneg_done() hook is called by the phylib state machine to check > for the completion of the 1st stage AN of the external PHY, > there's no mechanism to insure proper AN completion of the internal > SGMII link (which is actually handled on the eTSEC side by a > "internal PHY", called TBI). > > Fixes: f62265b "at803x: double check SGMII side autoneg" > > Signed-off-by: Claudiu Manoil> --- > drivers/net/phy/at803x.c | 9 - > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > index a52b560..55fa7c4 100644 > --- a/drivers/net/phy/at803x.c > +++ b/drivers/net/phy/at803x.c > @@ -366,6 +366,7 @@ static void at803x_link_change_notify(struct phy_device > *phydev) > static int at803x_aneg_done(struct phy_device *phydev) > { > int ccr; > + int timeout = 100; /* usecs */ > > int aneg_done = genphy_aneg_done(phydev); > if (aneg_done != BMSR_ANEGCOMPLETE) > @@ -383,7 +384,13 @@ static int at803x_aneg_done(struct phy_device *phydev) > phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL); > > /* check if the SGMII link is OK. */ > - if (!(phy_read(phydev, AT803X_PSSR) & AT803X_PSSR_MR_AN_COMPLETE)) { > + do { > + if (phy_read(phydev, AT803X_PSSR) & AT803X_PSSR_MR_AN_COMPLETE) > + break; > + udelay(1); > + } while (--timeout); > + > + if (!timeout) { > pr_warn("803x_aneg_done: SGMII link is not ok\n"); > aneg_done = 0; > } > Could you confirm that you are using PHY_HAS_INTERRUPT? In polling mode the effect would be very unlikely to happen, since you'd need to run the state machine exactly between the two AN stages. As for the 100us delay proposed, is this something Atheros suggested or is it based on empirical considerations? Since ending up in a situation where the double-check fails might left you with a permanent link loss, having a reliable minimum required delay between first and second stage AN is essential - to me 100us look quite short. Same goes for the readout polling proposed: given that reading an MDIO register takes ~16us (at assumed 2.5MHz MDC), delaying for 1us between reads is kind of useless and ends up in storm-reading the register (and also extends the wait-time by the same factor). Imo, it won't hurt to sleep for milliseconds between reads here (see phy_poll_reset() for reference). Cheers, Zefir
Re: [PATCH net] at803x: insure minimum delay for SGMII link AN completion ckeck
On 02/10/2017 08:42 AM, Claudiu Manoil wrote: > Commit: f62265b "at803x: double check SGMII side autoneg" > introduced a regression for the p1010rdb board which has > two of the ethernet controllers (eTSEC) connected through > SGMII links to external Atheros SGMII AR8033 PHYs. > The issue consists in a dead link for these ports, and is > 100% reproducible on kernel 4.9 (and later): > > root@p1010rdb-pb:~# ifconfig eth2 172.16.1.1 > [ 203.274263] IPv6: ADDRCONF(NETDEV_UP): eth2: link is not ready > root@p1010rdb-pb:~# [ 206.408255] 803x_aneg_done: SGMII link is not ok > > root@p1010rdb-pb:~# ethtool eth2 > Settings for eth2: > Supported ports: [ MII ] > Supported link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > 1000baseT/Full > Supported pause frame use: Symmetric Receive-only > Supports auto-negotiation: Yes > Advertised link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > 1000baseT/Full > Advertised pause frame use: No > Advertised auto-negotiation: Yes > Link partner advertised link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > 1000baseT/Half 1000baseT/Full > Link partner advertised pause frame use: Symmetric Receive-only > Link partner advertised auto-negotiation: Yes > Speed: 1000Mb/s > Duplex: Full > Port: MII > PHYAD: 2 > Transceiver: internal > Auto-negotiation: on > Supports Wake-on: g > Wake-on: d > Current message level: 0x003f (63) >drv probe link timer ifdown ifup > Link detected: no > > Insuring up to 100 usecs for the SGMII link side AN to complete > proves to be enough to have a working SGMII link, for this board. > The need for a delay for the SGMII link side may be explained by > the fact that there are two levels of auto-negotiation (AN) for a > SGMII link. First the PHY autonegotiates the link parameters w/ > its link partner over the copper link. In the second stage, the > AN results are then passed to the eTSEC MAC over the SGMII link > using the Clause 37 auto-negotiation functionality. While the > aneg_done() hook is called by the phylib state machine to check > for the completion of the 1st stage AN of the external PHY, > there's no mechanism to insure proper AN completion of the internal > SGMII link (which is actually handled on the eTSEC side by a > "internal PHY", called TBI). > > Fixes: f62265b "at803x: double check SGMII side autoneg" > > Signed-off-by: Claudiu Manoil> --- > drivers/net/phy/at803x.c | 9 - > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > index a52b560..55fa7c4 100644 > --- a/drivers/net/phy/at803x.c > +++ b/drivers/net/phy/at803x.c > @@ -366,6 +366,7 @@ static void at803x_link_change_notify(struct phy_device > *phydev) > static int at803x_aneg_done(struct phy_device *phydev) > { > int ccr; > + int timeout = 100; /* usecs */ unsigned int, and use reverse christmas tree declarations, order from longest variable to shortest. -- Florian
[PATCH net] at803x: insure minimum delay for SGMII link AN completion ckeck
Commit: f62265b "at803x: double check SGMII side autoneg" introduced a regression for the p1010rdb board which has two of the ethernet controllers (eTSEC) connected through SGMII links to external Atheros SGMII AR8033 PHYs. The issue consists in a dead link for these ports, and is 100% reproducible on kernel 4.9 (and later): root@p1010rdb-pb:~# ifconfig eth2 172.16.1.1 [ 203.274263] IPv6: ADDRCONF(NETDEV_UP): eth2: link is not ready root@p1010rdb-pb:~# [ 206.408255] 803x_aneg_done: SGMII link is not ok root@p1010rdb-pb:~# ethtool eth2 Settings for eth2: Supported ports: [ MII ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Advertised pause frame use: No Advertised auto-negotiation: Yes Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Half 1000baseT/Full Link partner advertised pause frame use: Symmetric Receive-only Link partner advertised auto-negotiation: Yes Speed: 1000Mb/s Duplex: Full Port: MII PHYAD: 2 Transceiver: internal Auto-negotiation: on Supports Wake-on: g Wake-on: d Current message level: 0x003f (63) drv probe link timer ifdown ifup Link detected: no Insuring up to 100 usecs for the SGMII link side AN to complete proves to be enough to have a working SGMII link, for this board. The need for a delay for the SGMII link side may be explained by the fact that there are two levels of auto-negotiation (AN) for a SGMII link. First the PHY autonegotiates the link parameters w/ its link partner over the copper link. In the second stage, the AN results are then passed to the eTSEC MAC over the SGMII link using the Clause 37 auto-negotiation functionality. While the aneg_done() hook is called by the phylib state machine to check for the completion of the 1st stage AN of the external PHY, there's no mechanism to insure proper AN completion of the internal SGMII link (which is actually handled on the eTSEC side by a "internal PHY", called TBI). Fixes: f62265b "at803x: double check SGMII side autoneg" Signed-off-by: Claudiu Manoil--- drivers/net/phy/at803x.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index a52b560..55fa7c4 100644 --- a/drivers/net/phy/at803x.c +++ b/drivers/net/phy/at803x.c @@ -366,6 +366,7 @@ static void at803x_link_change_notify(struct phy_device *phydev) static int at803x_aneg_done(struct phy_device *phydev) { int ccr; + int timeout = 100; /* usecs */ int aneg_done = genphy_aneg_done(phydev); if (aneg_done != BMSR_ANEGCOMPLETE) @@ -383,7 +384,13 @@ static int at803x_aneg_done(struct phy_device *phydev) phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL); /* check if the SGMII link is OK. */ - if (!(phy_read(phydev, AT803X_PSSR) & AT803X_PSSR_MR_AN_COMPLETE)) { + do { + if (phy_read(phydev, AT803X_PSSR) & AT803X_PSSR_MR_AN_COMPLETE) + break; + udelay(1); + } while (--timeout); + + if (!timeout) { pr_warn("803x_aneg_done: SGMII link is not ok\n"); aneg_done = 0; } -- 1.7.11.7