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

[PATCH net-next v3 0/9] 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 an 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 (with and without in-band AN) 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 functional change is intended at this
time.

Changes in v2:
 * got rid of the mdio_lynx_pcs structure and directly exported the
 functions without the need of an indirection
 * made the necessary adjustments for this in the Felix DSA driver
 * solved the broken allmodconfig build test by making the module
 tristate instead of bool
 * fixed a memory leakage in the Felix driver (the pcs structure was
 allocated twice)

Changes in v3:
 * added support for PHYLINK PCS ops in DSA (patch 5/9)
 * cleanup in Felix PHYLINK operations and migrate to
 phylink_mac_link_up() being the callback of choice for applying MAC
 configuration (patches 6-8)

As per Russell's request, the DSA core now exports PHYLINK PCS ops.
DSA adds a phylink_pcs_ops structure to PHYLINK only when a switch
driver is implementing all of the 4 PCS operations (get_state,
an_restart, config, link_up).

Note that there is no longer anything to be done in phylink_mac_config()
for Felix and, likely, for any other driver which uses the new PHYLINK
format. However, PHYLINK does want the phylink_mac_config() pointer to
be present. Currently, the DSA core supplied a stub and happily doesn't
do anything because felix_phylink_mac_config() is missing.

Felix's migration from mac_config() to mac_link_up() was added directly
into this patch set since everything in this general area is
intertwined and splitting would have been difficult.

Ioana Ciornei (5):
  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 module
  net: dsa: add support for phylink_pcs_ops
  net: dsa: felix: use the Lynx PCS helpers

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

Vladimir Oltean (3):
  net: dsa: felix: unconditionally configure MAC speed to 1000Mbps
  net: dsa: felix: set proper pause frame timers based on link speed
  net: dsa: felix: use resolved link config in mac_link_up()

 MAINTAINERS                            |   7 +
 drivers/net/dsa/ocelot/Kconfig         |   1 +
 drivers/net/dsa/ocelot/felix.c         | 129 ++++++---
 drivers/net/dsa/ocelot/felix.h         |  16 +-
 drivers/net/dsa/ocelot/felix_vsc9959.c | 385 +++----------------------
 drivers/net/phy/Kconfig                |   6 +
 drivers/net/phy/Makefile               |   1 +
 drivers/net/phy/pcs-lynx.c             | 337 ++++++++++++++++++++++
 drivers/net/phy/phylink.c              |  38 +++
 include/linux/fsl/enetc_mdio.h         |  21 --
 include/linux/mdio.h                   |   6 +
 include/linux/pcs-lynx.h               |  25 ++
 include/linux/phylink.h                |   3 +
 include/net/dsa.h                      |  12 +
 net/dsa/dsa_priv.h                     |   1 +
 net/dsa/port.c                         |  46 +++
 net/dsa/slave.c                        |   6 +
 17 files changed, 617 insertions(+), 423 deletions(-)
 create mode 100644 drivers/net/phy/pcs-lynx.c
 create mode 100644 include/linux/pcs-lynx.h

-- 
2.25.1

[PATCH net-next v3 1/9] 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>
---
Changes in v2:
 - none
Changes in v3:
 - none

 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 7ce787c227b3..f04ccdedda40 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 v3 2/9] 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>
---
Changes in v2:
 - none
Changes in v3:
 - none

 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 f04ccdedda40..117ddbcaaa6e 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 v3 3/9] 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>
---
Changes in v2:
 - none
Changes in v3:
 - none

 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 v3 4/9] net: phy: add Lynx PCS module

[Ioana Ciornei]

Add a Lynx PCS 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 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>
---
Changes in v2:
 * got rid of the mdio_lynx_pcs structure and directly exported the
 functions without the need of an indirection
 * solved the broken allmodconfig build test by making the module
 tristate instead of bool

Changes in v3:
 * renamed the file to pcs-lynx.c


 MAINTAINERS                |   7 +
 drivers/net/phy/Kconfig    |   6 +
 drivers/net/phy/Makefile   |   1 +
 drivers/net/phy/pcs-lynx.c | 337 +++++++++++++++++++++++++++++++++++++
 include/linux/pcs-lynx.h   |  25 +++
 5 files changed, 376 insertions(+)
 create mode 100644 drivers/net/phy/pcs-lynx.c
 create mode 100644 include/linux/pcs-lynx.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 301330e02bca..850d8b4f2d29 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10168,6 +10168,13 @@ S:	Maintained
 W:	http://linux-test-project.github.io/
 T:	git git://github.com/linux-test-project/ltp.git
 
+LYNX PCS MODULE
+M:	Ioana Ciornei <ioana.ciornei@nxp.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	drivers/net/phy/pcs-lynx.c
+F:	include/linux/pcs-lynx.h
+
 M68K ARCHITECTURE
 M:	Geert Uytterhoeven <geert@linux-m68k.org>
 L:	linux-m68k@lists.linux-m68k.org
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index f25702386d83..3a573afb21a3 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 PCS_LYNX
+	tristate
+	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..15ea3345fe3c 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_PCS_LYNX)		+= pcs-lynx.o
 
 obj-$(CONFIG_NETWORK_PHY_TIMESTAMPING) += mii_timestamper.o
 
diff --git a/drivers/net/phy/pcs-lynx.c b/drivers/net/phy/pcs-lynx.c
new file mode 100644
index 000000000000..23bdd9db4340
--- /dev/null
+++ b/drivers/net/phy/pcs-lynx.c
@@ -0,0 +1,337 @@
+// 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/pcs-lynx.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);
+}
+
+void lynx_pcs_an_restart(struct mdio_device *pcs, phy_interface_t ifmode)
+{
+	switch (ifmode) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		phylink_mii_c22_pcs_an_restart(pcs);
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		lynx_pcs_an_restart_usxgmii(pcs);
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		break;
+	default:
+		dev_err(&pcs->dev, "Invalid PCS interface type %s\n",
+			phy_modes(ifmode));
+		break;
+	}
+}
+EXPORT_SYMBOL(lynx_pcs_an_restart);
+
+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;
+}
+
+void lynx_pcs_get_state(struct mdio_device *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, state);
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		lynx_pcs_get_state_2500basex(pcs, state);
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		lynx_pcs_get_state_usxgmii(pcs, state);
+		break;
+	default:
+		break;
+	}
+
+	dev_dbg(&pcs->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);
+}
+EXPORT_SYMBOL(lynx_pcs_get_state);
+
+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;
+}
+
+int lynx_pcs_config(struct mdio_device *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, 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, mode, advertising);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(lynx_pcs_config);
+
+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));
+}
+
+void lynx_pcs_link_up(struct mdio_device *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, mode, speed, duplex);
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		lynx_pcs_link_up_2500basex(pcs, 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;
+	}
+}
+EXPORT_SYMBOL(lynx_pcs_link_up);
+
+MODULE_LICENSE("Dual BSD/GPL");
diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h
new file mode 100644
index 000000000000..336fccb8c55f
--- /dev/null
+++ b/include/linux/pcs-lynx.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
+/* Copyright 2020 NXP
+ * Lynx PCS helpers
+ */
+
+#ifndef __LINUX_PCS_LYNX_H
+#define __LINUX_PCS_LYNX_H
+
+#include <linux/phy.h>
+#include <linux/mdio.h>
+
+void lynx_pcs_an_restart(struct mdio_device *pcs, phy_interface_t ifmode);
+
+void lynx_pcs_get_state(struct mdio_device *pcs, phy_interface_t ifmode,
+			struct phylink_link_state *state);
+
+int lynx_pcs_config(struct mdio_device *pcs, unsigned int mode,
+		    phy_interface_t ifmode,
+		    const unsigned long *advertising);
+
+void lynx_pcs_link_up(struct mdio_device *pcs, unsigned int mode,
+		      phy_interface_t interface,
+		      int speed, int duplex);
+
+#endif /* __LINUX_PCS_LYNX_H */
-- 
2.25.1

Re: [PATCH net-next v3 4/9] net: phy: add Lynx PCS module

[Russell King - ARM Linux admin]

On Mon, Jun 22, 2020 at 01:54:46AM +0300, Ioana Ciornei wrote:
> Add a Lynx PCS 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 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>
> ---
> Changes in v2:
>  * got rid of the mdio_lynx_pcs structure and directly exported the
>  functions without the need of an indirection
>  * solved the broken allmodconfig build test by making the module
>  tristate instead of bool
> 
> Changes in v3:
>  * renamed the file to pcs-lynx.c
> 
> 
>  MAINTAINERS                |   7 +
>  drivers/net/phy/Kconfig    |   6 +
>  drivers/net/phy/Makefile   |   1 +
>  drivers/net/phy/pcs-lynx.c | 337 +++++++++++++++++++++++++++++++++++++
>  include/linux/pcs-lynx.h   |  25 +++
>  5 files changed, 376 insertions(+)
>  create mode 100644 drivers/net/phy/pcs-lynx.c
>  create mode 100644 include/linux/pcs-lynx.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 301330e02bca..850d8b4f2d29 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10168,6 +10168,13 @@ S:	Maintained
>  W:	http://linux-test-project.github.io/
>  T:	git git://github.com/linux-test-project/ltp.git
>  
> +LYNX PCS MODULE
> +M:	Ioana Ciornei <ioana.ciornei@nxp.com>
> +L:	netdev@vger.kernel.org
> +S:	Maintained
> +F:	drivers/net/phy/pcs-lynx.c
> +F:	include/linux/pcs-lynx.h
> +
>  M68K ARCHITECTURE
>  M:	Geert Uytterhoeven <geert@linux-m68k.org>
>  L:	linux-m68k@lists.linux-m68k.org
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index f25702386d83..3a573afb21a3 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 PCS_LYNX
> +	tristate
> +	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..15ea3345fe3c 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_PCS_LYNX)		+= pcs-lynx.o
>  
>  obj-$(CONFIG_NETWORK_PHY_TIMESTAMPING) += mii_timestamper.o
>  
> diff --git a/drivers/net/phy/pcs-lynx.c b/drivers/net/phy/pcs-lynx.c
> new file mode 100644
> index 000000000000..23bdd9db4340
> --- /dev/null
> +++ b/drivers/net/phy/pcs-lynx.c
> @@ -0,0 +1,337 @@
> +// 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/pcs-lynx.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)

Given that this is in the .c file, and this code will be re-used in
other places where there is support for more than Cisco SGMII, can
we lose the SGMII_ prefix please?  Maybe use names such as those
I have in "dpaa2-mac: add 1000BASE-X/SGMII PCS support" ?

(I hate the way a single lane gigabit serdes link that supports
1000base-x gets incorrectly called "SGMII".)

> +
> +#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,
> +};

These are not specific to the Lynx PCS, but are the standard layout
of the USXGMII word.  These ought to be in a header file similar to
what we do with the SGMII definitions in include/uapi/linux/mii.h.
I think as these are Clause 45, they possibly belong in
include/uapi/linux/mdio.h ?  In any case, one of my comments below
suggests that some of the uses of these definitions should be moved
into phylink's helpers.

> +
> +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);
> +}

Phylink will not call *_an_restart() for USXGMII, so this code is
unreachable.

> +
> +void lynx_pcs_an_restart(struct mdio_device *pcs, phy_interface_t ifmode)
> +{
> +	switch (ifmode) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_QSGMII:
> +		phylink_mii_c22_pcs_an_restart(pcs);

Phylink will not call *_an_restart() for SGMII, so this code is
unreachable.

> +		break;
> +	case PHY_INTERFACE_MODE_USXGMII:
> +		lynx_pcs_an_restart_usxgmii(pcs);
> +		break;
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		break;
> +	default:
> +		dev_err(&pcs->dev, "Invalid PCS interface type %s\n",
> +			phy_modes(ifmode));
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL(lynx_pcs_an_restart);
> +
> +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;

This should be added to phylink_mii_c45_pcs_get_state().  There is
nothing that is Lynx PCS specific here.

> +}
> +
> +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;

How do you know the other side is using pause frames, or is capable
of dealing with them?

In any case, phylink_mii_c22_pcs_get_state() should be expanded to
deal with the non-inband cases, where we are only interested in the
link state.  It isn't passed the link AN mode, which may be an
issue that needs resolving in some way.

> +}
> +
> +void lynx_pcs_get_state(struct mdio_device *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, state);
> +		break;
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		lynx_pcs_get_state_2500basex(pcs, state);
> +		break;
> +	case PHY_INTERFACE_MODE_USXGMII:
> +		lynx_pcs_get_state_usxgmii(pcs, state);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	dev_dbg(&pcs->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);
> +}
> +EXPORT_SYMBOL(lynx_pcs_get_state);
> +
> +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);

This will be overwritten by phylink_mii_c22_pcs_config() below.

> +
> +	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;
> +}
> +
> +int lynx_pcs_config(struct mdio_device *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, 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, mode, advertising);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(lynx_pcs_config);
> +
> +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));
> +}
> +
> +void lynx_pcs_link_up(struct mdio_device *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, mode, speed, duplex);
> +		break;
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		lynx_pcs_link_up_2500basex(pcs, 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;
> +	}
> +}
> +EXPORT_SYMBOL(lynx_pcs_link_up);
> +
> +MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h
> new file mode 100644
> index 000000000000..336fccb8c55f
> --- /dev/null
> +++ b/include/linux/pcs-lynx.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> +/* Copyright 2020 NXP
> + * Lynx PCS helpers
> + */
> +
> +#ifndef __LINUX_PCS_LYNX_H
> +#define __LINUX_PCS_LYNX_H
> +
> +#include <linux/phy.h>
> +#include <linux/mdio.h>
> +
> +void lynx_pcs_an_restart(struct mdio_device *pcs, phy_interface_t ifmode);
> +
> +void lynx_pcs_get_state(struct mdio_device *pcs, phy_interface_t ifmode,
> +			struct phylink_link_state *state);
> +
> +int lynx_pcs_config(struct mdio_device *pcs, unsigned int mode,
> +		    phy_interface_t ifmode,
> +		    const unsigned long *advertising);
> +
> +void lynx_pcs_link_up(struct mdio_device *pcs, unsigned int mode,
> +		      phy_interface_t interface,
> +		      int speed, int duplex);
> +
> +#endif /* __LINUX_PCS_LYNX_H */
> -- 
> 2.25.1
> 
> 

-- 
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 v3 4/9] net: phy: add Lynx PCS module

[Ioana Ciornei]

> Subject: Re: [PATCH net-next v3 4/9] net: phy: add Lynx PCS module
> 
> On Mon, Jun 22, 2020 at 01:54:46AM +0300, Ioana Ciornei wrote:
> > Add a Lynx PCS 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 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>
> > ---
> > Changes in v2:
> >  * got rid of the mdio_lynx_pcs structure and directly exported the
> > functions without the need of an indirection
> >  * solved the broken allmodconfig build test by making the module
> > tristate instead of bool
> >
> > Changes in v3:
> >  * renamed the file to pcs-lynx.c
> >
> >
> >  MAINTAINERS                |   7 +
> >  drivers/net/phy/Kconfig    |   6 +
> >  drivers/net/phy/Makefile   |   1 +
> >  drivers/net/phy/pcs-lynx.c | 337
> +++++++++++++++++++++++++++++++++++++
> >  include/linux/pcs-lynx.h   |  25 +++
> >  5 files changed, 376 insertions(+)
> >  create mode 100644 drivers/net/phy/pcs-lynx.c  create mode 100644
> > include/linux/pcs-lynx.h
> >

(...)

> > diff --git a/drivers/net/phy/pcs-lynx.c b/drivers/net/phy/pcs-lynx.c
> > new file mode 100644 index 000000000000..23bdd9db4340
> > --- /dev/null
> > +++ b/drivers/net/phy/pcs-lynx.c
> > @@ -0,0 +1,337 @@
> > +// 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/pcs-lynx.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)
> 
> Given that this is in the .c file, and this code will be re-used in other places where
> there is support for more than Cisco SGMII, can we lose the SGMII_ prefix
> please?  Maybe use names such as those I have in "dpaa2-mac: add 1000BASE-
> X/SGMII PCS support" ?

Yep, I can do that. 

> 
> (I hate the way a single lane gigabit serdes link that supports 1000base-x gets
> incorrectly called "SGMII".)
> 
> > +
> > +#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,
> > +};
> 
> These are not specific to the Lynx PCS, but are the standard layout of the
> USXGMII word.  These ought to be in a header file similar to what we do with
> the SGMII definitions in include/uapi/linux/mii.h.
> I think as these are Clause 45, they possibly belong in include/uapi/linux/mdio.h
> ?  In any case, one of my comments below suggests that some of the uses of
> these definitions should be moved into phylink's helpers.
>

Ok, since these are described in the USXGMII standard I can move them to the generic header (mdio.h).
I will comment below about their usage in phylink's helpers.
 
> > +
> > +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); }
> 
> Phylink will not call *_an_restart() for USXGMII, so this code is unreachable.
> 
> > +
> > +void lynx_pcs_an_restart(struct mdio_device *pcs, phy_interface_t
> > +ifmode) {
> > +	switch (ifmode) {
> > +	case PHY_INTERFACE_MODE_SGMII:
> > +	case PHY_INTERFACE_MODE_QSGMII:
> > +		phylink_mii_c22_pcs_an_restart(pcs);
> 
> Phylink will not call *_an_restart() for SGMII, so this code is unreachable.
> 

Good point. I'll remove this and the above one.
> > +		break;
> > +	case PHY_INTERFACE_MODE_USXGMII:
> > +		lynx_pcs_an_restart_usxgmii(pcs);
> > +		break;
> > +	case PHY_INTERFACE_MODE_2500BASEX:
> > +		break;
> > +	default:
> > +		dev_err(&pcs->dev, "Invalid PCS interface type %s\n",
> > +			phy_modes(ifmode));
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL(lynx_pcs_an_restart);
> > +
> > +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;
> 
> This should be added to phylink_mii_c45_pcs_get_state().  There is nothing that
> is Lynx PCS specific here.

The USXGMII standard only describes the auto-negotiation word, not the MMD
where this can be found (MMD_VEND2 in this case).
I would not add a generic phylink herper that reads the MMD and also decodes it.
Maybe a helper that just decodes the USXGMII word read from the
Lynx module - phylink_decode_usxgmii_word(state, lpa) ?

> 
> > +}
> > +
> > +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;
> 
> How do you know the other side is using pause frames, or is capable of dealing
> with them?

Isn't this done by also looking into the PHY's pause frame bits and enabling pause
frames in the MAC only when both the PCS and the PHY have flow enabled?

> 
> In any case, phylink_mii_c22_pcs_get_state() should be expanded to deal with
> the non-inband cases, where we are only interested in the link state.  It isn't
> passed the link AN mode, which may be an issue that needs resolving in some
> way.
>

Agreed, but I wouldn't just add all of this into this patch set.. it already is getting out of hand.

 
> > +}
> > +
> > +void lynx_pcs_get_state(struct mdio_device *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, state);
> > +		break;
> > +	case PHY_INTERFACE_MODE_2500BASEX:
> > +		lynx_pcs_get_state_2500basex(pcs, state);
> > +		break;
> > +	case PHY_INTERFACE_MODE_USXGMII:
> > +		lynx_pcs_get_state_usxgmii(pcs, state);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	dev_dbg(&pcs->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); }
> > +EXPORT_SYMBOL(lynx_pcs_get_state);
> > +
> > +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);
> 
> This will be overwritten by phylink_mii_c22_pcs_config() below.
> 

Yep, will remove. I've gone through the documentation and the register
should be initialized to 0x0001 when in SGMII mode
(as done by phylink_mii_c22_pcs_config()).

> > +
> > +	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;
> > +}
> > +
> > +int lynx_pcs_config(struct mdio_device *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, 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, mode, advertising);
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(lynx_pcs_config);
> > +
> > +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));
> > +}
> > +
> > +void lynx_pcs_link_up(struct mdio_device *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, mode, speed, duplex);
> > +		break;
> > +	case PHY_INTERFACE_MODE_2500BASEX:
> > +		lynx_pcs_link_up_2500basex(pcs, 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;
> > +	}
> > +}
> > +EXPORT_SYMBOL(lynx_pcs_link_up);
> > +
> > +MODULE_LICENSE("Dual BSD/GPL");
> > diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h new
> > file mode 100644 index 000000000000..336fccb8c55f
> > --- /dev/null
> > +++ b/include/linux/pcs-lynx.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> > +/* Copyright 2020 NXP
> > + * Lynx PCS helpers
> > + */
> > +
> > +#ifndef __LINUX_PCS_LYNX_H
> > +#define __LINUX_PCS_LYNX_H
> > +
> > +#include <linux/phy.h>
> > +#include <linux/mdio.h>
> > +
> > +void lynx_pcs_an_restart(struct mdio_device *pcs, phy_interface_t
> > +ifmode);
> > +
> > +void lynx_pcs_get_state(struct mdio_device *pcs, phy_interface_t ifmode,
> > +			struct phylink_link_state *state);
> > +
> > +int lynx_pcs_config(struct mdio_device *pcs, unsigned int mode,
> > +		    phy_interface_t ifmode,
> > +		    const unsigned long *advertising);
> > +
> > +void lynx_pcs_link_up(struct mdio_device *pcs, unsigned int mode,
> > +		      phy_interface_t interface,
> > +		      int speed, int duplex);
> > +
> > +#endif /* __LINUX_PCS_LYNX_H */
> > --
> > 2.25.1
> >
> >
> 

Re: [PATCH net-next v3 4/9] net: phy: add Lynx PCS module

[Russell King - ARM Linux admin]

On Tue, Jun 23, 2020 at 11:49:28AM +0000, Ioana Ciornei wrote:
> > This should be added to phylink_mii_c45_pcs_get_state().  There is nothing that
> > is Lynx PCS specific here.
> 
> The USXGMII standard only describes the auto-negotiation word, not the MMD
> where this can be found (MMD_VEND2 in this case).
> I would not add a generic phylink herper that reads the MMD and also
> decodes it.
> Maybe a helper that just decodes the USXGMII word read from the
> Lynx module - phylink_decode_usxgmii_word(state, lpa) ?

Yes, you're right - as they come from the vendor 2 MMD, there's no
standard.  So yes, just a helper to decode the USXGMII word please.

> > > +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;
> > 
> > How do you know the other side is using pause frames, or is capable of dealing
> > with them?
> 
> Isn't this done by also looking into the PHY's pause frame bits and
> enabling pause frames in the MAC only when both the PCS and the PHY
> have flow enabled?

You are assuming that there is a PHY to read that information from.

There may not be a PHY - I have 2500base-X fibre links here, there is
no PHY to read that from, there is only the PCS - but this runs with
the configuration word present, so is not supported by this code (at
least at the moment.)

If there is a PHY, these bits will not be used anyway, so there's no
point setting them.

> Yep, will remove. I've gone through the documentation and the register
> should be initialized to 0x0001 when in SGMII mode
> (as done by phylink_mii_c22_pcs_config()).

Yep, that was actually written referring to the LX2160A documentation.

-- 
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 v3 4/9] net: phy: add Lynx PCS module

[Russell King - ARM Linux admin]

I just spotted something else...

On Mon, Jun 22, 2020 at 01:54:46AM +0300, Ioana Ciornei wrote:
> +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;

You seem to have missed setting state->duplex here - it defaults to
DUPLEX_UNKNOWN, and will remain as such, which is probably not what
you want.

> +	state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX;
> +}
-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops

[Ioana Ciornei]

In order to support split PCS using PHYLINK properly, we need to add a
phylink_pcs_ops structure.

Note that a DSA driver that wants to use these needs to implement all 4
of them: the DSA core checks the presence of these 4 function pointers
in dsa_switch_ops and only then does it add a PCS to PHYLINK. This is
done in order to preserve compatibility with drivers that have not yet
been converted, or don't need, a split PCS setup.

Also, when pcs_get_state() and pcs_an_restart() are present, their mac
counterparts (mac_pcs_get_state(), mac_an_restart()) will no longer get
called, as can be seen in phylink.c.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v3:
 * patch added

 include/net/dsa.h  | 12 ++++++++++++
 net/dsa/dsa_priv.h |  1 +
 net/dsa/port.c     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 net/dsa/slave.c    |  6 ++++++
 4 files changed, 65 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 50389772c597..09aa36198f4b 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -446,6 +446,18 @@ struct dsa_switch_ops {
 				       bool tx_pause, bool rx_pause);
 	void	(*phylink_fixed_state)(struct dsa_switch *ds, int port,
 				       struct phylink_link_state *state);
+	void	(*phylink_pcs_get_state)(struct dsa_switch *ds, int port,
+					 struct phylink_link_state *state);
+	int	(*phylink_pcs_config)(struct dsa_switch *ds, int port,
+				      unsigned int mode,
+				      phy_interface_t interface,
+				      const unsigned long *advertising);
+	void	(*phylink_pcs_an_restart)(struct dsa_switch *ds, int port);
+	void	(*phylink_pcs_link_up)(struct dsa_switch *ds, int port,
+				       unsigned int mode,
+				       phy_interface_t interface, int speed,
+				       int duplex);
+
 	/*
 	 * ethtool hardware statistics.
 	 */
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index adecf73bd608..de8e11796178 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -169,6 +169,7 @@ int dsa_port_vid_del(struct dsa_port *dp, u16 vid);
 int dsa_port_link_register_of(struct dsa_port *dp);
 void dsa_port_link_unregister_of(struct dsa_port *dp);
 extern const struct phylink_mac_ops dsa_port_phylink_mac_ops;
+extern const struct phylink_pcs_ops dsa_port_phylink_pcs_ops;
 
 /* slave.c */
 extern const struct dsa_device_ops notag_netdev_ops;
diff --git a/net/dsa/port.c b/net/dsa/port.c
index e23ece229c7e..a2b0460d2593 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -592,6 +592,52 @@ const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
 	.mac_link_up = dsa_port_phylink_mac_link_up,
 };
 
+static void dsa_port_pcs_get_state(struct phylink_config *config,
+				   struct phylink_link_state *state)
+{
+	struct dsa_port *dp = container_of(config, struct dsa_port, pl_config);
+	struct dsa_switch *ds = dp->ds;
+
+	ds->ops->phylink_pcs_get_state(ds, dp->index, state);
+}
+
+static void dsa_port_pcs_an_restart(struct phylink_config *config)
+{
+	struct dsa_port *dp = container_of(config, struct dsa_port, pl_config);
+	struct dsa_switch *ds = dp->ds;
+
+	ds->ops->phylink_pcs_an_restart(ds, dp->index);
+}
+
+static int dsa_port_pcs_config(struct phylink_config *config,
+			       unsigned int mode, phy_interface_t interface,
+			       const unsigned long *advertising)
+{
+	struct dsa_port *dp = container_of(config, struct dsa_port, pl_config);
+	struct dsa_switch *ds = dp->ds;
+
+	return ds->ops->phylink_pcs_config(ds, dp->index, mode, interface,
+					   advertising);
+}
+
+static void dsa_port_pcs_link_up(struct phylink_config *config,
+				 unsigned int mode, phy_interface_t interface,
+				 int speed, int duplex)
+{
+	struct dsa_port *dp = container_of(config, struct dsa_port, pl_config);
+	struct dsa_switch *ds = dp->ds;
+
+	ds->ops->phylink_pcs_link_up(ds, dp->index, mode, interface, speed,
+				     duplex);
+}
+
+const struct phylink_pcs_ops dsa_port_phylink_pcs_ops = {
+	.pcs_get_state = dsa_port_pcs_get_state,
+	.pcs_config = dsa_port_pcs_config,
+	.pcs_an_restart = dsa_port_pcs_an_restart,
+	.pcs_link_up = dsa_port_pcs_link_up,
+};
+
 static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable)
 {
 	struct dsa_switch *ds = dp->ds;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 4c7f086a047b..8856e70f6a06 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1650,6 +1650,12 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 	if (ds->ops->get_phy_flags)
 		phy_flags = ds->ops->get_phy_flags(ds, dp->index);
 
+	if (ds->ops->phylink_pcs_get_state &&
+	    ds->ops->phylink_pcs_an_restart &&
+	    ds->ops->phylink_pcs_config &&
+	    ds->ops->phylink_pcs_link_up)
+		phylink_add_pcs(dp->pl, &dsa_port_phylink_pcs_ops);
+
 	ret = phylink_of_phy_connect(dp->pl, port_dn, phy_flags);
 	if (ret == -ENODEV && ds->slave_mii_bus) {
 		/* We could not connect to a designated PHY or SFP, so try to
-- 
2.25.1

Re: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops

[Russell King - ARM Linux admin]

On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote:
> In order to support split PCS using PHYLINK properly, we need to add a
> phylink_pcs_ops structure.
> 
> Note that a DSA driver that wants to use these needs to implement all 4
> of them: the DSA core checks the presence of these 4 function pointers
> in dsa_switch_ops and only then does it add a PCS to PHYLINK. This is
> done in order to preserve compatibility with drivers that have not yet
> been converted, or don't need, a split PCS setup.
> 
> Also, when pcs_get_state() and pcs_an_restart() are present, their mac
> counterparts (mac_pcs_get_state(), mac_an_restart()) will no longer get
> called, as can be seen in phylink.c.

I don't like this at all, it means we've got all this useless layering,
and that layering will force similar layering veneers into other parts
of the kernel (such as the DPAA2 MAC driver, when we eventually come to
re-use pcs-lynx there.)

I don't think we need that - I think we can get to a position where
pcs-lynx is called requesting that it bind to phylink as the PCS, and
it calls phylink_add_pcs() directly, which means we do not end up with
veneers in DSA nor in the DPAA2 MAC driver - they just need to call
the pcs-lynx initialisation function with the phylink instance for it
to attach to.

Yes, that requires phylink_add_pcs() to change slightly, and for there
to be a PCS private pointer, as I have previously stated.  At the
moment, changing that is really easy - the phylink PCS backend has no
in-tree users at the moment.  So there is no reason not to get this
correct right now.

-- 
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 v3 5/9] net: dsa: add support for phylink_pcs_ops

[Russell King - ARM Linux admin]

On Mon, Jun 22, 2020 at 11:22:13AM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote:
> > In order to support split PCS using PHYLINK properly, we need to add a
> > phylink_pcs_ops structure.
> > 
> > Note that a DSA driver that wants to use these needs to implement all 4
> > of them: the DSA core checks the presence of these 4 function pointers
> > in dsa_switch_ops and only then does it add a PCS to PHYLINK. This is
> > done in order to preserve compatibility with drivers that have not yet
> > been converted, or don't need, a split PCS setup.
> > 
> > Also, when pcs_get_state() and pcs_an_restart() are present, their mac
> > counterparts (mac_pcs_get_state(), mac_an_restart()) will no longer get
> > called, as can be seen in phylink.c.
> 
> I don't like this at all, it means we've got all this useless layering,
> and that layering will force similar layering veneers into other parts
> of the kernel (such as the DPAA2 MAC driver, when we eventually come to
> re-use pcs-lynx there.)
> 
> I don't think we need that - I think we can get to a position where
> pcs-lynx is called requesting that it bind to phylink as the PCS, and
> it calls phylink_add_pcs() directly, which means we do not end up with
> veneers in DSA nor in the DPAA2 MAC driver - they just need to call
> the pcs-lynx initialisation function with the phylink instance for it
> to attach to.
> 
> Yes, that requires phylink_add_pcs() to change slightly, and for there
> to be a PCS private pointer, as I have previously stated.  At the
> moment, changing that is really easy - the phylink PCS backend has no
> in-tree users at the moment.  So there is no reason not to get this
> correct right now.

How about something like this?  I don't want to embed a mdio_device
inside struct phylink_pcs because that would not be appropriate for
some potential users (e.g., Marvell 88E6xxx DSA drivers, Marvell
NETA, PP2, etc.)

From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH net-next] net: phylink: add struct phylink_pcs

Add a way for MAC PCS to have private data while keeping independence
from struct phylink_config, which is used for the MAC itself. We need
this independence as we will have stand-alone code for PCS that is
independent of the MAC.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 23 +++++++++++++++-------
 include/linux/phylink.h   | 41 ++++++++++++++++++++++++++-------------
 2 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 7ce787c227b3..37ed7455652a 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -43,6 +43,7 @@ struct phylink {
 	const struct phylink_mac_ops *mac_ops;
 	const struct phylink_pcs_ops *pcs_ops;
 	struct phylink_config *config;
+	struct phylink_pcs *pcs;
 	struct device *dev;
 	unsigned int old_link_state:1;
 
@@ -431,7 +432,7 @@ static void phylink_mac_pcs_an_restart(struct phylink *pl)
 	if (pl->link_config.an_enabled &&
 	    phy_interface_mode_is_8023z(pl->link_config.interface)) {
 		if (pl->pcs_ops)
-			pl->pcs_ops->pcs_an_restart(pl->config);
+			pl->pcs_ops->pcs_an_restart(pl->pcs);
 		else
 			pl->mac_ops->mac_an_restart(pl->config);
 	}
@@ -442,7 +443,7 @@ static void phylink_pcs_config(struct phylink *pl, bool force_restart,
 {
 	bool restart = force_restart;
 
-	if (pl->pcs_ops && pl->pcs_ops->pcs_config(pl->config,
+	if (pl->pcs_ops && pl->pcs_ops->pcs_config(pl->pcs,
 						   pl->cur_link_an_mode,
 						   state->interface,
 						   state->advertising))
@@ -468,7 +469,7 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
 	state->link = 1;
 
 	if (pl->pcs_ops)
-		pl->pcs_ops->pcs_get_state(pl->config, state);
+		pl->pcs_ops->pcs_get_state(pl->pcs, state);
 	else
 		pl->mac_ops->mac_pcs_get_state(pl->config, state);
 }
@@ -539,7 +540,7 @@ static void phylink_link_up(struct phylink *pl,
 	pl->cur_interface = link_state.interface;
 
 	if (pl->pcs_ops && pl->pcs_ops->pcs_link_up)
-		pl->pcs_ops->pcs_link_up(pl->config, pl->cur_link_an_mode,
+		pl->pcs_ops->pcs_link_up(pl->pcs, pl->cur_link_an_mode,
 					 pl->cur_interface,
 					 link_state.speed, link_state.duplex);
 
@@ -777,11 +778,19 @@ struct phylink *phylink_create(struct phylink_config *config,
 }
 EXPORT_SYMBOL_GPL(phylink_create);
 
-void phylink_add_pcs(struct phylink *pl, const struct phylink_pcs_ops *ops)
+/**
+ * phylink_set_pcs() - set the current PCS for phylink to use
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @pcs: a pointer to the &struct phylink_pcs
+ *
+ * Bind the MAC PCS to phylink.
+ */
+void phylink_set_pcs(struct phylink *pl, const struct phylink_pcs *pcs)
 {
-	pl->pcs_ops = ops;
+	pl->pcs = pcs;
+	pl->pcs_ops = pcs->ops;
 }
-EXPORT_SYMBOL_GPL(phylink_add_pcs);
+EXPORT_SYMBOL_GPL(phylink_set_pcs);
 
 /**
  * phylink_destroy() - cleanup and destroy the phylink instance
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index cc5b452a184e..77e20d48577f 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -273,6 +273,19 @@ void mac_link_up(struct phylink_config *config, struct phy_device *phy,
 		 int speed, int duplex, bool tx_pause, bool rx_pause);
 #endif
 
+struct phylink_pcs_ops;
+
+/**
+ * struct phylink_pcs - PHYLINK PCS instance
+ * @ops: a pointer to the &struct phylink_pcs_ops structure
+ *
+ * This structure is designed to be embedded within the PCS private data,
+ * and will be passed between phylink and the PCS.
+ */
+struct phylink_pcs {
+	const struct phylink_pcs_ops *ops;
+};
+
 /**
  * struct phylink_pcs_ops - MAC PCS operations structure.
  * @pcs_get_state: read the current MAC PCS link state from the hardware.
@@ -282,12 +295,12 @@ void mac_link_up(struct phylink_config *config, struct phy_device *phy,
  *               (where necessary).
  */
 struct phylink_pcs_ops {
-	void (*pcs_get_state)(struct phylink_config *config,
+	void (*pcs_get_state)(struct phylink_pcs *pcs,
 			      struct phylink_link_state *state);
-	int (*pcs_config)(struct phylink_config *config, unsigned int mode,
+	int (*pcs_config)(struct phylink_pcs *pcs, unsigned int mode,
 			  phy_interface_t interface,
 			  const unsigned long *advertising);
-	void (*pcs_an_restart)(struct phylink_config *config);
+	void (*pcs_an_restart)(struct phylink_pcs *pcs);
 	void (*pcs_link_up)(struct phylink_config *config, unsigned int mode,
 			    phy_interface_t interface, int speed, int duplex);
 };
@@ -295,7 +308,7 @@ struct phylink_pcs_ops {
 #if 0 /* For kernel-doc purposes only. */
 /**
  * pcs_get_state() - Read the current inband link state from the hardware
- * @config: a pointer to a &struct phylink_config.
+ * @pcs: a pointer to a &struct phylink_pcs.
  * @state: a pointer to a &struct phylink_link_state.
  *
  * Read the current inband link state from the MAC PCS, reporting the
@@ -308,12 +321,12 @@ struct phylink_pcs_ops {
  * When present, this overrides mac_pcs_get_state() in &struct
  * phylink_mac_ops.
  */
-void pcs_get_state(struct phylink_config *config,
+void pcs_get_state(struct phylink_pcs *pcs,
 		   struct phylink_link_state *state);
 
 /**
  * pcs_config() - Configure the PCS mode and advertisement
- * @config: a pointer to a &struct phylink_config.
+ * @pcs: a pointer to a &struct phylink_pcs.
  * @mode: one of %MLO_AN_FIXED, %MLO_AN_PHY, %MLO_AN_INBAND.
  * @interface: interface mode to be used
  * @advertising: adertisement ethtool link mode mask
@@ -331,21 +344,21 @@ void pcs_get_state(struct phylink_config *config,
  *
  * For most 10GBASE-R, there is no advertisement.
  */
-int (*pcs_config)(struct phylink_config *config, unsigned int mode,
-		  phy_interface_t interface, const unsigned long *advertising);
+int pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+	       phy_interface_t interface, const unsigned long *advertising);
 
 /**
  * pcs_an_restart() - restart 802.3z BaseX autonegotiation
- * @config: a pointer to a &struct phylink_config.
+ * @pcs: a pointer to a &struct phylink_pcs.
  *
  * When PCS ops are present, this overrides mac_an_restart() in &struct
  * phylink_mac_ops.
  */
-void (*pcs_an_restart)(struct phylink_config *config);
+void pcs_an_restart(struct phylink_pcs *pcs);
 
 /**
  * pcs_link_up() - program the PCS for the resolved link configuration
- * @config: a pointer to a &struct phylink_config.
+ * @pcs: a pointer to a &struct phylink_pcs.
  * @mode: link autonegotiation mode
  * @interface: link &typedef phy_interface_t mode
  * @speed: link speed
@@ -356,14 +369,14 @@ void (*pcs_an_restart)(struct phylink_config *config);
  * mode without in-band AN needs to be manually configured for the link
  * and duplex setting. Otherwise, this should be a no-op.
  */
-void (*pcs_link_up)(struct phylink_config *config, unsigned int mode,
-		    phy_interface_t interface, int speed, int duplex);
+void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
+		 phy_interface_t interface, int speed, int duplex);
 #endif
 
 struct phylink *phylink_create(struct phylink_config *, struct fwnode_handle *,
 			       phy_interface_t iface,
 			       const struct phylink_mac_ops *mac_ops);
-void phylink_add_pcs(struct phylink *, const struct phylink_pcs_ops *ops);
+void phylink_set_pcs(struct phylink *, struct phylink_pcs *pcs);
 void phylink_destroy(struct phylink *);
 
 int phylink_connect_phy(struct phylink *, struct phy_device *);
-- 
2.20.1


-- 
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 v3 5/9] net: dsa: add support for phylink_pcs_ops

[Russell King - ARM Linux admin]

On Mon, Jun 22, 2020 at 12:10:57PM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Jun 22, 2020 at 11:22:13AM +0100, Russell King - ARM Linux admin wrote:
> > On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote:
> > > In order to support split PCS using PHYLINK properly, we need to add a
> > > phylink_pcs_ops structure.
> > > 
> > > Note that a DSA driver that wants to use these needs to implement all 4
> > > of them: the DSA core checks the presence of these 4 function pointers
> > > in dsa_switch_ops and only then does it add a PCS to PHYLINK. This is
> > > done in order to preserve compatibility with drivers that have not yet
> > > been converted, or don't need, a split PCS setup.
> > > 
> > > Also, when pcs_get_state() and pcs_an_restart() are present, their mac
> > > counterparts (mac_pcs_get_state(), mac_an_restart()) will no longer get
> > > called, as can be seen in phylink.c.
> > 
> > I don't like this at all, it means we've got all this useless layering,
> > and that layering will force similar layering veneers into other parts
> > of the kernel (such as the DPAA2 MAC driver, when we eventually come to
> > re-use pcs-lynx there.)
> > 
> > I don't think we need that - I think we can get to a position where
> > pcs-lynx is called requesting that it bind to phylink as the PCS, and
> > it calls phylink_add_pcs() directly, which means we do not end up with
> > veneers in DSA nor in the DPAA2 MAC driver - they just need to call
> > the pcs-lynx initialisation function with the phylink instance for it
> > to attach to.
> > 
> > Yes, that requires phylink_add_pcs() to change slightly, and for there
> > to be a PCS private pointer, as I have previously stated.  At the
> > moment, changing that is really easy - the phylink PCS backend has no
> > in-tree users at the moment.  So there is no reason not to get this
> > correct right now.
> 
> How about something like this?  I don't want to embed a mdio_device
> inside struct phylink_pcs because that would not be appropriate for
> some potential users (e.g., Marvell 88E6xxx DSA drivers, Marvell
> NETA, PP2, etc.)

Just to be clear, I'm expecting discussion on this approach, rather
than a patch series appearing - I've already tweaked this patch
slightly, adding a "poll" option to cater for the "pcs_poll" facility
that was introduced into the phylink_config structure - which I think
makes more sense here, as it's all part of the PCS.

I'd like there to be some concensus _before_ I go through the users
I have in my tree converting them to this, rather than converting
them, and then having to convert them to something else.

So, if we can agree on what this should look like, then I feel it
would make the development process a lot easier for everyone
concerned.

-- 
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 v3 5/9] net: dsa: add support for phylink_pcs_ops

[Ioana Ciornei]

> Subject: Re: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops
> 
> On Mon, Jun 22, 2020 at 12:10:57PM +0100, Russell King - ARM Linux admin
> wrote:
> > On Mon, Jun 22, 2020 at 11:22:13AM +0100, Russell King - ARM Linux admin
> wrote:
> > > On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote:
> > > > In order to support split PCS using PHYLINK properly, we need to
> > > > add a phylink_pcs_ops structure.
> > > >
> > > > Note that a DSA driver that wants to use these needs to implement
> > > > all 4 of them: the DSA core checks the presence of these 4
> > > > function pointers in dsa_switch_ops and only then does it add a
> > > > PCS to PHYLINK. This is done in order to preserve compatibility
> > > > with drivers that have not yet been converted, or don't need, a split PCS
> setup.
> > > >
> > > > Also, when pcs_get_state() and pcs_an_restart() are present, their
> > > > mac counterparts (mac_pcs_get_state(), mac_an_restart()) will no
> > > > longer get called, as can be seen in phylink.c.
> > >
> > > I don't like this at all, it means we've got all this useless
> > > layering, and that layering will force similar layering veneers into
> > > other parts of the kernel (such as the DPAA2 MAC driver, when we
> > > eventually come to re-use pcs-lynx there.)
> > >

The veneers that you are talking about are one phylink_pcs_ops structure
and 4 functions that call lynx_pcs_* subsequently. We have the same thing
for the MAC operations.

Also, the "veneers" in DSA are just how it works, and I don't want to change
its structure without a really good reason and without a green light from
DSA maintainers.

> > > I don't think we need that - I think we can get to a position where
> > > pcs-lynx is called requesting that it bind to phylink as the PCS,
> > > and it calls phylink_add_pcs() directly, which means we do not end
> > > up with veneers in DSA nor in the DPAA2 MAC driver - they just need
> > > to call the pcs-lynx initialisation function with the phylink
> > > instance for it to attach to.

What I am most concerned about is that by passing the PCS ops directly to the
PCS module we would lose any ability to apply SoC specific quirks at runtime
such as errata workarounds.

On the other hand, I am not sure what is the concrete benefit of doing it your way.
I understand that for a PHY device the MAC is not involved in the call path but in the
case of the PCS the expectation is that it's tightly coupled in the silicon
and not plug-and-play.

- Ioana

> > >
> > > Yes, that requires phylink_add_pcs() to change slightly, and for
> > > there to be a PCS private pointer, as I have previously stated.  At
> > > the moment, changing that is really easy - the phylink PCS backend
> > > has no in-tree users at the moment.  So there is no reason not to
> > > get this correct right now.
> >
> > How about something like this?  I don't want to embed a mdio_device
> > inside struct phylink_pcs because that would not be appropriate for
> > some potential users (e.g., Marvell 88E6xxx DSA drivers, Marvell NETA,
> > PP2, etc.)
> 
> Just to be clear, I'm expecting discussion on this approach, rather than a patch
> series appearing - I've already tweaked this patch slightly, adding a "poll" option
> to cater for the "pcs_poll" facility that was introduced into the phylink_config
> structure - which I think makes more sense here, as it's all part of the PCS.
> 
> I'd like there to be some concensus _before_ I go through the users I have in my
> tree converting them to this, rather than converting them, and then having to
> convert them to something else.
> 
> So, if we can agree on what this should look like, then I feel it would make the
> development process a lot easier for everyone concerned.
> 

Re: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops

[Russell King - ARM Linux admin]

On Mon, Jun 22, 2020 at 12:35:06PM +0000, Ioana Ciornei wrote:
> 
> > Subject: Re: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops
> > 
> > On Mon, Jun 22, 2020 at 12:10:57PM +0100, Russell King - ARM Linux admin
> > wrote:
> > > On Mon, Jun 22, 2020 at 11:22:13AM +0100, Russell King - ARM Linux admin
> > wrote:
> > > > On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote:
> > > > > In order to support split PCS using PHYLINK properly, we need to
> > > > > add a phylink_pcs_ops structure.
> > > > >
> > > > > Note that a DSA driver that wants to use these needs to implement
> > > > > all 4 of them: the DSA core checks the presence of these 4
> > > > > function pointers in dsa_switch_ops and only then does it add a
> > > > > PCS to PHYLINK. This is done in order to preserve compatibility
> > > > > with drivers that have not yet been converted, or don't need, a split PCS
> > setup.
> > > > >
> > > > > Also, when pcs_get_state() and pcs_an_restart() are present, their
> > > > > mac counterparts (mac_pcs_get_state(), mac_an_restart()) will no
> > > > > longer get called, as can be seen in phylink.c.
> > > >
> > > > I don't like this at all, it means we've got all this useless
> > > > layering, and that layering will force similar layering veneers into
> > > > other parts of the kernel (such as the DPAA2 MAC driver, when we
> > > > eventually come to re-use pcs-lynx there.)
> > > >
> 
> The veneers that you are talking about are one phylink_pcs_ops structure
> and 4 functions that call lynx_pcs_* subsequently. We have the same thing
> for the MAC operations.
> 
> Also, the "veneers" in DSA are just how it works, and I don't want to change
> its structure without a really good reason and without a green light from
> DSA maintainers.

Right, but we're talking about hardware that is common not only in DSA
but elsewhere - and we already deal with that outside of DSA with PHYs.
So, what I'm proposing is really nothing new for DSA.

> > > > I don't think we need that - I think we can get to a position where
> > > > pcs-lynx is called requesting that it bind to phylink as the PCS,
> > > > and it calls phylink_add_pcs() directly, which means we do not end
> > > > up with veneers in DSA nor in the DPAA2 MAC driver - they just need
> > > > to call the pcs-lynx initialisation function with the phylink
> > > > instance for it to attach to.
> 
> What I am most concerned about is that by passing the PCS ops directly to the
> PCS module we would lose any ability to apply SoC specific quirks at runtime
> such as errata workarounds.

Do you know what those errata would be?  I'm only aware of A-011118 in
the LX2160A which I don't believe will impact this code.  I don't have
visibility of Ocelot/Felix.

> On the other hand, I am not sure what is the concrete benefit of doing
> it your way. I understand that for a PHY device the MAC is not involved
> in the call path but in the case of the PCS the expectation is that it's
> tightly coupled in the silicon and not plug-and-play.

The advantage is less lines of code to maintain, and a more efficient
and understandable code structure.  I would much rather start off
simple and then augment rather than start off with unnecessary
complexity and then get stuck with it while not really needing it.

-- 
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 v3 5/9] net: dsa: add support for phylink_pcs_ops

[Ioana Ciornei]

> Subject: Re: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops
> 
> On Mon, Jun 22, 2020 at 12:35:06PM +0000, Ioana Ciornei wrote:
> >
> > > Subject: Re: [PATCH net-next v3 5/9] net: dsa: add support for
> > > phylink_pcs_ops
> > >
> > > On Mon, Jun 22, 2020 at 12:10:57PM +0100, Russell King - ARM Linux
> > > admin
> > > wrote:
> > > > On Mon, Jun 22, 2020 at 11:22:13AM +0100, Russell King - ARM Linux
> > > > admin
> > > wrote:
> > > > > On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote:
> > > > > > In order to support split PCS using PHYLINK properly, we need
> > > > > > to add a phylink_pcs_ops structure.
> > > > > >
> > > > > > Note that a DSA driver that wants to use these needs to
> > > > > > implement all 4 of them: the DSA core checks the presence of
> > > > > > these 4 function pointers in dsa_switch_ops and only then does
> > > > > > it add a PCS to PHYLINK. This is done in order to preserve
> > > > > > compatibility with drivers that have not yet been converted,
> > > > > > or don't need, a split PCS
> > > setup.
> > > > > >
> > > > > > Also, when pcs_get_state() and pcs_an_restart() are present,
> > > > > > their mac counterparts (mac_pcs_get_state(), mac_an_restart())
> > > > > > will no longer get called, as can be seen in phylink.c.
> > > > >
> > > > > I don't like this at all, it means we've got all this useless
> > > > > layering, and that layering will force similar layering veneers
> > > > > into other parts of the kernel (such as the DPAA2 MAC driver,
> > > > > when we eventually come to re-use pcs-lynx there.)
> > > > >
> >
> > The veneers that you are talking about are one phylink_pcs_ops
> > structure and 4 functions that call lynx_pcs_* subsequently. We have
> > the same thing for the MAC operations.
> >
> > Also, the "veneers" in DSA are just how it works, and I don't want to
> > change its structure without a really good reason and without a green
> > light from DSA maintainers.
> 
> Right, but we're talking about hardware that is common not only in DSA but
> elsewhere - and we already deal with that outside of DSA with PHYs.

I said before why the PHY use case is different from a PCS tightly
coupled inside the SoC.

> So, what I'm proposing is really nothing new for DSA.
> 
> > > > > I don't think we need that - I think we can get to a position
> > > > > where pcs-lynx is called requesting that it bind to phylink as
> > > > > the PCS, and it calls phylink_add_pcs() directly, which means we
> > > > > do not end up with veneers in DSA nor in the DPAA2 MAC driver -
> > > > > they just need to call the pcs-lynx initialisation function with
> > > > > the phylink instance for it to attach to.
> >
> > What I am most concerned about is that by passing the PCS ops directly
> > to the PCS module we would lose any ability to apply SoC specific
> > quirks at runtime such as errata workarounds.
> 
> Do you know what those errata would be?  I'm only aware of A-011118 in the
> LX2160A which I don't believe will impact this code.  I don't have visibility of
> Ocelot/Felix.

I was mainly looking at this from a software architecture perspective, not having
any explicit erratum in mind.

> 
> > On the other hand, I am not sure what is the concrete benefit of doing
> > it your way. I understand that for a PHY device the MAC is not
> > involved in the call path but in the case of the PCS the expectation
> > is that it's tightly coupled in the silicon and not plug-and-play.
> 
> The advantage is less lines of code to maintain, and a more efficient and
> understandable code structure.  I would much rather start off simple and then
> augment rather than start off with unnecessary complexity and then get stuck
> with it while not really needing it.
> 

I am not going to argue ad infinitum on this. If you think keeping the
phylink_pcs_ops structure outside is the better approach then let's take that route. 

I will go through your feedback on the actual Lynx module and respond/make the
necessary adjustments.

Beside this, what should be our next move? Will you submit the new method of
working with the phylink_pcs_ops structure?

Ioana

Re: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops

[Russell King - ARM Linux admin]

On Mon, Jun 22, 2020 at 01:51:55PM +0000, Ioana Ciornei wrote:
> > Right, but we're talking about hardware that is common not only in DSA but
> > elsewhere - and we already deal with that outside of DSA with PHYs.
> 
> I said before why the PHY use case is different from a PCS tightly
> coupled inside the SoC.

Yes, however, I'm responding to your point about whether DSA maintainers
would be happy with it, so I've used the already existing example with
phylib to illustrate that this already happens in DSA-land.  I was not
meaning to refer to your point about the PCS being tightly coupled
inside the SoC.  That should've been clear by the comment immediately
below:

> > So, what I'm proposing is really nothing new for DSA.


> > Do you know what those errata would be?  I'm only aware of A-011118 in the
> > LX2160A which I don't believe will impact this code.  I don't have visibility of
> > Ocelot/Felix.
> 
> I was mainly looking at this from a software architecture perspective,
> not having any explicit erratum in mind.

Ok.

> > > On the other hand, I am not sure what is the concrete benefit of doing
> > > it your way. I understand that for a PHY device the MAC is not
> > > involved in the call path but in the case of the PCS the expectation
> > > is that it's tightly coupled in the silicon and not plug-and-play.
> > 
> > The advantage is less lines of code to maintain, and a more efficient and
> > understandable code structure.  I would much rather start off simple and then
> > augment rather than start off with unnecessary complexity and then get stuck
> > with it while not really needing it.
> > 
> 
> I am not going to argue ad infinitum on this. If you think keeping the
> phylink_pcs_ops structure outside is the better approach then let's
> take that route.
> 
> I will go through your feedback on the actual Lynx module and respond/make the
> necessary adjustments.
> 
> Beside this, what should be our next move? Will you submit the new method of
> working with the phylink_pcs_ops structure?

I do think it is the best approach _at the moment_ given what we know
at this time - without over-designing for situations that we don't know.

So, I'll take this as tentative agreement, and I'll begin work on
converting my local users, testing, and then sending a finished
patch.

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

[PATCH net-next v3 6/9] net: dsa: felix: unconditionally configure MAC speed to 1000Mbps

[Ioana Ciornei]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

In VSC9959, the PCS is the one who performs rate adaptation (symbol
duplication) to the speed negotiated by the PHY. The MAC is unaware of
that and must remain configured for gigabit. If it is configured at
OCELOT_SPEED_10 or OCELOT_SPEED_100, it'll start transmitting PAUSE
frames out of control and never recover, _even if_ we then reconfigure
it at OCELOT_SPEED_1000 afterwards.

This patch fixes a bug that luckily did not have any functional impact.
We were writing 10, 100, 1000 etc into this 2-bit field in
DEV_CLOCK_CFG, but the hardware expects values in the range 0, 1, 2, 3.
So all speed values were getting truncated to 0, which is
OCELOT_SPEED_2500, and which also appears to be fine.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v3:
 * patch added

 drivers/net/dsa/ocelot/felix.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 66648986e6e3..723d4cb89fdf 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -215,9 +215,14 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 	u32 mac_fc_cfg;
 
 	/* Take port out of reset by clearing the MAC_TX_RST, MAC_RX_RST and
-	 * PORT_RST bits in CLOCK_CFG
+	 * PORT_RST bits in DEV_CLOCK_CFG. Note that the way this system is
+	 * integrated is that the MAC speed is fixed and it's the PCS who is
+	 * performing the rate adaptation, so we have to write "1000Mbps" into
+	 * the LINK_SPEED field of DEV_CLOCK_CFG (which is also its default
+	 * value).
 	 */
-	ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(state->speed),
+	ocelot_port_writel(ocelot_port,
+			   DEV_CLOCK_CFG_LINK_SPEED(OCELOT_SPEED_1000),
 			   DEV_CLOCK_CFG);
 
 	/* Flow control. Link speed is only used here to evaluate the time
-- 
2.25.1

[PATCH net-next v3 7/9] net: dsa: felix: set proper pause frame timers based on link speed

[Ioana Ciornei]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

state->speed holds a value of 10, 100, 1000 or 2500, but
SYS_MAC_FC_CFG_FC_LINK_SPEED expects a value in the range 0, 1, 2 or 3.

So set the correct speed encoding into this register.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v3:
 * patch added

 drivers/net/dsa/ocelot/felix.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 723d4cb89fdf..98ae8421ef7f 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -225,10 +225,25 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 			   DEV_CLOCK_CFG_LINK_SPEED(OCELOT_SPEED_1000),
 			   DEV_CLOCK_CFG);
 
-	/* Flow control. Link speed is only used here to evaluate the time
-	 * specification in incoming pause frames.
-	 */
-	mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(state->speed);
+	switch (state->speed) {
+	case SPEED_10:
+		mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(3);
+		break;
+	case SPEED_100:
+		mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(2);
+		break;
+	case SPEED_1000:
+	case SPEED_2500:
+		mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(1);
+		break;
+	case SPEED_UNKNOWN:
+		mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(0);
+		break;
+	default:
+		dev_err(ocelot->dev, "Unsupported speed on port %d: %d\n",
+			port, state->speed);
+		return;
+	}
 
 	/* handle Rx pause in all cases, with 2500base-X this is used for rate
 	 * adaptation.
@@ -240,6 +255,10 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 			      SYS_MAC_FC_CFG_PAUSE_VAL_CFG(0xffff) |
 			      SYS_MAC_FC_CFG_FC_LATENCY_CFG(0x7) |
 			      SYS_MAC_FC_CFG_ZERO_PAUSE_ENA;
+
+	/* Flow control. Link speed is only used here to evaluate the time
+	 * specification in incoming pause frames.
+	 */
 	ocelot_write_rix(ocelot, mac_fc_cfg, SYS_MAC_FC_CFG, port);
 
 	ocelot_write_rix(ocelot, 0, ANA_POL_FLOWC, port);
-- 
2.25.1

[PATCH net-next v3 8/9] net: dsa: felix: use resolved link config in mac_link_up()

[Ioana Ciornei]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

PHYLINK now requires that parameters established through
auto-negotiation be written into the MAC at the time of the
mac_link_up() callback. In the case of felix, that means taking the port
out of reset, setting the correct timers for PAUSE frames, and
enabling/disabling TX flow control.

Note that this appears to be one of those cases where git fails
spectacularly: in the diff, it doesn't appear that code is moving from
felix_phylink_mac_config to felix_phylink_mac_link_up. Instead, it
appears as if code was moving _around_ it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v3:
 * patch added

 drivers/net/dsa/ocelot/felix.c | 83 +++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 42 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 98ae8421ef7f..f6a7e0839bb5 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -208,6 +208,41 @@ static int felix_phylink_mac_pcs_get_state(struct dsa_switch *ds, int port,
 static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 				     unsigned int link_an_mode,
 				     const struct phylink_link_state *state)
+{
+	struct ocelot *ocelot = ds->priv;
+	struct felix *felix = ocelot_to_felix(ocelot);
+
+	if (felix->info->pcs_init)
+		felix->info->pcs_init(ocelot, port, link_an_mode, state);
+}
+
+static void felix_phylink_mac_an_restart(struct dsa_switch *ds, int port)
+{
+	struct ocelot *ocelot = ds->priv;
+	struct felix *felix = ocelot_to_felix(ocelot);
+
+	if (felix->info->pcs_an_restart)
+		felix->info->pcs_an_restart(ocelot, port);
+}
+
+static void felix_phylink_mac_link_down(struct dsa_switch *ds, int port,
+					unsigned int link_an_mode,
+					phy_interface_t interface)
+{
+	struct ocelot *ocelot = ds->priv;
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+
+	ocelot_port_writel(ocelot_port, 0, DEV_MAC_ENA_CFG);
+	ocelot_rmw_rix(ocelot, 0, QSYS_SWITCH_PORT_MODE_PORT_ENA,
+		       QSYS_SWITCH_PORT_MODE, port);
+}
+
+static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
+				      unsigned int link_an_mode,
+				      phy_interface_t interface,
+				      struct phy_device *phydev,
+				      int speed, int duplex,
+				      bool tx_pause, bool rx_pause)
 {
 	struct ocelot *ocelot = ds->priv;
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
@@ -225,7 +260,7 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 			   DEV_CLOCK_CFG_LINK_SPEED(OCELOT_SPEED_1000),
 			   DEV_CLOCK_CFG);
 
-	switch (state->speed) {
+	switch (speed) {
 	case SPEED_10:
 		mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(3);
 		break;
@@ -241,7 +276,7 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 		break;
 	default:
 		dev_err(ocelot->dev, "Unsupported speed on port %d: %d\n",
-			port, state->speed);
+			port, speed);
 		return;
 	}
 
@@ -250,7 +285,7 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 	 */
 	mac_fc_cfg |= SYS_MAC_FC_CFG_RX_FC_ENA;
 
-	if (state->pause & MLO_PAUSE_TX)
+	if (tx_pause)
 		mac_fc_cfg |= SYS_MAC_FC_CFG_TX_FC_ENA |
 			      SYS_MAC_FC_CFG_PAUSE_VAL_CFG(0xffff) |
 			      SYS_MAC_FC_CFG_FC_LATENCY_CFG(0x7) |
@@ -263,45 +298,6 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 
 	ocelot_write_rix(ocelot, 0, ANA_POL_FLOWC, port);
 
-	if (felix->info->pcs_init)
-		felix->info->pcs_init(ocelot, port, link_an_mode, state);
-
-	if (felix->info->port_sched_speed_set)
-		felix->info->port_sched_speed_set(ocelot, port,
-						  state->speed);
-}
-
-static void felix_phylink_mac_an_restart(struct dsa_switch *ds, int port)
-{
-	struct ocelot *ocelot = ds->priv;
-	struct felix *felix = ocelot_to_felix(ocelot);
-
-	if (felix->info->pcs_an_restart)
-		felix->info->pcs_an_restart(ocelot, port);
-}
-
-static void felix_phylink_mac_link_down(struct dsa_switch *ds, int port,
-					unsigned int link_an_mode,
-					phy_interface_t interface)
-{
-	struct ocelot *ocelot = ds->priv;
-	struct ocelot_port *ocelot_port = ocelot->ports[port];
-
-	ocelot_port_writel(ocelot_port, 0, DEV_MAC_ENA_CFG);
-	ocelot_rmw_rix(ocelot, 0, QSYS_SWITCH_PORT_MODE_PORT_ENA,
-		       QSYS_SWITCH_PORT_MODE, port);
-}
-
-static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
-				      unsigned int link_an_mode,
-				      phy_interface_t interface,
-				      struct phy_device *phydev,
-				      int speed, int duplex,
-				      bool tx_pause, bool rx_pause)
-{
-	struct ocelot *ocelot = ds->priv;
-	struct ocelot_port *ocelot_port = ocelot->ports[port];
-
 	/* Enable MAC module */
 	ocelot_port_writel(ocelot_port, DEV_MAC_ENA_CFG_RX_ENA |
 			   DEV_MAC_ENA_CFG_TX_ENA, DEV_MAC_ENA_CFG);
@@ -319,6 +315,9 @@ 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->port_sched_speed_set)
+		felix->info->port_sched_speed_set(ocelot, port, speed);
 }
 
 static void felix_port_qos_map_init(struct ocelot *ocelot, int port)
-- 
2.25.1

[PATCH net-next v3 9/9] 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>
---
Changes in v2:
 * make the necessary adjustments for calling directly the exported Lynx
 functions
 * fixed a memory leakage in the Felix driver (the pcs structure was
 allocated twice)

Changes in v3:
 * use the DSA PCS operations

 drivers/net/dsa/ocelot/Kconfig         |   1 +
 drivers/net/dsa/ocelot/felix.c         |  46 ++-
 drivers/net/dsa/ocelot/felix.h         |  16 +-
 drivers/net/dsa/ocelot/felix_vsc9959.c | 385 +++----------------------
 include/linux/fsl/enetc_mdio.h         |  21 --
 5 files changed, 76 insertions(+), 393 deletions(-)

diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
index 3d3c2a6fb0c0..4dd4d2742cda 100644
--- a/drivers/net/dsa/ocelot/Kconfig
+++ b/drivers/net/dsa/ocelot/Kconfig
@@ -9,6 +9,7 @@ config NET_DSA_MSCC_FELIX
 	select MSCC_OCELOT_SWITCH_LIB
 	select NET_DSA_TAG_OCELOT
 	select FSL_ENETC_MDIO
+	select PCS_LYNX
 	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 f6a7e0839bb5..275d2d5d9709 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -193,30 +193,32 @@ static void felix_phylink_validate(struct dsa_switch *ds, int port,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS);
 }
 
-static int felix_phylink_mac_pcs_get_state(struct dsa_switch *ds, int port,
-					   struct phylink_link_state *state)
+static void felix_phylink_pcs_get_state(struct dsa_switch *ds, int port,
+					struct phylink_link_state *state)
 {
 	struct ocelot *ocelot = ds->priv;
 	struct felix *felix = ocelot_to_felix(ocelot);
 
-	if (felix->info->pcs_link_state)
-		felix->info->pcs_link_state(ocelot, port, state);
-
-	return 0;
+	if (felix->info->pcs_get_state)
+		felix->info->pcs_get_state(ocelot, port, state);
 }
 
-static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
-				     unsigned int link_an_mode,
-				     const struct phylink_link_state *state)
+static int felix_phylink_pcs_config(struct dsa_switch *ds, int port,
+				    unsigned int link_an_mode,
+				    phy_interface_t interface,
+				    const unsigned long *advertising)
 {
 	struct ocelot *ocelot = ds->priv;
 	struct felix *felix = ocelot_to_felix(ocelot);
 
-	if (felix->info->pcs_init)
-		felix->info->pcs_init(ocelot, port, link_an_mode, state);
+	if (felix->info->pcs_config)
+		return felix->info->pcs_config(ocelot, port, link_an_mode,
+					       interface, advertising);
+
+	return 0;
 }
 
-static void felix_phylink_mac_an_restart(struct dsa_switch *ds, int port)
+static void felix_phylink_pcs_an_restart(struct dsa_switch *ds, int port)
 {
 	struct ocelot *ocelot = ds->priv;
 	struct felix *felix = ocelot_to_felix(ocelot);
@@ -320,6 +322,19 @@ static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
 		felix->info->port_sched_speed_set(ocelot, port, speed);
 }
 
+static void felix_phylink_pcs_link_up(struct dsa_switch *ds, int port,
+				      unsigned int link_an_mode,
+				      phy_interface_t interface,
+				      int speed, int duplex)
+{
+	struct ocelot *ocelot = ds->priv;
+	struct felix *felix = ocelot_to_felix(ocelot);
+
+	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)
 {
 	int i;
@@ -783,10 +798,11 @@ static const struct dsa_switch_ops felix_switch_ops = {
 	.get_ethtool_stats	= felix_get_ethtool_stats,
 	.get_sset_count		= felix_get_sset_count,
 	.get_ts_info		= felix_get_ts_info,
+	.phylink_pcs_config	= felix_phylink_pcs_config,
+	.phylink_pcs_link_up	= felix_phylink_pcs_link_up,
+	.phylink_pcs_get_state	= felix_phylink_pcs_get_state,
+	.phylink_pcs_an_restart	= felix_phylink_pcs_an_restart,
 	.phylink_validate	= felix_phylink_validate,
-	.phylink_mac_link_state	= felix_phylink_mac_pcs_get_state,
-	.phylink_mac_config	= felix_phylink_mac_config,
-	.phylink_mac_an_restart	= felix_phylink_mac_an_restart,
 	.phylink_mac_link_down	= felix_phylink_mac_link_down,
 	.phylink_mac_link_up	= felix_phylink_mac_link_up,
 	.port_enable		= felix_port_enable,
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index a891736ca006..e30266c32db5 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -28,12 +28,16 @@ struct felix_info {
 	int				imdio_pci_bar;
 	int	(*mdio_bus_alloc)(struct ocelot *ocelot);
 	void	(*mdio_bus_free)(struct ocelot *ocelot);
-	void	(*pcs_init)(struct ocelot *ocelot, int port,
-			    unsigned int link_an_mode,
-			    const struct phylink_link_state *state);
+	int	(*pcs_config)(struct ocelot *ocelot, int port,
+			      unsigned int link_an_mode,
+			      phy_interface_t interface,
+			      const unsigned long *advertising);
 	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_get_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 +59,7 @@ struct felix {
 	struct felix_info		*info;
 	struct ocelot			ocelot;
 	struct mii_bus			*imdio;
-	struct phy_device		**pcs;
+	struct mdio_device		**pcs;
 };
 
 #endif
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 2067776773f7..a1df55b26a79 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -8,6 +8,7 @@
 #include <soc/mscc/ocelot_ptp.h>
 #include <soc/mscc/ocelot_sys.h>
 #include <soc/mscc/ocelot.h>
+#include <linux/pcs-lynx.h>
 #include <net/pkt_sched.h>
 #include <linux/iopoll.h>
 #include <linux/pci.h>
@@ -17,19 +18,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,360 +716,54 @@ 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_device *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);
-	}
+	lynx_pcs_an_restart(pcs, ocelot->ports[port]->phy_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 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);
-}
-
-static void vsc9959_pcs_init(struct ocelot *ocelot, int port,
-			     unsigned int link_an_mode,
-			     const struct phylink_link_state *state)
+static int vsc9959_pcs_config(struct ocelot *ocelot, int port,
+			      unsigned int link_an_mode,
+			      phy_interface_t interface,
+			      const unsigned long *advertising)
 {
 	struct felix *felix = ocelot_to_felix(ocelot);
-	struct phy_device *pcs = felix->pcs[port];
+	struct mdio_device *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);
-}
-
-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)
-{
-	int err;
-
-	err = genphy_update_link(pcs);
-	if (err < 0)
-		return;
+		return 0;
 
-	pcs->speed = SPEED_2500;
-	pcs->asym_pause = true;
-	pcs->pause = true;
+	return lynx_pcs_config(pcs, link_an_mode, interface, advertising);
 }
 
-static void vsc9959_pcs_link_state_usxgmii(struct phy_device *pcs,
-					   struct phylink_link_state *state)
+static void vsc9959_pcs_get_state(struct ocelot *ocelot, int port,
+				  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;
+	struct felix *felix = ocelot_to_felix(ocelot);
+	struct mdio_device *pcs = felix->pcs[port];
 
-	lpa = phy_read_mmd(pcs, MDIO_MMD_VEND2, MII_LPA);
-	if (lpa < 0)
+	if (!pcs)
 		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;
+	lynx_pcs_get_state(pcs, ocelot->ports[port]->phy_mode, state);
 }
 
-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_device *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);
+	lynx_pcs_link_up(pcs, mode, interface, speed, duplex);
 }
 
 static int vsc9959_prevalidate_phy_mode(struct ocelot *ocelot, int port,
@@ -1123,7 +805,7 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 	int rc;
 
 	felix->pcs = devm_kcalloc(dev, felix->info->num_ports,
-				  sizeof(struct phy_device *),
+				  sizeof(struct mdio_device *),
 				  GFP_KERNEL);
 	if (!felix->pcs) {
 		dev_err(dev, "failed to allocate array for PCS PHYs\n");
@@ -1177,17 +859,17 @@ 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;
 
-		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_USXGMII)
-			is_c45 = true;
+		if (dsa_is_unused_port(felix->ds, port))
+			continue;
 
-		pcs = get_phy_device(felix->imdio, port, is_c45);
-		if (IS_ERR(pcs))
+		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
 			continue;
 
-		pcs->interface = ocelot_port->phy_mode;
+		pcs = mdio_device_create(felix->imdio, port);
+		if (IS_ERR(pcs))
+			continue;
 		felix->pcs[port] = pcs;
 
 		dev_info(dev, "Found PCS at internal MDIO address %d\n", port);
@@ -1202,12 +884,12 @@ 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_device *pcs = felix->pcs[port];
 
 		if (!pcs)
 			continue;
 
-		put_device(&pcs->mdio.dev);
+		mdio_device_free(pcs);
 	}
 	mdiobus_unregister(felix->imdio);
 }
@@ -1412,9 +1094,10 @@ struct felix_info felix_info_vsc9959 = {
 	.imdio_pci_bar		= 0,
 	.mdio_bus_alloc		= vsc9959_mdio_bus_alloc,
 	.mdio_bus_free		= vsc9959_mdio_bus_free,
-	.pcs_init		= vsc9959_pcs_init,
+	.pcs_config		= vsc9959_pcs_config,
 	.pcs_an_restart		= vsc9959_pcs_an_restart,
-	.pcs_link_state		= vsc9959_pcs_link_state,
+	.pcs_get_state		= vsc9959_pcs_get_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 v3 0/9] net: phy: add Lynx PCS MDIO module

[Russell King - ARM Linux admin]

On Mon, Jun 22, 2020 at 01:54:42AM +0300, Ioana Ciornei wrote:
> 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 an 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 (with and without in-band AN) 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 functional change is intended at this
> time.

Overall, I think we need to sort out the remaining changes in phylink
before moving forward with this patch set - I've made some progress
with Florian and the Broadcom DSA switches late last night.  I'm now
working on updating the felix DSA driver.

There's another reason - having looked at the work I did with this
same PHY, I think you are missing configuration of the link timer,
which is different in SGMII and 1000BASE-X.  Please can you look at
the code I came up with?  "dpaa2-mac: add 1000BASE-X/SGMII PCS support".

Thanks.

-- 
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 v3 0/9] net: phy: add Lynx PCS MDIO module

[Vladimir Oltean]

On Mon, 22 Jun 2020 at 12:29, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Mon, Jun 22, 2020 at 01:54:42AM +0300, Ioana Ciornei wrote:
> > 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 an 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 (with and without in-band AN) 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 functional change is intended at this
> > time.
>
> Overall, I think we need to sort out the remaining changes in phylink
> before moving forward with this patch set - I've made some progress
> with Florian and the Broadcom DSA switches late last night.  I'm now
> working on updating the felix DSA driver.
>

What needs to be done in the felix driver that is not part of this
series? Maybe you could review this instead?

> There's another reason - having looked at the work I did with this
> same PHY, I think you are missing configuration of the link timer,
> which is different in SGMII and 1000BASE-X.  Please can you look at
> the code I came up with?  "dpaa2-mac: add 1000BASE-X/SGMII PCS support".
>
> Thanks.
>

felix does not have support code for 1000base-x, so I think it's
natural to not clutter this series with things like that.
Things like USXGMII up to 10G, 10GBase-R, are also missing, for much
of the same reason - we wanted to make no functional change to the
existing code, precisely because we wanted it to go in quickly. There
are multiple things that are waiting for it:
- Michael Walle's enetc patches are going to use pcs-lynx
- The new Seville driver will use pcs-lynx

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

Thanks,
-Vladimir

Re: [PATCH net-next v3 0/9] net: phy: add Lynx PCS MDIO module

[Michael Walle]

Hi all,

Am 2020-06-22 11:34, schrieb Vladimir Oltean:
> On Mon, 22 Jun 2020 at 12:29, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
>> 
>> On Mon, Jun 22, 2020 at 01:54:42AM +0300, Ioana Ciornei wrote:
>> > 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 an 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 (with and without in-band AN) 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 functional change is intended at this
>> > time.
>> 
>> Overall, I think we need to sort out the remaining changes in phylink
>> before moving forward with this patch set - I've made some progress
>> with Florian and the Broadcom DSA switches late last night.  I'm now
>> working on updating the felix DSA driver.
>> 
> 
> What needs to be done in the felix driver that is not part of this
> series? Maybe you could review this instead?
> 
>> There's another reason - having looked at the work I did with this
>> same PHY, I think you are missing configuration of the link timer,
>> which is different in SGMII and 1000BASE-X.  Please can you look at
>> the code I came up with?  "dpaa2-mac: add 1000BASE-X/SGMII PCS 
>> support".
>> 
>> Thanks.
>> 
> 
> felix does not have support code for 1000base-x, so I think it's
> natural to not clutter this series with things like that.
> Things like USXGMII up to 10G, 10GBase-R, are also missing, for much
> of the same reason - we wanted to make no functional change to the
> existing code, precisely because we wanted it to go in quickly. There
> are multiple things that are waiting for it:
> - Michael Walle's enetc patches are going to use pcs-lynx

How likely is it that this will be sorted out before the 5.9 merge
window will be closed? The thing is, we have boards out in the
wild which have a non-working ethernet with their stock bootloader
and which depend on the following patch series to get that fixed:

https://lore.kernel.org/netdev/20200528063847.27704-1-michael@walle.cc/

Thus, if this is going to take longer, I'd do a respin of that
series. We already missed the 5.8 release and I don't know if
a "Fixes:" tag (or a CC stable) is appropriate here because it
is kind of a new functionality.

-michael

Re: [PATCH net-next v3 0/9] net: phy: add Lynx PCS MDIO module

[Vladimir Oltean]

Hi Michael,

On Tue, 30 Jun 2020 at 09:01, Michael Walle <michael@walle.cc> wrote:
>
> Hi all,
>
> Am 2020-06-22 11:34, schrieb Vladimir Oltean:
> > On Mon, 22 Jun 2020 at 12:29, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> >>
> >> On Mon, Jun 22, 2020 at 01:54:42AM +0300, Ioana Ciornei wrote:
> >> > 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 an 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 (with and without in-band AN) 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 functional change is intended at this
> >> > time.
> >>
> >> Overall, I think we need to sort out the remaining changes in phylink
> >> before moving forward with this patch set - I've made some progress
> >> with Florian and the Broadcom DSA switches late last night.  I'm now
> >> working on updating the felix DSA driver.
> >>
> >
> > What needs to be done in the felix driver that is not part of this
> > series? Maybe you could review this instead?
> >
> >> There's another reason - having looked at the work I did with this
> >> same PHY, I think you are missing configuration of the link timer,
> >> which is different in SGMII and 1000BASE-X.  Please can you look at
> >> the code I came up with?  "dpaa2-mac: add 1000BASE-X/SGMII PCS
> >> support".
> >>
> >> Thanks.
> >>
> >
> > felix does not have support code for 1000base-x, so I think it's
> > natural to not clutter this series with things like that.
> > Things like USXGMII up to 10G, 10GBase-R, are also missing, for much
> > of the same reason - we wanted to make no functional change to the
> > existing code, precisely because we wanted it to go in quickly. There
> > are multiple things that are waiting for it:
> > - Michael Walle's enetc patches are going to use pcs-lynx
>
> How likely is it that this will be sorted out before the 5.9 merge
> window will be closed? The thing is, we have boards out in the
> wild which have a non-working ethernet with their stock bootloader
> and which depend on the following patch series to get that fixed:
>
> https://lore.kernel.org/netdev/20200528063847.27704-1-michael@walle.cc/
>
> Thus, if this is going to take longer, I'd do a respin of that
> series. We already missed the 5.8 release and I don't know if
> a "Fixes:" tag (or a CC stable) is appropriate here because it
> is kind of a new functionality.
>
> -michael

From my side I think it is reasonable that you resubmit your enetc
patches using the existing framework. Looks like we're in for some
pretty significant API changes for phylink, not sure if you need to
depend on those if you just need the PCS to work.
But, I don't know if marketing your patches as "fixes" is going to go
that well. In fact, you are also moving some definitions around, from
felix to enetc. I think if you want to break dependencies from felix
here, you should leave the definitions where they are and just
duplicate them.

Thanks,
-Vladimir

Re: [PATCH net-next v3 0/9] net: phy: add Lynx PCS MDIO module

[Michael Walle]

Hi Vladimir,

Am 2020-07-01 15:37, schrieb Vladimir Oltean:
[..]
>> > felix does not have support code for 1000base-x, so I think it's
>> > natural to not clutter this series with things like that.
>> > Things like USXGMII up to 10G, 10GBase-R, are also missing, for much
>> > of the same reason - we wanted to make no functional change to the
>> > existing code, precisely because we wanted it to go in quickly. There
>> > are multiple things that are waiting for it:
>> > - Michael Walle's enetc patches are going to use pcs-lynx
>> 
>> How likely is it that this will be sorted out before the 5.9 merge
>> window will be closed? The thing is, we have boards out in the
>> wild which have a non-working ethernet with their stock bootloader
>> and which depend on the following patch series to get that fixed:
>> 
>> https://lore.kernel.org/netdev/20200528063847.27704-1-michael@walle.cc/
>> 
>> Thus, if this is going to take longer, I'd do a respin of that
>> series. We already missed the 5.8 release and I don't know if
>> a "Fixes:" tag (or a CC stable) is appropriate here because it
>> is kind of a new functionality.
>> 
>> -michael
> 
> From my side I think it is reasonable that you resubmit your enetc
> patches using the existing framework. Looks like we're in for some
> pretty significant API changes for phylink, not sure if you need to
> depend on those if you just need the PCS to work.

Ok, I'll resubmit them.

> But, I don't know if marketing your patches as "fixes" is going to go
> that well. In fact, you are also moving some definitions around, from
> felix to enetc. I think if you want to break dependencies from felix
> here, you should leave the definitions where they are and just
> duplicate them.

Nice idea, but I didn't intend to add the Fixes tag anyways. I'm fine
with saying, please use the newest kernel.

-michael