diff mbox series

[net-next,3/3] net: phy: marvell-88q2xxx: Enable auto negotiation for mv88q2110

Message ID 20240906133951.3433788-4-niklas.soderlund+renesas@ragnatech.se (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series net: phy: marvell-88q2xxx: Enable auto negotiation for mv88q2110 | expand

Commit Message

Niklas Söderlund Sept. 6, 2024, 1:39 p.m. UTC
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(-)

Comments

Andrew Lunn Sept. 6, 2024, 8:36 p.m. UTC | #1
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
Niklas Söderlund Sept. 6, 2024, 9:39 p.m. UTC | #2
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
Andrew Lunn Sept. 10, 2024, 4:32 p.m. UTC | #3
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
Stefan Eichenberger Sept. 10, 2024, 6:02 p.m. UTC | #4
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
Andrew Lunn Sept. 10, 2024, 8:18 p.m. UTC | #5
> 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
Andrew Lunn Sept. 10, 2024, 8:23 p.m. UTC | #6
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
Stefan Eichenberger Sept. 14, 2024, 2 p.m. UTC | #7
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
Niklas Söderlund Sept. 14, 2024, 2:21 p.m. UTC | #8
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.
Andrew Lunn Sept. 14, 2024, 2:43 p.m. UTC | #9
> > 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
Andrew Lunn Sept. 14, 2024, 2:50 p.m. UTC | #10
> 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
Stefan Eichenberger Sept. 25, 2024, 11:56 a.m. UTC | #11
Hi Andrew and Niklas,

On Sat, Sep 14, 2024 at 04:50:02PM +0200, Andrew Lunn wrote:
> > 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


You were right about the autoneg off. I wasn't able to turn it off in my
first trial because when autoneg is not working I also have to set the
duplex mode. So the following four things worked:
ethtool -s end1 autoneg off duplex full speed 1000 master-slave forced-slave
ethtool -s end1 autoneg off duplex full speed 1000 master-slave forced-master
ethtool -s end1 autoneg off duplex full speed 100 master-slave forced-slave
ethtool -s end1 autoneg off duplex full speed 100 master-slave forced-master

So for our use case everything is working fine now and we can live with
the fact that autoneg is the default.

Tested with a mv88q2110 device.

Tested-by: Stefan Eichenberger <eichest@gmail.com>

Thanks,
Stefan
Stefan Eichenberger Sept. 25, 2024, 1:04 p.m. UTC | #12
Hi Niklas,

On Sat, Sep 14, 2024 at 04:21:36PM +0200, Niklas Söderlund wrote:
> 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.

Unfortunately this registers are not documented in the datasheet.
However, from the software initialization guide the following values
would be correct for A1 and A2 devices (A0 does not need one write):
static const struct mmd_val mv88q2110_init_seq1[] = {
	{ MDIO_MMD_PCS, 0xffde, 0x402f },
	{ MDIO_MMD_PCS, 0xfe2a, 0x3c3d},
	{ MDIO_MMD_PCS, 0xfe34, 0x4040 },
	{ MDIO_MMD_PCS, 0xfe4b, 0x9337},
	{ 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, 0xFE0F, 0x0000 },
	{ MDIO_MMD_PCS, 0x800C, 0x0000 },
	{ MDIO_MMD_PCS, 0x801D, 0x0800 },
	{ MDIO_MMD_PCS, 0xfc00, 0x01c0 },
	{ MDIO_MMD_PCS, 0xfc17, 0x0425},
	{ MDIO_MMD_PCS, 0xfc94, 0x5470},
	{ MDIO_MMD_PCS, 0xfc95, 0x0055},
	{ MDIO_MMD_PCS, 0xfc19, 0x08d8},
	{ MDIO_MMD_PCS, 0xfc1a, 0x0110},
	{ MDIO_MMD_PCS, 0xfc1b, 0x0a10},
	{ MDIO_MMD_PCS, 0xfc3a, 0x2725},
	{ MDIO_MMD_PCS, 0xfc61, 0x2627},
	{ MDIO_MMD_PCS, 0xfc3b, 0x1612},
	{ MDIO_MMD_PCS, 0xfc62, 0x1c12},
	{ MDIO_MMD_PCS, 0xfc9d, 0x6367},
	{ MDIO_MMD_PCS, 0xfc9e, 0x8060},
	{ MDIO_MMD_PCS, 0xfc00, 0x01c8},
	{ MDIO_MMD_PCS, 0x8000, 0x0000},
	{ MDIO_MMD_PCS, 0x8016, 0x0011},
	{ MDIO_MMD_PCS, 0xfda3, 0x1800}, /* According to datahsheet not for Rev A0 */
	{ MDIO_MMD_PCS, 0xfe02, 0x00c0},
	{ MDIO_MMD_PCS, 0xffdb, 0x0010},
	{ MDIO_MMD_PCS, 0xfff3, 0x0020},
	{ MDIO_MMD_PCS, 0xfe40, 0x00a6},
	{ MDIO_MMD_PCS, 0xfe60, 0x0000},
	{ MDIO_MMD_PCS, 0xfe04, 0x0008},
	{ MDIO_MMD_PCS, 0xfe2a, 0x3c3d},
	{ MDIO_MMD_PCS, 0xfe4b, 0x9334},
	{ MDIO_MMD_PCS, 0xfc10, 0xf600},
	{ MDIO_MMD_PCS, 0xfc11, 0x073d},
	{ MDIO_MMD_PCS, 0xfc12, 0x000d},
	{ MDIO_MMD_PCS, 0xfc13, 0x0010},
};

On my side, your values and the ones above are working.

By the way, do you know why you only get between 800 and 850 Mbps? On my
setup I see up to 930 Mbps in bidir mode. Just asking because maybe this
is the reason why fixed speed doesn't work in your setup (would be weird
though)?

[ ID][Role] Interval           Transfer     Bitrate         Retr
[  5][TX-C]   0.00-10.01  sec  1.09 GBytes   935 Mbits/sec    0             sender
[  5][TX-C]   0.00-10.01  sec  1.09 GBytes   932 Mbits/sec                  receiver
[  7][RX-C]   0.00-10.01  sec  1.09 GBytes   933 Mbits/sec  154             sender
[  7][RX-C]   0.00-10.01  sec  1.08 GBytes   931 Mbits/sec                  receiver

Regards,
Stefan
Niklas Söderlund Oct. 5, 2024, 11:08 a.m. UTC | #13
Hello Stefan,

On 2024-09-25 15:04:19 +0200, Stefan Eichenberger wrote:
> Hi Niklas,
> 
> On Sat, Sep 14, 2024 at 04:21:36PM +0200, Niklas Söderlund wrote:
> > 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.
> 
> Unfortunately this registers are not documented in the datasheet.
> However, from the software initialization guide the following values
> would be correct for A1 and A2 devices (A0 does not need one write):
> static const struct mmd_val mv88q2110_init_seq1[] = {
> 	{ MDIO_MMD_PCS, 0xffde, 0x402f },
> 	{ MDIO_MMD_PCS, 0xfe2a, 0x3c3d},
> 	{ MDIO_MMD_PCS, 0xfe34, 0x4040 },
> 	{ MDIO_MMD_PCS, 0xfe4b, 0x9337},
> 	{ 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, 0xFE0F, 0x0000 },
> 	{ MDIO_MMD_PCS, 0x800C, 0x0000 },
> 	{ MDIO_MMD_PCS, 0x801D, 0x0800 },
> 	{ MDIO_MMD_PCS, 0xfc00, 0x01c0 },
> 	{ MDIO_MMD_PCS, 0xfc17, 0x0425},
> 	{ MDIO_MMD_PCS, 0xfc94, 0x5470},
> 	{ MDIO_MMD_PCS, 0xfc95, 0x0055},
> 	{ MDIO_MMD_PCS, 0xfc19, 0x08d8},
> 	{ MDIO_MMD_PCS, 0xfc1a, 0x0110},
> 	{ MDIO_MMD_PCS, 0xfc1b, 0x0a10},
> 	{ MDIO_MMD_PCS, 0xfc3a, 0x2725},
> 	{ MDIO_MMD_PCS, 0xfc61, 0x2627},
> 	{ MDIO_MMD_PCS, 0xfc3b, 0x1612},
> 	{ MDIO_MMD_PCS, 0xfc62, 0x1c12},
> 	{ MDIO_MMD_PCS, 0xfc9d, 0x6367},
> 	{ MDIO_MMD_PCS, 0xfc9e, 0x8060},
> 	{ MDIO_MMD_PCS, 0xfc00, 0x01c8},
> 	{ MDIO_MMD_PCS, 0x8000, 0x0000},
> 	{ MDIO_MMD_PCS, 0x8016, 0x0011},
> 	{ MDIO_MMD_PCS, 0xfda3, 0x1800}, /* According to datahsheet not for Rev A0 */
> 	{ MDIO_MMD_PCS, 0xfe02, 0x00c0},
> 	{ MDIO_MMD_PCS, 0xffdb, 0x0010},
> 	{ MDIO_MMD_PCS, 0xfff3, 0x0020},
> 	{ MDIO_MMD_PCS, 0xfe40, 0x00a6},
> 	{ MDIO_MMD_PCS, 0xfe60, 0x0000},
> 	{ MDIO_MMD_PCS, 0xfe04, 0x0008},
> 	{ MDIO_MMD_PCS, 0xfe2a, 0x3c3d},
> 	{ MDIO_MMD_PCS, 0xfe4b, 0x9334},
> 	{ MDIO_MMD_PCS, 0xfc10, 0xf600},
> 	{ MDIO_MMD_PCS, 0xfc11, 0x073d},
> 	{ MDIO_MMD_PCS, 0xfc12, 0x000d},
> 	{ MDIO_MMD_PCS, 0xfc13, 0x0010},
> };
> 
> On my side, your values and the ones above are working.

Thanks for checking.

> 
> By the way, do you know why you only get between 800 and 850 Mbps? On my
> setup I see up to 930 Mbps in bidir mode. Just asking because maybe this
> is the reason why fixed speed doesn't work in your setup (would be weird
> though)?
> 
> [ ID][Role] Interval           Transfer     Bitrate         Retr
> [  5][TX-C]   0.00-10.01  sec  1.09 GBytes   935 Mbits/sec    0             sender
> [  5][TX-C]   0.00-10.01  sec  1.09 GBytes   932 Mbits/sec                  receiver
> [  7][RX-C]   0.00-10.01  sec  1.09 GBytes   933 Mbits/sec  154             sender
> [  7][RX-C]   0.00-10.01  sec  1.08 GBytes   931 Mbits/sec                  receiver

I suspect it's due to me hair pining mv88q2110 together on the same SoC.  
Unfortunately that is the only test setup I have for this device.
diff mbox series

Patch

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,