[PATCH net] at803x: insure minimum delay for SGMII link AN completion ckeck

[PATCH net] at803x: insure minimum delay for SGMII link AN completion ckeck

[Claudiu Manoil]

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: 0x0000003f (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 <claudiu.manoil@nxp.com>
---
 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

Re: [PATCH net] at803x: insure minimum delay for SGMII link AN completion ckeck

[Florian Fainelli]

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: 0x0000003f (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 <claudiu.manoil@nxp.com>
> ---
>  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

Re: [PATCH net] at803x: insure minimum delay for SGMII link AN completion ckeck

[Zefir Kurtisi]

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: 0x0000003f (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 <claudiu.manoil@nxp.com>
> ---
>  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

[Claudiu Manoil]

>-----Original Message-----
>From: Zefir Kurtisi [mailto:zefir.kurtisi@neratec.com]
>Sent: Monday, February 13, 2017 12:16 PM
>To: Claudiu Manoil <claudiu.manoil@nxp.com>
>Cc: netdev@vger.kernel.org; David S. Miller <davem@davemloft.net>; Florian
>Fainelli <f.fainelli@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

[Zefir Kurtisi]

On 02/13/2017 02:15 PM, Claudiu Manoil wrote:
>> -----Original Message-----
>> From: Zefir Kurtisi [mailto:zefir.kurtisi@neratec.com]
>> Sent: Monday, February 13, 2017 12:16 PM
>> To: Claudiu Manoil <claudiu.manoil@nxp.com>
>> Cc: netdev@vger.kernel.org; David S. Miller <davem@davemloft.net>; Florian
>> Fainelli <f.fainelli@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

[Andrew Lunn]

> 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

[Claudiu Manoil]

>-----Original Message-----
>From: Andrew Lunn [mailto:andrew@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

[Andrew Lunn]

On Tue, Feb 14, 2017 at 02:31:53PM +0000, Claudiu Manoil wrote:
> >-----Original Message-----
> >From: Andrew Lunn [mailto:andrew@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