Messages in this thread | | | Date | Fri, 13 Oct 2023 15:26:46 +0300 | From | Vladimir Oltean <> | Subject | Re: [PATCH net-next v2 3/3] net: phy: micrel: Fix forced link mode for KSZ886X switches |
| |
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 >
| |