| Message ID | 20231011123856.1443308-3-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1,1/3] net: phy: micrel: Extend KSZ886X PHY Special Ctrl/Status Reg definitions | expand |
Hi Oleksij, Am Mittwoch, 11. Oktober 2023, 14:38:56 CEST schrieb Oleksij Rempel: > 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(-) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 927d3d54658e..12f093aed4ff 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 { > + /* Make sure, the link state is not forced. > + * Otherwise, the PHY we create a link by skipping the PHY will create? > + * autonegotiation process. > + */ > + ret = phy_clear_bits(phydev, MII_KSZPHY_CTRL, > + KSZ886X_CTRL_FORCE_LINK); > + if (ret) > + return ret; Isn't this call to phy_clear_bits() a fix for autonegotiation mode? This should be a separate patch then. Best regards, Alexander > + > + 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
Hi Alexander, thank you for review! On Wed, Oct 11, 2023 at 03:29:49PM +0200, Alexander Stein wrote: > Hi Oleksij, > > Am Mittwoch, 11. Oktober 2023, 14:38:56 CEST schrieb Oleksij Rempel: > > 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(-) > > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > > index 927d3d54658e..12f093aed4ff 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 { > > + /* Make sure, the link state is not forced. > > + * Otherwise, the PHY we create a link by skipping the > > PHY will create? > > > + * autonegotiation process. > > + */ > > + ret = phy_clear_bits(phydev, MII_KSZPHY_CTRL, > > + KSZ886X_CTRL_FORCE_LINK); > > + if (ret) > > + return ret; > > Isn't this call to phy_clear_bits() a fix for autonegotiation mode? This > should be a separate patch then. First time this bit is set by this patch, I assume this problem would not exist without fixed link fix. Regards, Oleksij
On Wed, Oct 11, 2023 at 02:38:56PM +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> > --- > 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..12f093aed4ff 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 nit: autonegotiation > + * 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. > + */ ...
On Wed, Oct 11, 2023 at 03:29:49PM +0200, Alexander Stein wrote: > Hi Oleksij, > > Am Mittwoch, 11. Oktober 2023, 14:38:56 CEST schrieb Oleksij Rempel: > > + if (phydev->autoneg != AUTONEG_ENABLE) { > > + ret = genphy_setup_forced(phydev); > > + if (ret) > > + return ret; > > + ... > > + ret = phy_set_bits(phydev, MII_KSZPHY_CTRL, > > + KSZ886X_CTRL_FORCE_LINK); > > + if (ret) > > + return ret; > > + } else { ... > > + ret = phy_clear_bits(phydev, MII_KSZPHY_CTRL, > > + KSZ886X_CTRL_FORCE_LINK); > > + if (ret) > > + return ret; > > Isn't this call to phy_clear_bits() a fix for autonegotiation mode? This > should be a separate patch then. No, I don't think that is the case. Compare the two paths above, noting that patch 1 introduces the definition for KSZ886X_CTRL_FORCE_LINK. If autoneg is disabled, then this bit is then set, which forces the link. Clearly, if autoneg is then re-enabled, this bit has to be cleared to allow the effects of the autoneg-disabled path to be undone. So both of these, the phy_set_bits() and the phy_clear_bits() belong in the same patch. Splitting them up, so we introduce phy_set_bits() first will create a regression - which we don't want. These two belong logically together. What does concern me, however, is that the autoneg-disabled path avoids calling genphy_setup_master_slave(), and since establishing which end of the link is part of the fundamentals of a 1000base-T link, I wonder whether both paths should still call genphy_config_aneg(). Apart from that, I think the patch is otherwise fine.
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 927d3d54658e..12f093aed4ff 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 { + /* Make sure, the link state is not forced. + * Otherwise, the PHY we create a link by skipping the + * autonegotiation process. + */ + 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(-)