Skip to contentSkip to conversation

phylib: add hardware resolved pause mode support for marvell PHYs

PATCH New Russell King - ARM Linux admin 3 participants 8 messages Feb 17, 2020 – Feb 18, 2020

Subsystems (4)
Networking Network drivers Ethernet PHY Marvell MV88X3310 PHY driver
lore
Share & tools
lore

Russell King - ARM Linux admin

author maintainer
#0 lore raw

cover phylib: add hardware resolved pause mode support for marvell PHYs

Hi,

Marvell PHYs provide a register that gives us the resolution results for speed, duplex and the pause enablement status. This series adds support for using this, and passing it through phylib via the newly introduced phy_get_pause() interface.

In order to do this, we need to read the copper PHY results from the correct register; now that I have access to the Marvell documentation, this becomes much easier for the 88x3310 PHY.

I also find that the code in the marvell 1G driver does not correctly check that the resolved state in the status register is valid prior to using it, so there is a patch for that too; I don't deem it important enough for it to be sent via the -net tree, which would add additional dependency complexity for this series.

 drivers/net/phy/marvell.c    |  46 +++++++++++++-
 drivers/net/phy/marvell10g.c | 147 ++++++++++++++++++++++++++++---------------
 drivers/net/phy/phy_device.c |   6 ++
 include/linux/phy.h          |   9 +++
 4 files changed, 154 insertions(+), 54 deletions(-)
Signature · 3 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
#1 lore raw

PATCH 1/4 net: phy: marvell10g: read copper results from CSSR1

Read the copper autonegotiation results from the copper specific status register, rather than decoding the advertisements. Reading what the link is actually doing will allow us to support downshift modes.

Signed-off-by Russell King <[email protected]>
---
 drivers/net/phy/marvell10g.c | 141 ++++++++++++++++++++++-------------
 1 file changed, 89 insertions(+), 52 deletions(-)
drivers/net/phy/marvell10g.c +89 -52
index 64c9f3bba2cd..9a4e12a2af07 100644
@@ -39,10 +39,19 @@ enum {
 	MV_PCS_BASE_R		= 0x1000,
 	MV_PCS_1000BASEX	= 0x2000,
 
-	MV_PCS_PAIRSWAP		= 0x8182,
-	MV_PCS_PAIRSWAP_MASK	= 0x0003,
-	MV_PCS_PAIRSWAP_AB	= 0x0002,
-	MV_PCS_PAIRSWAP_NONE	= 0x0003,
+	MV_PCS_CSSR1		= 0x8008,
+	MV_PCS_CSSR1_SPD1_MASK	= 0xc000,
+	MV_PCS_CSSR1_SPD1_SPD2	= 0xc000,
+	MV_PCS_CSSR1_SPD1_1000	= 0x8000,
+	MV_PCS_CSSR1_SPD1_100	= 0x4000,
+	MV_PCS_CSSR1_SPD1_10	= 0x0000,
+	MV_PCS_CSSR1_DUPLEX_FULL= BIT(13),
+	MV_PCS_CSSR1_RESOLVED	= BIT(11),
+	MV_PCS_CSSR1_MDIX	= BIT(6),
+	MV_PCS_CSSR1_SPD2_MASK	= 0x000c,
+	MV_PCS_CSSR1_SPD2_5000	= 0x0008,
+	MV_PCS_CSSR1_SPD2_2500	= 0x0004,
+	MV_PCS_CSSR1_SPD2_10000	= 0x0000,
 
 	/* These registers appear at 0x800X and 0xa00X - the 0xa00X control
 	 * registers appear to set themselves to the 0x800X when AN is

@@ -413,35 +422,18 @@ static void mv3310_update_interface(struct phy_device *phydev)
 }
 
 /* 10GBASE-ER,LR,LRM,SR do not support autonegotiation. */
-static int mv3310_read_10gbr_status(struct phy_device *phydev)
+static int mv3310_read_status_10gbaser(struct phy_device *phydev)
 {
 	phydev->link = 1;
 	phydev->speed = SPEED_10000;
 	phydev->duplex = DUPLEX_FULL;
 
-	mv3310_update_interface(phydev);
-
 	return 0;
 }
 
-static int mv3310_read_status(struct phy_device *phydev)
+static int mv3310_read_status_copper(struct phy_device *phydev)
 {
-	int val;
-
-	phydev->speed = SPEED_UNKNOWN;
-	phydev->duplex = DUPLEX_UNKNOWN;
-	linkmode_zero(phydev->lp_advertising);
-	phydev->link = 0;
-	phydev->pause = 0;
-	phydev->asym_pause = 0;
-	phydev->mdix = 0;
-
-	val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_STAT1);
-	if (val < 0)
-		return val;
-
-	if (val & MDIO_STAT1_LSTATUS)
-		return mv3310_read_10gbr_status(phydev);
+	int cssr1, speed, val;
 
 	val = genphy_c45_read_link(phydev);
 	if (val < 0)

@@ -451,6 +443,52 @@ static int mv3310_read_status(struct phy_device *phydev)
 	if (val < 0)
 		return val;
 
+	cssr1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_CSSR1);
+	if (cssr1 < 0)
+		return val;
+
+	/* If the link settings are not resolved, mark the link down */
+	if (!(cssr1 & MV_PCS_CSSR1_RESOLVED)) {
+		phydev->link = 0;
+		return 0;
+	}
+
+	/* Read the copper link settings */
+	speed = cssr1 & MV_PCS_CSSR1_SPD1_MASK;
+	if (speed == MV_PCS_CSSR1_SPD1_SPD2)
+		speed |= cssr1 & MV_PCS_CSSR1_SPD2_MASK;
+
+	switch (speed) {
+	case MV_PCS_CSSR1_SPD1_SPD2 | MV_PCS_CSSR1_SPD2_10000:
+		phydev->speed = SPEED_10000;
+		break;
+
+	case MV_PCS_CSSR1_SPD1_SPD2 | MV_PCS_CSSR1_SPD2_5000:
+		phydev->speed = SPEED_5000;
+		break;
+
+	case MV_PCS_CSSR1_SPD1_SPD2 | MV_PCS_CSSR1_SPD2_2500:
+		phydev->speed = SPEED_2500;
+		break;
+
+	case MV_PCS_CSSR1_SPD1_1000:
+		phydev->speed = SPEED_1000;
+		break;
+
+	case MV_PCS_CSSR1_SPD1_100:
+		phydev->speed = SPEED_100;
+		break;
+
+	case MV_PCS_CSSR1_SPD1_10:
+		phydev->speed = SPEED_10;
+		break;
+	}
+
+	phydev->duplex = cssr1 & MV_PCS_CSSR1_DUPLEX_FULL ?
+			 DUPLEX_FULL : DUPLEX_HALF;
+	phydev->mdix = cssr1 & MV_PCS_CSSR1_MDIX ?
+		       ETH_TP_MDI_X : ETH_TP_MDI;
+
 	if (val & MDIO_AN_STAT1_COMPLETE) {
 		val = genphy_c45_read_lpa(phydev);
 		if (val < 0)

@@ -463,39 +501,38 @@ static int mv3310_read_status(struct phy_device *phydev)
 
 		mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, val);
 
-		if (phydev->autoneg == AUTONEG_ENABLE)
-			phy_resolve_aneg_linkmode(phydev);
+		/* Update the pause status */
+		phy_resolve_aneg_pause(phydev);
 	}
 
-	if (phydev->autoneg != AUTONEG_ENABLE) {
-		val = genphy_c45_read_pma(phydev);
-		if (val < 0)
-			return val;
-	}
+	return 0;
+}
 
-	if (phydev->speed == SPEED_10000) {
-		val = genphy_c45_read_mdix(phydev);
-		if (val < 0)
-			return val;
-	} else {
-		val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_PAIRSWAP);
-		if (val < 0)
-			return val;
+static int mv3310_read_status(struct phy_device *phydev)
+{
+	int err, val;
 
-		switch (val & MV_PCS_PAIRSWAP_MASK) {
-		case MV_PCS_PAIRSWAP_AB:
-			phydev->mdix = ETH_TP_MDI_X;
-			break;
-		case MV_PCS_PAIRSWAP_NONE:
-			phydev->mdix = ETH_TP_MDI;
-			break;
-		default:
-			phydev->mdix = ETH_TP_MDI_INVALID;
-			break;
-		}
-	}
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	linkmode_zero(phydev->lp_advertising);
+	phydev->link = 0;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+	phydev->mdix = ETH_TP_MDI_INVALID;
 
-	mv3310_update_interface(phydev);
+	val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_STAT1);
+	if (val < 0)
+		return val;
+
+	if (val & MDIO_STAT1_LSTATUS)
+		err = mv3310_read_status_10gbaser(phydev);
+	else
+		err = mv3310_read_status_copper(phydev);
+	if (err < 0)
+		return err;
+
+	if (phydev->link)
+		mv3310_update_interface(phydev);
 
 	return 0;
 }
Signature · 1 line
2.20.1
#3 lore raw

PATCH 2/4 net: phy: marvell: don't interpret PHY status unless resolved

In reply to Russell King - ARM Linux admin

Don't attempt to interpret the PHY specific status register unless the PHY is indicating that the resolution is valid.

Signed-off-by Russell King <[email protected]>
---
 drivers/net/phy/marvell.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/net/phy/marvell.c +5 -0
index 28e33ece4ce1..9a8badafea8a 100644
@@ -1306,6 +1306,9 @@ static int marvell_read_status_page_an(struct phy_device *phydev,
 		}
 	}
 
+	if (!(status & MII_M1011_PHY_STATUS_RESOLVED))
+		return 0;
+
 	if (status & MII_M1011_PHY_STATUS_FULLDUPLEX)
 		phydev->duplex = DUPLEX_FULL;
 	else

@@ -1365,6 +1368,8 @@ static int marvell_read_status_page(struct phy_device *phydev, int page)
 	linkmode_zero(phydev->lp_advertising);
 	phydev->pause = 0;
 	phydev->asym_pause = 0;
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
 
 	if (phydev->autoneg == AUTONEG_ENABLE)
 		err = marvell_read_status_page_an(phydev, fiber, status);
Signature · 1 line
2.20.1
#5 lore raw

PATCH 3/4 net: phy: add resolved pause support

In reply to Russell King - ARM Linux admin

Allow phylib drivers to pass the hardware-resolved pause state to MAC drivers, rather than using the software-based pause resolution code.

Signed-off-by Russell King <[email protected]>
---
 drivers/net/phy/phy_device.c | 6 ++++++
 include/linux/phy.h          | 9 +++++++++
 2 files changed, 15 insertions(+)
drivers/net/phy/phy_device.c +6 -0
index 2a973265de80..6e1b50defd62 100644
@@ -2414,6 +2414,12 @@ void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause)
 		return;
 	}
 
+	if (phydev->resolved_pause_valid) {
+		*tx_pause = phydev->resolved_tx_pause;
+		*rx_pause = phydev->resolved_rx_pause;
+		return;
+	}
+
 	return linkmode_resolve_pause(phydev->advertising,
 				      phydev->lp_advertising,
 				      tx_pause, rx_pause);
include/linux/phy.h +9 -0
index 80f8b2158271..fda23d95e640 100644
@@ -422,6 +422,15 @@ struct phy_device {
 	int pause;
 	int asym_pause;
 
+	/*
+	 * private to phylib: the resolved pause state - only valid if
+	 * resolved_pause_valid is true. only phy drivers and phylib
+	 * should touch this.
+	 */
+	bool resolved_pause_valid;
+	bool resolved_tx_pause;
+	bool resolved_rx_pause;
+
 	/* Union of PHY and Attached devices' supported link modes */
 	/* See ethtool.h for more info */
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
Signature · 1 line
2.20.1
#7 lore raw

PATCH 4/4 net: phy: marvell*: add support for hw resolved pause modes

In reply to Russell King - ARM Linux admin

Support reporting the hardware resolved pause enablement states via phylib, overriding our software implementation.

Signed-off-by Russell King <[email protected]>
---
 drivers/net/phy/marvell.c    | 41 ++++++++++++++++++++++++++++++++++--
 drivers/net/phy/marvell10g.c |  6 ++++++
 2 files changed, 45 insertions(+), 2 deletions(-)
drivers/net/phy/marvell.c +39 -2
index 9a8badafea8a..dabf60641b93 100644
@@ -152,6 +152,10 @@
 #define MII_M1011_PHY_STATUS_FULLDUPLEX	0x2000
 #define MII_M1011_PHY_STATUS_RESOLVED	0x0800
 #define MII_M1011_PHY_STATUS_LINK	0x0400
+#define MII_M1111_PHY_STATUS_TX_PAUSE	0x0008
+#define MII_M1111_PHY_STATUS_RX_PAUSE	0x0004
+#define MII_88E151X_PHY_STATUS_TX_PAUSE	0x0200
+#define MII_88E151X_PHY_STATUS_RX_PAUSE	0x0100
 
 #define MII_88E3016_PHY_SPEC_CTRL	0x10
 #define MII_88E3016_DISABLE_SCRAMBLER	0x0200

@@ -188,6 +192,8 @@ struct marvell_priv {
 	u64 stats[ARRAY_SIZE(marvell_hw_stats)];
 	char *hwmon_name;
 	struct device *hwmon_dev;
+	u16 tx_pause_mask;
+	u16 rx_pause_mask;
 };
 
 static int marvell_read_page(struct phy_device *phydev)

@@ -1275,6 +1281,7 @@ static void fiber_lpa_mod_linkmode_lpa_t(unsigned long *advertising, u32 lpa)
 static int marvell_read_status_page_an(struct phy_device *phydev,
 				       int fiber, int status)
 {
+	struct marvell_priv *priv = phydev->priv;
 	int lpa;
 	int err;
 

@@ -1328,6 +1335,11 @@ static int marvell_read_status_page_an(struct phy_device *phydev,
 		break;
 	}
 
+	phydev->resolved_tx_pause = !!(status & priv->tx_pause_mask);
+	phydev->resolved_rx_pause = !!(status & priv->rx_pause_mask);
+	phydev->resolved_pause_valid = !fiber && priv->tx_pause_mask &&
+				       priv->rx_pause_mask;
+
 	return 0;
 }
 

@@ -1370,6 +1382,7 @@ static int marvell_read_status_page(struct phy_device *phydev, int page)
 	phydev->asym_pause = 0;
 	phydev->speed = SPEED_UNKNOWN;
 	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->resolved_pause_valid = false;
 
 	if (phydev->autoneg == AUTONEG_ENABLE)
 		err = marvell_read_status_page_an(phydev, fiber, status);

@@ -2130,6 +2143,23 @@ static int marvell_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int marvell_probe_pause(struct phy_device *phydev, u16 tx_pause_mask,
+			       u16 rx_pause_mask)
+{
+	struct marvell_priv *priv;
+	int err;
+
+	err = marvell_probe(phydev);
+	if (err)
+		return err;
+
+	priv = phydev->priv;
+	priv->tx_pause_mask = tx_pause_mask;
+	priv->rx_pause_mask = rx_pause_mask;
+
+	return 0;
+}
+
 static int m88e1121_probe(struct phy_device *phydev)
 {
 	int err;

@@ -2141,11 +2171,18 @@ static int m88e1121_probe(struct phy_device *phydev)
 	return m88e1121_hwmon_probe(phydev);
 }
 
+static int m88e1111_probe(struct phy_device *phydev)
+{
+	return marvell_probe_pause(phydev, MII_M1111_PHY_STATUS_TX_PAUSE,
+				   MII_M1111_PHY_STATUS_RX_PAUSE);
+}
+
 static int m88e1510_probe(struct phy_device *phydev)
 {
 	int err;
 
-	err = marvell_probe(phydev);
+	err = marvell_probe_pause(phydev, MII_88E151X_PHY_STATUS_TX_PAUSE,
+				  MII_88E151X_PHY_STATUS_RX_PAUSE);
 	if (err)
 		return err;
 

@@ -2208,7 +2245,7 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1111",
 		/* PHY_GBIT_FEATURES */
-		.probe = marvell_probe,
+		.probe = m88e1111_probe,
 		.config_init = &m88e1111_config_init,
 		.config_aneg = &marvell_config_aneg,
 		.read_status = &marvell_read_status,
drivers/net/phy/marvell10g.c +6 -0
index 9a4e12a2af07..b33d75b1d506 100644
@@ -47,6 +47,8 @@ enum {
 	MV_PCS_CSSR1_SPD1_10	= 0x0000,
 	MV_PCS_CSSR1_DUPLEX_FULL= BIT(13),
 	MV_PCS_CSSR1_RESOLVED	= BIT(11),
+	MV_PCS_CSSR1_TX_PAUSE	= BIT(9),
+	MV_PCS_CSSR1_RX_PAUSE	= BIT(8),
 	MV_PCS_CSSR1_MDIX	= BIT(6),
 	MV_PCS_CSSR1_SPD2_MASK	= 0x000c,
 	MV_PCS_CSSR1_SPD2_5000	= 0x0008,

@@ -489,6 +491,10 @@ static int mv3310_read_status_copper(struct phy_device *phydev)
 	phydev->mdix = cssr1 & MV_PCS_CSSR1_MDIX ?
 		       ETH_TP_MDI_X : ETH_TP_MDI;
 
+	phydev->resolved_tx_pause = !!(cssr1 & MV_PCS_CSSR1_TX_PAUSE);
+	phydev->resolved_rx_pause = !!(cssr1 & MV_PCS_CSSR1_RX_PAUSE);
+	phydev->resolved_pause_valid = true;
+
 	if (val & MDIO_AN_STAT1_COMPLETE) {
 		val = genphy_c45_read_lpa(phydev);
 		if (val < 0)
Signature · 1 line
2.20.1
maintainer Reviewed-by on v1 1/4
#2 lore raw

Re: [PATCH net-next 1/4] net: phy: marvell10g: read copper results from CSSR1

In reply to Russell King
Quoted 6 lines · Russell King

Read the copper autonegotiation results from the copper specific status register, rather than decoding the advertisements. Reading what the link is actually doing will allow us to support downshift modes.

Signed-off-by Russell King <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>

Andrew

maintainer
#6 lore raw

Re: [PATCH net-next 3/4] net: phy: add resolved pause support

In reply to Russell King
Quoted 2 lines · Russell King

Allow phylib drivers to pass the hardware-resolved pause state to MAC drivers, rather than using the software-based pause resolution code.

Hi Russell

What i find missing here is an explanation of why hardware resolved pause is better than software. We know what the software based method is, and it will be consistent across all hardware. Can we say that about the hardware?

Thanks
Andrew