| Message ID | 20241003022512.370600-1-qingtao.cao@digi.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed | expand |
Hi, On 03/10/24 7:55 am, Qingtao Cao wrote: > [You don't often get email from qingtao.cao.au@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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 | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c > index 9964bf3dea2f..535c6e679ff7 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 (1<<6) I think you can use BIT(6) > +#define MII_88E1510_FSCR2_BYPASS_STATUS (1<<5) Here as well 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 > @@ -1625,9 +1629,26 @@ static int marvell_read_status_page_an(struct phy_device *phydev, > { > int lpa; > int err; > + int fscr2; Follow reverse xmas tree ordering. https://www.kernel.org/doc/Documentation/process/maintainer-netdev.rst#:~:text=Local%20variable%20ordering%20(%22reverse%20xmas%20tree%22%2C%20%22RCS%22) Best regards, Parthiban V > > if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) { > - phydev->link = 0; > + if (!fiber) { > + phydev->link = 0; > + } else { > + fscr2 = phy_read(phydev, MII_88E1510_FSCR2); > + if (fscr2 > 0) { > + if ((fscr2 & MII_88E1510_FSCR2_BYPASS_ENABLE) && > + (fscr2 & MII_88E1510_FSCR2_BYPASS_STATUS)) { > + if (genphy_read_status_fixed(phydev) < 0) > + phydev->link = 0; > + } else { > + phydev->link = 0; > + } > + } else { > + phydev->link = 0; > + } > + } > + > return 0; > } > > -- > 2.34.1 > >
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. Andrew
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.)
On Fri, Oct 04, 2024 at 11:35:30AM +1000, Qingtao Cao wrote: > Hi Andrew, > > Please see my inline replies. > > On Fri, Oct 4, 2024 at 12:30 AM Andrew Lunn <andrew@lunn.ch> 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. > > > Well, technically speaking, thanks to the 88E151X's bypass mode, in such case > with one end using autoneg but the other is using 1000M explicitly, the link > could still be up, but not with the current code. So we can make an invalid configuration work. Question is, should we? Are we teaching users they can wrongly configure their system and expect it to work? They then think it is actually a valid configuration and try the same on some other board with other PHYs, and find it does not work? Does Marvell document why this bypass mode exists? When it should be used? What do they see as its use cases? Andrew
On Fri, Oct 04, 2024 at 03:26:33PM +0200, Andrew Lunn wrote: > On Fri, Oct 04, 2024 at 11:35:30AM +1000, Qingtao Cao wrote: > > Hi Andrew, > > > > Please see my inline replies. > > > > On Fri, Oct 4, 2024 at 12:30 AM Andrew Lunn <andrew@lunn.ch> 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. > > > > > > Well, technically speaking, thanks to the 88E151X's bypass mode, in such case > > with one end using autoneg but the other is using 1000M explicitly, the link > > could still be up, but not with the current code. > > So we can make an invalid configuration work. Question is, should we? > > Are we teaching users they can wrongly configure their system and > expect it to work? They then think it is actually a valid > configuration and try the same on some other board with other PHYs, > and find it does not work? > > Does Marvell document why this bypass mode exists? When it should be > used? What do they see as its use cases? The paragraph about it is couched in terms of "if the MAC or the PHY implements the auto-negotiation function and the other end does not". That seems to point towards a MAC <-> PHY link rather than across a media. So I tend to agree with you that we should not be enabling bypass mode on a media side link.
On Sat, Oct 05, 2024 at 07:25:05AM +1000, Qingtao Cao wrote: > Right, the section about it further states that "... To solve this problem, > the device implements the autoneg bypass mode for serial interface" > and about several hundreds of ms if the device receives idles then it > goes to a new state similar to link up. > > In my v4 change no BMCR is used, but the partner's advertised > configs (1000BASE-X) is read from the phy status register, using > existing code in that function and testing still finds its working, > which makes me believe that the autoneg end once it is bypassed, > it simply adopts its partner's configuration. Well, for 1000Base-X, autoneg is not about speed, since that is hard coded to 1G. Autoneg is about pause and priority for duplex mode. So play with those settings and see what happens. Andrew
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 9964bf3dea2f..535c6e679ff7 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 (1<<6) +#define MII_88E1510_FSCR2_BYPASS_STATUS (1<<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 @@ -1625,9 +1629,26 @@ static int marvell_read_status_page_an(struct phy_device *phydev, { int lpa; int err; + int fscr2; if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) { - phydev->link = 0; + if (!fiber) { + phydev->link = 0; + } else { + fscr2 = phy_read(phydev, MII_88E1510_FSCR2); + if (fscr2 > 0) { + if ((fscr2 & MII_88E1510_FSCR2_BYPASS_ENABLE) && + (fscr2 & MII_88E1510_FSCR2_BYPASS_STATUS)) { + if (genphy_read_status_fixed(phydev) < 0) + phydev->link = 0; + } else { + phydev->link = 0; + } + } else { + phydev->link = 0; + } + } + 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 | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)