Message-ID: <b913c48d-1660-ae00-c23e-0014637153f8@gmail.com>
Date:   Thu, 28 Feb 2019 00:25:51 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: phylink / mv8e6xxx and SGMII aneg not working, link doesn't come
 up

On 27.02.2019 23:52, Russell King - ARM Linux admin wrote:
> On Wed, Feb 27, 2019 at 10:25:54PM +0100, Heiner Kallweit wrote:
>> For the ones who don't know my story yet:
>> I have a Marvell 88E6390 switch with a Clause 45 PHY connected via
>> SGMII to port 9. For other reasons I limit the PHY to 100Mbps currently.
>> DT says: managed = "in-band-status"
>> Driver is mv8e6xxx + phylink.
>> Problem is that the link doesn't come up. Debugging resulted in:
>>
>> PHY establishes link to link partner (100Mbps, FD) and fires the
>> "link up" interrupt. The "link up" is also properly transmitted via
>> SGMII in-band signalling and fires the SERDES interrupt in mv8e6xxx.
>> Problem is that the in-band transmitted values for speed and duplex
>> don't show up anywhere. And phylink_resolve() doesn't even print
>> the link-up message.
>>
>> First question: Both, PHY and SERDES interrupt, try to do the same
>> thing: they eventually call phylink_run_resolve(). Is this correct?
> 
> Correct - when the PHY interrupts, it triggers phylib to read the
> status and issue a callback via the link status hook into phylink,
> which stores the information from the PHY.  That triggers a resolve.
> Even though the PHY reports that it has established link, the link
> may not yet be up.
> 
> The Serdes is the MAC side of the link, and that should be calling
> phylink_mac_change().  That will re-run the resolve.
> 
OK, then most of my headache was caused by the fundamental
misunderstanding of who's PHY and who's MAC.

> Each time the resolve is triggered in "in-band" mode, we expect to
> read the link status from the MAC side of the link, along with the
> speed and duplex.  Since SGMII does not carry the flow control
> parameters, if a PHY is present, we merge the PHYs flow control 
> information and pass that back to the MAC to configure the MAC's
> flow control to match.
> 
>> I have some problems with understanding the code for MLO_AN_INBAND
>> in phylink_resolve(). Maybe also something is missing there for
>> proper in-band aneg support.
> 
> Nope, it works with mvneta and mvpp2.
> 
>> First we get the mac state (which is link down, 10Mbps, HD in my case).
>> Then the link state is calculated as "mac link up" && "phy link up".
>> This results in "link down", because mac link is down and phy link is
>> up. This logic isn't clear to me. How is the "link up" info supposed
>> to ever reach the mac?
> 
> Via the in-band status, and the MAC detecting that the link is now up.
> 
>> We're reading the port status, but IMO we want to set it based on the
>> auto-negotiated settings we get from the PHY.
> 
> No we don't - in SGMII in-band mode, the PHY is optional, and even if
> it is present, if the MAC reports that the link is down even if the PHY
> reports that the link is up, then the link is _not_ up.
> 
> If you want to use SGMII without in-band, then phylink supports that
> (which is PHY mode).  If the MAC is unable to report these parameters,
> then in-band mode is not appropriate.
> 
Now that it's clear that MAC is the SERDES PHY: I have these parameters
available in SGMII registers.

>> Later pause settings and possibly changed interface mode are
>> propagated to the mac. But no word about speed and duplex.
>>
>> Via "some callback" "some code" should read the in-band-transmitted
>> speed and duplex values from the SGMII port and use them to configure
>> the mac. Is this simply missing or do I miss something?
> 
>>>From what you've described, it sounds like what you actually have is:
> 
> 	MAC <---> Serdes PHY <---> PHY
> 
> The Serdes PHY receives the SGMII in-band negotiation from the external
> PHY, but there is no propagation of the status from the serdes PHY to
> the MAC.  What's more is the Serdes PHY is hidden in mv88e6xxx setups.
> 
That's exactly my setup. So what's missing is the status reporting
from SERDES PHY to actual MAC.
I could switch the port to forced mode and copy the parameters in the
SERDES "link-up" interrupt from the SGMII registers to the port forced
config. Hmm, would that be sufficient?
At least I could manage to get the link being reported as up. Whether
traffic flows I'll have to see. And I would have to see that I don't
break other usage of the SERDES port.

> This is a classic stacked PHY setup, one which we're now starting to
> encounter, and we need to support it - we have three cases I'm aware of
> now where we have:
> 
> 	MAC <---> PHY <---> SFP
> 
> and "SFP" can be another PHY, so we end up with:
> 
> 	MAC <---> PHY <---> PHY
> 
> and phylib does not support having two PHYs on the same net_device.
> 
> What's more is that the need for an "attached_dev" net_device is pretty
> heavily embedded into phylib, so attaching one PHY to another PHY
> doesn't work very well.
> 
> I've been tinkering with some patches to abstract out the "attached_dev"
> stuff, but in doing so I've walked into a whole load of issues in
> phy_attach_direct() and phy_detach() - it looks like phy_attach_direct()
> is lacking any form of locking while binding one of the generic drivers
> (it needs to hold the device lock while doing so.)  The comments around
> device_release_driver() are vague about whether the parent device needs
> to be locked.  Questions have been asked about that but so far I haven't
> had a response.
> 
> We can replace a load of phydev->attached_dev != NULL tests (which are
> checking whether the device is attached to a netdev) fairly easily,
> but there's a load of places which do (phydev->attached_dev &&
> phydev->adjust_link) - and I fear that phylink (which doesn't set
> phydev->adjust_link) results in this code being broken.
> 
Really appreciate the comprehensive explanation!

Heiner

Powered by blists - more mailing lists