[PATCH] [88q2xxx] Add support for handling master/slave in forced mode

[PATCH] [88q2xxx] Add support for handling master/slave in forced mode

[Ilya A. Evenbach]

88q2xxx PHYs have non-standard way of setting master/slave in
forced mode.
This change adds support for changing and reporting this setting
correctly through ethtool.

Signed-off-by: Ilya A. Evenbach <ievenbach@aurora.tech>
---
 drivers/net/phy/marvell-88q2xxx.c | 107 ++++++++++++++++++++++++++++--
 1 file changed, 102 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index f3d83b04c953..1ab450056e86 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -9,6 +9,7 @@
 #include <linux/ethtool_netlink.h>
 #include <linux/hwmon.h>
 #include <linux/marvell_phy.h>
+#include <linux/mdio.h>
 #include <linux/of.h>
 #include <linux/phy.h>
 
@@ -118,6 +119,11 @@
 #define MV88Q2XXX_LED_INDEX_TX_ENABLE			0
 #define MV88Q2XXX_LED_INDEX_GPIO			1
 
+/* Marvell vendor PMA/PMD control for forced master/slave when AN is disabled */
+#define PMAPMD_MVL_PMAPMD_CTL				0x0834
+#define MASTER_MODE					BIT(14)
+#define MODE_MASK					BIT(14)
+
 struct mv88q2xxx_priv {
 	bool enable_led0;
 };
@@ -377,13 +383,57 @@ static int mv88q2xxx_read_link(struct phy_device *phydev)
 static int mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
 {
 	int ret;
+	int adv_l, adv_m, stat, stat2;
+
+	/* In forced mode, state and config are controlled via PMAPMD 0x834 */
+	if (phydev->autoneg == AUTONEG_DISABLE) {
+		ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, PMAPMD_MVL_PMAPMD_CTL);
+		if (ret < 0)
+			return ret;
+
+		if (ret & MASTER_MODE) {
+			phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
+			phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
+		} else {
+			phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
+			phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_FORCE;
+		}
+		return 0;
+	}
 
-	phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
-	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT);
-	if (ret < 0)
-		return ret;
 
-	if (ret & MDIO_MMD_AN_MV_STAT_LOCAL_MASTER)
+	adv_l = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L);
+	if (adv_l < 0)
+		return adv_l;
+	adv_m = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_M);
+	if (adv_m < 0)
+		return adv_m;
+
+	if (adv_l & MDIO_AN_T1_ADV_L_FORCE_MS)
+		phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
+	else if (adv_m & MDIO_AN_T1_ADV_M_MST)
+		phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_PREFERRED;
+	else
+		phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_PREFERRED;
+
+	stat = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT);
+	if (stat < 0)
+		return stat;
+
+	if (stat & MDIO_MMD_AN_MV_STAT_MS_CONF_FAULT) {
+		phydev->master_slave_state = MASTER_SLAVE_STATE_ERR;
+		return 0;
+	}
+
+	stat2 = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT2);
+	if (stat2 < 0)
+		return stat2;
+	if (!(stat2 & MDIO_MMD_AN_MV_STAT2_AN_RESOLVED)) {
+		phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
+		return 0;
+	}
+
+	if (stat & MDIO_MMD_AN_MV_STAT_LOCAL_MASTER)
 		phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
 	else
 		phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
@@ -391,6 +441,34 @@ static int mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
 	return 0;
 }
 
+static int mv88q2xxx_setup_master_slave_forced(struct phy_device *phydev)
+{
+	int ret = 0;
+
+	switch (phydev->master_slave_set) {
+	case MASTER_SLAVE_CFG_MASTER_FORCE:
+	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
+		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_PMAPMD,
+					     PMAPMD_MVL_PMAPMD_CTL,
+					     MODE_MASK, MASTER_MODE);
+		break;
+	case MASTER_SLAVE_CFG_SLAVE_FORCE:
+	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
+		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_PMAPMD,
+					     PMAPMD_MVL_PMAPMD_CTL,
+					     MODE_MASK, 0);
+		break;
+	case MASTER_SLAVE_CFG_UNKNOWN:
+	case MASTER_SLAVE_CFG_UNSUPPORTED:
+	default:
+		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+		ret = 0;
+		break;
+	}
+
+	return ret;
+}
+
 static int mv88q2xxx_read_aneg_speed(struct phy_device *phydev)
 {
 	int ret;
@@ -448,6 +526,11 @@ static int mv88q2xxx_read_status(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
+	/* Populate master/slave status also for forced modes */
+	ret = mv88q2xxx_read_master_slave_state(phydev);
+	if (ret < 0 && ret != -EOPNOTSUPP)
+		return ret;
+
 	return genphy_c45_read_pma(phydev);
 }
 
@@ -478,6 +561,20 @@ static int mv88q2xxx_config_aneg(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	/* Configure Base-T1 master/slave per phydev->master_slave_set.
+	 * For AN disabled, program PMAPMD role directly; otherwise rely on
+	 * the standard Base-T1 AN advertisement bits.
+	 */
+	if (phydev->autoneg == AUTONEG_DISABLE) {
+		ret = mv88q2xxx_setup_master_slave_forced(phydev);
+		if (ret)
+			return ret;
+	} else {
+		ret = genphy_c45_pma_baset1_setup_master_slave(phydev);
+		if (ret)
+			return ret;
+	}
+
 	return phydev->drv->soft_reset(phydev);
 }
 
-- 
2.34.1

Re: [PATCH] [88q2xxx] Add support for handling master/slave in forced mode

[Andrew Lunn]

On Tue, Aug 19, 2025 at 02:29:01PM -0700, Ilya A. Evenbach wrote:
> 88q2xxx PHYs have non-standard way of setting master/slave in
> forced mode.
> This change adds support for changing and reporting this setting
> correctly through ethtool.

Please could you Cc: Dimitri Fedrau <dima.fedrau@gmail.com>. He has
done most of the work on this driver.

> Signed-off-by: Ilya A. Evenbach <ievenbach@aurora.tech>
> ---
>  drivers/net/phy/marvell-88q2xxx.c | 107 ++++++++++++++++++++++++++++--
>  1 file changed, 102 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index f3d83b04c953..1ab450056e86 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -9,6 +9,7 @@
>  #include <linux/ethtool_netlink.h>
>  #include <linux/hwmon.h>
>  #include <linux/marvell_phy.h>
> +#include <linux/mdio.h>

I'm curious. What is needed from this?

	Andrew

(no subject)

[Ilya A. Evenbach]

> Please could you Cc: Dimitri Fedrau <dima.fedrau@gmail.com>. He has
> done most of the work on this driver.
Done

>> +#include <linux/mdio.h>

> I'm curious. What is needed from this?
Oops. This is, indeed, unneeded. Fixed.

[PATCH] [88q2xxx] Add support for handling master/slave in forced mode

[Ilya A. Evenbach]

88q2xxx PHYs have non-standard way of setting master/slave in
forced mode.
This change adds support for changing and reporting this setting
correctly through ethtool.

Signed-off-by: Ilya A. Evenbach <ievenbach@aurora.tech>
---
 drivers/net/phy/marvell-88q2xxx.c | 106 ++++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index f3d83b04c953..b94d574fd9b7 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -118,6 +118,11 @@
 #define MV88Q2XXX_LED_INDEX_TX_ENABLE			0
 #define MV88Q2XXX_LED_INDEX_GPIO			1
 
+/* Marvell vendor PMA/PMD control for forced master/slave when AN is disabled */
+#define PMAPMD_MVL_PMAPMD_CTL				0x0834
+#define MASTER_MODE					BIT(14)
+#define MODE_MASK					BIT(14)
+
 struct mv88q2xxx_priv {
 	bool enable_led0;
 };
@@ -377,13 +382,57 @@ static int mv88q2xxx_read_link(struct phy_device *phydev)
 static int mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
 {
 	int ret;
+	int adv_l, adv_m, stat, stat2;
+
+	/* In forced mode, state and config are controlled via PMAPMD 0x834 */
+	if (phydev->autoneg == AUTONEG_DISABLE) {
+		ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, PMAPMD_MVL_PMAPMD_CTL);
+		if (ret < 0)
+			return ret;
+
+		if (ret & MASTER_MODE) {
+			phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
+			phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
+		} else {
+			phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
+			phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_FORCE;
+		}
+		return 0;
+	}
 
-	phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
-	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT);
-	if (ret < 0)
-		return ret;
 
-	if (ret & MDIO_MMD_AN_MV_STAT_LOCAL_MASTER)
+	adv_l = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L);
+	if (adv_l < 0)
+		return adv_l;
+	adv_m = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_M);
+	if (adv_m < 0)
+		return adv_m;
+
+	if (adv_l & MDIO_AN_T1_ADV_L_FORCE_MS)
+		phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
+	else if (adv_m & MDIO_AN_T1_ADV_M_MST)
+		phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_PREFERRED;
+	else
+		phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_PREFERRED;
+
+	stat = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT);
+	if (stat < 0)
+		return stat;
+
+	if (stat & MDIO_MMD_AN_MV_STAT_MS_CONF_FAULT) {
+		phydev->master_slave_state = MASTER_SLAVE_STATE_ERR;
+		return 0;
+	}
+
+	stat2 = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT2);
+	if (stat2 < 0)
+		return stat2;
+	if (!(stat2 & MDIO_MMD_AN_MV_STAT2_AN_RESOLVED)) {
+		phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
+		return 0;
+	}
+
+	if (stat & MDIO_MMD_AN_MV_STAT_LOCAL_MASTER)
 		phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
 	else
 		phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
@@ -391,6 +440,34 @@ static int mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
 	return 0;
 }
 
+static int mv88q2xxx_setup_master_slave_forced(struct phy_device *phydev)
+{
+	int ret = 0;
+
+	switch (phydev->master_slave_set) {
+	case MASTER_SLAVE_CFG_MASTER_FORCE:
+	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
+		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_PMAPMD,
+					     PMAPMD_MVL_PMAPMD_CTL,
+					     MODE_MASK, MASTER_MODE);
+		break;
+	case MASTER_SLAVE_CFG_SLAVE_FORCE:
+	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
+		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_PMAPMD,
+					     PMAPMD_MVL_PMAPMD_CTL,
+					     MODE_MASK, 0);
+		break;
+	case MASTER_SLAVE_CFG_UNKNOWN:
+	case MASTER_SLAVE_CFG_UNSUPPORTED:
+	default:
+		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+		ret = 0;
+		break;
+	}
+
+	return ret;
+}
+
 static int mv88q2xxx_read_aneg_speed(struct phy_device *phydev)
 {
 	int ret;
@@ -448,6 +525,11 @@ static int mv88q2xxx_read_status(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
+	/* Populate master/slave status also for forced modes */
+	ret = mv88q2xxx_read_master_slave_state(phydev);
+	if (ret < 0 && ret != -EOPNOTSUPP)
+		return ret;
+
 	return genphy_c45_read_pma(phydev);
 }
 
@@ -478,6 +560,20 @@ static int mv88q2xxx_config_aneg(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	/* Configure Base-T1 master/slave per phydev->master_slave_set.
+	 * For AN disabled, program PMAPMD role directly; otherwise rely on
+	 * the standard Base-T1 AN advertisement bits.
+	 */
+	if (phydev->autoneg == AUTONEG_DISABLE) {
+		ret = mv88q2xxx_setup_master_slave_forced(phydev);
+		if (ret)
+			return ret;
+	} else {
+		ret = genphy_c45_pma_baset1_setup_master_slave(phydev);
+		if (ret)
+			return ret;
+	}
+
 	return phydev->drv->soft_reset(phydev);
 }
 
-- 
2.34.1

Re: [PATCH] [88q2xxx] Add support for handling master/slave in forced mode

[Jakub Kicinski]

A few drive-by comments...

On Wed, 20 Aug 2025 11:11:43 -0700 Ilya A. Evenbach wrote:
> In-Reply-To: <20250820181143.2288755-1-ievenbach@aurora.tech>

Please start a new thread for each new revision.
Change subject to [PATCH vN], where vN is the revision.
Add

v1: https://lore.kernel.org/20250820181143.2288755-2-ievenbach@aurora.tech

under the --- for reference to older versions

> Cc: dima.fedrau@gmail.com, "Ilya A. Evenbach" <ievenbach@aurora.tech>

The cc list is still pretty sparse

> Subject: [PATCH] [88q2xxx] Add support for handling master/slave in forced mode

the [88q2xxx] will be stripped by git when applying. Please look thru
the history of the file and find the correct way to prefix.

> 88q2xxx PHYs have non-standard way of setting master/slave in
> forced mode.
> This change adds support for changing and reporting this setting
> correctly through ethtool.

imperative mood:

 Add support for changing...


> +/* Marvell vendor PMA/PMD control for forced master/slave when AN is disabled */
> +#define PMAPMD_MVL_PMAPMD_CTL				0x0834
> +#define MASTER_MODE					BIT(14)
> +#define MODE_MASK					BIT(14)

Other defines in context seem to be prefixed with  MV88Q2XXX_
please prefer prefixing local defines.

>  struct mv88q2xxx_priv {
>  	bool enable_led0;
>  };
> @@ -377,13 +382,57 @@ static int mv88q2xxx_read_link(struct phy_device *phydev)
>  static int mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
>  {
>  	int ret;
> +	int adv_l, adv_m, stat, stat2;

nit: in networking we like variable declaration lines to be sorted longest to shortest.
-- 
pw-bot: cr

Re: [PATCH] [88q2xxx] Add support for handling master/slave in forced mode

[Dimitri Fedrau]

Hi Ilya,

Am Wed, Aug 20, 2025 at 11:11:43AM -0700 schrieb Ilya A. Evenbach:
> 88q2xxx PHYs have non-standard way of setting master/slave in
> forced mode.
> This change adds support for changing and reporting this setting
> correctly through ethtool.
> 
> Signed-off-by: Ilya A. Evenbach <ievenbach@aurora.tech>
> ---
>  drivers/net/phy/marvell-88q2xxx.c | 106 ++++++++++++++++++++++++++++--
>  1 file changed, 101 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index f3d83b04c953..b94d574fd9b7 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -118,6 +118,11 @@
>  #define MV88Q2XXX_LED_INDEX_TX_ENABLE			0
>  #define MV88Q2XXX_LED_INDEX_GPIO			1
>  
> +/* Marvell vendor PMA/PMD control for forced master/slave when AN is disabled */
> +#define PMAPMD_MVL_PMAPMD_CTL				0x0834

Already defined, see MDIO_PMA_PMD_BT1_CTRL.

> +#define MASTER_MODE					BIT(14)

Already defines, see MDIO_PMA_PMD_BT1_CTRL_CFG_MST.

> +#define MODE_MASK					BIT(14)
> +
>  struct mv88q2xxx_priv {
>  	bool enable_led0;
>  };
> @@ -377,13 +382,57 @@ static int mv88q2xxx_read_link(struct phy_device *phydev)
>  static int mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
>  {
>  	int ret;
> +	int adv_l, adv_m, stat, stat2;
> +
> +	/* In forced mode, state and config are controlled via PMAPMD 0x834 */
> +	if (phydev->autoneg == AUTONEG_DISABLE) {
> +		ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, PMAPMD_MVL_PMAPMD_CTL);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (ret & MASTER_MODE) {
> +			phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
> +			phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
> +		} else {
> +			phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
> +			phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_FORCE;
> +		}
> +		return 0;
> +	}
>  
> -	phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> -	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT);
> -	if (ret < 0)
> -		return ret;
>  
> -	if (ret & MDIO_MMD_AN_MV_STAT_LOCAL_MASTER)
> +	adv_l = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L);
> +	if (adv_l < 0)
> +		return adv_l;
> +	adv_m = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_M);
> +	if (adv_m < 0)
> +		return adv_m;
> +
> +	if (adv_l & MDIO_AN_T1_ADV_L_FORCE_MS)
> +		phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
> +	else if (adv_m & MDIO_AN_T1_ADV_M_MST)
> +		phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_PREFERRED;
> +	else
> +		phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_PREFERRED;
> +
> +	stat = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT);
> +	if (stat < 0)
> +		return stat;
> +
> +	if (stat & MDIO_MMD_AN_MV_STAT_MS_CONF_FAULT) {
> +		phydev->master_slave_state = MASTER_SLAVE_STATE_ERR;
> +		return 0;
> +	}
> +
> +	stat2 = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT2);
> +	if (stat2 < 0)
> +		return stat2;
> +	if (!(stat2 & MDIO_MMD_AN_MV_STAT2_AN_RESOLVED)) {
> +		phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> +		return 0;
> +	}
> +
> +	if (stat & MDIO_MMD_AN_MV_STAT_LOCAL_MASTER)
>  		phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
>  	else
>  		phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
> @@ -391,6 +440,34 @@ static int mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
>  	return 0;
>  }
>  

Is there a issue with the function you are trying to fix ? Seems that
you copied some generic functions into it.

> +static int mv88q2xxx_setup_master_slave_forced(struct phy_device *phydev)
> +{
> +	int ret = 0;
> +
> +	switch (phydev->master_slave_set) {
> +	case MASTER_SLAVE_CFG_MASTER_FORCE:
> +	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> +		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_PMAPMD,
> +					     PMAPMD_MVL_PMAPMD_CTL,
> +					     MODE_MASK, MASTER_MODE);
> +		break;
> +	case MASTER_SLAVE_CFG_SLAVE_FORCE:
> +	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> +		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_PMAPMD,
> +					     PMAPMD_MVL_PMAPMD_CTL,
> +					     MODE_MASK, 0);
> +		break;
> +	case MASTER_SLAVE_CFG_UNKNOWN:
> +	case MASTER_SLAVE_CFG_UNSUPPORTED:
> +	default:
> +		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> +		ret = 0;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +

This function does the same as genphy_c45_pma_baset1_setup_master_slave.
Please use the generic function. Besides you are introducing register
PMAPMD_MVL_PMAPMD_CTL which is MDIO_PMA_PMD_BT1_CTRL.

>  static int mv88q2xxx_read_aneg_speed(struct phy_device *phydev)
>  {
>  	int ret;
> @@ -448,6 +525,11 @@ static int mv88q2xxx_read_status(struct phy_device *phydev)
>  	if (ret < 0)
>  		return ret;
>  
> +	/* Populate master/slave status also for forced modes */
> +	ret = mv88q2xxx_read_master_slave_state(phydev);
> +	if (ret < 0 && ret != -EOPNOTSUPP)
> +		return ret;
> +
>  	return genphy_c45_read_pma(phydev);
>  }
>  

Why ? This function is only used in case AUTONEG_ENABLE.

> @@ -478,6 +560,20 @@ static int mv88q2xxx_config_aneg(struct phy_device *phydev)
>  	if (ret)
>  		return ret;
>  
> +	/* Configure Base-T1 master/slave per phydev->master_slave_set.
> +	 * For AN disabled, program PMAPMD role directly; otherwise rely on
> +	 * the standard Base-T1 AN advertisement bits.
> +	 */
> +	if (phydev->autoneg == AUTONEG_DISABLE) {
> +		ret = mv88q2xxx_setup_master_slave_forced(phydev);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = genphy_c45_pma_baset1_setup_master_slave(phydev);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return phydev->drv->soft_reset(phydev);
>  }
>  

I don't see any reason why genphy_c45_config_aneg isn't sufficient here.
In case AUTONEG_DISABLE, genphy_c45_pma_setup_forced is called and calls
genphy_c45_pma_baset1_setup_master_slave which is basically the same as
mv88q2xxx_setup_master_slave_forced.
In case AUTONEG_ENABLE, calling genphy_c45_pma_baset1_setup_master_slave is
wrong, please look how genphy_c45_an_config_aneg is implemented.

Please take other users of the driver into CC, they did a lot of
reviewing and testing in the past. If there is some issue with the
driver, they should know:

"Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
"Gregor Herburger" <gregor.herburger@ew.tq-group.com>
"Stefan Eichenberger" <eichest@gmail.com>
"Geert Uytterhoeven" <geert@linux-m68k.org>

Which device are you using, and why did you need this patch ? Is there
any issue you are trying to fix ? On my side I did a lot of testing with
the different modes and never experienced any problems so far.

Best regards,
Dimitri Fedrau

Re: [PATCH] [88q2xxx] Add support for handling master/slave in forced mode

[Dimitri Fedrau]

Hi Ilya,

Am Thu, Aug 21, 2025 at 01:53:04PM -0700 schrieb Ilya Evenbach:
> I am using 88Q2221, This PHY has a bug - MDIO_PMA_EXTABLE is not properly
> implemented (reads as 0x20).
> As a result, genphy_c45_baset1_able() returns false, and master/slave is
> not set up or read properly in forced mode.

That is not 88Q2221 specific, please have a look at function
mv88q2xxx_config_init which solves this issue:

/* The 88Q2XXX PHYs do have the extended ability register available, but
 * register MDIO_PMA_EXTABLE where they should signalize it does not
 * work according to specification. Therefore, we force it here.
 */
phydev->pma_extable = MDIO_PMA_EXTABLE_BT1;

> I will try to clean up this patch to better utilize generic functions.
> 

What you are trying to do is adding support for 88Q2221 which has the
same phy_id as the 88Q2220 but needs a different(additional) setup to
work properly(init sequence, ...).
Please change your commit message then.

> On Thu, Aug 21, 2025 at 1:02 AM Dimitri Fedrau <dima.fedrau@gmail.com>
> wrote:
> >
> > Hi Ilya,
> >
> > Am Wed, Aug 20, 2025 at 11:11:43AM -0700 schrieb Ilya A. Evenbach:
> > > 88q2xxx PHYs have non-standard way of setting master/slave in
> > > forced mode.
> > > This change adds support for changing and reporting this setting
> > > correctly through ethtool.
> > >
> > > Signed-off-by: Ilya A. Evenbach <ievenbach@aurora.tech>
> > > ---
> > >  drivers/net/phy/marvell-88q2xxx.c | 106 ++++++++++++++++++++++++++++--
> > >  1 file changed, 101 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/phy/marvell-88q2xxx.c
> b/drivers/net/phy/marvell-88q2xxx.c
> > > index f3d83b04c953..b94d574fd9b7 100644
> > > --- a/drivers/net/phy/marvell-88q2xxx.c
> > > +++ b/drivers/net/phy/marvell-88q2xxx.c
> > > @@ -118,6 +118,11 @@
> > >  #define MV88Q2XXX_LED_INDEX_TX_ENABLE                        0
> > >  #define MV88Q2XXX_LED_INDEX_GPIO                     1
> > >
> > > +/* Marvell vendor PMA/PMD control for forced master/slave when AN is
> disabled */
> > > +#define PMAPMD_MVL_PMAPMD_CTL                                0x0834
> >
> > Already defined, see MDIO_PMA_PMD_BT1_CTRL.
> >
> > > +#define MASTER_MODE                                  BIT(14)
> >
> > Already defines, see MDIO_PMA_PMD_BT1_CTRL_CFG_MST.
> >
> > > +#define MODE_MASK                                    BIT(14)
> > > +
> > >  struct mv88q2xxx_priv {
> > >       bool enable_led0;
> > >  };
> > > @@ -377,13 +382,57 @@ static int mv88q2xxx_read_link(struct phy_device
> *phydev)
> > >  static int mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
> > >  {
> > >       int ret;
> > > +     int adv_l, adv_m, stat, stat2;
> > > +
> > > +     /* In forced mode, state and config are controlled via PMAPMD
> 0x834 */
> > > +     if (phydev->autoneg == AUTONEG_DISABLE) {
> > > +             ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
> PMAPMD_MVL_PMAPMD_CTL);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             if (ret & MASTER_MODE) {
> > > +                     phydev->master_slave_state =
> MASTER_SLAVE_STATE_MASTER;
> > > +                     phydev->master_slave_get =
> MASTER_SLAVE_CFG_MASTER_FORCE;
> > > +             } else {
> > > +                     phydev->master_slave_state =
> MASTER_SLAVE_STATE_SLAVE;
> > > +                     phydev->master_slave_get =
> MASTER_SLAVE_CFG_SLAVE_FORCE;
> > > +             }
> > > +             return 0;
> > > +     }
> > >
> > > -     phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> > > -     ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT);
> > > -     if (ret < 0)
> > > -             return ret;
> > >
> > > -     if (ret & MDIO_MMD_AN_MV_STAT_LOCAL_MASTER)
> > > +     adv_l = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L);
> > > +     if (adv_l < 0)
> > > +             return adv_l;
> > > +     adv_m = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_M);
> > > +     if (adv_m < 0)
> > > +             return adv_m;
> > > +
> > > +     if (adv_l & MDIO_AN_T1_ADV_L_FORCE_MS)
> > > +             phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
> > > +     else if (adv_m & MDIO_AN_T1_ADV_M_MST)
> > > +             phydev->master_slave_get =
> MASTER_SLAVE_CFG_MASTER_PREFERRED;
> > > +     else
> > > +             phydev->master_slave_get =
> MASTER_SLAVE_CFG_SLAVE_PREFERRED;
> > > +
> > > +     stat = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT);
> > > +     if (stat < 0)
> > > +             return stat;
> > > +
> > > +     if (stat & MDIO_MMD_AN_MV_STAT_MS_CONF_FAULT) {
> > > +             phydev->master_slave_state = MASTER_SLAVE_STATE_ERR;
> > > +             return 0;
> > > +     }
> > > +
> > > +     stat2 = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT2);
> > > +     if (stat2 < 0)
> > > +             return stat2;
> > > +     if (!(stat2 & MDIO_MMD_AN_MV_STAT2_AN_RESOLVED)) {
> > > +             phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> > > +             return 0;
> > > +     }
> > > +
> > > +     if (stat & MDIO_MMD_AN_MV_STAT_LOCAL_MASTER)
> > >               phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
> > >       else
> > >               phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
> > > @@ -391,6 +440,34 @@ static int
> mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
> > >       return 0;
> > >  }
> > >
> >
> > Is there a issue with the function you are trying to fix ? Seems that
> > you copied some generic functions into it.
> >
> > > +static int mv88q2xxx_setup_master_slave_forced(struct phy_device
> *phydev)
> > > +{
> > > +     int ret = 0;
> > > +
> > > +     switch (phydev->master_slave_set) {
> > > +     case MASTER_SLAVE_CFG_MASTER_FORCE:
> > > +     case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> > > +             ret = phy_modify_mmd_changed(phydev, MDIO_MMD_PMAPMD,
> > > +                                          PMAPMD_MVL_PMAPMD_CTL,
> > > +                                          MODE_MASK, MASTER_MODE);
> > > +             break;
> > > +     case MASTER_SLAVE_CFG_SLAVE_FORCE:
> > > +     case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> > > +             ret = phy_modify_mmd_changed(phydev, MDIO_MMD_PMAPMD,
> > > +                                          PMAPMD_MVL_PMAPMD_CTL,
> > > +                                          MODE_MASK, 0);
> > > +             break;
> > > +     case MASTER_SLAVE_CFG_UNKNOWN:
> > > +     case MASTER_SLAVE_CFG_UNSUPPORTED:
> > > +     default:
> > > +             phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> > > +             ret = 0;
> > > +             break;
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +
> >
> > This function does the same as genphy_c45_pma_baset1_setup_master_slave.
> > Please use the generic function. Besides you are introducing register
> > PMAPMD_MVL_PMAPMD_CTL which is MDIO_PMA_PMD_BT1_CTRL.
> >
> > >  static int mv88q2xxx_read_aneg_speed(struct phy_device *phydev)
> > >  {
> > >       int ret;
> > > @@ -448,6 +525,11 @@ static int mv88q2xxx_read_status(struct phy_device
> *phydev)
> > >       if (ret < 0)
> > >               return ret;
> > >
> > > +     /* Populate master/slave status also for forced modes */
> > > +     ret = mv88q2xxx_read_master_slave_state(phydev);
> > > +     if (ret < 0 && ret != -EOPNOTSUPP)
> > > +             return ret;
> > > +
> > >       return genphy_c45_read_pma(phydev);
> > >  }
> > >
> >
> > Why ? This function is only used in case AUTONEG_ENABLE.
> >
> > > @@ -478,6 +560,20 @@ static int mv88q2xxx_config_aneg(struct phy_device
> *phydev)
> > >       if (ret)
> > >               return ret;
> > >
> > > +     /* Configure Base-T1 master/slave per phydev->master_slave_set.
> > > +      * For AN disabled, program PMAPMD role directly; otherwise rely
> on
> > > +      * the standard Base-T1 AN advertisement bits.
> > > +      */
> > > +     if (phydev->autoneg == AUTONEG_DISABLE) {
> > > +             ret = mv88q2xxx_setup_master_slave_forced(phydev);
> > > +             if (ret)
> > > +                     return ret;
> > > +     } else {
> > > +             ret = genphy_c45_pma_baset1_setup_master_slave(phydev);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > >       return phydev->drv->soft_reset(phydev);
> > >  }
> > >
> >
> > I don't see any reason why genphy_c45_config_aneg isn't sufficient here.
> > In case AUTONEG_DISABLE, genphy_c45_pma_setup_forced is called and calls
> > genphy_c45_pma_baset1_setup_master_slave which is basically the same as
> > mv88q2xxx_setup_master_slave_forced.
> > In case AUTONEG_ENABLE, calling genphy_c45_pma_baset1_setup_master_slave
> is
> > wrong, please look how genphy_c45_an_config_aneg is implemented.
> >
> > Please take other users of the driver into CC, they did a lot of
> > reviewing and testing in the past. If there is some issue with the
> > driver, they should know:
> >
> > "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
> > "Gregor Herburger" <gregor.herburger@ew.tq-group.com>
> > "Stefan Eichenberger" <eichest@gmail.com>
> > "Geert Uytterhoeven" <geert@linux-m68k.org>
> >
> > Which device are you using, and why did you need this patch ? Is there
> > any issue you are trying to fix ? On my side I did a lot of testing with
> > the different modes and never experienced any problems so far.
> >
> > Best regards,
> > Dimitri Fedrau