Re: [PATCH 1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed

[Russell King (Oracle)]

On Thu, Oct 03, 2024 at 04:30:19PM +0200, Andrew Lunn wrote:
> On Thu, Oct 03, 2024 at 12:25:12PM +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.
> 
> What is actually wrong here?
> 
> If both ends are performing auto-neg, i would expect a link at the
> highest speeds both link peers support.
> 
> If one peer is doing autoneg, the other not, i expect link down, this
> is not a valid configuration, since one peer is going to fail to
> auto-neg.
> 
> If both peers are using forced 1000M, i would expect a link.

Since I've seen this patch posted, I've been wanting to pick through
the Marvell documentation for the PHY.

The bit in question is bit 11 of the Fiber Specific Status Register
(FSSR, page 1, register 17).

When AN is disabled or in 100FX mode, this bit will be set. It will
also be set when the speed and duplex have been resolved, and thus
those fields are valid. If the fiber specific control register 2
(FSCR2) bit 5 is set (AN bypass status), then this bit will also be
clear.

When FSSR bit 11 is clear, then duplex (bit 13) and speed (bits 14
and 15) are not valid, so we shouldn't interpret their values.

Further reading of the FSCR2 documentation indicates that bit 5 is
a simple status bit that bypass mode was entered, and thus it can
only be set when bypass mode was enabled (bit 6) - so checking that
bit 6 is set is unnecessary.

So, I'd suggest something like:

	int fscr2;

	if (fiber) {
		/* We are on page 1, so this reads the FSCR2 */
		fscr2 = phy_read(phydev, MII_88E151X_FSCR2);
		if (fscr2 & MII_88E151X_FSCR2_BYPASS_STATUS) {
			err = genphy_read_status_fixed(phydev);
			if (err < 0)
				return err;

			phydev->link = 1;
			return 0;
		}
	}

would be sufficient, _provided_ the BMCR fixed-speed setting is
correct for 1000base-X mode when AN is enabled (I haven't checked
that is the case.)

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