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

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

[Niklas Söderlund]

Hello,

This series enables auto negotiation for the mv88q2110 device.  
Previously this feature have been disabled for mv88q2110, while enabled 
for other devices supported by this driver.

The initial driver implementation states this is due to the 
configuration sequence provided by the vendor did not work. By comparing 
the initialization sequence of other devices this driver supports and 
the out-of-tree PHY driver for mv88q2110 found in the Renesas BSP [1] I 
was able to figure out a working configuration.

As I have no access to the datasheets of either of these devices it 
would be super if someone who has could sanity check the initialization 
sequence.

With this series I'm able to auto negotiate both 1000Mbps and 100Mbps 
links without issue.

    # 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

And the performance is good too. Without this change I was not able to 
manually configure a 1000Mbps link, only 100Mbps ones. So this gives a 
huge performance boost for my use-case.

    [  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

Patch 1/3 and 2/3 are preparation patches that align and move functions 
around as the mv88q2110 code paths can now reuses much of what is done 
for mv88q2220. While patch 3/3 adds the new initialization sequence and 
removes the auto negotiation limit for mv88q2110.

1.  https://github.com/renesas-rcar/linux-bsp/commit/2a1f07d0e722a18188cfe62842b61f2fbc0ba812

Niklas Söderlund (3):
  net: phy: marvell-88q2xxx: Align soft reset for mv88q2110 and
    mv88q2220
  net: phy: marvell-88q2xxx: Make register writer function generic
  net: phy: marvell-88q2xxx: Enable auto negotiation for mv88q2110

 drivers/net/phy/marvell-88q2xxx.c | 124 +++++++++++++++++-------------
 1 file changed, 71 insertions(+), 53 deletions(-)

-- 
2.46.0

[net-next 1/3] net: phy: marvell-88q2xxx: Align soft reset for mv88q2110 and mv88q2220

[Niklas Söderlund]

The soft reset implementations for mv88q2110 and mv88q2220 differ as the
later need to consider that auto negation is supported on mv88q2220
devices. In preparation of enabling auto negotiation on mv88q2110 merge
the two rest functions into a device generic one.

The mv88q2220 behavior is kept as is but extended to wait for the reset
bit to be clears before continuing, as was done previously on mv88q2220.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/phy/marvell-88q2xxx.c | 60 ++++++++++++++-----------------
 1 file changed, 26 insertions(+), 34 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index c812f16eaa3a..850beb4b1722 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -179,15 +179,34 @@ static int mv88q2xxx_soft_reset(struct phy_device *phydev)
 	int ret;
 	int val;
 
-	ret = phy_write_mmd(phydev, MDIO_MMD_PCS,
-			    MDIO_PCS_1000BT1_CTRL, MDIO_PCS_1000BT1_CTRL_RESET);
+	/* Enable RESET of DCL */
+	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000) {
+		ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_CTRL,
+			    MDIO_PCS_1000BT1_CTRL_RESET);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PCS,
+					MDIO_PCS_1000BT1_CTRL, val,
+					!(val & MDIO_PCS_1000BT1_CTRL_RESET),
+					50000, 600000, true);
 	if (ret < 0)
 		return ret;
 
-	return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PCS,
-					 MDIO_PCS_1000BT1_CTRL, val,
-					 !(val & MDIO_PCS_1000BT1_CTRL_RESET),
-					 50000, 600000, true);
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc);
+	if (ret < 0)
+		return ret;
+
+	/* Disable RESET of DCL */
+	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000)
+		return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58);
+
+	return 0;
 }
 
 static int mv88q2xxx_read_link_gbit(struct phy_device *phydev)
@@ -705,33 +724,6 @@ static int mv88q2xxx_probe(struct phy_device *phydev)
 	return mv88q2xxx_hwmon_probe(phydev);
 }
 
-static int mv88q222x_soft_reset(struct phy_device *phydev)
-{
-	int ret;
-
-	/* Enable RESET of DCL */
-	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000) {
-		ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48);
-		if (ret < 0)
-			return ret;
-	}
-
-	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_CTRL,
-			    MDIO_PCS_1000BT1_CTRL_RESET);
-	if (ret < 0)
-		return ret;
-
-	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc);
-	if (ret < 0)
-		return ret;
-
-	/* Disable RESET of DCL */
-	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000)
-		return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58);
-
-	return 0;
-}
-
 static int mv88q222x_write_mmd_vals(struct phy_device *phydev,
 				    const struct mmd_val *vals, size_t len)
 {
@@ -906,7 +898,7 @@ static struct phy_driver mv88q2xxx_driver[] = {
 		.aneg_done		= genphy_c45_aneg_done,
 		.config_init		= mv88q222x_config_init,
 		.read_status		= mv88q2xxx_read_status,
-		.soft_reset		= mv88q222x_soft_reset,
+		.soft_reset		= mv88q2xxx_soft_reset,
 		.config_intr		= mv88q2xxx_config_intr,
 		.handle_interrupt	= mv88q2xxx_handle_interrupt,
 		.set_loopback		= genphy_c45_loopback,
-- 
2.46.0

[net-next 2/3] net: phy: marvell-88q2xxx: Make register writer function generic

[Niklas Söderlund]

In preparation to adding auto negotiation support to mv88q2110 move and
rename the helper function used to write an array of register values to
the PHY.

Just as for mv88q2220 devices this helper will be needed to for the
initial configuration of the mv88q2110 to support auto negotiation.

The function is moved verbatim, there is no change in behavior.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/phy/marvell-88q2xxx.c | 40 +++++++++++++++----------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 850beb4b1722..31f8c976e387 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -174,6 +174,21 @@ static const struct mmd_val mv88q222x_revb1_revb2_init_seq1[] = {
 	{ MDIO_MMD_PCS, 0xfe11, 0x1105 },
 };
 
+static int mv88q2xxx_write_mmd_vals(struct phy_device *phydev,
+				    const struct mmd_val *vals, size_t len)
+{
+	int ret;
+
+	for (; len; vals++, len--) {
+		ret = phy_write_mmd(phydev, vals->devad, vals->regnum,
+				    vals->val);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int mv88q2xxx_soft_reset(struct phy_device *phydev)
 {
 	int ret;
@@ -724,33 +739,18 @@ static int mv88q2xxx_probe(struct phy_device *phydev)
 	return mv88q2xxx_hwmon_probe(phydev);
 }
 
-static int mv88q222x_write_mmd_vals(struct phy_device *phydev,
-				    const struct mmd_val *vals, size_t len)
-{
-	int ret;
-
-	for (; len; vals++, len--) {
-		ret = phy_write_mmd(phydev, vals->devad, vals->regnum,
-				    vals->val);
-		if (ret < 0)
-			return ret;
-	}
-
-	return 0;
-}
-
 static int mv88q222x_revb0_config_init(struct phy_device *phydev)
 {
 	int ret;
 
-	ret = mv88q222x_write_mmd_vals(phydev, mv88q222x_revb0_init_seq0,
+	ret = mv88q2xxx_write_mmd_vals(phydev, mv88q222x_revb0_init_seq0,
 				       ARRAY_SIZE(mv88q222x_revb0_init_seq0));
 	if (ret < 0)
 		return ret;
 
 	usleep_range(5000, 10000);
 
-	ret = mv88q222x_write_mmd_vals(phydev, mv88q222x_revb0_init_seq1,
+	ret = mv88q2xxx_write_mmd_vals(phydev, mv88q222x_revb0_init_seq1,
 				       ARRAY_SIZE(mv88q222x_revb0_init_seq1));
 	if (ret < 0)
 		return ret;
@@ -764,17 +764,17 @@ static int mv88q222x_revb1_revb2_config_init(struct phy_device *phydev)
 	int ret;
 
 	if (is_rev_b1)
-		ret = mv88q222x_write_mmd_vals(phydev, mv88q222x_revb1_init_seq0,
+		ret = mv88q2xxx_write_mmd_vals(phydev, mv88q222x_revb1_init_seq0,
 					       ARRAY_SIZE(mv88q222x_revb1_init_seq0));
 	else
-		ret = mv88q222x_write_mmd_vals(phydev, mv88q222x_revb2_init_seq0,
+		ret = mv88q2xxx_write_mmd_vals(phydev, mv88q222x_revb2_init_seq0,
 					       ARRAY_SIZE(mv88q222x_revb2_init_seq0));
 	if (ret < 0)
 		return ret;
 
 	usleep_range(3000, 5000);
 
-	ret = mv88q222x_write_mmd_vals(phydev, mv88q222x_revb1_revb2_init_seq1,
+	ret = mv88q2xxx_write_mmd_vals(phydev, mv88q222x_revb1_revb2_init_seq1,
 				       ARRAY_SIZE(mv88q222x_revb1_revb2_init_seq1));
 	if (ret < 0)
 		return ret;
-- 
2.46.0

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

[Niklas Söderlund]

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

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

Re: [net-next 2/3] net: phy: marvell-88q2xxx: Make register writer function generic

[Andrew Lunn]

On Fri, Sep 06, 2024 at 03:39:50PM +0200, Niklas Söderlund wrote:
> In preparation to adding auto negotiation support to mv88q2110 move and
> rename the helper function used to write an array of register values to
> the PHY.
> 
> Just as for mv88q2220 devices this helper will be needed to for the
> initial configuration of the mv88q2110 to support auto negotiation.
> 
> The function is moved verbatim, there is no change in behavior.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Andrew Lunn]

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

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

[Niklas Söderlund]

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

-- 
Kind Regards,
Niklas Söderlund

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

[Andrew Lunn]

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

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

[Stefan Eichenberger]

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

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

[Andrew Lunn]

> 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

Re: [net-next 1/3] net: phy: marvell-88q2xxx: Align soft reset for mv88q2110 and mv88q2220

[Andrew Lunn]

On Fri, Sep 06, 2024 at 03:39:49PM +0200, Niklas Söderlund wrote:
> The soft reset implementations for mv88q2110 and mv88q2220 differ as the
> later need to consider that auto negation is supported on mv88q2220
> devices. In preparation of enabling auto negotiation on mv88q2110 merge
> the two rest functions into a device generic one.
> 
> The mv88q2220 behavior is kept as is but extended to wait for the reset
> bit to be clears before continuing, as was done previously on mv88q2220.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [net-next 2/3] net: phy: marvell-88q2xxx: Make register writer function generic

[Andrew Lunn]

On Fri, Sep 06, 2024 at 03:39:50PM +0200, Niklas Söderlund wrote:
> In preparation to adding auto negotiation support to mv88q2110 move and
> rename the helper function used to write an array of register values to
> the PHY.
> 
> Just as for mv88q2220 devices this helper will be needed to for the
> initial configuration of the mv88q2110 to support auto negotiation.
> 
> The function is moved verbatim, there is no change in behavior.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Andrew Lunn]

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

Re: [net-next 1/3] net: phy: marvell-88q2xxx: Align soft reset for mv88q2110 and mv88q2220

[Dimitri Fedrau]

Am Fri, Sep 06, 2024 at 03:39:49PM +0200 schrieb Niklas Söderlund:
> The soft reset implementations for mv88q2110 and mv88q2220 differ as the
> later need to consider that auto negation is supported on mv88q2220
> devices. In preparation of enabling auto negotiation on mv88q2110 merge
> the two rest functions into a device generic one.
> 
> The mv88q2220 behavior is kept as is but extended to wait for the reset
> bit to be clears before continuing, as was done previously on mv88q2220.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/net/phy/marvell-88q2xxx.c | 60 ++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index c812f16eaa3a..850beb4b1722 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -179,15 +179,34 @@ static int mv88q2xxx_soft_reset(struct phy_device *phydev)
>  	int ret;
>  	int val;
>  
> -	ret = phy_write_mmd(phydev, MDIO_MMD_PCS,
> -			    MDIO_PCS_1000BT1_CTRL, MDIO_PCS_1000BT1_CTRL_RESET);
> +	/* Enable RESET of DCL */
> +	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000) {
> +		ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_CTRL,
> +			    MDIO_PCS_1000BT1_CTRL_RESET);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PCS,
> +					MDIO_PCS_1000BT1_CTRL, val,
> +					!(val & MDIO_PCS_1000BT1_CTRL_RESET),
> +					50000, 600000, true);
>  	if (ret < 0)
>  		return ret;
>  
> -	return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PCS,
> -					 MDIO_PCS_1000BT1_CTRL, val,
> -					 !(val & MDIO_PCS_1000BT1_CTRL_RESET),
> -					 50000, 600000, true);
> +	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Disable RESET of DCL */
> +	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000)
> +		return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58);
> +
> +	return 0;
>  }
>  
>  static int mv88q2xxx_read_link_gbit(struct phy_device *phydev)
> @@ -705,33 +724,6 @@ static int mv88q2xxx_probe(struct phy_device *phydev)
>  	return mv88q2xxx_hwmon_probe(phydev);
>  }
>  
> -static int mv88q222x_soft_reset(struct phy_device *phydev)
> -{
> -	int ret;
> -
> -	/* Enable RESET of DCL */
> -	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000) {
> -		ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_CTRL,
> -			    MDIO_PCS_1000BT1_CTRL_RESET);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc);
> -	if (ret < 0)
> -		return ret;
> -
> -	/* Disable RESET of DCL */
> -	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000)
> -		return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58);
> -
> -	return 0;
> -}
> -
>  static int mv88q222x_write_mmd_vals(struct phy_device *phydev,
>  				    const struct mmd_val *vals, size_t len)
>  {
> @@ -906,7 +898,7 @@ static struct phy_driver mv88q2xxx_driver[] = {
>  		.aneg_done		= genphy_c45_aneg_done,
>  		.config_init		= mv88q222x_config_init,
>  		.read_status		= mv88q2xxx_read_status,
> -		.soft_reset		= mv88q222x_soft_reset,
> +		.soft_reset		= mv88q2xxx_soft_reset,
>  		.config_intr		= mv88q2xxx_config_intr,
>  		.handle_interrupt	= mv88q2xxx_handle_interrupt,
>  		.set_loopback		= genphy_c45_loopback,
> -- 
> 2.46.0
>

Hi Niklas,

tested with a mv88q2220 device, worked as expected.

Tested-by: Dimitri Fedrau <dima.fedrau@gmail.com>

Best regards,
Dimitri Fedrau

Re: [net-next 2/3] net: phy: marvell-88q2xxx: Make register writer function generic

[Dimitri Fedrau]

Am Fri, Sep 06, 2024 at 03:39:50PM +0200 schrieb Niklas Söderlund:
> In preparation to adding auto negotiation support to mv88q2110 move and
> rename the helper function used to write an array of register values to
> the PHY.
> 
> Just as for mv88q2220 devices this helper will be needed to for the
> initial configuration of the mv88q2110 to support auto negotiation.
> 
> The function is moved verbatim, there is no change in behavior.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/net/phy/marvell-88q2xxx.c | 40 +++++++++++++++----------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index 850beb4b1722..31f8c976e387 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -174,6 +174,21 @@ static const struct mmd_val mv88q222x_revb1_revb2_init_seq1[] = {
>  	{ MDIO_MMD_PCS, 0xfe11, 0x1105 },
>  };
>  
> +static int mv88q2xxx_write_mmd_vals(struct phy_device *phydev,
> +				    const struct mmd_val *vals, size_t len)
> +{
> +	int ret;
> +
> +	for (; len; vals++, len--) {
> +		ret = phy_write_mmd(phydev, vals->devad, vals->regnum,
> +				    vals->val);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int mv88q2xxx_soft_reset(struct phy_device *phydev)
>  {
>  	int ret;
> @@ -724,33 +739,18 @@ static int mv88q2xxx_probe(struct phy_device *phydev)
>  	return mv88q2xxx_hwmon_probe(phydev);
>  }
>  
> -static int mv88q222x_write_mmd_vals(struct phy_device *phydev,
> -				    const struct mmd_val *vals, size_t len)
> -{
> -	int ret;
> -
> -	for (; len; vals++, len--) {
> -		ret = phy_write_mmd(phydev, vals->devad, vals->regnum,
> -				    vals->val);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static int mv88q222x_revb0_config_init(struct phy_device *phydev)
>  {
>  	int ret;
>  
> -	ret = mv88q222x_write_mmd_vals(phydev, mv88q222x_revb0_init_seq0,
> +	ret = mv88q2xxx_write_mmd_vals(phydev, mv88q222x_revb0_init_seq0,
>  				       ARRAY_SIZE(mv88q222x_revb0_init_seq0));
>  	if (ret < 0)
>  		return ret;
>  
>  	usleep_range(5000, 10000);
>  
> -	ret = mv88q222x_write_mmd_vals(phydev, mv88q222x_revb0_init_seq1,
> +	ret = mv88q2xxx_write_mmd_vals(phydev, mv88q222x_revb0_init_seq1,
>  				       ARRAY_SIZE(mv88q222x_revb0_init_seq1));
>  	if (ret < 0)
>  		return ret;
> @@ -764,17 +764,17 @@ static int mv88q222x_revb1_revb2_config_init(struct phy_device *phydev)
>  	int ret;
>  
>  	if (is_rev_b1)
> -		ret = mv88q222x_write_mmd_vals(phydev, mv88q222x_revb1_init_seq0,
> +		ret = mv88q2xxx_write_mmd_vals(phydev, mv88q222x_revb1_init_seq0,
>  					       ARRAY_SIZE(mv88q222x_revb1_init_seq0));
>  	else
> -		ret = mv88q222x_write_mmd_vals(phydev, mv88q222x_revb2_init_seq0,
> +		ret = mv88q2xxx_write_mmd_vals(phydev, mv88q222x_revb2_init_seq0,
>  					       ARRAY_SIZE(mv88q222x_revb2_init_seq0));
>  	if (ret < 0)
>  		return ret;
>  
>  	usleep_range(3000, 5000);
>  
> -	ret = mv88q222x_write_mmd_vals(phydev, mv88q222x_revb1_revb2_init_seq1,
> +	ret = mv88q2xxx_write_mmd_vals(phydev, mv88q222x_revb1_revb2_init_seq1,
>  				       ARRAY_SIZE(mv88q222x_revb1_revb2_init_seq1));
>  	if (ret < 0)
>  		return ret;
> -- 
> 2.46.0
>

Hi Niklas,

tested with a mv88q2220 device, worked as expected.

Tested-by: Dimitri Fedrau <dima.fedrau@gmail.com>

Best regards,
Dimitri Fedrau

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

[Stefan Eichenberger]

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

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

[Niklas Söderlund]

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.

-- 
Kind Regards,
Niklas Söderlund

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

[Andrew Lunn]

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

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

[Andrew Lunn]

> 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

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

[Stefan Eichenberger]

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

Re: [net-next 1/3] net: phy: marvell-88q2xxx: Align soft reset for mv88q2110 and mv88q2220

[Stefan Eichenberger]

On Fri, Sep 06, 2024 at 03:39:49PM +0200, Niklas Söderlund wrote:
> The soft reset implementations for mv88q2110 and mv88q2220 differ as the
> later need to consider that auto negation is supported on mv88q2220
> devices. In preparation of enabling auto negotiation on mv88q2110 merge
> the two rest functions into a device generic one.
> 
> The mv88q2220 behavior is kept as is but extended to wait for the reset
> bit to be clears before continuing, as was done previously on mv88q2220.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/net/phy/marvell-88q2xxx.c | 60 ++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index c812f16eaa3a..850beb4b1722 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -179,15 +179,34 @@ static int mv88q2xxx_soft_reset(struct phy_device *phydev)
>  	int ret;
>  	int val;
>  
> -	ret = phy_write_mmd(phydev, MDIO_MMD_PCS,
> -			    MDIO_PCS_1000BT1_CTRL, MDIO_PCS_1000BT1_CTRL_RESET);
> +	/* Enable RESET of DCL */
> +	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000) {
> +		ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_CTRL,
> +			    MDIO_PCS_1000BT1_CTRL_RESET);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PCS,
> +					MDIO_PCS_1000BT1_CTRL, val,
> +					!(val & MDIO_PCS_1000BT1_CTRL_RESET),
> +					50000, 600000, true);
>  	if (ret < 0)
>  		return ret;
>  
> -	return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PCS,
> -					 MDIO_PCS_1000BT1_CTRL, val,
> -					 !(val & MDIO_PCS_1000BT1_CTRL_RESET),
> -					 50000, 600000, true);
> +	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Disable RESET of DCL */
> +	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000)
> +		return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58);
> +
> +	return 0;
>  }
>  
>  static int mv88q2xxx_read_link_gbit(struct phy_device *phydev)
> @@ -705,33 +724,6 @@ static int mv88q2xxx_probe(struct phy_device *phydev)
>  	return mv88q2xxx_hwmon_probe(phydev);
>  }
>  
> -static int mv88q222x_soft_reset(struct phy_device *phydev)
> -{
> -	int ret;
> -
> -	/* Enable RESET of DCL */
> -	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000) {
> -		ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_CTRL,
> -			    MDIO_PCS_1000BT1_CTRL_RESET);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc);
> -	if (ret < 0)
> -		return ret;
> -
> -	/* Disable RESET of DCL */
> -	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000)
> -		return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58);
> -
> -	return 0;
> -}
> -
>  static int mv88q222x_write_mmd_vals(struct phy_device *phydev,
>  				    const struct mmd_val *vals, size_t len)
>  {
> @@ -906,7 +898,7 @@ static struct phy_driver mv88q2xxx_driver[] = {
>  		.aneg_done		= genphy_c45_aneg_done,
>  		.config_init		= mv88q222x_config_init,
>  		.read_status		= mv88q2xxx_read_status,
> -		.soft_reset		= mv88q222x_soft_reset,
> +		.soft_reset		= mv88q2xxx_soft_reset,
>  		.config_intr		= mv88q2xxx_config_intr,
>  		.handle_interrupt	= mv88q2xxx_handle_interrupt,
>  		.set_loopback		= genphy_c45_loopback,
> -- 
> 2.46.0
> 

Tested with a mv88q2110 device.

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

Regards,
Stefan

Re: [net-next 2/3] net: phy: marvell-88q2xxx: Make register writer function generic

[Stefan Eichenberger]

On Fri, Sep 06, 2024 at 03:39:50PM +0200, Niklas Söderlund wrote:
> In preparation to adding auto negotiation support to mv88q2110 move and
> rename the helper function used to write an array of register values to
> the PHY.
> 
> Just as for mv88q2220 devices this helper will be needed to for the
> initial configuration of the mv88q2110 to support auto negotiation.
> 
> The function is moved verbatim, there is no change in behavior.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Tested with a mv88q2110 device.

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

Regards,
Stefan

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

[Stefan Eichenberger]

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

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

[Niklas Söderlund]

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.

-- 
Kind Regards,
Niklas Söderlund