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

On Tue, Jun 01, 2021 at 03:44:27PM +0800, Xu Liang wrote:

This file is sorted based on the tristate string. So this entry should
come after the "Marvell 88X2222 PHY"

And this file is sorted on the CONFIG_XXX value.

+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);
+

Does genphy_c45_pma_read_abilities() do the wrong thing here? What
does it get wrong?

+	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 & BIT(15)) ? "release" : "test");

You define PHY_FWV_TYPE_MASK and PHY_FWV_MINOR_MASK. Does it make
sense to break this up a bit more?

+static bool gpy_sgmii_need_reaneg(struct phy_device *phydev)
+{
+	struct {
+		int type;
+		int minor;
+	} table[] = {
+		{7, 0x6D},
+		{8, 0x6D},
+		{9, 0x73},
+	};

Please make this const. And to fix reverse christmas tree, you might
want to move it out of the function.

+
+	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(table); i++) {
+		if (fw_type != table[i].type)
+			continue;
+		if (fw_minor < table[i].minor)
+			return true;
+		break;
+	}
+
+	return false;
+}
+
+static bool gpy_sgmii_2p5g_chk(struct phy_device *phydev)

This name got me for a while. p meaning point? I would prefer 
gpy_sgmii_2500_chk.

+	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;

Then function is called SGMII but here you use 2500BaseX? Which is the
hardware doing?

+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);

You seem to like bool functions meaning you cannot return an error
code. You might want to consider using int.

+		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)

It is normal to put the || at the end of the line.

+		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_sgmii_2p5g_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)

Please move the ||

+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);

can genphy_read_lpa() be used here?

+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 */
+		ret = phy_read(phydev, PHY_ISTAT);
+		if (ret < 0)
+			return ret;

It seems like there is the potential to loose other interrupts here?
I wonder if you should call phy_trigger_machine(phydev) if other bits
are set?

+
+	} 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);
+	} else {
+		/* Disable the link state change interrupt */
+		ret = phy_clear_bits(phydev, PHY_IMASK, PHY_IMASK_LSTC);
+	}
+
+	return ret;
+}
+
+static struct phy_driver gpy_drivers[] = {
+	{
+		.phy_id		= PHY_ID_GPY,
+		.phy_id_mask	= PHY_ID_MASK,

+#define PHY_ID_MASK		GENMASK(31, 10)

That is a rather large mask? Can you split this up into individual
devices?


  Andrew