Message-ID: <20171230173157.GC10595@n2100.armlinux.org.uk>
Date:   Sat, 30 Dec 2017 17:31:57 +0000
From:   Russell King - ARM Linux <linux@...linux.org.uk>
To:     Marcin Wojtas <mw@...ihalf.com>
Cc:     Stefan Chulski <stefanc@...vell.com>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Yan Markman <ymarkman@...vell.com>,
        Jason Cooper <jason@...edaemon.net>,
        netdev <netdev@...r.kernel.org>,
        Antoine Tenart <antoine.tenart@...e-electrons.com>,
        linux-kernel@...r.kernel.org, kishon@...com, nadavh@...vell.com,
        Miquèl Raynal 
        <miquel.raynal@...e-electrons.com>,
        Gregory Clément 
        <gregory.clement@...e-electrons.com>,
        "David S. Miller" <davem@...emloft.net>,
        linux-arm-kernel@...ts.infradead.org,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
Subject: Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the
 fourth network interface

Hi Marcin,

On Sat, Dec 30, 2017 at 05:34:23PM +0100, Marcin Wojtas wrote:
> Yes, I already split the series and will send first one right away. I
> will be followed by MDIO bus / PHY handling proposal, including the
> bits related to phylink. I'm looking forward to your opinion on that
> once sent.

I'm looking forward to the patches. :)

> This my understanding of how the PP2 HW works in terms of signalling
> the link interrupt:
> 
> The full in-band management, similar to mvneta is supported only in
> the SGMII mode (1G, not sure how it looks like in 2.5G mode). Such
> handling is not yet implemented in the mvpp2.c
> 
> 10G:
> The XGMII MAC (XLG) is capable of generating link status change
> interrupt upon information provided from the reconciliation layer (RS)
> of the interface.
> 
> 2.5G/1G SGMII:
> Apart from the in-band management, the MAC is also capable of
> generating IRQ during link-status change.
> 
> 1G RGMII:
> I was a bit surprised, but checked on my own - the link change IRQ can
> be generated here as well.
> 
> In addition to above the clause 22 PHYs can be automatically polled
> via SMI bus and provide complete information about link status, speed,
> etc., reflecting it directly in GMAC status registers. However, this
> feature had to be disabled, in order not to conflict with SW PHY
> management of the phylib.
> 
> Stefan, is above correct?

This sounds very much like mvneta's 'managed = "in-band"' mode.

Having done some research earlier this month on the "2.5G SGMII" I have
a number of comments about this:

1. Beware of "SGMII" being used as a generic term for single lane serdes
   based ethernet. Marvell seem to use this for 802.3z BASE-X in their
   code, but it is not. SGMII is a modification of 802.3z BASE-X by
   Cisco.  This leads to some confusion!

2. For Cisco SGMII running at 2.5G, PHYs such as those from Xilinx do not
   support the speed bits, because the speed is defined to be 2.5G.  IOW,
   they do not support 250Mbps or 25Mbps speeds by data replication as is
   done with 100Mbps and 10Mbps over 1G SGMII.

3. There is also 1000BASE-X upclocked to 2.5G speeds, which mvneta and
   mvpp2 both support by appropriate configuration of the comphy.  I've
   already tested this with 4.3Mbps Fiberchannel SFPs between clearfog
   and mcbin - but needing devmem2 to reconfigure the clearfog comphy.

> > If my guessing is correct, I have to wonder why mvpp2 invented a
> > different way to represent this from mvneta?  This makes it much more
> > difficult to convert mvpp2 to phylink, and it also makes it difficult
> > to add SFP support ignoring the phylink issue (since there is no phy
> > handle there either.)
> 
> Doesn't SFP require the fwnode handle to the sfp node? This is what I
> understand at least from the phylink_register_sfp.

Yes, internally within phylink.  What I'm concerned about is the
following disparity between mvneta and mvpp2 - I'll try to explain it
more clearly with DT examples:

1.1. mvneta phy
	&eth {
		phy = <&phy>;
		phy-mode = "whatever";
	};
1.2. mvneta fixed-link
	&eth {
		fixed-link {
			speed = <1000>;
			full-duplex;
		};
	};
1.3. mvneta in-band
	&eth {
		phy-mode = "sgmii";
		managed = "in-band-status";
	};
2.1. mvpp2 phy
	&eth {
		phy = <&phy>;
		phy-mode = "whatever";
	};
2.2. mvpp2 fixed-link
	&eth {
		fixed-link {
			speed = <1000>;
			full-duplex;
		};
	};
2.3. mvpp2 in-band (guess)
	&eth {
		phy-mode = "sgmii";
	};

In both cases, the representation for phy and fixed-link mode are the
same, but the in-band are different.  In mvneta in-band, the generic
"managed" property must be specified as specified by
Documentation/devicetree/bindings/net/ethernet.txt.  However, for mvpp2,
this mode is currently selected by omission of both a "phy" property and
a "fixed-link" sub-node/property - and that goes against the description
of the "managed" property in the ethernet.txt binding doc.

Phylink won't recognise the mvpp2's style of "in-band" because phylink,
being a piece of generic code, is written to follow the generic binding
documentation, rather than accomodating driver's individual quirks.

So, if what I think is correct (basically what I've said above) there is
a problem converting mvpp2 to use phylink - any existing DT files that
use the "2.3 mvpp2 in-band" method instantly break, and I think that's
what Antoine referred to when I picked out that the previous patches
avoided using phylink when there was no "phy" node present.

However, I haven't spotted anything using the 2.3 method, but it's not
that easy to find the lack of a property amongst the maze of .dts*
files - trying to track down which use mvpp2 and which do not specify
a phy or fixed-link node can't be done by grep alone due to the
includes etc.  I think the only possible way would be to build all DT
files, then reverse them back to dts and search those for the mvpp2
compatible strings, and then manually check each one.

> Anyway, once the phylink is introduced in mvpp2.c, its presence will
> simply be detected by port->phylink pointer. In such case the link IRQ
> will no be used. In longer perspective, link IRQ should be used only
> by ACPI and once MDIO bus is supported in generic way in this world,
> it could remain as the 'last resort' option.

It's not though - there are SFP modules that are SGMII and we have no
access to the PHY onboard, so the only way we know what they're doing
is from the inband status sent as part of the SGMII in-band
configuration.  So, even when using phylink, we need the in-band
stuff to work, and so we need those link IRQs.

There's also additional complexities around Cisco SGMII and "extended"
SGMII concerning the flow control settings - in Cisco SGMII, there
are no bits in the 16-bit control word for communicating the flow
control to the MAC.  In extended SGMII (which appears in some Marvell
devices) you can configure flow control to appear in the 16-bit
control word, and in some cases, also EEE.  When implemented correctly
by the MAC, phylink supports the "Cisco" method when it knows that
in-band AN is being used along with a PHY - it knows to read the
settings from the MAC but combine the flow control with what has been
read from the PHY.  If this is not done, we're likely to end up with
the link partner believing that FC is supported (eg, because the PHY
has defaulted to advertising FC) but the local MAC having FC disabled.

Note that there's another quirk as far as SGMII goes - some PHYs will
not pass data until their "negotiation" (iow, passing and acknowledgement
of the SGMII control word by the MAC) has completed.  Disabling SGMII
"AN" on the MAC causes some SGMII PHYs to apparently be in "link up"
state but with no traffic flow possible in either direction.  This is
a particularly important point if using phylib - the temptation is to
use phylib to pass the results of AN to the MAC for SGMII and disable
AN on the MAC, but this is, in fact, wrong for the reason set out in
this paragraph.

There are bits present that allow AN bypass if it doesn't complete in
a certain time, but that's an entirely separate issue - especially
when there's SGMII PHYs that we have no access to!

Sorting out these nuances over the life of phylink so far has been
"interesting".

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Powered by blists - more mailing lists