Re: [PATCH 1/6] net: phy: add interface modes for XFI, SFI

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Fri, Dec 20, 2019 at 07:38:45AM +0000, Madalin Bucur (OSS) wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux admin <linux@armlinux.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@armlinux.org.uk>
> > > > On Thu, Dec 19, 2019 at 06:32:51PM +0000, Madalin Bucur wrote:
> > > > > > -----Original Message-----
> > > > > > From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > > > > >
> > > > > > On Thu, Dec 19, 2019 at 05:21:16PM +0200, Madalin Bucur wrote:
> > > > > > > From: Madalin Bucur <madalin.bucur@nxp.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@semihalf.com
> arch/arm64/boot/dts/marvell/cn9132-db.dts:107:  phy-mode = "10gbase-kr";
> 
> // disabled, SFI with SFP cage, jaz@semihalf.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@free-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@free-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@tkos.co.il
> arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts:279: phy-mode = "10gbase-kr";
> 
> // SFP+, rmk+kernel@armlinux.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.

How does this help us when we can't simply change the existing usage?
We can update the DT but we can't free up the usage of "10gbase-kr".

-- 
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