Date: Sun, 12 Nov 2023 00:54:41 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Jie Luo <quic_luoj@...cinc.com>
Cc: Maxime Chevallier <maxime.chevallier@...tlin.com>, andrew@...n.ch,
	hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] net: phy: at803x: add QCA8084 ethernet phy support

On Fri, Nov 10, 2023 at 05:56:09PM +0800, Jie Luo wrote:
> > > > > > > What I understand from this is that this PHY can be used either as a
> > > > > > > switch, in which case port 4 would be connected to the host interface
> > > > > > > at up to 2.5G, or as a quad-phy, but since it uses QUSGMII the link
> > > > > > > speed would be limited to 1G per-port, is that correct ?
> > > > > > 
> > > > > > When the PHY works on the interface mode QUSGMII for quad-phy, all 4
> > > > > > PHYs can support to the max link speed 2.5G, actually the PHY can
> > > > > > support to max link speed 2.5G for all supported interface modes
> > > > > > including qusgmii and sgmii.
> > > > > 
> > > > > I'm a bit confused then, as the USGMII spec says that Quad USGMII really
> > > > > is for quad 10/100/1000 speeds, using 10b/8b encoding.
> > > > > 
> > > > > Aren't you using the USXGMII mode instead, which can convey 4 x 2.5Gbps
> > > > >    with 66b/64b encoding ?
> > > > 
> > > > Hi Maxime,
> > > > Yes, for quad PHY mode, it is using 66b/64 encoding.
> > > > 
> > > > it seems that PHY_INTERFACE_MODE_USXGMII is for single port,
> > > > so i take the interface name PHY_INTERFACE_MODE_QUSGMII for
> > > > quad PHYs here.
> > > > 
> > > > can we apply PHY_INTERFACE_MODE_USXGMII to quad PHYs in this
> > > > case(qca8084 quad PHY mode)?
> > > > 
> > > > Thanks,
> > > > Jie.
> > > 
> > > one more thing, if we use the PHY_INTERFACE_MODE_USXGMII for
> > > the quad PHY here, the MAC serdes can't distinguish the actual
> > > mode PHY_INTERFACE_MODE_USXGMII and 10G-QXGMII(qca8084 quad phy mode),
> > > the MAC serdes has the different configurations for usxgmii(10g single
> > > port) and qxsgmii(quad PHY).
> > 
> > Yes you do need a way to know which mode to use, what I'm wondering is
> > that the usxgmii spec actually defines something like 9 different modes
> > ( 1/2/4/8 ports, with a total bandwidth ranging from 2.5Gbps to 20 Gbps
> > ), should we define a phy mode for all of these variants, or should we
> > have another way of getting the mode variant (like, saying I want to
> > use usxgmii, in 4 ports mode, with the serdes at 10.3125Gbps).
> > 
> > That being said, QUSGMII already exists to define a specific variant of
> > USGMII, so maybe adding 10G-QXGMII is fine...
> 
> Yes, Maxime, I agree with this solution, the name 10G-QXGMII is exactly
> the working mode of qca8084 quad phy mode.

FWIW, NXP has these 2 patches in its trees. One day I was going to make
an attempt at upstreaming them, but the Aquantia PHY driver is a bit of
a sticky topic since I could find no way to distinguish between the
single-port and multi-port variants of USXGMII in this PHY IP, and the
AQR412 uses a multi-port mode which is now treated as USXGMII by Linux.

Anyway, if you do make progress with this ahead of me, please be aware
that these patches exist, and I would appreciate if you tried to keep
the name as "10g-qxgmii" so that I don't have to modify the (downstream)
device trees again :)

>From 8f4c8227362bb7acad09cd756390f1efd3311395 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@....com>
Date: Fri, 6 Oct 2023 23:42:35 +0300
Subject: [PATCH] net: dsa: felix: introduce phy-mode = "10g-qxgmii" to replace
 "usxgmii"

The "USXGMII" mode that the Felix switch ports support on LS1028A is not
quite USXGMII, it is defined by the USXGMII multiport specification
document as 10G-QXGMII. It uses the same signaling as USXGMII, but it
multiplexes 4 ports over the link, resulting in a maximum speed of 2.5G
per port.

This change is needed in preparation for the lynx-10g driver on LS1028A,
which will make a more clear distinction between usxgmii (supported on
lane 0) and 10g-qxgmii (supported on lane 1). These protocols have their
configuration in different PCCR registers (PCCRB vs PCCR9).

We touch the entire kernel side: the phylib core, phylink, the Felix
switch driver, the Lynx PCS and the AQR412 driver. The existing
LS1028A-QDS device trees will continue to work with "usxgmii", and will
be updated to "10g-qxgmii" as a separate patch.

Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 .../bindings/net/ethernet-controller.yaml         |  1 +
 Documentation/networking/phy.rst                  |  6 ++++++
 drivers/net/dsa/ocelot/felix.c                    |  1 +
 drivers/net/dsa/ocelot/felix.h                    |  3 ++-
 drivers/net/dsa/ocelot/felix_vsc9959.c            |  3 ++-
 drivers/net/pcs/pcs-lynx.c                        |  8 ++++++--
 drivers/net/phy/aquantia_main.c                   | 15 +++++++++++++++
 drivers/net/phy/phy-core.c                        |  1 +
 drivers/net/phy/phylink.c                         | 12 ++++++++++--
 include/linux/phy.h                               |  4 ++++
 include/linux/phylink.h                           |  1 +
 11 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 9f6a5ccbcefe..044880d804db 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -104,6 +104,7 @@ properties:
       - usxgmii
       - 10gbase-r
       - 25gbase-r
+      - 10g-qxgmii
 
   phy-mode:
     $ref: "#/properties/phy-connection-type"
diff --git a/Documentation/networking/phy.rst b/Documentation/networking/phy.rst
index 1283240d7620..f64641417c54 100644
--- a/Documentation/networking/phy.rst
+++ b/Documentation/networking/phy.rst
@@ -327,6 +327,12 @@ Some of the interface modes are described below:
     This is the Penta SGMII mode, it is similar to QSGMII but it combines 5
     SGMII lines into a single link compared to 4 on QSGMII.
 
+``PHY_INTERFACE_MODE_10G_QXGMII``
+    Represents the 10G-QXGMII PHY-MAC interface as defined by the Cisco USXGMII
+    Multiport Copper Interface document. It supports 4 ports over a 10.3125 GHz
+    SerDes lane, each port having speeds of 2.5G / 1G / 100M / 10M achieved
+    through symbol replication. The PCS expects the standard USXGMII code word.
+
 Pause frames / flow control
 ===========================
 
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 9a3e5ec16972..17fa31b58be4 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1232,6 +1232,7 @@ static const u32 felix_phy_match_table[PHY_INTERFACE_MODE_MAX] = {
 	[PHY_INTERFACE_MODE_SGMII] = OCELOT_PORT_MODE_SGMII,
 	[PHY_INTERFACE_MODE_QSGMII] = OCELOT_PORT_MODE_QSGMII,
 	[PHY_INTERFACE_MODE_USXGMII] = OCELOT_PORT_MODE_USXGMII,
+	[PHY_INTERFACE_MODE_10G_QXGMII] = OCELOT_PORT_MODE_10G_QXGMII,
 	[PHY_INTERFACE_MODE_1000BASEX] = OCELOT_PORT_MODE_1000BASEX,
 	[PHY_INTERFACE_MODE_2500BASEX] = OCELOT_PORT_MODE_2500BASEX,
 };
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index 1d4befe7cfe8..4cf849e5782f 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -12,8 +12,9 @@
 #define OCELOT_PORT_MODE_SGMII		BIT(1)
 #define OCELOT_PORT_MODE_QSGMII		BIT(2)
 #define OCELOT_PORT_MODE_2500BASEX	BIT(3)
-#define OCELOT_PORT_MODE_USXGMII	BIT(4)
+#define OCELOT_PORT_MODE_USXGMII	BIT(4) /* compatibility */
 #define OCELOT_PORT_MODE_1000BASEX	BIT(5)
+#define OCELOT_PORT_MODE_10G_QXGMII	BIT(6)
 
 struct device_node;
 
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 3c5509e75a54..8ae9c2e340ab 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -34,7 +34,8 @@
 					 OCELOT_PORT_MODE_QSGMII | \
 					 OCELOT_PORT_MODE_1000BASEX | \
 					 OCELOT_PORT_MODE_2500BASEX | \
-					 OCELOT_PORT_MODE_USXGMII)
+					 OCELOT_PORT_MODE_USXGMII | \
+					 OCELOT_PORT_MODE_10G_QXGMII)
 
 static const u32 vsc9959_port_modes[VSC9959_NUM_PORTS] = {
 	VSC9959_PORT_MODE_SERDES,
diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index 7e1e8f61f043..f3ab17ef4e31 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c
@@ -95,6 +95,7 @@ static void lynx_pcs_get_state(struct phylink_pcs *pcs,
 		lynx_pcs_get_state_2500basex(lynx->mdio, state);
 		break;
 	case PHY_INTERFACE_MODE_USXGMII:
+	case PHY_INTERFACE_MODE_10G_QXGMII:
 		lynx_pcs_get_state_usxgmii(lynx->mdio, state);
 		break;
 	case PHY_INTERFACE_MODE_10GBASER:
@@ -151,6 +152,7 @@ static int lynx_pcs_config_giga(struct mdio_device *pcs,
 }
 
 static int lynx_pcs_config_usxgmii(struct mdio_device *pcs,
+				   phy_interface_t interface,
 				   const unsigned long *advertising,
 				   unsigned int neg_mode)
 {
@@ -158,7 +160,8 @@ static int lynx_pcs_config_usxgmii(struct mdio_device *pcs,
 	int addr = pcs->addr;
 
 	if (neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
-		dev_err(&pcs->dev, "USXGMII only supports in-band AN for now\n");
+		dev_err(&pcs->dev, "%s only supports in-band AN for now\n",
+			phy_modes(interface));
 		return -EOPNOTSUPP;
 	}
 
@@ -189,7 +192,8 @@ static int lynx_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 		}
 		break;
 	case PHY_INTERFACE_MODE_USXGMII:
-		return lynx_pcs_config_usxgmii(lynx->mdio, advertising,
+	case PHY_INTERFACE_MODE_10G_QXGMII:
+		return lynx_pcs_config_usxgmii(lynx->mdio, ifmode, advertising,
 					       neg_mode);
 	case PHY_INTERFACE_MODE_10GBASER:
 	case PHY_INTERFACE_MODE_25GBASER:
diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 5fc93bbfbe49..4f5e6575be7b 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -512,6 +512,20 @@ static int aqr107_read_status(struct phy_device *phydev)
 	if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
 		return 0;
 
+	/* Quad port PHYs like AQR412 have 4 system interfaces, but they can
+	 * also be used with a single system interface over which all 4 ports
+	 * are multiplexed (10G-QXGMII). To the MDIO registers, this mode is
+	 * indistinguishable from USXGMII (which implies a single 10G port),
+	 * which is problematic because the detection logic below would
+	 * overwrite phydev->interface with a wrong value. If the device tree
+	 * is configured for "10g-qxgmii", always trust that value, since it is
+	 * very unlikely that the PHY firmware was configured for protocol
+	 * switching depending on link speed (all USXGMII variants are capable
+	 * of symbol replication).
+	 */
+	if (phydev->interface == PHY_INTERFACE_MODE_10G_QXGMII)
+		goto skip_iface_detection;
+
 	val = phy_read_mmd(phydev, MDIO_MMD_PHYXS, MDIO_PHYXS_VEND_IF_STATUS);
 	if (val < 0)
 		return val;
@@ -546,6 +560,7 @@ static int aqr107_read_status(struct phy_device *phydev)
 		break;
 	}
 
+skip_iface_detection:
 	/* Read possibly downshifted rate from vendor register */
 	return aqr107_read_rate(phydev);
 }
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index cfa6f08d8ca3..95222680a82d 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -142,6 +142,7 @@ int phy_interface_num_ports(phy_interface_t interface)
 		return 1;
 	case PHY_INTERFACE_MODE_QSGMII:
 	case PHY_INTERFACE_MODE_QUSGMII:
+	case PHY_INTERFACE_MODE_10G_QXGMII:
 		return 4;
 	case PHY_INTERFACE_MODE_PSGMII:
 		return 5;
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 3bf923f88494..dd0ac7139aa4 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -191,6 +191,7 @@ static unsigned int phylink_pcs_neg_mode(unsigned int mode, phy_interface_t inte
 	case PHY_INTERFACE_MODE_QSGMII:
 	case PHY_INTERFACE_MODE_QUSGMII:
 	case PHY_INTERFACE_MODE_USXGMII:
+	case PHY_INTERFACE_MODE_10G_QXGMII:
 		/* These protocols are designed for use with a PHY which
 		 * communicates its negotiation result back to the MAC via
 		 * inband communication. Note: there exist PHYs that run
@@ -284,6 +285,7 @@ static int phylink_interface_max_speed(phy_interface_t interface)
 
 	case PHY_INTERFACE_MODE_2500BASEX:
 	case PHY_INTERFACE_MODE_2500SGMII:
+	case PHY_INTERFACE_MODE_10G_QXGMII:
 		return SPEED_2500;
 
 	case PHY_INTERFACE_MODE_5GBASER:
@@ -553,7 +555,11 @@ unsigned long phylink_get_capabilities(phy_interface_t interface,
 
 	switch (interface) {
 	case PHY_INTERFACE_MODE_USXGMII:
-		caps |= MAC_10000FD | MAC_5000FD | MAC_2500FD;
+		caps |= MAC_10000FD | MAC_5000FD;
+		fallthrough;
+
+	case PHY_INTERFACE_MODE_10G_QXGMII:
+		caps |= MAC_2500FD;
 		fallthrough;
 
 	case PHY_INTERFACE_MODE_RGMII_TXID:
@@ -969,6 +975,7 @@ static int phylink_parse_mode(struct phylink *pl,
 		case PHY_INTERFACE_MODE_5GBASER:
 		case PHY_INTERFACE_MODE_25GBASER:
 		case PHY_INTERFACE_MODE_USXGMII:
+		case PHY_INTERFACE_MODE_10G_QXGMII:
 		case PHY_INTERFACE_MODE_10GKR:
 		case PHY_INTERFACE_MODE_10GBASER:
 		case PHY_INTERFACE_MODE_XLGMII:
@@ -1804,7 +1811,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	if (phy->is_c45 && config.rate_matching == RATE_MATCH_NONE &&
 	    interface != PHY_INTERFACE_MODE_RXAUI &&
 	    interface != PHY_INTERFACE_MODE_XAUI &&
-	    interface != PHY_INTERFACE_MODE_USXGMII)
+	    interface != PHY_INTERFACE_MODE_USXGMII &&
+	    interface != PHY_INTERFACE_MODE_10G_QXGMII)
 		config.interface = PHY_INTERFACE_MODE_NA;
 	else
 		config.interface = interface;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 184b04422877..0eeeea50d9a2 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -125,6 +125,7 @@ extern const int phy_10gbit_features_array[1];
  * @PHY_INTERFACE_MODE_10GKR: 10GBASE-KR - with Clause 73 AN
  * @PHY_INTERFACE_MODE_QUSGMII: Quad Universal SGMII
  * @PHY_INTERFACE_MODE_1000BASEKX: 1000Base-KX - with Clause 73 AN
+ * @PHY_INTERFACE_MODE_10G_QXGMII: 10G-QXGMII - 4 ports over 10G USXGMII
  * @PHY_INTERFACE_MODE_MAX: Book keeping
  *
  * Describes the interface between the MAC and PHY.
@@ -166,6 +167,7 @@ typedef enum {
 	PHY_INTERFACE_MODE_QUSGMII,
 	PHY_INTERFACE_MODE_1000BASEKX,
 	PHY_INTERFACE_MODE_2500SGMII,
+	PHY_INTERFACE_MODE_10G_QXGMII,
 	PHY_INTERFACE_MODE_MAX,
 } phy_interface_t;
 
@@ -289,6 +291,8 @@ static inline const char *phy_modes(phy_interface_t interface)
 		return "qusgmii";
 	case PHY_INTERFACE_MODE_2500SGMII:
 		return "sgmii-2500";
+	case PHY_INTERFACE_MODE_10G_QXGMII:
+		return "10g-qxgmii";
 	default:
 		return "unknown";
 	}
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 871db640ceb6..336780e76e34 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -648,6 +648,7 @@ static inline int phylink_get_link_timer_ns(phy_interface_t interface)
 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_QSGMII:
 	case PHY_INTERFACE_MODE_USXGMII:
+	case PHY_INTERFACE_MODE_10G_QXGMII:
 		return 1600000;
 
 	case PHY_INTERFACE_MODE_1000BASEX:
-- 
2.34.1


>From a5dc53f7155bf7e7ced2fb520ccfa1b7d8875fbd Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@....com>
Date: Wed, 25 Oct 2023 14:34:49 +0300
Subject: [PATCH] arm64: dts: ls1028a-qds: replace "usxgmii" with "10g-qxgmii"
 for 13bb

The Felix switch actually supports the quad-port 10g-qxgmii variant of
usxgmii, so use that as the phy-mode in this device tree overlay.

Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 arch/arm64/boot/dts/freescale/fsl-ls1028a-qds-13bb.dtso | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-qds-13bb.dtso b/arch/arm64/boot/dts/freescale/fsl-ls1028a-qds-13bb.dtso
index f826392c23fa..509cf95026df 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-qds-13bb.dtso
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-qds-13bb.dtso
@@ -60,28 +60,28 @@
 	port@0 {
 		status = "okay";
 		phy-handle = <&slot2_qxgmii0>;
-		phy-mode = "usxgmii";
+		phy-mode = "10g-qxgmii";
 		managed = "in-band-status";
 	};
 
 	port@1 {
 		status = "okay";
 		phy-handle = <&slot2_qxgmii1>;
-		phy-mode = "usxgmii";
+		phy-mode = "10g-qxgmii";
 		managed = "in-band-status";
 	};
 
 	port@2 {
 		status = "okay";
 		phy-handle = <&slot2_qxgmii2>;
-		phy-mode = "usxgmii";
+		phy-mode = "10g-qxgmii";
 		managed = "in-band-status";
 	};
 
 	port@3 {
 		status = "okay";
 		phy-handle = <&slot2_qxgmii3>;
-		phy-mode = "usxgmii";
+		phy-mode = "10g-qxgmii";
 		managed = "in-band-status";
 	};
 };
-- 
2.34.1

Powered by blists - more mailing lists