Message-ID: <3c77d9b2-0933-4da5-a12b-1dd7ebcfebad@engleder-embedded.com>
Date: Mon, 14 Oct 2024 20:40:39 +0200
From: Gerhard Engleder <gerhard@...leder-embedded.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
 Andrew Lunn <andrew@...n.ch>
Cc: hkallweit1@...il.com, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org
Subject: Re: [PATCH RFC net-next] net: phy: micrel: Improve loopback support
 if autoneg is enabled

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

Powered by blists - more mailing lists