| Message ID | 20231012065502.2928220-4-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | fix forced link mode for KSZ886X switches | expand |
Hi Oleksij, On Thu, Oct 12, 2023 at 08:55:02AM +0200, Oleksij Rempel wrote: > Address a link speed detection issue in KSZ886X PHY driver when in > forced link mode. Previously, link partners like "ASIX AX88772B" > with KSZ8873 could fall back to 10Mbit instead of configured 100Mbit. > > The issue arises as KSZ886X PHY continues sending Fast Link Pulses (FLPs) > even with autonegotiation off, misleading link partners in autoneg mode, > leading to incorrect link speed detection. > > Now, when autonegotiation is disabled, the driver sets the link state > forcefully using KSZ886X_CTRL_FORCE_LINK bit. This action, beyond just > disabling autonegotiation, makes the PHY state more reliably detected by > link partners using parallel detection, thus fixing the link speed > misconfiguration. > > With autonegotiation enabled, link state is not forced, allowing proper > autonegotiation process participation. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- Have you considered denying "ethtool -s swpN autoneg off" in "net.git" (considering that it doesn't work properly), and re-enabling it in "net-next.git"? > drivers/net/phy/micrel.c | 32 +++++++++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 927d3d54658e..599ebf54fafe 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -1729,9 +1729,35 @@ static int ksz886x_config_aneg(struct phy_device *phydev) > { > int ret; > > - ret = genphy_config_aneg(phydev); > - if (ret) > - return ret; > + if (phydev->autoneg != AUTONEG_ENABLE) { > + ret = genphy_setup_forced(phydev); > + if (ret) > + return ret; __genphy_config_aneg() will call genphy_setup_forced() as appropriate, and additionally it will resync the master-slave resolution to a forced value, if needed. So I think it's better to call genphy_config_aneg() from the common code path, and just use the "if (phydev->autoneg)" test to keep the vendor-specific register in sync with the autoneg setting. > + > + /* When autonegotation is disabled, we need to manually force > + * the link state. If we don't do this, the PHY will keep > + * sending Fast Link Pulses (FLPs) which are part of the > + * autonegotiation process. This is not desired when > + * autonegotiation is off. > + */ > + ret = phy_set_bits(phydev, MII_KSZPHY_CTRL, > + KSZ886X_CTRL_FORCE_LINK); > + if (ret) > + return ret; > + } else { > + /* If we had previously forced the link state, we need to > + * clear KSZ886X_CTRL_FORCE_LINK bit now. Otherwise, the PHY > + * will not perform autonegotiation. > + */ > + ret = phy_clear_bits(phydev, MII_KSZPHY_CTRL, > + KSZ886X_CTRL_FORCE_LINK); > + if (ret) > + return ret; > + > + ret = genphy_config_aneg(phydev); > + if (ret) > + return ret; > + } > > /* The MDI-X configuration is automatically changed by the PHY after > * switching from autoneg off to on. So, take MDI-X configuration under > -- > 2.39.2 >
Hi Vladimir, On Fri, Oct 13, 2023 at 03:26:46PM +0300, Vladimir Oltean wrote: > Hi Oleksij, > > On Thu, Oct 12, 2023 at 08:55:02AM +0200, Oleksij Rempel wrote: > > Address a link speed detection issue in KSZ886X PHY driver when in > > forced link mode. Previously, link partners like "ASIX AX88772B" > > with KSZ8873 could fall back to 10Mbit instead of configured 100Mbit. > > > > The issue arises as KSZ886X PHY continues sending Fast Link Pulses (FLPs) > > even with autonegotiation off, misleading link partners in autoneg mode, > > leading to incorrect link speed detection. > > > > Now, when autonegotiation is disabled, the driver sets the link state > > forcefully using KSZ886X_CTRL_FORCE_LINK bit. This action, beyond just > > disabling autonegotiation, makes the PHY state more reliably detected by > > link partners using parallel detection, thus fixing the link speed > > misconfiguration. > > > > With autonegotiation enabled, link state is not forced, allowing proper > > autonegotiation process participation. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > Have you considered denying "ethtool -s swpN autoneg off" in "net.git" > (considering that it doesn't work properly), and re-enabling it in > "net-next.git"? Yes, but for ksz8 task I don't have budget to go this way. > > drivers/net/phy/micrel.c | 32 +++++++++++++++++++++++++++++--- > > 1 file changed, 29 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > > index 927d3d54658e..599ebf54fafe 100644 > > --- a/drivers/net/phy/micrel.c > > +++ b/drivers/net/phy/micrel.c > > @@ -1729,9 +1729,35 @@ static int ksz886x_config_aneg(struct phy_device *phydev) > > { > > int ret; > > > > - ret = genphy_config_aneg(phydev); > > - if (ret) > > - return ret; > > + if (phydev->autoneg != AUTONEG_ENABLE) { > > + ret = genphy_setup_forced(phydev); > > + if (ret) > > + return ret; > > __genphy_config_aneg() will call genphy_setup_forced() as appropriate, > and additionally it will resync the master-slave resolution to a forced > value, if needed. So I think it's better to call genphy_config_aneg() > from the common code path, and just use the "if (phydev->autoneg)" test > to keep the vendor-specific register in sync with the autoneg setting. ack. Will update it. Regards, Oleksij
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 927d3d54658e..599ebf54fafe 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -1729,9 +1729,35 @@ static int ksz886x_config_aneg(struct phy_device *phydev) { int ret; - ret = genphy_config_aneg(phydev); - if (ret) - return ret; + if (phydev->autoneg != AUTONEG_ENABLE) { + ret = genphy_setup_forced(phydev); + if (ret) + return ret; + + /* When autonegotation is disabled, we need to manually force + * the link state. If we don't do this, the PHY will keep + * sending Fast Link Pulses (FLPs) which are part of the + * autonegotiation process. This is not desired when + * autonegotiation is off. + */ + ret = phy_set_bits(phydev, MII_KSZPHY_CTRL, + KSZ886X_CTRL_FORCE_LINK); + if (ret) + return ret; + } else { + /* If we had previously forced the link state, we need to + * clear KSZ886X_CTRL_FORCE_LINK bit now. Otherwise, the PHY + * will not perform autonegotiation. + */ + ret = phy_clear_bits(phydev, MII_KSZPHY_CTRL, + KSZ886X_CTRL_FORCE_LINK); + if (ret) + return ret; + + ret = genphy_config_aneg(phydev); + if (ret) + return ret; + } /* The MDI-X configuration is automatically changed by the PHY after * switching from autoneg off to on. So, take MDI-X configuration under
Address a link speed detection issue in KSZ886X PHY driver when in forced link mode. Previously, link partners like "ASIX AX88772B" with KSZ8873 could fall back to 10Mbit instead of configured 100Mbit. The issue arises as KSZ886X PHY continues sending Fast Link Pulses (FLPs) even with autonegotiation off, misleading link partners in autoneg mode, leading to incorrect link speed detection. Now, when autonegotiation is disabled, the driver sets the link state forcefully using KSZ886X_CTRL_FORCE_LINK bit. This action, beyond just disabling autonegotiation, makes the PHY state more reliably detected by link partners using parallel detection, thus fixing the link speed misconfiguration. With autonegotiation enabled, link state is not forced, allowing proper autonegotiation process participation. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/phy/micrel.c | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)