[PATCH net-next v2 0/2] Add auto-negotiation support for LAN887x T1 phy

[PATCH net-next v2 0/2] Add auto-negotiation support for LAN887x T1 phy

[Tarun Alle]

Adds support for auto-negotiation and also phy library
changes for auto-negotiation.

Tarun Alle (2):
  net: phy: phy-c45: Auto-negotiation restart status check for T1 phy
  net: phy: microchip_t1: Auto-negotiation support for LAN887x

 drivers/net/phy/microchip_t1.c | 159 +++++++++++++++++++++++++++------
 drivers/net/phy/phy-c45.c      |   5 +-
 2 files changed, 136 insertions(+), 28 deletions(-)

-- 
2.34.1

[PATCH net-next v2 1/2] net: phy: phy-c45: Auto-negotiation restart status check for T1 phy

[Tarun Alle]

Add support for auto-negotiation restart status check for T1 phys.

Signed-off-by: Tarun Alle <Tarun.Alle@microchip.com>
---
v1 -> v2
- Separated out the fix patch.
- Changed the commit message.
---
 drivers/net/phy/phy-c45.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 0dac08e85304..58be2d534b5c 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -418,11 +418,14 @@ EXPORT_SYMBOL_GPL(genphy_c45_aneg_done);
 int genphy_c45_read_link(struct phy_device *phydev)
 {
 	u32 mmd_mask = MDIO_DEVS_PMAPMD;
+	u16 reg = MDIO_CTRL1;
 	int val, devad;
 	bool link = true;
 
 	if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
-		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
+		if (genphy_c45_baset1_able(phydev))
+			reg = MDIO_AN_T1_CTRL;
+		val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
 		if (val < 0)
 			return val;
 
-- 
2.34.1

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

[Tarun Alle]

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

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,
-- 
2.34.1

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

[Andrew Lunn]

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

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

[Tarun.Alle]

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.

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

[Andrew Lunn]

> 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