Date:   Fri, 20 Jan 2017 16:31:09 +0100
From:   Zefir Kurtisi <zefir.kurtisi@...atec.com>
To:     Timur Tabi <timur@...eaurora.org>, netdev@...r.kernel.org
Cc:     andrew@...n.ch, f.fainelli@...il.com
Subject: Re: [PATCH 2/2] at803x: double check SGMII side autoneg

On 01/20/2017 03:38 AM, Timur Tabi wrote:
> Zefir Kurtisi wrote:
>> It always operates at 675MHz, which with two lines gives 1.25Gbps,
>> which at 10/8 coding gives exactly 1Gbps net data rate. If the
>> at803x's copper side autonegotiates to 1Gbps, the bits traversing
>> over the SGMII match the copper side 1:1. In case the copper side
>> autonegotiates to e.g. 100Mbps, each bit at the copper side on the
>> SGMII bus is replicated and sent 10x times - or 100x times in case of
>> 10Mbps. The MAC side of the ETH needs to be aware of how the SGMII
>> data has to be interpreted, and this is why you have to set the bits
>> you are referring to.
> 
> So does this mean that the SGMII link should not be autonegotiated? I currently
> have this code:
> 
>     if (phydev->autoneg == AUTONEG_ENABLE) {
>         val &= ~(FORCE_AN_RX_CFG | FORCE_AN_TX_CFG);
>         val |= AN_ENABLE;
>         writel(val, phy->base + EMAC_SGMII_PHY_AUTONEG_CFG2);
>     } else {
>         ...
> 
> So if the external PHY is set to autonegotiate, then the SGMII block is set to
> also negotiate.  Now that I think about it, this seems wrong. And in fact, I'm not
> sure how it works.  It seems that the this only makes sense if the SGMII block is
> configured to act as the only PHY. This is an option that the hardware supports
> but my driver does not.  So perhaps I should remove this part, and just do the rest:
> 
> 
>     u32 speed_cfg;
> 
>     switch (phydev->speed) {
>     case SPEED_10:
>         speed_cfg = SPDMODE_10;
>         break;
>     case SPEED_100:
>         speed_cfg = SPDMODE_100;
>         break;
>     case SPEED_1000:
>         speed_cfg = SPDMODE_1000;
>         break;
>     default:
>         return -EINVAL;
>     }
> 
>     if (phydev->duplex == DUPLEX_FULL)
>         speed_cfg |= DUPLEX_MODE;
> 
>     val &= ~AN_ENABLE;
>     writel(speed_cfg, phy->base + EMAC_SGMII_PHY_SPEED_CFG1);
>     writel(val, phy->base + EMAC_SGMII_PHY_AUTONEG_CFG2);
> 
> Should I be doing this all the time?
> 
Hm, that might be product dependent. From my experience with the gianfar/at8031
this is what I can share: there is a known flaw in some revisions of the eTSEC
that causes random failures in SGMII autonegotiation. As a workaround FSL proposes
to use fixed speed. I spent lots of time getting this working, but in the end it
turned out it does not. Setting fixed Gbps speed on both ends of the link is
reported to be ok (link failure bits clean, page transmitted ok), but no data goes
through the link.

>From a source I can't remember atm I was told that autonegotiation is mandatory
for SGMII, therefore I believe there is no need to change the autoneg bits for
SGMII in your case. I'd assume that resetting the link before setting the speed
value should be enough, i.e. the second write to EMAC_SGMII_PHY_AUTONEG_CFG2 might
be useless. YMMV

>> To track down who is causing the additional message, I would proceed
>> with following technique that helped me dig down a similar problem:
>> since you control the events in question and there is no risk of
>> flooding the kernel log, in the top of phy.c::phy_print_status() add
>> a dump_stack() call. In the debug log ensure that all of the traces
>> end up in the same phydev->adjust_link() callback handler (in your
>> case emac_adjust_link()).
> 
> That's a good idea, thanks.
> 
As Florian responded, it seems you need to double check for real changes before
printing the link status to get rid of the double printed notifications.

Good Luck.

Powered by blists - more mailing lists