Message-ID: <7fd147c9-7531-663a-923f-cdfb0a290fe8@nxp.com>
Date:   Wed, 18 Dec 2019 20:15:59 +0000
From:   Alexandru Marginean <alexandru.marginean@....com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        "Y.b. Lu" <yangbo.lu@....com>, netdev <netdev@...r.kernel.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [RFC PATCH v2 0/8] Convert Felix DSA switch to PHYLINK

On 12/18/2019 6:22 PM, Russell King - ARM Linux admin wrote:
> On Wed, Dec 18, 2019 at 03:00:41PM +0000, Alexandru Marginean wrote:
>> On 12/18/2019 2:29 PM, Russell King - ARM Linux admin wrote:
>>> On Wed, Dec 18, 2019 at 03:21:02PM +0200, Vladimir Oltean wrote:
>>>> - The at803x.c driver explicitly checks for the ACK from the MAC PCS,
>>>> and prints "SGMII link is not ok" otherwise, and refuses to bring the
>>>> link up. This hurts us in 4.19 because I think the check is a bit
>>>> misplaced in the .aneg_done callback. To be precise, what we observe
>>>> is that this function is not called by the state machine a second,
>>>> third time etc to recheck if the AN has completed in the meantime. In
>>>> current net-next, as far as I could figure out, at803x_aneg_done is
>>>> dead code. What is ironic about the commit f62265b53ef3 ("at803x:
>>>> double check SGMII side autoneg") that introduced this function is
>>>> that it's for the gianfar driver (Freescale eTSEC), a MAC that has
>>>> never supported reprogramming itself based on the in-band config word.
>>>> In fact, if you look at gfar_configure_serdes, it even configures its
>>>> register 0x4 with an advertisement for 1000Base-X, not SGMII (0x4001).
>>>> So I really wonder if there is any real purpose to this check in
>>>> at803x_aneg_done, and if not, I would respectfully remove it.
>>>
>>> Please check whether at803x will pass data if the SGMII config exchange
>>> has not completed - I'm aware of some PHYs that, although link comes up
>>> on the copper side, if AN does not complete on the SGMII side, they
>>> will not pass data, even if the MAC side is forced up.
>>>
>>> I don't see any configuration bits in the 8031 that suggest the SGMII
>>> config exchange can be bypassed.
>>>
>>>> - The vsc8514 PHY driver configures SerDes AN in U-Boot, but not in
>>>> Linux. So we observe that if we disable PHY configuration in U-Boot,
>>>> in-band AN breaks in Linux. We are actually wondering how we should
>>>> fix this: from what you wrote above, it seems ok to hardcode SGMII AN
>>>> in the PHY driver, and just ignore it in the PCS if managed =
>>>> "in-band-status" is not set with PHYLINK. But as you said, in the
>>>> general case maybe not all PHYs work until they haven't received the
>>>> ACK from the MAC PCS, which makes this insufficient as a general
>>>> solution.
>>>>
>>>> But the 2 cases above illustrate the lack of consistency among PHY
>>>> drivers w.r.t. in-band aneg.
>>>
>>> Indeed - it's something of a mine field at the moment, because we aren't
>>> quite sure whether "SGMII" means that the PHY requires in-band AN or
>>> doesn't provide it. For the Broadcom case I mentioned, when it's used on
>>> a SFP, I've had to add a quirk to phylink to work around it.
>>>
>>> The problem is, it's not a case that the MAC can demand that the PHY
>>> provides in-band config - some PHYs are incapable of doing so. Whatever
>>> solution we come up with needs to be a "negotiation" between the PHY
>>> driver and the MAC driver for it to work well in the known scenarios -
>>> like the case with the Broadcom PHY on a SFP that can be plugged into
>>> any SFP supporting network interface...
>>
>> Some sort of capability negotiation does seem to be the proper solution.
>> We can have a new capabilities field in phydev for system interface
>> capabilities and match that with MAC capabilities, configuration, factor
>> in the quirks.  The result would tell if a solution is possible,
>> especially with quirky PHYs, and if PHY drivers need to enable AN.
>>
>> Until we have that in place, any recommended approach for PHY drivers,
>> is it acceptable to hardcode system side AN on as a short term fix?
>> I've just tested VSC8514 and it doesn't allow traffic through if SI AN
>> is enabled but does not complete.  We do use it with AN on on NXP
>> systems, and it only works because U-Boot sets things up that way, but
>> relying on U-Boot isn't great.
>> Aquantia PHYs we use also require AN to complete if enabled.  For them
>> Linux depends on U-Boot or on PHY firmware to enable AN.  I don't know
>> if anyone out there uses these PHYs with AN off.  Would a patch that
>> hardcodes AN on for any of these PHYs be acceptable?
> 
> I'm not sure why you're talking about hard-coding anything. As I've
> already mentioned, phylink allows you to specify today whether you
> want to use in-band AN or not, provided the MAC implements it as is
> done with mvneta and mvpp2.

I was asking about PHY drivers, not MAC, in the meantime I noticed 
phy_device carries a pointer to phylink.  I assume it's OK if PHY 
drivers check link_an_mode and set up PHY system interface based on it.

Thanks!
Alex

Powered by blists - more mailing lists