[PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Nikita Yushchenko]

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(-)

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);
-- 
2.39.5

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Maxime Chevallier]

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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Nikita Yushchenko]

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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Maxime Chevallier]

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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Russell King (Oracle)]

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.

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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Russell King (Oracle)]

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.

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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Nikita Yushchenko]

>> 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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Russell King (Oracle)]

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?

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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Nikita Yushchenko]

>> 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
...

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Russell King (Oracle)]

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.

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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Andrew Lunn]

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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Nikita Yushchenko]

>> 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.

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Russell King (Oracle)]

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.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Nikita Yushchenko]

> 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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Nikita Yushchenko]

> 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.

RE: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Dennis Ostermann]

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!
________________________________

Renesas Electronics Europe GmbH
Registered Office: Arcadiastrasse 10
DE-40472 Duesseldorf
Commercial Registry: Duesseldorf, HRB 3708
Managing Director: Carsten Jauch
VAT-No.: DE 14978647
Tax-ID-No: 105/5839/1793

Legal Disclaimer: This e-mail communication (and any attachment/s) is confidential and contains proprietary information, some or all of which may be legally privileged. It is intended solely for the use of the individual or entity to which it is addressed. Access to this email by anyone else is unauthorized. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Andrew Lunn]

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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Russell King (Oracle)]

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.

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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Russell King (Oracle)]

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.

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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Maxime Chevallier]

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

Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

[Russell King (Oracle)]

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.

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