[PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Xu Liang]

Add driver to support the Maxlinear GPY115, GPY211, GPY212, GPY215,
GPY241, GPY245 PHYs.

Signed-off-by: Xu Liang <lxu@maxlinear.com>
---
v2 changes:
 Fix format warning from checkpath and some comments.
 Use smaller PHY ID mask.
 Split FWV register mask.
 Call phy_trigger_machine if necessary when clear interrupt.
v3 changes:
 Replace unnecessary phy_modify_mmd_changed with phy_modify_mmd
 Move firmware version print to probe.

 MAINTAINERS               |   6 +
 drivers/net/phy/Kconfig   |   6 +
 drivers/net/phy/Makefile  |   1 +
 drivers/net/phy/mxl-gpy.c | 552 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 565 insertions(+)
 create mode 100644 drivers/net/phy/mxl-gpy.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 948706174fae..140b19d038fb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11168,6 +11168,12 @@ W:	https://linuxtv.org
 T:	git git://linuxtv.org/media_tree.git
 F:	drivers/media/radio/radio-maxiradio*
 
+MAXLINEAR ETHERNET PHY DRIVER
+M:	Xu Liang <lxu@maxlinear.com>
+L:	netdev@vger.kernel.org
+S:	Supported
+F:	drivers/net/phy/mxl-gpy.c
+
 MCAN MMIO DEVICE DRIVER
 M:	Chandrasekar Ramakrishnan <rcsekar@samsung.com>
 L:	linux-can@vger.kernel.org
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 288bf405ebdb..d02098774d80 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -207,6 +207,12 @@ config MARVELL_88X2222_PHY
 	  Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet
 	  Transceiver.
 
+config MXL_GPHY
+	tristate "Maxlinear PHYs"
+	help
+	  Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
+	  GPY241, GPY245 PHYs.
+
 config MICREL_PHY
 	tristate "Micrel PHYs"
 	help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index bcda7ed2455d..1055fb73ac17 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_MICREL_PHY)	+= micrel.o
 obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
 obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.o
 obj-$(CONFIG_MICROSEMI_PHY)	+= mscc/
+obj-$(CONFIG_MXL_GPHY)		+= mxl-gpy.o
 obj-$(CONFIG_NATIONAL_PHY)	+= national.o
 obj-$(CONFIG_NXP_C45_TJA11XX_PHY)	+= nxp-c45-tja11xx.o
 obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
new file mode 100644
index 000000000000..3e3d3096e858
--- /dev/null
+++ b/drivers/net/phy/mxl-gpy.c
@@ -0,0 +1,552 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* Copyright (C) 2021 Maxlinear Corporation
+ * Copyright (C) 2020 Intel Corporation
+ *
+ * Drivers for Maxlinear Ethernet GPY
+ *
+ */
+
+#include <linux/version.h>
+#include <linux/module.h>
+#include <linux/bitfield.h>
+#include <linux/phy.h>
+#include <linux/netdevice.h>
+
+/* PHY ID */
+#define PHY_ID_GPY		0x67C9DC00
+#define PHY_ID_MASK		GENMASK(31, 4)
+
+#define PHY_MIISTAT		0x18	/* MII state */
+#define PHY_IMASK		0x19	/* interrupt mask */
+#define PHY_ISTAT		0x1A	/* interrupt status */
+#define PHY_FWV			0x1E	/* firmware version */
+
+#define PHY_MIISTAT_SPD_MASK	GENMASK(2, 0)
+#define PHY_MIISTAT_DPX		BIT(3)
+#define PHY_MIISTAT_LS		BIT(10)
+
+#define PHY_MIISTAT_SPD_10	0
+#define PHY_MIISTAT_SPD_100	1
+#define PHY_MIISTAT_SPD_1000	2
+#define PHY_MIISTAT_SPD_2500	4
+
+#define PHY_IMASK_WOL		BIT(15)	/* Wake-on-LAN */
+#define PHY_IMASK_ANC		BIT(10)	/* Auto-Neg complete */
+#define PHY_IMASK_ADSC		BIT(5)	/* Link auto-downspeed detect */
+#define PHY_IMASK_DXMC		BIT(2)	/* Duplex mode change */
+#define PHY_IMASK_LSPC		BIT(1)	/* Link speed change */
+#define PHY_IMASK_LSTC		BIT(0)	/* Link state change */
+#define PHY_IMASK_MASK		(PHY_IMASK_LSTC | \
+				 PHY_IMASK_LSPC | \
+				 PHY_IMASK_DXMC | \
+				 PHY_IMASK_ADSC | \
+				 PHY_IMASK_ANC)
+
+#define PHY_FWV_REL_MASK	BIT(15)
+#define PHY_FWV_TYPE_MASK	GENMASK(11, 8)
+#define PHY_FWV_MINOR_MASK	GENMASK(7, 0)
+
+/* ANEG dev */
+#define ANEG_MGBT_AN_CTRL	0x20
+#define ANEG_MGBT_AN_STAT	0x21
+#define CTRL_AB_2G5BT_BIT	BIT(7)
+#define CTRL_AB_FR_2G5BT	BIT(5)
+#define STAT_AB_2G5BT_BIT	BIT(5)
+#define STAT_AB_FR_2G5BT	BIT(3)
+
+/* SGMII */
+#define VSPEC1_SGMII_CTRL	0x08
+#define VSPEC1_SGMII_CTRL_ANEN	BIT(12)		/* Aneg enable */
+#define VSPEC1_SGMII_CTRL_ANRS	BIT(9)		/* Restart Aneg */
+#define VSPEC1_SGMII_ANEN_ANRS	(VSPEC1_SGMII_CTRL_ANEN | \
+				 VSPEC1_SGMII_CTRL_ANRS)
+
+/* WoL */
+#define VPSPEC2_WOL_CTL		0x0E06
+#define VPSPEC2_WOL_AD01	0x0E08
+#define VPSPEC2_WOL_AD23	0x0E09
+#define VPSPEC2_WOL_AD45	0x0E0A
+#define WOL_EN			BIT(0)
+
+static const struct {
+	int type;
+	int minor;
+} ver_need_sgmii_reaneg[] = {
+	{7, 0x6D},
+	{8, 0x6D},
+	{9, 0x73},
+};
+
+static int gpy_read_abilities(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_read_abilities(phydev);
+	if (ret < 0)
+		return ret;
+
+	/* Detect 2.5G/5G support. */
+	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT2);
+	if (ret < 0)
+		return ret;
+	if (!(ret & MDIO_PMA_STAT2_EXTABLE))
+		return 0;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE);
+	if (ret < 0)
+		return ret;
+	if (!(ret & MDIO_PMA_EXTABLE_NBT))
+		return 0;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);
+	if (ret < 0)
+		return ret;
+
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+			 phydev->supported,
+			 ret & MDIO_PMA_NG_EXTABLE_2_5GBT);
+
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+			 phydev->supported,
+			 ret & MDIO_PMA_NG_EXTABLE_5GBT);
+
+	return 0;
+}
+
+static int gpy_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Mask all interrupts */
+	ret = phy_write(phydev, PHY_IMASK, 0);
+	if (ret)
+		return ret;
+
+	/* Clear all pending interrupts */
+	ret = phy_read(phydev, PHY_ISTAT);
+	return ret < 0 ? ret : 0;
+}
+
+static int gpy_probe(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Show GPY PHY FW version in dmesg */
+	ret = phy_read(phydev, PHY_FWV);
+	if (ret < 0)
+		return ret;
+
+	phydev_info(phydev, "Firmware Version: 0x%04X (%s)\n", ret,
+		    (ret & PHY_FWV_REL_MASK) ? "release" : "test");
+
+	return 0;
+}
+
+static bool gpy_sgmii_need_reaneg(struct phy_device *phydev)
+{
+	int fw_ver, fw_type, fw_minor;
+	size_t i;
+
+	fw_ver = phy_read(phydev, PHY_FWV);
+	if (fw_ver < 0)
+		return true;
+
+	fw_type = FIELD_GET(PHY_FWV_TYPE_MASK, fw_ver);
+	fw_minor = FIELD_GET(PHY_FWV_MINOR_MASK, fw_ver);
+
+	for (i = 0; i < ARRAY_SIZE(ver_need_sgmii_reaneg); i++) {
+		if (fw_type != ver_need_sgmii_reaneg[i].type)
+			continue;
+		if (fw_minor < ver_need_sgmii_reaneg[i].minor)
+			return true;
+		break;
+	}
+
+	return false;
+}
+
+static bool gpy_2500basex_chk(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read(phydev, PHY_MIISTAT);
+	if (ret < 0) {
+		phydev_err(phydev, "Error: MDIO register access failed: %d\n",
+			   ret);
+		return false;
+	}
+
+	if (!(ret & PHY_MIISTAT_LS) ||
+	    FIELD_GET(PHY_MIISTAT_SPD_MASK, ret) != PHY_MIISTAT_SPD_2500)
+		return false;
+
+	phydev->speed = SPEED_2500;
+	phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
+	phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL,
+		       VSPEC1_SGMII_CTRL_ANEN, 0);
+	return true;
+}
+
+static bool gpy_sgmii_aneg_en(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL);
+	if (ret < 0) {
+		phydev_err(phydev, "Error: MMD register access failed: %d\n",
+			   ret);
+		return true;
+	}
+
+	return (ret & VSPEC1_SGMII_CTRL_ANEN) ? true : false;
+}
+
+static int gpy_config_aneg(struct phy_device *phydev)
+{
+	bool changed = false;
+	u32 adv;
+	int ret;
+
+	if (phydev->autoneg == AUTONEG_DISABLE) {
+		/* Configure half duplex with genphy_setup_forced,
+		 * because genphy_c45_pma_setup_forced does not support.
+		 */
+		return phydev->duplex != DUPLEX_FULL
+			? genphy_setup_forced(phydev)
+			: genphy_c45_pma_setup_forced(phydev);
+	}
+
+	ret = genphy_c45_an_config_aneg(phydev);
+	if (ret < 0)
+		return ret;
+	if (ret > 0)
+		changed = true;
+
+	adv = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising);
+	ret = phy_modify_changed(phydev, MII_CTRL1000,
+				 ADVERTISE_1000FULL | ADVERTISE_1000HALF,
+				 adv);
+	if (ret < 0)
+		return ret;
+	if (ret > 0)
+		changed = true;
+
+	ret = genphy_c45_check_and_restart_aneg(phydev, changed);
+	if (ret < 0)
+		return ret;
+
+	if (phydev->interface == PHY_INTERFACE_MODE_USXGMII ||
+	    phydev->interface == PHY_INTERFACE_MODE_INTERNAL)
+		return 0;
+
+	/* No need to trigger re-ANEG if SGMII link speed is 2.5G
+	 * or SGMII ANEG is disabled.
+	 */
+	if (!gpy_sgmii_need_reaneg(phydev) || gpy_2500basex_chk(phydev) ||
+	    !gpy_sgmii_aneg_en(phydev))
+		return 0;
+
+	/* There is a design constraint in GPY2xx device where SGMII AN is
+	 * only triggered when there is change of speed. If, PHY link
+	 * partner`s speed is still same even after PHY TPI is down and up
+	 * again, SGMII AN is not triggered and hence no new in-band message
+	 * from GPY to MAC side SGMII.
+	 * This could cause an issue during power up, when PHY is up prior to
+	 * MAC. At this condition, once MAC side SGMII is up, MAC side SGMII
+	 * wouldn`t receive new in-band message from GPY with correct link
+	 * status, speed and duplex info.
+	 *
+	 * 1) If PHY is already up and TPI link status is still down (such as
+	 *    hard reboot), TPI link status is polled for 4 seconds before
+	 *    retriggerring SGMII AN.
+	 * 2) If PHY is already up and TPI link status is also up (such as soft
+	 *    reboot), polling of TPI link status is not needed and SGMII AN is
+	 *    immediately retriggered.
+	 * 3) Other conditions such as PHY is down, speed change etc, skip
+	 *    retriggering SGMII AN. Note: in case of speed change, GPY FW will
+	 *    initiate SGMII AN.
+	 */
+
+	if (phydev->state != PHY_UP)
+		return 0;
+
+	ret = phy_read_poll_timeout(phydev, MII_BMSR, ret, ret & BMSR_LSTATUS,
+				    20000, 4000000, false);
+	if (ret == -ETIMEDOUT)
+		return 0;
+	else if (ret < 0)
+		return ret;
+
+	/* Trigger SGMII AN. */
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL,
+			      VSPEC1_SGMII_CTRL_ANRS, VSPEC1_SGMII_CTRL_ANRS);
+}
+
+static void gpy_update_interface(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Interface mode is fixed for USXGMII and integrated PHY */
+	if (phydev->interface == PHY_INTERFACE_MODE_USXGMII ||
+	    phydev->interface == PHY_INTERFACE_MODE_INTERNAL)
+		return;
+
+	/* Automatically switch SERDES interface between SGMII and 2500-BaseX
+	 * according to speed. Disable ANEG in 2500-BaseX mode.
+	 */
+	switch (phydev->speed) {
+	case SPEED_2500:
+		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
+		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL,
+				     VSPEC1_SGMII_CTRL_ANEN, 0);
+		if (ret < 0)
+			phydev_err(phydev,
+				   "Error: Disable of SGMII ANEG failed: %d\n",
+				   ret);
+		break;
+	case SPEED_1000:
+	case SPEED_100:
+	case SPEED_10:
+		phydev->interface = PHY_INTERFACE_MODE_SGMII;
+		if (gpy_sgmii_aneg_en(phydev))
+			break;
+		/* Enable and restart SGMII ANEG for 10/100/1000Mbps link speed
+		 * if ANEG is disabled (in 2500-BaseX mode).
+		 */
+		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL,
+				     VSPEC1_SGMII_ANEN_ANRS,
+				     VSPEC1_SGMII_ANEN_ANRS);
+		if (ret < 0)
+			phydev_err(phydev,
+				   "Error: Enable of SGMII ANEG failed: %d\n",
+				   ret);
+		break;
+	}
+}
+
+static int gpy_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_update_link(phydev);
+	if (ret)
+		return ret;
+
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
+		ret = genphy_c45_read_lpa(phydev);
+		if (ret < 0)
+			return ret;
+
+		/* Read the link partner's 1G advertisement */
+		ret = phy_read(phydev, MII_STAT1000);
+		if (ret < 0)
+			return ret;
+		mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, ret);
+	} else if (phydev->autoneg == AUTONEG_DISABLE) {
+		linkmode_zero(phydev->lp_advertising);
+	}
+
+	ret = phy_read(phydev, PHY_MIISTAT);
+	if (ret < 0)
+		return ret;
+
+	phydev->link = (ret & PHY_MIISTAT_LS) ? 1 : 0;
+	phydev->duplex = (ret & PHY_MIISTAT_DPX) ? DUPLEX_FULL : DUPLEX_HALF;
+	switch (FIELD_GET(PHY_MIISTAT_SPD_MASK, ret)) {
+	case PHY_MIISTAT_SPD_10:
+		phydev->speed = SPEED_10;
+		break;
+	case PHY_MIISTAT_SPD_100:
+		phydev->speed = SPEED_100;
+		break;
+	case PHY_MIISTAT_SPD_1000:
+		phydev->speed = SPEED_1000;
+		break;
+	case PHY_MIISTAT_SPD_2500:
+		phydev->speed = SPEED_2500;
+		break;
+	}
+
+	if (phydev->link)
+		gpy_update_interface(phydev);
+
+	return 0;
+}
+
+static int gpy_config_intr(struct phy_device *phydev)
+{
+	u16 mask = 0;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		mask = PHY_IMASK_MASK;
+
+	return phy_write(phydev, PHY_IMASK, mask);
+}
+
+static irqreturn_t gpy_handle_interrupt(struct phy_device *phydev)
+{
+	int reg;
+
+	reg = phy_read(phydev, PHY_ISTAT);
+	if (reg < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	if (!(reg & PHY_IMASK_MASK))
+		return IRQ_NONE;
+
+	phy_trigger_machine(phydev);
+
+	return IRQ_HANDLED;
+}
+
+static int gpy_set_wol(struct phy_device *phydev,
+		       struct ethtool_wolinfo *wol)
+{
+	struct net_device *attach_dev = phydev->attached_dev;
+	int ret;
+
+	if (wol->wolopts & WAKE_MAGIC) {
+		/* MAC address - Byte0:Byte1:Byte2:Byte3:Byte4:Byte5
+		 * VPSPEC2_WOL_AD45 = Byte0:Byte1
+		 * VPSPEC2_WOL_AD23 = Byte2:Byte3
+		 * VPSPEC2_WOL_AD01 = Byte4:Byte5
+		 */
+		ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
+				       VPSPEC2_WOL_AD45,
+				       ((attach_dev->dev_addr[0] << 8) |
+				       attach_dev->dev_addr[1]));
+		if (ret < 0)
+			return ret;
+
+		ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
+				       VPSPEC2_WOL_AD23,
+				       ((attach_dev->dev_addr[2] << 8) |
+				       attach_dev->dev_addr[3]));
+		if (ret < 0)
+			return ret;
+
+		ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
+				       VPSPEC2_WOL_AD01,
+				       ((attach_dev->dev_addr[4] << 8) |
+				       attach_dev->dev_addr[5]));
+		if (ret < 0)
+			return ret;
+
+		/* Enable the WOL interrupt */
+		ret = phy_write(phydev, PHY_IMASK, PHY_IMASK_WOL);
+		if (ret < 0)
+			return ret;
+
+		/* Enable magic packet matching */
+		ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
+				       VPSPEC2_WOL_CTL,
+				       WOL_EN);
+		if (ret < 0)
+			return ret;
+
+		/* Clear the interrupt status register.
+		 * Only WoL is enabled so clear all.
+		 */
+		ret = phy_read(phydev, PHY_ISTAT);
+		if (ret < 0)
+			return ret;
+	} else {
+		/* Disable magic packet matching */
+		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
+					 VPSPEC2_WOL_CTL,
+					 WOL_EN);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (wol->wolopts & WAKE_PHY) {
+		/* Enable the link state change interrupt */
+		ret = phy_set_bits(phydev, PHY_IMASK, PHY_IMASK_LSTC);
+		if (ret < 0)
+			return ret;
+
+		/* Clear the interrupt status register */
+		ret = phy_read(phydev, PHY_ISTAT);
+		if (ret < 0)
+			return ret;
+
+		if (ret & (PHY_IMASK_MASK & ~PHY_IMASK_LSTC))
+			phy_trigger_machine(phydev);
+
+		return 0;
+	}
+
+	/* Disable the link state change interrupt */
+	return phy_clear_bits(phydev, PHY_IMASK, PHY_IMASK_LSTC);
+}
+
+static void gpy_get_wol(struct phy_device *phydev,
+			struct ethtool_wolinfo *wol)
+{
+	int ret;
+
+	wol->supported = WAKE_MAGIC | WAKE_PHY;
+	wol->wolopts = 0;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, VPSPEC2_WOL_CTL);
+	if (ret & WOL_EN)
+		wol->wolopts |= WAKE_MAGIC;
+
+	ret = phy_read(phydev, PHY_IMASK);
+	if (ret & PHY_IMASK_LSTC)
+		wol->wolopts |= WAKE_PHY;
+}
+
+static int gpy_loopback(struct phy_device *phydev, bool enable)
+{
+	int ret;
+
+	ret = genphy_loopback(phydev, enable);
+	if (!ret) {
+		/* It takes some time for PHY device to switch
+		 * into/out-of loopback mode.
+		 */
+		usleep_range(100, 200);
+	}
+
+	return ret;
+}
+
+static struct phy_driver gpy_drivers[] = {
+	{
+		.phy_id		= PHY_ID_GPY,
+		.phy_id_mask	= PHY_ID_MASK,
+		.name		= "Maxlinear Ethernet GPY",
+		.get_features	= gpy_read_abilities,
+		.config_init	= gpy_config_init,
+		.probe		= gpy_probe,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.config_aneg	= gpy_config_aneg,
+		.aneg_done	= genphy_c45_aneg_done,
+		.read_status	= gpy_read_status,
+		.config_intr	= gpy_config_intr,
+		.handle_interrupt = gpy_handle_interrupt,
+		.set_wol	= gpy_set_wol,
+		.get_wol	= gpy_get_wol,
+		.set_loopback	= gpy_loopback,
+	},
+};
+module_phy_driver(gpy_drivers);
+
+static struct mdio_device_id __maybe_unused gpy_tbl[] = {
+	{PHY_ID_GPY, PHY_ID_MASK},
+	{ }
+};
+MODULE_DEVICE_TABLE(mdio, gpy_tbl);
+
+MODULE_DESCRIPTION("Maxlinear Ethernet GPY Driver");
+MODULE_AUTHOR("Maxlinear Corporation");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Florian Fainelli]

On 6/4/2021 9:12 AM, Xu Liang wrote:
> Add driver to support the Maxlinear GPY115, GPY211, GPY212, GPY215,
> GPY241, GPY245 PHYs.
> 
> Signed-off-by: Xu Liang <lxu@maxlinear.com>
> ---
> v2 changes:
>  Fix format warning from checkpath and some comments.
>  Use smaller PHY ID mask.
>  Split FWV register mask.
>  Call phy_trigger_machine if necessary when clear interrupt.
> v3 changes:
>  Replace unnecessary phy_modify_mmd_changed with phy_modify_mmd
>  Move firmware version print to probe.
> 
>  MAINTAINERS               |   6 +
>  drivers/net/phy/Kconfig   |   6 +
>  drivers/net/phy/Makefile  |   1 +
>  drivers/net/phy/mxl-gpy.c | 552 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 565 insertions(+)
>  create mode 100644 drivers/net/phy/mxl-gpy.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 948706174fae..140b19d038fb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11168,6 +11168,12 @@ W:	https://linuxtv.org
>  T:	git git://linuxtv.org/media_tree.git
>  F:	drivers/media/radio/radio-maxiradio*
>  
> +MAXLINEAR ETHERNET PHY DRIVER
> +M:	Xu Liang <lxu@maxlinear.com>
> +L:	netdev@vger.kernel.org
> +S:	Supported
> +F:	drivers/net/phy/mxl-gpy.c
> +
>  MCAN MMIO DEVICE DRIVER
>  M:	Chandrasekar Ramakrishnan <rcsekar@samsung.com>
>  L:	linux-can@vger.kernel.org
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 288bf405ebdb..d02098774d80 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -207,6 +207,12 @@ config MARVELL_88X2222_PHY
>  	  Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet
>  	  Transceiver.
>  
> +config MXL_GPHY
> +	tristate "Maxlinear PHYs"
> +	help
> +	  Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> +	  GPY241, GPY245 PHYs.
> +
>  config MICREL_PHY
>  	tristate "Micrel PHYs"
>  	help
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index bcda7ed2455d..1055fb73ac17 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_MICREL_PHY)	+= micrel.o
>  obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
>  obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.o
>  obj-$(CONFIG_MICROSEMI_PHY)	+= mscc/
> +obj-$(CONFIG_MXL_GPHY)		+= mxl-gpy.o

Could you spell it out completely: CONFIG_MAXLINEAR_GPHY for consistency
with the other vendor's and also to have some proper namespacing in case
we see a non-Ethernet PHY being submitted for a Maxlinear product in the
future?

>  obj-$(CONFIG_NATIONAL_PHY)	+= national.o
>  obj-$(CONFIG_NXP_C45_TJA11XX_PHY)	+= nxp-c45-tja11xx.o
>  obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> new file mode 100644
> index 000000000000..3e3d3096e858
> --- /dev/null
> +++ b/drivers/net/phy/mxl-gpy.c
> @@ -0,0 +1,552 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Copyright (C) 2021 Maxlinear Corporation
> + * Copyright (C) 2020 Intel Corporation
> + *
> + * Drivers for Maxlinear Ethernet GPY
> + *
> + */
> +
> +#include <linux/version.h>
> +#include <linux/module.h>
> +#include <linux/bitfield.h>
> +#include <linux/phy.h>
> +#include <linux/netdevice.h>
> +
> +/* PHY ID */
> +#define PHY_ID_GPY		0x67C9DC00
> +#define PHY_ID_MASK		GENMASK(31, 4)

Consider initializing your phy_driver with PHY_ID_MATCH_MODEL() which
would take care of populating the mask accordingly.

[snip]

> +
> +static int gpy_read_abilities(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_read_abilities(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Detect 2.5G/5G support. */
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT2);
> +	if (ret < 0)
> +		return ret;
> +	if (!(ret & MDIO_PMA_STAT2_EXTABLE))
> +		return 0;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE);
> +	if (ret < 0)
> +		return ret;
> +	if (!(ret & MDIO_PMA_EXTABLE_NBT))
> +		return 0;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);
> +	if (ret < 0)
> +		return ret;
> +
> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +			 phydev->supported,
> +			 ret & MDIO_PMA_NG_EXTABLE_2_5GBT);
> +
> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> +			 phydev->supported,
> +			 ret & MDIO_PMA_NG_EXTABLE_5GBT);

This does not access vendor specific registers, should not this be part
of the standard genphy_read_abilities() or moved to a helper?

> +
> +	return 0;
> +}
> +
> +static int gpy_config_init(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* Mask all interrupts */
> +	ret = phy_write(phydev, PHY_IMASK, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Clear all pending interrupts */
> +	ret = phy_read(phydev, PHY_ISTAT);
> +	return ret < 0 ? ret : 0;

You can certainly simplify this to:

	return phy_read(phydev, PHY_ISTAT);

[snip]

> +
> +MODULE_DESCRIPTION("Maxlinear Ethernet GPY Driver");
> +MODULE_AUTHOR("Maxlinear Corporation");

The author is not usually a company but an individual working on behalf
of that company.
-- 
Florian

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Liang Xu]

On 5/6/2021 12:24 am, Florian Fainelli wrote:
>
> > +/* PHY ID */
> > +#define PHY_ID_GPY 0x67C9DC00
> > +#define PHY_ID_MASK GENMASK(31, 4)
>
> Consider initializing your phy_driver with PHY_ID_MATCH_MODEL() which
> would take care of populating the mask accordingly.
>
Thanks, I will update.
> > +
> > +static int gpy_read_abilities(struct phy_device *phydev)
> > +{
> > + int ret;
> > +
> > + ret = genphy_read_abilities(phydev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Detect 2.5G/5G support. */
> > + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT2);
> > + if (ret < 0)
> > + return ret;
> > + if (!(ret & MDIO_PMA_STAT2_EXTABLE))
> > + return 0;
> > +
> > + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE);
> > + if (ret < 0)
> > + return ret;
> > + if (!(ret & MDIO_PMA_EXTABLE_NBT))
> > + return 0;
> > +
> > + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);
> > + if (ret < 0)
> > + return ret;
> > +
> > + linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> > + phydev->supported,
> > + ret & MDIO_PMA_NG_EXTABLE_2_5GBT);
> > +
> > + linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> > + phydev->supported,
> > + ret & MDIO_PMA_NG_EXTABLE_5GBT);
>
> This does not access vendor specific registers, should not this be part
> of the standard genphy_read_abilities() or moved to a helper?
>
genphy_read_abilities does not cover 2.5G.

genphy_c45_pma_read_abilities checks C45 ids and this check fail if 
is_c45 is not set.

Mix of C22 and C45 is handled here, as our device support C22 with 
extension of C45.

> > +
> > + return 0;
> > +}
> > +
> > +static int gpy_config_init(struct phy_device *phydev)
> > +{
> > + int ret;
> > +
> > + /* Mask all interrupts */
> > + ret = phy_write(phydev, PHY_IMASK, 0);
> > + if (ret)
> > + return ret;
> > +
> > + /* Clear all pending interrupts */
> > + ret = phy_read(phydev, PHY_ISTAT);
> > + return ret < 0 ? ret : 0;
>
> You can certainly simplify this to:
>
> return phy_read(phydev, PHY_ISTAT);
>
I'm thinking to clearly differentiate negative, 0, and positive.

I had experience that a positive value is treated as error sometimes.


> [snip]
>
> > +
> > +MODULE_DESCRIPTION("Maxlinear Ethernet GPY Driver");
> > +MODULE_AUTHOR("Maxlinear Corporation");
>
> The author is not usually a company but an individual working on behalf
> of that company.
> -- 
> Florian

Ok, I will update.

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Liang Xu]

On 5/6/2021 12:24 am, Florian Fainelli wrote:
>
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 288bf405ebdb..d02098774d80 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -207,6 +207,12 @@ config MARVELL_88X2222_PHY
> > Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet
> > Transceiver.
> >
> > +config MXL_GPHY
> > + tristate "Maxlinear PHYs"
> > + help
> > + Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> > + GPY241, GPY245 PHYs.
> > +
> > config MICREL_PHY
> > tristate "Micrel PHYs"
> > help
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index bcda7ed2455d..1055fb73ac17 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -70,6 +70,7 @@ obj-$(CONFIG_MICREL_PHY) += micrel.o
> > obj-$(CONFIG_MICROCHIP_PHY) += microchip.o
> > obj-$(CONFIG_MICROCHIP_T1_PHY) += microchip_t1.o
> > obj-$(CONFIG_MICROSEMI_PHY) += mscc/
> > +obj-$(CONFIG_MXL_GPHY) += mxl-gpy.o
>
> Could you spell it out completely: CONFIG_MAXLINEAR_GPHY for consistency
> with the other vendor's and also to have some proper namespacing in case
> we see a non-Ethernet PHY being submitted for a Maxlinear product in the
> future?

I will fix in v4 patch.

Thank you.

RE: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Martin Blumenstingl]

Hello,

> Add driver to support the Maxlinear GPY115, GPY211, GPY212, GPY215,
> GPY241, GPY245 PHYs.
to me this seems like an evolution of the Lantiq XWAY PHYs for which
we already have a driver: intel-xway.c.
Intel took over Lantiq some years ago and last year MaxLinear then
took over what was formerly Lantiq from Intel.

From what I can tell right away: the interrupt handling still seems
to be the same. Also the GPHY firmware version register also was there
on older SoCs (with two or more GPHYs embedded into the SoC). SGMII
support is new. And I am not sure about Wake-on-LAN.

Have you considered adding support for these new PHYs to intel-xway.c?



Best regards,
Martin

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Andrew Lunn]

> > > +static int gpy_read_abilities(struct phy_device *phydev)
> > > +{
> > > + int ret;
> > > +
> > > + ret = genphy_read_abilities(phydev);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + /* Detect 2.5G/5G support. */
> > > + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT2);
> > > + if (ret < 0)
> > > + return ret;
> > > + if (!(ret & MDIO_PMA_STAT2_EXTABLE))
> > > + return 0;
> > > +
> > > + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE);
> > > + if (ret < 0)
> > > + return ret;
> > > + if (!(ret & MDIO_PMA_EXTABLE_NBT))
> > > + return 0;
> > > +
> > > + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> > > + phydev->supported,
> > > + ret & MDIO_PMA_NG_EXTABLE_2_5GBT);
> > > +
> > > + linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> > > + phydev->supported,
> > > + ret & MDIO_PMA_NG_EXTABLE_5GBT);
> >
> > This does not access vendor specific registers, should not this be part
> > of the standard genphy_read_abilities() or moved to a helper?
> >
> genphy_read_abilities does not cover 2.5G.
> 
> genphy_c45_pma_read_abilities checks C45 ids and this check fail if 
> is_c45 is not set.

You appear to of ignored my comment about this. Please add the helper
to the core as i suggested, and then use
genphy_c45_pma_read_abilities().

	Andrew

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Liang Xu]

On 5/6/2021 3:44 am, Martin Blumenstingl wrote:
> This email was sent from outside of MaxLinear.
>
>
> Hello,
>
>> Add driver to support the Maxlinear GPY115, GPY211, GPY212, GPY215,
>> GPY241, GPY245 PHYs.
> to me this seems like an evolution of the Lantiq XWAY PHYs for which
> we already have a driver: intel-xway.c.
> Intel took over Lantiq some years ago and last year MaxLinear then
> took over what was formerly Lantiq from Intel.
>
>  From what I can tell right away: the interrupt handling still seems
> to be the same. Also the GPHY firmware version register also was there
> on older SoCs (with two or more GPHYs embedded into the SoC). SGMII
> support is new. And I am not sure about Wake-on-LAN.
>
> Have you considered adding support for these new PHYs to intel-xway.c?
>
>
>
> Best regards,
> Martin
>
I'm thinking to separate them.

They are based on different IP and have many differences.

The XWAY PHYs are going to EoL (or maybe already EoL).

It creates difficulty for the test coverage to put together.

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Liang Xu]

On 5/6/2021 4:10 am, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
>>>> +static int gpy_read_abilities(struct phy_device *phydev)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = genphy_read_abilities(phydev);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + /* Detect 2.5G/5G support. */
>>>> + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT2);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + if (!(ret & MDIO_PMA_STAT2_EXTABLE))
>>>> + return 0;
>>>> +
>>>> + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + if (!(ret & MDIO_PMA_EXTABLE_NBT))
>>>> + return 0;
>>>> +
>>>> + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>>>> + phydev->supported,
>>>> + ret & MDIO_PMA_NG_EXTABLE_2_5GBT);
>>>> +
>>>> + linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
>>>> + phydev->supported,
>>>> + ret & MDIO_PMA_NG_EXTABLE_5GBT);
>>> This does not access vendor specific registers, should not this be part
>>> of the standard genphy_read_abilities() or moved to a helper?
>>>
>> genphy_read_abilities does not cover 2.5G.
>>
>> genphy_c45_pma_read_abilities checks C45 ids and this check fail if
>> is_c45 is not set.
> You appear to of ignored my comment about this. Please add the helper
> to the core as i suggested, and then use
> genphy_c45_pma_read_abilities().
>
>          Andrew
>
I'm new to upstream and do not know the process to change code in core.

Should I add the support function in same patch?

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Andrew Lunn]

> >>> This does not access vendor specific registers, should not this be part
> >>> of the standard genphy_read_abilities() or moved to a helper?
> >>>
> >> genphy_read_abilities does not cover 2.5G.
> >>
> >> genphy_c45_pma_read_abilities checks C45 ids and this check fail if
> >> is_c45 is not set.
> > You appear to of ignored my comment about this. Please add the helper
> > to the core as i suggested, and then use
> > genphy_c45_pma_read_abilities().
> >
> >          Andrew
> >
> I'm new to upstream and do not know the process to change code in core.

Pretty much the same way you change code in a driver. Submit a path!

Please put it into a separate patch, so making a patch series. Please
add some kernel doc style documentation, describing what the function
does. Look at other functions in phy_device.c for examples.

Anybody can change core code. It just gets looked at closer, and need
to be generic.

   Andrew

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Andrew Lunn]

On Sat, Jun 05, 2021 at 03:32:39AM +0000, Liang Xu wrote:
> On 5/6/2021 3:44 am, Martin Blumenstingl wrote:
> > This email was sent from outside of MaxLinear.
> >
> >
> > Hello,
> >
> >> Add driver to support the Maxlinear GPY115, GPY211, GPY212, GPY215,
> >> GPY241, GPY245 PHYs.
> > to me this seems like an evolution of the Lantiq XWAY PHYs for which
> > we already have a driver: intel-xway.c.
> > Intel took over Lantiq some years ago and last year MaxLinear then
> > took over what was formerly Lantiq from Intel.
> >
> >  From what I can tell right away: the interrupt handling still seems
> > to be the same. Also the GPHY firmware version register also was there
> > on older SoCs (with two or more GPHYs embedded into the SoC). SGMII
> > support is new. And I am not sure about Wake-on-LAN.
> >
> > Have you considered adding support for these new PHYs to intel-xway.c?

The WOL interrupts are the same, which could imply WoL configuration
is the same? If it is the same, it would be good to also add WoL to
intel-xway.c.

What this new driver does not yet do is LEDs. It would be interesting
to see if the LED control is the same. Hopefully, one day, generic LED
support will get added. If this PHY and intel-xway.c has the same LED
code, we will want to share it.

> They are based on different IP and have many differences.

Please could you list the differences? Is the datasheet available?  I
found a datasheet for the XWAY, so it would be nice to be able to
compare them.

> The XWAY PHYs are going to EoL (or maybe already EoL).

That does not matter. They are going to be in use for the next 5 years
or more, until all the products using them have died or been
replaced. Linux supports hardware that is in use, not just what
vendors sell today.

> It creates difficulty for the test coverage to put together.

That also does not really matter. If somebody has both, does the work
to merge the drivers and overall we have less code and more features,
the patch will be accepted. 


	Andrew

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Liang Xu]

On 5/6/2021 10:51 pm, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
>>>>> This does not access vendor specific registers, should not this be part
>>>>> of the standard genphy_read_abilities() or moved to a helper?
>>>>>
>>>> genphy_read_abilities does not cover 2.5G.
>>>>
>>>> genphy_c45_pma_read_abilities checks C45 ids and this check fail if
>>>> is_c45 is not set.
>>> You appear to of ignored my comment about this. Please add the helper
>>> to the core as i suggested, and then use
>>> genphy_c45_pma_read_abilities().
>>>
>>>           Andrew
>>>
>> I'm new to upstream and do not know the process to change code in core.
> Pretty much the same way you change code in a driver. Submit a path!
>
> Please put it into a separate patch, so making a patch series. Please
> add some kernel doc style documentation, describing what the function
> does. Look at other functions in phy_device.c for examples.
>
> Anybody can change core code. It just gets looked at closer, and need
> to be generic.
>
>     Andrew
>
Thank you. I will create 2 patches for the core modification and driver 
separately

in next update.

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Liang Xu]

On 6/6/2021 1:24 am, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
> On Sat, Jun 05, 2021 at 03:32:39AM +0000, Liang Xu wrote:
>> On 5/6/2021 3:44 am, Martin Blumenstingl wrote:
>>> This email was sent from outside of MaxLinear.
>>>
>>>
>>> Hello,
>>>
>>>> Add driver to support the Maxlinear GPY115, GPY211, GPY212, GPY215,
>>>> GPY241, GPY245 PHYs.
>>> to me this seems like an evolution of the Lantiq XWAY PHYs for which
>>> we already have a driver: intel-xway.c.
>>> Intel took over Lantiq some years ago and last year MaxLinear then
>>> took over what was formerly Lantiq from Intel.
>>>
>>>   From what I can tell right away: the interrupt handling still seems
>>> to be the same. Also the GPHY firmware version register also was there
>>> on older SoCs (with two or more GPHYs embedded into the SoC). SGMII
>>> support is new. And I am not sure about Wake-on-LAN.
>>>
>>> Have you considered adding support for these new PHYs to intel-xway.c?
> The WOL interrupts are the same, which could imply WoL configuration
> is the same? If it is the same, it would be good to also add WoL to
> intel-xway.c.
>
> What this new driver does not yet do is LEDs. It would be interesting
> to see if the LED control is the same. Hopefully, one day, generic LED
> support will get added. If this PHY and intel-xway.c has the same LED
> code, we will want to share it.
>
>> They are based on different IP and have many differences.
> Please could you list the differences? Is the datasheet available?  I
> found a datasheet for the XWAY, so it would be nice to be able to
> compare them.
>
>> The XWAY PHYs are going to EoL (or maybe already EoL).
> That does not matter. They are going to be in use for the next 5 years
> or more, until all the products using them have died or been
> replaced. Linux supports hardware that is in use, not just what
> vendors sell today.
>
>> It creates difficulty for the test coverage to put together.
> That also does not really matter. If somebody has both, does the work
> to merge the drivers and overall we have less code and more features,
> the patch will be accepted.
>
>
>          Andrew
>
>
The datasheet of GPY2xx are only available under NDA currently, since we 
have

to protect our IP for the new devices.

I do not know much about XWAY feature set, but I guess the difference should

be 2.5G support, C45 register set, PTP offload, MACsec offload, etc.

Problem of merging the both drivers would be the verification of the old 
devices

for which I do not have a test environment. I can't deliver code without 
testing,

and it will be big problem for me if it breaks function of existing 
user/customer.

We will check for options and need approval from company, but this will 
not be

possible short term within this merge window.

For now, can I upstream this new driver first, and merge the old driver 
into new one later?

We will enhance the new driver step by step with different features, but 
currently

we would like to make the first version available and working for our 
customers.


Thanks & Regards,

Xu Liang

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Martin Blumenstingl]

On Mon, Jun 7, 2021 at 6:37 AM Liang Xu <lxu@maxlinear.com> wrote:
[...]
> >> It creates difficulty for the test coverage to put together.
> > That also does not really matter. If somebody has both, does the work
> > to merge the drivers and overall we have less code and more features,
> > the patch will be accepted.
> >
> >
> >          Andrew
> >
> >
> The datasheet of GPY2xx are only available under NDA currently, since we
> have
>
> to protect our IP for the new devices.
I don't understand how knowledge about some registers can lead to IP
being stolen by other vendors

> I do not know much about XWAY feature set, but I guess the difference should
>
> be 2.5G support, C45 register set, PTP offload, MACsec offload, etc.
I think [0] lists the functional differences - GPY115 and newer seem
to differ from older PHYs in:
- xMII interface
- MACSEC
- IEEE-1588 v2 (PTP)
- Syn-E (not sure what this is)
- Thermal Sensor

> Problem of merging the both drivers would be the verification of the old
> devices
>
> for which I do not have a test environment. I can't deliver code without
> testing,
According to the GPY111 and GPY112 product pages the status is
"Active" ("PARTS & PURCHASING" tab).
quote from the same tab:
"Active - the part is released for sale, standard product."

I believe that these are rebranded Lantiq PHYs:
GPY111 and GPY112 are using PHYID register values which are compatible
with the intel-xway driver.
So I think you can use these PHYs for testing

Also people from the OpenWrt community (for example: me, possible also
Aleksander and Hauke) can help testing on existing hardware

[...]
> We will check for options and need approval from company, but this will
> not be
>
> possible short term within this merge window.
I hope that it is possible with GPY111 and/or GPY112 as mentioned above

> For now, can I upstream this new driver first, and merge the old driver
> into new one later?
My answer to this question depends on the actual differences between
the "old" and "new" PHYs.
For example: if WoL and LED configuration are (mostly) identical then
personally I vote for having one driver from the beginning


Best regards,
Martin


[0] https://www.maxlinear.com/products/connectivity/wired/ethernet/ethernet-transceivers-phy
[1] https://www.maxlinear.com/product/connectivity/wired/ethernet/ethernet-transceivers-phy/gpy111

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Liang Xu]

On 7/6/2021 12:06 pm, Liang Xu wrote:
> On 5/6/2021 10:51 pm, Andrew Lunn wrote:
>> This email was sent from outside of MaxLinear.
>>
>>
>>>>>> This does not access vendor specific registers, should not this be part
>>>>>> of the standard genphy_read_abilities() or moved to a helper?
>>>>>>
>>>>> genphy_read_abilities does not cover 2.5G.
>>>>>
>>>>> genphy_c45_pma_read_abilities checks C45 ids and this check fail if
>>>>> is_c45 is not set.
>>>> You appear to of ignored my comment about this. Please add the helper
>>>> to the core as i suggested, and then use
>>>> genphy_c45_pma_read_abilities().
>>>>
>>>>            Andrew
>>>>
>>> I'm new to upstream and do not know the process to change code in core.
>> Pretty much the same way you change code in a driver. Submit a path!
>>
>> Please put it into a separate patch, so making a patch series. Please
>> add some kernel doc style documentation, describing what the function
>> does. Look at other functions in phy_device.c for examples.
>>
>> Anybody can change core code. It just gets looked at closer, and need
>> to be generic.
>>
>>      Andrew
>>
> Thank you. I will create 2 patches for the core modification and driver
> separately
>
> in next update.
>
Hi Andrew,


I need your advice regarding to our recent test in loopback.

My current implementation uses "genphy_loopback" to enable/disable 
loopback mode.

And it has intermittent issue (traffic not loopbacked) during the test 
with net-next.

There are difference in the implementation in net-next and Linux v5.12.

Net-next:

int genphy_loopback(struct phy_device *phydev, bool enable)
{
     if (enable) {
         u16 val, ctl = BMCR_LOOPBACK;
         int ret;

         if (phydev->speed == SPEED_1000)
             ctl |= BMCR_SPEED1000;
         else if (phydev->speed == SPEED_100)
             ctl |= BMCR_SPEED100;

         if (phydev->duplex == DUPLEX_FULL)
             ctl |= BMCR_FULLDPLX;

         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;
     } else {
         phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, 0);

         phy_config_aneg(phydev);
     }

     return 0;
}

v5.12.11:

int genphy_loopback(struct phy_device *phydev, bool enable)
{
     return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
               enable ? BMCR_LOOPBACK : 0);
}


Not sure whether anyone else reported similar issue.

Should I use phy_modify to set the LOOPBACK bit only in my driver 
implementation as force speed with loopback enable does not work in our 
device?


Thanks & Regards,

Xu Liang

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Andrew Lunn]

> Net-next:
> 
> int genphy_loopback(struct phy_device *phydev, bool enable)
> {
>      if (enable) {
>          u16 val, ctl = BMCR_LOOPBACK;
>          int ret;
> 
>          if (phydev->speed == SPEED_1000)
>              ctl |= BMCR_SPEED1000;
>          else if (phydev->speed == SPEED_100)
>              ctl |= BMCR_SPEED100;
> 
>          if (phydev->duplex == DUPLEX_FULL)
>              ctl |= BMCR_FULLDPLX;
> 
>          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;
>      } else {
>          phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, 0);
> 
>          phy_config_aneg(phydev);
>      }
> 
>      return 0;
> }
> 
> v5.12.11:
> 
> int genphy_loopback(struct phy_device *phydev, bool enable)
> {
>      return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
>                enable ? BMCR_LOOPBACK : 0);
> }
> 
> 
> Not sure whether anyone else reported similar issue.

The commit message says:

    net: phy: genphy_loopback: add link speed configuration
    
    In case of loopback, in most cases we need to disable autoneg support
    and force some speed configuration. Otherwise, depending on currently
    active auto negotiated link speed, the loopback may or may not work.

> Should I use phy_modify to set the LOOPBACK bit only in my driver 
> implementation as force speed with loopback enable does not work in our 
> device?

So you appear to have the exact opposite problem, you need to use
auto-neg, with yourself, in order to have link. So there are two
solutions:

1) As you say, implement it in your driver

2) Add a second generic implementation, which enables autoneg, if it
is not enabled, sets the loopback bit, and waits for the link to come
up.

Does your PHY driver error out when asked to do a forced mode? It
probably should, if your silicon does not support that part of C22.

	 Andrew

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Liang Xu]

On 18/6/2021 10:01 pm, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
>> Net-next:
>>
>> int genphy_loopback(struct phy_device *phydev, bool enable)
>> {
>>       if (enable) {
>>           u16 val, ctl = BMCR_LOOPBACK;
>>           int ret;
>>
>>           if (phydev->speed == SPEED_1000)
>>               ctl |= BMCR_SPEED1000;
>>           else if (phydev->speed == SPEED_100)
>>               ctl |= BMCR_SPEED100;
>>
>>           if (phydev->duplex == DUPLEX_FULL)
>>               ctl |= BMCR_FULLDPLX;
>>
>>           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;
>>       } else {
>>           phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, 0);
>>
>>           phy_config_aneg(phydev);
>>       }
>>
>>       return 0;
>> }
>>
>> v5.12.11:
>>
>> int genphy_loopback(struct phy_device *phydev, bool enable)
>> {
>>       return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
>>                 enable ? BMCR_LOOPBACK : 0);
>> }
>>
>>
>> Not sure whether anyone else reported similar issue.
> The commit message says:
>
>      net: phy: genphy_loopback: add link speed configuration
>
>      In case of loopback, in most cases we need to disable autoneg support
>      and force some speed configuration. Otherwise, depending on currently
>      active auto negotiated link speed, the loopback may or may not work.
>
>> Should I use phy_modify to set the LOOPBACK bit only in my driver
>> implementation as force speed with loopback enable does not work in our
>> device?
> So you appear to have the exact opposite problem, you need to use
> auto-neg, with yourself, in order to have link. So there are two
> solutions:
>
> 1) As you say, implement it in your driver
>
> 2) Add a second generic implementation, which enables autoneg, if it
> is not enabled, sets the loopback bit, and waits for the link to come
> up.
>
> Does your PHY driver error out when asked to do a forced mode? It
> probably should, if your silicon does not support that part of C22.
>
>           Andrew
>
Thank you for prompt reply.

The forced mode is successful because we support forced mode in C22.

The problem happens in the speed change no matter it's forced or 
auto-neg in our device.

If I keep current status and just set the loopback bit, it works.

This may be specific to our device, if nobody else report issue in the 
genphy_loopback implementation in net-next.

Then I will implement it in my driver.


Thanks & Regards,

Xu Liang

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Andrew Lunn]

On Fri, Jun 18, 2021 at 03:36:35PM +0000, Liang Xu wrote:
> On 18/6/2021 10:01 pm, Andrew Lunn wrote:
> > This email was sent from outside of MaxLinear.
> >
> >
> >> Net-next:
> >>
> >> int genphy_loopback(struct phy_device *phydev, bool enable)
> >> {
> >>       if (enable) {
> >>           u16 val, ctl = BMCR_LOOPBACK;
> >>           int ret;
> >>
> >>           if (phydev->speed == SPEED_1000)
> >>               ctl |= BMCR_SPEED1000;
> >>           else if (phydev->speed == SPEED_100)
> >>               ctl |= BMCR_SPEED100;
 
> The problem happens in the speed change no matter it's forced or 
> auto-neg in our device.

You say speed change. So do you just need to add support for 10Mbps,
so there is no speed change? Or are you saying phydev->speed does not
match the actual speed?

      Andrew

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Liang Xu]

On 21/6/2021 10:30 am, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
> On Fri, Jun 18, 2021 at 03:36:35PM +0000, Liang Xu wrote:
>> On 18/6/2021 10:01 pm, Andrew Lunn wrote:
>>> This email was sent from outside of MaxLinear.
>>>
>>>
>>>> Net-next:
>>>>
>>>> int genphy_loopback(struct phy_device *phydev, bool enable)
>>>> {
>>>>        if (enable) {
>>>>            u16 val, ctl = BMCR_LOOPBACK;
>>>>            int ret;
>>>>
>>>>            if (phydev->speed == SPEED_1000)
>>>>                ctl |= BMCR_SPEED1000;
>>>>            else if (phydev->speed == SPEED_100)
>>>>                ctl |= BMCR_SPEED100;
>> The problem happens in the speed change no matter it's forced or
>> auto-neg in our device.
> You say speed change. So do you just need to add support for 10Mbps,
> so there is no speed change? Or are you saying phydev->speed does not
> match the actual speed?
>
>        Andrew
>
We have 2.5G link speed, so mismatch happens.


Thanks & Regards,

Xu Liang

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Andrew Lunn]

On Mon, Jun 21, 2021 at 05:05:06AM +0000, Liang Xu wrote:
> On 21/6/2021 10:30 am, Andrew Lunn wrote:
> > This email was sent from outside of MaxLinear.
> >
> >
> > On Fri, Jun 18, 2021 at 03:36:35PM +0000, Liang Xu wrote:
> >> On 18/6/2021 10:01 pm, Andrew Lunn wrote:
> >>> This email was sent from outside of MaxLinear.
> >>>
> >>>
> >>>> Net-next:
> >>>>
> >>>> int genphy_loopback(struct phy_device *phydev, bool enable)
> >>>> {
> >>>>        if (enable) {
> >>>>            u16 val, ctl = BMCR_LOOPBACK;
> >>>>            int ret;
> >>>>
> >>>>            if (phydev->speed == SPEED_1000)
> >>>>                ctl |= BMCR_SPEED1000;
> >>>>            else if (phydev->speed == SPEED_100)
> >>>>                ctl |= BMCR_SPEED100;
> >> The problem happens in the speed change no matter it's forced or
> >> auto-neg in our device.
> > You say speed change. So do you just need to add support for 10Mbps,
> > so there is no speed change? Or are you saying phydev->speed does not
> > match the actual speed?
> >
> >        Andrew
> >
> We have 2.5G link speed, so mismatch happens.

Ah, yes.  Sorry.

Please implement it in your driver.

       Andrew

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Liang Xu]

On 21/6/2021 8:48 pm, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
> On Mon, Jun 21, 2021 at 05:05:06AM +0000, Liang Xu wrote:
>> On 21/6/2021 10:30 am, Andrew Lunn wrote:
>>> This email was sent from outside of MaxLinear.
>>>
>>>
>>> On Fri, Jun 18, 2021 at 03:36:35PM +0000, Liang Xu wrote:
>>>> On 18/6/2021 10:01 pm, Andrew Lunn wrote:
>>>>> This email was sent from outside of MaxLinear.
>>>>>
>>>>>
>>>>>> Net-next:
>>>>>>
>>>>>> int genphy_loopback(struct phy_device *phydev, bool enable)
>>>>>> {
>>>>>>         if (enable) {
>>>>>>             u16 val, ctl = BMCR_LOOPBACK;
>>>>>>             int ret;
>>>>>>
>>>>>>             if (phydev->speed == SPEED_1000)
>>>>>>                 ctl |= BMCR_SPEED1000;
>>>>>>             else if (phydev->speed == SPEED_100)
>>>>>>                 ctl |= BMCR_SPEED100;
>>>> The problem happens in the speed change no matter it's forced or
>>>> auto-neg in our device.
>>> You say speed change. So do you just need to add support for 10Mbps,
>>> so there is no speed change? Or are you saying phydev->speed does not
>>> match the actual speed?
>>>
>>>         Andrew
>>>
>> We have 2.5G link speed, so mismatch happens.
> Ah, yes.  Sorry.
>
> Please implement it in your driver.
>
>         Andrew
>
Sure. Thank you for advice.


Regards,

Xu Liang

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Martin Blumenstingl]

Hi Liang,

On Wed, Jun 23, 2021 at 12:56 PM Liang Xu <lxu@maxlinear.com> wrote:
[...]
> Hi Martin,
>
> 1) The legacy PHY GPY111 does not share the same register format and address for WoL and LED registers. Therefore code saving with a common driver is not possible.
> 2) The interrupt handling routine would be different when new features are added in driver, such as PTP offload, MACsec offload. These will be added step by step as enhancement patches afte initial version of this driver upstreamed.
I think that would leave only few shared registers with the older PHYs
(and thus the intel-xway driver). Due to the lack of a public
datasheet however I have no way to confirm this.
So with this information added to the patch description having a
different driver is fine for me

Maybe Andrew can also share his opinion - based on this new
information - as he previously also suggested to extend the intel-xway
driver instead of creating a new one

> 3) The new PHY generation comes with a reasonable pre-configuration for the LED registers which does not need any modification usually.
>    In case a customer needs a specific configuration (e.g. traffic pulsing only, no link status) he needs to configure this via MDIO. There is also another option for a fixed preconfiguration with the support of an external flash. However, this is out of scope of the driver.
older PHYs also do this, but not all boards use a reasonable LED setup

> 4) Many old products are mostly used on embedded devices which do not support WoL. Therefore there was no request yet to supported by the legacy XWAY driver.
my understanding of Andrew's argument is: "if the register layout is
the same for old and new PHY then why would we do some extra effort to
not add WoL support for the old PHYs"
Based on your information above the register layout is different, so
that means two different implementations are needed anyways.


Best regards,
Martin

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Andrew Lunn]

On Wed, Jun 23, 2021 at 11:09:23PM +0200, Martin Blumenstingl wrote:
> Hi Liang,
> 
> On Wed, Jun 23, 2021 at 12:56 PM Liang Xu <lxu@maxlinear.com> wrote:
> [...]
> > Hi Martin,
> >
> > 1) The legacy PHY GPY111 does not share the same register format and address for WoL and LED registers. Therefore code saving with a common driver is not possible.
> > 2) The interrupt handling routine would be different when new features are added in driver, such as PTP offload, MACsec offload. These will be added step by step as enhancement patches afte initial version of this driver upstreamed.
> I think that would leave only few shared registers with the older PHYs
> (and thus the intel-xway driver). Due to the lack of a public
> datasheet however I have no way to confirm this.
> So with this information added to the patch description having a
> different driver is fine for me
> 
> Maybe Andrew can also share his opinion - based on this new
> information - as he previously also suggested to extend the intel-xway
> driver instead of creating a new one

If the potential sharing is minimal, then a new driver makes sense. So
please do expand the patch description with a good justification.

> > 3) The new PHY generation comes with a reasonable pre-configuration for the LED registers which does not need any modification usually.
> >    In case a customer needs a specific configuration (e.g. traffic pulsing only, no link status) he needs to configure this via MDIO. There is also another option for a fixed preconfiguration with the support of an external flash. However, this is out of scope of the driver.
> older PHYs also do this, but not all boards use a reasonable LED setup
> 
> > 4) Many old products are mostly used on embedded devices which do not support WoL. Therefore there was no request yet to supported by the legacy XWAY driver.
> my understanding of Andrew's argument is: "if the register layout is
> the same for old and new PHY then why would we do some extra effort to
> not add WoL support for the old PHYs"
> Based on your information above the register layout is different, so
> that means two different implementations are needed anyways.

It would be nice to add WoL support for the older devices. They might
not be maxlinears latest and greatest, but there could be more of them
actually in use at the moment than this new PHY.

	 Andrew

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Andrew Lunn]

On Tue, Jun 22, 2021 at 04:21:47AM +0000, Ismail, Mohammad Athari wrote:
> > -----Original Message-----
> > From: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > Sent: Tuesday, June 22, 2021 12:15 PM
> > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > Subject:
> > 
> > > Net-next:
> > >
> > > int genphy_loopback(struct phy_device *phydev, bool enable) {
> > >      if (enable) {
> > >          u16 val, ctl = BMCR_LOOPBACK;
> > >          int ret;
> > >
> > >          if (phydev->speed == SPEED_1000)
> > >              ctl |= BMCR_SPEED1000;
> > >          else if (phydev->speed == SPEED_100)
> > >              ctl |= BMCR_SPEED100;
> > >
> > >          if (phydev->duplex == DUPLEX_FULL)
> > >              ctl |= BMCR_FULLDPLX;
> > >
> > >          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;
> > >      } else {
> > >          phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, 0);
> > >
> > >          phy_config_aneg(phydev);
> > >      }
> > >
> > >      return 0;
> > > }
> 
> Hi Andrew,
> 

> We also observe same issue on Marvell88E1510 PHY (C22 supported PHY)
> as well. It works with v5.12.11's genphy_loopback() but not
> net-next's.

Ah, yes. The Marvell probably needs a software reset after the write
to the MII_BMSR register. But just setting the loopback bit also
probably needed a software reset as well, so i suspect it was broken
before this change.

Oleksij, rather that writing registers directly, we probably need to
use the phylib API calls to configure the PHY. That will handle
oddities like the Marvell needing a reset, or PHYs with other speeds
etc.

	Andrew

Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

[Ismail, Mohammad Athari]

> -----Original Message-----
> From: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> Sent: Tuesday, June 22, 2021 12:15 PM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> Subject:
> 
> > Net-next:
> >
> > int genphy_loopback(struct phy_device *phydev, bool enable) {
> >      if (enable) {
> >          u16 val, ctl = BMCR_LOOPBACK;
> >          int ret;
> >
> >          if (phydev->speed == SPEED_1000)
> >              ctl |= BMCR_SPEED1000;
> >          else if (phydev->speed == SPEED_100)
> >              ctl |= BMCR_SPEED100;
> >
> >          if (phydev->duplex == DUPLEX_FULL)
> >              ctl |= BMCR_FULLDPLX;
> >
> >          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;
> >      } else {
> >          phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, 0);
> >
> >          phy_config_aneg(phydev);
> >      }
> >
> >      return 0;
> > }
> >
> > v5.12.11:
> >
> > int genphy_loopback(struct phy_device *phydev, bool enable) {
> >      return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
> >                enable ? BMCR_LOOPBACK : 0); }
> >
> >
> > Not sure whether anyone else reported similar issue.
> 
> The commit message says:
> 
>     net: phy: genphy_loopback: add link speed configuration
> 
>     In case of loopback, in most cases we need to disable autoneg support
>     and force some speed configuration. Otherwise, depending on currently
>     active auto negotiated link speed, the loopback may or may not work.
> 
> > Should I use phy_modify to set the LOOPBACK bit only in my driver
> > implementation as force speed with loopback enable does not work in
> > our device?
> 
> So you appear to have the exact opposite problem, you need to use auto-
> neg, with yourself, in order to have link. So there are two
> solutions:
> 
> 1) As you say, implement it in your driver
> 
> 2) Add a second generic implementation, which enables autoneg, if it is not
> enabled, sets the loopback bit, and waits for the link to come up.
> 
> Does your PHY driver error out when asked to do a forced mode? It probably
> should, if your silicon does not support that part of C22.
> 
> 	 Andrew

Hi Andrew,

We also observe same issue on Marvell88E1510 PHY (C22 supported PHY) as well. It works with v5.12.11's genphy_loopback() but not net-next's.

Regards,
Athari