| Message ID | 20241202083352.3865373-1-nikita.yoush@cogentembedded.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: phy_ethtool_ksettings_set: Allow any supported speed | expand |
Hello Nikita, On Mon, 2 Dec 2024 13:33:52 +0500 Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote: > When auto-negotiation is not used, allow any speed/duplex pair > supported by the PHY, not only 10/100/1000 half/full. > > This enables drivers to use phy_ethtool_set_link_ksettings() in their > ethtool_ops and still support configuring PHYs for speeds above 1 GBps. > > Also this will cause an error return on attempt to manually set > speed/duplex pair that is not supported by the PHY. There have been attempts to do the same thing before : https://lore.kernel.org/netdev/1c55b353-ddaf-48f2-985c-5cb67bd5cb0c@lunn.ch/ It seems that 1G and above require autoneg to properly work. The 802.3 spec for 2.5G/5G (126.6.1 Support for Auto-Negotiation) does say : All 2.5GBASE-T and 5GBASE-T PHYs shall provide support for Auto-Negotiation (Clause 28) and shall be capable of operating as MASTER or SLAVE. [...] Auto-Negotiation is performed as part of the initial set-up of the link, and allows the PHYs at each end to advertise their capabilities (speed, PHY type, half or full duplex) and to automatically select the operating mode for communication on the link. Auto-Negotiation signaling is used for the following primary purposes for 2.5GBASE-T and 5GBASE-T: a) To negotiate that the PHY is capable of supporting 2.5GBASE-T or 5GBASE-T transmission. b) To determine the MASTER-SLAVE relationship between the PHYs at each end of the link. Looking at this it does seem that autoneg should stay enabled when operating at other speeds than 10/100/1000, at least in BaseT. What's your use-case to need >1G fixed-settings link ? Thanks, Maxime
Hello.
> What's your use-case to need >1G fixed-settings link ?
My hardware is Renesas VC4 board (based on Renesas S4 SoC), network driver is rswitch, PHY in question
is Marvell 88Q3344 (2.5G Base-T1).
To get two such PHYs talk to each other, one of the two has to be manually configured as slave.
(e.g. ethtool -s tsn0 master-slave forced-slave).
This gets handled via driver's ethtool set_link_ksettings method, which is currently set to
phy_ethtool_ksettings_set().
Writing a custom set_link_ksettings method just to not error out when speed is 2500 looks ugly.
Nikita
Hello Nikita, On Mon, 2 Dec 2024 14:20:17 +0500 Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote: > Hello. > > > What's your use-case to need >1G fixed-settings link ? > > My hardware is Renesas VC4 board (based on Renesas S4 SoC), network driver is rswitch, PHY in question > is Marvell 88Q3344 (2.5G Base-T1). Ok so it's baseT1, which is indeed different than the BaseT4 case I was mentionning. It could be good to include that in the commit log :) > To get two such PHYs talk to each other, one of the two has to be manually configured as slave. > (e.g. ethtool -s tsn0 master-slave forced-slave). > > This gets handled via driver's ethtool set_link_ksettings method, which is currently set to > phy_ethtool_ksettings_set(). > > Writing a custom set_link_ksettings method just to not error out when speed is 2500 looks ugly. Yes and this would apply to any PHY that does >1G BaseT1. The thing is, while it does solve the problem you're facing, the current proposition will impact 2.5G/5G/10GBaseT4. I don't think you need to write a custom set_link_ksettings, however we should make an exception for BaseT1. Maybe add an extra condition in phy_ethtool_ksettings_set() to check in the advertising/supported if we are dealing with a BaseT1 PHY, and if so bypass the check for 10/100/1000 speeds, as it doesn't apply in your case ? Maybe the PHY maintainers have better ideas though. Thanks, Maxime
On Mon, Dec 02, 2024 at 01:33:52PM +0500, Nikita Yushchenko wrote: > When auto-negotiation is not used, allow any speed/duplex pair > supported by the PHY, not only 10/100/1000 half/full. > > This enables drivers to use phy_ethtool_set_link_ksettings() in their > ethtool_ops and still support configuring PHYs for speeds above 1 GBps. > > Also this will cause an error return on attempt to manually set > speed/duplex pair that is not supported by the PHY. Does IEEE 802.3 allow auto-negotiation to be turned off for speeds greater than 1Gbps? My research for 1G speeds indicated that AN is required as part of the establishment of link parameters other than the capabilities of each end. We have PHYs that require AN to be turned on for 1G speeds, and other PHYs that allow the AN enable bit to be cleared, but internally keep it enabled for 1G. To eliminate patches in drivers that force AN for 1G or error out the ksettings_set call, phylib now emulates the advertisement for all PHYs and keeps AN enabled when the user requests fixed-speed 1G, which is what Marvell PHYs do and is the most sensible solution. Presently, I don't think it makes sense to turn off AN for speeds beyond 1G. You need to provide a very good reason for why this is desired, a real use for it, and indicate why it would be safe to do.
On Mon, Dec 02, 2024 at 02:20:17PM +0500, Nikita Yushchenko wrote: > My hardware is Renesas VC4 board (based on Renesas S4 SoC), network driver > is rswitch, PHY in question is Marvell 88Q3344 (2.5G Base-T1). > > To get two such PHYs talk to each other, one of the two has to be manually configured as slave. > (e.g. ethtool -s tsn0 master-slave forced-slave). I don't see what that has to do with whether AN is enabled or not. Forcing master/slave mode is normally independent of whether AN is enabled. There's four modes for it. MASTER_PREFERRED - this causes the PHY to generate a seed that gives a higher chance that it will be chosen as the master. SLAVE_PREFERRED - ditto but biased towards being a slace. MASTER_FORCE and SLAVE_FORCE does what it says on the tin. We may not be implementing this for clause 45 PHYs.
>> To get two such PHYs talk to each other, one of the two has to be manually configured as slave. >> (e.g. ethtool -s tsn0 master-slave forced-slave). > > I don't see what that has to do with whether AN is enabled or not. > Forcing master/slave mode is normally independent of whether AN is > enabled. > > There's four modes for it. MASTER_PREFERRED - this causes the PHY to > generate a seed that gives a higher chance that it will be chosen as > the master. SLAVE_PREFERRED - ditto but biased towards being a slace. > MASTER_FORCE and SLAVE_FORCE does what it says on the tin. > > We may not be implementing this for clause 45 PHYs. Right now, 'ethtool -s tsn0 master-slave forced-slave' causes a call to driver's ethtool set_link_ksettings method. Which does error out for me because at the call time, speed field is 2500. Do you mean that the actual issue is elsewhere, e.g. the 2.5G PHY driver must not ever allow configuration without autoneg? Also for Base-T1? Nikita
On Mon, Dec 02, 2024 at 03:17:08PM +0500, Nikita Yushchenko wrote: > > > To get two such PHYs talk to each other, one of the two has to be manually configured as slave. > > > (e.g. ethtool -s tsn0 master-slave forced-slave). > > > > I don't see what that has to do with whether AN is enabled or not. > > Forcing master/slave mode is normally independent of whether AN is > > enabled. > > > > There's four modes for it. MASTER_PREFERRED - this causes the PHY to > > generate a seed that gives a higher chance that it will be chosen as > > the master. SLAVE_PREFERRED - ditto but biased towards being a slace. > > MASTER_FORCE and SLAVE_FORCE does what it says on the tin. > > > > We may not be implementing this for clause 45 PHYs. > > Right now, 'ethtool -s tsn0 master-slave forced-slave' causes a call to > driver's ethtool set_link_ksettings method. Which does error out for me > because at the call time, speed field is 2500. Are you saying that the PHY starts in fixed-speed 2.5G mode? What does ethtool tsn0 say after boot and the link has come up but before any ethtool settings are changed?
>> Right now, 'ethtool -s tsn0 master-slave forced-slave' causes a call to >> driver's ethtool set_link_ksettings method. Which does error out for me >> because at the call time, speed field is 2500. > > Are you saying that the PHY starts in fixed-speed 2.5G mode? > > What does ethtool tsn0 say after boot and the link has come up but > before any ethtool settings are changed? On a freshly booted board, with /etc/systemd/network temporary moved away. (there are two identical boards, connected to each other) root@vc4-033:~# ip l show dev tsn0 19: tsn0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 link/ether 3a:e3:5c:56:ba:bd brd ff:ff:ff:ff:ff:ff root@vc4-033:~# ethtool tsn0 Settings for tsn0: Supported ports: [ MII ] Supported link modes: 2500baseT/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: No Supported FEC modes: Not reported Advertised link modes: 2500baseT/Full Advertised pause frame use: No Advertised auto-negotiation: No Advertised FEC modes: Not reported Speed: 2500Mb/s Duplex: Unknown! (255) Auto-negotiation: off master-slave cfg: unknown Port: Twisted Pair PHYAD: 0 Transceiver: external MDI-X: Unknown PHY driver is out of tree and can do things wrong. AFAIU it does nothing more than wrapping Marvell setup sequences into a phy driver skeleton. Still, with the patch in question applied, things just work: root@vc4-033:~# ip l set dev tsn0 up root@vc4-033:~# ethtool -s tsn0 master-slave forced-slave [ 83.743711] renesas_eth_sw e68c0000.ethernet tsn0: Link is Up - 2.5Gbps/Full - flow control off root@vc4-033:~# ethtool tsn0 Settings for tsn0: Supported ports: [ MII ] Supported link modes: 2500baseT/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: No Supported FEC modes: Not reported Advertised link modes: 2500baseT/Full Advertised pause frame use: No Advertised auto-negotiation: No Advertised FEC modes: Not reported Speed: 2500Mb/s Duplex: Full Auto-negotiation: off master-slave cfg: forced slave master-slave status: slave Port: Twisted Pair PHYAD: 0 Transceiver: external MDI-X: Unknown root@vc4-033:~# ip a add 192.168.70.11/24 dev tsn0 root@vc4-033:~# ping 192.168.70.10 PING 192.168.70.10 (192.168.70.10) 56(84) bytes of data. 64 bytes from 192.168.70.10: icmp_seq=1 ttl=64 time=1.03 ms 64 bytes from 192.168.70.10: icmp_seq=2 ttl=64 time=0.601 ms ...
On Mon, Dec 02, 2024 at 04:09:43PM +0500, Nikita Yushchenko wrote: > > > Right now, 'ethtool -s tsn0 master-slave forced-slave' causes a call to > > > driver's ethtool set_link_ksettings method. Which does error out for me > > > because at the call time, speed field is 2500. > > > > Are you saying that the PHY starts in fixed-speed 2.5G mode? > > > > What does ethtool tsn0 say after boot and the link has come up but > > before any ethtool settings are changed? > > On a freshly booted board, with /etc/systemd/network temporary moved away. > > (there are two identical boards, connected to each other) > > root@vc4-033:~# ip l show dev tsn0 > 19: tsn0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > link/ether 3a:e3:5c:56:ba:bd brd ff:ff:ff:ff:ff:ff > > root@vc4-033:~# ethtool tsn0 > Settings for tsn0: > Supported ports: [ MII ] > Supported link modes: 2500baseT/Full > Supported pause frame use: Symmetric Receive-only > Supports auto-negotiation: No Okay, the PHY can apparently only operate in fixed mode, although I would suggest checking that is actually the case. I suspect that may be a driver bug, especially as... > Supported FEC modes: Not reported > Advertised link modes: 2500baseT/Full > Advertised pause frame use: No > Advertised auto-negotiation: No > Advertised FEC modes: Not reported > Speed: 2500Mb/s > Duplex: Unknown! (255) ... after link up: > Speed: 2500Mb/s > Duplex: Full it changes phydev->duplex, which is _not_ supposed to happen if negotiation has been disabled. When negitation is disabled, phydev->speed and phydev->duplex are supposed to set the link parameters, and the PHY driver is not supposed to alter them from what was set, possibly by the user. So there is something weird going on in the driver, and without seeing it, I'm not sure whether (a) it's just a badly coded driver that the PHY does support AN but the driver has decided to tell the kernel it doesn't, (b) whether it truly is not using AN.
On Mon, Dec 02, 2024 at 04:09:43PM +0500, Nikita Yushchenko wrote: > > > Right now, 'ethtool -s tsn0 master-slave forced-slave' causes a call to > > > driver's ethtool set_link_ksettings method. Which does error out for me > > > because at the call time, speed field is 2500. > > > > Are you saying that the PHY starts in fixed-speed 2.5G mode? > > > > What does ethtool tsn0 say after boot and the link has come up but > > before any ethtool settings are changed? > > On a freshly booted board, with /etc/systemd/network temporary moved away. > > (there are two identical boards, connected to each other) > > root@vc4-033:~# ip l show dev tsn0 > 19: tsn0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > link/ether 3a:e3:5c:56:ba:bd brd ff:ff:ff:ff:ff:ff > > root@vc4-033:~# ethtool tsn0 > Settings for tsn0: > Supported ports: [ MII ] > Supported link modes: 2500baseT/Full If it is a T1 PHY, we want it reporting 25000BaseT1/Full here. Having T1 then probably allows us to unlock forced master/slave without autoneg, and setting speeds above 1G without autoneg. Given this is an out of tree driver, i can understand why it does not, it means patching a number of in tree files. https://www.marvell.com/products/automotive/88q4364.html says it can actually do 2.5G/5G/10GBASE-T1 as defined by the IEEE 802.3ch standard. I would be reluctant to make changes to phylib without a kernel quality PHY driver queued for merging. So you might want to spend some time cleaning up the code. FYI: I've not looked at 802.3ch, but if that defines registers, i would expect the driver patches to actually be split into helpers for standard defined registers any 3ch driver can use, and a PHY driver gluing those together and accessing marvell specific registers. Andrew
>> root@vc4-033:~# ethtool tsn0 >> Settings for tsn0: >> Supported ports: [ MII ] >> Supported link modes: 2500baseT/Full >> Supported pause frame use: Symmetric Receive-only >> Supports auto-negotiation: No > > Okay, the PHY can apparently only operate in fixed mode, although I > would suggest checking that is actually the case. I suspect that may > be a driver bug, especially as... My contacts from Renesas say that this PHY chip is an engineering sample. I'm not sure about the origin of "driver" for this. I did not look inside before, but now I did, and it is almost completely a stub. Even no init sequence. The only hw operations that this stub does are (1) reading bit 0 of register 1.0901 and returning it as link status (phydev->link), (2) reading bit 0 of register 1.0000 and returning it as master/slave setting (phydev->master_slave_get / phydev->master_slave_state) (3) applying phydev->master_slave_set via writing to bit 0 of register 1.0000 and then writing 0x200 to register 7.0200 Per standard, writing 0x200 to 7.0200 is autoneg restart, however bit 0 of 1.0000 has nothing to do with master/slave. So what device actually does is unclear. Just a black box that provides 2.5G Base-T1 signalling, and software-wise can only report link and accept master-slave configuration. Not sure if supporting this sort of black box worths kernel changes. > it changes phydev->duplex, which is _not_ supposed to happen if > negotiation has been disabled. There are no writes to phydev->duplex inside the "driver". Something in the phy core is changing it.
On Mon, Dec 02, 2024 at 08:51:44PM +0500, Nikita Yushchenko wrote: > > > root@vc4-033:~# ethtool tsn0 > > > Settings for tsn0: > > > Supported ports: [ MII ] > > > Supported link modes: 2500baseT/Full > > > Supported pause frame use: Symmetric Receive-only > > > Supports auto-negotiation: No > > > > Okay, the PHY can apparently only operate in fixed mode, although I > > would suggest checking that is actually the case. I suspect that may > > be a driver bug, especially as... > > My contacts from Renesas say that this PHY chip is an engineering sample. > > I'm not sure about the origin of "driver" for this. I did not look inside > before, but now I did, and it is almost completely a stub. Even no init > sequence. The only hw operations that this stub does are > (1) reading bit 0 of register 1.0901 and returning it as link status (phydev->link), > (2) reading bit 0 of register 1.0000 and returning it as master/slave > setting (phydev->master_slave_get / phydev->master_slave_state) > (3) applying phydev->master_slave_set via writing to bit 0 of register > 1.0000 and then writing 0x200 to register 7.0200 > > Per standard, writing 0x200 to 7.0200 is autoneg restart, however bit 0 of > 1.0000 has nothing to do with master/slave. So what device actually does is > unclear. Just a black box that provides 2.5G Base-T1 signalling, and > software-wise can only report link and accept master-slave configuration. > > Not sure if supporting this sort of black box worths kernel changes. > > > > it changes phydev->duplex, which is _not_ supposed to happen if > > negotiation has been disabled. > > There are no writes to phydev->duplex inside the "driver". > Something in the phy core is changing it. Maybe it's calling phylib functions? Shrug, I'm losing interest in this problem without seeing the driver code. There's just too much unknown here. It's not so much about what the driver does with the hardware. We have some T1 library functions. We don't know which are being used (if any). Phylib won't randomly change phydev->duplex unless a library function that e.g. reads status from the PHY does it. As I say, need to see the code. Otherwise... sorry, I'm no longer interested in your problem.
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 4f3e742907cb..1f85a90cb3fc 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -1101,11 +1101,7 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev, return -EINVAL; if (autoneg == AUTONEG_DISABLE && - ((speed != SPEED_1000 && - speed != SPEED_100 && - speed != SPEED_10) || - (duplex != DUPLEX_HALF && - duplex != DUPLEX_FULL))) + !phy_check_valid(speed, duplex, phydev->supported)) return -EINVAL; mutex_lock(&phydev->lock);
When auto-negotiation is not used, allow any speed/duplex pair supported by the PHY, not only 10/100/1000 half/full. This enables drivers to use phy_ethtool_set_link_ksettings() in their ethtool_ops and still support configuring PHYs for speeds above 1 GBps. Also this will cause an error return on attempt to manually set speed/duplex pair that is not supported by the PHY. Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> --- drivers/net/phy/phy.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)