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

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

[Ioana Ciornei]

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

The module implements phylink_pcs_ops and exports a phylink_pcs
(incorporated into a lynx_pcs) which can be directly passed to phylink
through phylink_pcs_set.

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 and
Seville drivers in order to use the new common PCS implementation.

At the moment, USXGMII (only with in-band AN), 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)

Changes in v4:
 * use the newly introduced phylink PCS mechanism
 * install the phylink_pcs in the phylink_mac_config DSA ops
 * remove the direct implementations of the PCS ops
 * do no use the SGMII_ prefix when referring to the IF_MORE register
 * add a phylink helper to decode the USXGMII code word
 * remove cleanup patches for Felix (these have been already accepted)
 * Seville (recently introduced) now has PCS support through the same
 Lynx PCS module

Ioana Ciornei (5):
  net: phylink: add helper function to decode USXGMII word
  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: ocelot: use the Lynx PCS helpers in Felix and Seville

 MAINTAINERS                              |   7 +
 drivers/net/dsa/ocelot/Kconfig           |   1 +
 drivers/net/dsa/ocelot/felix.c           |  28 +-
 drivers/net/dsa/ocelot/felix.h           |  20 +-
 drivers/net/dsa/ocelot/felix_vsc9959.c   | 374 ++---------------------
 drivers/net/dsa/ocelot/seville_vsc9953.c |  21 +-
 drivers/net/phy/Kconfig                  |   6 +
 drivers/net/phy/Makefile                 |   1 +
 drivers/net/phy/pcs-lynx.c               | 314 +++++++++++++++++++
 drivers/net/phy/phylink.c                |  44 +++
 include/linux/mdio.h                     |   6 +
 include/linux/pcs-lynx.h                 |  21 ++
 include/linux/phylink.h                  |   3 +
 13 files changed, 442 insertions(+), 404 deletions(-)
 create mode 100644 drivers/net/phy/pcs-lynx.c
 create mode 100644 include/linux/pcs-lynx.h

-- 
2.25.1

[PATCH net-next v4 1/5] net: phylink: add helper function to decode USXGMII word

[Ioana Ciornei]

With the new addition of the USXGMII link partner ability constants we
can now introduce a phylink helper that decodes the USXGMII word and
populates the appropriate fields in the phylink_link_state structure
based on them.

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

 drivers/net/phy/phylink.c | 43 +++++++++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |  3 +++
 2 files changed, 46 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 32b4bd6a5b55..d7810c908bb3 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2318,6 +2318,49 @@ static void phylink_decode_sgmii_word(struct phylink_link_state *state,
 		state->duplex = DUPLEX_HALF;
 }
 
+/**
+ * phylink_decode_usxgmii_word() - decode the USXGMII word from a MAC PCS
+ * @state: a pointer to a struct phylink_link_state.
+ * @lpa: a 16 bit value which stores the USXGMII auto-negotiation word
+ *
+ * Helper for MAC PCS supporting the USXGMII protocol and the auto-negotiation
+ * code word.  Decode the USXGMII code word and populate the corresponding fields
+ * (speed, duplex) into the phylink_link_state structure.
+ */
+void phylink_decode_usxgmii_word(struct phylink_link_state *state,
+				 uint16_t lpa)
+{
+	switch (lpa & MDIO_USXGMII_SPD_MASK) {
+	case MDIO_USXGMII_10:
+		state->speed = SPEED_10;
+		break;
+	case MDIO_USXGMII_100:
+		state->speed = SPEED_100;
+		break;
+	case MDIO_USXGMII_1000:
+		state->speed = SPEED_1000;
+		break;
+	case MDIO_USXGMII_2500:
+		state->speed = SPEED_2500;
+		break;
+	case MDIO_USXGMII_5000:
+		state->speed = SPEED_5000;
+		break;
+	case MDIO_USXGMII_10G:
+		state->speed = SPEED_10000;
+		break;
+	default:
+		state->link = false;
+		return;
+	}
+
+	if (lpa & MDIO_USXGMII_FULL_DUPLEX)
+		state->duplex = DUPLEX_FULL;
+	else
+		state->duplex = DUPLEX_HALF;
+}
+EXPORT_SYMBOL_GPL(phylink_decode_usxgmii_word);
+
 /**
  * phylink_mii_c22_pcs_get_state() - read the MAC PCS state
  * @pcs: a pointer to a &struct mdio_device.
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 1aad2aea4610..83fc149a7bd7 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -485,4 +485,7 @@ void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs);
 
 void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs,
 				   struct phylink_link_state *state);
+
+void phylink_decode_usxgmii_word(struct phylink_link_state *state,
+				 uint16_t lpa);
 #endif
-- 
2.25.1

Re: [PATCH net-next v4 1/5] net: phylink: add helper function to decode USXGMII word

[Russell King - ARM Linux admin]

On Fri, Jul 24, 2020 at 11:01:39AM +0300, Ioana Ciornei wrote:
> With the new addition of the USXGMII link partner ability constants we
> can now introduce a phylink helper that decodes the USXGMII word and
> populates the appropriate fields in the phylink_link_state structure
> based on them.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>

Looks good, thanks.

Reviewed-by: Russell King <rmk+kernel@armlinux.org.uk>

> ---
> Changes in v4:
>  - patch added
> 
>  drivers/net/phy/phylink.c | 43 +++++++++++++++++++++++++++++++++++++++
>  include/linux/phylink.h   |  3 +++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 32b4bd6a5b55..d7810c908bb3 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2318,6 +2318,49 @@ static void phylink_decode_sgmii_word(struct phylink_link_state *state,
>  		state->duplex = DUPLEX_HALF;
>  }
>  
> +/**
> + * phylink_decode_usxgmii_word() - decode the USXGMII word from a MAC PCS
> + * @state: a pointer to a struct phylink_link_state.
> + * @lpa: a 16 bit value which stores the USXGMII auto-negotiation word
> + *
> + * Helper for MAC PCS supporting the USXGMII protocol and the auto-negotiation
> + * code word.  Decode the USXGMII code word and populate the corresponding fields
> + * (speed, duplex) into the phylink_link_state structure.
> + */
> +void phylink_decode_usxgmii_word(struct phylink_link_state *state,
> +				 uint16_t lpa)
> +{
> +	switch (lpa & MDIO_USXGMII_SPD_MASK) {
> +	case MDIO_USXGMII_10:
> +		state->speed = SPEED_10;
> +		break;
> +	case MDIO_USXGMII_100:
> +		state->speed = SPEED_100;
> +		break;
> +	case MDIO_USXGMII_1000:
> +		state->speed = SPEED_1000;
> +		break;
> +	case MDIO_USXGMII_2500:
> +		state->speed = SPEED_2500;
> +		break;
> +	case MDIO_USXGMII_5000:
> +		state->speed = SPEED_5000;
> +		break;
> +	case MDIO_USXGMII_10G:
> +		state->speed = SPEED_10000;
> +		break;
> +	default:
> +		state->link = false;
> +		return;
> +	}
> +
> +	if (lpa & MDIO_USXGMII_FULL_DUPLEX)
> +		state->duplex = DUPLEX_FULL;
> +	else
> +		state->duplex = DUPLEX_HALF;
> +}
> +EXPORT_SYMBOL_GPL(phylink_decode_usxgmii_word);
> +
>  /**
>   * phylink_mii_c22_pcs_get_state() - read the MAC PCS state
>   * @pcs: a pointer to a &struct mdio_device.
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 1aad2aea4610..83fc149a7bd7 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -485,4 +485,7 @@ void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs);
>  
>  void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs,
>  				   struct phylink_link_state *state);
> +
> +void phylink_decode_usxgmii_word(struct phylink_link_state *state,
> +				 uint16_t lpa);
>  #endif
> -- 
> 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!

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

[Ioana Ciornei]

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

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v4:
 - 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 d7810c908bb3..0219ddf94e92 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2404,6 +2404,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

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

[Russell King - ARM Linux admin]

On Fri, Jul 24, 2020 at 11:01:40AM +0300, Ioana Ciornei wrote:
> 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>
> ---

Reviewed-by: Russell King <rmk+kernel@armlinux.org.uk>

> Changes in v4:
>  - 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 d7810c908bb3..0219ddf94e92 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2404,6 +2404,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
> 
> 

-- 
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 v4 3/5] net: mdiobus: add clause 45 mdiobus write accessor

[Ioana Ciornei]

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

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v4:
 - none

 include/linux/mdio.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 898cbf00332a..3a88b699b758 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -358,6 +358,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

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

[Russell King - ARM Linux admin]

On Fri, Jul 24, 2020 at 11:01:41AM +0300, Ioana Ciornei wrote:
> Add the locked variant of the clause 45 mdiobus write accessor -
> mdiobus_c45_write().
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---

Reviewed-by: Russell King <rmk+kernel@armlinux.org.uk>

Thanks.

> Changes in v4:
>  - none
> 
>  include/linux/mdio.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/mdio.h b/include/linux/mdio.h
> index 898cbf00332a..3a88b699b758 100644
> --- a/include/linux/mdio.h
> +++ b/include/linux/mdio.h
> @@ -358,6 +358,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
> 
> 

-- 
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 v4 4/5] 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, 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 v4:
 * use the newly introduced phylink PCS mechanism
 * do no use the SGMII_ prefix when referring to the IF_MORE register

 MAINTAINERS                |   7 +
 drivers/net/phy/Kconfig    |   6 +
 drivers/net/phy/Makefile   |   1 +
 drivers/net/phy/pcs-lynx.c | 314 +++++++++++++++++++++++++++++++++++++
 include/linux/pcs-lynx.h   |  21 +++
 5 files changed, 349 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 6200eb14757c..bfd0d174f4e4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10158,6 +10158,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:	Supported
+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 dd20c2c27c2f..f8016e64e1a5 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -239,6 +239,12 @@ config MDIO_XPCS
 	  This module provides helper functions for Synopsys DesignWare XPCS
 	  controllers.
 
+config PCS_LYNX
+	tristate
+	help
+	  This module provides helpers to phylink for managing the Lynx PCS
+	  which is part of the Layerscape and QorIQ Ethernet SERDES.
+
 endif
 endif
 
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index d84bab489a53..0e5539c05c81 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -48,6 +48,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..9ea8b90a1350
--- /dev/null
+++ b/drivers/net/phy/pcs-lynx.c
@@ -0,0 +1,314 @@
+// 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 LINK_TIMER_VAL(ns)		((u32)((ns) / SGMII_CLOCK_PERIOD_NS))
+
+#define SGMII_AN_LINK_TIMER_NS		1600000 /* defined by SGMII spec */
+
+#define LINK_TIMER_LO			0x12
+#define LINK_TIMER_HI			0x13
+#define IF_MODE				0x14
+#define IF_MODE_SGMII_EN		BIT(0)
+#define IF_MODE_USE_SGMII_AN		BIT(1)
+#define IF_MODE_SPEED(x)		(((x) << 2) & GENMASK(3, 2))
+#define IF_MODE_SPEED_MSK		GENMASK(3, 2)
+#define IF_MODE_DUPLEX			BIT(4)
+
+enum sgmii_speed {
+	SGMII_SPEED_10		= 0,
+	SGMII_SPEED_100		= 1,
+	SGMII_SPEED_1000	= 2,
+	SGMII_SPEED_2500	= 2,
+};
+
+#define phylink_pcs_to_lynx(pl_pcs) container_of((pl_pcs), struct lynx_pcs, pcs)
+
+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;
+
+	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;
+	state->duplex = DUPLEX_FULL;
+}
+
+static void lynx_pcs_get_state(struct phylink_pcs *pcs,
+			       struct phylink_link_state *state)
+{
+	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		phylink_mii_c22_pcs_get_state(lynx->mdio, state);
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		lynx_pcs_get_state_2500basex(lynx->mdio, state);
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		lynx_pcs_get_state_usxgmii(lynx->mdio, state);
+		break;
+	default:
+		break;
+	}
+
+	dev_dbg(&lynx->mdio->dev,
+		"mode=%s/%s/%s link=%u an_enabled=%u an_complete=%u\n",
+		phy_modes(state->interface),
+		phy_speed_to_str(state->speed),
+		phy_duplex_to_str(state->duplex),
+		state->link, state->an_enabled, state->an_complete);
+}
+
+static int lynx_pcs_config_sgmii(struct mdio_device *pcs, unsigned int mode,
+				 const unsigned long *advertising)
+{
+	struct mii_bus *bus = pcs->bus;
+	int addr = pcs->addr;
+	u16 if_mode;
+	int err;
+
+	if_mode = IF_MODE_SGMII_EN;
+	if (mode == MLO_AN_INBAND) {
+		u32 link_timer;
+
+		if_mode |= IF_MODE_USE_SGMII_AN;
+
+		/* Adjust link timer for SGMII */
+		link_timer = LINK_TIMER_VAL(SGMII_AN_LINK_TIMER_NS);
+		mdiobus_write(bus, addr, LINK_TIMER_LO, link_timer & 0xffff);
+		mdiobus_write(bus, addr, LINK_TIMER_HI, link_timer >> 16);
+	}
+	mdiobus_modify(bus, addr, IF_MODE,
+		       IF_MODE_SGMII_EN | 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;
+
+	if (!phylink_autoneg_inband(mode)) {
+		dev_err(&pcs->dev, "USXGMII only supports in-band AN for now\n");
+		return -EOPNOTSUPP;
+	}
+
+	/* Configure device ability for the USXGMII Replicator */
+	mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, MII_ADVERTISE,
+			  MDIO_USXGMII_10G | MDIO_USXGMII_LINK |
+			  MDIO_USXGMII_FULL_DUPLEX |
+			  ADVERTISE_SGMII | ADVERTISE_LPACK);
+	return 0;
+}
+
+static int lynx_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+			   phy_interface_t ifmode,
+			   const unsigned long *advertising,
+			   bool permit)
+{
+	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
+
+	switch (ifmode) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		lynx_pcs_config_sgmii(lynx->mdio, mode, advertising);
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		if (phylink_autoneg_inband(mode)) {
+			dev_err(&lynx->mdio->dev,
+				"AN not supported on 3.125GHz SerDes lane\n");
+			return -EOPNOTSUPP;
+		}
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		lynx_pcs_config_usxgmii(lynx->mdio, mode, advertising);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static void lynx_pcs_link_up_sgmii(struct mdio_device *pcs, unsigned int mode,
+				   int speed, int duplex)
+{
+	struct mii_bus *bus = pcs->bus;
+	u16 if_mode = 0, sgmii_speed;
+	int addr = pcs->addr;
+
+	/* The PCS needs to be configured manually only
+	 * when not operating on in-band mode
+	 */
+	if (mode == MLO_AN_INBAND)
+		return;
+
+	if (duplex == DUPLEX_HALF)
+		if_mode |= 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 |= IF_MODE_SPEED(sgmii_speed);
+
+	mdiobus_modify(bus, addr, IF_MODE,
+		       IF_MODE_DUPLEX | 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;
+	u16 if_mode = 0;
+
+	if (mode == MLO_AN_INBAND) {
+		dev_err(&pcs->dev, "AN not supported for 2500BaseX\n");
+		return;
+	}
+
+	if (duplex == DUPLEX_HALF)
+		if_mode |= IF_MODE_DUPLEX;
+	if_mode |= IF_MODE_SPEED(SGMII_SPEED_2500);
+
+	mdiobus_modify(bus, addr, IF_MODE,
+		       IF_MODE_DUPLEX | IF_MODE_SPEED_MSK,
+		       if_mode);
+}
+
+static void lynx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
+			     phy_interface_t interface,
+			     int speed, int duplex)
+{
+	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
+
+	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		lynx_pcs_link_up_sgmii(lynx->mdio, mode, speed, duplex);
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		lynx_pcs_link_up_2500basex(lynx->mdio, 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;
+	}
+}
+
+static const struct phylink_pcs_ops lynx_pcs_phylink_ops = {
+	.pcs_get_state = lynx_pcs_get_state,
+	.pcs_config = lynx_pcs_config,
+	.pcs_link_up = lynx_pcs_link_up,
+};
+
+struct lynx_pcs *lynx_pcs_create(struct mdio_device *mdio)
+{
+	struct lynx_pcs *lynx_pcs;
+
+	lynx_pcs = kzalloc(sizeof(*lynx_pcs), GFP_KERNEL);
+	if (!lynx_pcs)
+		return NULL;
+
+	lynx_pcs->mdio = mdio;
+	lynx_pcs->pcs.ops = &lynx_pcs_phylink_ops;
+	lynx_pcs->pcs.poll = true;
+
+	return lynx_pcs;
+}
+EXPORT_SYMBOL(lynx_pcs_create);
+
+void lynx_pcs_destroy(struct lynx_pcs *pcs)
+{
+	kfree(pcs);
+}
+EXPORT_SYMBOL(lynx_pcs_destroy);
+
+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..a6440d6ebe95
--- /dev/null
+++ b/include/linux/pcs-lynx.h
@@ -0,0 +1,21 @@
+/* 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/mdio.h>
+#include <linux/phylink.h>
+
+struct lynx_pcs {
+	struct phylink_pcs pcs;
+	struct mdio_device *mdio;
+};
+
+struct lynx_pcs *lynx_pcs_create(struct mdio_device *mdio);
+
+void lynx_pcs_destroy(struct lynx_pcs *pcs);
+
+#endif /* __LINUX_PCS_LYNX_H */
-- 
2.25.1

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

[Russell King - ARM Linux admin]

On Fri, Jul 24, 2020 at 11:01:42AM +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, 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.

Probably better to say that "As such, this aims at feature and bug
parity with the existing Felix DSA driver, and thus USXGMII, SGMII..."
in light of the comments below.

> 
> The module can only be enabled by the drivers in need and not user
> selectable.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---

Generally looks good, this time I've gone through it with a fine tooth
comb, so have picked up a load of issues, which may already exist in
the Felix code - don't feel you need to address these right now, but
please address them in the near future.

> Changes in v4:
>  * use the newly introduced phylink PCS mechanism
>  * do no use the SGMII_ prefix when referring to the IF_MORE register

IF_MODE

> 
>  MAINTAINERS                |   7 +
>  drivers/net/phy/Kconfig    |   6 +
>  drivers/net/phy/Makefile   |   1 +
>  drivers/net/phy/pcs-lynx.c | 314 +++++++++++++++++++++++++++++++++++++
>  include/linux/pcs-lynx.h   |  21 +++
>  5 files changed, 349 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 6200eb14757c..bfd0d174f4e4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10158,6 +10158,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:	Supported
> +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 dd20c2c27c2f..f8016e64e1a5 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -239,6 +239,12 @@ config MDIO_XPCS
>  	  This module provides helper functions for Synopsys DesignWare XPCS
>  	  controllers.
>  
> +config PCS_LYNX
> +	tristate
> +	help
> +	  This module provides helpers to phylink for managing the Lynx PCS
> +	  which is part of the Layerscape and QorIQ Ethernet SERDES.
> +
>  endif
>  endif
>  
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index d84bab489a53..0e5539c05c81 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -48,6 +48,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..9ea8b90a1350
> --- /dev/null
> +++ b/drivers/net/phy/pcs-lynx.c
> @@ -0,0 +1,314 @@
> +// 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 LINK_TIMER_VAL(ns)		((u32)((ns) / SGMII_CLOCK_PERIOD_NS))
> +
> +#define SGMII_AN_LINK_TIMER_NS		1600000 /* defined by SGMII spec */

I've been wondering why you've decided to work the timer out using the
clock period, when it's just as easy to do it via:

#define LINK_TIMER_US_SGMII     1600
#define DPAA2_MAC_PCS_CLK_MHZ   125

link_timer = DPAA2_MAC_PCS_CLK_MHZ * LINK_TIMER_US_SGMII;

It makes little difference either way, since the compiler will reduce
the code down to a single constant of 200000, except this way requires
less explanation.

> +
> +#define LINK_TIMER_LO			0x12
> +#define LINK_TIMER_HI			0x13
> +#define IF_MODE				0x14
> +#define IF_MODE_SGMII_EN		BIT(0)
> +#define IF_MODE_USE_SGMII_AN		BIT(1)
> +#define IF_MODE_SPEED(x)		(((x) << 2) & GENMASK(3, 2))
> +#define IF_MODE_SPEED_MSK		GENMASK(3, 2)

Consider switching these two definitions, and use IF_MODE_SPEED_MSK in
the definition of IF_MODE_SPEED().  Also, if we define the speeds
directly:

#define IF_MODE_SGMII_SPEED_10  (0 << 2)
#define IF_MODE_SGMII_SPEED_100 (1 << 2)
#define IF_MODE_SGMII_SPEED_1G  (2 << 2)
#define IF_MODE_SGMII_SPEED_2G5 (2 << 2)

Then there is no need for the sgmii_speed enum or the extra code to
merge that value into the if_mode register value.  ("2G5" is a way
to specify 2.5G - it comes from schematics, where resistors commonly
use 1K2 for 1.2K or capacitors commonly use 2n2 instead of 2.2n since
the "." can vanish or be missed.)

> +#define IF_MODE_DUPLEX			BIT(4)

Maybe IF_MODE_HALF_DUPLEX, since this bit is set for half duplex.
Helps to make the definition more descriptive.

> +
> +enum sgmii_speed {
> +	SGMII_SPEED_10		= 0,
> +	SGMII_SPEED_100		= 1,
> +	SGMII_SPEED_1000	= 2,
> +	SGMII_SPEED_2500	= 2,
> +};
> +
> +#define phylink_pcs_to_lynx(pl_pcs) container_of((pl_pcs), struct lynx_pcs, pcs)
> +
> +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)

lynx_pcs_get_state_2500basex() sets state->link false on error,
should there be some consistency?

> +		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;
> +
> +	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;
> +	state->duplex = DUPLEX_FULL;
> +}
> +
> +static void lynx_pcs_get_state(struct phylink_pcs *pcs,
> +			       struct phylink_link_state *state)
> +{
> +	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
> +
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_QSGMII:
> +		phylink_mii_c22_pcs_get_state(lynx->mdio, state);
> +		break;
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		lynx_pcs_get_state_2500basex(lynx->mdio, state);
> +		break;
> +	case PHY_INTERFACE_MODE_USXGMII:
> +		lynx_pcs_get_state_usxgmii(lynx->mdio, state);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	dev_dbg(&lynx->mdio->dev,
> +		"mode=%s/%s/%s link=%u an_enabled=%u an_complete=%u\n",
> +		phy_modes(state->interface),
> +		phy_speed_to_str(state->speed),
> +		phy_duplex_to_str(state->duplex),
> +		state->link, state->an_enabled, state->an_complete);
> +}
> +
> +static int lynx_pcs_config_sgmii(struct mdio_device *pcs, unsigned int mode,
> +				 const unsigned long *advertising)
> +{
> +	struct mii_bus *bus = pcs->bus;
> +	int addr = pcs->addr;
> +	u16 if_mode;
> +	int err;
> +
> +	if_mode = IF_MODE_SGMII_EN;
> +	if (mode == MLO_AN_INBAND) {
> +		u32 link_timer;
> +
> +		if_mode |= IF_MODE_USE_SGMII_AN;
> +
> +		/* Adjust link timer for SGMII */
> +		link_timer = LINK_TIMER_VAL(SGMII_AN_LINK_TIMER_NS);
> +		mdiobus_write(bus, addr, LINK_TIMER_LO, link_timer & 0xffff);
> +		mdiobus_write(bus, addr, LINK_TIMER_HI, link_timer >> 16);
> +	}
> +	mdiobus_modify(bus, addr, IF_MODE,
> +		       IF_MODE_SGMII_EN | IF_MODE_USE_SGMII_AN,
> +		       if_mode);

Should this be error-checked?

> +
> +	err = phylink_mii_c22_pcs_config(pcs, mode, PHY_INTERFACE_MODE_SGMII,
> +					 advertising);
> +	return err;

Consider just:

	return phylink_mii_c22_pcs_config(...);

and eliminate "err".

> +}
> +
> +static int lynx_pcs_config_usxgmii(struct mdio_device *pcs, unsigned int mode,
> +				   const unsigned long *advertising)

This makes no use of "advertising" - do you need to pass it?

> +{
> +	struct mii_bus *bus = pcs->bus;
> +	int addr = pcs->addr;
> +
> +	if (!phylink_autoneg_inband(mode)) {
> +		dev_err(&pcs->dev, "USXGMII only supports in-band AN for now\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/* Configure device ability for the USXGMII Replicator */
> +	mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, MII_ADVERTISE,
> +			  MDIO_USXGMII_10G | MDIO_USXGMII_LINK |
> +			  MDIO_USXGMII_FULL_DUPLEX |
> +			  ADVERTISE_SGMII | ADVERTISE_LPACK);
> +	return 0;

Should we propagate any error from mdiobus_c45_write()?

> +}
> +
> +static int lynx_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
> +			   phy_interface_t ifmode,
> +			   const unsigned long *advertising,
> +			   bool permit)
> +{
> +	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
> +
> +	switch (ifmode) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_QSGMII:
> +		lynx_pcs_config_sgmii(lynx->mdio, mode, advertising);

error propagation?

> +		break;
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		if (phylink_autoneg_inband(mode)) {
> +			dev_err(&lynx->mdio->dev,
> +				"AN not supported on 3.125GHz SerDes lane\n");
> +			return -EOPNOTSUPP;
> +		}
> +		break;

Doesn't this need to do some configuration so the PCS is not in SGMII
mode, possibly expecting some autonegotiation?

> +	case PHY_INTERFACE_MODE_USXGMII:
> +		lynx_pcs_config_usxgmii(lynx->mdio, mode, advertising);

error propagation?

> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static void lynx_pcs_link_up_sgmii(struct mdio_device *pcs, unsigned int mode,
> +				   int speed, int duplex)
> +{
> +	struct mii_bus *bus = pcs->bus;
> +	u16 if_mode = 0, sgmii_speed;
> +	int addr = pcs->addr;
> +
> +	/* The PCS needs to be configured manually only
> +	 * when not operating on in-band mode
> +	 */
> +	if (mode == MLO_AN_INBAND)
> +		return;
> +
> +	if (duplex == DUPLEX_HALF)
> +		if_mode |= 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 |= IF_MODE_SPEED(sgmii_speed);
> +
> +	mdiobus_modify(bus, addr, IF_MODE,
> +		       IF_MODE_DUPLEX | 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;
> +	u16 if_mode = 0;
> +
> +	if (mode == MLO_AN_INBAND) {
> +		dev_err(&pcs->dev, "AN not supported for 2500BaseX\n");
> +		return;
> +	}
> +
> +	if (duplex == DUPLEX_HALF)
> +		if_mode |= IF_MODE_DUPLEX;
> +	if_mode |= IF_MODE_SPEED(SGMII_SPEED_2500);
> +
> +	mdiobus_modify(bus, addr, IF_MODE,
> +		       IF_MODE_DUPLEX | IF_MODE_SPEED_MSK,
> +		       if_mode);

From what I read in the LX2160A manual, the IF_MODE speed and duplex
settings are only applicable for SGMII mode.  This has IF_MODE_SGMII_EN
clear, which means it is not in SGMII mode, and thus the duplex and
speed settings here do not seem to be meaningful.  Maybe a point to be
addressed in a future patch?

Another point - half duplex is rarely supported at 1G.  The comment
above talks about rate adaption, which means the properties on the
media side (speed, duplex above) have little meaning on the system
side of the interface; the link between the PHY and the MAC is
effectively full duplex, with the half duplex-ness handled by the
PHY.

> +}
> +
> +static void lynx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
> +			     phy_interface_t interface,
> +			     int speed, int duplex)
> +{
> +	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_QSGMII:
> +		lynx_pcs_link_up_sgmii(lynx->mdio, mode, speed, duplex);
> +		break;
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		lynx_pcs_link_up_2500basex(lynx->mdio, 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;
> +	}
> +}
> +
> +static const struct phylink_pcs_ops lynx_pcs_phylink_ops = {
> +	.pcs_get_state = lynx_pcs_get_state,
> +	.pcs_config = lynx_pcs_config,
> +	.pcs_link_up = lynx_pcs_link_up,
> +};
> +
> +struct lynx_pcs *lynx_pcs_create(struct mdio_device *mdio)
> +{
> +	struct lynx_pcs *lynx_pcs;
> +
> +	lynx_pcs = kzalloc(sizeof(*lynx_pcs), GFP_KERNEL);
> +	if (!lynx_pcs)
> +		return NULL;
> +
> +	lynx_pcs->mdio = mdio;
> +	lynx_pcs->pcs.ops = &lynx_pcs_phylink_ops;
> +	lynx_pcs->pcs.poll = true;

Great, an example of why pcs.poll is necessary!

> +
> +	return lynx_pcs;
> +}
> +EXPORT_SYMBOL(lynx_pcs_create);
> +
> +void lynx_pcs_destroy(struct lynx_pcs *pcs)
> +{
> +	kfree(pcs);
> +}
> +EXPORT_SYMBOL(lynx_pcs_destroy);
> +
> +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..a6440d6ebe95
> --- /dev/null
> +++ b/include/linux/pcs-lynx.h
> @@ -0,0 +1,21 @@
> +/* 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/mdio.h>
> +#include <linux/phylink.h>
> +
> +struct lynx_pcs {
> +	struct phylink_pcs pcs;
> +	struct mdio_device *mdio;
> +};
> +
> +struct lynx_pcs *lynx_pcs_create(struct mdio_device *mdio);
> +
> +void lynx_pcs_destroy(struct lynx_pcs *pcs);
> +
> +#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!

[PATCH net-next v4 5/5] net: dsa: ocelot: use the Lynx PCS helpers in Felix and Seville

[Ioana Ciornei]

Use the helper functions introduced by the newly added
Lynx PCS MDIO module in the Felix VSC9959 and Seville VSC9953.

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 monitoring 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>
---
Changes in v4:
 * install the phylink_pcs in the phylink_mac_config DSA ops
 * remove the direct implementations of the PCS ops
 * Seville (recently introduced) now has PCS support through the same
 Lynx PCS module

 drivers/net/dsa/ocelot/Kconfig           |   1 +
 drivers/net/dsa/ocelot/felix.c           |  28 +-
 drivers/net/dsa/ocelot/felix.h           |  20 +-
 drivers/net/dsa/ocelot/felix_vsc9959.c   | 374 ++---------------------
 drivers/net/dsa/ocelot/seville_vsc9953.c |  21 +-
 5 files changed, 40 insertions(+), 404 deletions(-)

diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
index f121619d81fe..795940048e49 100644
--- a/drivers/net/dsa/ocelot/Kconfig
+++ b/drivers/net/dsa/ocelot/Kconfig
@@ -8,6 +8,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 network switches from the the Vitesse /
 	  Microsemi / Microchip Ocelot family of switching cores that are
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index c69d9592a2b7..ccc0427faf02 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -19,6 +19,7 @@
 #include <linux/of_net.h>
 #include <linux/pci.h>
 #include <linux/of.h>
+#include <linux/pcs-lynx.h>
 #include <net/pkt_sched.h>
 #include <net/dsa.h>
 #include "felix.h"
@@ -196,27 +197,16 @@ static void felix_phylink_validate(struct dsa_switch *ds, int port,
 		felix->info->phylink_validate(ocelot, port, supported, state);
 }
 
-static int felix_phylink_mac_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;
-}
-
 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);
+	struct dsa_port *dp = dsa_to_port(ds, port);
 
-	if (felix->info->pcs_config)
-		felix->info->pcs_config(ocelot, port, link_an_mode, state);
+	if (felix->pcs[port])
+		phylink_set_pcs(dp->pl, &felix->pcs[port]->pcs);
 }
 
 static void felix_phylink_mac_link_down(struct dsa_switch *ds, int port,
@@ -306,10 +296,6 @@ static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
 	ocelot_fields_write(ocelot, port,
 			    QSYS_SWITCH_PORT_MODE_PORT_ENA, 1);
 
-	if (felix->info->pcs_link_up)
-		felix->info->pcs_link_up(ocelot, port, link_an_mode, interface,
-					 speed, duplex);
-
 	if (felix->info->port_sched_speed_set)
 		felix->info->port_sched_speed_set(ocelot, port, speed);
 }
@@ -626,11 +612,6 @@ static int felix_setup(struct dsa_switch *ds)
 
 	ds->mtu_enforcement_ingress = true;
 	ds->configure_vlan_while_not_filtering = true;
-	/* It looks like the MAC/PCS interrupt register - PM0_IEVENT (0x8040)
-	 * isn't instantiated for the Felix PF.
-	 * In-band AN may take a few ms to complete, so we need to poll.
-	 */
-	ds->pcs_poll = true;
 
 	return 0;
 }
@@ -786,7 +767,6 @@ const struct dsa_switch_ops felix_switch_ops = {
 	.get_sset_count		= felix_get_sset_count,
 	.get_ts_info		= felix_get_ts_info,
 	.phylink_validate	= felix_phylink_validate,
-	.phylink_mac_link_state	= felix_phylink_mac_pcs_get_state,
 	.phylink_mac_config	= felix_phylink_mac_config,
 	.phylink_mac_link_down	= felix_phylink_mac_link_down,
 	.phylink_mac_link_up	= felix_phylink_mac_link_up,
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index 98f14621ac23..9bceb994b7db 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -28,15 +28,6 @@ struct felix_info {
 	int				imdio_pci_bar;
 	int	(*mdio_bus_alloc)(struct ocelot *ocelot);
 	void	(*mdio_bus_free)(struct ocelot *ocelot);
-	void	(*pcs_config)(struct ocelot *ocelot, int port,
-			      unsigned int link_an_mode,
-			      const struct phylink_link_state *state);
-	void	(*pcs_link_up)(struct ocelot *ocelot, int port,
-			       unsigned int link_an_mode,
-			       phy_interface_t interface,
-			       int speed, int duplex);
-	void	(*pcs_link_state)(struct ocelot *ocelot, int port,
-				  struct phylink_link_state *state);
 	void	(*phylink_validate)(struct ocelot *ocelot, int port,
 				    unsigned long *supported,
 				    struct phylink_link_state *state);
@@ -59,20 +50,11 @@ struct felix {
 	const struct felix_info		*info;
 	struct ocelot			ocelot;
 	struct mii_bus			*imdio;
-	struct phy_device		**pcs;
+	struct lynx_pcs			**pcs;
 	resource_size_t			switch_base;
 	resource_size_t			imdio_base;
 };
 
-void vsc9959_pcs_link_state(struct ocelot *ocelot, int port,
-			    struct phylink_link_state *state);
-void vsc9959_pcs_config(struct ocelot *ocelot, int port,
-			unsigned int link_an_mode,
-			const struct phylink_link_state *state);
-void vsc9959_pcs_link_up(struct ocelot *ocelot, int port,
-			 unsigned int link_an_mode,
-			 phy_interface_t interface,
-			 int speed, int duplex);
 void vsc9959_mdio_bus_free(struct ocelot *ocelot);
 
 #endif
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 9b720c8ddfc3..126a53a811f7 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -9,6 +9,7 @@
 #include <soc/mscc/ocelot_sys.h>
 #include <soc/mscc/ocelot.h>
 #include <linux/packing.h>
+#include <linux/pcs-lynx.h>
 #include <net/pkt_sched.h>
 #include <linux/iopoll.h>
 #include <linux/mdio.h>
@@ -766,347 +767,6 @@ static int vsc9959_reset(struct ocelot *ocelot)
 	return 0;
 }
 
-/* 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_config_sgmii(struct phy_device *pcs,
-				     unsigned int link_an_mode,
-				     const struct phylink_link_state *state)
-{
-	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_set_bits(pcs, MII_BMCR, BMCR_ANENABLE);
-}
-
-static void vsc9959_pcs_config_usxgmii(struct phy_device *pcs,
-				       unsigned int link_an_mode,
-				       const struct phylink_link_state *state)
-{
-	/* Configure device ability for the USXGMII Replicator */
-	phy_write_mmd(pcs, MDIO_MMD_VEND2, MII_ADVERTISE,
-		      MDIO_USXGMII_2500FULL |
-		      MDIO_USXGMII_LINK |
-		      ADVERTISE_SGMII |
-		      ADVERTISE_LPACK);
-}
-
-void vsc9959_pcs_config(struct ocelot *ocelot, int port,
-			unsigned int link_an_mode,
-			const struct phylink_link_state *state)
-{
-	struct felix *felix = ocelot_to_felix(ocelot);
-	struct phy_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_Half_BIT, pcs->supported);
-	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, pcs->supported);
-	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, pcs->supported);
-	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, pcs->supported);
-	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_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);
-
-	if (!phylink_autoneg_inband(link_an_mode))
-		return;
-
-	switch (pcs->interface) {
-	case PHY_INTERFACE_MODE_SGMII:
-	case PHY_INTERFACE_MODE_QSGMII:
-		vsc9959_pcs_config_sgmii(pcs, link_an_mode, state);
-		break;
-	case PHY_INTERFACE_MODE_2500BASEX:
-		phydev_err(pcs, "AN not supported on 3.125GHz SerDes lane\n");
-		break;
-	case PHY_INTERFACE_MODE_USXGMII:
-		vsc9959_pcs_config_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_up_sgmii(struct phy_device *pcs,
-				      unsigned int link_an_mode,
-				      int speed, int duplex)
-{
-	u16 if_mode = ENETC_PCS_IF_MODE_SGMII_EN;
-
-	switch (speed) {
-	case SPEED_1000:
-		if_mode |= ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_1000);
-		break;
-	case SPEED_100:
-		if_mode |= ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_100);
-		break;
-	case SPEED_10:
-		if_mode |= ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_10);
-		break;
-	default:
-		phydev_err(pcs, "Invalid PCS speed %d\n", speed);
-		return;
-	}
-
-	if (duplex == DUPLEX_HALF)
-		if_mode |= ENETC_PCS_IF_MODE_DUPLEX_HALF;
-
-	phy_write(pcs, ENETC_PCS_IF_MODE, if_mode);
-	phy_clear_bits(pcs, MII_BMCR, BMCR_ANENABLE);
-}
-
-/* 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_link_up_2500basex(struct phy_device *pcs,
-					  unsigned int link_an_mode,
-					  int speed, int duplex)
-{
-	u16 if_mode = ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_2500) |
-		      ENETC_PCS_IF_MODE_SGMII_EN;
-
-	if (duplex == DUPLEX_HALF)
-		if_mode |= ENETC_PCS_IF_MODE_DUPLEX_HALF;
-
-	phy_write(pcs, ENETC_PCS_IF_MODE, if_mode);
-	phy_clear_bits(pcs, MII_BMCR, BMCR_ANENABLE);
-}
-
-void vsc9959_pcs_link_up(struct ocelot *ocelot, int port,
-			 unsigned int link_an_mode,
-			 phy_interface_t interface,
-			 int speed, int duplex)
-{
-	struct felix *felix = ocelot_to_felix(ocelot);
-	struct phy_device *pcs = felix->pcs[port];
-
-	if (!pcs)
-		return;
-
-	if (phylink_autoneg_inband(link_an_mode))
-		return;
-
-	switch (interface) {
-	case PHY_INTERFACE_MODE_SGMII:
-	case PHY_INTERFACE_MODE_QSGMII:
-		vsc9959_pcs_link_up_sgmii(pcs, link_an_mode, speed, duplex);
-		break;
-	case PHY_INTERFACE_MODE_2500BASEX:
-		vsc9959_pcs_link_up_2500basex(pcs, link_an_mode, speed,
-					      duplex);
-		break;
-	case PHY_INTERFACE_MODE_USXGMII:
-		phydev_err(pcs, "USXGMII only supports in-band AN for now\n");
-		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;
-
-	pcs->speed = SPEED_2500;
-	pcs->asym_pause = true;
-	pcs->pause = true;
-}
-
-static void vsc9959_pcs_link_state_usxgmii(struct phy_device *pcs,
-					   struct phylink_link_state *state)
-{
-	int status, lpa;
-
-	status = phy_read_mmd(pcs, MDIO_MMD_VEND2, MII_BMSR);
-	if (status < 0)
-		return;
-
-	pcs->autoneg = true;
-	pcs->autoneg_complete = !!(status & BMSR_ANEGCOMPLETE);
-	pcs->link = !!(status & BMSR_LSTATUS);
-
-	if (!pcs->link || !pcs->autoneg_complete)
-		return;
-
-	lpa = phy_read_mmd(pcs, MDIO_MMD_VEND2, MII_LPA);
-	if (lpa < 0)
-		return;
-
-	switch (lpa & MDIO_USXGMII_SPD_MASK) {
-	case MDIO_USXGMII_10:
-		pcs->speed = SPEED_10;
-		break;
-	case MDIO_USXGMII_100:
-		pcs->speed = SPEED_100;
-		break;
-	case MDIO_USXGMII_1000:
-		pcs->speed = SPEED_1000;
-		break;
-	case MDIO_USXGMII_2500:
-		pcs->speed = SPEED_2500;
-		break;
-	default:
-		break;
-	}
-
-	if (lpa & MDIO_USXGMII_FULL_DUPLEX)
-		pcs->duplex = DUPLEX_FULL;
-	else
-		pcs->duplex = DUPLEX_HALF;
-}
-
-void vsc9959_pcs_link_state(struct ocelot *ocelot, int port,
-			    struct phylink_link_state *state)
-{
-	struct felix *felix = ocelot_to_felix(ocelot);
-	struct phy_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);
-}
-
 static void vsc9959_phylink_validate(struct ocelot *ocelot, int port,
 				     unsigned long *supported,
 				     struct phylink_link_state *state)
@@ -1195,7 +855,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 lynx_pcs *),
 				  GFP_KERNEL);
 	if (!felix->pcs) {
 		dev_err(dev, "failed to allocate array for PCS PHYs\n");
@@ -1246,18 +906,26 @@ 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;
+		struct lynx_pcs *lynx;
+
+		if (dsa_is_unused_port(felix->ds, port))
+			continue;
 
-		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_USXGMII)
-			is_c45 = true;
+		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
+			continue;
 
-		pcs = get_phy_device(felix->imdio, port, is_c45);
+		pcs = mdio_device_create(felix->imdio, port);
 		if (IS_ERR(pcs))
 			continue;
 
-		pcs->interface = ocelot_port->phy_mode;
-		felix->pcs[port] = pcs;
+		lynx = lynx_pcs_create(pcs);
+		if (!lynx) {
+			mdio_device_free(pcs);
+			continue;
+		}
+
+		felix->pcs[port] = lynx;
 
 		dev_info(dev, "Found PCS at internal MDIO address %d\n", port);
 	}
@@ -1271,12 +939,13 @@ 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 lynx_pcs *pcs = felix->pcs[port];
 
 		if (!pcs)
 			continue;
 
-		put_device(&pcs->mdio.dev);
+		mdio_device_free(pcs->mdio);
+		lynx_pcs_destroy(pcs);
 	}
 	mdiobus_unregister(felix->imdio);
 }
@@ -1499,9 +1168,6 @@ static const 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_config		= vsc9959_pcs_config,
-	.pcs_link_up		= vsc9959_pcs_link_up,
-	.pcs_link_state		= vsc9959_pcs_link_state,
 	.phylink_validate	= vsc9959_phylink_validate,
 	.prevalidate_phy_mode	= vsc9959_prevalidate_phy_mode,
 	.port_setup_tc          = vsc9959_port_setup_tc,
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index 625b1891d955..2d6a5f5758f8 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -7,6 +7,7 @@
 #include <soc/mscc/ocelot_sys.h>
 #include <soc/mscc/ocelot.h>
 #include <linux/of_platform.h>
+#include <linux/pcs-lynx.h>
 #include <linux/packing.h>
 #include <linux/iopoll.h>
 #include "felix.h"
@@ -960,18 +961,27 @@ static int vsc9953_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;
 		int addr = port + 4;
+		struct mdio_device *pcs;
+		struct lynx_pcs *lynx;
+
+		if (dsa_is_unused_port(felix->ds, port))
+			continue;
 
 		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
 			continue;
 
-		pcs = get_phy_device(felix->imdio, addr, false);
+		pcs = mdio_device_create(felix->imdio, addr);
 		if (IS_ERR(pcs))
 			continue;
 
-		pcs->interface = ocelot_port->phy_mode;
-		felix->pcs[port] = pcs;
+		lynx = lynx_pcs_create(pcs);
+		if (!lynx) {
+			mdio_device_free(pcs);
+			continue;
+		}
+
+		felix->pcs[port] = lynx;
 
 		dev_info(dev, "Found PCS at internal MDIO address %d\n", addr);
 	}
@@ -1013,9 +1023,6 @@ static const struct felix_info seville_info_vsc9953 = {
 	.num_ports		= 10,
 	.mdio_bus_alloc		= vsc9953_mdio_bus_alloc,
 	.mdio_bus_free		= vsc9959_mdio_bus_free,
-	.pcs_config		= vsc9959_pcs_config,
-	.pcs_link_up		= vsc9959_pcs_link_up,
-	.pcs_link_state		= vsc9959_pcs_link_state,
 	.phylink_validate	= vsc9953_phylink_validate,
 	.prevalidate_phy_mode	= vsc9953_prevalidate_phy_mode,
 	.xmit_template_populate	= vsc9953_xmit_template_populate,
-- 
2.25.1

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

[Vladimir Oltean]

On Fri, Jul 24, 2020 at 11:01:38AM +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, Seville, Felix DSA switch etc) can
> share the same implementation of PCS configuration and runtime
> management.
> 
> The module implements phylink_pcs_ops and exports a phylink_pcs
> (incorporated into a lynx_pcs) which can be directly passed to phylink
> through phylink_pcs_set.
> 
> 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 and
> Seville drivers in order to use the new common PCS implementation.
> 
> At the moment, USXGMII (only with in-band AN), 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)
> 
> Changes in v4:
>  * use the newly introduced phylink PCS mechanism
>  * install the phylink_pcs in the phylink_mac_config DSA ops
>  * remove the direct implementations of the PCS ops
>  * do no use the SGMII_ prefix when referring to the IF_MORE register
>  * add a phylink helper to decode the USXGMII code word
>  * remove cleanup patches for Felix (these have been already accepted)
>  * Seville (recently introduced) now has PCS support through the same
>  Lynx PCS module
> 
> Ioana Ciornei (5):
>   net: phylink: add helper function to decode USXGMII word
>   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: ocelot: use the Lynx PCS helpers in Felix and Seville
> 
>  MAINTAINERS                              |   7 +
>  drivers/net/dsa/ocelot/Kconfig           |   1 +
>  drivers/net/dsa/ocelot/felix.c           |  28 +-
>  drivers/net/dsa/ocelot/felix.h           |  20 +-
>  drivers/net/dsa/ocelot/felix_vsc9959.c   | 374 ++---------------------
>  drivers/net/dsa/ocelot/seville_vsc9953.c |  21 +-
>  drivers/net/phy/Kconfig                  |   6 +
>  drivers/net/phy/Makefile                 |   1 +
>  drivers/net/phy/pcs-lynx.c               | 314 +++++++++++++++++++
>  drivers/net/phy/phylink.c                |  44 +++
>  include/linux/mdio.h                     |   6 +
>  include/linux/pcs-lynx.h                 |  21 ++
>  include/linux/phylink.h                  |   3 +
>  13 files changed, 442 insertions(+), 404 deletions(-)
>  create mode 100644 drivers/net/phy/pcs-lynx.c
>  create mode 100644 include/linux/pcs-lynx.h
> 
> -- 
> 2.25.1
> 

For the entire series:

Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Seville QSGMII with in-band AN
Felix QSGMII with in-band AN
Felix SGMII without in-band AN
Felix USXGMII with in-band AN

Thanks,
-Vladimir

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

[Ioana Ciornei]

> Subject: [PATCH net-next v4 0/5] net: phy: add Lynx PCS MDIO module
> 
> 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, Seville, Felix DSA switch etc) can share the same
> implementation of PCS configuration and runtime management.
> 
> The module implements phylink_pcs_ops and exports a phylink_pcs
> (incorporated into a lynx_pcs) which can be directly passed to phylink through
> phylink_pcs_set.
> 
> 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 and Seville drivers in order to use
> the new common PCS implementation.
> 
> At the moment, USXGMII (only with in-band AN), 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.
> 

Any feedback on the use of phylink_pcs?

Thanks,
Ioana

> 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)
> 
> Changes in v4:
>  * use the newly introduced phylink PCS mechanism
>  * install the phylink_pcs in the phylink_mac_config DSA ops
>  * remove the direct implementations of the PCS ops
>  * do no use the SGMII_ prefix when referring to the IF_MORE register
>  * add a phylink helper to decode the USXGMII code word
>  * remove cleanup patches for Felix (these have been already accepted)
>  * Seville (recently introduced) now has PCS support through the same  Lynx PCS
> module
> 
> Ioana Ciornei (5):
>   net: phylink: add helper function to decode USXGMII word
>   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: ocelot: use the Lynx PCS helpers in Felix and Seville
> 
>  MAINTAINERS                              |   7 +
>  drivers/net/dsa/ocelot/Kconfig           |   1 +
>  drivers/net/dsa/ocelot/felix.c           |  28 +-
>  drivers/net/dsa/ocelot/felix.h           |  20 +-
>  drivers/net/dsa/ocelot/felix_vsc9959.c   | 374 ++---------------------
>  drivers/net/dsa/ocelot/seville_vsc9953.c |  21 +-
>  drivers/net/phy/Kconfig                  |   6 +
>  drivers/net/phy/Makefile                 |   1 +
>  drivers/net/phy/pcs-lynx.c               | 314 +++++++++++++++++++
>  drivers/net/phy/phylink.c                |  44 +++
>  include/linux/mdio.h                     |   6 +
>  include/linux/pcs-lynx.h                 |  21 ++
>  include/linux/phylink.h                  |   3 +
>  13 files changed, 442 insertions(+), 404 deletions(-)  create mode 100644
> drivers/net/phy/pcs-lynx.c  create mode 100644 include/linux/pcs-lynx.h
> 
> --
> 2.25.1

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

[Florian Fainelli]

On 7/24/2020 1:01 AM, 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, Seville, Felix DSA switch etc) can
> share the same implementation of PCS configuration and runtime
> management.
> 
> The module implements phylink_pcs_ops and exports a phylink_pcs
> (incorporated into a lynx_pcs) which can be directly passed to phylink
> through phylink_pcs_set.
> 
> 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 and
> Seville drivers in order to use the new common PCS implementation.
> 
> At the moment, USXGMII (only with in-band AN), 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)
> 
> Changes in v4:
>  * use the newly introduced phylink PCS mechanism
>  * install the phylink_pcs in the phylink_mac_config DSA ops
>  * remove the direct implementations of the PCS ops
>  * do no use the SGMII_ prefix when referring to the IF_MORE register
>  * add a phylink helper to decode the USXGMII code word
>  * remove cleanup patches for Felix (these have been already accepted)
>  * Seville (recently introduced) now has PCS support through the same
>  Lynx PCS module
> 
> Ioana Ciornei (5):
>   net: phylink: add helper function to decode USXGMII word
>   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: ocelot: use the Lynx PCS helpers in Felix and Seville
> 
>  MAINTAINERS                              |   7 +
>  drivers/net/dsa/ocelot/Kconfig           |   1 +
>  drivers/net/dsa/ocelot/felix.c           |  28 +-
>  drivers/net/dsa/ocelot/felix.h           |  20 +-
>  drivers/net/dsa/ocelot/felix_vsc9959.c   | 374 ++---------------------
>  drivers/net/dsa/ocelot/seville_vsc9953.c |  21 +-
>  drivers/net/phy/Kconfig                  |   6 +
>  drivers/net/phy/Makefile                 |   1 +
>  drivers/net/phy/pcs-lynx.c               | 314 +++++++++++++++++++

I believe Andrew had a plan to create a better organization within
drivers/net/phy, while this happens, maybe you can already create
drivers/net/phy/pcs/ regardless of the state of Andrew's work?

>  drivers/net/phy/phylink.c                |  44 +++
>  include/linux/mdio.h                     |   6 +
>  include/linux/pcs-lynx.h                 |  21 ++

And likewise for this header.
-- 
Florian

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

[Ioana Ciornei]

> Subject: Re: [PATCH net-next v4 0/5] net: phy: add Lynx PCS MDIO module
> 
> 
> 
> On 7/24/2020 1:01 AM, 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, Seville, Felix DSA switch etc)
> > can share the same implementation of PCS configuration and runtime
> > management.
> >
> > The module implements phylink_pcs_ops and exports a phylink_pcs
> > (incorporated into a lynx_pcs) which can be directly passed to phylink
> > through phylink_pcs_set.
> >
> > 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 and Seville drivers in order to use the new common PCS implementation.
> >
> > At the moment, USXGMII (only with in-band AN), 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)
> >
> > Changes in v4:
> >  * use the newly introduced phylink PCS mechanism
> >  * install the phylink_pcs in the phylink_mac_config DSA ops
> >  * remove the direct implementations of the PCS ops
> >  * do no use the SGMII_ prefix when referring to the IF_MORE register
> >  * add a phylink helper to decode the USXGMII code word
> >  * remove cleanup patches for Felix (these have been already accepted)
> >  * Seville (recently introduced) now has PCS support through the same
> > Lynx PCS module
> >
> > Ioana Ciornei (5):
> >   net: phylink: add helper function to decode USXGMII word
> >   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: ocelot: use the Lynx PCS helpers in Felix and Seville
> >
> >  MAINTAINERS                              |   7 +
> >  drivers/net/dsa/ocelot/Kconfig           |   1 +
> >  drivers/net/dsa/ocelot/felix.c           |  28 +-
> >  drivers/net/dsa/ocelot/felix.h           |  20 +-
> >  drivers/net/dsa/ocelot/felix_vsc9959.c   | 374 ++---------------------
> >  drivers/net/dsa/ocelot/seville_vsc9953.c |  21 +-
> >  drivers/net/phy/Kconfig                  |   6 +
> >  drivers/net/phy/Makefile                 |   1 +
> >  drivers/net/phy/pcs-lynx.c               | 314 +++++++++++++++++++
> 
> I believe Andrew had a plan to create a better organization within
> drivers/net/phy, while this happens, maybe you can already create
> drivers/net/phy/pcs/ regardless of the state of Andrew's work?
> 

I got the impression from Andrew that the plan was to do this at a later stage
together with the Synopsys XPCS. I could certainly do what you say, I am just
not very keen to add such things into this patch set. 

Ioana

> >  drivers/net/phy/phylink.c                |  44 +++
> >  include/linux/mdio.h                     |   6 +
> >  include/linux/pcs-lynx.h                 |  21 ++
> 
> And likewise for this header.
> --
> Florian

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

[Florian Fainelli]

On 7/27/20 11:48 AM, Ioana Ciornei wrote:
>> Subject: Re: [PATCH net-next v4 0/5] net: phy: add Lynx PCS MDIO module
>>
>>
>>
>> On 7/24/2020 1:01 AM, 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, Seville, Felix DSA switch etc)
>>> can share the same implementation of PCS configuration and runtime
>>> management.
>>>
>>> The module implements phylink_pcs_ops and exports a phylink_pcs
>>> (incorporated into a lynx_pcs) which can be directly passed to phylink
>>> through phylink_pcs_set.
>>>
>>> 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 and Seville drivers in order to use the new common PCS implementation.
>>>
>>> At the moment, USXGMII (only with in-band AN), 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)
>>>
>>> Changes in v4:
>>>  * use the newly introduced phylink PCS mechanism
>>>  * install the phylink_pcs in the phylink_mac_config DSA ops
>>>  * remove the direct implementations of the PCS ops
>>>  * do no use the SGMII_ prefix when referring to the IF_MORE register
>>>  * add a phylink helper to decode the USXGMII code word
>>>  * remove cleanup patches for Felix (these have been already accepted)
>>>  * Seville (recently introduced) now has PCS support through the same
>>> Lynx PCS module
>>>
>>> Ioana Ciornei (5):
>>>   net: phylink: add helper function to decode USXGMII word
>>>   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: ocelot: use the Lynx PCS helpers in Felix and Seville
>>>
>>>  MAINTAINERS                              |   7 +
>>>  drivers/net/dsa/ocelot/Kconfig           |   1 +
>>>  drivers/net/dsa/ocelot/felix.c           |  28 +-
>>>  drivers/net/dsa/ocelot/felix.h           |  20 +-
>>>  drivers/net/dsa/ocelot/felix_vsc9959.c   | 374 ++---------------------
>>>  drivers/net/dsa/ocelot/seville_vsc9953.c |  21 +-
>>>  drivers/net/phy/Kconfig                  |   6 +
>>>  drivers/net/phy/Makefile                 |   1 +
>>>  drivers/net/phy/pcs-lynx.c               | 314 +++++++++++++++++++
>>
>> I believe Andrew had a plan to create a better organization within
>> drivers/net/phy, while this happens, maybe you can already create
>> drivers/net/phy/pcs/ regardless of the state of Andrew's work?
>>
> 
> I got the impression from Andrew that the plan was to do this at a later stage
> together with the Synopsys XPCS. I could certainly do what you say, I am just
> not very keen to add such things into this patch set. 

Andrew, what directory structure do you have in mind such that Ioana can
already start putting the PCS drivers in the right location?
-- 
Florian

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

[Andrew Lunn]

> Andrew, what directory structure do you have in mind such that Ioana can
> already start putting the PCS drivers in the right location?

Hi Florian

I will push out an RFC soon. It will need some build testing, just to
make sure i've not broken any Kconfig dependencies.

     Andrew