Date:   Wed, 22 Sep 2021 21:14:46 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     netdev@...r.kernel.org
Cc:     Antoine Tenart <atenart@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Bjarni Jonasson <bjarni.jonasson@...rochip.com>,
        Steen Hegelund <steen.hegelund@...rochip.com>,
        UNGLinuxDriver@...rochip.com,
        bcm-kernel-feedback-list@...adcom.com,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>
Subject: [RFC PATCH v3 net-next 6/6] net: phy: at803x: configure in-band auto-negotiation for AR8031/AR8033

The Atheros PHY driver supports quite a wide variety of hardware, but
the validation function for the in-band autoneg setting was deliberately
made common to all revisions.

The reasoning went as follows:

- In-band autonegotiation is only of concern for protocols which use
  the IEEE 802.3 clause 37 state machines. In the case of Atheros PHYs,
  that is only SGMII. The only PHYs that support SGMII are AR8031 and
  AR8033.

- The other PHYs either use MII/RMII (AR8030/AR8032), RGMII (AR8035,
  AR8031/AR8033 in certain configurations), or are straight out internal
  to a switch, and in that case, in-band autoneg makes no sense.

- In any case it is buggy to request in-band autoneg for an
  MII/RMII/RGMII/internal PHY, so the common method also validates that.

In any case, for AR8031/AR8033, the original intention was to only
declare support for PHY_INBAND_ANEG_ON. The idea is that even that is
valuable in its own right. For example, this avoids future breakages
caused by conversions to phylink such as the one fixed by commit
df392aefe96b ("arm64: dts: fsl-ls1028a-kontron-sl28: specify in-band
mode for ENETC"), by pulling the MAC side of phylink into using
MLO_AN_INBAND.

Nonetheless, after playing around a bit, I managed to get my AR8033 to
work fine with all of 10/100/1000 link speeds even with in-band autoneg
disabled. The strategy to keep the fiber and copper page speeds in sync
was based on the comments made by Michael Walle here:
https://patchwork.kernel.org/project/netdevbpf/patch/20210212172341.3489046-2-olteanv@gmail.com/

I wanted to see if it works at all, more than anything else, but now I'm
in a bit of a dilemma whether to make this PHY driver support both
cases, but risk regressions with MAC drivers that don't disable inband
autoneg in MLO_AN_PHY mode, or just force PHY_INBAND_ANEG_ON and hence
MLO_AN_INBAND, and continue to work with those. The thing is, I'm pretty
sure that there isn't any in-tree user of Atheros PHYs in SGMII mode
with inband autoneg off, because that requires manually keeping the
speeds in sync, and since the code did not do that, that would have been
a pretty broken link, working just at 1Gbps. So the risk is definitely
larger to actually do what the PHY has been requested, but it also
requires the MAC driver to put its money where its mouth is. I've
audited the tree and macb_mac_config() looks suspicious, but I don't
have all the details to understand whether there is any system that
would be affected by this change.

Cc: Nicolas Ferre <nicolas.ferre@...rochip.com>
Cc: Claudiu Beznea <claudiu.beznea@...rochip.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 drivers/net/phy/at803x.c | 72 +++++++++++++++++++++++++++++++++++++++-
 include/linux/phy.h      |  1 +
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 3feee4d59030..7262ce509762 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -196,6 +196,7 @@ struct at803x_priv {
 	struct regulator_dev *vddh_rdev;
 	struct regulator *vddio;
 	u64 stats[ARRAY_SIZE(at803x_hw_stats)];
+	bool inband_an;
 };
 
 struct at803x_context {
@@ -784,8 +785,17 @@ static int at8031_pll_config(struct phy_device *phydev)
 
 static int at803x_config_init(struct phy_device *phydev)
 {
+	struct at803x_priv *priv = phydev->priv;
 	int ret;
 
+	if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
+		ret = phy_read_paged(phydev, AT803X_PAGE_FIBER, MII_BMCR);
+		if (ret < 0)
+			return ret;
+
+		priv->inband_an = !!(ret & BMCR_ANENABLE);
+	}
+
 	/* The RX and TX delay default is:
 	 *   after HW reset: RX delay enabled and TX delay disabled
 	 *   after SW reset: RX delay enabled, while TX delay retains the
@@ -922,8 +932,26 @@ static void at803x_link_change_notify(struct phy_device *phydev)
 	}
 }
 
+/* When in-band autoneg is turned off, this hardware has a split-brain problem,
+ * it requires the SGMII-side link speed needs to be kept in sync with the
+ * media-side link speed by the driver, so do that.
+ */
+static int at803x_sync_fiber_page_speed(struct phy_device *phydev)
+{
+	int mask = BMCR_SPEED1000 | BMCR_SPEED100;
+	int val = 0;
+
+	if (phydev->speed == SPEED_1000)
+		val = BMCR_SPEED1000;
+	else if (phydev->speed == SPEED_100)
+		val = BMCR_SPEED100;
+
+	return phy_modify_paged(phydev, AT803X_PAGE_FIBER, MII_BMCR, mask, val);
+}
+
 static int at803x_read_status(struct phy_device *phydev)
 {
+	struct at803x_priv *priv = phydev->priv;
 	int ss, err, old_link = phydev->link;
 
 	/* Update the link, but return if there was an error */
@@ -996,7 +1024,10 @@ static int at803x_read_status(struct phy_device *phydev)
 	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete)
 		phy_resolve_aneg_pause(phydev);
 
-	return 0;
+	if (priv->inband_an)
+		return 0;
+
+	return at803x_sync_fiber_page_speed(phydev);
 }
 
 static int at803x_config_mdix(struct phy_device *phydev, u8 ctrl)
@@ -1043,6 +1074,36 @@ static int at803x_config_aneg(struct phy_device *phydev)
 	return genphy_config_aneg(phydev);
 }
 
+static int at803x_config_inband_aneg(struct phy_device *phydev, bool enabled)
+{
+	struct at803x_priv *priv = phydev->priv;
+	int err, val = 0;
+
+	if (phydev->interface != PHY_INTERFACE_MODE_SGMII)
+		return 0;
+
+	if (enabled)
+		val = BMCR_ANENABLE;
+
+	err = phy_modify_paged(phydev, AT803X_PAGE_FIBER, MII_BMCR,
+			       BMCR_ANENABLE, val);
+	if (err)
+		return err;
+
+	priv->inband_an = enabled;
+
+	return at803x_sync_fiber_page_speed(phydev);
+}
+
+static int at803x_validate_inband_aneg(struct phy_device *phydev,
+				       phy_interface_t interface)
+{
+	if (interface == PHY_INTERFACE_MODE_SGMII)
+		return PHY_INBAND_ANEG_ON | PHY_INBAND_ANEG_OFF;
+
+	return PHY_INBAND_ANEG_OFF;
+}
+
 static int at803x_get_downshift(struct phy_device *phydev, u8 *d)
 {
 	int val;
@@ -1335,6 +1396,7 @@ static struct phy_driver at803x_driver[] = {
 	.set_tunable		= at803x_set_tunable,
 	.cable_test_start	= at803x_cable_test_start,
 	.cable_test_get_status	= at803x_cable_test_get_status,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* Qualcomm Atheros AR8030 */
 	.phy_id			= ATH8030_PHY_ID,
@@ -1351,6 +1413,7 @@ static struct phy_driver at803x_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* Qualcomm Atheros AR8031/AR8033 */
 	PHY_ID_MATCH_EXACT(ATH8031_PHY_ID),
@@ -1375,6 +1438,8 @@ static struct phy_driver at803x_driver[] = {
 	.set_tunable		= at803x_set_tunable,
 	.cable_test_start	= at803x_cable_test_start,
 	.cable_test_get_status	= at803x_cable_test_get_status,
+	.config_inband_aneg	= at803x_config_inband_aneg,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* Qualcomm Atheros AR8032 */
 	PHY_ID_MATCH_EXACT(ATH8032_PHY_ID),
@@ -1393,6 +1458,7 @@ static struct phy_driver at803x_driver[] = {
 	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
 	.cable_test_get_status	= at803x_cable_test_get_status,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* ATHEROS AR9331 */
 	PHY_ID_MATCH_EXACT(ATH9331_PHY_ID),
@@ -1408,6 +1474,7 @@ static struct phy_driver at803x_driver[] = {
 	.read_status		= at803x_read_status,
 	.soft_reset		= genphy_soft_reset,
 	.config_aneg		= at803x_config_aneg,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* QCA8337 */
 	.phy_id			= QCA8337_PHY_ID,
@@ -1423,6 +1490,7 @@ static struct phy_driver at803x_driver[] = {
 	.get_stats		= at803x_get_stats,
 	.suspend		= genphy_suspend,
 	.resume			= genphy_resume,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* QCA8327-A from switch QCA8327-AL1A */
 	.phy_id			= QCA8327_A_PHY_ID,
@@ -1438,6 +1506,7 @@ static struct phy_driver at803x_driver[] = {
 	.get_stats		= at803x_get_stats,
 	.suspend		= genphy_suspend,
 	.resume			= genphy_resume,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* QCA8327-B from switch QCA8327-BL1A */
 	.phy_id			= QCA8327_B_PHY_ID,
@@ -1453,6 +1522,7 @@ static struct phy_driver at803x_driver[] = {
 	.get_stats		= at803x_get_stats,
 	.suspend		= genphy_suspend,
 	.resume			= genphy_resume,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, };
 
 module_phy_driver(at803x_driver);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c81c6554d564..de90b22f014d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -584,6 +584,7 @@ struct phy_device {
 	unsigned mac_managed_pm:1;
 
 	unsigned autoneg:1;
+	unsigned inband_an:1;
 	/* The most recently read link state */
 	unsigned link:1;
 	unsigned autoneg_complete:1;
-- 
2.25.1

Powered by blists - more mailing lists