diff mbox series

[net-next,v2,2/2] net: phy: microchip_t1: Auto-negotiation support for LAN887x

Message ID 20241216155830.501596-3-Tarun.Alle@microchip.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Add auto-negotiation support for LAN887x T1 phy | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 5 this patch: 5
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-12-18--03-01 (tests: 878)

Commit Message

Tarun Alle Dec. 16, 2024, 3:58 p.m. UTC 
Adds auto-negotiation support for lan887x T1 phy. After this commit,
by default auto-negotiation is on and default advertised speed is 1000M.

Signed-off-by: Tarun Alle <Tarun.Alle@microchip.com>
---
v1 -> v2
- Changed the commit message.
- Elaborated the errata messages.
- Added helper functions for lan887x_100M_setup.
---
 drivers/net/phy/microchip_t1.c | 159 +++++++++++++++++++++++++++------
 1 file changed, 132 insertions(+), 27 deletions(-)

Comments

Andrew Lunn Dec. 16, 2024, 11:39 p.m. UTC | #1 
On Mon, Dec 16, 2024 at 09:28:30PM +0530, Tarun Alle wrote:
> Adds auto-negotiation support for lan887x T1 phy. After this commit,
> by default auto-negotiation is on and default advertised speed is 1000M.

So i asked about the implications of this. I would of expected
something like:

This will break any system which expects forced behaviour, without
actually configuring forced behaviour both on the local system and
where the link partner is expecting forced configuration, not autoneg.

I think we also need some more details about the autoneg in the commit
message. When used against a standards conforming 100M PHY,
negotiation will fail by default, because this PHY is not conformant
with 100M, or 1G autoneg.

I don't like you are going to cause regressions, especially when you
have decided regressions are worth it for a half broken autoneg.

I actually think it should default to fixed, as it is today. Maybe
with the option to enable the broken autoneg. This is different to all
PHYs we have today, but we try hard to avoid regressions.

What are the plans for this PHY? Will there be a new revision soon
which fixes the broken autoneg? Maybe you should forget about autoneg
for this revision of this PHY, it is too broken, and wait for the next
revision which actually conforms to the standard?

	Andrew
Tarun Alle Dec. 17, 2024, 9 a.m. UTC | #2 
Hi Andrew,

Thanks for the review.

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, December 17, 2024 5:10 AM
> To: Tarun Alle - I68638 <Tarun.Alle@microchip.com>
> Cc: Arun Ramadoss - I17769 <Arun.Ramadoss@microchip.com>;
> UNGLinuxDriver <UNGLinuxDriver@microchip.com>; hkallweit1@gmail.com;
> linux@armlinux.org.uk; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH net-next v2 2/2] net: phy: microchip_t1: Auto-negotiation
> support for LAN887x
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Mon, Dec 16, 2024 at 09:28:30PM +0530, Tarun Alle wrote:
> > Adds auto-negotiation support for lan887x T1 phy. After this commit,
> > by default auto-negotiation is on and default advertised speed is 1000M.
> 
> So i asked about the implications of this. I would of expected something like:
> 
> This will break any system which expects forced behaviour, without actually
> configuring forced behaviour both on the local system and where the link
> partner is expecting forced configuration, not autoneg.
> 

We confirmed that there are no customers who are directly using the net-next. 
Hence, we are setting this to default auto-neg which is also chip default. But if
any regressions on T1PHYs are dependent,  we will address this default setting.

> I think we also need some more details about the autoneg in the commit
> message. When used against a standards conforming 100M PHY, negotiation
> will fail by default, because this PHY is not conformant with 100M, or 1G
> autoneg.
> 

I should have given the same errata details in the commit message. Will take care.

> I don't like you are going to cause regressions, especially when you have decided
> regressions are worth it for a half broken autoneg.
> 
> I actually think it should default to fixed, as it is today. Maybe with the option to
> enable the broken autoneg. This is different to all PHYs we have today, but we try
> hard to avoid regressions.
> 
> What are the plans for this PHY? Will there be a new revision soon which fixes
> the broken autoneg? Maybe you should forget about autoneg for this revision
> of this PHY, it is too broken, and wait for the next revision which actually
> conforms to the standard?
> 

I understand your point and I agree with you. We can drop this patch for this chip 
revision as we have plans for new revision.

>         Andrew

Thanks,
Tarun Alle.
Andrew Lunn Dec. 17, 2024, 10:34 a.m. UTC | #3 
> We confirmed that there are no customers who are directly using the net-next. 
> Hence, we are setting this to default auto-neg which is also chip default. But if
> any regressions on T1PHYs are dependent,  we will address this default setting.

So this needs to be communicated, to avoid this sort of back and forth
with emails. It is not the first time we have changed a default like
this, after asking the early adopters if it will be an issue, but we
need to make it clear we have done our due diligence before making a
breaking change.

> > I think we also need some more details about the autoneg in the commit
> > message. When used against a standards conforming 100M PHY, negotiation
> > will fail by default, because this PHY is not conformant with 100M, or 1G
> > autoneg.
> 
> I should have given the same errata details in the commit message. Will take care.
> 
> > I don't like you are going to cause regressions, especially when you have decided
> > regressions are worth it for a half broken autoneg.
> > 
> > I actually think it should default to fixed, as it is today. Maybe with the option to
> > enable the broken autoneg. This is different to all PHYs we have today, but we try
> > hard to avoid regressions.
> > 
> > What are the plans for this PHY? Will there be a new revision soon which fixes
> > the broken autoneg? Maybe you should forget about autoneg for this revision
> > of this PHY, it is too broken, and wait for the next revision which actually
> > conforms to the standard?
> > 
> 
> I understand your point and I agree with you. We can drop this patch for this chip 
> revision as we have plans for new revision.

I would probably drop this patch if the new revision is coming soon.

	Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
index b17bf6708003..694e001f8a15 100644
--- a/drivers/net/phy/microchip_t1.c
+++ b/drivers/net/phy/microchip_t1.c
@@ -268,6 +268,11 @@ 
 /* End offset of samples */
 #define SQI_INLIERS_END (SQI_INLIERS_START + SQI_INLIERS_NUM)
 
+#define LAN887X_VEND_CTRL_STAT_REG		0x8013
+#define LAN887X_AN_LOCAL_CFG_FAULT		BIT(10)
+#define LAN887X_AN_LOCAL_SLAVE			BIT(9)
+#define LAN887X_AN_LOCAL_MASTER			BIT(8)
+
 #define DRIVER_AUTHOR	"Nisar Sayed <nisar.sayed@microchip.com>"
 #define DRIVER_DESC	"Microchip LAN87XX/LAN937x/LAN887x T1 PHY driver"
 
@@ -1259,11 +1264,6 @@  static int lan887x_get_features(struct phy_device *phydev)
 	/* Enable twisted pair */
 	linkmode_set_bit(ETHTOOL_LINK_MODE_TP_BIT, phydev->supported);
 
-	/* First patch only supports 100Mbps and 1000Mbps force-mode.
-	 * T1 Auto-Negotiation (Clause 98 of IEEE 802.3) will be added later.
-	 */
-	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
-
 	return 0;
 }
 
@@ -1342,28 +1342,44 @@  static int lan887x_phy_setup(struct phy_device *phydev)
 	return lan887x_phy_config(phydev, phy_cfg, ARRAY_SIZE(phy_cfg));
 }
 
+static int lan887x_100M_forced_slave_setup(struct phy_device *phydev)
+{
+	static const struct lan887x_regwr_map phy_cfg[] = {
+		{MDIO_MMD_PMAPMD, LAN887X_AFE_PORT_TESTBUS_CTRL4,
+		 0x0038},
+		{MDIO_MMD_VEND1, LAN887X_INIT_COEFF_DFE1_100,
+		 0x0014},
+	};
+
+	return lan887x_phy_config(phydev, phy_cfg, ARRAY_SIZE(phy_cfg));
+}
+
+static int lan887x_100M_common_setup(struct phy_device *phydev)
+{
+	static const struct lan887x_regwr_map phy_comm_cfg[] = {
+		{MDIO_MMD_PMAPMD, LAN887X_AFE_PORT_TESTBUS_CTRL4, 0x00b8},
+		{MDIO_MMD_PMAPMD, LAN887X_TX_AMPLT_1000T1_REG, 0x0038},
+		{MDIO_MMD_VEND1,  LAN887X_INIT_COEFF_DFE1_100, 0x000f},
+	};
+
+	return lan887x_phy_config(phydev, phy_comm_cfg,
+				  ARRAY_SIZE(phy_comm_cfg));
+}
+
 static int lan887x_100M_setup(struct phy_device *phydev)
 {
+	bool is_master;
 	int ret;
 
+	is_master = (phydev->master_slave_set == MASTER_SLAVE_CFG_MASTER_FORCE ||
+		     phydev->master_slave_set == MASTER_SLAVE_CFG_MASTER_PREFERRED);
+
 	/* (Re)configure the speed/mode dependent T1 settings */
-	if (phydev->master_slave_set == MASTER_SLAVE_CFG_MASTER_FORCE ||
-	    phydev->master_slave_set == MASTER_SLAVE_CFG_MASTER_PREFERRED){
-		static const struct lan887x_regwr_map phy_cfg[] = {
-			{MDIO_MMD_PMAPMD, LAN887X_AFE_PORT_TESTBUS_CTRL4, 0x00b8},
-			{MDIO_MMD_PMAPMD, LAN887X_TX_AMPLT_1000T1_REG, 0x0038},
-			{MDIO_MMD_VEND1,  LAN887X_INIT_COEFF_DFE1_100, 0x000f},
-		};
-
-		ret = lan887x_phy_config(phydev, phy_cfg, ARRAY_SIZE(phy_cfg));
-	} else {
-		static const struct lan887x_regwr_map phy_cfg[] = {
-			{MDIO_MMD_PMAPMD, LAN887X_AFE_PORT_TESTBUS_CTRL4, 0x0038},
-			{MDIO_MMD_VEND1, LAN887X_INIT_COEFF_DFE1_100, 0x0014},
-		};
+	if (phydev->autoneg == AUTONEG_ENABLE || is_master)
+		ret = lan887x_100M_common_setup(phydev);
+	else
+		ret = lan887x_100M_forced_slave_setup(phydev);
 
-		ret = lan887x_phy_config(phydev, phy_cfg, ARRAY_SIZE(phy_cfg));
-	}
 	if (ret < 0)
 		return ret;
 
@@ -1384,8 +1400,16 @@  static int lan887x_1000M_setup(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	return phy_set_bits_mmd(phydev, MDIO_MMD_PMAPMD, LAN887X_DSP_PMA_CONTROL,
-				LAN887X_DSP_PMA_CONTROL_LNK_SYNC);
+	if (phydev->autoneg == AUTONEG_ENABLE)
+		ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
+				       LAN887X_REG_REG26,
+				       LAN887X_REG_REG26_HW_INIT_SEQ_EN);
+	else
+		ret = phy_set_bits_mmd(phydev, MDIO_MMD_PMAPMD,
+				       LAN887X_DSP_PMA_CONTROL,
+				       LAN887X_DSP_PMA_CONTROL_LNK_SYNC);
+
+	return ret;
 }
 
 static int lan887x_link_setup(struct phy_device *phydev)
@@ -1407,6 +1431,11 @@  static int lan887x_phy_reset(struct phy_device *phydev)
 {
 	int ret, val;
 
+	/* Disable aneg */
+	ret = genphy_c45_an_disable_aneg(phydev);
+	if (ret < 0)
+		return ret;
+
 	/* Clear 1000M link sync */
 	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_PMAPMD, LAN887X_DSP_PMA_CONTROL,
 				 LAN887X_DSP_PMA_CONTROL_LNK_SYNC);
@@ -1435,23 +1464,71 @@  static int lan887x_phy_reset(struct phy_device *phydev)
 				    5000, 10000, true);
 }
 
+/* LAN887X Errata: The device may not link in auto-neg when both
+ * 100BASE-T1 and 1000BASE-T1 are advertised. Hence advertising
+ * only one speed. In this case auto-neg to determine Leader/Follower.
+ */
+static int lan887x_config_advert(struct phy_device *phydev)
+{
+	linkmode_and(phydev->advertising, phydev->advertising,
+		     phydev->supported);
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
+			      phydev->advertising)) {
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
+				   phydev->advertising);
+		phydev->speed = SPEED_1000;
+	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
+				     phydev->advertising)) {
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
+				   phydev->advertising);
+		phydev->speed = SPEED_100;
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int lan887x_phy_reconfig(struct phy_device *phydev)
 {
 	int ret;
 
-	linkmode_zero(phydev->advertising);
+	if (phydev->autoneg == AUTONEG_ENABLE)
+		ret = genphy_c45_an_config_aneg(phydev);
+	else
+		ret = genphy_c45_pma_setup_forced(phydev);
+	if (ret < 0)
+		return ret;
 
-	ret = genphy_c45_pma_setup_forced(phydev);
+	/* For link to comeup, (re)configure the speed/mode
+	 * dependent T1 settings
+	 */
+	ret = lan887x_link_setup(phydev);
 	if (ret < 0)
 		return ret;
 
-	return lan887x_link_setup(phydev);
+	/* Autoneg to be re-started only after all settings are done */
+	if (phydev->autoneg == AUTONEG_ENABLE) {
+		ret = genphy_c45_restart_aneg(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
 }
 
 static int lan887x_config_aneg(struct phy_device *phydev)
 {
 	int ret;
 
+	/* Reject the not support advertisement settings */
+	if (phydev->autoneg == AUTONEG_ENABLE) {
+		ret  = lan887x_config_advert(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
 	/* LAN887x Errata: speed configuration changes require soft reset
 	 * and chip soft reset
 	 */
@@ -2058,6 +2135,34 @@  static int lan887x_get_sqi(struct phy_device *phydev)
 	return FIELD_GET(T1_DCQ_SQI_MSK, rc);
 }
 
+static int lan887x_read_status(struct phy_device *phydev)
+{
+	int rc;
+
+	phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
+
+	rc = genphy_c45_read_status(phydev);
+	if (rc < 0)
+		return rc;
+
+	if (phydev->autoneg == AUTONEG_ENABLE) {
+		/* Fetch resolved mode */
+		rc = phy_read_mmd(phydev, MDIO_MMD_AN,
+				  LAN887X_VEND_CTRL_STAT_REG);
+		if (rc < 0)
+			return rc;
+
+		if (rc & LAN887X_AN_LOCAL_MASTER)
+			phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
+		else if (rc & LAN887X_AN_LOCAL_SLAVE)
+			phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
+		else if (rc & LAN887X_AN_LOCAL_CFG_FAULT)
+			phydev->master_slave_state = MASTER_SLAVE_STATE_ERR;
+	}
+
+	return 0;
+}
+
 static struct phy_driver microchip_t1_phy_driver[] = {
 	{
 		PHY_ID_MATCH_MODEL(PHY_ID_LAN87XX),
@@ -2106,7 +2211,7 @@  static struct phy_driver microchip_t1_phy_driver[] = {
 		.get_strings    = lan887x_get_strings,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
-		.read_status	= genphy_c45_read_status,
+		.read_status	= lan887x_read_status,
 		.cable_test_start = lan887x_cable_test_start,
 		.cable_test_get_status = lan887x_cable_test_get_status,
 		.config_intr    = lan887x_config_intr,