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

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

[Qingtao Cao]

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)
+#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;
 	}
 
-- 
2.34.1

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

[Parthiban.Veerasooran]

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

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

[Andrew Lunn]

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

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!

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

[Andrew Lunn]

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

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

[Russell King (Oracle)]

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.

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

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

[Andrew Lunn]

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