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

[PATCH net-next 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 changes for T1 phy in phy library
  net: phy: microchip_t1: Auto-negotiation support for LAN887x T1 phy

 drivers/net/phy/microchip_t1.c | 147 +++++++++++++++++++++++++++------
 drivers/net/phy/phy-c45.c      |  36 +++++---
 2 files changed, 147 insertions(+), 36 deletions(-)

-- 
2.34.1

[PATCH net-next 1/2] net: phy: phy-c45: Auto-negotiaion changes for T1 phy in phy library

[Tarun Alle]

Below auto-negotiation library changes required for T1 phys:
- Lower byte advertisement register need to read after higher byte as
  per 802.3-2022 : Section 45.2.7.22.
- Link status need to be get from control T1 registers for T1 phys.

Signed-off-by: Tarun Alle <Tarun.Alle@microchip.com>
---
 drivers/net/phy/phy-c45.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 0dac08e85304..85d8a9b9c3f6 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -234,15 +234,11 @@ static int genphy_c45_baset1_an_config_aneg(struct phy_device *phydev)
 		return -EOPNOTSUPP;
 	}
 
-	adv_l |= linkmode_adv_to_mii_t1_adv_l_t(phydev->advertising);
-
-	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L,
-				     adv_l_mask, adv_l);
-	if (ret < 0)
-		return ret;
-	if (ret > 0)
-		changed = 1;
-
+	/* Ref. 802.3-2022 : Section 45.2.7.22
+	 * The Base Page value is transferred to mr_adv_ability when register
+	 * 7.514 is written.
+	 * Therefore, registers 7.515 and 7.516 should be written before 7.514.
+	 */
 	adv_m |= linkmode_adv_to_mii_t1_adv_m_t(phydev->advertising);
 
 	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_M,
@@ -252,6 +248,23 @@ static int genphy_c45_baset1_an_config_aneg(struct phy_device *phydev)
 	if (ret > 0)
 		changed = 1;
 
+	adv_l |= linkmode_adv_to_mii_t1_adv_l_t(phydev->advertising);
+
+	if (changed) {
+		ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L,
+				    adv_l);
+		if (ret < 0)
+			return ret;
+	} else {
+		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN,
+					     MDIO_AN_T1_ADV_L,
+					     adv_l_mask, adv_l);
+		if (ret < 0)
+			return ret;
+		if (ret > 0)
+			changed = 1;
+	}
+
 	return changed;
 }
 
@@ -418,11 +431,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

Re: [PATCH net-next 1/2] net: phy: phy-c45: Auto-negotiaion changes for T1 phy in phy library

[Russell King (Oracle)]

On Mon, Dec 09, 2024 at 09:44:26PM +0530, Tarun Alle wrote:
> Below auto-negotiation library changes required for T1 phys:
> - Lower byte advertisement register need to read after higher byte as
>   per 802.3-2022 : Section 45.2.7.22.

In my copy of 802.3, this refers to the link partner base page
ability register, which is not the same as the advertisement
register.

The advertisement registers are covered by the preceeding section,
45.2.7.21. This says:

"The Base Page value is transferred to mr_adv_ability when register
7.514 is written. Therefore, registers 7.515 and 7.516 should be
written before 7.514."

which I think is what's pertinent to your commit.

> - Link status need to be get from control T1 registers for T1 phys.

This statement appears to be inaccurate - more below against the
actual code change.

> 
> Signed-off-by: Tarun Alle <Tarun.Alle@microchip.com>
> ---
>  drivers/net/phy/phy-c45.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index 0dac08e85304..85d8a9b9c3f6 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -234,15 +234,11 @@ static int genphy_c45_baset1_an_config_aneg(struct phy_device *phydev)
>  		return -EOPNOTSUPP;
>  	}
>  
> -	adv_l |= linkmode_adv_to_mii_t1_adv_l_t(phydev->advertising);
> -
> -	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L,
> -				     adv_l_mask, adv_l);
> -	if (ret < 0)
> -		return ret;
> -	if (ret > 0)
> -		changed = 1;
> -
> +	/* Ref. 802.3-2022 : Section 45.2.7.22
> +	 * The Base Page value is transferred to mr_adv_ability when register
> +	 * 7.514 is written.
> +	 * Therefore, registers 7.515 and 7.516 should be written before 7.514.
> +	 */
>  	adv_m |= linkmode_adv_to_mii_t1_adv_m_t(phydev->advertising);
>  
>  	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_M,
> @@ -252,6 +248,23 @@ static int genphy_c45_baset1_an_config_aneg(struct phy_device *phydev)
>  	if (ret > 0)
>  		changed = 1;
>  
> +	adv_l |= linkmode_adv_to_mii_t1_adv_l_t(phydev->advertising);
> +
> +	if (changed) {
> +		ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L,
> +				    adv_l);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN,
> +					     MDIO_AN_T1_ADV_L,
> +					     adv_l_mask, adv_l);

Why do you write the complete register if changed is true, but only
modify bits 12, 11 and 10 if changed is false?

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

This is not checking link status as you mention in your commit
message, it is checking whether the PHY is in the process of
restarting autoneg.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/2] net: phy: phy-c45: Auto-negotiaion changes for T1 phy in phy library

[Andrew Lunn]

On Mon, Dec 09, 2024 at 09:44:26PM +0530, Tarun Alle wrote:
> Below auto-negotiation library changes required for T1 phys:
> - Lower byte advertisement register need to read after higher byte as
>   per 802.3-2022 : Section 45.2.7.22.

Is this a fix? Does Linux have any T1 PHYs which already support
auto-neg? Either add a comment this is not a fix because...., or
please pull this out into a patch for net, with a Fixes: tag.

	Andrew

RE: [PATCH net-next 1/2] net: phy: phy-c45: Auto-negotiaion changes for T1 phy in phy library

[Tarun.Alle]

Hi Andrew,

Thanks for your review comments.

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, December 9, 2024 10:24 PM
> 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 1/2] net: phy: phy-c45: Auto-negotiaion
> changes for T1 phy in phy library
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the content is safe
> 
> On Mon, Dec 09, 2024 at 09:44:26PM +0530, Tarun Alle wrote:
> > Below auto-negotiation library changes required for T1 phys:
> > - Lower byte advertisement register need to read after higher byte as
> >   per 802.3-2022 : Section 45.2.7.22.
> 
> Is this a fix? Does Linux have any T1 PHYs which already support auto-neg?
> Either add a comment this is not a fix because...., or please pull this out into
> a patch for net, with a Fixes: tag.
> 

There are few implementations for T1 PHY auto-neg. I will pull this out as a fix.

>         Andrew

Thanks,
Tarun Alle.

[PATCH net-next 2/2] net: phy: microchip_t1: Autonegotiaion support for LAN887x T1 phy

[Tarun Alle]

Adds auto-negotiation support for lan887x T1 phy.

Signed-off-by: Tarun Alle <Tarun.Alle@microchip.com>
---
 drivers/net/phy/microchip_t1.c | 147 +++++++++++++++++++++++++++------
 1 file changed, 121 insertions(+), 26 deletions(-)

diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
index b17bf6708003..b8e65cb7d29e 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;
 }
 
@@ -1344,25 +1344,34 @@ static int lan887x_phy_setup(struct phy_device *phydev)
 
 static int lan887x_100M_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},
+	};
 	int ret;
 
 	/* (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));
+	if (phydev->autoneg == AUTONEG_DISABLE) {
+		if (phydev->master_slave_set == MASTER_SLAVE_CFG_MASTER_FORCE ||
+		    phydev->master_slave_set ==
+		    MASTER_SLAVE_CFG_MASTER_PREFERRED) {
+			ret = lan887x_phy_config(phydev, phy_comm_cfg,
+						 ARRAY_SIZE(phy_comm_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},
+			};
+
+			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},
-		};
-
-		ret = lan887x_phy_config(phydev, phy_cfg, ARRAY_SIZE(phy_cfg));
+		ret = lan887x_phy_config(phydev, phy_comm_cfg,
+					 ARRAY_SIZE(phy_comm_cfg));
 	}
 	if (ret < 0)
 		return ret;
@@ -1384,8 +1393,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 +1424,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 +1457,68 @@ static int lan887x_phy_reset(struct phy_device *phydev)
 				    5000, 10000, true);
 }
 
+/* LAN887X Errata: 100M master issue. Dual speed in Aneg is not supported. */
+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 +2125,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 +2201,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 2/2] net: phy: microchip_t1: Autonegotiaion support for LAN887x T1 phy

[Andrew Lunn]

> -	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));
> +	if (phydev->autoneg == AUTONEG_DISABLE) {
> +		if (phydev->master_slave_set == MASTER_SLAVE_CFG_MASTER_FORCE ||
> +		    phydev->master_slave_set ==
> +		    MASTER_SLAVE_CFG_MASTER_PREFERRED) {
> +			ret = lan887x_phy_config(phydev, phy_comm_cfg,
> +						 ARRAY_SIZE(phy_comm_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},
> +			};
> +
> +			ret = lan887x_phy_config(phydev, phy_cfg,
> +						 ARRAY_SIZE(phy_cfg));
> +		}

It might be better to pull this apart into two helper functions? That
would avoid most of the not so nice wrapping.

	Andrew

RE: [PATCH net-next 2/2] net: phy: microchip_t1: Autonegotiaion support for LAN887x T1 phy

[Tarun.Alle]

Hi Andrew,

Thanks for your review comments.

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, December 9, 2024 10:33 PM
> 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 2/2] net: phy: microchip_t1: Autonegotiaion
> support for LAN887x T1 phy
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the content is safe
> 
> > -     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));
> > +     if (phydev->autoneg == AUTONEG_DISABLE) {
> > +             if (phydev->master_slave_set ==
> MASTER_SLAVE_CFG_MASTER_FORCE ||
> > +                 phydev->master_slave_set ==
> > +                 MASTER_SLAVE_CFG_MASTER_PREFERRED) {
> > +                     ret = lan887x_phy_config(phydev, phy_comm_cfg,
> > +                                              ARRAY_SIZE(phy_comm_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},
> > +                     };
> > +
> > +                     ret = lan887x_phy_config(phydev, phy_cfg,
> > +                                              ARRAY_SIZE(phy_cfg));
> > +             }
> 
> It might be better to pull this apart into two helper functions? That would
> avoid most of the not so nice wrapping.
> 

I will implement helper functions for each case.

>         Andrew

Thanks,
Tarun Alle.

Re: [PATCH net-next 2/2] net: phy: microchip_t1: Autonegotiaion support for LAN887x T1 phy

[Andrew Lunn]

> -	/* 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);
> -

What are the backwards compatibility issues here? I would at least
expect some comments in the commit message. As far as i understand, up
until this patch, it always required forced configuration. With this
patch, it suddenly will auto-neg by default? If the link partner is
not expecting auto-neg, that will fail.

> +/* LAN887X Errata: 100M master issue. Dual speed in Aneg is not supported. */

Please could you expand on this. We are now doing auto-neg by default,
and auto-neg is somewhat broken?

	Andrew

RE: [PATCH net-next 2/2] net: phy: microchip_t1: Autonegotiaion support for LAN887x T1 phy

[Tarun.Alle]

Hi Andrew,

Thanks for the review comments.

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, December 9, 2024 10:41 PM
> 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 2/2] net: phy: microchip_t1: Autonegotiaion
> support for LAN887x T1 phy
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the content is safe
> 
> > -     /* 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);
> > -
> 
> What are the backwards compatibility issues here? I would at least expect
> some comments in the commit message. As far as i understand, up until
> this patch, it always required forced configuration. With this patch, it
> suddenly will auto-neg by default? If the link partner is not expecting auto-
> neg, that will fail.
> 

I will update the commit message.

> > +/* LAN887X Errata: 100M master issue. Dual speed in Aneg is not
> > +supported. */
> 
> Please could you expand on this. We are now doing auto-neg by default, and
> auto-neg is somewhat broken?
> 

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.

I will update comment as mentioned above.

>         Andrew

Thanks,
Tarun Alle.