| Message ID | 20241003071050.376502-1-qingtao.cao@digi.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3,1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed | expand |
On 10/3/2024 9:10 AM, Qingtao Cao wrote: > On 88E151x the SGMII autoneg bypass mode defaults to be enabled. When it is > activated, the device assumes a link-up status with existing configuration > in BMCR, avoid bringing down the fibre link in this case > > Test case: > 1. Two 88E151x connected with SFP, both enable autoneg, link is up with > speed 1000M > 2. Disable autoneg on one device and explicitly set its speed to 1000M > 3. The fibre link can still up with this change, otherwise not. > > Signed-off-by: Qingtao Cao <qingtao.cao@digi.com> > --- > drivers/net/phy/marvell.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c > index 9964bf3dea2f..efc4b2317466 100644 > --- a/drivers/net/phy/marvell.c > +++ b/drivers/net/phy/marvell.c > @@ -195,6 +195,10 @@ > > #define MII_88E1510_MSCR_2 0x15 > > +#define MII_88E1510_FSCR2 0x1a > +#define MII_88E1510_FSCR2_BYPASS_ENABLE BIT(6) > +#define MII_88E1510_FSCR2_BYPASS_STATUS BIT(5) > + > #define MII_VCT5_TX_RX_MDI0_COUPLING 0x10 > #define MII_VCT5_TX_RX_MDI1_COUPLING 0x11 > #define MII_VCT5_TX_RX_MDI2_COUPLING 0x12 > @@ -1623,11 +1627,21 @@ static void fiber_lpa_mod_linkmode_lpa_t(unsigned long *advertising, u32 lpa) > static int marvell_read_status_page_an(struct phy_device *phydev, > int fiber, int status) > { > + int fscr2; > int lpa; > int err; > > if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) { > phydev->link = 0; > + if (fiber) { > + fscr2 = phy_read(phydev, MII_88E1510_FSCR2); > + if (fscr2 < 0) > + return fscr2; > + if ((fscr2 & MII_88E1510_FSCR2_BYPASS_ENABLE) && > + (fscr2 & MII_88E1510_FSCR2_BYPASS_STATUS) && > + (genphy_read_status_fixed(phydev) == 0)) > + phydev->link = 1; > + } > return 0; > } > LGTM. Reviewed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
On Thu, Oct 03, 2024 at 05:10:50PM +1000, Qingtao Cao wrote: > On 88E151x the SGMII autoneg bypass mode defaults to be enabled. When it is > activated, the device assumes a link-up status with existing configuration > in BMCR, avoid bringing down the fibre link in this case > > Test case: > 1. Two 88E151x connected with SFP, both enable autoneg, link is up with > speed 1000M > 2. Disable autoneg on one device and explicitly set its speed to 1000M > 3. The fibre link can still up with this change, otherwise not. As you're clearly using fibre, there's this chunk of code in the same function that is (IMHO) wrong: if (phydev->duplex == DUPLEX_FULL) { if (!(lpa & LPA_PAUSE_FIBER)) { phydev->pause = 0; phydev->asym_pause = 0; } else if ((lpa & LPA_PAUSE_ASYM_FIBER)) { phydev->pause = 1; phydev->asym_pause = 1; } else { phydev->pause = 1; phydev->asym_pause = 0; } } as ->pause and ->asym_pause are supposed to be the _resolved_ state, and this is only looking at the link partner's state. fiber_lpa_mod_linkmode_lpa_t() also uses the baseT linkmodes for Fibre. IMHO, this should be: mii_lpa_mod_linkmode_x(phydev->lp_advertising, lpa, ETHTOOL_LINK_MODE_1000baseX_Full_BIT); phy_resolve_aneg_pause(phydev); Please can you test whether that works correctly in addition to your other fix? Thanks.
On Thu, Oct 03, 2024 at 05:10:50PM +1000, Qingtao Cao wrote: > On 88E151x the SGMII autoneg bypass mode defaults to be enabled. When it is > activated, the device assumes a link-up status with existing configuration > in BMCR, avoid bringing down the fibre link in this case https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html Please note the bullet point: * don’t repost your patches within one 24h period It would be good if you read all this document. Andrew
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 9964bf3dea2f..efc4b2317466 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -195,6 +195,10 @@ #define MII_88E1510_MSCR_2 0x15 +#define MII_88E1510_FSCR2 0x1a +#define MII_88E1510_FSCR2_BYPASS_ENABLE BIT(6) +#define MII_88E1510_FSCR2_BYPASS_STATUS BIT(5) + #define MII_VCT5_TX_RX_MDI0_COUPLING 0x10 #define MII_VCT5_TX_RX_MDI1_COUPLING 0x11 #define MII_VCT5_TX_RX_MDI2_COUPLING 0x12 @@ -1623,11 +1627,21 @@ static void fiber_lpa_mod_linkmode_lpa_t(unsigned long *advertising, u32 lpa) static int marvell_read_status_page_an(struct phy_device *phydev, int fiber, int status) { + int fscr2; int lpa; int err; if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) { phydev->link = 0; + if (fiber) { + fscr2 = phy_read(phydev, MII_88E1510_FSCR2); + if (fscr2 < 0) + return fscr2; + if ((fscr2 & MII_88E1510_FSCR2_BYPASS_ENABLE) && + (fscr2 & MII_88E1510_FSCR2_BYPASS_STATUS) && + (genphy_read_status_fixed(phydev) == 0)) + phydev->link = 1; + } return 0; }
On 88E151x the SGMII autoneg bypass mode defaults to be enabled. When it is activated, the device assumes a link-up status with existing configuration in BMCR, avoid bringing down the fibre link in this case Test case: 1. Two 88E151x connected with SFP, both enable autoneg, link is up with speed 1000M 2. Disable autoneg on one device and explicitly set its speed to 1000M 3. The fibre link can still up with this change, otherwise not. Signed-off-by: Qingtao Cao <qingtao.cao@digi.com> --- drivers/net/phy/marvell.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)