[PATCH net-next] net: phy: marvell: update abilities and advertising when switching to SGMII

[PATCH net-next] net: phy: marvell: update abilities and advertising when switching to SGMII

[Robert Hancock]

With some SFP modules, such as Finisar FCLF8522P2BTL, the PHY hardware
strapping defaults to 1000BaseX mode, but the kernel prefers to set them
for SGMII mode. When this happens and the PHY is soft reset, the BMSR
status register is updated, but this happens after the kernel has already
read the PHY abilities during probing. This results in support not being
detected for, and the PHY not advertising support for, 10 and 100 Mbps
modes, preventing the link from working with a non-gigabit link partner.

When the PHY is being configured for SGMII mode, call genphy_read_abilities
again in order to re-read the capabilities, and update the advertising
field accordingly.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/net/phy/marvell.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 2702faf7b0f6..47e83c1e9051 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -961,7 +961,21 @@ static int m88e1111_config_init(struct phy_device *phydev)
 	if (err < 0)
 		return err;
 
-	return genphy_soft_reset(phydev);
+	err = genphy_soft_reset(phydev);
+	if (err < 0)
+		return err;
+
+	if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
+		/* If the HWCFG_MODE was changed from another mode (such as
+		 * 1000BaseX) to SGMII, the state of the support bits may have
+		 * also changed now that the PHY has been reset.
+		 * Update the PHY abilities accordingly.
+		 */
+		err = genphy_read_abilities(phydev);
+		linkmode_or(phydev->advertising, phydev->advertising,
+			    phydev->supported);
+	}
+	return err;
 }
 
 static int m88e1111_get_downshift(struct phy_device *phydev, u8 *data)
-- 
2.31.1

Re: [PATCH net-next] net: phy: marvell: update abilities and advertising when switching to SGMII

[Andrew Lunn]

On Wed, Apr 27, 2022 at 01:39:28PM -0600, Robert Hancock wrote:
> With some SFP modules, such as Finisar FCLF8522P2BTL, the PHY hardware
> strapping defaults to 1000BaseX mode, but the kernel prefers to set them
> for SGMII mode.

Is this the SFP code determining this? Its copper == use SGMII?

> When this happens and the PHY is soft reset, the BMSR
> status register is updated, but this happens after the kernel has already
> read the PHY abilities during probing. This results in support not being
> detected for, and the PHY not advertising support for, 10 and 100 Mbps
> modes, preventing the link from working with a non-gigabit link partner.
> 
> When the PHY is being configured for SGMII mode, call genphy_read_abilities
> again in order to re-read the capabilities, and update the advertising
> field accordingly.

Is this actually a generic problem? There are other PHYs used in SFP
modules, and i assume they also could have their mode changed. So
should the re-reading of the abilities be in the core, not each
driver?

	Andrew

Re: [PATCH net-next] net: phy: marvell: update abilities and advertising when switching to SGMII

[Russell King (Oracle)]

On Fri, Apr 29, 2022 at 03:35:43AM +0200, Andrew Lunn wrote:
> On Wed, Apr 27, 2022 at 01:39:28PM -0600, Robert Hancock wrote:
> > With some SFP modules, such as Finisar FCLF8522P2BTL, the PHY hardware
> > strapping defaults to 1000BaseX mode, but the kernel prefers to set them
> > for SGMII mode.
> 
> Is this the SFP code determining this? Its copper == use SGMII?
> 
> > When this happens and the PHY is soft reset, the BMSR
> > status register is updated, but this happens after the kernel has already
> > read the PHY abilities during probing. This results in support not being
> > detected for, and the PHY not advertising support for, 10 and 100 Mbps
> > modes, preventing the link from working with a non-gigabit link partner.
> > 
> > When the PHY is being configured for SGMII mode, call genphy_read_abilities
> > again in order to re-read the capabilities, and update the advertising
> > field accordingly.
> 
> Is this actually a generic problem? There are other PHYs used in SFP
> modules, and i assume they also could have their mode changed. So
> should the re-reading of the abilities be in the core, not each
> driver?

The most common PHY in SFPs is the 88E1111, because that offers both
MDIO and I2C hookup. Some other modules use the AR8035, but that
doesn't support I2C, and consequently doesn't tend to be accessible.

Re-reading the capabilities when the PHY changes host interface mode
between 1000baseX and SGMII makes sense because of the fixed-speed vs
10/100/1000 capability of the other.

One of the problems is the core code doesn't know how the PHY is
initially configured, so we can't actually detect mode changes. We
would need all PHY drivers to read the MAC mode at probe time, save
it away as the current mode, and only then can we detect changes.

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

Re: [PATCH net-next] net: phy: marvell: update abilities and advertising when switching to SGMII

[Andrew Lunn]

On Wed, Apr 27, 2022 at 01:39:28PM -0600, Robert Hancock wrote:
> With some SFP modules, such as Finisar FCLF8522P2BTL, the PHY hardware
> strapping defaults to 1000BaseX mode, but the kernel prefers to set them
> for SGMII mode. When this happens and the PHY is soft reset, the BMSR
> status register is updated, but this happens after the kernel has already
> read the PHY abilities during probing. This results in support not being
> detected for, and the PHY not advertising support for, 10 and 100 Mbps
> modes, preventing the link from working with a non-gigabit link partner.
> 
> When the PHY is being configured for SGMII mode, call genphy_read_abilities
> again in order to re-read the capabilities, and update the advertising
> field accordingly.
> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next] net: phy: marvell: update abilities and advertising when switching to SGMII

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Wed, 27 Apr 2022 13:39:28 -0600 you wrote:
> With some SFP modules, such as Finisar FCLF8522P2BTL, the PHY hardware
> strapping defaults to 1000BaseX mode, but the kernel prefers to set them
> for SGMII mode. When this happens and the PHY is soft reset, the BMSR
> status register is updated, but this happens after the kernel has already
> read the PHY abilities during probing. This results in support not being
> detected for, and the PHY not advertising support for, 10 and 100 Mbps
> modes, preventing the link from working with a non-gigabit link partner.
> 
> [...]

Here is the summary with links:
  - [net-next] net: phy: marvell: update abilities and advertising when switching to SGMII
    https://git.kernel.org/netdev/net-next/c/0ed99ecc95b9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html