| Message ID | 20240906133951.3433788-4-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | net: phy: marvell-88q2xxx: Enable auto negotiation for mv88q2110 | expand |
On Fri, Sep 06, 2024 at 03:39:51PM +0200, Niklas Söderlund wrote: > The initial marvell-88q2xxx driver only supported the Marvell 88Q2110 > PHY without auto negotiation support. The reason documented states that > the provided initialization sequence did not to work. Now a method to > enable auto negotiation have been found by comparing the initialization > of other supported devices and an out-of-tree PHY driver. > > Perform the minimal needed initialization of the PHY to get auto > negotiation working and remove the limitation that disables the auto > negotiation feature for the mv88q2110 device. > > With this change a 1000Mbps full duplex link is able to be negotiated > between two mv88q2110 and the link works perfectly. The other side also > reflects the manually configure settings of the master device. > > # ethtool eth0 > Settings for eth0: > Supported ports: [ ] > Supported link modes: 100baseT1/Full > 1000baseT1/Full > Supported pause frame use: Symmetric Receive-only > Supports auto-negotiation: Yes My understanding is that most automotive applications using T1 don't actually want auto-neg, because it is slow. Given the static use case, everything can be statically configured. Is there a danger this change is going to cause regressions? There are users who are happy for it to use 100BaseT1 without negotiation, and the link partner is not offering any sort of negotiation. But with this change, autoneg is now the default, and the link fails to come up? To me, this actually seems like a generic problem for automotive. We want to indicate the device does support autoneg, but we don't want it on by default? I don't know if we can express that at the moment? Andrew
On 2024-09-06 22:36:49 +0200, Andrew Lunn wrote: > On Fri, Sep 06, 2024 at 03:39:51PM +0200, Niklas Söderlund wrote: > > The initial marvell-88q2xxx driver only supported the Marvell 88Q2110 > > PHY without auto negotiation support. The reason documented states that > > the provided initialization sequence did not to work. Now a method to > > enable auto negotiation have been found by comparing the initialization > > of other supported devices and an out-of-tree PHY driver. > > > > Perform the minimal needed initialization of the PHY to get auto > > negotiation working and remove the limitation that disables the auto > > negotiation feature for the mv88q2110 device. > > > > With this change a 1000Mbps full duplex link is able to be negotiated > > between two mv88q2110 and the link works perfectly. The other side also > > reflects the manually configure settings of the master device. > > > > # ethtool eth0 > > Settings for eth0: > > Supported ports: [ ] > > Supported link modes: 100baseT1/Full > > 1000baseT1/Full > > Supported pause frame use: Symmetric Receive-only > > Supports auto-negotiation: Yes > > My understanding is that most automotive applications using T1 don't > actually want auto-neg, because it is slow. Given the static use case, > everything can be statically configured. > > Is there a danger this change is going to cause regressions? There are > users who are happy for it to use 100BaseT1 without negotiation, and > the link partner is not offering any sort of negotiation. But with > this change, autoneg is now the default, and the link fails to come > up? I'm not sure how the generic use-case looks like. All I can say all other devices supported by this driver support autoneg by default and the initial commit adds some of the autoneg features but disables it with a comment that they could not get it to work. > > To me, this actually seems like a generic problem for automotive. We > want to indicate the device does support autoneg, but we don't want it > on by default? I don't know if we can express that at the moment? > > Andrew
On Fri, Sep 06, 2024 at 11:39:23PM +0200, Niklas Söderlund wrote: > On 2024-09-06 22:36:49 +0200, Andrew Lunn wrote: > > On Fri, Sep 06, 2024 at 03:39:51PM +0200, Niklas Söderlund wrote: > > > The initial marvell-88q2xxx driver only supported the Marvell 88Q2110 > > > PHY without auto negotiation support. The reason documented states that > > > the provided initialization sequence did not to work. Now a method to > > > enable auto negotiation have been found by comparing the initialization > > > of other supported devices and an out-of-tree PHY driver. > > > > > > Perform the minimal needed initialization of the PHY to get auto > > > negotiation working and remove the limitation that disables the auto > > > negotiation feature for the mv88q2110 device. > > > > > > With this change a 1000Mbps full duplex link is able to be negotiated > > > between two mv88q2110 and the link works perfectly. The other side also > > > reflects the manually configure settings of the master device. > > > > > > # ethtool eth0 > > > Settings for eth0: > > > Supported ports: [ ] > > > Supported link modes: 100baseT1/Full > > > 1000baseT1/Full > > > Supported pause frame use: Symmetric Receive-only > > > Supports auto-negotiation: Yes > > > > My understanding is that most automotive applications using T1 don't > > actually want auto-neg, because it is slow. Given the static use case, > > everything can be statically configured. > > > > Is there a danger this change is going to cause regressions? There are > > users who are happy for it to use 100BaseT1 without negotiation, and > > the link partner is not offering any sort of negotiation. But with > > this change, autoneg is now the default, and the link fails to come > > up? > > I'm not sure how the generic use-case looks like. All I can say all > other devices supported by this driver support autoneg by default and > the initial commit adds some of the autoneg features but disables it > with a comment that they could not get it to work. I'm just worried about reports of regressions. It could be you are currently the only user of this driver, and you obviously don't care about the change in behaviour, and can change the configuration of the other end so that it also does autoneg. But then again, Stefan Eichenberger <eichest@gmail.com> added this driver. Does autoneg by default, not forced, cause problems for his systems? Stefan? Andrew
Hi Andrew and Niklas, On Tue, 10 Sept 2024 at 17:32, Andrew Lunn <andrew@lunn.ch> wrote: > > On Fri, Sep 06, 2024 at 11:39:23PM +0200, Niklas Söderlund wrote: > > On 2024-09-06 22:36:49 +0200, Andrew Lunn wrote: > > > On Fri, Sep 06, 2024 at 03:39:51PM +0200, Niklas Söderlund wrote: > > > > The initial marvell-88q2xxx driver only supported the Marvell 88Q2110 > > > > PHY without auto negotiation support. The reason documented states that > > > > the provided initialization sequence did not to work. Now a method to > > > > enable auto negotiation have been found by comparing the initialization > > > > of other supported devices and an out-of-tree PHY driver. > > > > > > > > Perform the minimal needed initialization of the PHY to get auto > > > > negotiation working and remove the limitation that disables the auto > > > > negotiation feature for the mv88q2110 device. > > > > > > > > With this change a 1000Mbps full duplex link is able to be negotiated > > > > between two mv88q2110 and the link works perfectly. The other side also > > > > reflects the manually configure settings of the master device. > > > > > > > > # ethtool eth0 > > > > Settings for eth0: > > > > Supported ports: [ ] > > > > Supported link modes: 100baseT1/Full > > > > 1000baseT1/Full > > > > Supported pause frame use: Symmetric Receive-only > > > > Supports auto-negotiation: Yes > > > > > > My understanding is that most automotive applications using T1 don't > > > actually want auto-neg, because it is slow. Given the static use case, > > > everything can be statically configured. > > > > > > Is there a danger this change is going to cause regressions? There are > > > users who are happy for it to use 100BaseT1 without negotiation, and > > > the link partner is not offering any sort of negotiation. But with > > > this change, autoneg is now the default, and the link fails to come > > > up? > > > > I'm not sure how the generic use-case looks like. All I can say all > > other devices supported by this driver support autoneg by default and > > the initial commit adds some of the autoneg features but disables it > > with a comment that they could not get it to work. > > I'm just worried about reports of regressions. It could be you are > currently the only user of this driver, and you obviously don't care > about the change in behaviour, and can change the configuration of the > other end so that it also does autoneg. > > But then again, Stefan Eichenberger <eichest@gmail.com> added this > driver. Does autoneg by default, not forced, cause problems for his > systems? Sorry I didn't reply before, I'm currently travelling. That's also why I couldn't test the changes yet. For our use case the proposed change shouldn't be an issue. We anyway need to configure the speed from userspace. So I think the proposed change is fine from a user's perspective. It will require user space to configure the interface correctly before it starts the interface or it will fallback to autoneg. Regards, Stefan
> Sorry I didn't reply before, I'm currently travelling. That's also why I > couldn't test the changes yet. No problems. Please report back when you have tested it. > For our use case the proposed change shouldn't be an issue. O.K, it would make the whole story simpler if auto-neg was the default. So if this does not cause you any issues, lets go with that, until somebody else reports a regression. Andrew
On Fri, Sep 06, 2024 at 03:39:51PM +0200, Niklas Söderlund wrote: > The initial marvell-88q2xxx driver only supported the Marvell 88Q2110 > PHY without auto negotiation support. The reason documented states that > the provided initialization sequence did not to work. Now a method to > enable auto negotiation have been found by comparing the initialization > of other supported devices and an out-of-tree PHY driver. > > Perform the minimal needed initialization of the PHY to get auto > negotiation working and remove the limitation that disables the auto > negotiation feature for the mv88q2110 device. > > With this change a 1000Mbps full duplex link is able to be negotiated > between two mv88q2110 and the link works perfectly. The other side also > reflects the manually configure settings of the master device. > > # ethtool eth0 > Settings for eth0: > Supported ports: [ ] > Supported link modes: 100baseT1/Full > 1000baseT1/Full > Supported pause frame use: Symmetric Receive-only > Supports auto-negotiation: Yes > Supported FEC modes: Not reported > Advertised link modes: 100baseT1/Full > 1000baseT1/Full > Advertised pause frame use: No > Advertised auto-negotiation: Yes > Advertised FEC modes: Not reported > Link partner advertised link modes: 100baseT1/Full > 1000baseT1/Full > Link partner advertised pause frame use: No > Link partner advertised auto-negotiation: Yes > Link partner advertised FEC modes: Not reported > Speed: 1000Mb/s > Duplex: Full > Auto-negotiation: on > master-slave cfg: preferred master > master-slave status: slave > Port: Twisted Pair > PHYAD: 0 > Transceiver: external > MDI-X: Unknown > Link detected: yes > SQI: 15/15 > > Before this change I was not able to manually configure 1000Mbps link, > only a 100Mpps link so this change providers an improvement in > performance for this device. > > [ 5] local 10.1.0.2 port 5201 connected to 10.1.0.1 port 38346 > [ ID] Interval Transfer Bitrate Retr Cwnd > [ 5] 0.00-1.00 sec 96.8 MBytes 812 Mbits/sec 0 469 KBytes > [ 5] 1.00-2.00 sec 94.3 MBytes 791 Mbits/sec 0 469 KBytes > [ 5] 2.00-3.00 sec 96.1 MBytes 806 Mbits/sec 0 469 KBytes > [ 5] 3.00-4.00 sec 98.3 MBytes 825 Mbits/sec 0 469 KBytes > [ 5] 4.00-5.00 sec 98.4 MBytes 825 Mbits/sec 0 469 KBytes > [ 5] 5.00-6.00 sec 98.4 MBytes 826 Mbits/sec 0 469 KBytes > [ 5] 6.00-7.00 sec 98.9 MBytes 830 Mbits/sec 0 469 KBytes > [ 5] 7.00-8.00 sec 91.7 MBytes 769 Mbits/sec 0 469 KBytes > [ 5] 8.00-9.00 sec 99.4 MBytes 834 Mbits/sec 0 747 KBytes > [ 5] 9.00-10.00 sec 101 MBytes 851 Mbits/sec 0 747 KBytes > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> I will mark this one as 'change-requested', when in fact it is more a test-requested. Once we get a Tested-by: we can merge this next cycle. Andrew --- pw-bot: cr
Hi Niklas and Andrew, On Tue, Sep 10, 2024 at 10:23:18PM +0200, Andrew Lunn wrote: > On Fri, Sep 06, 2024 at 03:39:51PM +0200, Niklas Söderlund wrote: > > The initial marvell-88q2xxx driver only supported the Marvell 88Q2110 > > PHY without auto negotiation support. The reason documented states that > > the provided initialization sequence did not to work. Now a method to > > enable auto negotiation have been found by comparing the initialization > > of other supported devices and an out-of-tree PHY driver. > > > > Perform the minimal needed initialization of the PHY to get auto > > negotiation working and remove the limitation that disables the auto > > negotiation feature for the mv88q2110 device. > > > > With this change a 1000Mbps full duplex link is able to be negotiated > > between two mv88q2110 and the link works perfectly. The other side also > > reflects the manually configure settings of the master device. > > > > # ethtool eth0 > > Settings for eth0: > > Supported ports: [ ] > > Supported link modes: 100baseT1/Full > > 1000baseT1/Full > > Supported pause frame use: Symmetric Receive-only > > Supports auto-negotiation: Yes > > Supported FEC modes: Not reported > > Advertised link modes: 100baseT1/Full > > 1000baseT1/Full > > Advertised pause frame use: No > > Advertised auto-negotiation: Yes > > Advertised FEC modes: Not reported > > Link partner advertised link modes: 100baseT1/Full > > 1000baseT1/Full > > Link partner advertised pause frame use: No > > Link partner advertised auto-negotiation: Yes > > Link partner advertised FEC modes: Not reported > > Speed: 1000Mb/s > > Duplex: Full > > Auto-negotiation: on > > master-slave cfg: preferred master > > master-slave status: slave > > Port: Twisted Pair > > PHYAD: 0 > > Transceiver: external > > MDI-X: Unknown > > Link detected: yes > > SQI: 15/15 > > > > Before this change I was not able to manually configure 1000Mbps link, > > only a 100Mpps link so this change providers an improvement in > > performance for this device. > > > > [ 5] local 10.1.0.2 port 5201 connected to 10.1.0.1 port 38346 > > [ ID] Interval Transfer Bitrate Retr Cwnd > > [ 5] 0.00-1.00 sec 96.8 MBytes 812 Mbits/sec 0 469 KBytes > > [ 5] 1.00-2.00 sec 94.3 MBytes 791 Mbits/sec 0 469 KBytes > > [ 5] 2.00-3.00 sec 96.1 MBytes 806 Mbits/sec 0 469 KBytes > > [ 5] 3.00-4.00 sec 98.3 MBytes 825 Mbits/sec 0 469 KBytes > > [ 5] 4.00-5.00 sec 98.4 MBytes 825 Mbits/sec 0 469 KBytes > > [ 5] 5.00-6.00 sec 98.4 MBytes 826 Mbits/sec 0 469 KBytes > > [ 5] 6.00-7.00 sec 98.9 MBytes 830 Mbits/sec 0 469 KBytes > > [ 5] 7.00-8.00 sec 91.7 MBytes 769 Mbits/sec 0 469 KBytes > > [ 5] 8.00-9.00 sec 99.4 MBytes 834 Mbits/sec 0 747 KBytes > > [ 5] 9.00-10.00 sec 101 MBytes 851 Mbits/sec 0 747 KBytes > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > I will mark this one as 'change-requested', when in fact it is more a > test-requested. Once we get a Tested-by: we can merge this next cycle. > I was able to do a first basic test on my setup. I'm using the MV88Q2110 and connecting it to a Göpel media converter that I use as a reference. However, with your patch applied, I can't get a link. When I set a fixed link speed of 1GBit/s and the media converter is configured as the master, I can normally do: ethtool -s end1 speed 1000 master-slave forced-slave After that, the link came up. However, with the changes made, I can't do this anymore. Can you reproduce this in your setup? What is your setup like? Are you connecting two MV88Q2110 physically to each other? I'm out of office again next week, afterwards I should be able to do some more testing again. I think being able to set fixed link speeds is a must for this PHY. Regards, Stefan
Hello, On 2024-09-14 16:00:01 +0200, Stefan Eichenberger wrote: > Hi Niklas and Andrew, > > I was able to do a first basic test on my setup. I'm using the MV88Q2110 > and connecting it to a Göpel media converter that I use as a reference. Thanks for testing this work. > However, with your patch applied, I can't get a link. When I set a fixed > link speed of 1GBit/s and the media converter is configured as the > master, I can normally do: > ethtool -s end1 speed 1000 master-slave forced-slave > After that, the link came up. However, with the changes made, I can't do > this anymore. Can you reproduce this in your setup? Without this patch I can't bring up a 1GBit/s link at all, I can only setup a 100 MBit/s link with, ethtool -s eth1 speed 100 master-slave forced-slave If I do the same with speed set to a 1000 I never get a link. That's why autoneg is a such a boon for me, as with that I do get a 1 Gbit/s link. As you have the MV88Q2110 datasheets, can you check the register writes in mv88q2110_init_seq0 and mv88q2110_init_seq1 for sanity? Maybe something is not quiet right there, I have only been able to reveres engineer support for autoneg so it's quiet likely. > What is your setup > like? Are you connecting two MV88Q2110 physically to each other? Yes, I hair-pin two MV88Q2110 together. > out > of office again next week, afterwards I should be able to do some more > testing again. I think being able to set fixed link speeds is a must for > this PHY. I'm also at LPC next week but I will do some more testing on this and see if I can reproduce your finding with a 100 speed link.
> > out > > of office again next week, afterwards I should be able to do some more > > testing again. I think being able to set fixed link speeds is a must for > > this PHY. > > I'm also at LPC next week but I will do some more testing on this and > see if I can reproduce your finding with a 100 speed link. There is no real rush. netdev is not accepting patches at the moment, due to the merge window opening, and lots of developers travelling to LPC. So you have up to 8 weeks to sort this out. Andrew
> master, I can normally do: > ethtool -s end1 speed 1000 master-slave forced-slave You might want to try adding 'autoneg off' If the new default is to perform autoneg, speed 1000 tells it to try to autoneg only advertising 1000. If the link partner is not actually performing autoneg, it will never succeed. This is the regression i was talking about. For this device, due to its history, we might want to default to autoneg off. Andrew
diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c index 31f8c976e387..5107f58338af 100644 --- a/drivers/net/phy/marvell-88q2xxx.c +++ b/drivers/net/phy/marvell-88q2xxx.c @@ -101,6 +101,22 @@ struct mmd_val { u16 val; }; +static const struct mmd_val mv88q2110_init_seq0[] = { + { MDIO_MMD_PCS, 0xffe4, 0x07b5 }, + { MDIO_MMD_PCS, 0xffe4, 0x06b6 }, +}; + +static const struct mmd_val mv88q2110_init_seq1[] = { + { MDIO_MMD_PCS, 0xffde, 0x402f }, + { MDIO_MMD_PCS, 0xfe34, 0x4040 }, + { MDIO_MMD_PCS, 0xfe2a, 0x3c1d }, + { MDIO_MMD_PCS, 0xfe34, 0x0040 }, + { MDIO_MMD_AN, 0x8032, 0x0064 }, + { MDIO_MMD_AN, 0x8031, 0x0a01 }, + { MDIO_MMD_AN, 0x8031, 0x0c01 }, + { MDIO_MMD_PCS, 0xffdb, 0x0010 }, +}; + static const struct mmd_val mv88q222x_revb0_init_seq0[] = { { MDIO_MMD_PCS, 0x8033, 0x6801 }, { MDIO_MMD_AN, MDIO_AN_T1_CTRL, 0x0 }, @@ -424,15 +440,6 @@ static int mv88q2xxx_get_features(struct phy_device *phydev) if (ret) return ret; - /* The PHY signalizes it supports autonegotiation. Unfortunately, so - * far it was not possible to get a link even when following the init - * sequence provided by Marvell. Disable it for now until a proper - * workaround is found or a new PHY revision is released. - */ - if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2110) - linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, - phydev->supported); - return 0; } @@ -739,6 +746,25 @@ static int mv88q2xxx_probe(struct phy_device *phydev) return mv88q2xxx_hwmon_probe(phydev); } +static int mv88q2110_config_init(struct phy_device *phydev) +{ + int ret; + + ret = mv88q2xxx_write_mmd_vals(phydev, mv88q2110_init_seq0, + ARRAY_SIZE(mv88q2110_init_seq0)); + if (ret < 0) + return ret; + + usleep_range(5000, 10000); + + ret = mv88q2xxx_write_mmd_vals(phydev, mv88q2110_init_seq1, + ARRAY_SIZE(mv88q2110_init_seq1)); + if (ret < 0) + return ret; + + return mv88q2xxx_config_init(phydev); +} + static int mv88q222x_revb0_config_init(struct phy_device *phydev) { int ret; @@ -880,7 +906,7 @@ static struct phy_driver mv88q2xxx_driver[] = { .name = "mv88q2110", .get_features = mv88q2xxx_get_features, .config_aneg = mv88q2xxx_config_aneg, - .config_init = mv88q2xxx_config_init, + .config_init = mv88q2110_config_init, .read_status = mv88q2xxx_read_status, .soft_reset = mv88q2xxx_soft_reset, .set_loopback = genphy_c45_loopback,
The initial marvell-88q2xxx driver only supported the Marvell 88Q2110 PHY without auto negotiation support. The reason documented states that the provided initialization sequence did not to work. Now a method to enable auto negotiation have been found by comparing the initialization of other supported devices and an out-of-tree PHY driver. Perform the minimal needed initialization of the PHY to get auto negotiation working and remove the limitation that disables the auto negotiation feature for the mv88q2110 device. With this change a 1000Mbps full duplex link is able to be negotiated between two mv88q2110 and the link works perfectly. The other side also reflects the manually configure settings of the master device. # ethtool eth0 Settings for eth0: Supported ports: [ ] Supported link modes: 100baseT1/Full 1000baseT1/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 100baseT1/Full 1000baseT1/Full Advertised pause frame use: No Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: 100baseT1/Full 1000baseT1/Full Link partner advertised pause frame use: No Link partner advertised auto-negotiation: Yes Link partner advertised FEC modes: Not reported Speed: 1000Mb/s Duplex: Full Auto-negotiation: on master-slave cfg: preferred master master-slave status: slave Port: Twisted Pair PHYAD: 0 Transceiver: external MDI-X: Unknown Link detected: yes SQI: 15/15 Before this change I was not able to manually configure 1000Mbps link, only a 100Mpps link so this change providers an improvement in performance for this device. [ 5] local 10.1.0.2 port 5201 connected to 10.1.0.1 port 38346 [ ID] Interval Transfer Bitrate Retr Cwnd [ 5] 0.00-1.00 sec 96.8 MBytes 812 Mbits/sec 0 469 KBytes [ 5] 1.00-2.00 sec 94.3 MBytes 791 Mbits/sec 0 469 KBytes [ 5] 2.00-3.00 sec 96.1 MBytes 806 Mbits/sec 0 469 KBytes [ 5] 3.00-4.00 sec 98.3 MBytes 825 Mbits/sec 0 469 KBytes [ 5] 4.00-5.00 sec 98.4 MBytes 825 Mbits/sec 0 469 KBytes [ 5] 5.00-6.00 sec 98.4 MBytes 826 Mbits/sec 0 469 KBytes [ 5] 6.00-7.00 sec 98.9 MBytes 830 Mbits/sec 0 469 KBytes [ 5] 7.00-8.00 sec 91.7 MBytes 769 Mbits/sec 0 469 KBytes [ 5] 8.00-9.00 sec 99.4 MBytes 834 Mbits/sec 0 747 KBytes [ 5] 9.00-10.00 sec 101 MBytes 851 Mbits/sec 0 747 KBytes Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/net/phy/marvell-88q2xxx.c | 46 ++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 10 deletions(-)