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

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 [off-list ref]
wrote:

quoted

Hi Ilya,

Am Wed, Aug 20, 2025 at 11:11:43AM -0700 schrieb Ilya A. Evenbach:

quoted

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

quoted

quoted

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

quoted

quoted

+#define PMAPMD_MVL_PMAPMD_CTL                                0x0834

Already defined, see MDIO_PMA_PMD_BT1_CTRL.

quoted

+#define MASTER_MODE                                  BIT(14)

Already defines, see MDIO_PMA_PMD_BT1_CTRL_CFG_MST.

quoted

+#define MODE_MASK                                    BIT(14)
+
 struct mv88q2xxx_priv {
      bool enable_led0;
 };

@@ -377,13 +382,57 @@ static int mv88q2xxx_read_link(struct phy_device

*phydev)

quoted

quoted

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

quoted

quoted

+     if (phydev->autoneg == AUTONEG_DISABLE) {
+             ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,

PMAPMD_MVL_PMAPMD_CTL);

quoted

quoted

+             if (ret < 0)
+                     return ret;
+
+             if (ret & MASTER_MODE) {
+                     phydev->master_slave_state =

MASTER_SLAVE_STATE_MASTER;

quoted

quoted

+                     phydev->master_slave_get =

MASTER_SLAVE_CFG_MASTER_FORCE;

quoted

quoted

+             } else {
+                     phydev->master_slave_state =

MASTER_SLAVE_STATE_SLAVE;

quoted

quoted

+                     phydev->master_slave_get =

MASTER_SLAVE_CFG_SLAVE_FORCE;

quoted

quoted

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

quoted

quoted

+     else
+             phydev->master_slave_get =

MASTER_SLAVE_CFG_SLAVE_PREFERRED;

quoted

quoted

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

quoted

quoted

      return 0;
 }

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

quoted

+static int mv88q2xxx_setup_master_slave_forced(struct phy_device

*phydev)

quoted

quoted

+{
+     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.

quoted

 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)

quoted

quoted

      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.

quoted

@@ -478,6 +560,20 @@ static int mv88q2xxx_config_aneg(struct phy_device

*phydev)

quoted

quoted

      if (ret)
              return ret;

+     /* Configure Base-T1 master/slave per phydev->master_slave_set.
+      * For AN disabled, program PMAPMD role directly; otherwise rely

on

quoted

quoted

+      * 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

quoted

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" [off-list ref]
"Gregor Herburger" [off-list ref]
"Stefan Eichenberger" [off-list ref]
"Geert Uytterhoeven" [off-list ref]

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