[PATCH net v2] net: phy: c45: select BASE-T1 type for the forced speed

[PATCH net v2] net: phy: c45: select BASE-T1 type for the forced speed

[Iustin Stolniceanu]

Commit 25108a834e14 ("net: phy: c45: add support for 1000BASE-T1 forced
setup") made genphy_c45_pma_setup_forced() write the type selection
field (bits 3:0) of the BASE-T1 PMA/PMD control register (1.2100) on
every forced setup: 1000BASE-T1 for SPEED_1000, and 0 for everything
else. 0 selects 100BASE-T1, so forcing a 10BASE-T1L PHY to 10 Mbit/s
now also switches its PMA to 100BASE-T1, a mode it does not implement.

Seen on an ADIN1110 (internal ADIN1100 PHY): after

  ethtool -s ethX autoneg off speed 10 master-slave forced-master

1.2100 reads 0xc000 instead of the 0xc002. Since the field is not
restored by re-enabling autoneg, the PHY keeps the wrong type until
the next forced setup or reset.

Add a helper that returns the type selection value for the forced speed
and the supported link modes: 100BASE-T1, 1000BASE-T1, and 10BASE-T1L or
10BASE-T1S for SPEED_10 depending on the supported 10 Mbit/s modes. Fail
with -EINVAL when no type matches instead of writing 100BASE-T1. This
also applies to forced 2.5G, 5G and 10G on BASE-T1 capable PHYs, for
which there are no link modes yet.

The field is called "type selection" in IEEE 802.3-2022 (45.2.1.214,
Table 45-178). Keep the old STRAP names as aliases.

Tested on EVAL-ADIN1110EBZ: with this change, forcing 10 Mbit/s writes
1.2100 = 0xc002 (10BASE-T1L) instead of the invalid 0xc000.

Fixes: 25108a834e14 ("net: phy: c45: add support for 1000BASE-T1 forced setup")
Signed-off-by: Iustin Stolniceanu <iustin.stolniceanu@analog.com>
---
Changes in v2 (review by Andrew Lunn):
- Name the type selection values after 802.3-2022 (Table 45-178) and
  define all of them, including 2.5GBASE-T1, 5GBASE-T1 and 10GBASE-T1.
  The old STRAP name comes from 6f1c646d88c5 and is kept as an alias
  for userspace.
- Add a helper that returns the type selection for the forced speed and
  the supported link modes, also covering 10BASE-T1S. It returns
  -EINVAL when no type matches instead of writing 100BASE-T1, which
  also applies to forced 2.5G, 5G and 10G on BASE-T1 capable PHYs.
- Resolve the type before the first register write, so a failure leaves
  the PHY untouched (reported by the Sashiko review bot).
- Retitle the patch, as it is no longer specific to 10BASE-T1L.
- Link to v1: https://lore.kernel.org/r/20261104152534.1425241-1-iustin.stolniceanu@analog.com

 drivers/net/phy/phy-c45.c | 46 ++++++++++++++++++++++++++++++++++-----
 include/uapi/linux/mdio.h | 13 +++++++++--
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 870920311f9a..8334678df841 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -104,6 +104,36 @@ int genphy_c45_pma_baset1_setup_master_slave(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(genphy_c45_pma_baset1_setup_master_slave);
 
+/* Type selection value for the BASE-T1 PMA/PMD control register (1.2100,
+ * 802.3-2022 Table 45-178) matching the forced speed. 10 Mbit/s is ambiguous
+ * between 10BASE-T1L and 10BASE-T1S, so the supported link modes decide.
+ * 2.5G, 5G and 10GBASE-T1 are not handled as there are no link modes for them
+ * yet.
+ */
+static int genphy_c45_baset1_type_sel(struct phy_device *phydev)
+{
+	switch (phydev->speed) {
+	case SPEED_100:
+		return MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_100BT1;
+	case SPEED_1000:
+		return MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_1000BT1;
+	case SPEED_10:
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT,
+				      phydev->supported))
+			return MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_10BT1L;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT1S_Full_BIT,
+				      phydev->supported) ||
+		    linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT1S_Half_BIT,
+				      phydev->supported) ||
+		    linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT,
+				      phydev->supported))
+			return MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_10BT1S;
+		return -EINVAL;
+	default:
+		return -EINVAL;
+	}
+}
+
 /**
  * genphy_c45_pma_setup_forced - configures a forced speed
  * @phydev: target phy_device struct
@@ -166,6 +196,16 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
 		return -EINVAL;
 	}
 
+	/* Resolve the type before the first write, so a speed without a
+	 * matching type leaves the PHY untouched.
+	 */
+	bt1_ctrl = 0;
+	if (genphy_c45_baset1_able(phydev)) {
+		bt1_ctrl = genphy_c45_baset1_type_sel(phydev);
+		if (bt1_ctrl < 0)
+			return bt1_ctrl;
+	}
+
 	ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, ctrl1);
 	if (ret < 0)
 		return ret;
@@ -179,12 +219,8 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
 		if (ret < 0)
 			return ret;
 
-		bt1_ctrl = 0;
-		if (phydev->speed == SPEED_1000)
-			bt1_ctrl = MDIO_PMA_PMD_BT1_CTRL_STRAP_B1000;
-
 		ret = phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_PMD_BT1_CTRL,
-				     MDIO_PMA_PMD_BT1_CTRL_STRAP, bt1_ctrl);
+				     MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL, bt1_ctrl);
 		if (ret < 0)
 			return ret;
 	}
diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index 06f4bc3c20c7..2806f60ed105 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -417,8 +417,17 @@
 #define MDIO_AN_10BT1_AN_STAT_LPA_EEE_T1L	0x4000 /* 10BASE-T1L LP EEE ability advertisement */
 
 /* BASE-T1 PMA/PMD control register */
-#define MDIO_PMA_PMD_BT1_CTRL_STRAP		0x000F /* Type selection (Strap) */
-#define MDIO_PMA_PMD_BT1_CTRL_STRAP_B1000	0x0001 /* Select 1000BASE-T1 */
+#define MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL		0x000F /* Type selection */
+#define MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_100BT1	0x0000 /* Select 100BASE-T1 */
+#define MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_1000BT1	0x0001 /* Select 1000BASE-T1 */
+#define MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_10BT1L	0x0002 /* Select 10BASE-T1L */
+#define MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_10BT1S	0x0003 /* Select 10BASE-T1S */
+#define MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_2P5GBT1	0x0004 /* Select 2.5GBASE-T1 */
+#define MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_5GBT1	0x0005 /* Select 5GBASE-T1 */
+#define MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_10GBT1	0x0006 /* Select 10GBASE-T1 */
+/* Legacy names, kept for userspace */
+#define MDIO_PMA_PMD_BT1_CTRL_STRAP		MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL
+#define MDIO_PMA_PMD_BT1_CTRL_STRAP_B1000	MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_1000BT1
 #define MDIO_PMA_PMD_BT1_CTRL_CFG_MST		0x4000 /* MASTER-SLAVE config value */
 
 /* 1000BASE-T1 PCS control register */

base-commit: 241dec6f9ab74d8c3ef246764f3611666b96b9e1
-- 
2.43.0

Re: [PATCH net v2] net: phy: c45: select BASE-T1 type for the forced speed

[Andrew Lunn]

> +/* Type selection value for the BASE-T1 PMA/PMD control register (1.2100,
> + * 802.3-2022 Table 45-178) matching the forced speed. 10 Mbit/s is ambiguous
> + * between 10BASE-T1L and 10BASE-T1S, so the supported link modes decide.
> + * 2.5G, 5G and 10GBASE-T1 are not handled as there are no link modes for them
> + * yet.
> + */
> +static int genphy_c45_baset1_type_sel(struct phy_device *phydev)
> +{
> +	switch (phydev->speed) {
> +	case SPEED_100:
> +		return MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_100BT1;
> +	case SPEED_1000:
> +		return MDIO_PMA_PMD_BT1_CTRL_TYPE_SEL_1000BT1;
> +	case SPEED_10:
> +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT,

Overall, this looks good. A nitpick, please could you sort these by
speed. I can see why you put them in this order, 802.3 lists the bits
in this order. But many kernel developers don't seem to read 802.3
that closely.

With this changed, please add: Reviewed-by: Andrew Lunn
<andrew@lunn.ch>

    Andrew

---
pw-bot: cr