[PATCH net-next 0/5] net: phy: add Lynx PCS MDIO module

[PATCH net-next 0/5] net: phy: add Lynx PCS MDIO module

[Ioana Ciornei]

Add support for the Lynx PCS as a separate module in drivers/net/phy/.
The advantage of this structure is that multiple ethernet or switch
drivers used on NXP hardware (ENETC, Felix DSA switch etc) can share the
same implementation of PCS configuration and runtime management.

The PCS is represented as a mdio_device and the callbacks exported are
highly tied with PHYLINK and can't be used without it.

The first 3 patches add some missing pieces in PHYLINK and the locked
mdiobus write accessor. Next, the Lynx PCS MDIO module is added as a
standalone module. The majority of the code is extracted from the Felix
DSA driver. The last patch makes the necessary changes in the Felix
driver in order to use the new common PCS implementation.

At the moment, USXGMII (only with in-band AN and speeds up to 2500),
SGMII, QSGMII and 2500Base-X (only w/o in-band AN) are supported by the
Lynx PCS MDIO module since these were also supported by Felix and no
funtional change is intended at this time.


Ioana Ciornei (4):
  net: phylink: consider QSGMII interface mode in
    phylink_mii_c22_pcs_get_state
  net: mdiobus: add clause 45 mdiobus write accessor
  net: phy: add Lynx PCS MDIO module
  net: dsa: felix: use the Lynx PCS helpers

Russell King (1):
  net: phylink: add interface to configure clause 22 PCS PHY

 MAINTAINERS                            |   7 +
 drivers/net/dsa/ocelot/Kconfig         |   1 +
 drivers/net/dsa/ocelot/felix.c         |   5 +
 drivers/net/dsa/ocelot/felix.h         |   7 +-
 drivers/net/dsa/ocelot/felix_vsc9959.c | 385 +++----------------------
 drivers/net/phy/Kconfig                |   6 +
 drivers/net/phy/Makefile               |   1 +
 drivers/net/phy/mdio-lynx-pcs.c        | 358 +++++++++++++++++++++++
 drivers/net/phy/phylink.c              |  38 +++
 include/linux/fsl/enetc_mdio.h         |  21 --
 include/linux/mdio-lynx-pcs.h          |  43 +++
 include/linux/mdio.h                   |   6 +
 include/linux/phylink.h                |   3 +
 13 files changed, 514 insertions(+), 367 deletions(-)
 create mode 100644 drivers/net/phy/mdio-lynx-pcs.c
 create mode 100644 include/linux/mdio-lynx-pcs.h

-- 
2.25.1

[PATCH net-next 1/5] net: phylink: add interface to configure clause 22 PCS PHY

[Ioana Ciornei]

From: Russell King <rmk+kernel@armlinux.org.uk>

Add helper for clause 22 PCS configuration via the MII bus.
Apart from applying the advertisements, the pcs_config helper also sets
up the BMCR_ANENABLE depending on the in-band AN state.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/phylink.c | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |  3 +++
 2 files changed, 40 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 0ab65fb75258..9670e8757fe1 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2267,6 +2267,43 @@ int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
 }
 EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_set_advertisement);
 
+/**
+ * phylink_mii_c22_pcs_config() - configure clause 22 PCS
+ * @pcs: a pointer to a &struct mdio_device.
+ * @mode: link autonegotiation mode
+ * @interface: the PHY interface mode being configured
+ * @advertising: the ethtool advertisement mask
+ *
+ * Configure a Clause 22 PCS PHY with the appropriate negotiation
+ * parameters for the @mode, @interface and @advertising parameters.
+ * Returns negative error number on failure, zero if the advertisement
+ * has not changed, or positive if there is a change.
+ */
+int phylink_mii_c22_pcs_config(struct mdio_device *pcs, unsigned int mode,
+			       phy_interface_t interface,
+			       const unsigned long *advertising)
+{
+	bool changed;
+	u16 bmcr;
+	int ret;
+
+	ret = phylink_mii_c22_pcs_set_advertisement(pcs, interface,
+						    advertising);
+	if (ret < 0)
+		return ret;
+
+	changed = ret > 0;
+
+	bmcr = mode == MLO_AN_INBAND ? BMCR_ANENABLE : 0;
+	ret = mdiobus_modify(pcs->bus, pcs->addr, MII_BMCR,
+			     BMCR_ANENABLE, bmcr);
+	if (ret < 0)
+		return ret;
+
+	return changed ? 1 : 0;
+}
+EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_config);
+
 /**
  * phylink_mii_c22_pcs_an_restart() - restart 802.3z autonegotiation
  * @pcs: a pointer to a &struct mdio_device.
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index cc5b452a184e..d979832d0c71 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -410,6 +410,9 @@ void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
 int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
 					  phy_interface_t interface,
 					  const unsigned long *advertising);
+int phylink_mii_c22_pcs_config(struct mdio_device *pcs, unsigned int mode,
+			       phy_interface_t interface,
+			       const unsigned long *advertising);
 void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs);
 
 void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs,
-- 
2.25.1

[PATCH net-next 2/5] net: phylink: consider QSGMII interface mode in phylink_mii_c22_pcs_get_state

[Ioana Ciornei]

The same link partner advertisement word is used for both QSGMII and
SGMII, thus treat both interface modes using the same
phylink_decode_sgmii_word() function.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/phylink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 9670e8757fe1..46f1e638b287 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2186,6 +2186,7 @@ void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
 		break;
 
 	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
 		phylink_decode_sgmii_word(state, lpa);
 		break;
 
-- 
2.25.1

[PATCH net-next 3/5] net: mdiobus: add clause 45 mdiobus write accessor

[Ioana Ciornei]

Add the locked variant of the clause 45 mdiobus write accessor -
mdiobus_c45_write().

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 include/linux/mdio.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 36d2e0673d03..323f1d1fa271 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -357,6 +357,12 @@ static inline int mdiobus_c45_read(struct mii_bus *bus, int prtad, int devad,
 	return mdiobus_read(bus, prtad, mdiobus_c45_addr(devad, regnum));
 }
 
+static inline int mdiobus_c45_write(struct mii_bus *bus, int prtad, int devad,
+				    u16 regnum, u16 val)
+{
+	return mdiobus_write(bus, prtad, mdiobus_c45_addr(devad, regnum), val);
+}
+
 int mdiobus_register_device(struct mdio_device *mdiodev);
 int mdiobus_unregister_device(struct mdio_device *mdiodev);
 bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);
-- 
2.25.1

[PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Ioana Ciornei]

Add a Lynx PCS MDIO module which exposes the necessary operations to
drive the PCS using PHYLINK.

The majority of the code is extracted from the Felix DSA driver, which
will be also modified in a later patch, and exposed as a separate module
for code reusability purposes.

At the moment, USXGMII (only with in-band AN and speeds up to 2500),
SGMII, QSGMII and 2500Base-X (only w/o in-band AN) are supported by the
Lynx PCS MDIO module since these were also supported by Felix.

The module can only be enabled by the drivers in need and not user
selectable.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 MAINTAINERS                     |   7 +
 drivers/net/phy/Kconfig         |   6 +
 drivers/net/phy/Makefile        |   1 +
 drivers/net/phy/mdio-lynx-pcs.c | 358 ++++++++++++++++++++++++++++++++
 include/linux/mdio-lynx-pcs.h   |  43 ++++
 5 files changed, 415 insertions(+)
 create mode 100644 drivers/net/phy/mdio-lynx-pcs.c
 create mode 100644 include/linux/mdio-lynx-pcs.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 301330e02bca..febba4b0a1fd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10529,6 +10529,13 @@ F:	Documentation/devicetree/bindings/net/ieee802154/mcr20a.txt
 F:	drivers/net/ieee802154/mcr20a.c
 F:	drivers/net/ieee802154/mcr20a.h
 
+MDIO LYNX PCS MODULE
+M:	Ioana Ciornei <ioana.ciornei@nxp.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	drivers/net/phy/mdio-lynx-pcs.c
+F:	include/linux/mdio-lynx-pcs.h
+
 MEASUREMENT COMPUTING CIO-DAC IIO DRIVER
 M:	William Breathitt Gray <vilhelm.gray@gmail.com>
 L:	linux-iio@vger.kernel.org
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index f25702386d83..6ea835e5d8ec 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -235,6 +235,12 @@ config MDIO_XPCS
 	  This module provides helper functions for Synopsys DesignWare XPCS
 	  controllers.
 
+config MDIO_LYNX_PCS
+	bool
+	help
+	  This module provides helper functions for Lynx PCS enablement
+	  representing the PCS as an MDIO device.
+
 endif
 endif
 
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index dc9e53b511d6..931d826b3a2b 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_MDIO_SUN4I)	+= mdio-sun4i.o
 obj-$(CONFIG_MDIO_THUNDER)	+= mdio-thunder.o
 obj-$(CONFIG_MDIO_XGENE)	+= mdio-xgene.o
 obj-$(CONFIG_MDIO_XPCS)		+= mdio-xpcs.o
+obj-$(CONFIG_MDIO_LYNX_PCS)	+= mdio-lynx-pcs.o
 
 obj-$(CONFIG_NETWORK_PHY_TIMESTAMPING) += mii_timestamper.o
 
diff --git a/drivers/net/phy/mdio-lynx-pcs.c b/drivers/net/phy/mdio-lynx-pcs.c
new file mode 100644
index 000000000000..becd01651500
--- /dev/null
+++ b/drivers/net/phy/mdio-lynx-pcs.c
@@ -0,0 +1,358 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
+/* Copyright 2020 NXP
+ * Lynx PCS MDIO helpers
+ */
+
+#include <linux/mdio.h>
+#include <linux/phylink.h>
+#include <linux/mdio-lynx-pcs.h>
+
+#define SGMII_CLOCK_PERIOD_NS		8 /* PCS is clocked at 125 MHz */
+#define SGMII_LINK_TIMER_VAL(ns)	((u32)((ns) / SGMII_CLOCK_PERIOD_NS))
+
+#define SGMII_AN_LINK_TIMER_NS		1600000 /* defined by SGMII spec */
+
+#define SGMII_LINK_TIMER_LO		0x12
+#define SGMII_LINK_TIMER_HI		0x13
+#define SGMII_IF_MODE			0x14
+#define SGMII_IF_MODE_SGMII_EN		BIT(0)
+#define SGMII_IF_MODE_USE_SGMII_AN	BIT(1)
+#define SGMII_IF_MODE_SPEED(x)		(((x) << 2) & GENMASK(3, 2))
+#define SGMII_IF_MODE_SPEED_MSK		GENMASK(3, 2)
+#define SGMII_IF_MODE_DUPLEX		BIT(4)
+
+#define USXGMII_ADVERTISE_LSTATUS(x)	(((x) << 15) & BIT(15))
+#define USXGMII_ADVERTISE_FDX		BIT(12)
+#define USXGMII_ADVERTISE_SPEED(x)	(((x) << 9) & GENMASK(11, 9))
+
+#define USXGMII_LPA_LSTATUS(lpa)	((lpa) >> 15)
+#define USXGMII_LPA_DUPLEX(lpa)		(((lpa) & GENMASK(12, 12)) >> 12)
+#define USXGMII_LPA_SPEED(lpa)		(((lpa) & GENMASK(11, 9)) >> 9)
+
+enum usxgmii_speed {
+	USXGMII_SPEED_10	= 0,
+	USXGMII_SPEED_100	= 1,
+	USXGMII_SPEED_1000	= 2,
+	USXGMII_SPEED_2500	= 4,
+};
+
+enum sgmii_speed {
+	SGMII_SPEED_10		= 0,
+	SGMII_SPEED_100		= 1,
+	SGMII_SPEED_1000	= 2,
+	SGMII_SPEED_2500	= 2,
+};
+
+static void lynx_pcs_an_restart_usxgmii(struct mdio_device *pcs)
+{
+	mdiobus_c45_write(pcs->bus, pcs->addr,
+			  MDIO_MMD_VEND2, MII_BMCR,
+			  BMCR_RESET | BMCR_ANENABLE | BMCR_ANRESTART);
+}
+
+static void lynx_pcs_an_restart(struct mdio_lynx_pcs *pcs, phy_interface_t ifmode)
+{
+	switch (ifmode) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		phylink_mii_c22_pcs_an_restart(pcs->dev);
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		lynx_pcs_an_restart_usxgmii(pcs->dev);
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		break;
+	default:
+		dev_err(&pcs->dev->dev, "Invalid PCS interface type %s\n",
+			phy_modes(ifmode));
+		break;
+	}
+}
+
+static void lynx_pcs_get_state_usxgmii(struct mdio_device *pcs,
+				       struct phylink_link_state *state)
+{
+	struct mii_bus *bus = pcs->bus;
+	int addr = pcs->addr;
+	int status, lpa;
+
+	status = mdiobus_c45_read(bus, addr, MDIO_MMD_VEND2, MII_BMSR);
+	if (status < 0)
+		return;
+
+	state->link = !!(status & MDIO_STAT1_LSTATUS);
+	state->an_complete = !!(status & MDIO_AN_STAT1_COMPLETE);
+	if (!state->link || !state->an_complete)
+		return;
+
+	lpa = mdiobus_c45_read(bus, addr, MDIO_MMD_VEND2, MII_LPA);
+	if (lpa < 0)
+		return;
+
+	switch (USXGMII_LPA_SPEED(lpa)) {
+	case USXGMII_SPEED_10:
+		state->speed = SPEED_10;
+		break;
+	case USXGMII_SPEED_100:
+		state->speed = SPEED_100;
+		break;
+	case USXGMII_SPEED_1000:
+		state->speed = SPEED_1000;
+		break;
+	case USXGMII_SPEED_2500:
+		state->speed = SPEED_2500;
+		break;
+	default:
+		break;
+	}
+
+	if (USXGMII_LPA_DUPLEX(lpa))
+		state->duplex = DUPLEX_FULL;
+	else
+		state->duplex = DUPLEX_HALF;
+}
+
+static void lynx_pcs_get_state_2500basex(struct mdio_device *pcs,
+					 struct phylink_link_state *state)
+{
+	struct mii_bus *bus = pcs->bus;
+	int addr = pcs->addr;
+	int bmsr, lpa;
+
+	bmsr = mdiobus_read(bus, addr, MII_BMSR);
+	lpa = mdiobus_read(bus, addr, MII_LPA);
+	if (bmsr < 0 || lpa < 0) {
+		state->link = false;
+		return;
+	}
+
+	state->link = !!(bmsr & BMSR_LSTATUS);
+	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
+	if (!state->link)
+		return;
+
+	state->speed = SPEED_2500;
+	state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX;
+}
+
+static void lynx_pcs_get_state(struct mdio_lynx_pcs *pcs, phy_interface_t ifmode,
+			       struct phylink_link_state *state)
+{
+	switch (ifmode) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		phylink_mii_c22_pcs_get_state(pcs->dev, state);
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		lynx_pcs_get_state_2500basex(pcs->dev, state);
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		lynx_pcs_get_state_usxgmii(pcs->dev, state);
+		break;
+	default:
+		break;
+	}
+
+	dev_dbg(&pcs->dev->dev,
+		"mode=%s/%s/%s link=%u an_enabled=%u an_complete=%u\n",
+		phy_modes(ifmode),
+		phy_speed_to_str(state->speed),
+		phy_duplex_to_str(state->duplex),
+		state->link, state->an_enabled, state->an_complete);
+}
+
+static int lynx_pcs_config_sgmii(struct mdio_device *pcs, unsigned int mode,
+				 const unsigned long *advertising)
+{
+	struct mii_bus *bus = pcs->bus;
+	int addr = pcs->addr;
+	u16 if_mode;
+	int err;
+
+	/* SGMII spec requires tx_config_Reg[15:0] to be exactly 0x4001
+	 * for the MAC PCS in order to acknowledge the AN.
+	 */
+	mdiobus_write(bus, addr, MII_ADVERTISE,
+		      ADVERTISE_SGMII | ADVERTISE_LPACK);
+
+	if_mode = SGMII_IF_MODE_SGMII_EN;
+	if (mode == MLO_AN_INBAND) {
+		u32 link_timer;
+
+		if_mode |= SGMII_IF_MODE_USE_SGMII_AN;
+
+		/* Adjust link timer for SGMII */
+		link_timer = SGMII_LINK_TIMER_VAL(SGMII_AN_LINK_TIMER_NS);
+		mdiobus_write(bus, addr, SGMII_LINK_TIMER_LO, link_timer & 0xffff);
+		mdiobus_write(bus, addr, SGMII_LINK_TIMER_HI, link_timer >> 16);
+	}
+	mdiobus_modify(bus, addr, SGMII_IF_MODE,
+		       SGMII_IF_MODE_SGMII_EN | SGMII_IF_MODE_USE_SGMII_AN,
+		       if_mode);
+
+	err = phylink_mii_c22_pcs_config(pcs, mode, PHY_INTERFACE_MODE_SGMII,
+					 advertising);
+	return err;
+}
+
+static int lynx_pcs_config_usxgmii(struct mdio_device *pcs, unsigned int mode,
+				   const unsigned long *advertising)
+{
+	struct mii_bus *bus = pcs->bus;
+	int addr = pcs->addr;
+
+	/* Configure device ability for the USXGMII Replicator */
+	mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, MII_ADVERTISE,
+			  USXGMII_ADVERTISE_SPEED(USXGMII_SPEED_2500) |
+			  USXGMII_ADVERTISE_LSTATUS(1) |
+			  ADVERTISE_SGMII |
+			  ADVERTISE_LPACK |
+			  USXGMII_ADVERTISE_FDX);
+	return 0;
+}
+
+static int lynx_pcs_config(struct mdio_lynx_pcs *pcs, unsigned int mode,
+			   phy_interface_t ifmode,
+			   const unsigned long *advertising)
+{
+	switch (ifmode) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		lynx_pcs_config_sgmii(pcs->dev, mode, advertising);
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		/* 2500Base-X only works without in-band AN,
+		 * thus nothing to do here
+		 */
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		lynx_pcs_config_usxgmii(pcs->dev, mode, advertising);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static void lynx_pcs_link_up_sgmii(struct mdio_device *pcs, unsigned int mode,
+				   int speed, int duplex)
+{
+	struct mii_bus *bus = pcs->bus;
+	u16 if_mode = 0, sgmii_speed;
+	int addr = pcs->addr;
+
+	/* The PCS needs to be configured manually only
+	 * when not operating on in-band mode
+	 */
+	if (mode == MLO_AN_INBAND)
+		return;
+
+	if (duplex == DUPLEX_HALF)
+		if_mode |= SGMII_IF_MODE_DUPLEX;
+
+	switch (speed) {
+	case SPEED_1000:
+		sgmii_speed = SGMII_SPEED_1000;
+		break;
+	case SPEED_100:
+		sgmii_speed = SGMII_SPEED_100;
+		break;
+	case SPEED_10:
+		sgmii_speed = SGMII_SPEED_10;
+		break;
+	case SPEED_UNKNOWN:
+		/* Silently don't do anything */
+		return;
+	default:
+		dev_err(&pcs->dev, "Invalid PCS speed %d\n", speed);
+		return;
+	}
+	if_mode |= SGMII_IF_MODE_SPEED(sgmii_speed);
+
+	mdiobus_modify(bus, addr, SGMII_IF_MODE,
+		       SGMII_IF_MODE_DUPLEX | SGMII_IF_MODE_SPEED_MSK,
+		       if_mode);
+}
+
+/* 2500Base-X is SerDes protocol 7 on Felix and 6 on ENETC. It is a SerDes lane
+ * clocked at 3.125 GHz which encodes symbols with 8b/10b and does not have
+ * auto-negotiation of any link parameters. Electrically it is compatible with
+ * a single lane of XAUI.
+ * The hardware reference manual wants to call this mode SGMII, but it isn't
+ * really, since the fundamental features of SGMII:
+ * - Downgrading the link speed by duplicating symbols
+ * - Auto-negotiation
+ * are not there.
+ * The speed is configured at 1000 in the IF_MODE because the clock frequency
+ * is actually given by a PLL configured in the Reset Configuration Word (RCW).
+ * Since there is no difference between fixed speed SGMII w/o AN and 802.3z w/o
+ * AN, we call this PHY interface type 2500Base-X. In case a PHY negotiates a
+ * lower link speed on line side, the system-side interface remains fixed at
+ * 2500 Mbps and we do rate adaptation through pause frames.
+ */
+static void lynx_pcs_link_up_2500basex(struct mdio_device *pcs,
+				       unsigned int mode,
+				       int speed, int duplex)
+{
+	struct mii_bus *bus = pcs->bus;
+	int addr = pcs->addr;
+
+	if (mode == MLO_AN_INBAND) {
+		dev_err(&pcs->dev, "AN not supported for 2500BaseX\n");
+		return;
+	}
+
+	mdiobus_write(bus, addr, SGMII_IF_MODE,
+		      SGMII_IF_MODE_SGMII_EN |
+		      SGMII_IF_MODE_SPEED(SGMII_SPEED_2500));
+}
+
+static void lynx_pcs_link_up(struct mdio_lynx_pcs *pcs, unsigned int mode,
+			     phy_interface_t interface,
+			     int speed, int duplex)
+{
+	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		lynx_pcs_link_up_sgmii(pcs->dev, mode, speed, duplex);
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		lynx_pcs_link_up_2500basex(pcs->dev, mode, speed, duplex);
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		/* At the moment, only in-band AN is supported for USXGMII
+		 * so nothing to do in link_up
+		 */
+		break;
+	default:
+		break;
+	}
+}
+
+struct mdio_lynx_pcs *mdio_lynx_pcs_create(struct mdio_device *mdio_dev)
+{
+	struct mdio_lynx_pcs *pcs;
+
+	if (WARN_ON(!mdio_dev))
+		return NULL;
+
+	pcs = kzalloc(sizeof(*pcs), GFP_KERNEL);
+	if (!pcs)
+		return NULL;
+
+	pcs->dev = mdio_dev;
+	pcs->an_restart = lynx_pcs_an_restart;
+	pcs->get_state = lynx_pcs_get_state;
+	pcs->link_up = lynx_pcs_link_up;
+	pcs->config = lynx_pcs_config;
+
+	return pcs;
+}
+EXPORT_SYMBOL(mdio_lynx_pcs_create);
+
+void mdio_lynx_pcs_free(struct mdio_lynx_pcs *pcs)
+{
+	kfree(pcs);
+}
+EXPORT_SYMBOL(mdio_lynx_pcs_free);
diff --git a/include/linux/mdio-lynx-pcs.h b/include/linux/mdio-lynx-pcs.h
new file mode 100644
index 000000000000..480ae2e2ecd8
--- /dev/null
+++ b/include/linux/mdio-lynx-pcs.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
+/* Copyright 2020 NXP
+ * Lynx PCS MDIO helpers
+ */
+
+#ifndef __LINUX_MDIO_LYNX_PCS_H
+#define __LINUX_MDIO_LYNX_PCS_H
+
+#include <linux/phy.h>
+#include <linux/mdio.h>
+
+struct mdio_lynx_pcs {
+	struct mdio_device *dev;
+
+	void (*an_restart)(struct mdio_lynx_pcs *pcs, phy_interface_t ifmode);
+
+	void (*get_state)(struct mdio_lynx_pcs *pcs, phy_interface_t ifmode,
+			  struct phylink_link_state *state);
+
+	int (*config)(struct mdio_lynx_pcs *pcs, unsigned int mode,
+		      phy_interface_t ifmode,
+		      const unsigned long *advertising);
+
+	void (*link_up)(struct mdio_lynx_pcs *pcs, unsigned int mode,
+			phy_interface_t ifmode, int speed, int duplex);
+};
+
+#if IS_ENABLED(CONFIG_MDIO_LYNX_PCS)
+struct mdio_lynx_pcs *mdio_lynx_pcs_create(struct mdio_device *mdio_dev);
+
+void mdio_lynx_pcs_free(struct mdio_lynx_pcs *pcs);
+#else
+static struct mdio_lynx_pcs *mdio_lynx_pcs_create(struct mdio_device *mdio_dev)
+{
+	return NULL;
+}
+
+static void mdio_lynx_pcs_free(struct mdio_lynx_pcs *pcs)
+{
+}
+#endif
+
+#endif /* __LINUX_MDIO_LYNX_PCS_H */
-- 
2.25.1

[PATCH net-next 5/5] net: dsa: felix: use the Lynx PCS helpers

[Ioana Ciornei]

Use the helper functions introduced by the newly added
Lynx PCS MDIO module.

Instead of representing the PCS as a phy_device, a mdio_device structure
will be passed to the Lynx module which is now actually implementing all
the PCS configuration and status reporting.

All code previously used for PCS momnitoring and runtime configuration
is removed and replaced will calls to the Lynx PCS operations.

Tested on the following SERDES protocols of LS1028A: 0x7777
(2500Base-X), 0x85bb (QSGMII), 0x9999 (SGMII) and 0x13bb (USXGMII).

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/Kconfig         |   1 +
 drivers/net/dsa/ocelot/felix.c         |   5 +
 drivers/net/dsa/ocelot/felix.h         |   7 +-
 drivers/net/dsa/ocelot/felix_vsc9959.c | 385 +++----------------------
 include/linux/fsl/enetc_mdio.h         |  21 --
 5 files changed, 52 insertions(+), 367 deletions(-)

diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
index a5b7cca03d09..d6bdb511aac5 100644
--- a/drivers/net/dsa/ocelot/Kconfig
+++ b/drivers/net/dsa/ocelot/Kconfig
@@ -7,6 +7,7 @@ config NET_DSA_MSCC_FELIX
 	select MSCC_OCELOT_SWITCH
 	select NET_DSA_TAG_OCELOT
 	select FSL_ENETC_MDIO
+	select MDIO_LYNX_PCS
 	help
 	  This driver supports the VSC9959 network switch, which is a member of
 	  the Vitesse / Microsemi / Microchip Ocelot family of switching cores.
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 66648986e6e3..7995695fae0a 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -277,6 +277,7 @@ static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
 {
 	struct ocelot *ocelot = ds->priv;
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	struct felix *felix = ocelot_to_felix(ocelot);
 
 	/* Enable MAC module */
 	ocelot_port_writel(ocelot_port, DEV_MAC_ENA_CFG_RX_ENA |
@@ -295,6 +296,10 @@ static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
 			 QSYS_SWITCH_PORT_MODE_SCH_NEXT_CFG(1) |
 			 QSYS_SWITCH_PORT_MODE_PORT_ENA,
 			 QSYS_SWITCH_PORT_MODE, port);
+
+	if (felix->info->pcs_link_up)
+		felix->info->pcs_link_up(ocelot, port, link_an_mode, interface,
+					 speed, duplex);
 }
 
 static void felix_port_qos_map_init(struct ocelot *ocelot, int port)
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index a891736ca006..81d93bfee23b 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -4,6 +4,8 @@
 #ifndef _MSCC_FELIX_H
 #define _MSCC_FELIX_H
 
+#include <linux/mdio-lynx-pcs.h>
+
 #define ocelot_to_felix(o)		container_of((o), struct felix, ocelot)
 #define FELIX_NUM_TC			8
 
@@ -34,6 +36,9 @@ struct felix_info {
 	void	(*pcs_an_restart)(struct ocelot *ocelot, int port);
 	void	(*pcs_link_state)(struct ocelot *ocelot, int port,
 				  struct phylink_link_state *state);
+	void	(*pcs_link_up)(struct ocelot *ocelot, int port,
+			       unsigned int mode, phy_interface_t interface,
+			       int speed, int duplex);
 	int	(*prevalidate_phy_mode)(struct ocelot *ocelot, int port,
 					phy_interface_t phy_mode);
 	int	(*port_setup_tc)(struct dsa_switch *ds, int port,
@@ -55,7 +60,7 @@ struct felix {
 	struct felix_info		*info;
 	struct ocelot			ocelot;
 	struct mii_bus			*imdio;
-	struct phy_device		**pcs;
+	struct mdio_lynx_pcs		**pcs;
 };
 
 #endif
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 1dd9e348152d..6f18fd4ea44a 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -17,19 +17,6 @@
 #define VSC9959_VCAP_IS2_ENTRY_WIDTH	376
 #define VSC9959_VCAP_PORT_CNT		6
 
-/* TODO: should find a better place for these */
-#define USXGMII_BMCR_RESET		BIT(15)
-#define USXGMII_BMCR_AN_EN		BIT(12)
-#define USXGMII_BMCR_RST_AN		BIT(9)
-#define USXGMII_BMSR_LNKS(status)	(((status) & GENMASK(2, 2)) >> 2)
-#define USXGMII_BMSR_AN_CMPL(status)	(((status) & GENMASK(5, 5)) >> 5)
-#define USXGMII_ADVERTISE_LNKS(x)	(((x) << 15) & BIT(15))
-#define USXGMII_ADVERTISE_FDX		BIT(12)
-#define USXGMII_ADVERTISE_SPEED(x)	(((x) << 9) & GENMASK(11, 9))
-#define USXGMII_LPA_LNKS(lpa)		((lpa) >> 15)
-#define USXGMII_LPA_DUPLEX(lpa)		(((lpa) & GENMASK(12, 12)) >> 12)
-#define USXGMII_LPA_SPEED(lpa)		(((lpa) & GENMASK(11, 9)) >> 9)
-
 #define VSC9959_TAS_GCL_ENTRY_MAX	63
 
 enum usxgmii_speed {
@@ -728,181 +715,15 @@ static int vsc9959_reset(struct ocelot *ocelot)
 	return 0;
 }
 
-static void vsc9959_pcs_an_restart_sgmii(struct phy_device *pcs)
-{
-	phy_set_bits(pcs, MII_BMCR, BMCR_ANRESTART);
-}
-
-static void vsc9959_pcs_an_restart_usxgmii(struct phy_device *pcs)
-{
-	phy_write_mmd(pcs, MDIO_MMD_VEND2, MII_BMCR,
-		      USXGMII_BMCR_RESET |
-		      USXGMII_BMCR_AN_EN |
-		      USXGMII_BMCR_RST_AN);
-}
-
 static void vsc9959_pcs_an_restart(struct ocelot *ocelot, int port)
 {
 	struct felix *felix = ocelot_to_felix(ocelot);
-	struct phy_device *pcs = felix->pcs[port];
+	struct mdio_lynx_pcs *pcs = felix->pcs[port];
 
 	if (!pcs)
 		return;
 
-	switch (pcs->interface) {
-	case PHY_INTERFACE_MODE_SGMII:
-	case PHY_INTERFACE_MODE_QSGMII:
-		vsc9959_pcs_an_restart_sgmii(pcs);
-		break;
-	case PHY_INTERFACE_MODE_USXGMII:
-		vsc9959_pcs_an_restart_usxgmii(pcs);
-		break;
-	default:
-		dev_err(ocelot->dev, "Invalid PCS interface type %s\n",
-			phy_modes(pcs->interface));
-		break;
-	}
-}
-
-/* We enable SGMII AN only when the PHY has managed = "in-band-status" in the
- * device tree. If we are in MLO_AN_PHY mode, we program directly state->speed
- * into the PCS, which is retrieved out-of-band over MDIO. This also has the
- * benefit of working with SGMII fixed-links, like downstream switches, where
- * both link partners attempt to operate as AN slaves and therefore AN never
- * completes.  But it also has the disadvantage that some PHY chips don't pass
- * traffic if SGMII AN is enabled but not completed (acknowledged by us), so
- * setting MLO_AN_INBAND is actually required for those.
- */
-static void vsc9959_pcs_init_sgmii(struct phy_device *pcs,
-				   unsigned int link_an_mode,
-				   const struct phylink_link_state *state)
-{
-	if (link_an_mode == MLO_AN_INBAND) {
-		int bmsr, bmcr;
-
-		/* Some PHYs like VSC8234 don't like it when AN restarts on
-		 * their system  side and they restart line side AN too, going
-		 * into an endless link up/down loop.  Don't restart PCS AN if
-		 * link is up already.
-		 * We do check that AN is enabled just in case this is the 1st
-		 * call, PCS detects a carrier but AN is disabled from power on
-		 * or by boot loader.
-		 */
-		bmcr = phy_read(pcs, MII_BMCR);
-		if (bmcr < 0)
-			return;
-
-		bmsr = phy_read(pcs, MII_BMSR);
-		if (bmsr < 0)
-			return;
-
-		if ((bmcr & BMCR_ANENABLE) && (bmsr & BMSR_LSTATUS))
-			return;
-
-		/* SGMII spec requires tx_config_Reg[15:0] to be exactly 0x4001
-		 * for the MAC PCS in order to acknowledge the AN.
-		 */
-		phy_write(pcs, MII_ADVERTISE, ADVERTISE_SGMII |
-					      ADVERTISE_LPACK);
-
-		phy_write(pcs, ENETC_PCS_IF_MODE,
-			  ENETC_PCS_IF_MODE_SGMII_EN |
-			  ENETC_PCS_IF_MODE_USE_SGMII_AN);
-
-		/* Adjust link timer for SGMII */
-		phy_write(pcs, ENETC_PCS_LINK_TIMER1,
-			  ENETC_PCS_LINK_TIMER1_VAL);
-		phy_write(pcs, ENETC_PCS_LINK_TIMER2,
-			  ENETC_PCS_LINK_TIMER2_VAL);
-
-		phy_write(pcs, MII_BMCR, BMCR_ANRESTART | BMCR_ANENABLE);
-	} else {
-		int speed;
-
-		if (state->duplex == DUPLEX_HALF) {
-			phydev_err(pcs, "Half duplex not supported\n");
-			return;
-		}
-		switch (state->speed) {
-		case SPEED_1000:
-			speed = ENETC_PCS_SPEED_1000;
-			break;
-		case SPEED_100:
-			speed = ENETC_PCS_SPEED_100;
-			break;
-		case SPEED_10:
-			speed = ENETC_PCS_SPEED_10;
-			break;
-		case SPEED_UNKNOWN:
-			/* Silently don't do anything */
-			return;
-		default:
-			phydev_err(pcs, "Invalid PCS speed %d\n", state->speed);
-			return;
-		}
-
-		phy_write(pcs, ENETC_PCS_IF_MODE,
-			  ENETC_PCS_IF_MODE_SGMII_EN |
-			  ENETC_PCS_IF_MODE_SGMII_SPEED(speed));
-
-		/* Yes, not a mistake: speed is given by IF_MODE. */
-		phy_write(pcs, MII_BMCR, BMCR_RESET |
-					 BMCR_SPEED1000 |
-					 BMCR_FULLDPLX);
-	}
-}
-
-/* 2500Base-X is SerDes protocol 7 on Felix and 6 on ENETC. It is a SerDes lane
- * clocked at 3.125 GHz which encodes symbols with 8b/10b and does not have
- * auto-negotiation of any link parameters. Electrically it is compatible with
- * a single lane of XAUI.
- * The hardware reference manual wants to call this mode SGMII, but it isn't
- * really, since the fundamental features of SGMII:
- * - Downgrading the link speed by duplicating symbols
- * - Auto-negotiation
- * are not there.
- * The speed is configured at 1000 in the IF_MODE and BMCR MDIO registers
- * because the clock frequency is actually given by a PLL configured in the
- * Reset Configuration Word (RCW).
- * Since there is no difference between fixed speed SGMII w/o AN and 802.3z w/o
- * AN, we call this PHY interface type 2500Base-X. In case a PHY negotiates a
- * lower link speed on line side, the system-side interface remains fixed at
- * 2500 Mbps and we do rate adaptation through pause frames.
- */
-static void vsc9959_pcs_init_2500basex(struct phy_device *pcs,
-				       unsigned int link_an_mode,
-				       const struct phylink_link_state *state)
-{
-	if (link_an_mode == MLO_AN_INBAND) {
-		phydev_err(pcs, "AN not supported on 3.125GHz SerDes lane\n");
-		return;
-	}
-
-	phy_write(pcs, ENETC_PCS_IF_MODE,
-		  ENETC_PCS_IF_MODE_SGMII_EN |
-		  ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_2500));
-
-	phy_write(pcs, MII_BMCR, BMCR_SPEED1000 |
-				 BMCR_FULLDPLX |
-				 BMCR_RESET);
-}
-
-static void vsc9959_pcs_init_usxgmii(struct phy_device *pcs,
-				     unsigned int link_an_mode,
-				     const struct phylink_link_state *state)
-{
-	if (link_an_mode != MLO_AN_INBAND) {
-		phydev_err(pcs, "USXGMII only supports in-band AN for now\n");
-		return;
-	}
-
-	/* Configure device ability for the USXGMII Replicator */
-	phy_write_mmd(pcs, MDIO_MMD_VEND2, MII_ADVERTISE,
-		      USXGMII_ADVERTISE_SPEED(USXGMII_SPEED_2500) |
-		      USXGMII_ADVERTISE_LNKS(1) |
-		      ADVERTISE_SGMII |
-		      ADVERTISE_LPACK |
-		      USXGMII_ADVERTISE_FDX);
+	pcs->an_restart(pcs, ocelot->ports[port]->phy_mode);
 }
 
 static void vsc9959_pcs_init(struct ocelot *ocelot, int port,
@@ -910,178 +731,37 @@ static void vsc9959_pcs_init(struct ocelot *ocelot, int port,
 			     const struct phylink_link_state *state)
 {
 	struct felix *felix = ocelot_to_felix(ocelot);
-	struct phy_device *pcs = felix->pcs[port];
+	struct mdio_lynx_pcs *pcs = felix->pcs[port];
 
 	if (!pcs)
 		return;
 
-	/* The PCS does not implement the BMSR register fully, so capability
-	 * detection via genphy_read_abilities does not work. Since we can get
-	 * the PHY config word from the LPA register though, there is still
-	 * value in using the generic phy_resolve_aneg_linkmode function. So
-	 * populate the supported and advertising link modes manually here.
-	 */
-	linkmode_set_bit_array(phy_basic_ports_array,
-			       ARRAY_SIZE(phy_basic_ports_array),
-			       pcs->supported);
-	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, pcs->supported);
-	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, pcs->supported);
-	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, pcs->supported);
-	if (pcs->interface == PHY_INTERFACE_MODE_2500BASEX ||
-	    pcs->interface == PHY_INTERFACE_MODE_USXGMII)
-		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
-				 pcs->supported);
-	if (pcs->interface != PHY_INTERFACE_MODE_2500BASEX)
-		linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
-				 pcs->supported);
-	phy_advertise_supported(pcs);
-
-	switch (pcs->interface) {
-	case PHY_INTERFACE_MODE_SGMII:
-	case PHY_INTERFACE_MODE_QSGMII:
-		vsc9959_pcs_init_sgmii(pcs, link_an_mode, state);
-		break;
-	case PHY_INTERFACE_MODE_2500BASEX:
-		vsc9959_pcs_init_2500basex(pcs, link_an_mode, state);
-		break;
-	case PHY_INTERFACE_MODE_USXGMII:
-		vsc9959_pcs_init_usxgmii(pcs, link_an_mode, state);
-		break;
-	default:
-		dev_err(ocelot->dev, "Unsupported link mode %s\n",
-			phy_modes(pcs->interface));
-	}
-}
-
-static void vsc9959_pcs_link_state_resolve(struct phy_device *pcs,
-					   struct phylink_link_state *state)
-{
-	state->an_complete = pcs->autoneg_complete;
-	state->an_enabled = pcs->autoneg;
-	state->link = pcs->link;
-	state->duplex = pcs->duplex;
-	state->speed = pcs->speed;
-	/* SGMII AN does not negotiate flow control, but that's ok,
-	 * since phylink already knows that, and does:
-	 *	link_state.pause |= pl->phy_state.pause;
-	 */
-	state->pause = MLO_PAUSE_NONE;
-
-	phydev_dbg(pcs,
-		   "mode=%s/%s/%s adv=%*pb lpa=%*pb link=%u an_enabled=%u an_complete=%u\n",
-		   phy_modes(pcs->interface),
-		   phy_speed_to_str(pcs->speed),
-		   phy_duplex_to_str(pcs->duplex),
-		   __ETHTOOL_LINK_MODE_MASK_NBITS, pcs->advertising,
-		   __ETHTOOL_LINK_MODE_MASK_NBITS, pcs->lp_advertising,
-		   pcs->link, pcs->autoneg, pcs->autoneg_complete);
+	pcs->config(pcs, link_an_mode, state->interface, state->advertising);
 }
 
-static void vsc9959_pcs_link_state_sgmii(struct phy_device *pcs,
-					 struct phylink_link_state *state)
-{
-	int err;
-
-	err = genphy_update_link(pcs);
-	if (err < 0)
-		return;
-
-	if (pcs->autoneg_complete) {
-		u16 lpa = phy_read(pcs, MII_LPA);
-
-		mii_lpa_to_linkmode_lpa_sgmii(pcs->lp_advertising, lpa);
-
-		phy_resolve_aneg_linkmode(pcs);
-	}
-}
-
-static void vsc9959_pcs_link_state_2500basex(struct phy_device *pcs,
-					     struct phylink_link_state *state)
+static void vsc9959_pcs_link_state(struct ocelot *ocelot, int port,
+				   struct phylink_link_state *state)
 {
-	int err;
+	struct felix *felix = ocelot_to_felix(ocelot);
+	struct mdio_lynx_pcs *pcs = felix->pcs[port];
 
-	err = genphy_update_link(pcs);
-	if (err < 0)
+	if (!pcs)
 		return;
 
-	pcs->speed = SPEED_2500;
-	pcs->asym_pause = true;
-	pcs->pause = true;
+	pcs->get_state(pcs, ocelot->ports[port]->phy_mode, state);
 }
 
-static void vsc9959_pcs_link_state_usxgmii(struct phy_device *pcs,
-					   struct phylink_link_state *state)
-{
-	int status, lpa;
-
-	status = phy_read_mmd(pcs, MDIO_MMD_VEND2, MII_BMSR);
-	if (status < 0)
-		return;
-
-	pcs->autoneg = true;
-	pcs->autoneg_complete = USXGMII_BMSR_AN_CMPL(status);
-	pcs->link = USXGMII_BMSR_LNKS(status);
-
-	if (!pcs->link || !pcs->autoneg_complete)
-		return;
-
-	lpa = phy_read_mmd(pcs, MDIO_MMD_VEND2, MII_LPA);
-	if (lpa < 0)
-		return;
-
-	switch (USXGMII_LPA_SPEED(lpa)) {
-	case USXGMII_SPEED_10:
-		pcs->speed = SPEED_10;
-		break;
-	case USXGMII_SPEED_100:
-		pcs->speed = SPEED_100;
-		break;
-	case USXGMII_SPEED_1000:
-		pcs->speed = SPEED_1000;
-		break;
-	case USXGMII_SPEED_2500:
-		pcs->speed = SPEED_2500;
-		break;
-	default:
-		break;
-	}
-
-	if (USXGMII_LPA_DUPLEX(lpa))
-		pcs->duplex = DUPLEX_FULL;
-	else
-		pcs->duplex = DUPLEX_HALF;
-}
-
-static void vsc9959_pcs_link_state(struct ocelot *ocelot, int port,
-				   struct phylink_link_state *state)
+static void vsc9959_pcs_link_up(struct ocelot *ocelot, int port,
+				unsigned int mode, phy_interface_t interface,
+				int speed, int duplex)
 {
 	struct felix *felix = ocelot_to_felix(ocelot);
-	struct phy_device *pcs = felix->pcs[port];
+	struct mdio_lynx_pcs *pcs = felix->pcs[port];
 
 	if (!pcs)
 		return;
 
-	pcs->speed = SPEED_UNKNOWN;
-	pcs->duplex = DUPLEX_UNKNOWN;
-	pcs->pause = 0;
-	pcs->asym_pause = 0;
-
-	switch (pcs->interface) {
-	case PHY_INTERFACE_MODE_SGMII:
-	case PHY_INTERFACE_MODE_QSGMII:
-		vsc9959_pcs_link_state_sgmii(pcs, state);
-		break;
-	case PHY_INTERFACE_MODE_2500BASEX:
-		vsc9959_pcs_link_state_2500basex(pcs, state);
-		break;
-	case PHY_INTERFACE_MODE_USXGMII:
-		vsc9959_pcs_link_state_usxgmii(pcs, state);
-		break;
-	default:
-		return;
-	}
-
-	vsc9959_pcs_link_state_resolve(pcs, state);
+	pcs->link_up(pcs, mode, interface, speed, duplex);
 }
 
 static int vsc9959_prevalidate_phy_mode(struct ocelot *ocelot, int port,
@@ -1130,6 +810,14 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 		return -ENOMEM;
 	}
 
+	felix->pcs = devm_kcalloc(dev, felix->info->num_ports,
+				  sizeof(struct mdio_lynx_pcs *),
+				  GFP_KERNEL);
+	if (!felix->pcs) {
+		dev_err(dev, "failed to allocate array for Lynx PCS devices\n");
+		return -ENOMEM;
+	}
+
 	imdio_base = pci_resource_start(felix->pdev,
 					felix->info->imdio_pci_bar);
 
@@ -1177,18 +865,23 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 
 	for (port = 0; port < felix->info->num_ports; port++) {
 		struct ocelot_port *ocelot_port = ocelot->ports[port];
-		struct phy_device *pcs;
-		bool is_c45 = false;
+		struct mdio_device *pcs_mdio;
 
-		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_USXGMII)
-			is_c45 = true;
+		if (dsa_is_unused_port(felix->ds, port))
+			continue;
+
+		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
+			continue;
 
-		pcs = get_phy_device(felix->imdio, port, is_c45);
-		if (IS_ERR(pcs))
+		pcs_mdio = mdio_device_create(felix->imdio, port);
+		if (IS_ERR(pcs_mdio))
 			continue;
 
-		pcs->interface = ocelot_port->phy_mode;
-		felix->pcs[port] = pcs;
+		felix->pcs[port] = mdio_lynx_pcs_create(pcs_mdio);
+		if (!felix->pcs[port]) {
+			mdio_device_free(pcs_mdio);
+			continue;
+		}
 
 		dev_info(dev, "Found PCS at internal MDIO address %d\n", port);
 	}
@@ -1202,12 +895,13 @@ static void vsc9959_mdio_bus_free(struct ocelot *ocelot)
 	int port;
 
 	for (port = 0; port < ocelot->num_phys_ports; port++) {
-		struct phy_device *pcs = felix->pcs[port];
+		struct mdio_lynx_pcs *pcs = felix->pcs[port];
 
 		if (!pcs)
 			continue;
 
-		put_device(&pcs->mdio.dev);
+		mdio_device_free(pcs->dev);
+		mdio_lynx_pcs_free(pcs);
 	}
 	mdiobus_unregister(felix->imdio);
 }
@@ -1415,6 +1109,7 @@ struct felix_info felix_info_vsc9959 = {
 	.pcs_init		= vsc9959_pcs_init,
 	.pcs_an_restart		= vsc9959_pcs_an_restart,
 	.pcs_link_state		= vsc9959_pcs_link_state,
+	.pcs_link_up		= vsc9959_pcs_link_up,
 	.prevalidate_phy_mode	= vsc9959_prevalidate_phy_mode,
 	.port_setup_tc          = vsc9959_port_setup_tc,
 	.port_sched_speed_set   = vsc9959_sched_speed_set,
diff --git a/include/linux/fsl/enetc_mdio.h b/include/linux/fsl/enetc_mdio.h
index 4875dd38af7e..483679f53a91 100644
--- a/include/linux/fsl/enetc_mdio.h
+++ b/include/linux/fsl/enetc_mdio.h
@@ -6,27 +6,6 @@
 
 #include <linux/phy.h>
 
-/* PCS registers */
-#define ENETC_PCS_LINK_TIMER1			0x12
-#define ENETC_PCS_LINK_TIMER1_VAL		0x06a0
-#define ENETC_PCS_LINK_TIMER2			0x13
-#define ENETC_PCS_LINK_TIMER2_VAL		0x0003
-#define ENETC_PCS_IF_MODE			0x14
-#define ENETC_PCS_IF_MODE_SGMII_EN		BIT(0)
-#define ENETC_PCS_IF_MODE_USE_SGMII_AN		BIT(1)
-#define ENETC_PCS_IF_MODE_SGMII_SPEED(x)	(((x) << 2) & GENMASK(3, 2))
-
-/* Not a mistake, the SerDes PLL needs to be set at 3.125 GHz by Reset
- * Configuration Word (RCW, outside Linux control) for 2.5G SGMII mode. The PCS
- * still thinks it's at gigabit.
- */
-enum enetc_pcs_speed {
-	ENETC_PCS_SPEED_10	= 0,
-	ENETC_PCS_SPEED_100	= 1,
-	ENETC_PCS_SPEED_1000	= 2,
-	ENETC_PCS_SPEED_2500	= 2,
-};
-
 struct enetc_hw;
 
 struct enetc_mdio_priv {
-- 
2.25.1

Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Russell King - ARM Linux admin]

On Thu, Jun 18, 2020 at 03:08:36PM +0300, Ioana Ciornei wrote:
> Add a Lynx PCS MDIO module which exposes the necessary operations to
> drive the PCS using PHYLINK.
> 
> The majority of the code is extracted from the Felix DSA driver, which
> will be also modified in a later patch, and exposed as a separate module
> for code reusability purposes.
> 
> At the moment, USXGMII (only with in-band AN and speeds up to 2500),
> SGMII, QSGMII and 2500Base-X (only w/o in-band AN) are supported by the
> Lynx PCS MDIO module since these were also supported by Felix.
> 
> The module can only be enabled by the drivers in need and not user
> selectable.

Is this the same PCS found in the LX2160A?  It looks very similar.

> +/* 2500Base-X is SerDes protocol 7 on Felix and 6 on ENETC. It is a SerDes lane
> + * clocked at 3.125 GHz which encodes symbols with 8b/10b and does not have
> + * auto-negotiation of any link parameters. Electrically it is compatible with
> + * a single lane of XAUI.
> + * The hardware reference manual wants to call this mode SGMII, but it isn't
> + * really, since the fundamental features of SGMII:
> + * - Downgrading the link speed by duplicating symbols
> + * - Auto-negotiation
> + * are not there.

I welcome that others are waking up to the industry wide obfuscation of
terminology surrounding "SGMII" and "1000base-X", and calling it out
where it is blatently incorrectly described in documentation.

> + * The speed is configured at 1000 in the IF_MODE because the clock frequency
> + * is actually given by a PLL configured in the Reset Configuration Word (RCW).
> + * Since there is no difference between fixed speed SGMII w/o AN and 802.3z w/o
> + * AN, we call this PHY interface type 2500Base-X. In case a PHY negotiates a
> + * lower link speed on line side, the system-side interface remains fixed at
> + * 2500 Mbps and we do rate adaptation through pause frames.

We have systems that do have AN with 2500base-X however - which is what
you want when you couple two potentially remote systems over a fibre
cable.  The AN in 802.3z (1000base-X) is used to negotiate:

- duplex
- pause mode

although in practice, half-duplex is not supported by lots of hardware,
which leaves just pause mode.  It is useful to have pause mode
negotiation remain present, whether it's 1000base-X or 2500base-X, but
obviously within the hardware boundaries.

I suspect the hardware is capable of supporting 802.3z AN when operating
at 2500base-X, but not the SGMII symbol duplication for slower speeds.

> +struct mdio_lynx_pcs *mdio_lynx_pcs_create(struct mdio_device *mdio_dev)
> +{
> +	struct mdio_lynx_pcs *pcs;
> +
> +	if (WARN_ON(!mdio_dev))
> +		return NULL;
> +
> +	pcs = kzalloc(sizeof(*pcs), GFP_KERNEL);
> +	if (!pcs)
> +		return NULL;
> +
> +	pcs->dev = mdio_dev;
> +	pcs->an_restart = lynx_pcs_an_restart;
> +	pcs->get_state = lynx_pcs_get_state;
> +	pcs->link_up = lynx_pcs_link_up;
> +	pcs->config = lynx_pcs_config;

We really should not have these private structure interfaces.  Private
structure-based driver specific interfaces really don't constitute a
sane approach to interface design.

Would it work if there was a "struct mdio_device" add to the
phylink_config structure, and then you could have the phylink_pcs_ops
embedded into this driver?

If not, then we need some kind of mdio_pcs_device that offers this
kind of functionality.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Jakub Kicinski]

On Thu, 18 Jun 2020 15:08:36 +0300 Ioana Ciornei wrote:
> +#if IS_ENABLED(CONFIG_MDIO_LYNX_PCS)
> +struct mdio_lynx_pcs *mdio_lynx_pcs_create(struct mdio_device *mdio_dev);
> +
> +void mdio_lynx_pcs_free(struct mdio_lynx_pcs *pcs);
> +#else
> +static struct mdio_lynx_pcs *mdio_lynx_pcs_create(struct mdio_device *mdio_dev)
> +{
> +	return NULL;
> +}
> +
> +static void mdio_lynx_pcs_free(struct mdio_lynx_pcs *pcs)
> +{
> +}
> +#endif

Do you want these to be static inline?

Re: [PATCH net-next 5/5] net: dsa: felix: use the Lynx PCS helpers

[Jakub Kicinski]

On Thu, 18 Jun 2020 15:08:37 +0300 Ioana Ciornei wrote:
> Use the helper functions introduced by the newly added
> Lynx PCS MDIO module.
> 
> Instead of representing the PCS as a phy_device, a mdio_device structure
> will be passed to the Lynx module which is now actually implementing all
> the PCS configuration and status reporting.
> 
> All code previously used for PCS momnitoring and runtime configuration
> is removed and replaced will calls to the Lynx PCS operations.
> 
> Tested on the following SERDES protocols of LS1028A: 0x7777
> (2500Base-X), 0x85bb (QSGMII), 0x9999 (SGMII) and 0x13bb (USXGMII).
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Hm, this does not build with allmodconfig.

drivers/net/phy/mdio-lynx-pcs.o: In function `lynx_pcs_link_up':
mdio-lynx-pcs.c:(.text+0x115): undefined reference to `mdiobus_modify'
mdio-lynx-pcs.c:(.text+0x1a3): undefined reference to `mdiobus_write'
drivers/net/phy/mdio-lynx-pcs.o: In function `lynx_pcs_config':
mdio-lynx-pcs.c:(.text+0x33a): undefined reference to `mdiobus_write'
mdio-lynx-pcs.c:(.text+0x371): undefined reference to `mdiobus_modify'
mdio-lynx-pcs.c:(.text+0x384): undefined reference to `phylink_mii_c22_pcs_config'
mdio-lynx-pcs.c:(.text+0x3e4): undefined reference to `mdiobus_write'
mdio-lynx-pcs.c:(.text+0x40d): undefined reference to `mdiobus_write'
mdio-lynx-pcs.c:(.text+0x422): undefined reference to `mdiobus_write'
drivers/net/phy/mdio-lynx-pcs.o: In function `lynx_pcs_get_state_usxgmii.isra.0':
mdio-lynx-pcs.c:(.text+0x457): undefined reference to `mdiobus_read'
mdio-lynx-pcs.c:(.text+0x4f1): undefined reference to `mdiobus_read'
drivers/net/phy/mdio-lynx-pcs.o: In function `lynx_pcs_get_state':
mdio-lynx-pcs.c:(.text+0x650): undefined reference to `phylink_mii_c22_pcs_get_state'
mdio-lynx-pcs.c:(.text+0x6fb): undefined reference to `phy_duplex_to_str'
mdio-lynx-pcs.c:(.text+0x711): undefined reference to `phy_speed_to_str'
mdio-lynx-pcs.c:(.text+0x7c0): undefined reference to `mdiobus_read'
mdio-lynx-pcs.c:(.text+0x7d4): undefined reference to `mdiobus_read'
drivers/net/phy/mdio-lynx-pcs.o: In function `lynx_pcs_an_restart':
mdio-lynx-pcs.c:(.text+0x954): undefined reference to `mdiobus_write'
mdio-lynx-pcs.c:(.text+0x978): undefined reference to `phylink_mii_c22_pcs_an_restart'
make[1]: *** [vmlinux] Error 1
make: *** [__sub-make] Error 2

RE: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Ioana Ciornei]

> Subject: Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module
> 
> On Thu, Jun 18, 2020 at 03:08:36PM +0300, Ioana Ciornei wrote:
> > Add a Lynx PCS MDIO module which exposes the necessary operations to
> > drive the PCS using PHYLINK.
> >
> > The majority of the code is extracted from the Felix DSA driver, which
> > will be also modified in a later patch, and exposed as a separate
> > module for code reusability purposes.
> >
> > At the moment, USXGMII (only with in-band AN and speeds up to 2500),
> > SGMII, QSGMII and 2500Base-X (only w/o in-band AN) are supported by
> > the Lynx PCS MDIO module since these were also supported by Felix.
> >
> > The module can only be enabled by the drivers in need and not user
> > selectable.
> 
> Is this the same PCS found in the LX2160A?  It looks very similar.
> 

Yes, it is.
I already tested these protocols on LX2160A (and some other DPAA2 SoCs).
The idea is to have this patch set without any functional changes accepted and
then I will wire up dpaa2-eth as well into this.

> > +/* 2500Base-X is SerDes protocol 7 on Felix and 6 on ENETC. It is a
> > +SerDes lane
> > + * clocked at 3.125 GHz which encodes symbols with 8b/10b and does
> > +not have
> > + * auto-negotiation of any link parameters. Electrically it is
> > +compatible with
> > + * a single lane of XAUI.
> > + * The hardware reference manual wants to call this mode SGMII, but
> > +it isn't
> > + * really, since the fundamental features of SGMII:
> > + * - Downgrading the link speed by duplicating symbols
> > + * - Auto-negotiation
> > + * are not there.
> 
> I welcome that others are waking up to the industry wide obfuscation of
> terminology surrounding "SGMII" and "1000base-X", and calling it out where it is
> blatently incorrectly described in documentation.

I will not take the credit for this since this is mainly just a comment being moved.

> 
> > + * The speed is configured at 1000 in the IF_MODE because the clock
> > + frequency
> > + * is actually given by a PLL configured in the Reset Configuration Word
> (RCW).
> > + * Since there is no difference between fixed speed SGMII w/o AN and
> > + 802.3z w/o
> > + * AN, we call this PHY interface type 2500Base-X. In case a PHY
> > + negotiates a
> > + * lower link speed on line side, the system-side interface remains
> > + fixed at
> > + * 2500 Mbps and we do rate adaptation through pause frames.
> 
> We have systems that do have AN with 2500base-X however - which is what you
> want when you couple two potentially remote systems over a fibre cable.  The
> AN in 802.3z (1000base-X) is used to negotiate:
> 
> - duplex
> - pause mode
> 
> although in practice, half-duplex is not supported by lots of hardware, which
> leaves just pause mode.  It is useful to have pause mode negotiation remain
> present, whether it's 1000base-X or 2500base-X, but obviously within the
> hardware boundaries.
> 
> I suspect the hardware is capable of supporting 802.3z AN when operating at
> 2500base-X, but not the SGMII symbol duplication for slower speeds.
> 

I don't have a definitive answer to this this right now, I'll have to actually test this
if I can get my hands on some hardware for this setup.

> > +struct mdio_lynx_pcs *mdio_lynx_pcs_create(struct mdio_device
> > +*mdio_dev) {
> > +	struct mdio_lynx_pcs *pcs;
> > +
> > +	if (WARN_ON(!mdio_dev))
> > +		return NULL;
> > +
> > +	pcs = kzalloc(sizeof(*pcs), GFP_KERNEL);
> > +	if (!pcs)
> > +		return NULL;
> > +
> > +	pcs->dev = mdio_dev;
> > +	pcs->an_restart = lynx_pcs_an_restart;
> > +	pcs->get_state = lynx_pcs_get_state;
> > +	pcs->link_up = lynx_pcs_link_up;
> > +	pcs->config = lynx_pcs_config;
> 
> We really should not have these private structure interfaces.  Private structure-
> based driver specific interfaces really don't constitute a sane approach to
> interface design.
> 
> Would it work if there was a "struct mdio_device" add to the phylink_config
> structure, and then you could have the phylink_pcs_ops embedded into this
> driver?

I think that would restrict too much the usage.
I am afraid there will be instances where the PCS is not recognizable by PHY_ID,
thus no way of knowing which driver to probe which mdio_device.
Also, I would leave to the driver the choice of using (or not) the functions 
exported by Lynx.

> 
> If not, then we need some kind of mdio_pcs_device that offers this kind of
> functionality.
> 

Maybe we can meet in the middle?

What if we directly export the helper functions directly as symbols which can
be used by the driver without any mdio_lynx_pcs in the middle
(just the mdio_device passed to the function).
This would be exactly as phylink_mii_c22_pcs_[an_restart/config] are currently
used.

We can somehow standardize the functions prototypes (which will likely mean
mdio_device instead of the phylink_pcs_ops's phylink_config).

Ioana

Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Russell King - ARM Linux admin]

On Thu, Jun 18, 2020 at 04:17:56PM +0000, Ioana Ciornei wrote:
> > > +struct mdio_lynx_pcs *mdio_lynx_pcs_create(struct mdio_device
> > > +*mdio_dev) {
> > > +	struct mdio_lynx_pcs *pcs;
> > > +
> > > +	if (WARN_ON(!mdio_dev))
> > > +		return NULL;
> > > +
> > > +	pcs = kzalloc(sizeof(*pcs), GFP_KERNEL);
> > > +	if (!pcs)
> > > +		return NULL;
> > > +
> > > +	pcs->dev = mdio_dev;
> > > +	pcs->an_restart = lynx_pcs_an_restart;
> > > +	pcs->get_state = lynx_pcs_get_state;
> > > +	pcs->link_up = lynx_pcs_link_up;
> > > +	pcs->config = lynx_pcs_config;
> > 
> > We really should not have these private structure interfaces.  Private structure-
> > based driver specific interfaces really don't constitute a sane approach to
> > interface design.
> > 
> > Would it work if there was a "struct mdio_device" add to the phylink_config
> > structure, and then you could have the phylink_pcs_ops embedded into this
> > driver?
> 
> I think that would restrict too much the usage.
> I am afraid there will be instances where the PCS is not recognizable by PHY_ID,
> thus no way of knowing which driver to probe which mdio_device.
> Also, I would leave to the driver the choice of using (or not) the functions 
> exported by Lynx.

I think you've taken my point way too far.  What I'm complaining about
is the indirection of lynx_pcs_an_restart() et.al. through a driver-
private structure.

In order to access lynx_pcs_an_restart(), we need to dereference
struct mdio_lynx_pcs, which is a structure specific to this lynx PCS
driver.  What this leads to is users doing this:

	if (pcs_is_lynx) {
		struct mdio_lynx_pcs *pcs = foo->bar;

		pcs->an_restart(...);
	} else if (pcs_is_something_else) {
		struct mdio_somethingelse_pcs *pcs = foo->bar;

		pcs->an_restart(...);
	}

which really does not scale.  A step forward would be:

	if (pcs_is_lynx) {
		lynx_pcs_an_restart(...);
	} else if (pcs_is_something_else) {
		something_else_pcs_an_restart(...);
	}

but that also scales horribly.

Even better would be to have a generic set of operations for PCS
devices that can be declared in the lynx PCS driver and used
externally... like, maybe struct phylink_pcs_ops, which is made
globally visible for MAC drivers to use with phylink_add_pcs().

Or maybe a function in this lynx PCS driver that calls phylink_add_pcs()
itself with its own PCS operations, and maybe also sets a field in
struct phylink_config for the PCS mdio device?

Or something like that - just some a way that doesn't force us down
a path that we end up forcing people into code that has to decide
what sort of PCS we have at runtime in all these method paths.

> What if we directly export the helper functions directly as symbols which can
> be used by the driver without any mdio_lynx_pcs in the middle
> (just the mdio_device passed to the function).
> This would be exactly as phylink_mii_c22_pcs_[an_restart/config] are currently
> used.

The difference is that phylink_mii_c22_pcs_* are designed as library
functions - functions that are very likely to be re-used for multiple
different PCS (because the format, location, and access method of
these registers is defined by IEEE 802.3).  It's a bit like phylib's
configuration of autoneg - we don't have all the individual drivers
doing that, we have core code that does that for us in the form of
helpers.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Ioana Ciornei]

> Subject: Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module
> 
> On Thu, Jun 18, 2020 at 04:17:56PM +0000, Ioana Ciornei wrote:
> > > > +struct mdio_lynx_pcs *mdio_lynx_pcs_create(struct mdio_device
> > > > +*mdio_dev) {
> > > > +	struct mdio_lynx_pcs *pcs;
> > > > +
> > > > +	if (WARN_ON(!mdio_dev))
> > > > +		return NULL;
> > > > +
> > > > +	pcs = kzalloc(sizeof(*pcs), GFP_KERNEL);
> > > > +	if (!pcs)
> > > > +		return NULL;
> > > > +
> > > > +	pcs->dev = mdio_dev;
> > > > +	pcs->an_restart = lynx_pcs_an_restart;
> > > > +	pcs->get_state = lynx_pcs_get_state;
> > > > +	pcs->link_up = lynx_pcs_link_up;
> > > > +	pcs->config = lynx_pcs_config;
> > >
> > > We really should not have these private structure interfaces.
> > > Private structure- based driver specific interfaces really don't
> > > constitute a sane approach to interface design.
> > >
> > > Would it work if there was a "struct mdio_device" add to the
> > > phylink_config structure, and then you could have the
> > > phylink_pcs_ops embedded into this driver?
> >
> > I think that would restrict too much the usage.
> > I am afraid there will be instances where the PCS is not recognizable
> > by PHY_ID, thus no way of knowing which driver to probe which mdio_device.
> > Also, I would leave to the driver the choice of using (or not) the
> > functions exported by Lynx.
> 
> I think you've taken my point way too far.  What I'm complaining about is the
> indirection of lynx_pcs_an_restart() et.al. through a driver- private structure.
> 
> In order to access lynx_pcs_an_restart(), we need to dereference struct
> mdio_lynx_pcs, which is a structure specific to this lynx PCS driver.  What this
> leads to is users doing this:
> 
> 	if (pcs_is_lynx) {
> 		struct mdio_lynx_pcs *pcs = foo->bar;
> 
> 		pcs->an_restart(...);
> 	} else if (pcs_is_something_else) {
> 		struct mdio_somethingelse_pcs *pcs = foo->bar;
> 
> 		pcs->an_restart(...);
> 	}
> 
> which really does not scale.  A step forward would be:
> 
> 	if (pcs_is_lynx) {
> 		lynx_pcs_an_restart(...);
> 	} else if (pcs_is_something_else) {
> 		something_else_pcs_an_restart(...);
> 	}
> 
> but that also scales horribly.

This is what I was proposing. I can of course take the indirection away
and just export the functions.

Are there really instances where the ethernet driver has to manage multiple
different types of PCSs? I am not sure this type of snippet of code is really
going to occur.

> 
> Even better would be to have a generic set of operations for PCS devices that
> can be declared in the lynx PCS driver and used externally... like, maybe struct
> phylink_pcs_ops, which is made globally visible for MAC drivers to use with
> phylink_add_pcs().
> 
> Or maybe a function in this lynx PCS driver that calls phylink_add_pcs() itself with
> its own PCS operations, and maybe also sets a field in struct phylink_config for
> the PCS mdio device?
>

I am not sure how this would work with Felix and DSA drivers in general since the
DSA core is hiding the phylink_pcs_ops from the actual switch driver.

> Or something like that - just some a way that doesn't force us down a path that
> we end up forcing people into code that has to decide what sort of PCS we have
> at runtime in all these method paths.

I get what you are saying but I do not know of any drivers that actually need this
distinction at runtime.

Ioana

> 
> > What if we directly export the helper functions directly as symbols
> > which can be used by the driver without any mdio_lynx_pcs in the
> > middle (just the mdio_device passed to the function).
> > This would be exactly as phylink_mii_c22_pcs_[an_restart/config] are
> > currently used.
> 
> The difference is that phylink_mii_c22_pcs_* are designed as library functions -
> functions that are very likely to be re-used for multiple different PCS (because
> the format, location, and access method of these registers is defined by IEEE
> 802.3).  It's a bit like phylib's configuration of autoneg - we don't have all the
> individual drivers doing that, we have core code that does that for us in the form
> of helpers.
> 

Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Russell King - ARM Linux admin]

On Thu, Jun 18, 2020 at 05:34:49PM +0000, Ioana Ciornei wrote:
> I am not sure how this would work with Felix and DSA drivers in general
> since the DSA core is hiding the phylink_pcs_ops from the actual switch
> driver.

Here's an idea to work around DSA (untested):

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 1f8e0023f4f4..f578a5bb8ad0 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -67,6 +67,7 @@ enum phylink_op_type {
 struct phylink_config {
 	struct device *dev;
 	enum phylink_op_type type;
+	void *pcs_private;
 	bool pcs_poll;
 	bool poll_fixed_state;
 	void (*get_fixed_state)(struct phylink_config *config,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index a50d5007fd39..0fbb3a542b6e 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -742,6 +742,9 @@ int dsa_port_get_phy_strings(struct dsa_port *dp, uint8_t *data);
 int dsa_port_get_ethtool_phy_stats(struct dsa_port *dp, uint64_t *data);
 int dsa_port_get_phy_sset_count(struct dsa_port *dp);
 void dsa_port_phylink_mac_change(struct dsa_switch *ds, int port, bool up);
+void dsa_slave_attach_phylink_pcs(struct dsa_switch *ds, int port,
+				  const struct phylink_pcs_ops *ops,
+				  void *priv);
 
 struct dsa_tag_driver {
 	const struct dsa_device_ops *ops;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 2ae17f95cb63..c80f62d88b63 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1619,6 +1619,17 @@ static int dsa_slave_phy_connect(struct net_device *slave_dev, int addr)
 	return phylink_connect_phy(dp->pl, slave_dev->phydev);
 }
 
+void dsa_slave_attach_phylink_pcs(struct dsa_switch *ds, int port,
+				  const struct phylink_pcs_ops *ops,
+				  void *priv)
+{
+	const struct dsa_port *dp = dsa_to_port(ds, port);
+
+	dp->pl_config.pcs_private = priv;
+	phylink_add_pcs(dp->pl, ops);
+}
+EXPORT_SYMBOL_GPL(dsa_slave_attach_phylink_pcs);
+
 static int dsa_slave_phy_setup(struct net_device *slave_dev)
 {
 	struct dsa_port *dp = dsa_slave_to_port(slave_dev);


dsa_slave_attach_phylink_pcs() can be passed anything as it's priv
argument - that could be "dp", or it could be a lynx PCS private
data structure, but that's up to the driver code to decide.

At least this gives a way for us to have some standardised PCS
code that can be bolted into either DSA or a MAC driver by the
higher levels without the PCS code having to know which it's
connected to, and without having to have veneers to bridge into
the PCS code.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Andrew Lunn]

> Are there really instances where the ethernet driver has to manage multiple
> different types of PCSs? I am not sure this type of snippet of code is really
> going to occur.

Hi Ioana

The Marvell mv88e6390 family has three PCS's, one for SGMII/1000BaseX,
a 10Gbase-X4/X2 and a 10GBAse-R. So this sort of code could appear.

  Andrew

Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Andrew Lunn]

>  MAINTAINERS                     |   7 +
>  drivers/net/phy/Kconfig         |   6 +
>  drivers/net/phy/Makefile        |   1 +
>  drivers/net/phy/mdio-lynx-pcs.c | 358 ++++++++++++++++++++++++++++++++
>  include/linux/mdio-lynx-pcs.h   |  43 ++++
>  5 files changed, 415 insertions(+)
>  create mode 100644 drivers/net/phy/mdio-lynx-pcs.c
>  create mode 100644 include/linux/mdio-lynx-pcs.h

Hi Ioana

We should think about naming convention here.

All MDIO bus driver, MDIO multiplexors etc use mdio- as a prefix.

This is not a bus driver, so i don't think it should use the mdio-
prefix. How about pcs-lynx.c?

In terms of Kconfig, MDIO_ prefix is used for MDIO bus drivers etc.  I
don't think it is appropriate here. How about PCS_LYNX? I don't think
any other subsystem is using PCS_ as a prefix.

> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -235,6 +235,12 @@ config MDIO_XPCS
>  	  This module provides helper functions for Synopsys DesignWare XPCS
>  	  controllers.
>  
> +config MDIO_LYNX_PCS
> +	bool
> +	help
> +	  This module provides helper functions for Lynx PCS enablement
> +	  representing the PCS as an MDIO device.
> +
>  endif
>  endif

Maybe add this at the end, and add a

comment "PCS device drivers"

before it? I'm assuming with time we will have more of these drivers.

       Andrew

Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Russell King - ARM Linux admin]

On Fri, Jun 19, 2020 at 12:03:31AM +0200, Andrew Lunn wrote:
> > Are there really instances where the ethernet driver has to manage multiple
> > different types of PCSs? I am not sure this type of snippet of code is really
> > going to occur.
> 
> Hi Ioana
> 
> The Marvell mv88e6390 family has three PCS's, one for SGMII/1000BaseX,
> a 10Gbase-X4/X2 and a 10GBAse-R. So this sort of code could appear.

It in fact already does, but only for the 1G and 10GBase-R cases.
We don't have a PHY interface mode for 10Gbase-X4/X2, so I never
added that, but if it happens, we end up with a third case.

It may be tempting to suggest that moving the mv88e6390 PCS support
to common code would be a good idea, but it's really not - the PCS
don't follow IEEE 802.3 register layout.  The 1G registers are in
the PHYXS MMD, following Clause 22 layout at offset 0x2000.  The
10GBASE-R registers are in the PHYXS MMD at offset 0x1000.

So, I don't think it's sensible to compare the mv88e6390 switch
with this; the mv88e6390 serdes PCS is unlikely to be useful
outside of the DSA switch environment.

However, the Lynx PCS appears to be used in a range of different
situations, which include DSA switches and conventional ethernet
drivers.  That means we need some kind of solution where the code
driving the PCS does not rely on the code structures for the
device it is embedded with.

The solution I've suggested for DSA may help us get towards having
generic PCS drivers, but it doesn't fully solve the problem.  I
would ideally like DSA drivers to have access to "struct phylink"
so that they can attach the PCS themselves without having any of
the DSA layering veneers get in the way between phylink and the
PCS code - thereby allowing the PCS code to be re-used cleanly
with a conventional network driver.

Basically, I'm thinking is:

int phylink_attach_pcs(struct phylink *pl, const struct phylink_pcs_ops *ops,
		       void *pcs_priv);

which the PCS driver itself would call when requested to by the
DSA driver or ethernet driver.

Thoughts?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Vladimir Oltean]

Hi Andrew,

On 6/19/20 1:13 AM, Andrew Lunn wrote:
> 
>>   MAINTAINERS                     |   7 +
>>   drivers/net/phy/Kconfig         |   6 +
>>   drivers/net/phy/Makefile        |   1 +
>>   drivers/net/phy/mdio-lynx-pcs.c | 358 ++++++++++++++++++++++++++++++++
>>   include/linux/mdio-lynx-pcs.h   |  43 ++++
>>   5 files changed, 415 insertions(+)
>>   create mode 100644 drivers/net/phy/mdio-lynx-pcs.c
>>   create mode 100644 include/linux/mdio-lynx-pcs.h
> 
> Hi Ioana
> 
> We should think about naming convention here.
> 
> All MDIO bus driver, MDIO multiplexors etc use mdio- as a prefix.
> 
> This is not a bus driver, so i don't think it should use the mdio-
> prefix. How about pcs-lynx.c?
> 
> In terms of Kconfig, MDIO_ prefix is used for MDIO bus drivers etc.  I
> don't think it is appropriate here. How about PCS_LYNX? I don't think
> any other subsystem is using PCS_ as a prefix.
> 
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -235,6 +235,12 @@ config MDIO_XPCS
>>          This module provides helper functions for Synopsys DesignWare XPCS
>>          controllers.
>>
>> +config MDIO_LYNX_PCS
>> +     bool
>> +     help
>> +       This module provides helper functions for Lynx PCS enablement
>> +       representing the PCS as an MDIO device.
>> +
>>   endif
>>   endif
> 
> Maybe add this at the end, and add a
> 
> comment "PCS device drivers"
> 
> before it? I'm assuming with time we will have more of these drivers.
> 
>         Andrew
> 

This driver faithfully follows the model of drivers/net/phy/mdio-xpcs.c. 
Should we rename that as well?

Thanks,
-Vladimir

Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Russell King - ARM Linux admin]

On Fri, Jun 19, 2020 at 12:13:52AM +0200, Andrew Lunn wrote:
> >  MAINTAINERS                     |   7 +
> >  drivers/net/phy/Kconfig         |   6 +
> >  drivers/net/phy/Makefile        |   1 +
> >  drivers/net/phy/mdio-lynx-pcs.c | 358 ++++++++++++++++++++++++++++++++
> >  include/linux/mdio-lynx-pcs.h   |  43 ++++
> >  5 files changed, 415 insertions(+)
> >  create mode 100644 drivers/net/phy/mdio-lynx-pcs.c
> >  create mode 100644 include/linux/mdio-lynx-pcs.h
> 
> Hi Ioana
> 
> We should think about naming convention here.
> 
> All MDIO bus driver, MDIO multiplexors etc use mdio- as a prefix.
> 
> This is not a bus driver, so i don't think it should use the mdio-
> prefix. How about pcs-lynx.c?
> 
> In terms of Kconfig, MDIO_ prefix is used for MDIO bus drivers etc.  I
> don't think it is appropriate here. How about PCS_LYNX? I don't think
> any other subsystem is using PCS_ as a prefix.
> 
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -235,6 +235,12 @@ config MDIO_XPCS
> >  	  This module provides helper functions for Synopsys DesignWare XPCS
> >  	  controllers.
> >  
> > +config MDIO_LYNX_PCS
> > +	bool
> > +	help
> > +	  This module provides helper functions for Lynx PCS enablement
> > +	  representing the PCS as an MDIO device.
> > +
> >  endif
> >  endif
> 
> Maybe add this at the end, and add a
> 
> comment "PCS device drivers"
> 
> before it? I'm assuming with time we will have more of these drivers.

It think we will.

The other thing is, drivers/net/phy is becoming a little cluttered -
we have approaching 100 files there.

Should we be thinking about drivers/net/phy/mdio/ (for mdio*),
drivers/net/phy/lib/ for the core phylib code or leaving it where
it is, and, hmm, drivers/net/phy/media/ maybe for the PHY and PCS
drivers?  Or something like that?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Andrew Lunn]

> The other thing is, drivers/net/phy is becoming a little cluttered -
> we have approaching 100 files there.
> 
> Should we be thinking about drivers/net/phy/mdio/ (for mdio*),
> drivers/net/phy/lib/ for the core phylib code or leaving it where
> it is, and, hmm, drivers/net/phy/media/ maybe for the PHY and PCS
> drivers?  Or something like that?

Hi Russell

Do you have any experience how good git is at following file moves
like this. We don't want to make it too hard for backporters of fixes.

If it is not going to be too painful, then yes, mdio, phy and pcs
subdirectories would be nice.

       Andrew

Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Russell King - ARM Linux admin]

On Fri, Jun 19, 2020 at 12:37:40AM +0200, Andrew Lunn wrote:
> > The other thing is, drivers/net/phy is becoming a little cluttered -
> > we have approaching 100 files there.
> > 
> > Should we be thinking about drivers/net/phy/mdio/ (for mdio*),
> > drivers/net/phy/lib/ for the core phylib code or leaving it where
> > it is, and, hmm, drivers/net/phy/media/ maybe for the PHY and PCS
> > drivers?  Or something like that?
> 
> Hi Russell
> 
> Do you have any experience how good git is at following file moves
> like this. We don't want to make it too hard for backporters of fixes.
> 
> If it is not going to be too painful, then yes, mdio, phy and pcs
> subdirectories would be nice.

It becomes problematical if git doesn't have the objects referenced
on the index line in the patch file when applying.  If it has the
objects, then git can work out where the file moved to via it's
rename detection.

I think the stable team probably have a better idea of how big an
issue it may be, but over the years there have been some quite big
reorganisations done.  The biggest I remember is a whole raft of
drivers moving from (iirc) drivers/net into
drivers/net/ethernet/<vendor> - moving the files in drivers/net/phy
would be quite a bit smaller in comparison.  I think drivers/media
has also seen quite a lot of renames, and drivers/video has been
significantly reorganised.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Ioana Ciornei]

> Subject: Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module
> 
> > Are there really instances where the ethernet driver has to manage
> > multiple different types of PCSs? I am not sure this type of snippet
> > of code is really going to occur.
> 
> Hi Ioana
> 
> The Marvell mv88e6390 family has three PCS's, one for SGMII/1000BaseX, a
> 10Gbase-X4/X2 and a 10GBAse-R. So this sort of code could appear.
> 

I should have been more clear as I was wondering about different types of PCS IPs
(i.e. different vendors and such).

We are in the same case as the Marvell mv88e6390 family, it seems, and we treat
this directly in the Lynx PCS module by a 'switch case' statement by the interface type.
The MAC driver just calls the functions, no choosing necessary on its part, all of that
is done by the PCS module.

Ioana

RE: [PATCH net-next 5/5] net: dsa: felix: use the Lynx PCS helpers

[Ioana Ciornei]

> Subject: Re: [PATCH net-next 5/5] net: dsa: felix: use the Lynx PCS helpers
> 
> On Thu, 18 Jun 2020 15:08:37 +0300 Ioana Ciornei wrote:
> > Use the helper functions introduced by the newly added Lynx PCS MDIO
> > module.
> >
> > Instead of representing the PCS as a phy_device, a mdio_device
> > structure will be passed to the Lynx module which is now actually
> > implementing all the PCS configuration and status reporting.
> >
> > All code previously used for PCS momnitoring and runtime configuration
> > is removed and replaced will calls to the Lynx PCS operations.
> >
> > Tested on the following SERDES protocols of LS1028A: 0x7777
> > (2500Base-X), 0x85bb (QSGMII), 0x9999 (SGMII) and 0x13bb (USXGMII).
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Hm, this does not build with allmodconfig.
> 
> drivers/net/phy/mdio-lynx-pcs.o: In function `lynx_pcs_link_up':
> mdio-lynx-pcs.c:(.text+0x115): undefined reference to `mdiobus_modify'
> mdio-lynx-pcs.c:(.text+0x1a3): undefined reference to `mdiobus_write'
> drivers/net/phy/mdio-lynx-pcs.o: In function `lynx_pcs_config':
> mdio-lynx-pcs.c:(.text+0x33a): undefined reference to `mdiobus_write'
> mdio-lynx-pcs.c:(.text+0x371): undefined reference to `mdiobus_modify'
> mdio-lynx-pcs.c:(.text+0x384): undefined reference to
> `phylink_mii_c22_pcs_config'
> mdio-lynx-pcs.c:(.text+0x3e4): undefined reference to `mdiobus_write'
> mdio-lynx-pcs.c:(.text+0x40d): undefined reference to `mdiobus_write'
> mdio-lynx-pcs.c:(.text+0x422): undefined reference to `mdiobus_write'
> drivers/net/phy/mdio-lynx-pcs.o: In function
> `lynx_pcs_get_state_usxgmii.isra.0':
> mdio-lynx-pcs.c:(.text+0x457): undefined reference to `mdiobus_read'
> mdio-lynx-pcs.c:(.text+0x4f1): undefined reference to `mdiobus_read'
> drivers/net/phy/mdio-lynx-pcs.o: In function `lynx_pcs_get_state':
> mdio-lynx-pcs.c:(.text+0x650): undefined reference to
> `phylink_mii_c22_pcs_get_state'
> mdio-lynx-pcs.c:(.text+0x6fb): undefined reference to `phy_duplex_to_str'
> mdio-lynx-pcs.c:(.text+0x711): undefined reference to `phy_speed_to_str'
> mdio-lynx-pcs.c:(.text+0x7c0): undefined reference to `mdiobus_read'
> mdio-lynx-pcs.c:(.text+0x7d4): undefined reference to `mdiobus_read'
> drivers/net/phy/mdio-lynx-pcs.o: In function `lynx_pcs_an_restart':
> mdio-lynx-pcs.c:(.text+0x954): undefined reference to `mdiobus_write'
> mdio-lynx-pcs.c:(.text+0x978): undefined reference to
> `phylink_mii_c22_pcs_an_restart'
> make[1]: *** [vmlinux] Error 1
> make: *** [__sub-make] Error 2

Hmm, it seems that making the Kconfig just bool instead of tristate is the problem.

Thank,
Ioana

Re: [PATCH net-next 5/5] net: dsa: felix: use the Lynx PCS helpers

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2311 bytes --]

Hi Ioana,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Ioana-Ciornei/net-phy-add-Lynx-PCS-MDIO-module/20200618-201314
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git cb8e59cc87201af93dfbb6c3dccc8fcad72a09c2
config: i386-allmodconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: drivers/net/phy/mdio-lynx-pcs.o: in function `lynx_pcs_config':
   mdio-lynx-pcs.c:(.text+0x47): undefined reference to `mdiobus_write'
>> ld: mdio-lynx-pcs.c:(.text+0x68): undefined reference to `mdiobus_modify'
>> ld: mdio-lynx-pcs.c:(.text+0x79): undefined reference to `phylink_mii_c22_pcs_config'
>> ld: mdio-lynx-pcs.c:(.text+0xae): undefined reference to `mdiobus_write'
   ld: mdio-lynx-pcs.c:(.text+0xed): undefined reference to `mdiobus_write'
   ld: mdio-lynx-pcs.c:(.text+0xfe): undefined reference to `mdiobus_write'
   ld: drivers/net/phy/mdio-lynx-pcs.o: in function `lynx_pcs_an_restart':
   mdio-lynx-pcs.c:(.text+0x14d): undefined reference to `mdiobus_write'
>> ld: mdio-lynx-pcs.c:(.text+0x160): undefined reference to `phylink_mii_c22_pcs_an_restart'
   ld: drivers/net/phy/mdio-lynx-pcs.o: in function `lynx_pcs_link_up':
   mdio-lynx-pcs.c:(.text+0x262): undefined reference to `mdiobus_modify'
   ld: mdio-lynx-pcs.c:(.text+0x2a3): undefined reference to `mdiobus_write'
   ld: drivers/net/phy/mdio-lynx-pcs.o: in function `lynx_pcs_get_state':
   mdio-lynx-pcs.c:(.text+0x2f2): undefined reference to `phylink_mii_c22_pcs_get_state'
>> ld: mdio-lynx-pcs.c:(.text+0x32b): undefined reference to `mdiobus_read'
   ld: mdio-lynx-pcs.c:(.text+0x364): undefined reference to `mdiobus_read'
>> ld: mdio-lynx-pcs.c:(.text+0x3da): undefined reference to `phy_duplex_to_str'
>> ld: mdio-lynx-pcs.c:(.text+0x3e5): undefined reference to `phy_speed_to_str'
   ld: mdio-lynx-pcs.c:(.text+0x44c): undefined reference to `mdiobus_read'
   ld: mdio-lynx-pcs.c:(.text+0x45e): undefined reference to `mdiobus_read'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 74071 bytes --]