[PATCH net-next 0/4] Support loopback mode speed selection

[PATCH net-next 0/4] Support loopback mode speed selection

[Gerhard Engleder]

Previously to commit 6ff3cddc365b
("net: phylib: do not disable autoneg for fixed speeds >= 1G") it was
possible to select the speed of the loopback mode by configuring a fixed
speed before enabling the loopback mode. Now autoneg is always enabled
for >= 1G and a fixed speed of >= 1G requires successful autoneg. Thus,
the speed of the loopback mode depends on the link partner for >= 1G.
There is no technical reason to depend on the link partner for loopback
mode. With this behavior the loopback mode is less useful for testing.

Allow PHYs to support optional speed selection for the loopback mode.
This support is implemented for the generic loopback support and for PHY
drivers, which obviously support speed selection for loopback mode.
Additionally, loopback support according to the data sheet is added to
the KSZ9031 PHY.

Use this loopback speed selection in the tsnep driver to select the
loopback mode speed depending the previously active speed. User space
tests with 100 Mbps and 1 Gbps loopback are possible again.

Gerhard Engleder (4):
  net: phy: Allow loopback speed selection for PHY drivers
  net: phy: Support speed selection for PHY loopback
  net: phy: micrel: Add loopback support
  tsnep: Select speed for loopback

 drivers/net/ethernet/engleder/tsnep_main.c    | 12 +++++++-
 .../net/ethernet/hisilicon/hns/hns_ethtool.c  |  4 +--
 .../hisilicon/hns3/hns3pf/hclge_main.c        |  4 +--
 .../hisilicon/hns3/hns3pf/hclge_mdio.c        |  2 +-
 .../stmicro/stmmac/stmmac_selftests.c         |  8 +++---
 drivers/net/phy/adin1100.c                    |  5 +++-
 drivers/net/phy/dp83867.c                     |  5 +++-
 drivers/net/phy/marvell.c                     |  8 +++++-
 drivers/net/phy/micrel.c                      | 28 +++++++++++++++++++
 drivers/net/phy/mxl-gpy.c                     | 11 +++++---
 drivers/net/phy/phy-c45.c                     |  5 +++-
 drivers/net/phy/phy_device.c                  | 14 +++++++---
 drivers/net/phy/xilinx_gmii2rgmii.c           |  7 +++--
 include/linux/phy.h                           | 18 ++++++++----
 net/core/selftests.c                          |  4 +--
 15 files changed, 103 insertions(+), 32 deletions(-)

-- 
2.39.2

[PATCH net-next 1/4] net: phy: Allow loopback speed selection for PHY drivers

[Gerhard Engleder]

PHY drivers support loopback mode, but it is not possible to select the
speed of the loopback mode. The speed is chosen by the set_loopback()
operation of the PHY driver. Same is valid for genphy_loopback().

There are PHYs that support loopback with different speeds. Extend
set_loopback() to make loopback speed selection possible.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/phy/adin1100.c          |  5 ++++-
 drivers/net/phy/dp83867.c           |  5 ++++-
 drivers/net/phy/marvell.c           |  8 +++++++-
 drivers/net/phy/mxl-gpy.c           | 11 +++++++----
 drivers/net/phy/phy-c45.c           |  5 ++++-
 drivers/net/phy/phy_device.c        | 12 +++++++++---
 drivers/net/phy/xilinx_gmii2rgmii.c |  7 ++++---
 include/linux/phy.h                 | 16 ++++++++++++----
 8 files changed, 51 insertions(+), 18 deletions(-)

diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c
index 85f910e2d4fb..dd8cce925668 100644
--- a/drivers/net/phy/adin1100.c
+++ b/drivers/net/phy/adin1100.c
@@ -215,8 +215,11 @@ static int adin_resume(struct phy_device *phydev)
 	return adin_set_powerdown_mode(phydev, false);
 }
 
-static int adin_set_loopback(struct phy_device *phydev, bool enable)
+static int adin_set_loopback(struct phy_device *phydev, bool enable, int speed)
 {
+	if (enable && speed)
+		return -EOPNOTSUPP;
+
 	if (enable)
 		return phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_10T1L_CTRL,
 					BMCR_LOOPBACK);
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 4120385c5a79..b10ad482d566 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -1009,8 +1009,11 @@ static void dp83867_link_change_notify(struct phy_device *phydev)
 	}
 }
 
-static int dp83867_loopback(struct phy_device *phydev, bool enable)
+static int dp83867_loopback(struct phy_device *phydev, bool enable, int speed)
 {
+	if (enable && speed)
+		return -EOPNOTSUPP;
+
 	return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
 			  enable ? BMCR_LOOPBACK : 0);
 }
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 28aec37acd2c..c70c5c23b339 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -2095,13 +2095,19 @@ static void marvell_get_stats_simple(struct phy_device *phydev,
 		data[i] = marvell_get_stat_simple(phydev, i);
 }
 
-static int m88e1510_loopback(struct phy_device *phydev, bool enable)
+static int m88e1510_loopback(struct phy_device *phydev, bool enable, int speed)
 {
 	int err;
 
 	if (enable) {
 		u16 bmcr_ctl, mscr2_ctl = 0;
 
+		if (speed == SPEED_10 || speed == SPEED_100 ||
+		    speed == SPEED_1000)
+			phydev->speed = speed;
+		else if (speed)
+			return -EINVAL;
+
 		bmcr_ctl = mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
 
 		err = phy_write(phydev, MII_BMCR, bmcr_ctl);
diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index db3c1f72b407..9b863b18a043 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -813,7 +813,7 @@ static void gpy_get_wol(struct phy_device *phydev,
 	wol->wolopts = priv->wolopts;
 }
 
-static int gpy_loopback(struct phy_device *phydev, bool enable)
+static int gpy_loopback(struct phy_device *phydev, bool enable, int speed)
 {
 	struct gpy_priv *priv = phydev->priv;
 	u16 set = 0;
@@ -822,6 +822,9 @@ static int gpy_loopback(struct phy_device *phydev, bool enable)
 	if (enable) {
 		u64 now = get_jiffies_64();
 
+		if (speed)
+			return -EOPNOTSUPP;
+
 		/* wait until 3 seconds from last disable */
 		if (time_before64(now, priv->lb_dis_to))
 			msleep(jiffies64_to_msecs(priv->lb_dis_to - now));
@@ -845,15 +848,15 @@ static int gpy_loopback(struct phy_device *phydev, bool enable)
 	return 0;
 }
 
-static int gpy115_loopback(struct phy_device *phydev, bool enable)
+static int gpy115_loopback(struct phy_device *phydev, bool enable, int speed)
 {
 	struct gpy_priv *priv = phydev->priv;
 
 	if (enable)
-		return gpy_loopback(phydev, enable);
+		return gpy_loopback(phydev, enable, speed);
 
 	if (priv->fw_minor > 0x76)
-		return gpy_loopback(phydev, 0);
+		return gpy_loopback(phydev, 0, 0);
 
 	return genphy_soft_reset(phydev);
 }
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 5695935fdce9..3399028f0e92 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1230,8 +1230,11 @@ int gen10g_config_aneg(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(gen10g_config_aneg);
 
-int genphy_c45_loopback(struct phy_device *phydev, bool enable)
+int genphy_c45_loopback(struct phy_device *phydev, bool enable, int speed)
 {
+	if (enable && speed)
+		return -EOPNOTSUPP;
+
 	return phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
 			      MDIO_PCS_CTRL1_LOOPBACK,
 			      enable ? MDIO_PCS_CTRL1_LOOPBACK : 0);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 563497a3274c..1c34cb947588 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2112,9 +2112,9 @@ int phy_loopback(struct phy_device *phydev, bool enable)
 	}
 
 	if (phydev->drv->set_loopback)
-		ret = phydev->drv->set_loopback(phydev, enable);
+		ret = phydev->drv->set_loopback(phydev, enable, 0);
 	else
-		ret = genphy_loopback(phydev, enable);
+		ret = genphy_loopback(phydev, enable, 0);
 
 	if (ret)
 		goto out;
@@ -2906,12 +2906,18 @@ int genphy_resume(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_resume);
 
-int genphy_loopback(struct phy_device *phydev, bool enable)
+int genphy_loopback(struct phy_device *phydev, bool enable, int speed)
 {
 	if (enable) {
 		u16 ctl = BMCR_LOOPBACK;
 		int ret, val;
 
+		if (speed == SPEED_10 || speed == SPEED_100 ||
+		    speed == SPEED_1000)
+			phydev->speed = speed;
+		else if (speed)
+			return -EINVAL;
+
 		ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
 
 		phy_modify(phydev, MII_BMCR, ~0, ctl);
diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index 7c51daecf18e..2024d8ef36d9 100644
--- a/drivers/net/phy/xilinx_gmii2rgmii.c
+++ b/drivers/net/phy/xilinx_gmii2rgmii.c
@@ -64,15 +64,16 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
 	return 0;
 }
 
-static int xgmiitorgmii_set_loopback(struct phy_device *phydev, bool enable)
+static int xgmiitorgmii_set_loopback(struct phy_device *phydev, bool enable,
+				     int speed)
 {
 	struct gmii2rgmii *priv = mdiodev_get_drvdata(&phydev->mdio);
 	int err;
 
 	if (priv->phy_drv->set_loopback)
-		err = priv->phy_drv->set_loopback(phydev, enable);
+		err = priv->phy_drv->set_loopback(phydev, enable, speed);
 	else
-		err = genphy_loopback(phydev, enable);
+		err = genphy_loopback(phydev, enable, speed);
 	if (err < 0)
 		return err;
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index bf0eb4e5d35c..83b705cfbf46 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1107,8 +1107,16 @@ struct phy_driver {
 	int (*set_tunable)(struct phy_device *dev,
 			    struct ethtool_tunable *tuna,
 			    const void *data);
-	/** @set_loopback: Set the loopback mood of the PHY */
-	int (*set_loopback)(struct phy_device *dev, bool enable);
+	/**
+	 * @set_loopback: Set the loopback mode of the PHY
+	 * enable selects if the loopback mode is enabled or disabled. If the
+	 * loopback mode is enabled, then the speed of the loopback mode can be
+	 * requested with the speed argument. If the speed argument is zero,
+	 * then any speed can be selected. If the speed argument is > 0, then
+	 * this speed shall be selected for the loopback mode or EOPNOTSUPP
+	 * shall be returned if speed selection is not supported.
+	 */
+	int (*set_loopback)(struct phy_device *dev, bool enable, int speed);
 	/** @get_sqi: Get the signal quality indication */
 	int (*get_sqi)(struct phy_device *dev);
 	/** @get_sqi_max: Get the maximum signal quality indication */
@@ -1895,7 +1903,7 @@ int genphy_read_status(struct phy_device *phydev);
 int genphy_read_master_slave(struct phy_device *phydev);
 int genphy_suspend(struct phy_device *phydev);
 int genphy_resume(struct phy_device *phydev);
-int genphy_loopback(struct phy_device *phydev, bool enable);
+int genphy_loopback(struct phy_device *phydev, bool enable, int speed);
 int genphy_soft_reset(struct phy_device *phydev);
 irqreturn_t genphy_handle_interrupt_no_ack(struct phy_device *phydev);
 
@@ -1937,7 +1945,7 @@ int genphy_c45_pma_baset1_read_master_slave(struct phy_device *phydev);
 int genphy_c45_read_status(struct phy_device *phydev);
 int genphy_c45_baset1_read_status(struct phy_device *phydev);
 int genphy_c45_config_aneg(struct phy_device *phydev);
-int genphy_c45_loopback(struct phy_device *phydev, bool enable);
+int genphy_c45_loopback(struct phy_device *phydev, bool enable, int speed);
 int genphy_c45_pma_resume(struct phy_device *phydev);
 int genphy_c45_pma_suspend(struct phy_device *phydev);
 int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable);
-- 
2.39.2

[PATCH net-next 2/4] net: phy: Support speed selection for PHY loopback

[Gerhard Engleder]

phy_loopback() leaves it to the PHY driver to select the speed of the
loopback mode. Thus, the speed of the loopback mode depends on the PHY
driver in use.

Add support for speed selection to phy_loopback() to enable loopback
with defined speeds.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/engleder/tsnep_main.c              | 2 +-
 drivers/net/ethernet/hisilicon/hns/hns_ethtool.c        | 4 ++--
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 4 ++--
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c | 2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c  | 8 ++++----
 drivers/net/phy/phy_device.c                            | 6 +++---
 include/linux/phy.h                                     | 2 +-
 net/core/selftests.c                                    | 4 ++--
 8 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index e4ce8575ff76..5c501e4f9e3e 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -230,7 +230,7 @@ static int tsnep_phy_loopback(struct tsnep_adapter *adapter, bool enable)
 {
 	int retval;
 
-	retval = phy_loopback(adapter->phydev, enable);
+	retval = phy_loopback(adapter->phydev, enable, 0);
 
 	/* PHY link state change is not signaled if loopback is enabled, it
 	 * would delay a working loopback anyway, let's ensure that loopback
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
index a5bb306b2cf1..4ad8d8da594d 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
@@ -266,9 +266,9 @@ static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en)
 		if (err)
 			goto out;
 
-		err = phy_loopback(phy_dev, true);
+		err = phy_loopback(phy_dev, true, 0);
 	} else {
-		err = phy_loopback(phy_dev, false);
+		err = phy_loopback(phy_dev, false, 0);
 		if (err)
 			goto out;
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index bd86efd92a5a..653cef115b49 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -7859,7 +7859,7 @@ static int hclge_enable_phy_loopback(struct hclge_dev *hdev,
 	if (ret)
 		return ret;
 
-	return phy_loopback(phydev, true);
+	return phy_loopback(phydev, true, 0);
 }
 
 static int hclge_disable_phy_loopback(struct hclge_dev *hdev,
@@ -7867,7 +7867,7 @@ static int hclge_disable_phy_loopback(struct hclge_dev *hdev,
 {
 	int ret;
 
-	ret = phy_loopback(phydev, false);
+	ret = phy_loopback(phydev, false, 0);
 	if (ret)
 		return ret;
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c
index 80079657afeb..9a456ebf9b7c 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c
@@ -258,7 +258,7 @@ void hclge_mac_start_phy(struct hclge_dev *hdev)
 	if (!phydev)
 		return;
 
-	phy_loopback(phydev, false);
+	phy_loopback(phydev, false, 0);
 
 	phy_start(phydev);
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index 3ca1c2a816ff..a01bc394d1ac 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -382,14 +382,14 @@ static int stmmac_test_phy_loopback(struct stmmac_priv *priv)
 	if (!priv->dev->phydev)
 		return -EOPNOTSUPP;
 
-	ret = phy_loopback(priv->dev->phydev, true);
+	ret = phy_loopback(priv->dev->phydev, true, 0);
 	if (ret)
 		return ret;
 
 	attr.dst = priv->dev->dev_addr;
 	ret = __stmmac_test_loopback(priv, &attr);
 
-	phy_loopback(priv->dev->phydev, false);
+	phy_loopback(priv->dev->phydev, false, 0);
 	return ret;
 }
 
@@ -1985,7 +1985,7 @@ void stmmac_selftest_run(struct net_device *dev,
 		case STMMAC_LOOPBACK_PHY:
 			ret = -EOPNOTSUPP;
 			if (dev->phydev)
-				ret = phy_loopback(dev->phydev, true);
+				ret = phy_loopback(dev->phydev, true, 0);
 			if (!ret)
 				break;
 			fallthrough;
@@ -2018,7 +2018,7 @@ void stmmac_selftest_run(struct net_device *dev,
 		case STMMAC_LOOPBACK_PHY:
 			ret = -EOPNOTSUPP;
 			if (dev->phydev)
-				ret = phy_loopback(dev->phydev, false);
+				ret = phy_loopback(dev->phydev, false, 0);
 			if (!ret)
 				break;
 			fallthrough;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1c34cb947588..b3242c59afa2 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2092,7 +2092,7 @@ int phy_resume(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_resume);
 
-int phy_loopback(struct phy_device *phydev, bool enable)
+int phy_loopback(struct phy_device *phydev, bool enable, int speed)
 {
 	int ret = 0;
 
@@ -2112,9 +2112,9 @@ int phy_loopback(struct phy_device *phydev, bool enable)
 	}
 
 	if (phydev->drv->set_loopback)
-		ret = phydev->drv->set_loopback(phydev, enable, 0);
+		ret = phydev->drv->set_loopback(phydev, enable, speed);
 	else
-		ret = genphy_loopback(phydev, enable, 0);
+		ret = genphy_loopback(phydev, enable, speed);
 
 	if (ret)
 		goto out;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 83b705cfbf46..8fe838a1bffb 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1790,7 +1790,7 @@ int phy_init_hw(struct phy_device *phydev);
 int phy_suspend(struct phy_device *phydev);
 int phy_resume(struct phy_device *phydev);
 int __phy_resume(struct phy_device *phydev);
-int phy_loopback(struct phy_device *phydev, bool enable);
+int phy_loopback(struct phy_device *phydev, bool enable, int speed);
 int phy_sfp_connect_phy(void *upstream, struct phy_device *phy);
 void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy);
 void phy_sfp_attach(void *upstream, struct sfp_bus *bus);
diff --git a/net/core/selftests.c b/net/core/selftests.c
index 8f801e6e3b91..e99ae983fca9 100644
--- a/net/core/selftests.c
+++ b/net/core/selftests.c
@@ -299,7 +299,7 @@ static int net_test_phy_loopback_enable(struct net_device *ndev)
 	if (!ndev->phydev)
 		return -EOPNOTSUPP;
 
-	return phy_loopback(ndev->phydev, true);
+	return phy_loopback(ndev->phydev, true, 0);
 }
 
 static int net_test_phy_loopback_disable(struct net_device *ndev)
@@ -307,7 +307,7 @@ static int net_test_phy_loopback_disable(struct net_device *ndev)
 	if (!ndev->phydev)
 		return -EOPNOTSUPP;
 
-	return phy_loopback(ndev->phydev, false);
+	return phy_loopback(ndev->phydev, false, 0);
 }
 
 static int net_test_phy_loopback_udp(struct net_device *ndev)
-- 
2.39.2

[PATCH net-next 3/4] net: phy: micrel: Add loopback support

[Gerhard Engleder]

The KSZ9031 PHYs requires full duplex for loopback mode. Add PHY
specific set_loopback() to ensure this.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/phy/micrel.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 65b0a3115e14..41b92d4e1d17 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1028,6 +1028,33 @@ static int ksz9021_config_init(struct phy_device *phydev)
 #define MII_KSZ9031RN_EDPD		0x23
 #define MII_KSZ9031RN_EDPD_ENABLE	BIT(0)
 
+static int ksz9031_set_loopback(struct phy_device *phydev, bool enable,
+				int speed)
+{
+	u16 ctl = BMCR_LOOPBACK;
+	int ret, val;
+
+	if (!enable)
+		return genphy_loopback(phydev, enable, 0);
+
+	if (speed == SPEED_10 || speed == SPEED_100 || speed == SPEED_1000)
+		phydev->speed = speed;
+	else if (speed)
+		return -EINVAL;
+	phydev->duplex = DUPLEX_FULL;
+
+	ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
+
+	phy_modify(phydev, MII_BMCR, ~0, ctl);
+
+	ret = phy_read_poll_timeout(phydev, MII_BMSR, val, val & BMSR_LSTATUS,
+				    5000, 500000, true);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int ksz9031_of_load_skew_values(struct phy_device *phydev,
 				       const struct device_node *of_node,
 				       u16 reg, size_t field_sz,
@@ -5478,6 +5505,7 @@ static struct phy_driver ksphy_driver[] = {
 	.resume		= kszphy_resume,
 	.cable_test_start	= ksz9x31_cable_test_start,
 	.cable_test_get_status	= ksz9x31_cable_test_get_status,
+	.set_loopback	= ksz9031_set_loopback,
 }, {
 	.phy_id		= PHY_ID_LAN8814,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
-- 
2.39.2

[PATCH net-next 4/4] tsnep: Select speed for loopback

[Gerhard Engleder]

Use 100 Mbps only if the PHY is configured to this speed. Otherwise use
always the maximum speed of 1000 Mbps.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/engleder/tsnep_main.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 5c501e4f9e3e..7a89dc87b7a4 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -229,8 +229,18 @@ static void tsnep_phy_link_status_change(struct net_device *netdev)
 static int tsnep_phy_loopback(struct tsnep_adapter *adapter, bool enable)
 {
 	int retval;
+	int speed;
 
-	retval = phy_loopback(adapter->phydev, enable, 0);
+	if (enable) {
+		if (adapter->phydev->speed == SPEED_100)
+			speed = SPEED_100;
+		else
+			speed = SPEED_1000;
+	} else {
+		speed = 0;
+	}
+
+	retval = phy_loopback(adapter->phydev, enable, speed);
 
 	/* PHY link state change is not signaled if loopback is enabled, it
 	 * would delay a working loopback anyway, let's ensure that loopback
-- 
2.39.2

Re: [PATCH net-next 1/4] net: phy: Allow loopback speed selection for PHY drivers

[Lee Trager]

On 10/28/24 1:38 PM, Gerhard Engleder wrote:
> PHY drivers support loopback mode, but it is not possible to select the
> speed of the loopback mode. The speed is chosen by the set_loopback()
> operation of the PHY driver. Same is valid for genphy_loopback().
>
> There are PHYs that support loopback with different speeds. Extend
> set_loopback() to make loopback speed selection possible.
>
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> ---
>   drivers/net/phy/adin1100.c          |  5 ++++-
>   drivers/net/phy/dp83867.c           |  5 ++++-
>   drivers/net/phy/marvell.c           |  8 +++++++-
>   drivers/net/phy/mxl-gpy.c           | 11 +++++++----
>   drivers/net/phy/phy-c45.c           |  5 ++++-
>   drivers/net/phy/phy_device.c        | 12 +++++++++---
>   drivers/net/phy/xilinx_gmii2rgmii.c |  7 ++++---
>   include/linux/phy.h                 | 16 ++++++++++++----
>   8 files changed, 51 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c
> index 85f910e2d4fb..dd8cce925668 100644
> --- a/drivers/net/phy/adin1100.c
> +++ b/drivers/net/phy/adin1100.c
> @@ -215,8 +215,11 @@ static int adin_resume(struct phy_device *phydev)
>   	return adin_set_powerdown_mode(phydev, false);
>   }
>   
> -static int adin_set_loopback(struct phy_device *phydev, bool enable)
> +static int adin_set_loopback(struct phy_device *phydev, bool enable, int speed)
>   {
> +	if (enable && speed)
> +		return -EOPNOTSUPP;
> +
>   	if (enable)
>   		return phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_10T1L_CTRL,
>   					BMCR_LOOPBACK);
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 4120385c5a79..b10ad482d566 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -1009,8 +1009,11 @@ static void dp83867_link_change_notify(struct phy_device *phydev)
>   	}
>   }
>   
> -static int dp83867_loopback(struct phy_device *phydev, bool enable)
> +static int dp83867_loopback(struct phy_device *phydev, bool enable, int speed)
>   {
> +	if (enable && speed)
> +		return -EOPNOTSUPP;
> +
>   	return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
>   			  enable ? BMCR_LOOPBACK : 0);
>   }
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 28aec37acd2c..c70c5c23b339 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -2095,13 +2095,19 @@ static void marvell_get_stats_simple(struct phy_device *phydev,
>   		data[i] = marvell_get_stat_simple(phydev, i);
>   }
>   
> -static int m88e1510_loopback(struct phy_device *phydev, bool enable)
> +static int m88e1510_loopback(struct phy_device *phydev, bool enable, int speed)
>   {
>   	int err;
>   
>   	if (enable) {
>   		u16 bmcr_ctl, mscr2_ctl = 0;
>   
> +		if (speed == SPEED_10 || speed == SPEED_100 ||
> +		    speed == SPEED_1000)
> +			phydev->speed = speed;
> +		else if (speed)
> +			return -EINVAL;
> +
>   		bmcr_ctl = mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
>   
>   		err = phy_write(phydev, MII_BMCR, bmcr_ctl);
> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> index db3c1f72b407..9b863b18a043 100644
> --- a/drivers/net/phy/mxl-gpy.c
> +++ b/drivers/net/phy/mxl-gpy.c
> @@ -813,7 +813,7 @@ static void gpy_get_wol(struct phy_device *phydev,
>   	wol->wolopts = priv->wolopts;
>   }
>   
> -static int gpy_loopback(struct phy_device *phydev, bool enable)
> +static int gpy_loopback(struct phy_device *phydev, bool enable, int speed)
>   {
>   	struct gpy_priv *priv = phydev->priv;
>   	u16 set = 0;
> @@ -822,6 +822,9 @@ static int gpy_loopback(struct phy_device *phydev, bool enable)
>   	if (enable) {
>   		u64 now = get_jiffies_64();
>   
> +		if (speed)
> +			return -EOPNOTSUPP;
> +
>   		/* wait until 3 seconds from last disable */
>   		if (time_before64(now, priv->lb_dis_to))
>   			msleep(jiffies64_to_msecs(priv->lb_dis_to - now));
> @@ -845,15 +848,15 @@ static int gpy_loopback(struct phy_device *phydev, bool enable)
>   	return 0;
>   }
>   
> -static int gpy115_loopback(struct phy_device *phydev, bool enable)
> +static int gpy115_loopback(struct phy_device *phydev, bool enable, int speed)
>   {
>   	struct gpy_priv *priv = phydev->priv;
>   
>   	if (enable)
> -		return gpy_loopback(phydev, enable);
> +		return gpy_loopback(phydev, enable, speed);
>   
>   	if (priv->fw_minor > 0x76)
> -		return gpy_loopback(phydev, 0);
> +		return gpy_loopback(phydev, 0, 0);
>   
>   	return genphy_soft_reset(phydev);
>   }
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index 5695935fdce9..3399028f0e92 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -1230,8 +1230,11 @@ int gen10g_config_aneg(struct phy_device *phydev)
>   }
>   EXPORT_SYMBOL_GPL(gen10g_config_aneg);
>   
> -int genphy_c45_loopback(struct phy_device *phydev, bool enable)
> +int genphy_c45_loopback(struct phy_device *phydev, bool enable, int speed)
>   {
> +	if (enable && speed)
> +		return -EOPNOTSUPP;
> +
>   	return phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
>   			      MDIO_PCS_CTRL1_LOOPBACK,
>   			      enable ? MDIO_PCS_CTRL1_LOOPBACK : 0);
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 563497a3274c..1c34cb947588 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2112,9 +2112,9 @@ int phy_loopback(struct phy_device *phydev, bool enable)
>   	}
>   
>   	if (phydev->drv->set_loopback)
> -		ret = phydev->drv->set_loopback(phydev, enable);
> +		ret = phydev->drv->set_loopback(phydev, enable, 0);
>   	else
> -		ret = genphy_loopback(phydev, enable);
> +		ret = genphy_loopback(phydev, enable, 0);
>   
>   	if (ret)
>   		goto out;
> @@ -2906,12 +2906,18 @@ int genphy_resume(struct phy_device *phydev)
>   }
>   EXPORT_SYMBOL(genphy_resume);
>   
> -int genphy_loopback(struct phy_device *phydev, bool enable)
> +int genphy_loopback(struct phy_device *phydev, bool enable, int speed)
>   {
>   	if (enable) {
>   		u16 ctl = BMCR_LOOPBACK;
>   		int ret, val;
>   
> +		if (speed == SPEED_10 || speed == SPEED_100 ||
> +		    speed == SPEED_1000)
> +			phydev->speed = speed;
Why is this limited to 1000? Shouldn't the max loopback speed be limited 
by max hardware speed? We currently have definitions going up to 
SPEED_800000 so some devices should support higher than 1000.
> +		else if (speed)
> +			return -EINVAL;
> +
>   		ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
>   
>   		phy_modify(phydev, MII_BMCR, ~0, ctl);
> diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
> index 7c51daecf18e..2024d8ef36d9 100644
> --- a/drivers/net/phy/xilinx_gmii2rgmii.c
> +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
> @@ -64,15 +64,16 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
>   	return 0;
>   }
>   
> -static int xgmiitorgmii_set_loopback(struct phy_device *phydev, bool enable)
> +static int xgmiitorgmii_set_loopback(struct phy_device *phydev, bool enable,
> +				     int speed)
>   {
>   	struct gmii2rgmii *priv = mdiodev_get_drvdata(&phydev->mdio);
>   	int err;
>   
>   	if (priv->phy_drv->set_loopback)
> -		err = priv->phy_drv->set_loopback(phydev, enable);
> +		err = priv->phy_drv->set_loopback(phydev, enable, speed);
>   	else
> -		err = genphy_loopback(phydev, enable);
> +		err = genphy_loopback(phydev, enable, speed);
>   	if (err < 0)
>   		return err;
>   
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index bf0eb4e5d35c..83b705cfbf46 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1107,8 +1107,16 @@ struct phy_driver {
>   	int (*set_tunable)(struct phy_device *dev,
>   			    struct ethtool_tunable *tuna,
>   			    const void *data);
> -	/** @set_loopback: Set the loopback mood of the PHY */
> -	int (*set_loopback)(struct phy_device *dev, bool enable);
> +	/**
> +	 * @set_loopback: Set the loopback mode of the PHY
> +	 * enable selects if the loopback mode is enabled or disabled. If the
> +	 * loopback mode is enabled, then the speed of the loopback mode can be
> +	 * requested with the speed argument. If the speed argument is zero,
> +	 * then any speed can be selected. If the speed argument is > 0, then
> +	 * this speed shall be selected for the loopback mode or EOPNOTSUPP
> +	 * shall be returned if speed selection is not supported.
> +	 */
> +	int (*set_loopback)(struct phy_device *dev, bool enable, int speed);
>   	/** @get_sqi: Get the signal quality indication */
>   	int (*get_sqi)(struct phy_device *dev);
>   	/** @get_sqi_max: Get the maximum signal quality indication */
> @@ -1895,7 +1903,7 @@ int genphy_read_status(struct phy_device *phydev);
>   int genphy_read_master_slave(struct phy_device *phydev);
>   int genphy_suspend(struct phy_device *phydev);
>   int genphy_resume(struct phy_device *phydev);
> -int genphy_loopback(struct phy_device *phydev, bool enable);
> +int genphy_loopback(struct phy_device *phydev, bool enable, int speed);
>   int genphy_soft_reset(struct phy_device *phydev);
>   irqreturn_t genphy_handle_interrupt_no_ack(struct phy_device *phydev);
>   
> @@ -1937,7 +1945,7 @@ int genphy_c45_pma_baset1_read_master_slave(struct phy_device *phydev);
>   int genphy_c45_read_status(struct phy_device *phydev);
>   int genphy_c45_baset1_read_status(struct phy_device *phydev);
>   int genphy_c45_config_aneg(struct phy_device *phydev);
> -int genphy_c45_loopback(struct phy_device *phydev, bool enable);
> +int genphy_c45_loopback(struct phy_device *phydev, bool enable, int speed);
>   int genphy_c45_pma_resume(struct phy_device *phydev);
>   int genphy_c45_pma_suspend(struct phy_device *phydev);
>   int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable);

  Why is speed defined as an int? It can never be negative, I normally 
see it defined as u32.

Re: [PATCH net-next 1/4] net: phy: Allow loopback speed selection for PHY drivers

[Andrew Lunn]

> -	int (*set_loopback)(struct phy_device *dev, bool enable);
> +	/**
> +	 * @set_loopback: Set the loopback mode of the PHY
> +	 * enable selects if the loopback mode is enabled or disabled. If the
> +	 * loopback mode is enabled, then the speed of the loopback mode can be
> +	 * requested with the speed argument. If the speed argument is zero,
> +	 * then any speed can be selected. If the speed argument is > 0, then
> +	 * this speed shall be selected for the loopback mode or EOPNOTSUPP
> +	 * shall be returned if speed selection is not supported.
> +	 */

I think we need to take a step back here and think about how this
currently works.

The MAC and the PHY probably need to agree on the speed. Does an RGMII
MAC sending at 10Mbps work against a PHY expecting 1000Mbps? The MAC
is repeating the symbols 100 times to fill the channel, so it might?
But does a PHY expecting 100 repeats work when actually passed a
signal instance of a symbol?  What about an SGMII link, 10Mbps one
side, 1G the other? What about 1000BaseX vs 2500BaseX?

I've never thought about how this actually works today. My _guess_
would be, you first either have the link perform auto-neg, or you
force a speed using ethtool. In both cases, the adjust_link callback
of phylib is called, which for phylink will result in the mac_link_up
callback being called with the current link mode, etc. So the PHY and
the MAC know the properties of the link between themselves.

> +	int (*set_loopback)(struct phy_device *dev, bool enable, int speed);

You say:

> +                                    If the speed argument is zero,
> +	 * then any speed can be selected.

How do the PHY and the MAC agree on the speed? Are you assuming the
adjust_link/mac_link_up will be called? Phylink has the hard
assumption the link will go down before coming up at another speed. Is
that guaranteed here?

What happens when you pass a specific speed? How does the MAC get to
know?

I think we need to keep as close as possible to the current
scheme. The problem is >= 1G, where the core will now enable autoneg
even for forced speeds. If there is a link partner, auto-neg should
succeed and adjust_link/mac_link_up will be called, and so the PHY and
MAC will already be in sync when loopback is enabled. If there is no
link partner, auto-neg will fail. In this case, we need set_loopback
to trigger link up so that the PHY and MAC get in sync. And then it is
disabled, the link needs to go down again.

	Andrew

Re: [PATCH net-next 1/4] net: phy: Allow loopback speed selection for PHY drivers

[Gerhard Engleder]

On 29.10.24 03:45, Andrew Lunn wrote:
>> -	int (*set_loopback)(struct phy_device *dev, bool enable);
>> +	/**
>> +	 * @set_loopback: Set the loopback mode of the PHY
>> +	 * enable selects if the loopback mode is enabled or disabled. If the
>> +	 * loopback mode is enabled, then the speed of the loopback mode can be
>> +	 * requested with the speed argument. If the speed argument is zero,
>> +	 * then any speed can be selected. If the speed argument is > 0, then
>> +	 * this speed shall be selected for the loopback mode or EOPNOTSUPP
>> +	 * shall be returned if speed selection is not supported.
>> +	 */
> 
> I think we need to take a step back here and think about how this
> currently works.
> 
> The MAC and the PHY probably need to agree on the speed. Does an RGMII
> MAC sending at 10Mbps work against a PHY expecting 1000Mbps? The MAC
> is repeating the symbols 100 times to fill the channel, so it might?
> But does a PHY expecting 100 repeats work when actually passed a
> signal instance of a symbol?  What about an SGMII link, 10Mbps one
> side, 1G the other? What about 1000BaseX vs 2500BaseX?
> 
> I've never thought about how this actually works today. My _guess_
> would be, you first either have the link perform auto-neg, or you
> force a speed using ethtool. In both cases, the adjust_link callback
> of phylib is called, which for phylink will result in the mac_link_up
> callback being called with the current link mode, etc. So the PHY and
> the MAC know the properties of the link between themselves.
> 
>> +	int (*set_loopback)(struct phy_device *dev, bool enable, int speed);
> 
> You say:
> 
>> +                                    If the speed argument is zero,
>> +	 * then any speed can be selected.
> 
> How do the PHY and the MAC agree on the speed? Are you assuming the
> adjust_link/mac_link_up will be called? Phylink has the hard
> assumption the link will go down before coming up at another speed. Is
> that guaranteed here?

Yes, the PHY and the MAC has to agree on the speed. In my opinion
adjust_link/mac_link_up shall be called like in normal operation without
loopback. The question is when? adjust_link/mac_link_up could be called
after phy_loopback() returns with success or before phy_loopback()
returns. I would prefer that a successful return of phy_loopback()
guarantees that the loopback mode is active and signaled. This would
eliminate the need to wait for the loopback, which would be again
error prone.

It is not guaranteed that the link goes down before its coming up at
another speed. That needs to be fixed. If the link is up and the speed
matches, then the link shall stay up. If the link is up and the speed
does not match, then the link shall first go down and then come up after
the loopback is established. If the link is down, then the link come up
after the loopback is established. Would that be ok? Or should the link
always go down before the loopback is switched on?

> What happens when you pass a specific speed? How does the MAC get to
> know?

I agree that adjust_link/mac_link_up shall still be used to let the MAC
know the speed.

> I think we need to keep as close as possible to the current
> scheme. The problem is >= 1G, where the core will now enable autoneg
> even for forced speeds. If there is a link partner, auto-neg should
> succeed and adjust_link/mac_link_up will be called, and so the PHY and
> MAC will already be in sync when loopback is enabled. If there is no
> link partner, auto-neg will fail. In this case, we need set_loopback
> to trigger link up so that the PHY and MAC get in sync. And then it is
> disabled, the link needs to go down again.

I would like to try it. Would be great to hear your opinion about when
adjust_link/mac_link_up should be called in case of successful
set_loopback() and if the link shall always go down before loopback.

Thank you for your feedback!

Gerhard

Re: [PATCH net-next 1/4] net: phy: Allow loopback speed selection for PHY drivers

[Gerhard Engleder]

On 29.10.24 00:23, Lee Trager wrote:
> On 10/28/24 1:38 PM, Gerhard Engleder wrote:
>> -int genphy_loopback(struct phy_device *phydev, bool enable)
>> +int genphy_loopback(struct phy_device *phydev, bool enable, int speed)
>>   {
>>       if (enable) {
>>           u16 ctl = BMCR_LOOPBACK;
>>           int ret, val;
>> +        if (speed == SPEED_10 || speed == SPEED_100 ||
>> +            speed == SPEED_1000)
>> +            phydev->speed = speed;
> Why is this limited to 1000? Shouldn't the max loopback speed be limited 
> by max hardware speed? We currently have definitions going up to 
> SPEED_800000 so some devices should support higher than 1000.

This generic loopback implementation only supports those three speeds
currently. If there is the need for higher speed, then there should be
PHY specific implementations in the PHY drivers.

>   Why is speed defined as an int? It can never be negative, I normally 
> see it defined as u32.

speed is defined within phy_device also as int. I aligned the data type
to this structure.

Gerhard

Re: [PATCH net-next 1/4] net: phy: Allow loopback speed selection for PHY drivers

[Andrew Lunn]

On Tue, Oct 29, 2024 at 06:58:12AM +0100, Gerhard Engleder wrote:
> On 29.10.24 00:23, Lee Trager wrote:
> > On 10/28/24 1:38 PM, Gerhard Engleder wrote:
> > > -int genphy_loopback(struct phy_device *phydev, bool enable)
> > > +int genphy_loopback(struct phy_device *phydev, bool enable, int speed)
> > >   {
> > >       if (enable) {
> > >           u16 ctl = BMCR_LOOPBACK;
> > >           int ret, val;
> > > +        if (speed == SPEED_10 || speed == SPEED_100 ||
> > > +            speed == SPEED_1000)
> > > +            phydev->speed = speed;
> > Why is this limited to 1000? Shouldn't the max loopback speed be limited
> > by max hardware speed? We currently have definitions going up to
> > SPEED_800000 so some devices should support higher than 1000.
> 
> This generic loopback implementation only supports those three speeds
> currently. If there is the need for higher speed, then there should be
> PHY specific implementations in the PHY drivers.
> 
> >   Why is speed defined as an int? It can never be negative, I normally
> > see it defined as u32.

https://elixir.bootlin.com/linux/v6.11.5/source/include/uapi/linux/ethtool.h#L2172

#define SPEED_UNKNOWN		-1

It cannot be unsigned.

	Andrew

Re: [PATCH net-next 1/4] net: phy: Allow loopback speed selection for PHY drivers

[Lee Trager]

Thanks for the explanations, both make sense.

Lee

On 10/29/24 6:02 AM, Andrew Lunn wrote:
> On Tue, Oct 29, 2024 at 06:58:12AM +0100, Gerhard Engleder wrote:
>> On 29.10.24 00:23, Lee Trager wrote:
>>> On 10/28/24 1:38 PM, Gerhard Engleder wrote:
>>>> -int genphy_loopback(struct phy_device *phydev, bool enable)
>>>> +int genphy_loopback(struct phy_device *phydev, bool enable, int speed)
>>>>    {
>>>>        if (enable) {
>>>>            u16 ctl = BMCR_LOOPBACK;
>>>>            int ret, val;
>>>> +        if (speed == SPEED_10 || speed == SPEED_100 ||
>>>> +            speed == SPEED_1000)
>>>> +            phydev->speed = speed;
>>> Why is this limited to 1000? Shouldn't the max loopback speed be limited
>>> by max hardware speed? We currently have definitions going up to
>>> SPEED_800000 so some devices should support higher than 1000.
>> This generic loopback implementation only supports those three speeds
>> currently. If there is the need for higher speed, then there should be
>> PHY specific implementations in the PHY drivers.
>>
>>>    Why is speed defined as an int? It can never be negative, I normally
>>> see it defined as u32.
> https://elixir.bootlin.com/linux/v6.11.5/source/include/uapi/linux/ethtool.h#L2172
>
> #define SPEED_UNKNOWN		-1
>
> It cannot be unsigned.
>
> 	Andrew
>