| Message ID | 20220427193928.2155805-1-robert.hancock@calian.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0ed99ecc95b9c1d3c46d3bc34459088e82caef32 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: marvell: update abilities and advertising when switching to SGMII | expand |
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
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.
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
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!
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)
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(-)