[net-next,v1,3/3] net: phy: micrel: Fix forced link mode for KSZ886X switches

  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(-)
  

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.