Date:   Thu, 19 Jan 2017 10:01:48 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Zefir Kurtisi <zefir.kurtisi@...atec.com>,
        Timur Tabi <timur@...eaurora.org>, netdev@...r.kernel.org
Cc:     andrew@...n.ch
Subject: Re: [PATCH 2/2] at803x: double check SGMII side autoneg



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

Powered by blists - more mailing lists