Message-Id: <20230923134904.3627402-1-vladimir.oltean@nxp.com>
Date: Sat, 23 Sep 2023 16:48:49 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: netdev@...r.kernel.org,
	devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	linux-phy@...ts.infradead.org
Cc: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	Madalin Bucur <madalin.bucur@....com>,
	Ioana Ciornei <ioana.ciornei@....com>,
	Camelia Groza <camelia.groza@....com>,
	Li Yang <leoyang.li@....com>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor@...nel.org>,
	Sean Anderson <sean.anderson@...o.com>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>
Subject: [RFC PATCH v2 net-next 00/15] Add C72/C73 copper backplane support for LX2160

THIS PATCH SET DOES NOT WORK, AND IT ISN'T INTENDED TO WORK.

Changes in v2:
- replace phy_check_cdr_lock() with phy_get_status(PHY_STATUS_CDR_LOCK)
- rename PHY_MODE_ETHERNET_PHY to PHY_MODE_ETHTOOL
- stop describing the AN/LT block in the device tree and make use of the
  fact that it is discoverable. Add phy_get_status(PHY_STATUS_PCVT_ADDR)
  to the generic PHY layer to discover it.
- add the 25GBase-KR-S and 25GBase-CR-S link modes. Proper treatment of
  RS-FEC and BASE-R FEC is still TODO (will also require new API in the
  generic PHY layer).
- rework the implementation from a phylib phy_device to a phylink_pcs
  component (library code). The phy-mode is still "internal". It may or
  may have not been the right thing to do. There are some things to say
  about that on patch 08/15.
- support multi-lane link modes - tested with 40GBase-KR4
- solve the pre-configuration and register access problem for 1000Base-KX
  by having mtip_get_mdiodev() pre-configure the SerDes lane and
  protocol converter for the highest supported backplane link mode.

The original cover letter for the RFC v1 at
https://patchwork.kernel.org/project/netdevbpf/cover/20230817150644.3605105-1-vladimir.oltean@nxp.com/
is still attached below.

=================================================

I've been tasked with maintaining the copper backplane support on
Layerscape SoCs. There was a previous attempt to upstream that, which is
located here:
https://lore.kernel.org/lkml/1587732391-3374-1-git-send-email-florinel.iordache@nxp.com/

Nonetheless, I am presenting here a complete rewrite based on my
understanding. The quality of implementation is not great, and there are
probably bugs which I've yet to identify. The reason for this RFC is to
collect feedback for the overall design from networking PHY and generic
PHY maintainers.

The newly proposed generic PHY API has the mtip_backplane.c driver as a
user (a consumer), but its actual hardware implementation (which should
go in drivers/phy/freescale/phy-fsl-lynx-28g.c), is not included in this
patch set. That would just have just uselessly distracted the reviewers'
attention. This is why the patch set, as posted, does not work.

I recommend reviewing from the bottom up - dt-bindings first, those give
an overall picture of the AN/LT block and its integration in the SoC.

Future work consists of:

- supporting multi-lane link modes

- advertising more than a single link mode, and performing dynamic
  SerDes protocol switching towards that link mode. Strictly speaking,
  the hardware was intended to be used with a single link mode advertised
  over C73 (the link mode pre-configured through the RCW - Reset
  Configuration Word), and that is quite apparent in its design. With
  some inventive workarounds which I've yet to try out, it might be
  possible to circumvent the design limitations and advertise any link
  mode that the SerDes PLLs can sustain. This is in an exploratory stage
  and in the end it might not come to fruition, but I've identified some
  aspects which require forethought in the driver's design.

Both these features hit a wall given the current driver design, which is
"how do we access the AN/LT block's registers?".

The hardware designers were inconsistent in the way that they've
integrated this AN/LT blocks for 1G/10G ports, vs how they did it for
25G/40G/50G/100G ports. There is always one single AN/LT block per
SerDes lane, but for 1G/10G modes, hardware design wanted to make it
appear as if the AN/LT is part of the PCS. So it inherently responds to
the same MDIO address as the PCS. Whereas in the >25G modes, it responds
to an MDIO address defined by a different control register, and that
address is also different from the PCS' MDIO address.

In the current dt-bindings, I am requesting DT writers to put a
phy-handle towards the MDIO address of the AN/LT block (which depends on
a lot of things, see the dt-bindings document for details).

As opposed to regular ports where the PCS and SerDes PHY are controlled
by the MAC driver (drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c),
with backplane, those same components are controlled by the
mtip_backplane.c driver. The backplane AN/LT driver has a pcs-handle
towards the PCS, and it treats it as a raw MDIO device when polling for
link status.

This gets obviously problematic when switching SerDes protocols, because
the location of the backplane AN/LT block changes on the MDIO bus with
the new SerDes protocol, and what's in the device tree becomes invalid
(the phydev->mdio.addr would need to be updated). It's the same AN/LT
block, and after a SerDes protocol change it has been reset, but it moved.

The SerDes control registers for the MDIO addresses of the PCS and AN/LT blocks:
- SGMIInCR1[MDEV_PORT]
- SXGMIInCR1[MDEV_PORT]
- ANLTnCR1[MDEV_PORT]
- E25GnCR1[MDEV_PORT]
- E40GnCR1[MDEV_PORT]
- E50GnCR1[MDEV_PORT]
- E100GnCR1[MDEV_PORT]

are in premise all configurable, but it's an open question to me as to
how Linux should configure them. Currently, we treat all those addresses
as read-only and populate the device trees based on their default values.

There's an additional (related) problem for 1000base-KX. The lane starts
up at power-on reset in 1000base-X mode (non-backplane) and this means
that the PCS which responds to MDIO commands is the one used for
SGMII/1000Base-X by drivers/net/pcs/pcs-lynx.c. That responds to C22
MDIO commands. The PCS for 1000Base-KX is different, and it responds to
C45 MDIO commands. It also incorporates the AN/LT block at this C45 MDIO
address. In the current mtip_backplane.c driver design, the lane switches
to backplane mode (thus enabling the respective PCS) once the link mode
was resolved to 1000base-KX through C73 autoneg, using phy_set_mode_ext().
But that is too late with 1000base-KX, since the AN/LT block won't
respond to C45 transactions either, if the port isn't pre-configured for
1000base-KX. So C73 autoneg won't work to begin with... Somebody needs
to pre-configure the SerDes lane for backplane mode, so that the
mtip_backplane.c driver can probe, but it's not clear who and based on
what.

Finally, I reckon that in the end, the PCS should be driven using a
struct phylink_pcs, by the lynx_pcs driver, and not as an mdio_device
using raw accesses by the mtip_backplane.c. In premise, this AN/LT IP
core can be integrated, to my knowledge, with any PCS and any SerDes,
so that should be also possible in the driver design. Yet, there's a
problem: in the 1G/10G modes, there would be 2 drivers for the same
mdio_device: one for the PCS and the other for the AN/LT block (PHY).
True, they are at different MMDs, but bindings multiple Linux drivers to
mdio_devices per MMD is not a thing, I guess?

Interrupt support is also missing, and that's very far away on my TODO
list. AFAIU, PCS/ANLT interrupts are routed by the SoC to the attached
MAC's IEVENT register, which isn't enabled as an interrupt source on any
Layerscape platform yet. I've structured the code using an irqpoll
thread for now, so it is more-or-less just as event-driven as an IRQ
based driver would be.

As a side note, I have analyzed the feedback previously given to Florinel,
especially the one from Russell:
https://lore.kernel.org/lkml/20200425105210.GZ25745@shell.armlinux.org.uk/

"This uses phylib, which is a problem ..."

and I still haven't changed that here. Without going into a wall of text
explanation, I'm not fully convinced that phylib isn't, in fact, the
best place for a backplane AN/LT driver to live. At least in the initial
implementation shown here, I'm not sure that going straight for a
phylink implementation of the AN/LT block would have solved any of the
problems that I described above.

Vladimir Oltean (15):
  phy: introduce phy_get_status() and use it to report CDR lock
  phy: introduce the PHY_MODE_ETHTOOL mode for phy_set_mode_ext()
  phy: ethernet: add configuration interface for copper backplane
    Ethernet PHYs
  phy: allow querying the address of protocol converters through
    phy_get_status()
  net: add 25GBase-KR-S and 25GBase-CR-S to ethtool link mode UAPI
  net: mii: add C73 base page helpers
  net: phylink: centralize phy_interface_mode_is_8023z() &&
    phylink_autoneg_inband() checks
  net: phylink: allow PCS to handle C73 autoneg for phy-mode =
    "internal"
  net: ethtool: introduce ethtool_link_mode_str()
  net: phylink: support all ethtool port modes with inband modes
  net: phylink: support the 25GBase-KR-S and 25GBase-CR-S link modes too
  net: phylink: add the 25G link modes to
    phylink_c73_priority_resolution[]
  dt-bindings: lynx-pcs: add properties for backplane mode
  net: pcs: mtip_backplane: add driver for MoreThanIP backplane AN/LT
    core
  net: pcs: lynx: use MTIP AN/LT block for copper backplanes

 .../bindings/net/pcs/fsl,lynx-pcs.yaml        |   15 +-
 drivers/net/mii.c                             |   34 +-
 drivers/net/pcs/Kconfig                       |    8 +
 drivers/net/pcs/Makefile                      |    1 +
 drivers/net/pcs/mtip_backplane.c              | 2022 +++++++++++++++++
 drivers/net/pcs/mtip_backplane.h              |   87 +
 drivers/net/pcs/pcs-lynx.c                    |  135 ++
 drivers/net/phy/phy-core.c                    |    2 +-
 drivers/net/phy/phylink.c                     |   53 +-
 drivers/phy/phy-core.c                        |   31 +
 include/linux/ethtool.h                       |    6 +
 include/linux/mii.h                           |  105 +
 include/linux/phy/phy-ethernet.h              |  292 +++
 include/linux/phy/phy.h                       |   83 +
 include/linux/phylink.h                       |    1 +
 include/uapi/linux/ethtool.h                  |    2 +
 net/ethtool/common.c                          |   12 +
 17 files changed, 2876 insertions(+), 13 deletions(-)
 create mode 100644 drivers/net/pcs/mtip_backplane.c
 create mode 100644 drivers/net/pcs/mtip_backplane.h
 create mode 100644 include/linux/phy/phy-ethernet.h

-- 
2.34.1

Powered by blists - more mailing lists