| Message ID | 20241013202430.93851-1-gerhard@engleder-embedded.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] net: phy: micrel: Improve loopback support if autoneg is enabled | expand |
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
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.
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
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,
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(+)