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

[Dimitri Fedrau <dima.fedrau@gmail.com>]

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