[PATCH net-next 0/7] net: phy: Add support for 2.5GBASET PHYs

Hello everyone,

This series introduces support for 2.5GBASET mode in Network PHYs, and
implements that support in some PHYs in the Marvell Alaska product
range, the PPv2 driver, and a new PHY from the Alaska family.

2.5GBASET and 5GBASET were originally defined by the NBASET alliance,
and are now being specified as an IEEE standard, under the 802.3bz
specification.

The first patch in the series generalizes the way we build the list of
supported modes in C45 PHYs, relying on the various modes exposed in the
PMA registers. It extracts the logic from the marvell10g driver.

The second patch adds the new register definitions from the 802.3bz
specifications, and adds the generic code to handle both 2.5GBASET and
5GBASET.

The third patch allows to automatically detect whether or not a PHY
supports 2.5GBASET and 5GBASET, by reading the PMA abilities.

Patches 4 and 5 add support for 2.5GBASET into the marvell10g driver,
with a workaround for incorrect abilities being reported by the 88X3310
and the 88E2010 PHYs. (see patch 5).

Patch 6 adds support for 2.5GBASET in the PPv2 ethernet driver, thus
enabling support for that mode on the MacchiatoBin.

Finally, patch 7 introduces support for the 88x2110 PHY, which is very
similar to the 88x3310, the only difference being the lack of Fiber
support. The 88x2110 already reports the correct abilities from it's
PMA, and thus doesn't require the workaround implemented in patch 5 for
2.5GBASET to work. This is based on work originally done by Antoine Tenart.

This patch series was tested on the MacchiatoBin, a custom board using a
88e2010, and another using a 88x2110.

The 88x2110 support is enclosed in the series to better show the need
for the workaround in the 88x3310, feel free to tell me if we should
handle such hardware issues in another way, or if support for that PHY
should be submitted separately.

At this point, future development adding 5GBASET to Marvell PHYs should be
straightforward, but this isn't included in the series since I lack any
hardware to test the 5GBASE-R mode that the PHY uses to connect to the
MAC in that case.

Thanks to Andrew for doing the rework of the link_mode representation
this series depends on.

Maxime Chevallier (7):
  net: phy: Extract genphy_c45_read_abilities from marvell10g
  net: phy: Add generic support for 2.5GBaseT and 5GBaseT
  net: phy: Read 2.5G and 5G extended abilities
  net: phy: marvell10g: Add support for 2.5GBASET and 5GBASET
  net: phy: marvell10g: Force reading of 2.5/5G PMA extended abilities
  net: mvpp2: Add 2.5GBaseT support
  net: phy: marvell10g: add support for the 88x2110 PHY

 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |   1 +
 drivers/net/phy/marvell10g.c                  | 155 +++++++++---------
 drivers/net/phy/phy-c45.c                     | 132 +++++++++++++++
 include/linux/phy.h                           |   1 +
 include/uapi/linux/mdio.h                     |  16 ++
 5 files changed, 226 insertions(+), 79 deletions(-)

-- 
2.19.2

Register 1.21 "2.5G/5G PMA Extended abilities" contains the information
indicating whether or not 2.5GBASET and 5GBASET are supported by a PHY,
as per the 802.3bz specification.

If the bit 14 is set in the 1.11 "PMA Extended abilities" register, the
modes specified in the above-mentionned 1.21 register should be taken
into account.

This commit adds that logic into the genphy_c45_read_abilities function.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy-c45.c | 14 ++++++++++++++
 include/uapi/linux/mdio.h |  6 ++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 61ca4f89e94a..ee32eba3dc20 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c

@@ -377,6 +377,20 @@ int genphy_c45_read_abilities(struct phy_device *phydev)
 			__set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
 				  supported);
 		}
+
+		if (val & MDIO_PMA_EXTABLE_NBT) {
+			val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
+					   MDIO_PMA_NG_EXTABLE);
+			if (val < 0)
+				return val;
+
+			if (val & MDIO_PMA_NG_EXTABLE_2_5GBT)
+				__set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+					  supported);
+			if (val & MDIO_PMA_NG_EXTABLE_5GBT)
+				__set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+					  supported);
+		}
 	}
 
 	linkmode_copy(phydev->supported, supported);

diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index e2ab03606c1b..546509898867 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h

@@ -45,6 +45,7 @@
 #define MDIO_AN_ADVERTISE	16	/* AN advertising (base page) */
 #define MDIO_AN_LPA		19	/* AN LP abilities (base page) */
 #define MDIO_PCS_EEE_ABLE	20	/* EEE Capability register */
+#define MDIO_PMA_NG_EXTABLE	21	/* 2.5G/5G PMA/PMD extended ability */
 #define MDIO_PCS_EEE_WK_ERR	22	/* EEE wake error counter */
 #define MDIO_PHYXS_LNSTAT	24	/* PHY XGXS lane state */
 #define MDIO_AN_EEE_ADV		60	/* EEE advertisement */

@@ -201,6 +202,7 @@
 #define MDIO_PMA_EXTABLE_1000BKX	0x0040	/* 1000BASE-KX ability */
 #define MDIO_PMA_EXTABLE_100BTX		0x0080	/* 100BASE-TX ability */
 #define MDIO_PMA_EXTABLE_10BT		0x0100	/* 10BASE-T ability */
+#define MDIO_PMA_EXTABLE_NBT		0x4000  /* 2.5/5GBASE-T ability */
 
 /* PHY XGXS lane state register. */
 #define MDIO_PHYXS_LNSTAT_SYNC0		0x0001

@@ -272,6 +274,10 @@
 #define MDIO_EEE_10GKX4		0x0020	/* 10G KX4 EEE cap */
 #define MDIO_EEE_10GKR		0x0040	/* 10G KR EEE cap */
 
+/* 2.5G/5G Extended abilities register. */
+#define MDIO_PMA_NG_EXTABLE_2_5GBT	0x0001	/* 2.5GBASET ability */
+#define MDIO_PMA_NG_EXTABLE_5GBT	0x0002	/* 5GBASET ability */
+
 /* LASI RX_ALARM control/status registers. */
 #define MDIO_PMA_LASI_RX_PHYXSLFLT	0x0001	/* PHY XS RX local fault */
 #define MDIO_PMA_LASI_RX_PCSLFLT	0x0008	/* PCS RX local fault */

-- 
2.19.2

The 802.3bz specification, based on previous by the NBASET alliance,
defines the 2.5GBaseT and 5GBaseT link modes for ethernet traffic on
cat5e, cat6 and cat7 cables.

These mode integrate with the already defined C45 MDIO PMA/PMD registers
set that added 10G support, by defining some previously reserved bits,
and adding a new register (2.5G/5G Extended abilities).

This commit adds the required definitions in include/uapi/linux/mdio.h
to support these modes, and detect when a link-partner advertises them.

It also adds support for these mode in the generic C45 PHY
infrastructure.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy-c45.c | 22 ++++++++++++++++++++++
 include/uapi/linux/mdio.h | 10 ++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 31806b432734..61ca4f89e94a 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c

@@ -47,6 +47,16 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
 		/* Assume 1000base-T */
 		ctrl2 |= MDIO_PMA_CTRL2_1000BT;
 		break;
+	case SPEED_2500:
+		ctrl1 |= MDIO_CTRL1_SPEED2_5G;
+		/* Assume 2.5Gbase-T */
+		ctrl2 |= MDIO_PMA_CTRL2_2_5GBT;
+		break;
+	case SPEED_5000:
+		ctrl1 |= MDIO_CTRL1_SPEED5G;
+		/* Assume 5Gbase-T */
+		ctrl2 |= MDIO_PMA_CTRL2_5GBT;
+		break;
 	case SPEED_10000:
 		ctrl1 |= MDIO_CTRL1_SPEED10G;
 		/* Assume 10Gbase-T */

@@ -190,6 +200,12 @@ int genphy_c45_read_lpa(struct phy_device *phydev)
 	if (val < 0)
 		return val;
 
+	if (val & MDIO_AN_10GBT_STAT_LP2_5G)
+		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+				 phydev->lp_advertising);
+	if (val & MDIO_AN_10GBT_STAT_LP5G)
+		linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+				 phydev->lp_advertising);
 	if (val & MDIO_AN_10GBT_STAT_LP10G)
 		linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
 				 phydev->lp_advertising);

@@ -220,6 +236,12 @@ int genphy_c45_read_pma(struct phy_device *phydev)
 	case MDIO_PMA_CTRL1_SPEED1000:
 		phydev->speed = SPEED_1000;
 		break;
+	case MDIO_CTRL1_SPEED2_5G:
+		phydev->speed = SPEED_2500;
+		break;
+	case MDIO_CTRL1_SPEED5G:
+		phydev->speed = SPEED_5000;
+		break;
 	case MDIO_CTRL1_SPEED10G:
 		phydev->speed = SPEED_10000;
 		break;

diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index d435b00d64ad..e2ab03606c1b 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h

@@ -92,6 +92,10 @@
 #define MDIO_CTRL1_SPEED10G		(MDIO_CTRL1_SPEEDSELEXT | 0x00)
 /* 10PASS-TS/2BASE-TL */
 #define MDIO_CTRL1_SPEED10P2B		(MDIO_CTRL1_SPEEDSELEXT | 0x04)
+/* 2.5 Gb/s */
+#define MDIO_CTRL1_SPEED2_5G		(MDIO_CTRL1_SPEEDSELEXT | 0x18)
+/* 5 Gb/s */
+#define MDIO_CTRL1_SPEED5G		(MDIO_CTRL1_SPEEDSELEXT | 0x1c)
 
 /* Status register 1. */
 #define MDIO_STAT1_LPOWERABLE		0x0002	/* Low-power ability */

@@ -142,6 +146,8 @@
 #define MDIO_PMA_CTRL2_1000BKX		0x000d	/* 1000BASE-KX type */
 #define MDIO_PMA_CTRL2_100BTX		0x000e	/* 100BASE-TX type */
 #define MDIO_PMA_CTRL2_10BT		0x000f	/* 10BASE-T type */
+#define MDIO_PMA_CTRL2_2_5GBT		0x0030  /* 2.5GBaseT type */
+#define MDIO_PMA_CTRL2_5GBT		0x0031  /* 5GBaseT type */
 #define MDIO_PCS_CTRL2_TYPE		0x0003	/* PCS type selection */
 #define MDIO_PCS_CTRL2_10GBR		0x0000	/* 10GBASE-R type */
 #define MDIO_PCS_CTRL2_10GBX		0x0001	/* 10GBASE-X type */

@@ -231,9 +237,13 @@
 #define MDIO_PCS_10GBRT_STAT2_BER	0x3f00
 
 /* AN 10GBASE-T control register. */
+#define MDIO_AN_10GBT_CTRL_ADV2_5G	0x0080	/* Advertise 2.5GBASE-T */
+#define MDIO_AN_10GBT_CTRL_ADV5G	0x0100	/* Advertise 5GBASE-T */
 #define MDIO_AN_10GBT_CTRL_ADV10G	0x1000	/* Advertise 10GBASE-T */
 
 /* AN 10GBASE-T status register. */
+#define MDIO_AN_10GBT_STAT_LP2_5G	0x0020  /* LP is 2.5GBT capable */
+#define MDIO_AN_10GBT_STAT_LP5G		0x0040  /* LP is 5GBT capable */
 #define MDIO_AN_10GBT_STAT_LPTRR	0x0200	/* LP training reset req. */
 #define MDIO_AN_10GBT_STAT_LPLTABLE	0x0400	/* LP loop timing ability */
 #define MDIO_AN_10GBT_STAT_LP10G	0x0800	/* LP is 10GBT capable */

-- 
2.19.2

As per 802.3bz, if bit 14 of (1.11) "PMA Extended Abilities" indicates
whether or not we should read register (1.21) "2.52/5G PMA Extended
Abilities", which contains information on the support of 2.5GBASET and
5GBASET.

After testing on several variants of PHYS of this family, it appears
that bit 14 in (1.11) isn't always set when it should be.

PHYs 88X3310 (on MacchiatoBin) and 88E2010 do support 2.5G and 5GBASET,
but don't have 1.11.14 set. Their register 1.21 is filled with the
correct values, indicating 2.5G and 5G support.

PHYs 88X2110 do have their 1.11.14 bit set, as it should.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/marvell10g.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index f45ddf3bc138..0a35bf0fac47 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c

@@ -251,7 +251,7 @@ static int mv3310_resume(struct phy_device *phydev)
 
 static int mv3310_config_init(struct phy_device *phydev)
 {
-	int ret;
+	int ret, val;
 
 	/* Check that the PHY interface type is compatible */
 	if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&

@@ -265,6 +265,23 @@ static int mv3310_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	/* Some PHYs in the Alaska family such as the 88X3310 and the 88E2010
+	 * don't set bit 14 in PMA Extended Abilities (1.11), although they do
+	 * support 2.5GBASET and 5GBASET. Their 2.5/5G PMA Extended abilities
+	 * (1.21) still have a meaningful value, so read it anyway to make sure
+	 * we enable support for these modes if needed.
+	 */
+	val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);
+	if (val < 0)
+		return val;
+
+	if (val & MDIO_PMA_NG_EXTABLE_2_5GBT)
+		__set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+			  phydev->supported);
+	if (val & MDIO_PMA_NG_EXTABLE_5GBT)
+		__set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+			  phydev->supported);
+
 	/* Make sure we advertise all the supported modes, and not just the
 	 * default one specified in the driver's .features.
 	 */

-- 
2.19.2

This patch adds support for the 88x2110 PHY, which is similar to the
already supported 88x3310 PHY without the SFP interface.

It supports 10/100/1000BASET along with 2.5GBASET, 5GBASET and 10GBASET,
with the same interface modes that are used by the 3310.

This PHY don't have the same issue as the 88x3310 regarding 2.5/5G
abilities, and correctly follows the 802.3bz standard to list the
supported abilities.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Suggested-by: Antoine Tenart <redacted>
---
 drivers/net/phy/marvell10g.c | 37 ++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 0a35bf0fac47..106491ecc016 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c

@@ -290,6 +290,30 @@ static int mv3310_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int mv2110_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Check that the PHY interface type is compatible */
+	if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
+	    phydev->interface != PHY_INTERFACE_MODE_2500BASEX &&
+	    phydev->interface != PHY_INTERFACE_MODE_XAUI &&
+	    phydev->interface != PHY_INTERFACE_MODE_RXAUI &&
+	    phydev->interface != PHY_INTERFACE_MODE_10GKR)
+		return -ENODEV;
+
+	ret = genphy_c45_read_abilities(phydev);
+	if (ret)
+		return ret;
+
+	/* Make sure we advertise all the supported modes, and not just the
+	 * default one specified in the driver's .features.
+	 */
+	linkmode_copy(phydev->advertising, phydev->supported);
+
+	return 0;
+}
+
 static int mv3310_config_aneg(struct phy_device *phydev)
 {
 	bool changed = false;

@@ -504,12 +528,25 @@ static struct phy_driver mv3310_drivers[] = {
 		.aneg_done	= mv3310_aneg_done,
 		.read_status	= mv3310_read_status,
 	},
+	{
+		.phy_id		= 0x002b09b8,
+		.phy_id_mask	= MARVELL_PHY_ID_MASK,
+		.name		= "mv88x2110",
+		.features	= PHY_10GBIT_FEATURES,
+		.probe		= mv3310_probe,
+		.soft_reset	= gen10g_no_soft_reset,
+		.config_init	= mv2110_config_init,
+		.config_aneg	= mv3310_config_aneg,
+		.aneg_done	= mv3310_aneg_done,
+		.read_status	= mv3310_read_status,
+	},
 };
 
 module_phy_driver(mv3310_drivers);
 
 static struct mdio_device_id __maybe_unused mv3310_tbl[] = {
 	{ 0x002b09aa, MARVELL_PHY_ID_MASK },
+	{ 0x002b09b8, MARVELL_PHY_ID_MASK },
 	{ },
 };
 MODULE_DEVICE_TABLE(mdio, mv3310_tbl);

-- 
2.19.2

The Marvell Alaska family of PHYs supports 2.5GBaseT and 5GBaseT modes,
as defined in the 802.3bz specification.

When the link partner requests a 2.5GBASET link, the PHY will
reconfigure it's MII interface to 2500BASEX.

At 5G, the PHY will reconfigure it's interface to 5GBASE-R, but this
mode isn't supported by any MAC for now.

This was tested with :
 - The 88X3310, which is on the MacchiatoBin
 - The 88E2010, an Alaska PHY that has no fiber interfaces, and is
   limited to 5G maximum speed.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/marvell10g.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index f2a6d6e7041a..f45ddf3bc138 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c

@@ -255,6 +255,7 @@ static int mv3310_config_init(struct phy_device *phydev)
 
 	/* Check that the PHY interface type is compatible */
 	if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
+	    phydev->interface != PHY_INTERFACE_MODE_2500BASEX &&
 	    phydev->interface != PHY_INTERFACE_MODE_XAUI &&
 	    phydev->interface != PHY_INTERFACE_MODE_RXAUI &&
 	    phydev->interface != PHY_INTERFACE_MODE_10GKR)

@@ -264,8 +265,10 @@ static int mv3310_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	linkmode_and(phydev->advertising, phydev->advertising,
-		     phydev->supported);
+	/* Make sure we advertise all the supported modes, and not just the
+	 * default one specified in the driver's .features.
+	 */
+	linkmode_copy(phydev->advertising, phydev->supported);
 
 	return 0;
 }

@@ -314,8 +317,17 @@ static int mv3310_config_aneg(struct phy_device *phydev)
 	else
 		reg = 0;
 
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+			      phydev->advertising))
+		reg |= MDIO_AN_10GBT_CTRL_ADV2_5G;
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+			      phydev->advertising))
+		reg |= MDIO_AN_10GBT_CTRL_ADV5G;
+
 	ret = mv3310_modify(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
-			    MDIO_AN_10GBT_CTRL_ADV10G, reg);
+			    MDIO_AN_10GBT_CTRL_ADV10G |
+			    MDIO_AN_10GBT_CTRL_ADV5G |
+			    MDIO_AN_10GBT_CTRL_ADV2_5G, reg);
 	if (ret < 0)
 		return ret;
 	if (ret > 0)

@@ -344,17 +356,20 @@ static int mv3310_aneg_done(struct phy_device *phydev)
 static void mv3310_update_interface(struct phy_device *phydev)
 {
 	if ((phydev->interface == PHY_INTERFACE_MODE_SGMII ||
+	     phydev->interface == PHY_INTERFACE_MODE_2500BASEX ||
 	     phydev->interface == PHY_INTERFACE_MODE_10GKR) && phydev->link) {
 		/* The PHY automatically switches its serdes interface (and
-		 * active PHYXS instance) between Cisco SGMII and 10GBase-KR
-		 * modes according to the speed.  Florian suggests setting
-		 * phydev->interface to communicate this to the MAC. Only do
-		 * this if we are already in either SGMII or 10GBase-KR mode.
+		 * active PHYXS instance) between Cisco SGMII, 10GBase-KR and
+		 * 2500BaseX modes according to the speed.  Florian suggests
+		 * setting phydev->interface to communicate this to the MAC.
+		 * Only do this if we are already in one of the above modes.
 		 */
 		if (phydev->speed == SPEED_10000)
 			phydev->interface = PHY_INTERFACE_MODE_10GKR;
+		else if (phydev->speed == SPEED_2500)
+			phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
 		else if (phydev->speed >= SPEED_10 &&
-			 phydev->speed < SPEED_10000)
+			 phydev->speed < SPEED_2500)
 			phydev->interface = PHY_INTERFACE_MODE_SGMII;
 	}
 }

-- 
2.19.2

The PPv2 controller is able to support 2.5G speeds, allowing to use
2.5GBASET in conjunction with PHYs that use 2500BASEX as their MII
interface when using this mode.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 16066c2d5b3a..82f210c7e3e9 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c

@@ -4414,6 +4414,7 @@ static void mvpp2_phylink_validate(struct net_device *dev,
 	case PHY_INTERFACE_MODE_2500BASEX:
 		phylink_set(mask, 1000baseT_Full);
 		phylink_set(mask, 1000baseX_Full);
+		phylink_set(mask, 2500baseT_Full);
 		phylink_set(mask, 2500baseX_Full);
 		break;
 	default:

-- 
2.19.2

Marvell 10G PHY driver has a generic way of initializing the supported
link modes by reading the PHY's C45 PMA abilities. This can be made
generic, since these registers are part of the 802.3 specifications.

This commit extracts the config_init link_mode initialization code from
marvell10g and uses it to introduce the genphy_c45_read_abilities
function.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/marvell10g.c | 80 ++----------------------------
 drivers/net/phy/phy-c45.c    | 96 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  1 +
 3 files changed, 101 insertions(+), 76 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 82ab6ed3b74e..f2a6d6e7041a 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c

@@ -251,8 +251,7 @@ static int mv3310_resume(struct phy_device *phydev)
 
 static int mv3310_config_init(struct phy_device *phydev)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
-	int val;
+	int ret;
 
 	/* Check that the PHY interface type is compatible */
 	if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&

@@ -261,81 +260,10 @@ static int mv3310_config_init(struct phy_device *phydev)
 	    phydev->interface != PHY_INTERFACE_MODE_10GKR)
 		return -ENODEV;
 
-	__set_bit(ETHTOOL_LINK_MODE_Pause_BIT, supported);
-	__set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, supported);
-
-	if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
-		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
-		if (val < 0)
-			return val;
-
-		if (val & MDIO_AN_STAT1_ABLE)
-			__set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, supported);
-	}
-
-	val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT2);
-	if (val < 0)
-		return val;
-
-	/* Ethtool does not support the WAN mode bits */
-	if (val & (MDIO_PMA_STAT2_10GBSR | MDIO_PMA_STAT2_10GBLR |
-		   MDIO_PMA_STAT2_10GBER | MDIO_PMA_STAT2_10GBLX4 |
-		   MDIO_PMA_STAT2_10GBSW | MDIO_PMA_STAT2_10GBLW |
-		   MDIO_PMA_STAT2_10GBEW))
-		__set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, supported);
-	if (val & MDIO_PMA_STAT2_10GBSR)
-		__set_bit(ETHTOOL_LINK_MODE_10000baseSR_Full_BIT, supported);
-	if (val & MDIO_PMA_STAT2_10GBLR)
-		__set_bit(ETHTOOL_LINK_MODE_10000baseLR_Full_BIT, supported);
-	if (val & MDIO_PMA_STAT2_10GBER)
-		__set_bit(ETHTOOL_LINK_MODE_10000baseER_Full_BIT, supported);
-
-	if (val & MDIO_PMA_STAT2_EXTABLE) {
-		val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE);
-		if (val < 0)
-			return val;
-
-		if (val & (MDIO_PMA_EXTABLE_10GBT | MDIO_PMA_EXTABLE_1000BT |
-			   MDIO_PMA_EXTABLE_100BTX | MDIO_PMA_EXTABLE_10BT))
-			__set_bit(ETHTOOL_LINK_MODE_TP_BIT, supported);
-		if (val & MDIO_PMA_EXTABLE_10GBLRM)
-			__set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, supported);
-		if (val & (MDIO_PMA_EXTABLE_10GBKX4 | MDIO_PMA_EXTABLE_10GBKR |
-			   MDIO_PMA_EXTABLE_1000BKX))
-			__set_bit(ETHTOOL_LINK_MODE_Backplane_BIT, supported);
-		if (val & MDIO_PMA_EXTABLE_10GBLRM)
-			__set_bit(ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT,
-				  supported);
-		if (val & MDIO_PMA_EXTABLE_10GBT)
-			__set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
-				  supported);
-		if (val & MDIO_PMA_EXTABLE_10GBKX4)
-			__set_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT,
-				  supported);
-		if (val & MDIO_PMA_EXTABLE_10GBKR)
-			__set_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
-				  supported);
-		if (val & MDIO_PMA_EXTABLE_1000BT)
-			__set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
-				  supported);
-		if (val & MDIO_PMA_EXTABLE_1000BKX)
-			__set_bit(ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
-				  supported);
-		if (val & MDIO_PMA_EXTABLE_100BTX) {
-			__set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
-				  supported);
-			__set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
-				  supported);
-		}
-		if (val & MDIO_PMA_EXTABLE_10BT) {
-			__set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
-				  supported);
-			__set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
-				  supported);
-		}
-	}
+	ret = genphy_c45_read_abilities(phydev);
+	if (ret)
+		return ret;
 
-	linkmode_copy(phydev->supported, supported);
 	linkmode_and(phydev->advertising, phydev->advertising,
 		     phydev->supported);
 

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 03af927fa5ad..31806b432734 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c

@@ -267,6 +267,102 @@ int genphy_c45_read_mdix(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(genphy_c45_read_mdix);
 
+/**
+ * genphy_c45_read_abilities - read supported link modes from PMA
+ * @phydev: target phy_device struct
+ *
+ * Read the supported link modes from the PMA Status 2 (1.8) register. If bit
+ * 1.8.9 is set, the list of supported modes is completed with the values in the
+ * PMA Extended Abilities (1.11) register, indicating 1000BASET an 10G related
+ * modes. If bit 1.11.14 is set, then the list is also completed with the modes
+ * in the 2.5G/5G PMA Extended register (1.21), indicating if 2.5GBASET and
+ * 5GBASET are supported.
+ */
+int genphy_c45_read_abilities(struct phy_device *phydev)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
+	int val;
+
+	__set_bit(ETHTOOL_LINK_MODE_Pause_BIT, supported);
+	__set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, supported);
+
+	if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
+		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
+		if (val < 0)
+			return val;
+
+		if (val & MDIO_AN_STAT1_ABLE)
+			__set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, supported);
+	}
+
+	val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT2);
+	if (val < 0)
+		return val;
+
+	/* Ethtool does not support the WAN mode bits */
+	if (val & (MDIO_PMA_STAT2_10GBSR | MDIO_PMA_STAT2_10GBLR |
+		   MDIO_PMA_STAT2_10GBER | MDIO_PMA_STAT2_10GBLX4 |
+		   MDIO_PMA_STAT2_10GBSW | MDIO_PMA_STAT2_10GBLW |
+		   MDIO_PMA_STAT2_10GBEW))
+		__set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, supported);
+	if (val & MDIO_PMA_STAT2_10GBSR)
+		__set_bit(ETHTOOL_LINK_MODE_10000baseSR_Full_BIT, supported);
+	if (val & MDIO_PMA_STAT2_10GBLR)
+		__set_bit(ETHTOOL_LINK_MODE_10000baseLR_Full_BIT, supported);
+	if (val & MDIO_PMA_STAT2_10GBER)
+		__set_bit(ETHTOOL_LINK_MODE_10000baseER_Full_BIT, supported);
+
+	if (val & MDIO_PMA_STAT2_EXTABLE) {
+		val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE);
+		if (val < 0)
+			return val;
+
+		if (val & (MDIO_PMA_EXTABLE_10GBT | MDIO_PMA_EXTABLE_1000BT |
+			   MDIO_PMA_EXTABLE_100BTX | MDIO_PMA_EXTABLE_10BT))
+			__set_bit(ETHTOOL_LINK_MODE_TP_BIT, supported);
+		if (val & MDIO_PMA_EXTABLE_10GBLRM)
+			__set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, supported);
+		if (val & (MDIO_PMA_EXTABLE_10GBKX4 | MDIO_PMA_EXTABLE_10GBKR |
+			   MDIO_PMA_EXTABLE_1000BKX))
+			__set_bit(ETHTOOL_LINK_MODE_Backplane_BIT, supported);
+		if (val & MDIO_PMA_EXTABLE_10GBLRM)
+			__set_bit(ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT,
+				  supported);
+		if (val & MDIO_PMA_EXTABLE_10GBT)
+			__set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+				  supported);
+		if (val & MDIO_PMA_EXTABLE_10GBKX4)
+			__set_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT,
+				  supported);
+		if (val & MDIO_PMA_EXTABLE_10GBKR)
+			__set_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
+				  supported);
+		if (val & MDIO_PMA_EXTABLE_1000BT)
+			__set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+				  supported);
+		if (val & MDIO_PMA_EXTABLE_1000BKX)
+			__set_bit(ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
+				  supported);
+		if (val & MDIO_PMA_EXTABLE_100BTX) {
+			__set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+				  supported);
+			__set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
+				  supported);
+		}
+		if (val & MDIO_PMA_EXTABLE_10BT) {
+			__set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
+				  supported);
+			__set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
+				  supported);
+		}
+	}
+
+	linkmode_copy(phydev->supported, supported);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(genphy_c45_read_abilities);
+
 /* The gen10g_* functions are the old Clause 45 stub */
 
 int gen10g_config_aneg(struct phy_device *phydev)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index f1c19bf8c658..a639c3c2d3af 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -1017,6 +1017,7 @@ int genphy_c45_read_pma(struct phy_device *phydev);
 int genphy_c45_pma_setup_forced(struct phy_device *phydev);
 int genphy_c45_an_disable_aneg(struct phy_device *phydev);
 int genphy_c45_read_mdix(struct phy_device *phydev);
+int genphy_c45_read_abilities(struct phy_device *phydev);
 
 /* The gen10g_* functions are the old Clause 45 stub */
 int gen10g_config_aneg(struct phy_device *phydev);

-- 
2.19.2

On Fri, Jan 18, 2019 at 04:23:49PM +0100, Maxime Chevallier wrote:

This looks wrong to me, it tramples over the work that phy_probe()
did, specifically with regard to the pause mode bits.  Please see
the comments in that function about the pause mode bits.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

On Fri, Jan 18, 2019 at 04:23:46PM +0100, Maxime Chevallier wrote:

Marvell 10G PHY driver has a generic way of initializing the supported
link modes by reading the PHY's C45 PMA abilities. This can be made
generic, since these registers are part of the 802.3 specifications.

This commit extracts the config_init link_mode initialization code from
marvell10g and uses it to introduce the genphy_c45_read_abilities
function.

Hi Maxime

I think the idea is good. I just have a few English language
issues/questions.

Capabilities is more often used than abilities. I just wondered if
abilities is taken from the 802.3 specifications?

Also, i wonder if we should include _pma_ in the name? At some point
we might want to do something similar for other sublayers.

completed is not the right word. 'is build using the values in the' ?

+ * PMA Extended Abilities (1.11) register, indicating 1000BASET an 10G related
+ * modes. If bit 1.11.14 is set, then the list is also completed with the modes

list is also extended with the modes

Thanks
	Andrew

On Fri, Jan 18, 2019 at 04:23:50PM +0100, Maxime Chevallier wrote:

As per 802.3bz, if bit 14 of (1.11) "PMA Extended Abilities" indicates
whether or not we should read register (1.21) "2.52/5G PMA Extended
Abilities", which contains information on the support of 2.5GBASET and
5GBASET.

After testing on several variants of PHYS of this family, it appears
that bit 14 in (1.11) isn't always set when it should be.

PHYs 88X3310 (on MacchiatoBin) and 88E2010 do support 2.5G and 5GBASET,
but don't have 1.11.14 set. Their register 1.21 is filled with the
correct values, indicating 2.5G and 5G support.

PHYs 88X2110 do have their 1.11.14 bit set, as it should.

Hi Maxime

Is there anything about this in any Errata?

We potentially have an issue if Marvell have any PHYs in this family
which don't support 2.5G/5G. Maybe this workaround needs to check the
IDs and only enable it on device we know are broken.

    Andrew

We need to look a this. This is something the core should be doing.

   Andrew

Hello Andrew,

On Sun, 20 Jan 2019 20:08:09 +0100
Andrew Lunn [off-list ref] wrote:

On Fri, Jan 18, 2019 at 04:23:50PM +0100, Maxime Chevallier wrote:

quoted

As per 802.3bz, if bit 14 of (1.11) "PMA Extended Abilities" indicates
whether or not we should read register (1.21) "2.52/5G PMA Extended
Abilities", which contains information on the support of 2.5GBASET and
5GBASET.

After testing on several variants of PHYS of this family, it appears
that bit 14 in (1.11) isn't always set when it should be.

PHYs 88X3310 (on MacchiatoBin) and 88E2010 do support 2.5G and 5GBASET,
but don't have 1.11.14 set. Their register 1.21 is filled with the
correct values, indicating 2.5G and 5G support.

PHYs 88X2110 do have their 1.11.14 bit set, as it should.  

Hi Maxime

Is there anything about this in any Errata?

I haven't seen any Errata on that unfortunately.

I also thought about reading (1.4) "PMA/PMD Speed Ability", but the
2.5G and 5G speeds are also reported as not being supported on the
88X3310.

We potentially have an issue if Marvell have any PHYs in this family
which don't support 2.5G/5G. Maybe this workaround needs to check the
IDs and only enable it on device we know are broken.

I agree with you, this might be a better way to handle that issue. For
now, I've only seen that issue on the 3310 and 2010, with PHY IDs
respectively 002b09aa and 002b09ab.

I 'll add a test for ids '002b09aX', hopefully there won't be any PHYs
with these IDs that don't support 2.5/5G.

In that case, there's no need for a separate mv2110_config_init in
patch 7.

Thanks,

Maxime.

   Andrew

-- 
Maxime Chevallier, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

On Mon, Jan 21, 2019 at 11:35:31AM +0100, Maxime Chevallier wrote:

Hello Andrew,

On Sun, 20 Jan 2019 20:08:09 +0100
Andrew Lunn [off-list ref] wrote:

quoted

On Fri, Jan 18, 2019 at 04:23:50PM +0100, Maxime Chevallier wrote:

quoted

As per 802.3bz, if bit 14 of (1.11) "PMA Extended Abilities" indicates
whether or not we should read register (1.21) "2.52/5G PMA Extended
Abilities", which contains information on the support of 2.5GBASET and
5GBASET.

After testing on several variants of PHYS of this family, it appears
that bit 14 in (1.11) isn't always set when it should be.

PHYs 88X3310 (on MacchiatoBin) and 88E2010 do support 2.5G and 5GBASET,
but don't have 1.11.14 set. Their register 1.21 is filled with the
correct values, indicating 2.5G and 5G support.

PHYs 88X2110 do have their 1.11.14 bit set, as it should.  

Hi Maxime

Is there anything about this in any Errata?

I haven't seen any Errata on that unfortunately.

I also thought about reading (1.4) "PMA/PMD Speed Ability", but the
2.5G and 5G speeds are also reported as not being supported on the
88X3310.

It's entirely possible that the 3310 switches to different hardware
blocks for 2.5G and 5G speeds, and reading _just_ the 1.4 register
is not sufficient.

The 88x3310 is multiple PHY devices in one package, with a CPU that
manages the routing between each individual device.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Hello Russell,

On Mon, 21 Jan 2019 10:52:06 +0000
Russell King - ARM Linux admin [off-list ref] wrote:

On Mon, Jan 21, 2019 at 11:35:31AM +0100, Maxime Chevallier wrote:

quoted

Hello Andrew,

On Sun, 20 Jan 2019 20:08:09 +0100
Andrew Lunn [off-list ref] wrote:
  

quoted

On Fri, Jan 18, 2019 at 04:23:50PM +0100, Maxime Chevallier wrote:  

quoted

As per 802.3bz, if bit 14 of (1.11) "PMA Extended Abilities" indicates
whether or not we should read register (1.21) "2.52/5G PMA Extended
Abilities", which contains information on the support of 2.5GBASET and
5GBASET.

After testing on several variants of PHYS of this family, it appears
that bit 14 in (1.11) isn't always set when it should be.

PHYs 88X3310 (on MacchiatoBin) and 88E2010 do support 2.5G and 5GBASET,
but don't have 1.11.14 set. Their register 1.21 is filled with the
correct values, indicating 2.5G and 5G support.

PHYs 88X2110 do have their 1.11.14 bit set, as it should.    

Hi Maxime

Is there anything about this in any Errata?  

I haven't seen any Errata on that unfortunately.

I also thought about reading (1.4) "PMA/PMD Speed Ability", but the
2.5G and 5G speeds are also reported as not being supported on the
88X3310.  

It's entirely possible that the 3310 switches to different hardware
blocks for 2.5G and 5G speeds, and reading _just_ the 1.4 register
is not sufficient.

I agree with you but in that particular case, I think we are reading
from the correct device. The datasheet itself says that we should be
reading 1.4 and 1.11 as we expect, with 2.5G/5G support being set (these
registers are read-only, and the datasheet's values aren't what we
actually read).

The 88x3310 is multiple PHY devices in one package, with a CPU that
manages the routing between each individual device.

I also just checked register 3.4 "PCS Speed Ability", and 3.8 "PCS
Status 2" which also contains informations on 2.5G/5G abilities, but
there's the same issue here.

Maxime

-- 
Maxime Chevallier, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

On Mon, Jan 21, 2019 at 01:29:45PM +0100, Maxime Chevallier wrote:

Hello Russell,

On Mon, 21 Jan 2019 10:52:06 +0000
Russell King - ARM Linux admin [off-list ref] wrote:

quoted

It's entirely possible that the 3310 switches to different hardware
blocks for 2.5G and 5G speeds, and reading _just_ the 1.4 register
is not sufficient.

I agree with you but in that particular case, I think we are reading
from the correct device. The datasheet itself says that we should be
reading 1.4 and 1.11 as we expect, with 2.5G/5G support being set (these
registers are read-only, and the datasheet's values aren't what we
actually read).

No, you missed what I was saying.

The 88x3310 is a hybrid device.  It contains multiple instances of
each individual device at different offsets in each MMD address space.

For example, there isn't just one PCS in MMD 3, there are three:

offset 0x0000: 10GBASE-T PCS
offset 0x1000: 10GBASE-R, 10GBASE-X, 40GBASE-R PCS
offset 0x2000: 1000baseX-FD PCS

Which get used depends on what is active.  For example, on the
Macchiatobin, the PCS at 0x1000 gets used for the SFP port, and
the PCS at 0x0000 gets used for the copper port.  The PCS at
0x2000 doesn't ever seem to become active.

MMD 4 (PHYXS) is very similar, and MMD 7 (AN) has four instances.
Different AN instances get used for 10GBASE-T, 10GBASE-KR, SGMII etc.

Each of these blocks either comes from Marvell or another manufacturer.
The 3310 is a mismash of IPs bolted together, with a bunch of routing
between them.  It is not simply a Clause 45 compliant PHY (it isn't
compliant, because C45 requires certain registers to be reserved,
but the 3310 has incomplete address decoding - visible as repeats in
the address space of each MMD.)

The exception seems to be the PMA/PMD MMD which I've only discovered
a single instance.

quoted

The 88x3310 is multiple PHY devices in one package, with a CPU that
manages the routing between each individual device.

I also just checked register 3.4 "PCS Speed Ability", and 3.8 "PCS
Status 2" which also contains informations on 2.5G/5G abilities, but
there's the same issue here.

What I'm saying is, there is much more to this PHY than just the 802.3
register layout because of there being multiple instances.  It _may_
be that a different set of instances gets used to the ones at offset 0
for any particular set of speeds.

I can't look at this at the moment because I am unable to get access to
the 802.3bz specifications - I try to get them through the IEEE get
program website, and I can get to the 802.3 Ethernet specs, click on
either of the 802.3bz amendment 7 links, and I just get a couple of
spinning blobs in Firefox - the page never finishes loading.  So, I
can't get the register information for NBASE-T and compare it to the
various MMD instances in the 88x3310.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Hello Andrew,

On Sun, 20 Jan 2019 19:51:07 +0100
Andrew Lunn [off-list ref] wrote:

On Fri, Jan 18, 2019 at 04:23:46PM +0100, Maxime Chevallier wrote:

quoted

Marvell 10G PHY driver has a generic way of initializing the supported
link modes by reading the PHY's C45 PMA abilities. This can be made
generic, since these registers are part of the 802.3 specifications.

This commit extracts the config_init link_mode initialization code from
marvell10g and uses it to introduce the genphy_c45_read_abilities
function.  

Hi Maxime

I think the idea is good. I just have a few English language
issues/questions.

Thanks for the review.

Capabilities is more often used than abilities. I just wondered if
abilities is taken from the 802.3 specifications?

Yes, the term 'Ability' is used in the 802.3 specifications, both in
the register names and the descriptions.

Also, i wonder if we should include _pma_ in the name? At some point
we might want to do something similar for other sublayers.

You're right, we do read only from the PMA and I see that we could also
to similar thing for the PCS layer, I'll rename the function.

quoted

+/**
+ * genphy_c45_read_abilities - read supported link modes from PMA
+ * @phydev: target phy_device struct
+ *
+ * Read the supported link modes from the PMA Status 2 (1.8) register. If bit
+ * 1.8.9 is set, the list of supported modes is completed with the values in the  

completed is not the right word. 'is build using the values in the' ?

That's better indeed, I'll correct that sentence.

quoted

+ * PMA Extended Abilities (1.11) register, indicating 1000BASET an 10G related
+ * modes. If bit 1.11.14 is set, then the list is also completed with the modes  

list is also extended with the modes

And that one too.

quoted

+ * in the 2.5G/5G PMA Extended register (1.21), indicating if 2.5GBASET and
+ * 5GBASET are supported.
+ */  

Thanks,

Maxime

Yes, the term 'Ability' is used in the 802.3 specifications, both in
the register names and the descriptions.

 
O.K, please keep with what the standard uses.

     Andrew

Hi Maxime

What Russell is referring to is:

static int phy_probe(struct device *dev)
{
..
        /* Start out supporting everything. Eventually,
         * a controller will attach, and may modify one
         * or both of these values
         */
        linkmode_copy(phydev->supported, phydrv->features);
        of_set_phy_supported(phydev);
        linkmode_copy(phydev->advertising, phydev->supported);

        /* Get the EEE modes we want to prohibit. We will ask
         * the PHY stop advertising these mode later on
         */
        of_set_phy_eee_broken(phydev);

        /* The Pause Frame bits indicate that the PHY can support passing
         * pause frames. During autonegotiation, the PHYs will determine if
         * they should allow pause frames to pass.  The MAC driver should then
         * use that result to determine whether to enable flow control via
         * pause frames.
         *
         * Normally, PHY drivers should not set the Pause bits, and instead
         * allow phylib to do that.  However, there may be some situations
         * (e.g. hardware erratum) where the driver wants to set only one
         * of these bits.
         */
        if (test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydrv->features) ||
            test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydrv->features)) {
                linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT,
                                   phydev->supported);
                linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
                                  phydev->supported);
                if (test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydrv->features))
                        linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
                                         phydev->supported);
                if (test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
                             phydrv->features))
                        linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
                                         phydev->supported);
        } else {
                linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
                                 phydev->supported);
                linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
                                 phydev->supported);
        }

So by doing a copy of supported into advertising, you can stomping
over any restrictions applied via of_set_phy_supported(),
of_set_phy_eee_broken(phydev), and any pause control settings which
might of happened.

What might make sense here is that a PHY driver can replace its
.features member at run time, in its config_init() call. The core then
needs to perform these evaluations. So i'm guessing we need to split
this code out of probe() and move it into phy_init_hw()?

Heiner, you know this code better than anybody. What do you think?

	Andrew

Hello Andrew, Russell,

On Mon, 21 Jan 2019 21:17:15 +0100
Andrew Lunn [off-list ref] wrote:

quoted

@@ -264,8 +265,10 @@ static int mv3310_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	linkmode_and(phydev->advertising, phydev->advertising,
-		     phydev->supported);
+	/* Make sure we advertise all the supported modes, and not just the
+	 * default one specified in the driver's .features.
+	 */
+	linkmode_copy(phydev->advertising, phydev->supported);  

So by doing a copy of supported into advertising, you can stomping
over any restrictions applied via of_set_phy_supported(),
of_set_phy_eee_broken(phydev), and any pause control settings which
might of happened.

Thanks for the explanations, this is indeed clearly not a good solution.

What might make sense here is that a PHY driver can replace its
.features member at run time, in its config_init() call. The core then
needs to perform these evaluations. So i'm guessing we need to split
this code out of probe() and move it into phy_init_hw()?

So the .features won't be read-only anymore ? We could also simply make
a helper that would add a mode to both the supported and advertising
modes list, that would be used in the 'genphy_c45_pma_read_abilities'
and config_init ?

I lack the big picture of the PHY init sequence, there seems to be a
lot of quirks and complex cases that we need to take into account, so
I'll let you decide :)

Heiner, you know this code better than anybody. What do you think?

Andrew

Thanks for the feedback,

Maxime

-- 
Maxime Chevallier, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Hello Russell,

On Mon, 21 Jan 2019 13:00:30 +0000
Russell King - ARM Linux admin [off-list ref] wrote:

On Mon, Jan 21, 2019 at 01:29:45PM +0100, Maxime Chevallier wrote:

quoted

Hello Russell,

On Mon, 21 Jan 2019 10:52:06 +0000
Russell King - ARM Linux admin [off-list ref] wrote:  

quoted

It's entirely possible that the 3310 switches to different hardware
blocks for 2.5G and 5G speeds, and reading _just_ the 1.4 register
is not sufficient.  

I agree with you but in that particular case, I think we are reading
from the correct device. The datasheet itself says that we should be
reading 1.4 and 1.11 as we expect, with 2.5G/5G support being set (these
registers are read-only, and the datasheet's values aren't what we
actually read).  

No, you missed what I was saying.

The 88x3310 is a hybrid device.  It contains multiple instances of
each individual device at different offsets in each MMD address space.

Ah I see, I indeed thought you refered to the MMDs. 

[...]

The exception seems to be the PMA/PMD MMD which I've only discovered
a single instance.

Yes there only seems to be one. There are some other registers in the
1.0xCxxx range, but those who are documented don't help a lot with
determing wether or not these modes are supported.

I wonder if these values are correctly reported in newer PHY firmware
revisions.

I've checked other PCS instances, but it seems the one at 3.0x0xxx is
the one used in 2.5/5GBASET.

I've tested with other PHYs from this family, it looks like they are
derivatives of the 33x0 design, with the addition/removal of internal
IPs. Since the 2110 returns the correct values and has a similar
design, with the PMA returning the correct abilities, I think we are
reading from the correct instance.

Thanks,

Maxime

-- 
Maxime Chevallier, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

On Mon, Jan 28, 2019 at 03:26:21PM +0100, Maxime Chevallier wrote:

Hello Russell,

On Mon, 21 Jan 2019 13:00:30 +0000
Russell King - ARM Linux admin [off-list ref] wrote:

quoted

On Mon, Jan 21, 2019 at 01:29:45PM +0100, Maxime Chevallier wrote:

quoted

Hello Russell,

On Mon, 21 Jan 2019 10:52:06 +0000
Russell King - ARM Linux admin [off-list ref] wrote:  

quoted

It's entirely possible that the 3310 switches to different hardware
blocks for 2.5G and 5G speeds, and reading _just_ the 1.4 register
is not sufficient.  

I agree with you but in that particular case, I think we are reading
from the correct device. The datasheet itself says that we should be
reading 1.4 and 1.11 as we expect, with 2.5G/5G support being set (these
registers are read-only, and the datasheet's values aren't what we
actually read).  

No, you missed what I was saying.

The 88x3310 is a hybrid device.  It contains multiple instances of
each individual device at different offsets in each MMD address space.

Ah I see, I indeed thought you refered to the MMDs. 

[...]

quoted

The exception seems to be the PMA/PMD MMD which I've only discovered
a single instance.

Yes there only seems to be one. There are some other registers in the
1.0xCxxx range, but those who are documented don't help a lot with
determing wether or not these modes are supported.

I wonder if these values are correctly reported in newer PHY firmware
revisions.

I've checked other PCS instances, but it seems the one at 3.0x0xxx is
the one used in 2.5/5GBASET.

In the following, I use "subdevice N" to mean instance "N + 1".  So
the instance at address 0 is the main device and the following instance
is subdevice 1.

Looking at the PCS, none of them have the 2.5G/5G bits set in the 3310
which is rather interesting, except the one at 3.0 has the EEE bits
set in 3.21.  You are quite correct that the first instance is used
for 2.5G copper, but PCS subdevice 2 also changes as a result of
switching down to 2.5G (its transmit fault status clears.)

It looks like PhyXS subdevice 3 is used (which is a SGMII/1000base-X
MAC facing PHY), is switched to fixed speed mode.  It doesn't have
what looks like a sensible advertisement either, so my guess is that
this will need the MAC side to be configured _not_ to expect the
in-band control word in this mode.  In other words, it may be SGMII
or 1000base-X upclocked to 2.5G speeds - which it is doesn't matter
because the difference is in the control word which looks like it's
omitted, or if it isn't, doesn't contain useful information.

Note that mvpp2 is slightly buggy in this area, and I have a patch
set that fixes it up - patches can be found in my "mvpp2" branch,
which I'm waiting for my problem reporter to report back after his
testing.  If you use this patch set, as long as you don't use
in-band mode, it should be fine.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up