[PATCH RFC net-next] net: phy: micrel: Improve loopback support if autoneg is enabled

[PATCH RFC net-next] net: phy: micrel: Improve loopback support if autoneg is enabled

[Gerhard Engleder]

Prior to commit 6ff3cddc365b it was possible to enable loopback with
a defined speed. First a fixed speed was set with ETHTOOL_SLINKSETTINGS
and afterwards the loopback was enabled. This worked, because
genphy_loopback() uses the current speed and duplex. I used this
mechanism to test tsnep in loopback with different speeds. A KSZ9031 PHY
is used.

With commit 6ff3cddc365b for 1000 Mbit/s auto negotiation was enabled.
Setting a fixed speed with ETHTOOL_SLINKSETTINGS does not work anymore
for 1000 Mbit/s as speed and duplex of the PHY now depend on the result
of the auto negotiation. As a result, genphy_loopback() also depends on
the result of the auto negotiation. But enabling loopback shall be
independent of any auto negotiation process.

Make loopback of KSZ9031 PHY work even if current speed and/or duplex of
PHY are unkown because of autoneg.

Fixes: 6ff3cddc365b ("net: phylib: do not disable autoneg for fixed speeds >= 1G")
Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/phy/micrel.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 65b0a3115e14..3cbe40265190 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1028,6 +1028,32 @@ static int ksz9021_config_init(struct phy_device *phydev)
 #define MII_KSZ9031RN_EDPD		0x23
 #define MII_KSZ9031RN_EDPD_ENABLE	BIT(0)
 
+static int ksz9031_set_loopback(struct phy_device *phydev, bool enable)
+{
+	if (enable) {
+		u16 ctl = BMCR_LOOPBACK;
+		int ret, val;
+
+		if ((phydev->speed != SPEED_10) && (phydev->speed != SPEED_100))
+			phydev->speed = SPEED_1000;
+		phydev->duplex = DUPLEX_FULL;
+
+		ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
+
+		phy_modify(phydev, MII_BMCR, ~0, ctl);
+
+		ret = phy_read_poll_timeout(phydev, MII_BMSR, val,
+					    val & BMSR_LSTATUS, 5000, 500000,
+					    true);
+		if (ret)
+			return ret;
+	} else {
+		return genphy_loopback(phydev, enable);
+	}
+
+	return 0;
+}
+
 static int ksz9031_of_load_skew_values(struct phy_device *phydev,
 				       const struct device_node *of_node,
 				       u16 reg, size_t field_sz,
@@ -5478,6 +5504,7 @@ static struct phy_driver ksphy_driver[] = {
 	.resume		= kszphy_resume,
 	.cable_test_start	= ksz9x31_cable_test_start,
 	.cable_test_get_status	= ksz9x31_cable_test_get_status,
+	.set_loopback	= ksz9031_set_loopback,
 }, {
 	.phy_id		= PHY_ID_LAN8814,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
-- 
2.39.2

Re: [PATCH RFC net-next] net: phy: micrel: Improve loopback support if autoneg is enabled

[Andrew Lunn]

On Sun, Oct 13, 2024 at 10:24:30PM +0200, Gerhard Engleder wrote:
> Prior to commit 6ff3cddc365b it was possible to enable loopback with
> a defined speed. First a fixed speed was set with ETHTOOL_SLINKSETTINGS
> and afterwards the loopback was enabled. This worked, because
> genphy_loopback() uses the current speed and duplex. I used this
> mechanism to test tsnep in loopback with different speeds. A KSZ9031 PHY
> is used.
> 
> With commit 6ff3cddc365b for 1000 Mbit/s auto negotiation was enabled.
> Setting a fixed speed with ETHTOOL_SLINKSETTINGS does not work anymore
> for 1000 Mbit/s as speed and duplex of the PHY now depend on the result
> of the auto negotiation. As a result, genphy_loopback() also depends on
> the result of the auto negotiation. But enabling loopback shall be
> independent of any auto negotiation process.
> 
> Make loopback of KSZ9031 PHY work even if current speed and/or duplex of
> PHY are unkown because of autoneg.

We probably should think about the big picture, not one particular
PHY.

Russell's reading of 802.3 is that 1G requires autoneg. Hence the
change. Loopback is however special. Either the PHY needs to do
autoneg with itself, which is pretty unlikely, or it needs to ignore
autoneg and loopback packets independent of the autoneg status.

What does 802.3 say about loopback? At least the bit in BMCR must be
defined. Is there more? Any mention of how it should work in
combination with autoneg?

	Andrew

Re: [PATCH RFC net-next] net: phy: micrel: Improve loopback support if autoneg is enabled

[Russell King (Oracle)]

On Mon, Oct 14, 2024 at 03:18:29PM +0200, Andrew Lunn wrote:
> Russell's reading of 802.3 is that 1G requires autoneg. Hence the
> change. Loopback is however special. Either the PHY needs to do
> autoneg with itself, which is pretty unlikely, or it needs to ignore
> autoneg and loopback packets independent of the autoneg status.
> 
> What does 802.3 say about loopback? At least the bit in BMCR must be
> defined. Is there more? Any mention of how it should work in
> combination with autoneg?

Loopback is not defined to a great degree in 802.3, its only suggesting
that as much of the PHY data path should be exercised as possible.
However, it does state in clause 22 that the duplex bit should not
affect loopback.

It doesn't make any reference to speed or autoneg.

Given that PHYs that support multiple speeds need to have different
data paths through them, and the requirement for loopback to use as
much of the data path as possible, it does seem sensible that some
PHYs may not be able to negotiate with themselves in loopback mode,
(e.g. at 1G speeds, one PHY needs to be master the other slave, how
does a single PHY become both master and slave at the same time...)
then maybe forced speed is necessary when entering loopback.

So probably yes, when entering loopback, we probably ought to force
the PHY speed, but that gets difficult for a PHY that is down and
as autoneg enabled (what speed do we force?)

We do have the forced-settings in phy->autoneg, phy->speed and
phy->duplex even after the referred to commit, so we could use
that to switch the PHY back to a forced mode. However, I suepct
we're into PHY specific waters here - whether the PHY supports
1G without AN even in loopback mode is probably implementation
specific.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: micrel: Improve loopback support if autoneg is enabled

[Gerhard Engleder]

On 14.10.24 15:56, Russell King (Oracle) wrote:
> On Mon, Oct 14, 2024 at 03:18:29PM +0200, Andrew Lunn wrote:
>> Russell's reading of 802.3 is that 1G requires autoneg. Hence the
>> change. Loopback is however special. Either the PHY needs to do
>> autoneg with itself, which is pretty unlikely, or it needs to ignore
>> autoneg and loopback packets independent of the autoneg status.
>>
>> What does 802.3 say about loopback? At least the bit in BMCR must be
>> defined. Is there more? Any mention of how it should work in
>> combination with autoneg?
> 
> Loopback is not defined to a great degree in 802.3, its only suggesting
> that as much of the PHY data path should be exercised as possible.
> However, it does state in clause 22 that the duplex bit should not
> affect loopback.
> 
> It doesn't make any reference to speed or autoneg.
> 
> Given that PHYs that support multiple speeds need to have different
> data paths through them, and the requirement for loopback to use as
> much of the data path as possible, it does seem sensible that some
> PHYs may not be able to negotiate with themselves in loopback mode,
> (e.g. at 1G speeds, one PHY needs to be master the other slave, how
> does a single PHY become both master and slave at the same time...)
> then maybe forced speed is necessary when entering loopback.
> 
> So probably yes, when entering loopback, we probably ought to force
> the PHY speed, but that gets difficult for a PHY that is down and
> as autoneg enabled (what speed do we force?)
> 
> We do have the forced-settings in phy->autoneg, phy->speed and
> phy->duplex even after the referred to commit, so we could use
> that to switch the PHY back to a forced mode. However, I suepct
> we're into PHY specific waters here - whether the PHY supports
> 1G without AN even in loopback mode is probably implementation
> specific.

I posted as a RFC, because I felt not able to suggest a more generic
solution without any input. But I can add some facts about the KSZ9031
PHY. The data sheet agrees with Russells commit: "For 1000BASE-T mode,
auto-negotiation is required and always used to establish a link"
It also requests autoneg disable, full duplex and speed bits set for
loopback. So loopback speed is configurable. For 1000 Mbps it also
requires some slave configuration. genphy_loopback() mostly follows
the data sheet.

In my opinion the genphy_loopback() seems to work with most PHYs
or most real use cases. Otherwise, there would have been more PHY
specific set_loopback() implementations. The only problem is how
speed/duplex is determined. It is not guaranteed that phydev->speed and
phydev->duplex have reasonable values if autoneg has been triggered,
maybe because autoneg is still in progress or link is down or just
because the PHY state machine has not run so far. Always enabling
autoneg for 1000 Mbps only made the problem more apparent.

My suggestion would be to improve the speed/duplex selection for
loopback in the generic code. The selected speed/duplex should be
forwarded to genphy_loopback() or PHY specific set_loopback().
If speed/duplex have valid values, then these values should be used.
Otherwise the maximum speed/duplex of the PHY should be used.
Would that be a valid solution?

Gerhard