On 01/19/2017 01:43 AM, Zefir Kurtisi wrote: > On 01/18/2017 04:02 PM, Timur Tabi wrote: >> On 01/18/2017 07:53 AM, Zefir Kurtisi wrote: >>> No, not necessarily. The SGMII link default configuration is set such that >>> you do >>> not have to bother at all. >> >> Does the SGMII link need to make the speed and duplex of the external link? >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c#n50 >> >> >> Here is where I can configure the SGMII link to match whatever phydev says >> the >> external link is. This is code that was handed down to me. I've never >> really >> understood what the purpose was. Shouldn't I just be able to configure the >> SGMII >> link to 1Gbs full-duplex, regardless of what the external link is set to? >> > > This is because the SGMII link is a transparent interface to the upper layer > with > no means to inform the other side of the link type in the lower layer. > > 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. > >>> That is, if in your case you see the warning popping up but the link always >>> regains connection, then it is an ignorable false positive. >> >> Unfortunately, I can't really ignore it because genphy does this: >> >> $ ifup eth1 >> [ 1034.754276] qcom-emac QCOM8070:00 eth1: Link is Up - 1Gbps/Full - flow >> control >> rx/tx >> >> But the at803x driver does this: >> >> $ ifup eth1 >> [ 1020.507728] 803x_aneg_done: SGMII link is not ok >> [ 1020.513517] qcom-emac QCOM8070:00 eth1: Link is Up - 1Gbps/Full - flow >> control >> rx/tx >> [ 1020.522839] qcom-emac QCOM8070:00 eth1: Link is Up - 1Gbps/Full - flow >> control >> rx/tx >> >> Customers are going to complain. >> > Yes, the double link-status message is annoying - I was referring to the other > message. > > 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()). > > In the gianfar's handler there is an additional check whether the link really > changed before phy_print_status() is called, in emac_adjust_link() this > happens > unconditionally - maybe that is the problem you are facing. > > @Florian: is it safe to assume that when phydev->adjust_link() is called > there was > in fact a link change (link, duplex, pause), or does the handler have to > double > check for a change? I see some ETH drivers maintain a private old state > instance > for that, while others don't.
Yes, it is not just safe, it is a contract. The reason most drivers cache the values from one run on adjust_link to the other is because you don't want to needlessly read/write registers if nothing has changed, or just a subset of link parameters did change. NB: at some point I was considering bringing this caching into the core PHY library to save some housekeeping code in drivers... -- Florian
