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

[PATCH v2] 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.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 673cadd5107a..9346e6357960 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11167,6 +11167,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..70efab3659ee 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..350ad3c610f7
--- /dev/null
+++ b/drivers/net/phy/mxl-gpy.c
@@ -0,0 +1,545 @@
+// 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, fw_ver;
+
+	/* Show GPY PHY FW version in dmesg */
+	fw_ver = phy_read(phydev, PHY_FWV);
+	if (fw_ver < 0)
+		return fw_ver;
+
+	phydev_info(phydev, "Firmware Version: 0x%04X (%s)\n", fw_ver,
+		    (fw_ver & PHY_FWV_REL_MASK) ? "release" : "test");
+
+	/* 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 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_changed(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) {
+		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)
+		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_changed(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_changed(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_changed(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,
+		.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 v2] net: phy: add Maxlinear GPY115/21x/24x driver

[Russell King (Oracle)]

Hi,

On Thu, Jun 03, 2021 at 03:34:38PM +0800, Xu Liang wrote:
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index bcda7ed2455d..70efab3659ee 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

This should use tab(s) to align indentation rather than spaces.

> +static int gpy_config_init(struct phy_device *phydev)
> +{
> +	int ret, fw_ver;
> +
> +	/* Show GPY PHY FW version in dmesg */
> +	fw_ver = phy_read(phydev, PHY_FWV);
> +	if (fw_ver < 0)
> +		return fw_ver;
> +
> +	phydev_info(phydev, "Firmware Version: 0x%04X (%s)\n", fw_ver,
> +		    (fw_ver & PHY_FWV_REL_MASK) ? "release" : "test");

Does this need to print the firmware version each time config_init()
is called? Is it likely to change beyond? Would it be more sensible
to print it in the probe() method?

> +static int gpy_config_aneg(struct phy_device *phydev)
> +{
> +	bool changed = false;
> +	u32 adv;
> +	int ret;
> +
> +	if (phydev->autoneg == AUTONEG_DISABLE) {
> +		return phydev->duplex != DUPLEX_FULL
> +			? genphy_setup_forced(phydev)
> +			: genphy_c45_pma_setup_forced(phydev);

I think this needs a comment to describe what is going on here to
explain why the duplex setting influences whether we program the PHY
via C22 or C45.

> +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_changed(phydev, MDIO_MMD_VEND1,
> +					     VSPEC1_SGMII_CTRL,
> +					     VSPEC1_SGMII_CTRL_ANEN, 0);

Do you need to know if the bit was changed? It doesn't appear so, so
please consider using phy_modify_mmd() here and in the other part of
this switch block.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Liang Xu]

> > +static int gpy_config_aneg(struct phy_device *phydev)
> > +{
> > + bool changed = false;
> > + u32 adv;
> > + int ret;
> > +
> > + if (phydev->autoneg == AUTONEG_DISABLE) {
> > + return phydev->duplex != DUPLEX_FULL
> > + ? genphy_setup_forced(phydev)
> > + : genphy_c45_pma_setup_forced(phydev);
>
> I think this needs a comment to describe what is going on here to
> explain why the duplex setting influences whether we program the PHY
> via C22 or C45.
>
Thank you for review.

We support half/full duplex for 10/100/1000 and full duplex only for 2500.

genphy_c45_pma_setup_forced supports full duplex only.

So in half duplex, I use genphy_setup_forced.

Is there recommended way to handle this situation?

Or I should keep this code and put a comment.

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

[Liang Xu]

On 3/6/2021 5:17 pm, Russell King (Oracle) wrote:
>
> > +static int gpy_config_init(struct phy_device *phydev)
> > +{
> > + int ret, fw_ver;
> > +
> > + /* Show GPY PHY FW version in dmesg */
> > + fw_ver = phy_read(phydev, PHY_FWV);
> > + if (fw_ver < 0)
> > + return fw_ver;
> > +
> > + phydev_info(phydev, "Firmware Version: 0x%04X (%s)\n", fw_ver,
> > + (fw_ver & PHY_FWV_REL_MASK) ? "release" : "test");
>
> Does this need to print the firmware version each time config_init()
> is called? Is it likely to change beyond? Would it be more sensible
> to print it in the probe() method?

The firmware version can change in device with different firmware 
loading mechanism.

I moved the print to probe and tested a few devices, found in some cases 
it did not print the active version number.

So I'm thinking to keep it here to cover all scenarios.

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

[Andrew Lunn]

On Thu, Jun 03, 2021 at 03:10:31PM +0000, Liang Xu wrote:
> On 3/6/2021 5:17 pm, Russell King (Oracle) wrote:
> >
> > > +static int gpy_config_init(struct phy_device *phydev)
> > > +{
> > > + int ret, fw_ver;
> > > +
> > > + /* Show GPY PHY FW version in dmesg */
> > > + fw_ver = phy_read(phydev, PHY_FWV);
> > > + if (fw_ver < 0)
> > > + return fw_ver;
> > > +
> > > + phydev_info(phydev, "Firmware Version: 0x%04X (%s)\n", fw_ver,
> > > + (fw_ver & PHY_FWV_REL_MASK) ? "release" : "test");
> >
> > Does this need to print the firmware version each time config_init()
> > is called? Is it likely to change beyond? Would it be more sensible
> > to print it in the probe() method?
> 
> The firmware version can change in device with different firmware 
> loading mechanism.
> 
> I moved the print to probe and tested a few devices, found in some cases 
> it did not print the active version number.

That actually sounds like a real problem. If it is still in the
bootloader when the driver is probed, the driver should not be writing
any configuration registers until the real image is running. So it
sounds like you need a probe function which checks if the PHY has
finished booting, and if not, wait for the real firmware to start
running.

	Andrew

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

[Liang Xu]

On 3/6/2021 11:21 pm, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
> On Thu, Jun 03, 2021 at 03:10:31PM +0000, Liang Xu wrote:
>> On 3/6/2021 5:17 pm, Russell King (Oracle) wrote:
>>>> +static int gpy_config_init(struct phy_device *phydev)
>>>> +{
>>>> + int ret, fw_ver;
>>>> +
>>>> + /* Show GPY PHY FW version in dmesg */
>>>> + fw_ver = phy_read(phydev, PHY_FWV);
>>>> + if (fw_ver < 0)
>>>> + return fw_ver;
>>>> +
>>>> + phydev_info(phydev, "Firmware Version: 0x%04X (%s)\n", fw_ver,
>>>> + (fw_ver & PHY_FWV_REL_MASK) ? "release" : "test");
>>> Does this need to print the firmware version each time config_init()
>>> is called? Is it likely to change beyond? Would it be more sensible
>>> to print it in the probe() method?
>> The firmware version can change in device with different firmware
>> loading mechanism.
>>
>> I moved the print to probe and tested a few devices, found in some cases
>> it did not print the active version number.
> That actually sounds like a real problem. If it is still in the
> bootloader when the driver is probed, the driver should not be writing
> any configuration registers until the real image is running. So it
> sounds like you need a probe function which checks if the PHY has
> finished booting, and if not, wait for the real firmware to start
> running.
>
>          Andrew
>
I think my word was misleading.

The device always has valid firmware running.

The firmware version can change because of switch of the firmware during 
running time.

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

[Andrew Lunn]

> The firmware version can change because of switch of the firmware during 
> running time.

Please could you explain this some more. What happens if i initialize
the PHY and then it decided to switch its firmware. Is the
configuration kept? Does it need reconfiguring?

      Andrew

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

[Liang Xu]

On 4/6/2021 1:05 am, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
>> The firmware version can change because of switch of the firmware during
>> running time.
> Please could you explain this some more. What happens if i initialize
> the PHY and then it decided to switch its firmware. Is the
> configuration kept? Does it need reconfiguring?
>
>        Andrew
>
In one of our product (in developing), the NIC driver can decide to 
switch to 2nd firmware.

The switch happens when network interface is down.

After the switch, when the network interface is up, phy_init_hw is 
triggered to init the phy.

The PHY device itself won't decide to switch firmware.

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

[Andrew Lunn]

> In one of our product (in developing), the NIC driver can decide to 
> switch to 2nd firmware.

There are currently no systems like this in Linux. So this is going to
need new support. Ideally, we don't want the MAC involved in this
switch, because then you need to teach every MAC driver in linux about
how your PHY is special.

I would say, for the moment, forget about this setup. When you submit
support for it, we can look at the bigger picture, and see how best to
support it.

So please put the firmware version print in the driver probe
function. That should work for the devices the driver currently
supports.

	  Andrew

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

[Andrew Lunn]

> +config MXL_GPHY
> +	tristate "Maxlinear PHYs"
> +	help
> +	  Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> +	  GPY241, GPY245 PHYs.

Do these PHYs have unique IDs in register 2 and 3? What is the format
of these IDs?

The OUI is fixed. But often the rest is split into two. The higher
part indicates the product, and the lower part is the revision. We
then have a struct phy_driver for each product, and the mask is used
to match on all the revisions of the product.

     Andrew

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

[Liang Xu]

On 4/6/2021 8:09 pm, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
>> In one of our product (in developing), the NIC driver can decide to
>> switch to 2nd firmware.
> There are currently no systems like this in Linux. So this is going to
> need new support. Ideally, we don't want the MAC involved in this
> switch, because then you need to teach every MAC driver in linux about
> how your PHY is special.
>
> I would say, for the moment, forget about this setup. When you submit
> support for it, we can look at the bigger picture, and see how best to
> support it.
>
> So please put the firmware version print in the driver probe
> function. That should work for the devices the driver currently
> supports.
>
>            Andrew
>
OK

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

[Liang Xu]

On 4/6/2021 8:15 pm, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
>> +config MXL_GPHY
>> +     tristate "Maxlinear PHYs"
>> +     help
>> +       Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
>> +       GPY241, GPY245 PHYs.
> Do these PHYs have unique IDs in register 2 and 3? What is the format
> of these IDs?
>
> The OUI is fixed. But often the rest is split into two. The higher
> part indicates the product, and the lower part is the revision. We
> then have a struct phy_driver for each product, and the mask is used
> to match on all the revisions of the product.
>
>       Andrew
>
Register 2, Register 3 bit 10~15 - OUI

Register 3 bit 4~9 - product number

Register 3 bit 0~3 - revision number

These PHYs have same ID in register 2 and 3.

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

[Andrew Lunn]

On Fri, Jun 04, 2021 at 12:52:02PM +0000, Liang Xu wrote:
> On 4/6/2021 8:15 pm, Andrew Lunn wrote:
> > This email was sent from outside of MaxLinear.
> >
> >
> >> +config MXL_GPHY
> >> +     tristate "Maxlinear PHYs"
> >> +     help
> >> +       Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> >> +       GPY241, GPY245 PHYs.
> > Do these PHYs have unique IDs in register 2 and 3? What is the format
> > of these IDs?
> >
> > The OUI is fixed. But often the rest is split into two. The higher
> > part indicates the product, and the lower part is the revision. We
> > then have a struct phy_driver for each product, and the mask is used
> > to match on all the revisions of the product.
> >
> >       Andrew
> >
> Register 2, Register 3 bit 10~15 - OUI
> 
> Register 3 bit 4~9 - product number
> 
> Register 3 bit 0~3 - revision number
> 
> These PHYs have same ID in register 2 and 3.

O.K, that is pretty normal. Please add a phy_device for each
individual PHY. There are macros to help you do this.

	Andrew

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

[Liang Xu]

On 5/6/2021 4:39 am, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
> On Fri, Jun 04, 2021 at 12:52:02PM +0000, Liang Xu wrote:
>> On 4/6/2021 8:15 pm, Andrew Lunn wrote:
>>> This email was sent from outside of MaxLinear.
>>>
>>>
>>>> +config MXL_GPHY
>>>> +     tristate "Maxlinear PHYs"
>>>> +     help
>>>> +       Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
>>>> +       GPY241, GPY245 PHYs.
>>> Do these PHYs have unique IDs in register 2 and 3? What is the format
>>> of these IDs?
>>>
>>> The OUI is fixed. But often the rest is split into two. The higher
>>> part indicates the product, and the lower part is the revision. We
>>> then have a struct phy_driver for each product, and the mask is used
>>> to match on all the revisions of the product.
>>>
>>>        Andrew
>>>
>> Register 2, Register 3 bit 10~15 - OUI
>>
>> Register 3 bit 4~9 - product number
>>
>> Register 3 bit 0~3 - revision number
>>
>> These PHYs have same ID in register 2 and 3.
> O.K, that is pretty normal. Please add a phy_device for each
> individual PHY. There are macros to help you do this.
>
>          Andrew
>
Sorry, I didn't get the point.

Do you mean create the MDIO device for each PHY like below?

static struct mdio_device_id __maybe_unused gpy_tbl[] = {
         {PHY_ID_MATCH_MODEL(PHY_ID_GPY)},
         { }
};
MODULE_DEVICE_TABLE(mdio, gpy_tbl);

These PHYs have same ID and no difference OUI, product number, revision 
number.

Please give me more guidance if my understanding is wrong.

Thank you.

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

[Andrew Lunn]

On Sat, Jun 05, 2021 at 03:46:18AM +0000, Liang Xu wrote:
> On 5/6/2021 4:39 am, Andrew Lunn wrote:
> > This email was sent from outside of MaxLinear.
> >
> >
> > On Fri, Jun 04, 2021 at 12:52:02PM +0000, Liang Xu wrote:
> >> On 4/6/2021 8:15 pm, Andrew Lunn wrote:
> >>> This email was sent from outside of MaxLinear.
> >>>
> >>>
> >>>> +config MXL_GPHY
> >>>> +     tristate "Maxlinear PHYs"
> >>>> +     help
> >>>> +       Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> >>>> +       GPY241, GPY245 PHYs.
> >>> Do these PHYs have unique IDs in register 2 and 3? What is the format
> >>> of these IDs?
> >>>
> >>> The OUI is fixed. But often the rest is split into two. The higher
> >>> part indicates the product, and the lower part is the revision. We
> >>> then have a struct phy_driver for each product, and the mask is used
> >>> to match on all the revisions of the product.
> >>>
> >>>        Andrew
> >>>
> >> Register 2, Register 3 bit 10~15 - OUI
> >>
> >> Register 3 bit 4~9 - product number
> >>
> >> Register 3 bit 0~3 - revision number
> >>

> These PHYs have same ID and no difference OUI, product number, revision 
> number.

Are you saying GPY115, GPY211, GPY212, GPY215, GPY241, GPY245 all have
the same product number?

Normally, each PHY has its own product ID, and so we have:

/* Vitesse 82xx */
static struct phy_driver vsc82xx_driver[] = {
{
        .phy_id         = PHY_ID_VSC8234,
        .name           = "Vitesse VSC8234",
        .phy_id_mask    = 0x000ffff0,
        /* PHY_GBIT_FEATURES */
        .config_init    = &vsc824x_config_init,
        .config_aneg    = &vsc82x4_config_aneg,
        .config_intr    = &vsc82xx_config_intr,
        .handle_interrupt = &vsc82xx_handle_interrupt,
}, {
        .phy_id         = PHY_ID_VSC8244,
        .name           = "Vitesse VSC8244",
        .phy_id_mask    = 0x000fffc0,
        /* PHY_GBIT_FEATURES */
        .config_init    = &vsc824x_config_init,
        .config_aneg    = &vsc82x4_config_aneg,
        .config_intr    = &vsc82xx_config_intr,
        .handle_interrupt = &vsc82xx_handle_interrupt,
}, {
        .phy_id         = PHY_ID_VSC8572,
        .name           = "Vitesse VSC8572",
        .phy_id_mask    = 0x000ffff0,
        /* PHY_GBIT_FEATURES */
        .config_init    = &vsc824x_config_init,
        .config_aneg    = &vsc82x4_config_aneg,
        .config_intr    = &vsc82xx_config_intr,
        .handle_interrupt = &vsc82xx_handle_interrupt,
}, {

one entry to describe one PHY.

    Andrew

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

[Liang Xu]

On 5/6/2021 10:37 pm, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
> On Sat, Jun 05, 2021 at 03:46:18AM +0000, Liang Xu wrote:
>> On 5/6/2021 4:39 am, Andrew Lunn wrote:
>>> This email was sent from outside of MaxLinear.
>>>
>>>
>>> On Fri, Jun 04, 2021 at 12:52:02PM +0000, Liang Xu wrote:
>>>> On 4/6/2021 8:15 pm, Andrew Lunn wrote:
>>>>> This email was sent from outside of MaxLinear.
>>>>>
>>>>>
>>>>>> +config MXL_GPHY
>>>>>> +     tristate "Maxlinear PHYs"
>>>>>> +     help
>>>>>> +       Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
>>>>>> +       GPY241, GPY245 PHYs.
>>>>> Do these PHYs have unique IDs in register 2 and 3? What is the format
>>>>> of these IDs?
>>>>>
>>>>> The OUI is fixed. But often the rest is split into two. The higher
>>>>> part indicates the product, and the lower part is the revision. We
>>>>> then have a struct phy_driver for each product, and the mask is used
>>>>> to match on all the revisions of the product.
>>>>>
>>>>>         Andrew
>>>>>
>>>> Register 2, Register 3 bit 10~15 - OUI
>>>>
>>>> Register 3 bit 4~9 - product number
>>>>
>>>> Register 3 bit 0~3 - revision number
>>>>
>> These PHYs have same ID and no difference OUI, product number, revision
>> number.
> Are you saying GPY115, GPY211, GPY212, GPY215, GPY241, GPY245 all have
> the same product number?
>
> Normally, each PHY has its own product ID, and so we have:
>
> /* Vitesse 82xx */
> static struct phy_driver vsc82xx_driver[] = {
> {
>          .phy_id         = PHY_ID_VSC8234,
>          .name           = "Vitesse VSC8234",
>          .phy_id_mask    = 0x000ffff0,
>          /* PHY_GBIT_FEATURES */
>          .config_init    = &vsc824x_config_init,
>          .config_aneg    = &vsc82x4_config_aneg,
>          .config_intr    = &vsc82xx_config_intr,
>          .handle_interrupt = &vsc82xx_handle_interrupt,
> }, {
>          .phy_id         = PHY_ID_VSC8244,
>          .name           = "Vitesse VSC8244",
>          .phy_id_mask    = 0x000fffc0,
>          /* PHY_GBIT_FEATURES */
>          .config_init    = &vsc824x_config_init,
>          .config_aneg    = &vsc82x4_config_aneg,
>          .config_intr    = &vsc82xx_config_intr,
>          .handle_interrupt = &vsc82xx_handle_interrupt,
> }, {
>          .phy_id         = PHY_ID_VSC8572,
>          .name           = "Vitesse VSC8572",
>          .phy_id_mask    = 0x000ffff0,
>          /* PHY_GBIT_FEATURES */
>          .config_init    = &vsc824x_config_init,
>          .config_aneg    = &vsc82x4_config_aneg,
>          .config_intr    = &vsc82xx_config_intr,
>          .handle_interrupt = &vsc82xx_handle_interrupt,
> }, {
>
> one entry to describe one PHY.
>
>      Andrew
>
Yes, they all have same product number.

They are one IP.

The difference is feature set it's enabled by fusing in silicon.

For example, GPY115 has 10/100/1000Mbps support, so in the ability 
register 2.5G capable is 0.

GPY211 has 10/100/1000/2500Mbps support, so in the capability register 
2.5G capable is 1.

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

[Andrew Lunn]

> Yes, they all have same product number.
> 
> They are one IP.

O.K, this is the sort of information which is useful to have in the
commit message. Basically anything which is odd about your PHY it is
good to mention, because reviewers are probably going to notice and
ask.

> The difference is feature set it's enabled by fusing in silicon.
> 
> For example, GPY115 has 10/100/1000Mbps support, so in the ability 
> register 2.5G capable is 0.
> 
> GPY211 has 10/100/1000/2500Mbps support, so in the capability register 
> 2.5G capable is 1.
 
I assume it is more than just the capability register? Linux could
easily ignore that and make use of 2.5G if it still actually works.

       Andrew

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

[Liang Xu]

On 7/6/2021 8:15 pm, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
>> Yes, they all have same product number.
>>
>> They are one IP.
> O.K, this is the sort of information which is useful to have in the
> commit message. Basically anything which is odd about your PHY it is
> good to mention, because reviewers are probably going to notice and
> ask.
Thanks, will update.
>> The difference is feature set it's enabled by fusing in silicon.
>>
>> For example, GPY115 has 10/100/1000Mbps support, so in the ability
>> register 2.5G capable is 0.
>>
>> GPY211 has 10/100/1000/2500Mbps support, so in the capability register
>> 2.5G capable is 1.
> I assume it is more than just the capability register? Linux could
> easily ignore that and make use of 2.5G if it still actually works.
>
>         Andrew

You are right, not only capability register. The 2.5G function including 
relative

registers/bit-fields do not work.