Date:   Fri, 20 Dec 2019 07:38:45 +0000
From:   "Madalin Bucur (OSS)" <madalin.bucur@....nxp.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>,
        "Madalin Bucur (OSS)" <madalin.bucur@....nxp.com>,
        "antoine.tenart@...e-electrons.com" 
        <antoine.tenart@...e-electrons.com>,
        "jaz@...ihalf.com" <jaz@...ihalf.com>,
        "baruch@...s.co.il" <baruch@...s.co.il>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [PATCH 1/6] net: phy: add interface modes for XFI, SFI

> -----Original Message-----
> From: Russell King - ARM Linux admin <linux@...linux.org.uk>
> On Thu, Dec 19, 2019 at 09:34:57PM +0000, Madalin Bucur (OSS) wrote:
> > > -----Original Message-----
> > > From: Russell King - ARM Linux admin <linux@...linux.org.uk>
> > > On Thu, Dec 19, 2019 at 06:32:51PM +0000, Madalin Bucur wrote:
> > > > > -----Original Message-----
> > > > > From: Russell King - ARM Linux admin <linux@...linux.org.uk>
> > > > >
> > > > > On Thu, Dec 19, 2019 at 05:21:16PM +0200, Madalin Bucur wrote:
> > > > > > From: Madalin Bucur <madalin.bucur@....com>
> > > > > >
> > > > > > Add explicit entries for XFI, SFI to make sure the device
> > > > > > tree entries for phy-connection-type "xfi" or "sfi" are
> > > > > > properly parsed and differentiated against the existing
> > > > > > backplane 10GBASE-KR mode.
> > > > >
> > > > > 10GBASE-KR is actually used for XFI and SFI (due to a slight
> > > > > mistake on my part, it should've been just 10GBASE-R).
> > > > >
> > > > > Please explain exactly what the difference is between XFI, SFI
> > > > > and 10GBASE-R. I have not been able to find definitive definitions
> > > > > for XFI and SFI anywhere, and they appear to be precisely identical
> > > > > to 10GBASE-R. It seems that it's just a terminology thing, with
> > > > > different groups wanting to "own" what is essentially exactly the
> > > > > same interface type.
> > > >
> > > > Hi Russell,
> > > >
> > > > 10GBase-R could be used as a common nominator but just as well 10G
> > > > and remove the rest while we're at it. There are/may be differences in
> > > > features, differences in the way the HW is configured (the most
> > > > important aspect) and one should be able to determine what interface
> > > > type is in use to properly configure the HW. SFI does not have the
> > > > CDR function in the PMD, relying on the PMA signal conditioning vs the
> > > > XFI that requires this in the PMD. We kept the xgmii compatible for so
> > > > long without much issues until someone started cleaning up the PHY
> > > > supported modes. Since we're doing that, let's be rigorous. The 10GBase-KR
> > > > is important too, we have some backplane code in preparation and
> > > > having it there could pave the way for a simpler integration.
> > >
> > > The problem we currently have is:
> > >
> > > $ grep '10gbase-kr' arch/*/boot/dts -r
> > >
> > > virtually none of those are actually backplane. For the mcbin
> > > matches, these are either to a 88x3310 PHY for the doubleshot, which
> > > dynamically operates between XFI, 5GBASE-R, 2500BASE-X, or SGMII according
> > > to the datasheet.
> >
> > Yes, I've seen it's used already in several places:
> >
> > $ grep PHY_INTERFACE_MODE_10GKR drivers/net -nr
> > drivers/net/phy/marvell10g.c:219:       if (iface !=
> PHY_INTERFACE_MODE_10GKR) {
> > drivers/net/phy/marvell10g.c:307:           phydev->interface !=
> PHY_INTERFACE_MODE_10GKR)
> > drivers/net/phy/marvell10g.c:389:            phydev->interface ==
> PHY_INTERFACE_MODE_10GKR) && phydev->link) {
> > drivers/net/phy/marvell10g.c:398:                       phydev-
> >interface = PHY_INTERFACE_MODE_10GKR;
> > drivers/net/phy/phylink.c:296:          case PHY_INTERFACE_MODE_10GKR:
> > drivers/net/phy/aquantia_main.c:361:            phydev->interface =
> PHY_INTERFACE_MODE_10GKR;
> > drivers/net/phy/aquantia_main.c:499:        phydev->interface !=
> PHY_INTERFACE_MODE_10GKR)
> > drivers/net/phy/sfp-bus.c:340:          return
> PHY_INTERFACE_MODE_10GKR;
> > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:1117:   return
> interface == PHY_INTERFACE_MODE_10GKR ||
> > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:1203:   case
> PHY_INTERFACE_MODE_10GKR:
> > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:1652:   case
> PHY_INTERFACE_MODE_10GKR:
> > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:4761:   case
> PHY_INTERFACE_MODE_10GKR:
> > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:4783:   case
> PHY_INTERFACE_MODE_10GKR:
> >
> > We should fix this, if it's incorrect.
> >
> > > If we add something else, then the problem becomes what to do about
> > > that lot - one of the problems is, it seems we're going to be
> > > breaking DT compatibility by redefining 10gbase-kr to be correct.
> >
> > We need the committer/maintainer to update that to a correct value.
> 
> The general principle is, we don't break existing DT - in that, we
> expect DT files from current kernels to work with future kernels. So,
> we're kind of stuck with "10gbase-kr" being used for this at least in
> the medium term.
> 
> By all means introduce "xfi" and "sfi" if you think that there is a
> need to discriminate between the two, but I've seen no hardware which
> that treats them any differently from 10gbase-r.
> 
> If we want to support real 10gbase-kr, then I think we need to consider
> how to do that without affecting compatibility with what we already
> have.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down
> 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

I've looked at the device tree entries using 10GBase-KR:

all these are disabled:

// disabled, commit mentions interface is SFI, jaz@...ihalf.com
arch/arm64/boot/dts/marvell/cn9132-db.dts:107:  phy-mode = "10gbase-kr";

// disabled, SFI with SFP cage, jaz@...ihalf.com
arch/arm64/boot/dts/marvell/cn9130-db.dts:131:  phy-mode = "10gbase-kr";
arch/arm64/boot/dts/marvell/cn9131-db.dts:89:   phy-mode = "10gbase-kr";

these are used:

// SFP ports, antoine.tenart@...e-electrons.com
arch/arm64/boot/dts/marvell/armada-7040-db.dts:279:     phy-mode = "10gbase-kr"; 
arch/arm64/boot/dts/marvell/armada-8040-db.dts:190:     phy-mode = "10gbase-kr";
arch/arm64/boot/dts/marvell/armada-8040-db.dts:334:     phy-mode = "10gbase-kr";

// SFP, 10GKR, antoine.tenart@...e-electrons.com
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts:37:   phy-mode = "10gbase-kr";
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts:44:   phy-mode = "10gbase-kr";

// SFP, baruch@...s.co.il
arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts:279: phy-mode = "10gbase-kr";

// SFP+, rmk+kernel@...linux.org.uk
arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dts:19:        phy-mode = "10gbase-kr"; 
arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dts:26:        phy-mode = "10gbase-kr"; 

I've added the information I could derive from the commit message.
Maybe the original authors of the commits can help us with more
information on the actual HW capabilities/operation mode.

Regards,
Madalin

Powered by blists - more mailing lists