| Message ID | 20241202083352.3865373-1-nikita.yoush@cogentembedded.com (mailing list archive) |
|---|---|
| State | Rejected |
| 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.
> As I say, need to see the code. Otherwise... sorry, I'm no longer > interested in your problem. We already got valuable information. I agree that the patch I tried to submit is a wrong way to handle the issue we have. Instead, need to improve the PHY driver stub, such that it does not declare no autoneg support for 2.5G Base-T1 PHY. (creating a real driver for the PHY is not possible at this time due to lack of technical information) Thank you. Nikita
> Does IEEE 802.3 allow auto-negotiation to be turned off for speeds > greater than 1Gbps? Per 802.3-2022 ch, autoneg is optional: | 125.2.4.3 Auto-Negotiation, type single differential-pair media | | Auto-Negotiation (Clause 98) may be used by 2.5GBASE-T1 and | 5GBASE-T1 devices to detect the abilities (modes of operation) | supported by the device at the other end of a link segment, | determine common abilities, and configure for joint operation. | Auto-Negotiation is performed upon link startup through the use | of half-duplex differential Manchester encoding. | | The use of Clause 98 Auto-Negotiation is optional for 2.5GBASE-T1 | and 5GBASE-T1 PHYs.
Hi, according to IEE 802.3-2022, ch. 125.2.4.3, Auto-Negotiation is optional for 2.5GBASE-T1 > 125.2.4.3 Auto-Negotiation, type single differential-pair media > Auto-Negotiation (Clause 98) may be used by 2.5GBASE-T1 and 5GBASE-T1 devices to detect the > abilities (modes of operation) supported by the device at the other end of a link segment, determine common > abilities, and configure for joint operation. Auto-Negotiation is performed upon link startup through the use > of half-duplex differential Manchester encoding. > The use of Clause 98 Auto-Negotiation is optional for 2.5GBASE-T1 and 5GBASE-T1 PHYs So, purposed change could make sense for T1 PHYs. BR Dennis Ostermann > -----Original Message----- > From: Russell King <linux@armlinux.org.uk> > Sent: Monday, December 2, 2024 5:03 PM > To: nikita.yoush <nikita.yoush@cogentembedded.com> > Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>; Andrew Lunn > <andrew@lunn.ch>; Heiner Kallweit <hkallweit1@gmail.com>; David S. Miller > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski > <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Michael Dege > <michael.dege@renesas.com>; Christian Mardmoeller > <christian.mardmoeller@renesas.com>; Dennis Ostermann > <dennis.ostermann@renesas.com> > Subject: Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any > supported speed > > 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. > > -- > RMK's Patch system: > https://www.arml/ > inux.org.uk%2Fdeveloper%2Fpatches%2F&data=05%7C02%7Cdennis.ostermann%40ren > esas.com%7Cbfa991c0ba974db7c9ea08dd12eadd3d%7C53d82571da1947e49cb4625a166a > 4a2a%7C0%7C0%7C638687522161126854%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGki > OnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=EBEG33bbhh3A7DQMfoVJNOXeGyJsQ%2FaVk8xjS8DK17s%3D&res > erved=0 > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Dec 03, 2024 at 02:05:07PM +0000, Dennis Ostermann wrote: > Hi, > > according to IEE 802.3-2022, ch. 125.2.4.3, Auto-Negotiation is optional for 2.5GBASE-T1 > > > 125.2.4.3 Auto-Negotiation, type single differential-pair media > > Auto-Negotiation (Clause 98) may be used by 2.5GBASE-T1 and 5GBASE-T1 devices to detect the > > abilities (modes of operation) supported by the device at the other end of a link segment, determine common > > abilities, and configure for joint operation. Auto-Negotiation is performed upon link startup through the use > > of half-duplex differential Manchester encoding. > > The use of Clause 98 Auto-Negotiation is optional for 2.5GBASE-T1 and 5GBASE-T1 PHYs > > So, purposed change could make sense for T1 PHYs. The proposed change it too liberal. We need the PHY to say it supports 2.5GBASE-T1, not 2.5GBASE-T. We can then allow 2.5GBASE-T1 to not use autoneg, but 2.5GBASE-T has to use autoneg. Andrew
On Tue, Dec 03, 2024 at 04:01:56PM +0500, Nikita Yushchenko wrote: > > As I say, need to see the code. Otherwise... sorry, I'm no longer > > interested in your problem. > > We already got valuable information. > > I agree that the patch I tried to submit is a wrong way to handle the issue > we have. Instead, need to improve the PHY driver stub, such that it does not > declare no autoneg support for 2.5G Base-T1 PHY. > > (creating a real driver for the PHY is not possible at this time due to lack of technical information) Even seeing the existing code would help, rather than the current situation which means we're poking about in total darkness. It doesn't need to be "mainline quality". There is nothing worse for a maintainer than to have someone trying to fix a problem, but not be able to know the full details. We're not asking for "a real driver", but just _seeing_ what the driver is currently doing will help to answer questions such as why phydev->duplex is changing. Not being able to see what a driver is doing is very demotivating.
On Tue, Dec 03, 2024 at 03:45:09PM +0100, Andrew Lunn wrote: > On Tue, Dec 03, 2024 at 02:05:07PM +0000, Dennis Ostermann wrote: > > Hi, > > > > according to IEE 802.3-2022, ch. 125.2.4.3, Auto-Negotiation is optional for 2.5GBASE-T1 > > > > > 125.2.4.3 Auto-Negotiation, type single differential-pair media > > > Auto-Negotiation (Clause 98) may be used by 2.5GBASE-T1 and 5GBASE-T1 devices to detect the > > > abilities (modes of operation) supported by the device at the other end of a link segment, determine common > > > abilities, and configure for joint operation. Auto-Negotiation is performed upon link startup through the use > > > of half-duplex differential Manchester encoding. > > > The use of Clause 98 Auto-Negotiation is optional for 2.5GBASE-T1 and 5GBASE-T1 PHYs > > > > So, purposed change could make sense for T1 PHYs. > > The proposed change it too liberal. We need the PHY to say it supports > 2.5GBASE-T1, not 2.5GBASE-T. We can then allow 2.5GBASE-T1 to not use > autoneg, but 2.5GBASE-T has to use autoneg. I'm wondering whether we should add: __ETHTOOL_DECLARE_LINK_MODE_MASK(requires_an); to struct phy_device, and have phylib populate that by default with all base-T link modes > 1G (which would be the default case as it is now.) Then, PHY drivers can change this bitmap as they need for their device. After the PHY features have been discovered, this should then get AND-ed with the supported bitmap. We can then check this in ksettings_set() to determine whether the fixed speed requires AN. This would be more flexible, and allow future cases to be handled. Good idea, or over-engineering at this point? Another idea would be to have a boolean in struct phy_device that identifies the PHY as base-T1, and makes an exception that way.
Hi Andrew, On Tue, 3 Dec 2024 15:21:27 +0000 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Tue, Dec 03, 2024 at 03:45:09PM +0100, Andrew Lunn wrote: > > On Tue, Dec 03, 2024 at 02:05:07PM +0000, Dennis Ostermann wrote: > > > Hi, > > > > > > according to IEE 802.3-2022, ch. 125.2.4.3, Auto-Negotiation is optional for 2.5GBASE-T1 > > > > > > > 125.2.4.3 Auto-Negotiation, type single differential-pair media > > > > Auto-Negotiation (Clause 98) may be used by 2.5GBASE-T1 and 5GBASE-T1 devices to detect the > > > > abilities (modes of operation) supported by the device at the other end of a link segment, determine common > > > > abilities, and configure for joint operation. Auto-Negotiation is performed upon link startup through the use > > > > of half-duplex differential Manchester encoding. > > > > The use of Clause 98 Auto-Negotiation is optional for 2.5GBASE-T1 and 5GBASE-T1 PHYs > > > > > > So, purposed change could make sense for T1 PHYs. > > > > The proposed change it too liberal. We need the PHY to say it supports > > 2.5GBASE-T1, not 2.5GBASE-T. We can then allow 2.5GBASE-T1 to not use > > autoneg, but 2.5GBASE-T has to use autoneg. > > I'm wondering whether we should add: > > __ETHTOOL_DECLARE_LINK_MODE_MASK(requires_an); > > to struct phy_device, and have phylib populate that by default with all > base-T link modes > 1G (which would be the default case as it is now.) > Then, PHY drivers can change this bitmap as they need for their device. > After the PHY features have been discovered, this should then get > AND-ed with the supported bitmap. If the standards says that BaseT4 >1G needs aneg, and that we can't have it for baseT1, couldn't we just have some lookup table for each mode indicating if they need or support aneg ? I'm thinking about something similar as the big table in net/ethtool/common.c where we have the linkmode - speed - duplex - lanes mapping : https://elixir.bootlin.com/linux/v6.12.1/source/net/ethtool/common.c#L270 maybe looking it up for each config operation would be too expensive ? or it maybe isn't flexible enough in case we have to deal with phy-pecific quirks... Maxime
On Tue, Dec 03, 2024 at 04:51:47PM +0100, Maxime Chevallier wrote: > Hi Andrew, > > On Tue, 3 Dec 2024 15:21:27 +0000 > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > > > On Tue, Dec 03, 2024 at 03:45:09PM +0100, Andrew Lunn wrote: > > > On Tue, Dec 03, 2024 at 02:05:07PM +0000, Dennis Ostermann wrote: > > > > Hi, > > > > > > > > according to IEE 802.3-2022, ch. 125.2.4.3, Auto-Negotiation is optional for 2.5GBASE-T1 > > > > > > > > > 125.2.4.3 Auto-Negotiation, type single differential-pair media > > > > > Auto-Negotiation (Clause 98) may be used by 2.5GBASE-T1 and 5GBASE-T1 devices to detect the > > > > > abilities (modes of operation) supported by the device at the other end of a link segment, determine common > > > > > abilities, and configure for joint operation. Auto-Negotiation is performed upon link startup through the use > > > > > of half-duplex differential Manchester encoding. > > > > > The use of Clause 98 Auto-Negotiation is optional for 2.5GBASE-T1 and 5GBASE-T1 PHYs > > > > > > > > So, purposed change could make sense for T1 PHYs. > > > > > > The proposed change it too liberal. We need the PHY to say it supports > > > 2.5GBASE-T1, not 2.5GBASE-T. We can then allow 2.5GBASE-T1 to not use > > > autoneg, but 2.5GBASE-T has to use autoneg. > > > > I'm wondering whether we should add: > > > > __ETHTOOL_DECLARE_LINK_MODE_MASK(requires_an); > > > > to struct phy_device, and have phylib populate that by default with all > > base-T link modes > 1G (which would be the default case as it is now.) > > Then, PHY drivers can change this bitmap as they need for their device. > > After the PHY features have been discovered, this should then get > > AND-ed with the supported bitmap. > > If the standards says that BaseT4 >1G needs aneg, and that we can't > have it for baseT1, couldn't we just have some lookup table for each > mode indicating if they need or support aneg ? When operating in !AN mode, all that the ethtool API passes is the speed and duplex, with a guess at the advertising mask (which doesn't take account of the PHY's supported ethtool link modes.) If e.g. we have a PHY that supports 1000base-T and 1000base-X, and the user attempts to disable AN specifying speed 1000 duplex full, we don't know whether the user means 1000base-T (which actually requires AN, but we work around that *) or 1000base-X without AN. Specifying speed + duplex for !AN is nice for humans, but ambiguous for computers. * - the workaround adopted is to do what Marvell PHYs internally do but in phylib code, which is to accept the request to disable AN and operate at the specified speed, but actually program AN to be enabled with only a single speed/duplex that can be negotiated. Without this, we end up with hacks in MAC drivers because the PHY they're paired with doesn't support AN being disabled at 1G speed. See __genphy_config_aneg(). Note: this is probably going to interact badly with the baseT1 case.
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(-)