[PATCH v1 0/5] net: phy: marvell: Sync Marvell ethernet PHY code with Marvell version

[PATCH v1 0/5] net: phy: marvell: Sync Marvell ethernet PHY code with Marvell version

[Stefan Roese]

This patchset adds the missing ethernet PHY patches from the Marvell
U-Boot SDK version to support and fix higher connection speeds. This is
done in preparation for the integration of the Octeon TX2 CN913x
support, which uses the updated version of this code.

Thanks,
Stefan


Igal Liberman (1):
  net: phy: marvell: add support for 88E2110 phy

Marcin Wojtas (1):
  net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

Stefan Chulski (3):
  net: phy: marvell: remove hardcoded PHY 2210 lane swap
  net: phy: marvell: Fix PHY 221 2.5G speed detection
  net: phy: marvell: Fix 2210 link and speed detection resolution

 drivers/net/phy/marvell.c | 200 ++++++++++++++++++++++++++++++++++++++
 include/linux/ethtool.h   |   3 +-
 2 files changed, 202 insertions(+), 1 deletion(-)

-- 
2.31.0

[PATCH v1 1/5] net: phy: marvell: add support for 88E2110 phy

[Stefan Roese]

From: Igal Liberman <igall@marvell.com>

E2110 support 10M/100M/1G/2.5G/5G speed and use C45 register definition.
Need to use C45 or C22 r13/r14 indirect method to access

Signed-off-by: Kevin Shi <kshi@marvell.com>
Signed-off-by: Igal Liberman <igall@marvell.com>
Signed-off-by: Stefan Roese <sr@denx.de>
---

 drivers/net/phy/marvell.c | 205 ++++++++++++++++++++++++++++++++++++++
 include/linux/ethtool.h   |   3 +-
 2 files changed, 207 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index a62c695c5c84..c3f86d98f9e3 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -6,6 +6,7 @@
  * author Andy Fleming
  */
 #include <common.h>
+#include <console.h>
 #include <errno.h>
 #include <phy.h>
 #include <linux/bitops.h>
@@ -104,6 +105,19 @@
 #define MIIM_88E151x_MODE_SGMII		1
 #define MIIM_88E151x_RESET_OFFS		15
 
+/* 88E2110 PHY defines */
+#define MIIM_88E2110_PHY_STATUS		0x8008
+#define MIIM_88E2110_PHYSTAT_SPEED	0xc000
+#define MIIM_88E2110_PHYSTAT_10GBIT	0xc000
+#define MIIM_88E2110_PHYSTAT_GBIT	0x8000
+#define MIIM_88E2110_PHYSTAT_100	0x4000
+#define MIIM_88E2110_PHYSTAT_DUPLEX	0x2000
+#define MIIM_88E2110_PHYSTAT_SPDDONE	0x0800
+#define MIIM_88E2110_PHYSTAT_LINK	0x0400
+#define MIIM_88E2110_PHYSTAT_SPEED_5G	0x000c
+#define MIIM_88E2110_PHYSTAT_5GBIT	0x0008
+#define MIIM_88E2110_PHYSTAT_2_5GBIT	0x0004
+
 static int m88e1xxx_phy_extread(struct phy_device *phydev, int addr,
 				int devaddr, int regnum)
 {
@@ -590,6 +604,185 @@ static int m88e1680_config(struct phy_device *phydev)
 	return 0;
 }
 
+/* Marvell 88E2110 */
+static int m88e2110_probe(struct phy_device *phydev)
+{
+	/*
+	 * skip reset since phy has its own initial value.
+	 * resettting leads to weird behavior
+	 */
+	phydev->flags |= PHY_FLAG_BROKEN_RESET;
+
+	return 0;
+}
+
+static int m88e2110_config(struct phy_device *phydev)
+{
+	u16 reg;
+
+	/* Perform lane swap */
+	reg = phy_read(phydev, 1, 0xc000);
+	reg |= 0x1;
+	phy_write(phydev, 1, 0xc000, reg);
+
+	/* Configure auto-negotiation advertisement */
+	if (phydev->interface == PHY_INTERFACE_MODE_SFI) {
+		/* Disabled 10G advertisement */
+		phy_write(phydev, 7, 0x20, 0x1e1);
+	} else {
+		if (phydev->interface == PHY_INTERFACE_MODE_SGMII_2500) {
+			/* Disabled 10G/5G advertisements */
+			phy_write(phydev, 7, 0x20, 0xa1);
+		} else {
+			/* Disable 10G/5G/2.5G auto-negotiation advertisement */
+			phy_write(phydev, 7, 0x20, 0x1);
+		}
+	}
+
+	/* Restart auto-negotiation */
+	phy_write(phydev, 7, 0, 0x3200);
+
+	return 0;
+}
+
+/* Parse the 88E2110's status register for speed and duplex
+ * information
+ */
+static uint m88e2110_parse_status(struct phy_device *phydev)
+{
+	unsigned int speed;
+	unsigned int mii_reg;
+
+	mii_reg = phy_read(phydev, 3, MIIM_88E2110_PHY_STATUS);
+
+	if ((mii_reg & MIIM_88E2110_PHYSTAT_LINK) &&
+	    !(mii_reg & MIIM_88E2110_PHYSTAT_SPDDONE)) {
+		int i = 0;
+
+		puts("Waiting for PHY realtime link");
+		while (!(mii_reg & MIIM_88E2110_PHYSTAT_SPDDONE)) {
+			/* Timeout reached ? */
+			if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
+				puts(" TIMEOUT !\n");
+				phydev->link = 0;
+				break;
+			}
+
+			if ((i++ % 1000) == 0)
+				putc('.');
+			udelay(1000);
+			mii_reg = phy_read(phydev, 3, MIIM_88E2110_PHY_STATUS);
+		}
+		puts(" done\n");
+		mdelay(500);	/* another 500 ms (results in faster booting) */
+	} else {
+		if (mii_reg & MIIM_88E2110_PHYSTAT_LINK)
+			phydev->link = 1;
+		else
+			phydev->link = 0;
+	}
+
+	if (mii_reg & MIIM_88E2110_PHYSTAT_DUPLEX)
+		phydev->duplex = DUPLEX_FULL;
+	else
+		phydev->duplex = DUPLEX_HALF;
+
+	speed = mii_reg & MIIM_88E2110_PHYSTAT_SPEED;
+
+	switch (speed) {
+	case MIIM_88E2110_PHYSTAT_10GBIT:
+		switch (mii_reg & MIIM_88E2110_PHYSTAT_SPEED_5G) {
+		case MIIM_88E2110_PHYSTAT_5GBIT:
+			phydev->speed = SPEED_5000;
+			break;
+		case MIIM_88E2110_PHYSTAT_2_5GBIT:
+			phydev->speed = SPEED_2500;
+			break;
+		default:
+			puts(" Unknown speed detected\n");
+			break;
+		}
+	case MIIM_88E2110_PHYSTAT_GBIT:
+		phydev->speed = SPEED_1000;
+		break;
+	case MIIM_88E2110_PHYSTAT_100:
+		phydev->speed = SPEED_100;
+		break;
+	default:
+		phydev->speed = SPEED_10;
+		break;
+	}
+
+	return 0;
+}
+
+static int m88e2110_update_link(struct phy_device *phydev)
+{
+	unsigned int mii_reg;
+
+	/*
+	 * Wait if the link is up, and autonegotiation is in progress
+	 * (ie - we're capable and it's not done)
+	 */
+	mii_reg = phy_read(phydev, 7, MII_BMSR);
+
+	/*
+	 * If we already saw the link up, and it hasn't gone down, then
+	 * we don't need to wait for autoneg again
+	 */
+	if (phydev->link && mii_reg & BMSR_LSTATUS)
+		return 0;
+
+	if ((mii_reg & BMSR_ANEGCAPABLE) && !(mii_reg & BMSR_ANEGCOMPLETE)) {
+		int i = 0;
+
+		debug("%s Waiting for PHY auto negotiation to complete",
+		      phydev->drv->name);
+		while (!(mii_reg & BMSR_ANEGCOMPLETE)) {
+			/*
+			 * Timeout reached ?
+			 */
+			if (i > PHY_ANEG_TIMEOUT) {
+				debug(" TIMEOUT !\n");
+				phydev->link = 0;
+				return 0;
+			}
+
+			if (ctrlc()) {
+				puts("user interrupt!\n");
+				phydev->link = 0;
+				return -EINTR;
+			}
+
+			if ((i++ % 500) == 0)
+				debug(".");
+
+			udelay(1000);	/* 1 ms */
+			mii_reg = phy_read(phydev, 7, MII_BMSR);
+		}
+		debug(" done\n");
+		phydev->link = 1;
+	} else {
+		/* Read the link a second time to clear the latched state */
+		mii_reg = phy_read(phydev, 7, MII_BMSR);
+
+		if (mii_reg & BMSR_LSTATUS)
+			phydev->link = 1;
+		else
+			phydev->link = 0;
+	}
+
+	return 0;
+}
+
+static int m88e2110_startup(struct phy_device *phydev)
+{
+	m88e2110_update_link(phydev);
+	m88e2110_parse_status(phydev);
+
+	return 0;
+}
+
 static struct phy_driver M88E1011S_driver = {
 	.name = "Marvell 88E1011S",
 	.uid = 0x1410c60,
@@ -692,6 +885,17 @@ static struct phy_driver M88E1680_driver = {
 	.shutdown = &genphy_shutdown,
 };
 
+static struct phy_driver M88E2110_driver = {
+	.name = "Marvell 88E2110",
+	.uid = 0x2b09b8,
+	.mask = 0xffffff0,
+	.features = PHY_GBIT_FEATURES,
+	.probe = &m88e2110_probe,
+	.config = &m88e2110_config,
+	.startup = &m88e2110_startup,
+	.shutdown = &genphy_shutdown,
+};
+
 int phy_marvell_init(void)
 {
 	phy_register(&M88E1310_driver);
@@ -704,6 +908,7 @@ int phy_marvell_init(void)
 	phy_register(&M88E1011S_driver);
 	phy_register(&M88E151x_driver);
 	phy_register(&M88E1680_driver);
+	phy_register(&M88E2110_driver);
 
 	return 0;
 }
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index f6dbdb096d34..a97bde48df99 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -614,11 +614,12 @@ enum ethtool_sfeatures_retval_bits {
  * it was foced up into this mode or autonegotiated.
  */
 
-/* The forced speed, 10Mb, 100Mb, gigabit, 2.5Gb, 10GbE. */
+/* The forced speed, 10Mb, 100Mb, gigabit, 2.5Gb,  5Gb, 10GbE. */
 #define SPEED_10		10
 #define SPEED_100		100
 #define SPEED_1000		1000
 #define SPEED_2500		2500
+#define SPEED_5000		5000
 #define SPEED_10000		10000
 
 /* Duplex, half or full. */
-- 
2.31.0

[PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Stefan Roese]

From: Marcin Wojtas <mw@semihalf.com>

Allow 88E2110 to configure advertisements for both
SGMII @2.5Ghz and 2500BaseX modes.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Tested-by: sa_ip-sw-jenkins <sa_ip-sw-jenkins@marvell.com>
Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
Reviewed-by: Stefan Chulski <stefanc@marvell.com>
Reviewed-by: Nadav Haklai <nadavh@marvell.com>
Signed-off-by: Stefan Roese <sr@denx.de>
---

 drivers/net/phy/marvell.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index c3f86d98f9e3..2e08bf3e2f79 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -630,7 +630,8 @@ static int m88e2110_config(struct phy_device *phydev)
 		/* Disabled 10G advertisement */
 		phy_write(phydev, 7, 0x20, 0x1e1);
 	} else {
-		if (phydev->interface == PHY_INTERFACE_MODE_SGMII_2500) {
+		if (phydev->interface == PHY_INTERFACE_MODE_SGMII_2500 ||
+		    phydev->interface == PHY_INTERFACE_MODE_2500BASEX) {
 			/* Disabled 10G/5G advertisements */
 			phy_write(phydev, 7, 0x20, 0xa1);
 		} else {
-- 
2.31.0

[PATCH v1 3/5] net: phy: marvell: remove hardcoded PHY 2210 lane swap

[Stefan Roese]

From: Stefan Chulski <stefanc@marvell.com>

Lane swapped only if "enet-phy-lane-swap" set in device tree.

Signed-off-by: Stefan Chulski <stefanc@marvell.com>
Tested-by: sa_ip-sw-jenkins <sa_ip-sw-jenkins@marvell.com>
Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
Reviewed-by: Yan Markman <ymarkman@marvell.com>
Reviewed-by: Marcin Wojtas <marcin@marvell.com>
Signed-off-by: Stefan Roese <sr@denx.de>
---

 drivers/net/phy/marvell.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 2e08bf3e2f79..8311b3b4722f 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -619,11 +619,17 @@ static int m88e2110_probe(struct phy_device *phydev)
 static int m88e2110_config(struct phy_device *phydev)
 {
 	u16 reg;
+	ofnode node = phy_get_ofnode(phydev);
 
-	/* Perform lane swap */
-	reg = phy_read(phydev, 1, 0xc000);
-	reg |= 0x1;
-	phy_write(phydev, 1, 0xc000, reg);
+	if (!ofnode_valid(node))
+		return -EINVAL;
+
+	if (ofnode_read_bool(node, "enet-phy-lane-swap")) {
+		/* Perform lane swap */
+		reg = phy_read(phydev, 1, 0xc000);
+		reg |= 0x1;
+		phy_write(phydev, 1, 0xc000, reg);
+	}
 
 	/* Configure auto-negotiation advertisement */
 	if (phydev->interface == PHY_INTERFACE_MODE_SFI) {
-- 
2.31.0

[PATCH v1 4/5] net: phy: marvell: Fix PHY 221 2.5G speed detection

[Stefan Roese]

From: Stefan Chulski <stefanc@marvell.com>

Add missed break in speed detection procedure and remove
nested switch.

Signed-off-by: Stefan Chulski <stefanc@marvell.com>
Tested-by: sa_ip-sw-jenkins <sa_ip-sw-jenkins@marvell.com>
Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
Reviewed-by: Yan Markman <ymarkman@marvell.com>
Signed-off-by: Stefan Roese <sr@denx.de>
---

 drivers/net/phy/marvell.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 8311b3b4722f..36e678755ae8 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -107,16 +107,14 @@
 
 /* 88E2110 PHY defines */
 #define MIIM_88E2110_PHY_STATUS		0x8008
-#define MIIM_88E2110_PHYSTAT_SPEED	0xc000
-#define MIIM_88E2110_PHYSTAT_10GBIT	0xc000
+#define MIIM_88E2110_PHYSTAT_SPEED	0xc00c
 #define MIIM_88E2110_PHYSTAT_GBIT	0x8000
 #define MIIM_88E2110_PHYSTAT_100	0x4000
 #define MIIM_88E2110_PHYSTAT_DUPLEX	0x2000
 #define MIIM_88E2110_PHYSTAT_SPDDONE	0x0800
 #define MIIM_88E2110_PHYSTAT_LINK	0x0400
-#define MIIM_88E2110_PHYSTAT_SPEED_5G	0x000c
-#define MIIM_88E2110_PHYSTAT_5GBIT	0x0008
-#define MIIM_88E2110_PHYSTAT_2_5GBIT	0x0004
+#define MIIM_88E2110_PHYSTAT_5GBIT	0xc008
+#define MIIM_88E2110_PHYSTAT_2_5GBIT	0xc004
 
 static int m88e1xxx_phy_extread(struct phy_device *phydev, int addr,
 				int devaddr, int regnum)
@@ -657,7 +655,6 @@ static int m88e2110_config(struct phy_device *phydev)
  */
 static uint m88e2110_parse_status(struct phy_device *phydev)
 {
-	unsigned int speed;
 	unsigned int mii_reg;
 
 	mii_reg = phy_read(phydev, 3, MIIM_88E2110_PHY_STATUS);
@@ -694,21 +691,13 @@ static uint m88e2110_parse_status(struct phy_device *phydev)
 	else
 		phydev->duplex = DUPLEX_HALF;
 
-	speed = mii_reg & MIIM_88E2110_PHYSTAT_SPEED;
-
-	switch (speed) {
-	case MIIM_88E2110_PHYSTAT_10GBIT:
-		switch (mii_reg & MIIM_88E2110_PHYSTAT_SPEED_5G) {
-		case MIIM_88E2110_PHYSTAT_5GBIT:
-			phydev->speed = SPEED_5000;
-			break;
-		case MIIM_88E2110_PHYSTAT_2_5GBIT:
-			phydev->speed = SPEED_2500;
-			break;
-		default:
-			puts(" Unknown speed detected\n");
-			break;
-		}
+	switch (mii_reg & MIIM_88E2110_PHYSTAT_SPEED) {
+	case MIIM_88E2110_PHYSTAT_5GBIT:
+		phydev->speed = SPEED_5000;
+		break;
+	case MIIM_88E2110_PHYSTAT_2_5GBIT:
+		phydev->speed = SPEED_2500;
+		break;
 	case MIIM_88E2110_PHYSTAT_GBIT:
 		phydev->speed = SPEED_1000;
 		break;
-- 
2.31.0

[PATCH v1 5/5] net: phy: marvell: Fix 2210 link and speed detection resolution

[Stefan Roese]

From: Stefan Chulski <stefanc@marvell.com>

Patch fix couple of link and speed detection resolution bugs:
1. Waiting for PHY real time link condition.
2. Replace PHYSTAT_SPDDONE with PHYSTAT_LINK in while loop,
   PHYSTAT_SPDDONE could be enabled before link status changed to
   enable.
3. Update link status after link resolution loop.

Signed-off-by: Stefan Chulski <stefanc@marvell.com>
Tested-by: sa_ip-sw-jenkins <sa_ip-sw-jenkins@marvell.com>
Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
Reviewed-by: Yan Markman <ymarkman@marvell.com>
Reviewed-by: Marcin Wojtas <marcin@marvell.com>
Signed-off-by: Stefan Roese <sr@denx.de>
---

 drivers/net/phy/marvell.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 36e678755ae8..3bcb0033b391 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -659,12 +659,11 @@ static uint m88e2110_parse_status(struct phy_device *phydev)
 
 	mii_reg = phy_read(phydev, 3, MIIM_88E2110_PHY_STATUS);
 
-	if ((mii_reg & MIIM_88E2110_PHYSTAT_LINK) &&
-	    !(mii_reg & MIIM_88E2110_PHYSTAT_SPDDONE)) {
+	if (!(mii_reg & MIIM_88E2110_PHYSTAT_LINK)) {
 		int i = 0;
 
 		puts("Waiting for PHY realtime link");
-		while (!(mii_reg & MIIM_88E2110_PHYSTAT_SPDDONE)) {
+		while (!(mii_reg & MIIM_88E2110_PHYSTAT_LINK)) {
 			/* Timeout reached ? */
 			if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
 				puts(" TIMEOUT !\n");
@@ -679,13 +678,13 @@ static uint m88e2110_parse_status(struct phy_device *phydev)
 		}
 		puts(" done\n");
 		mdelay(500);	/* another 500 ms (results in faster booting) */
-	} else {
-		if (mii_reg & MIIM_88E2110_PHYSTAT_LINK)
-			phydev->link = 1;
-		else
-			phydev->link = 0;
 	}
 
+	if (mii_reg & MIIM_88E2110_PHYSTAT_LINK)
+		phydev->link = 1;
+	else
+		phydev->link = 0;
+
 	if (mii_reg & MIIM_88E2110_PHYSTAT_DUPLEX)
 		phydev->duplex = DUPLEX_FULL;
 	else
-- 
2.31.0

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Kostya Porotchkin]

Hi, Marek,

> -----Original Message-----
> From: Marek Behun <marek.behun@nic.cz>
> Sent: Wednesday, March 24, 2021 12:44
> To: Stefan Roese <sr@denx.de>
> Cc: u-boot at lists.denx.de; Kostya Porotchkin <kostap@marvell.com>; Stefan
> Chulski <stefanc@marvell.com>; Ramon Fried <rfried.dev@gmail.com>;
> Nadav Haklai <nadavh@marvell.com>; Joe Hershberger
> <joe.hershberger@ni.com>; Marcin Wojtas <mw@semihalf.com>; sa_ip-sw-
> jenkins <sa_ip-sw-jenkins@marvell.com>; Igal Liberman <igall@marvell.com>;
> Simon Glass <sjg@chromium.org>; Yan Markman <ymarkman@marvell.com>
> Subject: [EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use
> both 2.5GHz modes
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Wed, 24 Mar 2021 10:20:05 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
> > PHY_INTERFACE_MODE_SGMII_2500
> 
> What the hell is this mode???
> 
> AFAIK something like this does not actually exist.
[KP] I think you are wrong. These modes are definitely exist
https://en.wikipedia.org/wiki/2.5GBASE-T_and_5GBASE-T

Regards
Kosta

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Stefan Chulski]

> Hi, Marek,
> 
> > -----Original Message-----
> > From: Marek Behun <marek.behun@nic.cz>
> > Sent: Wednesday, March 24, 2021 12:44
> > To: Stefan Roese <sr@denx.de>
> > Cc: u-boot at lists.denx.de; Kostya Porotchkin <kostap@marvell.com>;
> > Stefan Chulski <stefanc@marvell.com>; Ramon Fried
> > <rfried.dev@gmail.com>; Nadav Haklai <nadavh@marvell.com>; Joe
> > Hershberger <joe.hershberger@ni.com>; Marcin Wojtas
> <mw@semihalf.com>;
> > sa_ip-sw- jenkins <sa_ip-sw-jenkins@marvell.com>; Igal Liberman
> > <igall@marvell.com>; Simon Glass <sjg@chromium.org>; Yan Markman
> > <ymarkman@marvell.com>
> > Subject: [EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to
> > use both 2.5GHz modes
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > On Wed, 24 Mar 2021 10:20:05 +0100
> > Stefan Roese <sr@denx.de> wrote:
> >
> > > PHY_INTERFACE_MODE_SGMII_2500
> >
> > What the hell is this mode???
> >
> > AFAIK something like this does not actually exist.
> [KP] I think you are wrong. These modes are definitely exist
> https://en.wikipedia.org/wiki/2.5GBASE-T_and_5GBASE-T
> 
> Regards
> Kosta

Hi,

In COMPHY driver this called(COMPHY_HS_SGMII_MODE), SGMII working at 2.5G network speed.

Regards,
Stefan.

[PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Marek Behun]

On Wed, 24 Mar 2021 10:20:05 +0100
Stefan Roese <sr@denx.de> wrote:

> PHY_INTERFACE_MODE_SGMII_2500

What the hell is this mode???

AFAIK something like this does not actually exist.

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Kostya Porotchkin]

Hi, Marek,

> -----Original Message-----
> From: Marek Behun <marek.behun@nic.cz>
> Sent: Wednesday, March 24, 2021 13:47
> To: Kostya Porotchkin <kostap@marvell.com>
> Cc: Stefan Roese <sr@denx.de>; u-boot at lists.denx.de; Stefan Chulski
> <stefanc@marvell.com>; Ramon Fried <rfried.dev@gmail.com>; Nadav Haklai
> <nadavh@marvell.com>; Joe Hershberger <joe.hershberger@ni.com>; Marcin
> Wojtas <mw@semihalf.com>; sa_ip-sw-jenkins <sa_ip-sw-
> jenkins at marvell.com>; Igal Liberman <igall@marvell.com>; Simon Glass
> <sjg@chromium.org>; Yan Markman <ymarkman@marvell.com>
> Subject: Re: [EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use
> both 2.5GHz modes
> 
> On Wed, 24 Mar 2021 09:55:04 +0000
> Kostya Porotchkin <kostap@marvell.com> wrote:
> 
> > Hi, Marek,
> >
> > > -----Original Message-----
> > > From: Marek Behun <marek.behun@nic.cz>
> > > Sent: Wednesday, March 24, 2021 12:44
> > > To: Stefan Roese <sr@denx.de>
> > > Cc: u-boot at lists.denx.de; Kostya Porotchkin <kostap@marvell.com>;
> > > Stefan Chulski <stefanc@marvell.com>; Ramon Fried
> > > <rfried.dev@gmail.com>; Nadav Haklai <nadavh@marvell.com>; Joe
> > > Hershberger <joe.hershberger@ni.com>; Marcin Wojtas
> > > <mw@semihalf.com>; sa_ip-sw- jenkins <sa_ip-sw-jenkins@marvell.com>;
> > > Igal Liberman <igall@marvell.com>; Simon Glass <sjg@chromium.org>;
> > > Yan Markman <ymarkman@marvell.com>
> > > Subject: [EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110
> > > to use both 2.5GHz modes
> > >
> > > External Email
> > >
> > > --------------------------------------------------------------------
> > > --
> > > On Wed, 24 Mar 2021 10:20:05 +0100
> > > Stefan Roese <sr@denx.de> wrote:
> > >
> > > > PHY_INTERFACE_MODE_SGMII_2500
> > >
> > > What the hell is this mode???
> > >
> > > AFAIK something like this does not actually exist.
> > [KP] I think you are wrong. These modes are definitely exist
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__en.wikipedia.org_
> > wiki_2.5GBASE-2DT-5Fand-5F5GBASE-
> 2DT&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ
> > &r=-
> N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o&m=SCz6AxibFIYZS7
> OKXvlPK
> >
> CJvtAUaDPOmYRpvSNR3jv4&s=R_4D7fksFwBgE8_2RMOIyt5hIs8vgXjimR3E4O
> FUXeU&e
> > =
> >
> > Regards
> > Kosta
> 
> Hi Kosta,
> 
> the wikipedia page you linked specifies copper modes, not PHY modes.
> 
> We are discussing PHY modes here.
[KP] Ahh, sorry, mea culpa.
I think Stefan has some answers to your questions.

Regards
Kosta
> 
> What I am confused about is that this patch adds check for
>   PHY_INTERFACE_MODE_SGMII_2500
> in addition to
>   PHY_INTERFACE_MODE_2500BASEX
> 
> But what is the difference between these two?
> 
> Marvell named this protocol HS-SGMII in some of their datasheets and code. I
> guess this was done because of the similarities with 1000base-x and SGMII.
> Marvell uses the names SGMII and 1000base-x interchangably, although this is
> not correct. I guess they are similarily using names 2500base-x and HS-SGMII
> (and now SGMII_2500) interchangably, which is also not correct.
> 
> SGMII uses the same coding as 1000base-x, but the latter works only with one
> speed (1000mbps), while the former can also work in 10mbps and 100mbps (by
> repeating each byte 100 or 10 times, respectively).
> 
> Then there is 2500base-x, which is the same as 1000base-x, but with the clock
> being at 2.5x the speed of 1000base-x clock.
> 
> But there is no analogue of the SGMII protocol (i.e. the repearing of bytes in
> order to achieve lower speed) for the 2500base-x.
> 
> So what I am confused about here is what is supposed to be the difference
> between
>   PHY_INTERFACE_MODE_SGMII_2500
> and
>   PHY_INTERFACE_MODE_2500BASEX
> ?
> 
> Marek

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Marek Behun]

On Wed, 24 Mar 2021 09:55:04 +0000
Kostya Porotchkin <kostap@marvell.com> wrote:

> Hi, Marek,
> 
> > -----Original Message-----
> > From: Marek Behun <marek.behun@nic.cz>
> > Sent: Wednesday, March 24, 2021 12:44
> > To: Stefan Roese <sr@denx.de>
> > Cc: u-boot at lists.denx.de; Kostya Porotchkin <kostap@marvell.com>; Stefan
> > Chulski <stefanc@marvell.com>; Ramon Fried <rfried.dev@gmail.com>;
> > Nadav Haklai <nadavh@marvell.com>; Joe Hershberger
> > <joe.hershberger@ni.com>; Marcin Wojtas <mw@semihalf.com>; sa_ip-sw-
> > jenkins <sa_ip-sw-jenkins@marvell.com>; Igal Liberman <igall@marvell.com>;
> > Simon Glass <sjg@chromium.org>; Yan Markman <ymarkman@marvell.com>
> > Subject: [EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use
> > both 2.5GHz modes
> > 
> > External Email
> > 
> > ----------------------------------------------------------------------
> > On Wed, 24 Mar 2021 10:20:05 +0100
> > Stefan Roese <sr@denx.de> wrote:
> >   
> > > PHY_INTERFACE_MODE_SGMII_2500  
> > 
> > What the hell is this mode???
> > 
> > AFAIK something like this does not actually exist.  
> [KP] I think you are wrong. These modes are definitely exist
> https://en.wikipedia.org/wiki/2.5GBASE-T_and_5GBASE-T
> 
> Regards
> Kosta

Hi Kosta,

the wikipedia page you linked specifies copper modes, not PHY modes.

We are discussing PHY modes here.

What I am confused about is that this patch adds check for
  PHY_INTERFACE_MODE_SGMII_2500 
in addition to
  PHY_INTERFACE_MODE_2500BASEX

But what is the difference between these two?

Marvell named this protocol HS-SGMII in some of their datasheets
and code. I guess this was done because of the similarities with
1000base-x and SGMII. Marvell uses the names SGMII and 1000base-x
interchangably, although this is not correct. I guess they are
similarily using names 2500base-x and HS-SGMII (and now SGMII_2500)
interchangably, which is also not correct.

SGMII uses the same coding as 1000base-x, but the latter works only
with one speed (1000mbps), while the former can also work in 10mbps and
100mbps (by repeating each byte 100 or 10 times, respectively).

Then there is 2500base-x, which is the same as 1000base-x, but with the
clock being at 2.5x the speed of 1000base-x clock.

But there is no analogue of the SGMII protocol (i.e. the repearing of
bytes in order to achieve lower speed) for the 2500base-x.

So what I am confused about here is what is supposed to be the
difference between
  PHY_INTERFACE_MODE_SGMII_2500 
and
  PHY_INTERFACE_MODE_2500BASEX
?

Marek

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Stefan Chulski]

> > What I am confused about is that this patch adds check for
> >   PHY_INTERFACE_MODE_SGMII_2500
> > in addition to
> >   PHY_INTERFACE_MODE_2500BASEX
> >
> > But what is the difference between these two?
> >
> > Marvell named this protocol HS-SGMII in some of their datasheets and
> > code. I guess this was done because of the similarities with 1000base-x and
> SGMII.
> > Marvell uses the names SGMII and 1000base-x interchangably, although
> > this is not correct. I guess they are similarily using names
> > 2500base-x and HS-SGMII (and now SGMII_2500) interchangably, which is
> also not correct.
> >
> > SGMII uses the same coding as 1000base-x, but the latter works only
> > with one speed (1000mbps), while the former can also work in 10mbps
> > and 100mbps (by repeating each byte 100 or 10 times, respectively).
> >
> > Then there is 2500base-x, which is the same as 1000base-x, but with
> > the clock being at 2.5x the speed of 1000base-x clock.
> >
> > But there is no analogue of the SGMII protocol (i.e. the repearing of
> > bytes in order to achieve lower speed) for the 2500base-x.
> >
> > So what I am confused about here is what is supposed to be the
> > difference between
> >   PHY_INTERFACE_MODE_SGMII_2500
> > and
> >   PHY_INTERFACE_MODE_2500BASEX
> > ?
> >
> > Marek

I not sure what is correct naming for these mode. PHY_INTERFACE includes both MAC2PHY interfaces(MII, RGMII and etc), PHY2PHY interfaces(like BASEX) and SGMII(which is kind of both).
For both 2500BASEX and SGMII_2500 Serdes lanes set to HS-SGMII in 3.125G speed, but MAC configured differently and autoneg cannot be supported.

Regards,
Stefan.

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Stefan Chulski]

> > >
> > > SGMII uses the same coding as 1000base-x, but the latter works only
> > > with one speed (1000mbps), while the former can also work in 10mbps
> > > and 100mbps (by repeating each byte 100 or 10 times, respectively).
> > >
> > > Then there is 2500base-x, which is the same as 1000base-x, but with
> > > the clock being at 2.5x the speed of 1000base-x clock.
> > >
> > > But there is no analogue of the SGMII protocol (i.e. the repearing
> > > of bytes in order to achieve lower speed) for the 2500base-x.
> > >
> > > So what I am confused about here is what is supposed to be the
> > > difference between
> > >   PHY_INTERFACE_MODE_SGMII_2500
> > > and
> > >   PHY_INTERFACE_MODE_2500BASEX
> > > ?
> > >
> > > Marek
> 
> I not sure what is correct naming for these mode. PHY_INTERFACE includes
> both MAC2PHY interfaces(MII, RGMII and etc), PHY2PHY interfaces(like
> BASEX) and SGMII(which is kind of both).
> For both 2500BASEX and SGMII_2500 Serdes lanes set to HS-SGMII in 3.125G
> speed, but MAC configured differently and autoneg cannot be supported.
> 
> Regards,
> Stefan.

Since we already has PHY_INTERFACE_MODE_SGMII and PHY_INTERFACE_MODE_QSGMII (5G mode), maybe we should call
PHY_INTERFACE_MODE_HSGMII - High-Serial Gigabit Media Independent Interface (HSGMII, 3.125Gbps).

Regards,
Stefan.

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Marek Behun]

On Wed, 24 Mar 2021 13:09:00 +0000
Stefan Chulski <stefanc@marvell.com> wrote:

> > > >
> > > > SGMII uses the same coding as 1000base-x, but the latter works only
> > > > with one speed (1000mbps), while the former can also work in 10mbps
> > > > and 100mbps (by repeating each byte 100 or 10 times, respectively).
> > > >
> > > > Then there is 2500base-x, which is the same as 1000base-x, but with
> > > > the clock being at 2.5x the speed of 1000base-x clock.
> > > >
> > > > But there is no analogue of the SGMII protocol (i.e. the repearing
> > > > of bytes in order to achieve lower speed) for the 2500base-x.
> > > >
> > > > So what I am confused about here is what is supposed to be the
> > > > difference between
> > > >   PHY_INTERFACE_MODE_SGMII_2500
> > > > and
> > > >   PHY_INTERFACE_MODE_2500BASEX
> > > > ?
> > > >
> > > > Marek  
> > 
> > I not sure what is correct naming for these mode. PHY_INTERFACE includes
> > both MAC2PHY interfaces(MII, RGMII and etc), PHY2PHY interfaces(like
> > BASEX) and SGMII(which is kind of both).
> > For both 2500BASEX and SGMII_2500 Serdes lanes set to HS-SGMII in 3.125G
> > speed, but MAC configured differently and autoneg cannot be supported.
> > 
> > Regards,
> > Stefan.  
> 
> Since we already has PHY_INTERFACE_MODE_SGMII and PHY_INTERFACE_MODE_QSGMII (5G mode), maybe we should call
> PHY_INTERFACE_MODE_HSGMII - High-Serial Gigabit Media Independent Interface (HSGMII, 3.125Gbps).

And we have now autonegotiation in this discussion. Just today I sent a
question to Marvell about 2500base-x and why inband autonegotiation is
not supported there, while it is for 1000base-x.

So are you saying that 2500base-x mode is like 1000base-x, but at 2.5g
speed, and without inband autonegotiation?

And meanwhile HS-SGMII mode is like SGMII, but at 2.5g speed, and
_WITH_ autonegotiation? And what does this autonegotiation support?
Does is support only negotiation of Pause? Or does it support
negotiation of duplexicity and speed as well?

Thank you.

Marek

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Stefan Chulski]

> > > > > SGMII uses the same coding as 1000base-x, but the latter works
> > > > > only with one speed (1000mbps), while the former can also work
> > > > > in 10mbps and 100mbps (by repeating each byte 100 or 10 times,
> respectively).
> > > > >
> > > > > Then there is 2500base-x, which is the same as 1000base-x, but
> > > > > with the clock being at 2.5x the speed of 1000base-x clock.
> > > > >
> > > > > But there is no analogue of the SGMII protocol (i.e. the
> > > > > repearing of bytes in order to achieve lower speed) for the 2500base-
> x.
> > > > >
> > > > > So what I am confused about here is what is supposed to be the
> > > > > difference between
> > > > >   PHY_INTERFACE_MODE_SGMII_2500
> > > > > and
> > > > >   PHY_INTERFACE_MODE_2500BASEX
> > > > > ?
> > > > >
> > > > > Marek
> > >
> > > I not sure what is correct naming for these mode. PHY_INTERFACE
> > > includes both MAC2PHY interfaces(MII, RGMII and etc), PHY2PHY
> > > interfaces(like
> > > BASEX) and SGMII(which is kind of both).
> > > For both 2500BASEX and SGMII_2500 Serdes lanes set to HS-SGMII in
> > > 3.125G speed, but MAC configured differently and autoneg cannot be
> supported.
> > >
> > > Regards,
> > > Stefan.
> >
> > Since we already has PHY_INTERFACE_MODE_SGMII and
> > PHY_INTERFACE_MODE_QSGMII (5G mode), maybe we should call
> PHY_INTERFACE_MODE_HSGMII - High-Serial Gigabit Media Independent
> Interface (HSGMII, 3.125Gbps).
> 
> And we have now autonegotiation in this discussion. Just today I sent a
> question to Marvell about 2500base-x and why inband autonegotiation is not
> supported there, while it is for 1000base-x.

To clear thing out, inband autonegotiation is negotiation of pause, speed and duplex over PCS(usually between MAC and PHY).

> So are you saying that 2500base-x mode is like 1000base-x, but at 2.5g speed,
> and without inband autonegotiation?

From Serdes configurations point of view - lanes configured to 3_125G in 2500base-x and to 1_25G in 1000base-x. 
MAC doesn't support inband autonegotiation in base-x mode.
PHY side still can negotiate pause. 

So: Host side doesn't support inband autoneg(speed/duplex) and speed/duplex fixed. Line side can support autoneg of pause.

> And meanwhile HS-SGMII mode is like SGMII, but at 2.5g speed, and _WITH_
> autonegotiation? And what does this autonegotiation support?

SGMII has inband negotiation of speed and duplex(1000/100/10 - full/half), but since serdes reconfiguration required to switch between HS-SGMII and SGMII.
inband negotiation between 2.5C and 1G not supported.

So for example if you have:

MAC/Serdes set to 2.5G <-> PHY that support negotiation of link and negotiated 1G speed with link partner. Packet processor driver should reconfigure Serdes Lanes speed.

> Does is support only negotiation of Pause? Or does it support negotiation of link 
> duplexicity and speed as well?

In HS-SGMII there no much options speed always 2500 and duplex full, only pause taken from PCS inband Auto-Negotiation.
For SGMII pause, duplexicity and speed negotiation supported. 

Stefan,
Regards.

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Marek Behun]

On Wed, 24 Mar 2021 16:36:10 +0000
Stefan Chulski <stefanc@marvell.com> wrote:

> > > > > > SGMII uses the same coding as 1000base-x, but the latter works
> > > > > > only with one speed (1000mbps), while the former can also work
> > > > > > in 10mbps and 100mbps (by repeating each byte 100 or 10 times,  
> > respectively).  
> > > > > >
> > > > > > Then there is 2500base-x, which is the same as 1000base-x, but
> > > > > > with the clock being at 2.5x the speed of 1000base-x clock.
> > > > > >
> > > > > > But there is no analogue of the SGMII protocol (i.e. the
> > > > > > repearing of bytes in order to achieve lower speed) for the 2500base-  
> > x.  
> > > > > >
> > > > > > So what I am confused about here is what is supposed to be the
> > > > > > difference between
> > > > > >   PHY_INTERFACE_MODE_SGMII_2500
> > > > > > and
> > > > > >   PHY_INTERFACE_MODE_2500BASEX
> > > > > > ?
> > > > > >
> > > > > > Marek  
> > > >
> > > > I not sure what is correct naming for these mode. PHY_INTERFACE
> > > > includes both MAC2PHY interfaces(MII, RGMII and etc), PHY2PHY
> > > > interfaces(like
> > > > BASEX) and SGMII(which is kind of both).
> > > > For both 2500BASEX and SGMII_2500 Serdes lanes set to HS-SGMII in
> > > > 3.125G speed, but MAC configured differently and autoneg cannot be  
> > supported.  
> > > >
> > > > Regards,
> > > > Stefan.  
> > >
> > > Since we already has PHY_INTERFACE_MODE_SGMII and
> > > PHY_INTERFACE_MODE_QSGMII (5G mode), maybe we should call  
> > PHY_INTERFACE_MODE_HSGMII - High-Serial Gigabit Media Independent
> > Interface (HSGMII, 3.125Gbps).
> > 
> > And we have now autonegotiation in this discussion. Just today I sent a
> > question to Marvell about 2500base-x and why inband autonegotiation is not
> > supported there, while it is for 1000base-x.  
> 
> To clear thing out, inband autonegotiation is negotiation of pause, speed and duplex over PCS(usually between MAC and PHY).
> 
> > So are you saying that 2500base-x mode is like 1000base-x, but at 2.5g speed,
> > and without inband autonegotiation?  
> 
> From Serdes configurations point of view - lanes configured to 3_125G in 2500base-x and to 1_25G in 1000base-x. 
> MAC doesn't support inband autonegotiation in base-x mode.

Hi Stefan,

This is where you are wrong. MAC _does_ support inband autonegotiation
in base-x mode, specifically in 1000base-x mode (on the SerDes
connection between MAC and PHY). This is supported by Linux' mvneta
driver and also by 

Even Marvell's documentation for various PHYs says that inband
autonegotiation is supported for 1000base-x mode.

Look for example at document MV-S111371-00 Rev E
Marvell Alaska M 88E2181/88E2180/88E2111/88E2110
section 1.3.3 "H Unit 1000BASE-X/2500BASE-X Register Description"
There is register 4.0x2000 "1000BASE-X/SGMII Control Register"
bit 12 of this register is called "1000BASE-X Auto-Negotiation Enable"

And then look for example at datasheet for 88E2110 (doc MV-S111947-00
Rev A)
section 5.2.2 "2500BASE-X". It says:
  2500BASE-X is identical to 1000GBASE-X operation except at
  2.5 times speed. Auto-Negotiation is not supported in 2500BASE-X.

Then look at section 5.2.3.2, which says
  SGMII uses 1000BASE-X coding to send data as well as Auto-Negotiation
  information between the PHY and the MAC. However, the contents of the
  SGMII Auto-Negotiation are different than the 1000BASE-X Auto-Negotiation.

So clearly there is AN for 1000base-x, but not for 2500base-x.

So I think there is still misunderstanding.

I am going to explain to you how I understand the whole thing.
Note that all this time we are talking about auto-negotiation on the
link between MAC and PHY (i.e. how mvneta driver connects to a PHY, for
example).
- 1000base-x PCS is a SerDes protocol using 8b/10b encoding, and is
  capable of auto-negotiation. It allows to auto-negotiate flow-control
  (Pauses) and duplexicity (but is always full-duplex)
- sgmii extends this AN to also support speed autonegotiation
- you do not need to configure AN in SerDes code - no such thing is
  done by the comphy driver, only by the MAC driver for the MAC and PHY
  drivers for the PHY
- MAC can negotiate pause with the PHY in 1000base-x mode (mvneta
  ethernet driver + marvell10g PHY driver in kernel could do this, for
  example)
- 2500base-x PCS is the same as 1000base-x, i.e. a SerDes protocol
  using 8b/10b encoding, via which you can again connect a MAC with a
  PHY, or a PHY with another PHY, or even a MAC with another MAC.
  It's only difference from 1000base-x should be clock speed: it runs
  at 2.5x the speed of 1000base-x

  But for some reason, Marvell documentation for various PHYs says that
  AN is _NOT_ supported in this mode (although it is in 1000base-x).

  Reality is that Marvell's Armada 3720 MAC (mvneta driver) can enable
  AN in 2500base-x mode, and SerDes PHY in the Marvell 88E6141 Switch
  (Topaz) can also enable AN in 2500base-x mode, and they will link
  correctly, and the registers will look as though AN is supported. But
  for some reason the PHY documentation says that AN is NOT supported
  on the PHY for this mode.
  The same for Marvell's Peridot switch (88E6190).
  But it seems that in Amethyst (88E6393X) the AN registers are
  unavailable (always 0xffff) when in 2500base-x mode.
- there is no official specification for 2500base-x mode

> PHY side still can negotiate pause. 
> 
> So: Host side doesn't support inband autoneg(speed/duplex) and speed/duplex fixed. Line side can support autoneg of pause.
> 
> > And meanwhile HS-SGMII mode is like SGMII, but at 2.5g speed, and _WITH_
> > autonegotiation? And what does this autonegotiation support?  
> 
> SGMII has inband negotiation of speed and duplex(1000/100/10 - full/half), but since serdes reconfiguration required to switch between HS-SGMII and SGMII.
> inband negotiation between 2.5C and 1G not supported.
> 
> So for example if you have:
> 
> MAC/Serdes set to 2.5G <-> PHY that support negotiation of link and negotiated 1G speed with link partner. Packet processor driver should reconfigure Serdes Lanes speed.

I think that I am starting to understand where is the misunderstanding.
When I am talking about 1000base-x autonegotiation, I am talking about
autonegotiation between two SerDes peers (this can be MAC-PHY, or
MAC-MAC via a SFP module, for example).

> > Does is support only negotiation of Pause? Or does it support negotiation of link 
> > duplexicity and speed as well?  
> 
> In HS-SGMII there no much options speed always 2500 and duplex full, only pause taken from PCS inband Auto-Negotiation.
> For SGMII pause, duplexicity and speed negotiation supported. 

So you are saying that HS-SGMII does support Pause autonegotiation? But
that is contrary to what I wrote above, that Marvell's documentation
says that AN is not supported in 2500base-x mode.

This means that you are saying that 2500base-x is different mode from
HS-SGMII on the SerDes?

Because you are using PHY_INTERFACE_MODE_SGMII_2500 additionaly to
PHY_INTERFACE_MODE_2500BASEX. Note that Linux only defines the latter,
not the former. This is because there is NO SGMII supported over 2.5g
SerDes.

You have
  1000base-x supporting AN of Pause (and Duplex, but always full)
  sgmii      supporting AN of Pause and Speed and Duplex

And then if you multiply the clock 2.5x, you should theoretically have
  2500base-x supporting AN of Pause (and Duplex, but always full)

But you are acting as if there was
  2500base-x NOT supporting AN
  hs-sgmii   supporting AN of Pause

which is completely bonkers.

Could you please ask internally at Marvell?
We are trying to get to the bottom of this because we are stuck in
development of code for Amethyst. We need to get 2500base-x to work,
we need to know whether it does or does not support AN, or maybe does
but only for some devices. Otherwise it may happen that some SFPs
won't link with our hardware.

Thank you.

Marek

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Stefan Chulski]

> Could you please ask internally at Marvell?
> We are trying to get to the bottom of this because we are stuck in
> development of code for Amethyst. We need to get 2500base-x to work, we
> need to know whether it does or does not support AN, or maybe does but
> only for some devices. Otherwise it may happen that some SFPs won't link
> with our hardware.
> 
> Thank you.
> 
> Marek

To avoid confusions, I suggest we take this issue directly with Marvell SoHo switch FAE. I'm willing to start another thread to discuss this(I will check who can assist you).

Regarding to this patch series. Let's drop PHY_INTERFACE_MODE_SGMII_2500, from PHY_INTERFACE enum and we would handle SGMII mode in 2.5G speed differently in PPv2 driver.

Thanks,
Stefan.

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Stefan Roese]

Hi Stefan,
Hi Marek,

On 25.03.21 13:59, Stefan Chulski wrote:
>> Could you please ask internally at Marvell?
>> We are trying to get to the bottom of this because we are stuck in
>> development of code for Amethyst. We need to get 2500base-x to work, we
>> need to know whether it does or does not support AN, or maybe does but
>> only for some devices. Otherwise it may happen that some SFPs won't link
>> with our hardware.
>>
>> Thank you.
>>
>> Marek
> 
> To avoid confusions, I suggest we take this issue directly with Marvell
> SoHo switch FAE. I'm willing to start another thread to discuss this(I
> will check who can assist you).
> 
> Regarding to this patch series. Let's drop PHY_INTERFACE_MODE_SGMII_2500,
> from PHY_INTERFACE enum and we would handle SGMII mode in 2.5G speed
> differently in PPv2 driver.

I'm just now getting back to this patch.

JFYI: PHY_INTERFACE_MODE_SGMII_2500 is used in NXP ethernet drivers as
well:

$ git grep PHY_INTERFACE_MODE_SGMII_2500
board/freescale/ls1012aqds/eth.c: 
  PHY_INTERFACE_MODE_SGMII_2500);
board/freescale/ls1012aqds/eth.c: 
  PHY_INTERFACE_MODE_SGMII_2500);
board/freescale/ls1012ardb/eth.c: 
          PHY_INTERFACE_MODE_SGMII_2500);
board/freescale/ls1012ardb/eth.c: 
          PHY_INTERFACE_MODE_SGMII_2500);
board/freescale/ls1043aqds/eth.c: 
PHY_INTERFACE_MODE_SGMII_2500) {
board/freescale/ls1043aqds/eth.c:               case 
PHY_INTERFACE_MODE_SGMII_2500:
board/freescale/ls1043aqds/eth.c:                       } else if 
(interface == PHY_INTERFACE_MODE_SGMII_2500) {
board/freescale/ls1046aqds/eth.c:       } else if 
(fm_info_get_enet_if(port) == PHY_INTERFACE_MODE_SGMII_2500) {
board/freescale/t102xrdb/eth_t102xrdb.c:                case 
PHY_INTERFACE_MODE_SGMII_2500:
board/freescale/t102xrdb/eth_t102xrdb.c:        if 
(((fm_info_get_enet_if(port) == PHY_INTERFACE_MODE_SGMII_2500) ||
drivers/net/fm/eth.c:                   PHY_INTERFACE_MODE_SGMII_2500) ? 
true : false;
drivers/net/fm/eth.c:       fm_eth->enet_if == 
PHY_INTERFACE_MODE_SGMII_2500)
drivers/net/fm/eth.c:        (fm_eth->enet_if == 
PHY_INTERFACE_MODE_SGMII_2500) ||
drivers/net/fm/eth.c:   if (fm_eth->enet_if == 
PHY_INTERFACE_MODE_SGMII_2500)
drivers/net/fm/eth.c:   case PHY_INTERFACE_MODE_SGMII_2500:
drivers/net/fm/ls1043.c:                        return 
PHY_INTERFACE_MODE_SGMII_2500;
drivers/net/fm/ls1043.c:                        return 
PHY_INTERFACE_MODE_SGMII_2500;
drivers/net/fm/ls1046.c:                        return 
PHY_INTERFACE_MODE_SGMII_2500;
drivers/net/fm/memac.c: case PHY_INTERFACE_MODE_SGMII_2500:
drivers/net/fm/t1024.c:                 return 
PHY_INTERFACE_MODE_SGMII_2500;
drivers/net/fsl_enetc.c:        if (priv->if_type == 
PHY_INTERFACE_MODE_SGMII_2500)
drivers/net/fsl_enetc.c:        case PHY_INTERFACE_MODE_SGMII_2500:
drivers/net/mscc_eswitch/felix_switch.c:            phy->interface == 
PHY_INTERFACE_MODE_SGMII_2500)
drivers/net/mscc_eswitch/felix_switch.c:        case 
PHY_INTERFACE_MODE_SGMII_2500:
...

Stefan, you suggest to drop this define from PHY_INTERFACE enum which
we can't easily do with other drivers (like NXP) also referencing this
macro.

How to continue then?

Thanks,
Stefan

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Marek Behun]

On Thu, 8 Apr 2021 10:24:22 +0200
Stefan Roese <sr@denx.de> wrote:

> Hi Stefan,
> Hi Marek,
> 
> On 25.03.21 13:59, Stefan Chulski wrote:
> >> Could you please ask internally at Marvell?
> >> We are trying to get to the bottom of this because we are stuck in
> >> development of code for Amethyst. We need to get 2500base-x to work, we
> >> need to know whether it does or does not support AN, or maybe does but
> >> only for some devices. Otherwise it may happen that some SFPs won't link
> >> with our hardware.
> >>
> >> Thank you.
> >>
> >> Marek  
> > 
> > To avoid confusions, I suggest we take this issue directly with Marvell
> > SoHo switch FAE. I'm willing to start another thread to discuss this(I
> > will check who can assist you).
> > 
> > Regarding to this patch series. Let's drop PHY_INTERFACE_MODE_SGMII_2500,
> > from PHY_INTERFACE enum and we would handle SGMII mode in 2.5G speed
> > differently in PPv2 driver.  
> 
> I'm just now getting back to this patch.
> 
> JFYI: PHY_INTERFACE_MODE_SGMII_2500 is used in NXP ethernet drivers as
> well:
> 
> $ git grep PHY_INTERFACE_MODE_SGMII_2500
> board/freescale/ls1012aqds/eth.c: 
>   PHY_INTERFACE_MODE_SGMII_2500);
> board/freescale/ls1012aqds/eth.c: 
>   PHY_INTERFACE_MODE_SGMII_2500);
> board/freescale/ls1012ardb/eth.c: 
>           PHY_INTERFACE_MODE_SGMII_2500);
> board/freescale/ls1012ardb/eth.c: 
>           PHY_INTERFACE_MODE_SGMII_2500);
> board/freescale/ls1043aqds/eth.c: 
> PHY_INTERFACE_MODE_SGMII_2500) {
> board/freescale/ls1043aqds/eth.c:               case 
> PHY_INTERFACE_MODE_SGMII_2500:
> board/freescale/ls1043aqds/eth.c:                       } else if 
> (interface == PHY_INTERFACE_MODE_SGMII_2500) {
> board/freescale/ls1046aqds/eth.c:       } else if 
> (fm_info_get_enet_if(port) == PHY_INTERFACE_MODE_SGMII_2500) {
> board/freescale/t102xrdb/eth_t102xrdb.c:                case 
> PHY_INTERFACE_MODE_SGMII_2500:
> board/freescale/t102xrdb/eth_t102xrdb.c:        if 
> (((fm_info_get_enet_if(port) == PHY_INTERFACE_MODE_SGMII_2500) ||
> drivers/net/fm/eth.c:                   PHY_INTERFACE_MODE_SGMII_2500) ? 
> true : false;
> drivers/net/fm/eth.c:       fm_eth->enet_if == 
> PHY_INTERFACE_MODE_SGMII_2500)
> drivers/net/fm/eth.c:        (fm_eth->enet_if == 
> PHY_INTERFACE_MODE_SGMII_2500) ||
> drivers/net/fm/eth.c:   if (fm_eth->enet_if == 
> PHY_INTERFACE_MODE_SGMII_2500)
> drivers/net/fm/eth.c:   case PHY_INTERFACE_MODE_SGMII_2500:
> drivers/net/fm/ls1043.c:                        return 
> PHY_INTERFACE_MODE_SGMII_2500;
> drivers/net/fm/ls1043.c:                        return 
> PHY_INTERFACE_MODE_SGMII_2500;
> drivers/net/fm/ls1046.c:                        return 
> PHY_INTERFACE_MODE_SGMII_2500;
> drivers/net/fm/memac.c: case PHY_INTERFACE_MODE_SGMII_2500:
> drivers/net/fm/t1024.c:                 return 
> PHY_INTERFACE_MODE_SGMII_2500;
> drivers/net/fsl_enetc.c:        if (priv->if_type == 
> PHY_INTERFACE_MODE_SGMII_2500)
> drivers/net/fsl_enetc.c:        case PHY_INTERFACE_MODE_SGMII_2500:
> drivers/net/mscc_eswitch/felix_switch.c:            phy->interface == 
> PHY_INTERFACE_MODE_SGMII_2500)
> drivers/net/mscc_eswitch/felix_switch.c:        case 
> PHY_INTERFACE_MODE_SGMII_2500:
> ...
> 
> Stefan, you suggest to drop this define from PHY_INTERFACE enum which
> we can't easily do with other drivers (like NXP) also referencing this
> macro.
> 
> How to continue then?

I think NXP also uses this macro incorrectly, they should instead use
2500BASEX. SGMII_2500 does not exist in Linux. We can look at the
corresponding NXP driver in Linux (if it exists) to see if the
corresponging mode is 2500BASEX.

Marek

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Stefan Chulski]

> 
> Stefan, you suggest to drop this define from PHY_INTERFACE enum which we
> can't easily do with other drivers (like NXP) also referencing this macro.
> 
> How to continue then?
> 
> Thanks,
> Stefan

Probably we should drop SGMII_2500 from this series, introduce "manage" devicetree property(that would indicate if inband supported or not).
For example this is armada-8040-mcbin.dtsi In kernel, when in-band supported:
&cp1_eth2 {
	/* CPS Lane 5 */
	status = "okay";
	/* Network PHY */
	phy-mode = "2500base-x";
	managed = "in-band-status";
	/* Generic PHY, providing serdes lanes */
	phys = <&cp1_comphy5 2>;
	sfp = <&sfp_eth3>;
}; 

If in-band not supported(for example PPv2 MAC connected to 88E2110 in 2.5G speed) we would use default managed = "auto" and fixed link property.

Regards.

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Marek Behun]

On Thu, 8 Apr 2021 19:18:09 +0000
Stefan Chulski <stefanc@marvell.com> wrote:

> > 
> > Stefan, you suggest to drop this define from PHY_INTERFACE enum which we
> > can't easily do with other drivers (like NXP) also referencing this macro.
> > 
> > How to continue then?
> > 
> > Thanks,
> > Stefan  
> 
> Probably we should drop SGMII_2500 from this series, introduce "manage" devicetree property(that would indicate if inband supported or not).
> For example this is armada-8040-mcbin.dtsi In kernel, when in-band supported:
> &cp1_eth2 {
> 	/* CPS Lane 5 */
> 	status = "okay";
> 	/* Network PHY */
> 	phy-mode = "2500base-x";
> 	managed = "in-band-status";
> 	/* Generic PHY, providing serdes lanes */
> 	phys = <&cp1_comphy5 2>;
> 	sfp = <&sfp_eth3>;
> }; 
> 
> If in-band not supported(for example PPv2 MAC connected to 88E2110 in 2.5G speed) we would use default managed = "auto" and fixed link property.

Such DTS properties should first be proposed to device-tree ML
and documented in devicetree bindings documentation.

Marek

[EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes

[Stefan Chulski]

> > If in-band not supported(for example PPv2 MAC connected to 88E2110 in
> 2.5G speed) we would use default managed = "auto" and fixed link property.
> 
> Such DTS properties should first be proposed to device-tree ML and
> documented in devicetree bindings documentation.
> 
> Marek

This properties already exist in kernel device-tree, see "managed"
https://elixir.bootlin.com/linux/v4.12.14/source/Documentation/devicetree/bindings/net/ethernet.txt

Regards.