[RFC PATCH 0/2] mxl-gpy: add option to match SGMII speed to the TPI speed

[RFC PATCH 0/2] mxl-gpy: add option to match SGMII speed to the TPI speed

[Stefan Eichenberger]

The mxl-gpy phy supports a mode in which it sets the SGMII speed to that
negotiated over the twisted pair interface (tpi). This is useful if the
Ethernet controller is not capable of autonegotiation over SGMII, or
does not support the mode implemented by the mxl-gpy driver. This patch
adds a new property to the device tree bindings to enable this mode.

I marked this series as an RFC because I'm not sure if adding a new
property is the way to go or if we should use the in-band-status of the
phylink instead. Unfortunately, it is not possible to access the phylink
structure in the phy context. This would mean that I would have to add a
new function to the phylink driver that would allow us to access the
cur_link_an_mode property, or am I missing something?

Stefan Eichenberger (2):
  dt-bindings: net: phy: gpy2xx: add sgmii-match-tpi-speed property
  net: phy: mxl-gpy: add new device tree property to disable SGMII
    autoneg

 .../bindings/net/maxlinear,gpy2xx.yaml         |  6 ++++++
 drivers/net/phy/mxl-gpy.c                      | 18 +++++++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

-- 
2.40.1

[RFC PATCH 1/2] dt-bindings: net: phy: gpy2xx: add sgmii-match-tpi-speed property

[Stefan Eichenberger]

Add a new sgmii-match-tpi-speed property to the gpy2xx binding to allow
the phy to match the SGMII link speed to that negotiated on the TPI
interface.

Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
---
 Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml b/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml
index 8a3713abd1ca..9deae36bd837 100644
--- a/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml
+++ b/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml
@@ -27,6 +27,12 @@ properties:
       Affected PHYs (as far as known) are GPY215B and GPY215C.
     type: boolean
 
+  maxlinear,sgmii-match-tpi-speed:
+    description: |
+      When this property is present, the SGMII speed is set to the twisted
+      pair interface (tpi) speed.
+    type: boolean
+
 dependencies:
   maxlinear,use-broken-interrupts: [ interrupts ]
 
-- 
2.40.1

[RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Stefan Eichenberger]

Add a new device tree property to disable SGMII autonegotiation and
instead use the option to match the SGMII speed to what was negotiated
on the twisted pair interface (tpi). This allows us to disable SGMII
autonegotiation on Ethernet controllers that are not compatible with
this mode.

Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
---
 drivers/net/phy/mxl-gpy.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index b2d36a3a96f1..4147b4c29eaf 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -114,6 +114,7 @@ struct gpy_priv {
 	 * is enabled.
 	 */
 	u64 lb_dis_to;
+	bool sgmii_match_tpi_speed;
 };
 
 static const struct {
@@ -262,8 +263,17 @@ static int gpy_mbox_read(struct phy_device *phydev, u32 addr)
 
 static int gpy_config_init(struct phy_device *phydev)
 {
+	struct gpy_priv *priv = phydev->priv;
 	int ret;
 
+	/* Disalbe SGMII Autoneg if we want to match SGMII to TPI speed */
+	if (priv->sgmii_match_tpi_speed) {
+		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL,
+				     VSPEC1_SGMII_CTRL_ANEN, 0);
+		if (ret < 0)
+			return ret;
+	}
+
 	/* Mask all interrupts */
 	ret = phy_write(phydev, PHY_IMASK, 0);
 	if (ret)
@@ -304,6 +314,9 @@ static int gpy_probe(struct phy_device *phydev)
 	if (!device_property_present(dev, "maxlinear,use-broken-interrupts"))
 		phydev->dev_flags |= PHY_F_NO_IRQ;
 
+	priv->sgmii_match_tpi_speed =
+		device_property_present(dev, "maxlinear,sgmii-match-tpi-speed");
+
 	fw_version = phy_read(phydev, PHY_FWV);
 	if (fw_version < 0)
 		return fw_version;
@@ -516,6 +529,7 @@ static int gpy_update_mdix(struct phy_device *phydev)
 
 static int gpy_update_interface(struct phy_device *phydev)
 {
+	struct gpy_priv *priv = phydev->priv;
 	int ret;
 
 	/* Interface mode is fixed for USXGMII and integrated PHY */
@@ -529,6 +543,8 @@ static int gpy_update_interface(struct phy_device *phydev)
 	switch (phydev->speed) {
 	case SPEED_2500:
 		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
+		if (!gpy_sgmii_aneg_en(phydev))
+			break;
 		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL,
 				     VSPEC1_SGMII_CTRL_ANEN, 0);
 		if (ret < 0) {
@@ -542,7 +558,7 @@ static int gpy_update_interface(struct phy_device *phydev)
 	case SPEED_100:
 	case SPEED_10:
 		phydev->interface = PHY_INTERFACE_MODE_SGMII;
-		if (gpy_sgmii_aneg_en(phydev))
+		if (gpy_sgmii_aneg_en(phydev) || priv->sgmii_match_tpi_speed)
 			break;
 		/* Enable and restart SGMII ANEG for 10/100/1000Mbps link speed
 		 * if ANEG is disabled (in 2500-BaseX mode).
-- 
2.40.1

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Andrew Lunn]

On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote:
> Add a new device tree property to disable SGMII autonegotiation and
> instead use the option to match the SGMII speed to what was negotiated
> on the twisted pair interface (tpi).

Could you explain this is more detail.

SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate
the symbols 100 times when running at 10Mbs, and 10 times when running
at 100Mbps.

    Andrew

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Stefan Eichenberger]

Hi Andrew,

Thanks a lot for the feedback.

On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote:
> On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote:
> > Add a new device tree property to disable SGMII autonegotiation and
> > instead use the option to match the SGMII speed to what was negotiated
> > on the twisted pair interface (tpi).
> 
> Could you explain this is more detail.
> 
> SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate
> the symbols 100 times when running at 10Mbs, and 10 times when running
> at 100Mbps.

Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps,
100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an
Octeon TX2 SoC, this means that we have to enable "in-band-status" on
the controller. This will work for all three speed settings. However, if
we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will
disable SGMII autonegotiation in gpy_update_interface. This is not
supported by this Ethernet controller because in-band-status is still
enabled. Therefore, we will not be able to transfer data at 2.5 Gbps,
the SGMII link will not go into a working state.

What this patch does is, if the maxlinear,sgmii-match-tpi-speed property
is set, it will always use the link speed negotiated on the twisted pair
interface and adjust the SGMII data rate accordingly. For the Octeon
controller, this means that we don't set the in-band-status mode because
we don't use the SGMII autnegotiation and all 4 speeds (10, 100, 1000,
2500 Mbps) are working.

Here the description from the datasheet (this patch forces the SGMII
speed to the TPI speed):
If bit 12 is set to a logic one, ANMODE field determines the Auto-
Negotiation protocol. If bit 12 is cleared to a logic zero, speed is set
to maximum in full duplex mode. Once the TPI link is up, the SGMII speed
is automatically forced to match the TPI speed. This bit has no effect
when SGMII_FIXED2G5 is ‘1’.

Best regards,
Stefan

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Russell King (Oracle)]

On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote:
> Hi Andrew,
> 
> Thanks a lot for the feedback.
> 
> On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote:
> > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote:
> > > Add a new device tree property to disable SGMII autonegotiation and
> > > instead use the option to match the SGMII speed to what was negotiated
> > > on the twisted pair interface (tpi).
> > 
> > Could you explain this is more detail.
> > 
> > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate
> > the symbols 100 times when running at 10Mbs, and 10 times when running
> > at 100Mbps.
> 
> Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps,
> 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an
> Octeon TX2 SoC, this means that we have to enable "in-band-status" on
> the controller. This will work for all three speed settings. However, if
> we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will
> disable SGMII autonegotiation in gpy_update_interface. This is not
> supported by this Ethernet controller because in-band-status is still
> enabled. Therefore, we will not be able to transfer data at 2.5 Gbps,
> the SGMII link will not go into a working state.

I have been working on a phylink/phylib patch set to address this. As
I've been busy with health-based appointments during last week and this
week, I haven't been able to spend enough time to get that to a point
that I'm happy to publish it yet.

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

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Russell King (Oracle)]

On Tue, Apr 16, 2024 at 04:48:34PM +0100, Russell King (Oracle) wrote:
> On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote:
> > Hi Andrew,
> > 
> > Thanks a lot for the feedback.
> > 
> > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote:
> > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote:
> > > > Add a new device tree property to disable SGMII autonegotiation and
> > > > instead use the option to match the SGMII speed to what was negotiated
> > > > on the twisted pair interface (tpi).
> > > 
> > > Could you explain this is more detail.
> > > 
> > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate
> > > the symbols 100 times when running at 10Mbs, and 10 times when running
> > > at 100Mbps.
> > 
> > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps,
> > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an
> > Octeon TX2 SoC, this means that we have to enable "in-band-status" on
> > the controller. This will work for all three speed settings. However, if
> > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will
> > disable SGMII autonegotiation in gpy_update_interface. This is not
> > supported by this Ethernet controller because in-band-status is still
> > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps,
> > the SGMII link will not go into a working state.
> 
> I have been working on a phylink/phylib patch set to address this. As
> I've been busy with health-based appointments during last week and this
> week, I haven't been able to spend enough time to get that to a point
> that I'm happy to publish it yet.

You can find the experimental patches at:

http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=0c2fb62db211312ad2f5695997694908b54e9a17

and the three parents to that patch.

It's buried in:

http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue

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

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Andrew Lunn]

On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote:
> Hi Andrew,
> 
> Thanks a lot for the feedback.
> 
> On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote:
> > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote:
> > > Add a new device tree property to disable SGMII autonegotiation and
> > > instead use the option to match the SGMII speed to what was negotiated
> > > on the twisted pair interface (tpi).
> > 
> > Could you explain this is more detail.
> > 
> > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate
> > the symbols 100 times when running at 10Mbs, and 10 times when running
> > at 100Mbps.
> 
> Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps,
> 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an
> Octeon TX2 SoC, this means that we have to enable "in-band-status" on
> the controller. This will work for all three speed settings. However, if
> we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will
> disable SGMII autonegotiation in gpy_update_interface. This is not
> supported by this Ethernet controller because in-band-status is still
> enabled. Therefore, we will not be able to transfer data at 2.5 Gbps,
> the SGMII link will not go into a working state.

This is where i expect Russel to point out that SGMII does not support
2.5G. What you actually mean is that the PHY swaps to 2500BaseX. And
2500BaseX does not perform speed negotiation, since it only supports
2500. So you also need the MAC to swap to 2500BaseX.

I don't think any DT property is required here. This is fundamental to
SGMII only be 10/100/1G, and when you go above that, you swap to
something else.

Lets see what Russell patches do.

    Andrew

---
pw-bot: cr

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Russell King (Oracle)]

On Tue, Apr 16, 2024 at 06:02:08PM +0200, Andrew Lunn wrote:
> On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote:
> > Hi Andrew,
> > 
> > Thanks a lot for the feedback.
> > 
> > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote:
> > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote:
> > > > Add a new device tree property to disable SGMII autonegotiation and
> > > > instead use the option to match the SGMII speed to what was negotiated
> > > > on the twisted pair interface (tpi).
> > > 
> > > Could you explain this is more detail.
> > > 
> > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate
> > > the symbols 100 times when running at 10Mbs, and 10 times when running
> > > at 100Mbps.
> > 
> > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps,
> > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an
> > Octeon TX2 SoC, this means that we have to enable "in-band-status" on
> > the controller. This will work for all three speed settings. However, if
> > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will
> > disable SGMII autonegotiation in gpy_update_interface. This is not
> > supported by this Ethernet controller because in-band-status is still
> > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps,
> > the SGMII link will not go into a working state.
> 
> This is where i expect Russel to point out that SGMII does not support
> 2.5G. What you actually mean is that the PHY swaps to 2500BaseX. And
> 2500BaseX does not perform speed negotiation, since it only supports
> 2500. So you also need the MAC to swap to 2500BaseX.

Yes, absolutely true that SGMII does not support 2.5G... and when
operating faster, than 2500base-X is normally used.

How, 2500base-X was slow to be standardised, and consequently different
manufacturers came up with different ideas. The common theme is that
it's 1000base-X up-clocked by 2.5x. Where the ideas differ is whether
in-band negotiation is supported or not. This has been a pain point for
a while now.

As I mentioned in my previous two messages, I have an experimental
patch series that helps to address this.

The issue is that implementations mix manufacturers, so we need to
know the capabilities of the PHY and the capabilities of the PCS, and
then hope that we can find some common ground between their
requirements.

There is then the issue that if you're not using phylink, then...
guess what... you either need to convert to use phylink or implement
the logic in your own MAC driver to detect what the PHY is doing
and what its capabilities are - but I think from what you've said,
you are using phylink.

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

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Stefan Eichenberger]

Hi Russell and Andrew,

On Tue, Apr 16, 2024 at 05:24:02PM +0100, Russell King (Oracle) wrote:
> On Tue, Apr 16, 2024 at 06:02:08PM +0200, Andrew Lunn wrote:
> > On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote:
> > > Hi Andrew,
> > > 
> > > Thanks a lot for the feedback.
> > > 
> > > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote:
> > > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote:
> > > > > Add a new device tree property to disable SGMII autonegotiation and
> > > > > instead use the option to match the SGMII speed to what was negotiated
> > > > > on the twisted pair interface (tpi).
> > > > 
> > > > Could you explain this is more detail.
> > > > 
> > > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate
> > > > the symbols 100 times when running at 10Mbs, and 10 times when running
> > > > at 100Mbps.
> > > 
> > > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps,
> > > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an
> > > Octeon TX2 SoC, this means that we have to enable "in-band-status" on
> > > the controller. This will work for all three speed settings. However, if
> > > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will
> > > disable SGMII autonegotiation in gpy_update_interface. This is not
> > > supported by this Ethernet controller because in-band-status is still
> > > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps,
> > > the SGMII link will not go into a working state.
> > 
> > This is where i expect Russel to point out that SGMII does not support
> > 2.5G. What you actually mean is that the PHY swaps to 2500BaseX. And
> > 2500BaseX does not perform speed negotiation, since it only supports
> > 2500. So you also need the MAC to swap to 2500BaseX.
> 
> Yes, absolutely true that SGMII does not support 2.5G... and when
> operating faster, than 2500base-X is normally used.
> 
> How, 2500base-X was slow to be standardised, and consequently different
> manufacturers came up with different ideas. The common theme is that
> it's 1000base-X up-clocked by 2.5x. Where the ideas differ is whether
> in-band negotiation is supported or not. This has been a pain point for
> a while now.
> 
> As I mentioned in my previous two messages, I have an experimental
> patch series that helps to address this.
> 
> The issue is that implementations mix manufacturers, so we need to
> know the capabilities of the PHY and the capabilities of the PCS, and
> then hope that we can find some common ground between their
> requirements.
> 
> There is then the issue that if you're not using phylink, then...
> guess what... you either need to convert to use phylink or implement
> the logic in your own MAC driver to detect what the PHY is doing
> and what its capabilities are - but I think from what you've said,
> you are using phylink.

Thanks for the patch series and the explanation. In our use case we have
the mismatch between the PHY and the mvpp2 driver in 2500BaseX mode. If
I understand the patches correctly, the PHY should just return in its
query_inband function that it does not support inband when 2500BaseX is
configured as it is done for the Marvell phy driver. I will try to test
this on my end and give feedback, unfortunately it will become next week
as I won't have access to my test setup until then.

@Russell: I hope it is nothing serious with your health and wish you a
fast recovery!

Best regards,
Stefan

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Russell King (Oracle)]

On Tue, Apr 16, 2024 at 07:23:03PM +0200, Stefan Eichenberger wrote:
> Hi Russell and Andrew,
> 
> On Tue, Apr 16, 2024 at 05:24:02PM +0100, Russell King (Oracle) wrote:
> > On Tue, Apr 16, 2024 at 06:02:08PM +0200, Andrew Lunn wrote:
> > > On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote:
> > > > Hi Andrew,
> > > > 
> > > > Thanks a lot for the feedback.
> > > > 
> > > > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote:
> > > > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote:
> > > > > > Add a new device tree property to disable SGMII autonegotiation and
> > > > > > instead use the option to match the SGMII speed to what was negotiated
> > > > > > on the twisted pair interface (tpi).
> > > > > 
> > > > > Could you explain this is more detail.
> > > > > 
> > > > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate
> > > > > the symbols 100 times when running at 10Mbs, and 10 times when running
> > > > > at 100Mbps.
> > > > 
> > > > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps,
> > > > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an
> > > > Octeon TX2 SoC, this means that we have to enable "in-band-status" on
> > > > the controller. This will work for all three speed settings. However, if
> > > > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will
> > > > disable SGMII autonegotiation in gpy_update_interface. This is not
> > > > supported by this Ethernet controller because in-band-status is still
> > > > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps,
> > > > the SGMII link will not go into a working state.
> > > 
> > > This is where i expect Russel to point out that SGMII does not support
> > > 2.5G. What you actually mean is that the PHY swaps to 2500BaseX. And
> > > 2500BaseX does not perform speed negotiation, since it only supports
> > > 2500. So you also need the MAC to swap to 2500BaseX.
> > 
> > Yes, absolutely true that SGMII does not support 2.5G... and when
> > operating faster, than 2500base-X is normally used.
> > 
> > How, 2500base-X was slow to be standardised, and consequently different
> > manufacturers came up with different ideas. The common theme is that
> > it's 1000base-X up-clocked by 2.5x. Where the ideas differ is whether
> > in-band negotiation is supported or not. This has been a pain point for
> > a while now.
> > 
> > As I mentioned in my previous two messages, I have an experimental
> > patch series that helps to address this.
> > 
> > The issue is that implementations mix manufacturers, so we need to
> > know the capabilities of the PHY and the capabilities of the PCS, and
> > then hope that we can find some common ground between their
> > requirements.
> > 
> > There is then the issue that if you're not using phylink, then...
> > guess what... you either need to convert to use phylink or implement
> > the logic in your own MAC driver to detect what the PHY is doing
> > and what its capabilities are - but I think from what you've said,
> > you are using phylink.
> 
> Thanks for the patch series and the explanation. In our use case we have
> the mismatch between the PHY and the mvpp2 driver in 2500BaseX mode.

Ah, mvpp2. This is one of those cases where I think you have a
disagreement between manufacturers over 2500base-X.

Marvell's documentation clearly states that when operating in 1000base-X
mode, AN _must_ be enabled. Since programming 2500base-X is programming
the mvpp2 hardware for 1000base-X and then configuring the COMPHY to
clock faster, AN must also be enabled when operating at 2500base-X.

It seems you've coupled it with the mxl-gpy PHY which doesn't apparently
support AN when in 2500base-X.

Welcome to the mess of 2500base-X, and sadly we finally have the
situation that I've feared for years: one end of a 2500base-X link
that's documented as requiring AN, and the other end not providing it.

Sigh. If only the IEEE 802.3 committee had been more pro-active and
standardised 2500base-X _before_ manufacturers went off and did their
own different things.

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

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Stefan Eichenberger]

On Tue, Apr 16, 2024 at 07:12:49PM +0100, Russell King (Oracle) wrote:
> On Tue, Apr 16, 2024 at 07:23:03PM +0200, Stefan Eichenberger wrote:
> > Hi Russell and Andrew,
> > 
> > On Tue, Apr 16, 2024 at 05:24:02PM +0100, Russell King (Oracle) wrote:
> > > On Tue, Apr 16, 2024 at 06:02:08PM +0200, Andrew Lunn wrote:
> > > > On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote:
> > > > > Hi Andrew,
> > > > > 
> > > > > Thanks a lot for the feedback.
> > > > > 
> > > > > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote:
> > > > > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote:
> > > > > > > Add a new device tree property to disable SGMII autonegotiation and
> > > > > > > instead use the option to match the SGMII speed to what was negotiated
> > > > > > > on the twisted pair interface (tpi).
> > > > > > 
> > > > > > Could you explain this is more detail.
> > > > > > 
> > > > > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate
> > > > > > the symbols 100 times when running at 10Mbs, and 10 times when running
> > > > > > at 100Mbps.
> > > > > 
> > > > > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps,
> > > > > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an
> > > > > Octeon TX2 SoC, this means that we have to enable "in-band-status" on
> > > > > the controller. This will work for all three speed settings. However, if
> > > > > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will
> > > > > disable SGMII autonegotiation in gpy_update_interface. This is not
> > > > > supported by this Ethernet controller because in-band-status is still
> > > > > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps,
> > > > > the SGMII link will not go into a working state.
> > > > 
> > > > This is where i expect Russel to point out that SGMII does not support
> > > > 2.5G. What you actually mean is that the PHY swaps to 2500BaseX. And
> > > > 2500BaseX does not perform speed negotiation, since it only supports
> > > > 2500. So you also need the MAC to swap to 2500BaseX.
> > > 
> > > Yes, absolutely true that SGMII does not support 2.5G... and when
> > > operating faster, than 2500base-X is normally used.
> > > 
> > > How, 2500base-X was slow to be standardised, and consequently different
> > > manufacturers came up with different ideas. The common theme is that
> > > it's 1000base-X up-clocked by 2.5x. Where the ideas differ is whether
> > > in-band negotiation is supported or not. This has been a pain point for
> > > a while now.
> > > 
> > > As I mentioned in my previous two messages, I have an experimental
> > > patch series that helps to address this.
> > > 
> > > The issue is that implementations mix manufacturers, so we need to
> > > know the capabilities of the PHY and the capabilities of the PCS, and
> > > then hope that we can find some common ground between their
> > > requirements.
> > > 
> > > There is then the issue that if you're not using phylink, then...
> > > guess what... you either need to convert to use phylink or implement
> > > the logic in your own MAC driver to detect what the PHY is doing
> > > and what its capabilities are - but I think from what you've said,
> > > you are using phylink.
> > 
> > Thanks for the patch series and the explanation. In our use case we have
> > the mismatch between the PHY and the mvpp2 driver in 2500BaseX mode.
> 
> Ah, mvpp2. This is one of those cases where I think you have a
> disagreement between manufacturers over 2500base-X.
> 
> Marvell's documentation clearly states that when operating in 1000base-X
> mode, AN _must_ be enabled. Since programming 2500base-X is programming
> the mvpp2 hardware for 1000base-X and then configuring the COMPHY to
> clock faster, AN must also be enabled when operating at 2500base-X.
> 
> It seems you've coupled it with the mxl-gpy PHY which doesn't apparently
> support AN when in 2500base-X.
> 
> Welcome to the mess of 2500base-X, and sadly we finally have the
> situation that I've feared for years: one end of a 2500base-X link
> that's documented as requiring AN, and the other end not providing it.
> 
> Sigh. If only the IEEE 802.3 committee had been more pro-active and
> standardised 2500base-X _before_ manufacturers went off and did their
> own different things.

I also checked the datasheet and you are right about the 1000base-X mode
and in-band AN. What worked for us so far was to use SGMII mode even for
2.5Gbps and disable in-band AN (which is possible for SGMII). I think
this works because as you wrote, the genphy just multiplies the clock by
2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we
might even be able to use in-band autonegoation for 10,100 and 1000Mbps
and then just disable it for 2.5Gbps. I need to test it, but I have hope
that this should work.

Regards,
Stefan

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Russell King (Oracle)]

On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote:
> On Tue, Apr 16, 2024 at 07:12:49PM +0100, Russell King (Oracle) wrote:
> > On Tue, Apr 16, 2024 at 07:23:03PM +0200, Stefan Eichenberger wrote:
> > > Hi Russell and Andrew,
> > > 
> > > On Tue, Apr 16, 2024 at 05:24:02PM +0100, Russell King (Oracle) wrote:
> > > > On Tue, Apr 16, 2024 at 06:02:08PM +0200, Andrew Lunn wrote:
> > > > > On Tue, Apr 16, 2024 at 05:43:16PM +0200, Stefan Eichenberger wrote:
> > > > > > Hi Andrew,
> > > > > > 
> > > > > > Thanks a lot for the feedback.
> > > > > > 
> > > > > > On Tue, Apr 16, 2024 at 03:46:19PM +0200, Andrew Lunn wrote:
> > > > > > > On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote:
> > > > > > > > Add a new device tree property to disable SGMII autonegotiation and
> > > > > > > > instead use the option to match the SGMII speed to what was negotiated
> > > > > > > > on the twisted pair interface (tpi).
> > > > > > > 
> > > > > > > Could you explain this is more detail.
> > > > > > > 
> > > > > > > SGMII always runs its clocks at 1000Mbps. The MAC needs to duplicate
> > > > > > > the symbols 100 times when running at 10Mbs, and 10 times when running
> > > > > > > at 100Mbps.
> > > > > > 
> > > > > > Currently, the mxl-gpy driver uses SGMII autonegotiation for 10 Mbps,
> > > > > > 100 Mbps, and 1000 Mbps. For our Ethernet controller, which is on an
> > > > > > Octeon TX2 SoC, this means that we have to enable "in-band-status" on
> > > > > > the controller. This will work for all three speed settings. However, if
> > > > > > we have a link partner that can do 2.5 Gbps, the mxl-gpy driver will
> > > > > > disable SGMII autonegotiation in gpy_update_interface. This is not
> > > > > > supported by this Ethernet controller because in-band-status is still
> > > > > > enabled. Therefore, we will not be able to transfer data at 2.5 Gbps,
> > > > > > the SGMII link will not go into a working state.
> > > > > 
> > > > > This is where i expect Russel to point out that SGMII does not support
> > > > > 2.5G. What you actually mean is that the PHY swaps to 2500BaseX. And
> > > > > 2500BaseX does not perform speed negotiation, since it only supports
> > > > > 2500. So you also need the MAC to swap to 2500BaseX.
> > > > 
> > > > Yes, absolutely true that SGMII does not support 2.5G... and when
> > > > operating faster, than 2500base-X is normally used.
> > > > 
> > > > How, 2500base-X was slow to be standardised, and consequently different
> > > > manufacturers came up with different ideas. The common theme is that
> > > > it's 1000base-X up-clocked by 2.5x. Where the ideas differ is whether
> > > > in-band negotiation is supported or not. This has been a pain point for
> > > > a while now.
> > > > 
> > > > As I mentioned in my previous two messages, I have an experimental
> > > > patch series that helps to address this.
> > > > 
> > > > The issue is that implementations mix manufacturers, so we need to
> > > > know the capabilities of the PHY and the capabilities of the PCS, and
> > > > then hope that we can find some common ground between their
> > > > requirements.
> > > > 
> > > > There is then the issue that if you're not using phylink, then...
> > > > guess what... you either need to convert to use phylink or implement
> > > > the logic in your own MAC driver to detect what the PHY is doing
> > > > and what its capabilities are - but I think from what you've said,
> > > > you are using phylink.
> > > 
> > > Thanks for the patch series and the explanation. In our use case we have
> > > the mismatch between the PHY and the mvpp2 driver in 2500BaseX mode.
> > 
> > Ah, mvpp2. This is one of those cases where I think you have a
> > disagreement between manufacturers over 2500base-X.
> > 
> > Marvell's documentation clearly states that when operating in 1000base-X
> > mode, AN _must_ be enabled. Since programming 2500base-X is programming
> > the mvpp2 hardware for 1000base-X and then configuring the COMPHY to
> > clock faster, AN must also be enabled when operating at 2500base-X.
> > 
> > It seems you've coupled it with the mxl-gpy PHY which doesn't apparently
> > support AN when in 2500base-X.
> > 
> > Welcome to the mess of 2500base-X, and sadly we finally have the
> > situation that I've feared for years: one end of a 2500base-X link
> > that's documented as requiring AN, and the other end not providing it.
> > 
> > Sigh. If only the IEEE 802.3 committee had been more pro-active and
> > standardised 2500base-X _before_ manufacturers went off and did their
> > own different things.
> 
> I also checked the datasheet and you are right about the 1000base-X mode
> and in-band AN. What worked for us so far was to use SGMII mode even for
> 2.5Gbps and disable in-band AN (which is possible for SGMII). I think
> this works because as you wrote, the genphy just multiplies the clock by
> 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we
> might even be able to use in-band autonegoation for 10,100 and 1000Mbps
> and then just disable it for 2.5Gbps. I need to test it, but I have hope
> that this should work.

There is another way we could address this. If the querying support
had a means to identify that the endpoint supports bypass mode, we
could then have phylink identify that, and arrange to program the
mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would
mean it wouldn't require the inband 16-bit word to be present.

I haven't fully thought it through yet - for example, I haven't
considered how we should indicate to the PCS that AN bypass mode
should be enabled or disabled via the pcs_config() method.

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

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Russell King (Oracle)]

On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote:
> On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote:
> > I also checked the datasheet and you are right about the 1000base-X mode
> > and in-band AN. What worked for us so far was to use SGMII mode even for
> > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think
> > this works because as you wrote, the genphy just multiplies the clock by
> > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we
> > might even be able to use in-band autonegoation for 10,100 and 1000Mbps
> > and then just disable it for 2.5Gbps. I need to test it, but I have hope
> > that this should work.
> 
> There is another way we could address this. If the querying support
> had a means to identify that the endpoint supports bypass mode, we
> could then have phylink identify that, and arrange to program the
> mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would
> mean it wouldn't require the inband 16-bit word to be present.
> 
> I haven't fully thought it through yet - for example, I haven't
> considered how we should indicate to the PCS that AN bypass mode
> should be enabled or disabled via the pcs_config() method.

Okay, I've been trying to put more effort into this, but it's been slow
progress (sorry).

My thoughts from a design point of view were that we could just switch
to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and
everything at the PCS layer should be able to cope, but this is not the
case, especially with mvneta/mvpp2.

The problem is that mvneta/mvpp2 (and probably more) expect that

1) MLO_AN_INBAND means that the PCS will be using inband, and that
   means the link up/down state won't be forced. This basically implies
   that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the
   PCS.

2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and
   that means that the link needs to be forced (since there's no way
   for the hardware to know whether the link should be up or down.)
   It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be
   used for the PCS.

So, attempting to put a resolution of the PHY and PCS abilities into
phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_*
mode alone just doesn't work. Yet... we need to do that in there when
considering whether inband can be enabled or not for non-PHY links.

Basically, it needs a re-think how to solve this...

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

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Stefan Eichenberger]

On Wed, Apr 24, 2024 at 03:58:00PM +0100, Russell King (Oracle) wrote:
> On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote:
> > On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote:
> > > I also checked the datasheet and you are right about the 1000base-X mode
> > > and in-band AN. What worked for us so far was to use SGMII mode even for
> > > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think
> > > this works because as you wrote, the genphy just multiplies the clock by
> > > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we
> > > might even be able to use in-band autonegoation for 10,100 and 1000Mbps
> > > and then just disable it for 2.5Gbps. I need to test it, but I have hope
> > > that this should work.
> > 
> > There is another way we could address this. If the querying support
> > had a means to identify that the endpoint supports bypass mode, we
> > could then have phylink identify that, and arrange to program the
> > mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would
> > mean it wouldn't require the inband 16-bit word to be present.
> > 
> > I haven't fully thought it through yet - for example, I haven't
> > considered how we should indicate to the PCS that AN bypass mode
> > should be enabled or disabled via the pcs_config() method.
> 
> Okay, I've been trying to put more effort into this, but it's been slow
> progress (sorry).
> 
> My thoughts from a design point of view were that we could just switch
> to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and
> everything at the PCS layer should be able to cope, but this is not the
> case, especially with mvneta/mvpp2.
> 
> The problem is that mvneta/mvpp2 (and probably more) expect that
> 
> 1) MLO_AN_INBAND means that the PCS will be using inband, and that
>    means the link up/down state won't be forced. This basically implies
>    that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the
>    PCS.
> 
> 2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and
>    that means that the link needs to be forced (since there's no way
>    for the hardware to know whether the link should be up or down.)
>    It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be
>    used for the PCS.
> 
> So, attempting to put a resolution of the PHY and PCS abilities into
> phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_*
> mode alone just doesn't work. Yet... we need to do that in there when
> considering whether inband can be enabled or not for non-PHY links.
> 
> Basically, it needs a re-think how to solve this...

Today I was playing around with my combination of mxl-gpy and mvpp2 and
I got it working again with your patches applied. However, I hacked the
phylink driver to only rely on what the phy and pcs support. I know this
is not a proper solution, but it allowed me to verify the other changes.
My idea was if the phy and pcs support inband then use it, otherwise use
outband and ignore the rest.

Here is how my minimal phylink_pcs_neg_mode test function looks like:

static unsigned int phylink_pcs_neg_mode(struct phylink *pl,
					 struct phylink_pcs *pcs,
					 unsigned int mode,
					 phy_interface_t interface,
					 const unsigned long *advertising)
{
	unsigned int phy_link_mode = 0;
	unsigned int pcs_link_mode;

	pcs_link_mode = phylink_pcs_query_inband(pcs, interface);
	if (pl->phydev)
		phy_link_mode = phy_query_inband(pl->phydev, interface);

	/* If the PCS or PHY can not provide inband, then use
	 * outband.
	 */
	if (!(pcs_link_mode & LINK_INBAND_VALID) ||
	    !(phy_link_mode & LINK_INBAND_VALID))
		return PHYLINK_PCS_NEG_OUTBAND;

	return PHYLINK_PCS_NEG_INBAND_ENABLED;
}

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Russell King (Oracle)]

On Wed, Apr 24, 2024 at 05:56:47PM +0200, Stefan Eichenberger wrote:
> On Wed, Apr 24, 2024 at 03:58:00PM +0100, Russell King (Oracle) wrote:
> > On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote:
> > > On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote:
> > > > I also checked the datasheet and you are right about the 1000base-X mode
> > > > and in-band AN. What worked for us so far was to use SGMII mode even for
> > > > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think
> > > > this works because as you wrote, the genphy just multiplies the clock by
> > > > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we
> > > > might even be able to use in-band autonegoation for 10,100 and 1000Mbps
> > > > and then just disable it for 2.5Gbps. I need to test it, but I have hope
> > > > that this should work.
> > > 
> > > There is another way we could address this. If the querying support
> > > had a means to identify that the endpoint supports bypass mode, we
> > > could then have phylink identify that, and arrange to program the
> > > mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would
> > > mean it wouldn't require the inband 16-bit word to be present.
> > > 
> > > I haven't fully thought it through yet - for example, I haven't
> > > considered how we should indicate to the PCS that AN bypass mode
> > > should be enabled or disabled via the pcs_config() method.
> > 
> > Okay, I've been trying to put more effort into this, but it's been slow
> > progress (sorry).
> > 
> > My thoughts from a design point of view were that we could just switch
> > to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and
> > everything at the PCS layer should be able to cope, but this is not the
> > case, especially with mvneta/mvpp2.
> > 
> > The problem is that mvneta/mvpp2 (and probably more) expect that
> > 
> > 1) MLO_AN_INBAND means that the PCS will be using inband, and that
> >    means the link up/down state won't be forced. This basically implies
> >    that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the
> >    PCS.
> > 
> > 2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and
> >    that means that the link needs to be forced (since there's no way
> >    for the hardware to know whether the link should be up or down.)
> >    It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be
> >    used for the PCS.
> > 
> > So, attempting to put a resolution of the PHY and PCS abilities into
> > phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_*
> > mode alone just doesn't work. Yet... we need to do that in there when
> > considering whether inband can be enabled or not for non-PHY links.
> > 
> > Basically, it needs a re-think how to solve this...
> 
> Today I was playing around with my combination of mxl-gpy and mvpp2 and
> I got it working again with your patches applied. However, I hacked the
> phylink driver to only rely on what the phy and pcs support. I know this
> is not a proper solution, but it allowed me to verify the other changes.
> My idea was if the phy and pcs support inband then use it, otherwise use
> outband and ignore the rest.
> 
> Here is how my minimal phylink_pcs_neg_mode test function looks like:
> 
> static unsigned int phylink_pcs_neg_mode(struct phylink *pl,
> 					 struct phylink_pcs *pcs,
> 					 unsigned int mode,
> 					 phy_interface_t interface,
> 					 const unsigned long *advertising)
> {
> 	unsigned int phy_link_mode = 0;
> 	unsigned int pcs_link_mode;
> 
> 	pcs_link_mode = phylink_pcs_query_inband(pcs, interface);
> 	if (pl->phydev)
> 		phy_link_mode = phy_query_inband(pl->phydev, interface);
> 
> 	/* If the PCS or PHY can not provide inband, then use
> 	 * outband.
> 	 */
> 	if (!(pcs_link_mode & LINK_INBAND_VALID) ||
> 	    !(phy_link_mode & LINK_INBAND_VALID))
> 		return PHYLINK_PCS_NEG_OUTBAND;
> 
> 	return PHYLINK_PCS_NEG_INBAND_ENABLED;
> }

Note that I've changed the flags that get reported to be disable (bit 0)/
enable (bit 1) rather than valid/possible/required because the former
makes the resolution easier.

The problem is that merely returning outband doesn't cause mvneta/mvpp2
to force the link up. So for example, here's a SFP module which doesn't
support any inband for 2500base-X nor SGMII:

mvneta f1034000.ethernet eno2: copper SFP: interfaces=[mac=4,9-12,19,22-23, sfp=
4,23,27]
mvneta f1034000.ethernet eno2: copper SFP: chosen 2500base-x interface
mvneta f1034000.ethernet eno2: PHY i2c:sfp:16 uses interfaces 4,23,27, validatin
g 4,23
mvneta f1034000.ethernet eno2:  interface 4 (sgmii) rate match none supports 2-3
,5-6,13
mvneta f1034000.ethernet eno2:  interface 23 (2500base-x) rate match none suppor
ts 6,13,47
mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq=
POLL)
mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,000
08000,0000206c advertising 00,00000000,00008000,0000206c
mvneta f1034000.ethernet eno2: copper SFP: PHY link in-band modes 0x1
mvneta f1034000.ethernet eno2: major config 2500base-x
mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01
mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none a
dv=00,00000000,00008000,0000206c pause=04
mvneta f1034000.ethernet eno2: phylink_sfp_module_start()
mvneta f1034000.ethernet eno2: phylink_sfp_link_up()
mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off
mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off
mvneta f1034000.ethernet eno2: major config sgmii
mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01
mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/sgmii/none adv=00,00000000,00008000,0000206c pause=00
mvneta f1034000.ethernet eno2: pcs link down
mvneta f1034000.ethernet eno2: pcs link down
mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active
mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us
mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off

This looks like the link is up, but it isn't - note "pcs link down".
If we look at the value of the GMAC AN status register:

Value at address 0xf1036c10: 0x0000600a

which indicates that the link is down, so no packets will pass.

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

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Stefan Eichenberger]

On Wed, Apr 24, 2024 at 07:13:35PM +0100, Russell King (Oracle) wrote:
> On Wed, Apr 24, 2024 at 05:56:47PM +0200, Stefan Eichenberger wrote:
> > On Wed, Apr 24, 2024 at 03:58:00PM +0100, Russell King (Oracle) wrote:
> > > On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote:
> > > > On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote:
> > > > > I also checked the datasheet and you are right about the 1000base-X mode
> > > > > and in-band AN. What worked for us so far was to use SGMII mode even for
> > > > > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think
> > > > > this works because as you wrote, the genphy just multiplies the clock by
> > > > > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we
> > > > > might even be able to use in-band autonegoation for 10,100 and 1000Mbps
> > > > > and then just disable it for 2.5Gbps. I need to test it, but I have hope
> > > > > that this should work.
> > > > 
> > > > There is another way we could address this. If the querying support
> > > > had a means to identify that the endpoint supports bypass mode, we
> > > > could then have phylink identify that, and arrange to program the
> > > > mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would
> > > > mean it wouldn't require the inband 16-bit word to be present.
> > > > 
> > > > I haven't fully thought it through yet - for example, I haven't
> > > > considered how we should indicate to the PCS that AN bypass mode
> > > > should be enabled or disabled via the pcs_config() method.
> > > 
> > > Okay, I've been trying to put more effort into this, but it's been slow
> > > progress (sorry).
> > > 
> > > My thoughts from a design point of view were that we could just switch
> > > to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and
> > > everything at the PCS layer should be able to cope, but this is not the
> > > case, especially with mvneta/mvpp2.
> > > 
> > > The problem is that mvneta/mvpp2 (and probably more) expect that
> > > 
> > > 1) MLO_AN_INBAND means that the PCS will be using inband, and that
> > >    means the link up/down state won't be forced. This basically implies
> > >    that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the
> > >    PCS.
> > > 
> > > 2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and
> > >    that means that the link needs to be forced (since there's no way
> > >    for the hardware to know whether the link should be up or down.)
> > >    It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be
> > >    used for the PCS.
> > > 
> > > So, attempting to put a resolution of the PHY and PCS abilities into
> > > phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_*
> > > mode alone just doesn't work. Yet... we need to do that in there when
> > > considering whether inband can be enabled or not for non-PHY links.
> > > 
> > > Basically, it needs a re-think how to solve this...
> > 
> > Today I was playing around with my combination of mxl-gpy and mvpp2 and
> > I got it working again with your patches applied. However, I hacked the
> > phylink driver to only rely on what the phy and pcs support. I know this
> > is not a proper solution, but it allowed me to verify the other changes.
> > My idea was if the phy and pcs support inband then use it, otherwise use
> > outband and ignore the rest.
> > 
> > Here is how my minimal phylink_pcs_neg_mode test function looks like:
> > 
> > static unsigned int phylink_pcs_neg_mode(struct phylink *pl,
> > 					 struct phylink_pcs *pcs,
> > 					 unsigned int mode,
> > 					 phy_interface_t interface,
> > 					 const unsigned long *advertising)
> > {
> > 	unsigned int phy_link_mode = 0;
> > 	unsigned int pcs_link_mode;
> > 
> > 	pcs_link_mode = phylink_pcs_query_inband(pcs, interface);
> > 	if (pl->phydev)
> > 		phy_link_mode = phy_query_inband(pl->phydev, interface);
> > 
> > 	/* If the PCS or PHY can not provide inband, then use
> > 	 * outband.
> > 	 */
> > 	if (!(pcs_link_mode & LINK_INBAND_VALID) ||
> > 	    !(phy_link_mode & LINK_INBAND_VALID))
> > 		return PHYLINK_PCS_NEG_OUTBAND;
> > 
> > 	return PHYLINK_PCS_NEG_INBAND_ENABLED;
> > }
> 
> Note that I've changed the flags that get reported to be disable (bit 0)/
> enable (bit 1) rather than valid/possible/required because the former
> makes the resolution easier.
> 
> The problem is that merely returning outband doesn't cause mvneta/mvpp2
> to force the link up. So for example, here's a SFP module which doesn't
> support any inband for 2500base-X nor SGMII:
> 
> mvneta f1034000.ethernet eno2: copper SFP: interfaces=[mac=4,9-12,19,22-23, sfp=
> 4,23,27]
> mvneta f1034000.ethernet eno2: copper SFP: chosen 2500base-x interface
> mvneta f1034000.ethernet eno2: PHY i2c:sfp:16 uses interfaces 4,23,27, validatin
> g 4,23
> mvneta f1034000.ethernet eno2:  interface 4 (sgmii) rate match none supports 2-3
> ,5-6,13
> mvneta f1034000.ethernet eno2:  interface 23 (2500base-x) rate match none suppor
> ts 6,13,47
> mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq=
> POLL)
> mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,000
> 08000,0000206c advertising 00,00000000,00008000,0000206c
> mvneta f1034000.ethernet eno2: copper SFP: PHY link in-band modes 0x1
> mvneta f1034000.ethernet eno2: major config 2500base-x
> mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01
> mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none a
> dv=00,00000000,00008000,0000206c pause=04
> mvneta f1034000.ethernet eno2: phylink_sfp_module_start()
> mvneta f1034000.ethernet eno2: phylink_sfp_link_up()
> mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off
> mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off
> mvneta f1034000.ethernet eno2: major config sgmii
> mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01
> mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/sgmii/none adv=00,00000000,00008000,0000206c pause=00
> mvneta f1034000.ethernet eno2: pcs link down
> mvneta f1034000.ethernet eno2: pcs link down
> mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active
> mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us
> mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off
> 
> This looks like the link is up, but it isn't - note "pcs link down".
> If we look at the value of the GMAC AN status register:
> 
> Value at address 0xf1036c10: 0x0000600a
> 
> which indicates that the link is down, so no packets will pass.

What I changed in mvpp2 is to allow turing off inband in 2500base-x. The
mvpp2 driver can handle this use case in pcs_config, it will turn off AN
and force the link up. I think it should also work for mvneta, at least
the code looks almost the same. I get the following for the Port
Auto-Negotiation Configuration Register:

For 1Gbit/s it switches to SGMII and enables inband AN:
Memory mapped at address 0xffffa0112000.
Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6

For 2.5Gbit/s it disables inband AN and forces the link to be up:
Memory mapped at address 0xffffaff88000.
Value at address 0xF2132E0C (0xffffaff88e0c): 0x9042

Then the status register shows also link up for 2.5Gbit/s:
Memory mapped at address 0xffffa5fe2000.
Value at address 0xF2132E10 (0xffffa5fe2e10): 0x683B

What might be confusing is that the port type, bit 1 in MAC Control
Register0, is set to SGMII for 2.5Gbit/s, because we can only turn off
autonegotiation for SGMII:
Memory mapped at address 0xffff8c26c000.
Value at address 0xF2132E00 (0xffff8c26ce00): 0x8BFD

My example patch still uses the old macros:

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 97e38f61ac65..15fadfd61313 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -6223,9 +6223,12 @@ static unsigned int mvpp2_gmac_pcs_query_inband(struct phylink_pcs *pcs,
 	 * When <PortType> = 1 (1000BASE-X) this field must be set to 1.
 	 * Therefore, inband is "required".
 	 */
-	if (phy_interface_mode_is_8023z(interface))
+	if (interface == PHY_INTERFACE_MODE_1000BASEX)
 		return LINK_INBAND_VALID | LINK_INBAND_REQUIRED;
 
+	if (interface == PHY_INTERFACE_MODE_2500BASEX)
+		return LINK_INBAND_VALID | LINK_INBAND_POSSIBLE;
+
 	/* SGMII and RGMII can be configured to use inband signalling of the
 	 * AN result. Indicate these as "possible".
 	 */

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Russell King (Oracle)]

On Thu, Apr 25, 2024 at 01:24:51PM +0200, Stefan Eichenberger wrote:
> On Wed, Apr 24, 2024 at 07:13:35PM +0100, Russell King (Oracle) wrote:
> > On Wed, Apr 24, 2024 at 05:56:47PM +0200, Stefan Eichenberger wrote:
> > > On Wed, Apr 24, 2024 at 03:58:00PM +0100, Russell King (Oracle) wrote:
> > > > On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote:
> > > > > On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote:
> > > > > > I also checked the datasheet and you are right about the 1000base-X mode
> > > > > > and in-band AN. What worked for us so far was to use SGMII mode even for
> > > > > > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think
> > > > > > this works because as you wrote, the genphy just multiplies the clock by
> > > > > > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we
> > > > > > might even be able to use in-band autonegoation for 10,100 and 1000Mbps
> > > > > > and then just disable it for 2.5Gbps. I need to test it, but I have hope
> > > > > > that this should work.
> > > > > 
> > > > > There is another way we could address this. If the querying support
> > > > > had a means to identify that the endpoint supports bypass mode, we
> > > > > could then have phylink identify that, and arrange to program the
> > > > > mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would
> > > > > mean it wouldn't require the inband 16-bit word to be present.
> > > > > 
> > > > > I haven't fully thought it through yet - for example, I haven't
> > > > > considered how we should indicate to the PCS that AN bypass mode
> > > > > should be enabled or disabled via the pcs_config() method.
> > > > 
> > > > Okay, I've been trying to put more effort into this, but it's been slow
> > > > progress (sorry).
> > > > 
> > > > My thoughts from a design point of view were that we could just switch
> > > > to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and
> > > > everything at the PCS layer should be able to cope, but this is not the
> > > > case, especially with mvneta/mvpp2.
> > > > 
> > > > The problem is that mvneta/mvpp2 (and probably more) expect that
> > > > 
> > > > 1) MLO_AN_INBAND means that the PCS will be using inband, and that
> > > >    means the link up/down state won't be forced. This basically implies
> > > >    that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the
> > > >    PCS.
> > > > 
> > > > 2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and
> > > >    that means that the link needs to be forced (since there's no way
> > > >    for the hardware to know whether the link should be up or down.)
> > > >    It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be
> > > >    used for the PCS.
> > > > 
> > > > So, attempting to put a resolution of the PHY and PCS abilities into
> > > > phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_*
> > > > mode alone just doesn't work. Yet... we need to do that in there when
> > > > considering whether inband can be enabled or not for non-PHY links.
> > > > 
> > > > Basically, it needs a re-think how to solve this...
> > > 
> > > Today I was playing around with my combination of mxl-gpy and mvpp2 and
> > > I got it working again with your patches applied. However, I hacked the
> > > phylink driver to only rely on what the phy and pcs support. I know this
> > > is not a proper solution, but it allowed me to verify the other changes.
> > > My idea was if the phy and pcs support inband then use it, otherwise use
> > > outband and ignore the rest.
> > > 
> > > Here is how my minimal phylink_pcs_neg_mode test function looks like:
> > > 
> > > static unsigned int phylink_pcs_neg_mode(struct phylink *pl,
> > > 					 struct phylink_pcs *pcs,
> > > 					 unsigned int mode,
> > > 					 phy_interface_t interface,
> > > 					 const unsigned long *advertising)
> > > {
> > > 	unsigned int phy_link_mode = 0;
> > > 	unsigned int pcs_link_mode;
> > > 
> > > 	pcs_link_mode = phylink_pcs_query_inband(pcs, interface);
> > > 	if (pl->phydev)
> > > 		phy_link_mode = phy_query_inband(pl->phydev, interface);
> > > 
> > > 	/* If the PCS or PHY can not provide inband, then use
> > > 	 * outband.
> > > 	 */
> > > 	if (!(pcs_link_mode & LINK_INBAND_VALID) ||
> > > 	    !(phy_link_mode & LINK_INBAND_VALID))
> > > 		return PHYLINK_PCS_NEG_OUTBAND;
> > > 
> > > 	return PHYLINK_PCS_NEG_INBAND_ENABLED;
> > > }
> > 
> > Note that I've changed the flags that get reported to be disable (bit 0)/
> > enable (bit 1) rather than valid/possible/required because the former
> > makes the resolution easier.
> > 
> > The problem is that merely returning outband doesn't cause mvneta/mvpp2
> > to force the link up. So for example, here's a SFP module which doesn't
> > support any inband for 2500base-X nor SGMII:
> > 
> > mvneta f1034000.ethernet eno2: copper SFP: interfaces=[mac=4,9-12,19,22-23, sfp=
> > 4,23,27]
> > mvneta f1034000.ethernet eno2: copper SFP: chosen 2500base-x interface
> > mvneta f1034000.ethernet eno2: PHY i2c:sfp:16 uses interfaces 4,23,27, validatin
> > g 4,23
> > mvneta f1034000.ethernet eno2:  interface 4 (sgmii) rate match none supports 2-3
> > ,5-6,13
> > mvneta f1034000.ethernet eno2:  interface 23 (2500base-x) rate match none suppor
> > ts 6,13,47
> > mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq=
> > POLL)
> > mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,000
> > 08000,0000206c advertising 00,00000000,00008000,0000206c
> > mvneta f1034000.ethernet eno2: copper SFP: PHY link in-band modes 0x1
> > mvneta f1034000.ethernet eno2: major config 2500base-x
> > mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01
> > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none a
> > dv=00,00000000,00008000,0000206c pause=04
> > mvneta f1034000.ethernet eno2: phylink_sfp_module_start()
> > mvneta f1034000.ethernet eno2: phylink_sfp_link_up()
> > mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off
> > mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off
> > mvneta f1034000.ethernet eno2: major config sgmii
> > mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01
> > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/sgmii/none adv=00,00000000,00008000,0000206c pause=00
> > mvneta f1034000.ethernet eno2: pcs link down
> > mvneta f1034000.ethernet eno2: pcs link down
> > mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active
> > mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us
> > mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off
> > 
> > This looks like the link is up, but it isn't - note "pcs link down".
> > If we look at the value of the GMAC AN status register:
> > 
> > Value at address 0xf1036c10: 0x0000600a
> > 
> > which indicates that the link is down, so no packets will pass.
> 
> What I changed in mvpp2 is to allow turing off inband in 2500base-x. The
> mvpp2 driver can handle this use case in pcs_config, it will turn off AN
> and force the link up.

pcs_config can't force the link up.

> I think it should also work for mvneta, at least
> the code looks almost the same. I get the following for the Port
> Auto-Negotiation Configuration Register:
> 
> For 1Gbit/s it switches to SGMII and enables inband AN:
> Memory mapped at address 0xffffa0112000.
> Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6

So here the link is forced up which is wrong for inband, because then
we have no way to detect the link going down.

> For 2.5Gbit/s it disables inband AN and forces the link to be up:
> Memory mapped at address 0xffffaff88000.
> Value at address 0xF2132E0C (0xffffaff88e0c): 0x9042
> 
> Then the status register shows also link up for 2.5Gbit/s:
> Memory mapped at address 0xffffa5fe2000.
> Value at address 0xF2132E10 (0xffffa5fe2e10): 0x683B
> 
> What might be confusing is that the port type, bit 1 in MAC Control
> Register0, is set to SGMII for 2.5Gbit/s, because we can only turn off
> autonegotiation for SGMII:
> Memory mapped at address 0xffff8c26c000.
> Value at address 0xF2132E00 (0xffff8c26ce00): 0x8BFD

Control register 0 bit 1 is the port type bit, which controls whether
the port is in "1000base-X" mode or SGMII mode. This has no effect on
the interpretation of the inband control word.

Control register 2 bit 0 controls whether the port uses 802.3z
format control words or SGMII format control words.

> My example patch still uses the old macros:
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 97e38f61ac65..15fadfd61313 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -6223,9 +6223,12 @@ static unsigned int mvpp2_gmac_pcs_query_inband(struct phylink_pcs *pcs,
>  	 * When <PortType> = 1 (1000BASE-X) this field must be set to 1.
>  	 * Therefore, inband is "required".
>  	 */
> -	if (phy_interface_mode_is_8023z(interface))
> +	if (interface == PHY_INTERFACE_MODE_1000BASEX)
>  		return LINK_INBAND_VALID | LINK_INBAND_REQUIRED;
>  
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		return LINK_INBAND_VALID | LINK_INBAND_POSSIBLE;

This is not correct though. If we set PortType = 1, then this applies:

  Bit   Field        Type / InitVal   Description

  2     InBandAnEn   RW               In-band Auto-Negotiation enable.
...
                                      When <PortType> = 1 (1000BASE-X)
                                      this field must be set to 1.
                                                                                                                      When <PortType> = 0 (SGMII) and this
                                      field is 1, in-band Flow Control not
				      supported.

So, if we have the port in 1000base-X mode (PortType = 1) then bit 2
of the Autoneg configuration register needs to be set to be compliant
with the documentation. Therefore, since we set PortType = 1 for
2500BASE-X (and note that I _do_ run 2500BASE-X with inband AN enabled
over fibre transceivers here, so this is needed), we also need to
enable InBandAnEn.

This is exactly where the difficulty comes - because what you're
suggesting will break currently working setups such as what I have
here.

Switching the port to SGMII mode for 2500base-X doesn't sound like a
sensible thing to do.

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

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Stefan Eichenberger]

On Thu, Apr 25, 2024 at 03:30:52PM +0100, Russell King (Oracle) wrote:
> On Thu, Apr 25, 2024 at 01:24:51PM +0200, Stefan Eichenberger wrote:
> > On Wed, Apr 24, 2024 at 07:13:35PM +0100, Russell King (Oracle) wrote:
> > > On Wed, Apr 24, 2024 at 05:56:47PM +0200, Stefan Eichenberger wrote:
> > > > On Wed, Apr 24, 2024 at 03:58:00PM +0100, Russell King (Oracle) wrote:
> > > > > On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote:
> > > > > > On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote:
> > > > > > > I also checked the datasheet and you are right about the 1000base-X mode
> > > > > > > and in-band AN. What worked for us so far was to use SGMII mode even for
> > > > > > > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think
> > > > > > > this works because as you wrote, the genphy just multiplies the clock by
> > > > > > > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we
> > > > > > > might even be able to use in-band autonegoation for 10,100 and 1000Mbps
> > > > > > > and then just disable it for 2.5Gbps. I need to test it, but I have hope
> > > > > > > that this should work.
> > > > > > 
> > > > > > There is another way we could address this. If the querying support
> > > > > > had a means to identify that the endpoint supports bypass mode, we
> > > > > > could then have phylink identify that, and arrange to program the
> > > > > > mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would
> > > > > > mean it wouldn't require the inband 16-bit word to be present.
> > > > > > 
> > > > > > I haven't fully thought it through yet - for example, I haven't
> > > > > > considered how we should indicate to the PCS that AN bypass mode
> > > > > > should be enabled or disabled via the pcs_config() method.
> > > > > 
> > > > > Okay, I've been trying to put more effort into this, but it's been slow
> > > > > progress (sorry).
> > > > > 
> > > > > My thoughts from a design point of view were that we could just switch
> > > > > to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and
> > > > > everything at the PCS layer should be able to cope, but this is not the
> > > > > case, especially with mvneta/mvpp2.
> > > > > 
> > > > > The problem is that mvneta/mvpp2 (and probably more) expect that
> > > > > 
> > > > > 1) MLO_AN_INBAND means that the PCS will be using inband, and that
> > > > >    means the link up/down state won't be forced. This basically implies
> > > > >    that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the
> > > > >    PCS.
> > > > > 
> > > > > 2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and
> > > > >    that means that the link needs to be forced (since there's no way
> > > > >    for the hardware to know whether the link should be up or down.)
> > > > >    It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be
> > > > >    used for the PCS.
> > > > > 
> > > > > So, attempting to put a resolution of the PHY and PCS abilities into
> > > > > phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_*
> > > > > mode alone just doesn't work. Yet... we need to do that in there when
> > > > > considering whether inband can be enabled or not for non-PHY links.
> > > > > 
> > > > > Basically, it needs a re-think how to solve this...
> > > > 
> > > > Today I was playing around with my combination of mxl-gpy and mvpp2 and
> > > > I got it working again with your patches applied. However, I hacked the
> > > > phylink driver to only rely on what the phy and pcs support. I know this
> > > > is not a proper solution, but it allowed me to verify the other changes.
> > > > My idea was if the phy and pcs support inband then use it, otherwise use
> > > > outband and ignore the rest.
> > > > 
> > > > Here is how my minimal phylink_pcs_neg_mode test function looks like:
> > > > 
> > > > static unsigned int phylink_pcs_neg_mode(struct phylink *pl,
> > > > 					 struct phylink_pcs *pcs,
> > > > 					 unsigned int mode,
> > > > 					 phy_interface_t interface,
> > > > 					 const unsigned long *advertising)
> > > > {
> > > > 	unsigned int phy_link_mode = 0;
> > > > 	unsigned int pcs_link_mode;
> > > > 
> > > > 	pcs_link_mode = phylink_pcs_query_inband(pcs, interface);
> > > > 	if (pl->phydev)
> > > > 		phy_link_mode = phy_query_inband(pl->phydev, interface);
> > > > 
> > > > 	/* If the PCS or PHY can not provide inband, then use
> > > > 	 * outband.
> > > > 	 */
> > > > 	if (!(pcs_link_mode & LINK_INBAND_VALID) ||
> > > > 	    !(phy_link_mode & LINK_INBAND_VALID))
> > > > 		return PHYLINK_PCS_NEG_OUTBAND;
> > > > 
> > > > 	return PHYLINK_PCS_NEG_INBAND_ENABLED;
> > > > }
> > > 
> > > Note that I've changed the flags that get reported to be disable (bit 0)/
> > > enable (bit 1) rather than valid/possible/required because the former
> > > makes the resolution easier.
> > > 
> > > The problem is that merely returning outband doesn't cause mvneta/mvpp2
> > > to force the link up. So for example, here's a SFP module which doesn't
> > > support any inband for 2500base-X nor SGMII:
> > > 
> > > mvneta f1034000.ethernet eno2: copper SFP: interfaces=[mac=4,9-12,19,22-23, sfp=
> > > 4,23,27]
> > > mvneta f1034000.ethernet eno2: copper SFP: chosen 2500base-x interface
> > > mvneta f1034000.ethernet eno2: PHY i2c:sfp:16 uses interfaces 4,23,27, validatin
> > > g 4,23
> > > mvneta f1034000.ethernet eno2:  interface 4 (sgmii) rate match none supports 2-3
> > > ,5-6,13
> > > mvneta f1034000.ethernet eno2:  interface 23 (2500base-x) rate match none suppor
> > > ts 6,13,47
> > > mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq=
> > > POLL)
> > > mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,000
> > > 08000,0000206c advertising 00,00000000,00008000,0000206c
> > > mvneta f1034000.ethernet eno2: copper SFP: PHY link in-band modes 0x1
> > > mvneta f1034000.ethernet eno2: major config 2500base-x
> > > mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01
> > > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none a
> > > dv=00,00000000,00008000,0000206c pause=04
> > > mvneta f1034000.ethernet eno2: phylink_sfp_module_start()
> > > mvneta f1034000.ethernet eno2: phylink_sfp_link_up()
> > > mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off
> > > mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off
> > > mvneta f1034000.ethernet eno2: major config sgmii
> > > mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01
> > > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/sgmii/none adv=00,00000000,00008000,0000206c pause=00
> > > mvneta f1034000.ethernet eno2: pcs link down
> > > mvneta f1034000.ethernet eno2: pcs link down
> > > mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active
> > > mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us
> > > mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off
> > > 
> > > This looks like the link is up, but it isn't - note "pcs link down".
> > > If we look at the value of the GMAC AN status register:
> > > 
> > > Value at address 0xf1036c10: 0x0000600a
> > > 
> > > which indicates that the link is down, so no packets will pass.
> > 
> > What I changed in mvpp2 is to allow turing off inband in 2500base-x. The
> > mvpp2 driver can handle this use case in pcs_config, it will turn off AN
> > and force the link up.
> 
> pcs_config can't force the link up.
> 
> > I think it should also work for mvneta, at least
> > the code looks almost the same. I get the following for the Port
> > Auto-Negotiation Configuration Register:
> > 
> > For 1Gbit/s it switches to SGMII and enables inband AN:
> > Memory mapped at address 0xffffa0112000.
> > Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6
> 
> So here the link is forced up which is wrong for inband, because then
> we have no way to detect the link going down.

Yes I also saw this and didn't understand it. When I clear the force bit
it will be set back to 1 again when AN is enabled. I thought this might
be a bug of the controller.

> 
> > For 2.5Gbit/s it disables inband AN and forces the link to be up:
> > Memory mapped at address 0xffffaff88000.
> > Value at address 0xF2132E0C (0xffffaff88e0c): 0x9042
> > 
> > Then the status register shows also link up for 2.5Gbit/s:
> > Memory mapped at address 0xffffa5fe2000.
> > Value at address 0xF2132E10 (0xffffa5fe2e10): 0x683B
> > 
> > What might be confusing is that the port type, bit 1 in MAC Control
> > Register0, is set to SGMII for 2.5Gbit/s, because we can only turn off
> > autonegotiation for SGMII:
> > Memory mapped at address 0xffff8c26c000.
> > Value at address 0xF2132E00 (0xffff8c26ce00): 0x8BFD
> 
> Control register 0 bit 1 is the port type bit, which controls whether
> the port is in "1000base-X" mode or SGMII mode. This has no effect on
> the interpretation of the inband control word.
> 
> Control register 2 bit 0 controls whether the port uses 802.3z
> format control words or SGMII format control words.
> 
> > My example patch still uses the old macros:
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > index 97e38f61ac65..15fadfd61313 100644
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > @@ -6223,9 +6223,12 @@ static unsigned int mvpp2_gmac_pcs_query_inband(struct phylink_pcs *pcs,
> >  	 * When <PortType> = 1 (1000BASE-X) this field must be set to 1.
> >  	 * Therefore, inband is "required".
> >  	 */
> > -	if (phy_interface_mode_is_8023z(interface))
> > +	if (interface == PHY_INTERFACE_MODE_1000BASEX)
> >  		return LINK_INBAND_VALID | LINK_INBAND_REQUIRED;
> >  
> > +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> > +		return LINK_INBAND_VALID | LINK_INBAND_POSSIBLE;
> 
> This is not correct though. If we set PortType = 1, then this applies:
> 
>   Bit   Field        Type / InitVal   Description
> 
>   2     InBandAnEn   RW               In-band Auto-Negotiation enable.
> ...
>                                       When <PortType> = 1 (1000BASE-X)
>                                       this field must be set to 1.
>                                                                                                                       When <PortType> = 0 (SGMII) and this
>                                       field is 1, in-band Flow Control not
> 				      supported.
> 
> So, if we have the port in 1000base-X mode (PortType = 1) then bit 2
> of the Autoneg configuration register needs to be set to be compliant
> with the documentation. Therefore, since we set PortType = 1 for
> 2500BASE-X (and note that I _do_ run 2500BASE-X with inband AN enabled
> over fibre transceivers here, so this is needed), we also need to
> enable InBandAnEn.
> 
> This is exactly where the difficulty comes - because what you're
> suggesting will break currently working setups such as what I have
> here.
> 
> Switching the port to SGMII mode for 2500base-X doesn't sound like a
> sensible thing to do.

Maybe this is my misunderstanding. Is SGMII and 1000BASE-X not the same
if there is no inband AN? I thought the only difference is how the
inband AN works.

Regards,
Stefan

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Russell King (Oracle)]

On Thu, Apr 25, 2024 at 05:51:57PM +0200, Stefan Eichenberger wrote:
> On Thu, Apr 25, 2024 at 03:30:52PM +0100, Russell King (Oracle) wrote:
> > On Thu, Apr 25, 2024 at 01:24:51PM +0200, Stefan Eichenberger wrote:
> > > I think it should also work for mvneta, at least
> > > the code looks almost the same. I get the following for the Port
> > > Auto-Negotiation Configuration Register:
> > > 
> > > For 1Gbit/s it switches to SGMII and enables inband AN:
> > > Memory mapped at address 0xffffa0112000.
> > > Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6
> > 
> > So here the link is forced up which is wrong for inband, because then
> > we have no way to detect the link going down.
> 
> Yes I also saw this and didn't understand it. When I clear the force bit
> it will be set back to 1 again when AN is enabled. I thought this might
> be a bug of the controller.

No it isn't, the hardware never changes the value in this register.
The difference will be because of what I explained previously.

Because the mvneta and mvpp2 hardware is "weird" in that these two
registers provide both PCS and MAC functions, we have to deal with
them in a way that is split between the phylink PCS and MAC
operations.

This code was written at a time when all we had was MLO_AN_* and
none of the PHYLINK_PCS_NEG_* stuff. PCSes got passed the MLO_AN_*
constants which were the same as what was passed via the MAC
operations. This made the implementation entirely consistent.

However, that lead PCS implementers to go off and do their own
things, so we ended up with a range of different PCS behaviours
depending on the implementation (because the way people interpreted
MLO_AN_INBAND, interface mode, and the Autoneg bit in the advertising
mask was all different.

So to bring some consistency, I changed the PCS interface to what we
have now, in the belief that it would later allow us to solve the
2500base-X problem.

However, I'd forgotten about the mvneta/mvpp2 details, but that was
fine at the time of this change because everything was still
consistent - we would only ever use PHYLINK_PCS_NEG_OUTBAND or
PHYLINK_PCS_NEG_NONE for non-MLO_AN_INBAND modes, and
PHYLINK_PCS_NEG_INBAND_* for interface modes that support inband
when using MLO_AN_INBAND.

Now, when trying to solve the 2500base-X problem which needs us to
use PHYLINK_PCS_NEG_OUTBAND in some situations, this means we can
end up with MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND, and this is
what is causing me problems (the link isn't forced up.)

Conversely, I suspect you have the situation where you have MLO_AN_PHY
or MLO_AN_FIXED being passed to the mac_config() and mac_link_up()
operations, but the PCS is being configured for a different mode.

I am wondering whether we should at the very least move the
configuration of the control register 0 and 2 to the pcs_config()
method so at least that's consistent with the PHYLINK_PCS_NEG_*
value passed to the PCS and thus the values programmed into the
autoneg config register. However, that still leaves a big hole in
how we handle the link forcing - which is necessary if inband AN
is disabled (in other words, if we need to read the link status
from the PHY as is done in MLO_AN_PHY mode.)

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

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Stefan Eichenberger]

On Thu, Apr 25, 2024 at 05:30:50PM +0100, Russell King (Oracle) wrote:
> On Thu, Apr 25, 2024 at 05:51:57PM +0200, Stefan Eichenberger wrote:
> > On Thu, Apr 25, 2024 at 03:30:52PM +0100, Russell King (Oracle) wrote:
> > > On Thu, Apr 25, 2024 at 01:24:51PM +0200, Stefan Eichenberger wrote:
> > > > I think it should also work for mvneta, at least
> > > > the code looks almost the same. I get the following for the Port
> > > > Auto-Negotiation Configuration Register:
> > > > 
> > > > For 1Gbit/s it switches to SGMII and enables inband AN:
> > > > Memory mapped at address 0xffffa0112000.
> > > > Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6
> > > 
> > > So here the link is forced up which is wrong for inband, because then
> > > we have no way to detect the link going down.
> > 
> > Yes I also saw this and didn't understand it. When I clear the force bit
> > it will be set back to 1 again when AN is enabled. I thought this might
> > be a bug of the controller.
> 
> No it isn't, the hardware never changes the value in this register.
> The difference will be because of what I explained previously.
> 
> Because the mvneta and mvpp2 hardware is "weird" in that these two
> registers provide both PCS and MAC functions, we have to deal with
> them in a way that is split between the phylink PCS and MAC
> operations.
> 
> This code was written at a time when all we had was MLO_AN_* and
> none of the PHYLINK_PCS_NEG_* stuff. PCSes got passed the MLO_AN_*
> constants which were the same as what was passed via the MAC
> operations. This made the implementation entirely consistent.
> 
> However, that lead PCS implementers to go off and do their own
> things, so we ended up with a range of different PCS behaviours
> depending on the implementation (because the way people interpreted
> MLO_AN_INBAND, interface mode, and the Autoneg bit in the advertising
> mask was all different.
> 
> So to bring some consistency, I changed the PCS interface to what we
> have now, in the belief that it would later allow us to solve the
> 2500base-X problem.
> 
> However, I'd forgotten about the mvneta/mvpp2 details, but that was
> fine at the time of this change because everything was still
> consistent - we would only ever use PHYLINK_PCS_NEG_OUTBAND or
> PHYLINK_PCS_NEG_NONE for non-MLO_AN_INBAND modes, and
> PHYLINK_PCS_NEG_INBAND_* for interface modes that support inband
> when using MLO_AN_INBAND.
> 
> Now, when trying to solve the 2500base-X problem which needs us to
> use PHYLINK_PCS_NEG_OUTBAND in some situations, this means we can
> end up with MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND, and this is
> what is causing me problems (the link isn't forced up.)
> 
> Conversely, I suspect you have the situation where you have MLO_AN_PHY
> or MLO_AN_FIXED being passed to the mac_config() and mac_link_up()
> operations, but the PCS is being configured for a different mode.
> 
> I am wondering whether we should at the very least move the
> configuration of the control register 0 and 2 to the pcs_config()
> method so at least that's consistent with the PHYLINK_PCS_NEG_*
> value passed to the PCS and thus the values programmed into the
> autoneg config register. However, that still leaves a big hole in
> how we handle the link forcing - which is necessary if inband AN
> is disabled (in other words, if we need to read the link status
> from the PHY as is done in MLO_AN_PHY mode.)
> 

Now I got it, thanks a lot for the explanation. So the issue is that
MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND is happening in my use case and
therefore the link is not forced up because for that MLO_AN_PHY would be
needed. I will also try to think about it.

Thanks,
Stefan

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Russell King (Oracle)]

On Thu, Apr 25, 2024 at 07:33:59PM +0200, Stefan Eichenberger wrote:
> Now I got it, thanks a lot for the explanation. So the issue is that
> MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND is happening in my use case and
> therefore the link is not forced up because for that MLO_AN_PHY would be
> needed. I will also try to think about it.

Now that I've moved the setting of PortType and InBandAutoNegMode into
the pcs_config() method, I now have (on mvneta):

Value at address 0xf1036c00: 0x00008bfd	- PortType = 0
					  (SGMII, necessary to be able
					   to set InBandAnEn=0 below)

Value at address 0xf1036c08: 0x0000c018 - InBandAutoNegMode = 0
					  (1000Base-X mode)

Value at address 0xf1036c0c: 0x00009240	- 1000M, FD, unforced link
					  InBandAnEn = 0

Value at address 0xf1036c10: 0x0000600a - Sync, 1000M, FD, but no link

The reason that the link isn't being forced is because
mvneta_mac_link_up() is being called with mode = MLO_AN_INBAND
which expects the link to be controlled as a result of autoneg,
but we've configured autoneg to be off.

I'm wondering whether we need pl->cur_link_an_mode to be the desired
mode for selecting the result from phylink_pcs_neg_mode(), but also
maintain a separate pl->act_link_an_mode which phylink_pcs_neg_mode()
chooses, dependent on whether the PCS is using inband or outband
mode - and pl->act_link_an_mode is what gets passed to the MAC layer.
That would at least keep the MAC MLO_AN_* consistent with what the
PCS layer is using - and also has the advantage that it makes it
clear that pl->act_link_an_mode only gets updated in the "major
config" path.

A quick test of that... seems to work:

mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq=POLL)
mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,00008000,0000206c advertising 00,00000000,00008000,0000206c
mvneta f1034000.ethernet eno2: major config 2500base-x
mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01
mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none adv=00,00000000,00008000,0000206c pause=04
mvneta f1034000.ethernet eno2: phylink_sfp_module_start()
mvneta f1034000.ethernet eno2: phylink_sfp_link_up()
mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off
mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off
mvneta f1034000.ethernet eno2: major config sgmii
mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01
mvneta f1034000.ethernet eno2: phylink_mac_config: mode=phy/sgmii/none adv=00,00000000,00008000,0000206c pause=00
mvneta f1034000.ethernet eno2: pcs link down
mvneta f1034000.ethernet eno2: pcs link down
mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active
mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us
mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off

Value at address 0xf1036c00: 0x00008bfd
Value at address 0xf1036c08: 0x0000c018
Value at address 0xf1036c0c: 0x00009242
Value at address 0xf1036c10: 0x0000600b

So we can see in the two phylink_mac_config calls that the mode has
switched from "inband" to "phy" with this PHY (BCM84881) which
doesn't support inband in any interface modes.

However, there's still the issue with:

link modes: pcs=02 phy=01
phylink_mac_config: mode=inband/2500base-x/none adv=00,00000000,00008000,0000206c pause=04

and this is because of the missing code in this part:

	/* PHY present, inband mode depends on the capabilities
	 * of both.
	 */

but there's also the issue that the PCS and PHY capabilities like that
are incompatible. In this case, we're saved by the fact that if we do
this act_link_an_mode thing:

        pl->act_link_an_mode = pl->cur_link_an_mode;
        if (pl->pcs_neg_mode == PHYLINK_PCS_NEG_OUTBAND &&
            pl->act_link_an_mode == MLO_AN_INBAND)
                pl->act_link_an_mode = MLO_AN_PHY;

coupled with the new _behaviour_ of mvneta/mvpp2, we don't actually
end up in the "1000base-X must have AN enabled" trap... but that is
no basis to basing decisions at the phylink layer on.

So, I'm wondering whether we need to be a little more creative here.
Instead of simply passing a few bits, maybe something like:

	31-24: bitfield of "partner" capabilities that are supported
		for inband enabled mode
	23-16: bitfield of "partner" capabilities that are supported
		for inband disabled mode
	15-8: bitfield of "partner" capabilities that are supported
		for outband mode
	2: bypass mode supported
	1: inband enabled mode supported
	0: inband disabled mode supported

Now, a question will come up... what is different between inband
disabled and outband mode?

Consider 1000base-X fibre. 1000base-X is the media interface, and we
need to be able to configure autoneg there, enabling or disabling it.
If we don't support disabling autoneg (as is the case with mvneta
et.al. over fibre) then being able to use ethtool to disable autoneg
can't be used. In both these modes, the 1000base-X is the media side.

However, 1000base-X can be used to connect to a PHY, and the PHY
could do rate matching, so the we need to use an outband way to
access the media side (we need to talk to the PHY.)

Hence why PCS have a distinction between OUTBAND and INBAND_DISABLED.

Now, with 2500base-X we run into the problem that e.g. mvneta
operating in 1000base-X mode upclocked to 2.5G can only support
INBAND_ENABLED and not INBAND_DISABLED (we can't just turn off the
InBandAnEn bit). The change between INBAND_ENABLED and INBAND_DISABLED
can happen with the link up.

However, it can support OUTBAND by disabling the PortType bit and then
turning off InBandAnEn (which can only be done with the link *down*
and that is only guaranteed during a "major config" not through the
ethtool settings API - which is why pcs_config() can't do this for 
INBAND_DISABLED.)

So, a little bit of progress but not at a usable solution yet.

I'm afraid my current tree is in a hacky mess at the moment, I'll see
about updating the published patches as soon as I can.

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

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Stefan Eichenberger]

Hi Russell,

Sorry for the late reply but I wanted to give you some update after
testing with the latest version of your patches on net-queue.

On Thu, Apr 25, 2024 at 09:15:54PM +0100, Russell King (Oracle) wrote:
> On Thu, Apr 25, 2024 at 07:33:59PM +0200, Stefan Eichenberger wrote:
> > Now I got it, thanks a lot for the explanation. So the issue is that
> > MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND is happening in my use case and
> > therefore the link is not forced up because for that MLO_AN_PHY would be
> > needed. I will also try to think about it.
> 
> Now that I've moved the setting of PortType and InBandAutoNegMode into
> the pcs_config() method, I now have (on mvneta):
> 
> Value at address 0xf1036c00: 0x00008bfd	- PortType = 0
> 					  (SGMII, necessary to be able
> 					   to set InBandAnEn=0 below)
> 
> Value at address 0xf1036c08: 0x0000c018 - InBandAutoNegMode = 0
> 					  (1000Base-X mode)
> 
> Value at address 0xf1036c0c: 0x00009240	- 1000M, FD, unforced link
> 					  InBandAnEn = 0
> 
> Value at address 0xf1036c10: 0x0000600a - Sync, 1000M, FD, but no link
> 
> The reason that the link isn't being forced is because
> mvneta_mac_link_up() is being called with mode = MLO_AN_INBAND
> which expects the link to be controlled as a result of autoneg,
> but we've configured autoneg to be off.
> 
> I'm wondering whether we need pl->cur_link_an_mode to be the desired
> mode for selecting the result from phylink_pcs_neg_mode(), but also
> maintain a separate pl->act_link_an_mode which phylink_pcs_neg_mode()
> chooses, dependent on whether the PCS is using inband or outband
> mode - and pl->act_link_an_mode is what gets passed to the MAC layer.
> That would at least keep the MAC MLO_AN_* consistent with what the
> PCS layer is using - and also has the advantage that it makes it
> clear that pl->act_link_an_mode only gets updated in the "major
> config" path.
> 
> A quick test of that... seems to work:
> 
> mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq=POLL)
> mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,00008000,0000206c advertising 00,00000000,00008000,0000206c
> mvneta f1034000.ethernet eno2: major config 2500base-x
> mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01
> mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none adv=00,00000000,00008000,0000206c pause=04
> mvneta f1034000.ethernet eno2: phylink_sfp_module_start()
> mvneta f1034000.ethernet eno2: phylink_sfp_link_up()
> mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off
> mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off
> mvneta f1034000.ethernet eno2: major config sgmii
> mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01
> mvneta f1034000.ethernet eno2: phylink_mac_config: mode=phy/sgmii/none adv=00,00000000,00008000,0000206c pause=00
> mvneta f1034000.ethernet eno2: pcs link down
> mvneta f1034000.ethernet eno2: pcs link down
> mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active
> mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us
> mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off
> 
> Value at address 0xf1036c00: 0x00008bfd
> Value at address 0xf1036c08: 0x0000c018
> Value at address 0xf1036c0c: 0x00009242
> Value at address 0xf1036c10: 0x0000600b
> 
> So we can see in the two phylink_mac_config calls that the mode has
> switched from "inband" to "phy" with this PHY (BCM84881) which
> doesn't support inband in any interface modes.
> 
> However, there's still the issue with:
> 
> link modes: pcs=02 phy=01
> phylink_mac_config: mode=inband/2500base-x/none adv=00,00000000,00008000,0000206c pause=04
> 
> and this is because of the missing code in this part:
> 
> 	/* PHY present, inband mode depends on the capabilities
> 	 * of both.
> 	 */
> 
> but there's also the issue that the PCS and PHY capabilities like that
> are incompatible. In this case, we're saved by the fact that if we do
> this act_link_an_mode thing:
> 
>         pl->act_link_an_mode = pl->cur_link_an_mode;
>         if (pl->pcs_neg_mode == PHYLINK_PCS_NEG_OUTBAND &&
>             pl->act_link_an_mode == MLO_AN_INBAND)
>                 pl->act_link_an_mode = MLO_AN_PHY;
> 
> coupled with the new _behaviour_ of mvneta/mvpp2, we don't actually
> end up in the "1000base-X must have AN enabled" trap... but that is
> no basis to basing decisions at the phylink layer on.
> 
> So, I'm wondering whether we need to be a little more creative here.
> Instead of simply passing a few bits, maybe something like:
> 
> 	31-24: bitfield of "partner" capabilities that are supported
> 		for inband enabled mode
> 	23-16: bitfield of "partner" capabilities that are supported
> 		for inband disabled mode
> 	15-8: bitfield of "partner" capabilities that are supported
> 		for outband mode
> 	2: bypass mode supported
> 	1: inband enabled mode supported
> 	0: inband disabled mode supported
> 
> Now, a question will come up... what is different between inband
> disabled and outband mode?
> 
> Consider 1000base-X fibre. 1000base-X is the media interface, and we
> need to be able to configure autoneg there, enabling or disabling it.
> If we don't support disabling autoneg (as is the case with mvneta
> et.al. over fibre) then being able to use ethtool to disable autoneg
> can't be used. In both these modes, the 1000base-X is the media side.
> 
> However, 1000base-X can be used to connect to a PHY, and the PHY
> could do rate matching, so the we need to use an outband way to
> access the media side (we need to talk to the PHY.)
> 
> Hence why PCS have a distinction between OUTBAND and INBAND_DISABLED.
> 
> Now, with 2500base-X we run into the problem that e.g. mvneta
> operating in 1000base-X mode upclocked to 2.5G can only support
> INBAND_ENABLED and not INBAND_DISABLED (we can't just turn off the
> InBandAnEn bit). The change between INBAND_ENABLED and INBAND_DISABLED
> can happen with the link up.
> 
> However, it can support OUTBAND by disabling the PortType bit and then
> turning off InBandAnEn (which can only be done with the link *down*
> and that is only guaranteed during a "major config" not through the
> ethtool settings API - which is why pcs_config() can't do this for 
> INBAND_DISABLED.)
> 
> So, a little bit of progress but not at a usable solution yet.
> 
> I'm afraid my current tree is in a hacky mess at the moment, I'll see
> about updating the published patches as soon as I can.

I think I see the problem you are describing.

When the driver starts it will negotiate MLO_AN_PHY based on the
capabilities of the PHY and of the PCS. However when I switch to 1GBit/s
it should switch to MLO_AN_INBAND but this does not work. Here the
output of phylink:
[   14.695170] mvpp2 f2000000.ethernet eth1: PHY [f212a600.mdio-mii:11] driver [Maxlinear Ethernet GPY215C] (irq=POLL)
[   14.706541] mvpp2 f2000000.ethernet eth1: phy: sgmii setting supported 00,00000000,00008000,0000606f advertising 00,00000000,00008000,0000606f
[   14.707770] mvpp2 f2000000.ethernet eth1: configuring for phy/sgmii link mode
[   14.716437] mvpp2 f2000000.ethernet eth1: major config, requested phy/sgmii
[   14.723260] mvpp2 f2000000.ethernet eth1: firmware wants phy mode, but PHY requires inband
[   14.731623] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/sgmii
[   14.731630] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/sgmii/none adv=00,00000000,00000000,00000000 pause=00
[   14.733905] mvpp2 f2000000.ethernet eth1: phy link down sgmii/2.5Gbps/Full/none/rx/tx
[   18.825056] mvpp2 f2000000.ethernet eth1: phy link up 2500base-x/2.5Gbps/Full/none/rx/tx
[   18.825083] mvpp2 f2000000.ethernet eth1: major config, requested phy/2500base-x
[   18.831331] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/2500base-x
[   18.832568] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/2500base-x/none adv=00,00000000,00000000,00000000 pause=03
[   18.832638] mvpp2 f2000000.ethernet eth1: pcs link up
[   18.832905] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, inactive
[   18.832936] mvpp2 f2000000.ethernet eth1: Link is Up - 2.5Gbps/Full - flow control rx/tx
[   60.808001] mvpp2 f2000000.ethernet eth1: phy link down 2500base-x/Unknown/Full/none/rx/tx
[   60.808038] mvpp2 f2000000.ethernet eth1: deactivating EEE, was inactive
[   60.808090] mvpp2 f2000000.ethernet eth1: pcs link down
[   60.809075] mvpp2 f2000000.ethernet eth1: Link is Down
[   62.857142] mvpp2 f2000000.ethernet eth1: phy link up sgmii/1Gbps/Full/none/rx/tx
[   62.857163] mvpp2 f2000000.ethernet eth1: major config, requested phy/sgmii
[   62.863412] mvpp2 f2000000.ethernet eth1: firmware wants phy mode, but PHY requires inband
[   62.871786] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/sgmii
[   62.872987] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/sgmii/none adv=00,00000000,00000000,00000000 pause=03
[   62.873059] mvpp2 f2000000.ethernet eth1: pcs link up
[   62.873362] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, active
[   62.873379] mvpp2 f2000000.ethernet eth1: enabling tx_lpi, timer 250us
[   62.873414] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control rx/tx

The problem is that the PCS continues to be in phy mode but the PHY
driver currently only supports LINK_INBAND_ENABLE and SGMII for 1GBit/s.

What I'm wondering is if it wouldn't make sense to adapt the phy driver
to support MLO_AN_PHY in SGMII/1000BASE-X mode. However, I don't know
how I could do this because I can't get the information in the PHY
driver (I can't use phylink_autoneg_inband to querry if inband should be
enabled or disabled). I know we would still have the problem you
described above. However, it would allow us to configure the phy to what
phylink decided works best. Maybe this would also solve other issues
where e.g. the PCS is not as flexible as it is for mvpp2 and mvneta?

Currently the mxl-gpy phy driver can only support:
10/100/1000 MBit/s: SGMII with MLO_AN_INBAND
2500MBit/s          2500Base-X with MLO_AN_PHY

However, the PHY would also support the following mode:
10/100/1000 MBit/s: SGMII with MLO_AN_PHY

I just don't know how the PHY driver could know about what it should
configure.

Regards,
Stefan

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Russell King (Oracle)]

On Thu, May 02, 2024 at 03:44:24PM +0200, Stefan Eichenberger wrote:
> Hi Russell,
> 
> Sorry for the late reply but I wanted to give you some update after
> testing with the latest version of your patches on net-queue.

I've also been randomly distracted, and I've been meaning to ping you
to test some of the updates.

http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue

The current set begins with:

"net: sfp-bus: constify link_modes to sfp_select_interface()" which is
now in net-next, then the patches between and including:

"net: phylink: validate sfp_select_interface() returned interface" to
"net: phylink: clean up phylink_resolve()"

That should get enough together for the PCS "neg" mode to be consistent
with what the MAC driver sees.

The remaining bits that I still need to sort out is the contents of
phylink_pcs_neg_mode() for the 802.3z mode with PHY, and also working
out some way of handling the SGMII case where the PHY and PCS disagree
(one only supporting inband the other not supporting inband.)

I'm not sure when I'll be able to get to that - things are getting
fairly chaotic again, meaning I have again less time to spend on
mainline... and I'd like to take some vacation time very soon (I really
need some time off!)

> I think I see the problem you are describing.
> 
> When the driver starts it will negotiate MLO_AN_PHY based on the
> capabilities of the PHY and of the PCS. However when I switch to 1GBit/s
> it should switch to MLO_AN_INBAND but this does not work. Here the
> output of phylink:

I'm designing this to work the other way - inband being able to fall
back to PHY (out of band) mode rather than PHY mode being able to fall
forwards to inband mode.

> The problem is that the PCS continues to be in phy mode but the PHY
> driver currently only supports LINK_INBAND_ENABLE and SGMII for 1GBit/s.
> 
> What I'm wondering is if it wouldn't make sense to adapt the phy driver
> to support MLO_AN_PHY in SGMII/1000BASE-X mode.

PHYs have no idea about MLO_AN_xxx at all, neither should they. This
is not part of phylib's API, and I don't think PHYs should ever know
about MLO_AN_xxx stuff (which is something purely between phylink and
the MAC driver.) The structure here is:

      MAC            PCS                   PHY
       ^              ^                    ^ ^-----.
       v              v                    v       |
   MAC driver <-> PCS driver <-------> PHY driver  |
       ^           ^                    ^          |
       |           |                    |          |
  MLO_AN_xxx  PHYLINK_PCS_NEG_xxx       |          |
       `           '                    |          |
        \         /                     v          |
          phylink <----------------> phylib <------'

MLO_AN_xxx is far beyond the PHY, and more or less defines the overall
"system" operating mode. PHYLINK_PCS_NEG_xxx defines the properties
used for the PCS link to the next device towards the media. This is
more of relevance to what the PHY should be using on its MAC-facing
interface.

> Currently the mxl-gpy phy driver can only support:
> 10/100/1000 MBit/s: SGMII with MLO_AN_INBAND
> 2500MBit/s          2500Base-X with MLO_AN_PHY
> 
> However, the PHY would also support the following mode:
> 10/100/1000 MBit/s: SGMII with MLO_AN_PHY

The problem with this is some PHYs will not pass data _unless_ their
SGMII control frame to the PCS has been acknowledged by the PCS - in
other words, inband has to be used. However, that can be coped with,
because such a PHY driver should be saying that it only supports
LINK_INBAND_ENABLE in SGMII mode... and firmware must tell phylink
that it wants to use inband mode (as that's exactly what firmware
must do today in this situation.)

> I just don't know how the PHY driver could know about what it should
> configure.

Currently, I haven't added an interface to cope with the case where
a PHY says LINK_INBAND_ENABLE | LINK_INBAND_DISABLE to allow it to be
configured in that case... that's something that will eventually be
needed.

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

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Stefan Eichenberger]

On Thu, May 02, 2024 at 03:14:13PM +0100, Russell King (Oracle) wrote:
> On Thu, May 02, 2024 at 03:44:24PM +0200, Stefan Eichenberger wrote:
> > Hi Russell,
> > 
> > Sorry for the late reply but I wanted to give you some update after
> > testing with the latest version of your patches on net-queue.
> 
> I've also been randomly distracted, and I've been meaning to ping you
> to test some of the updates.
> 
> http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
> 
> The current set begins with:
> 
> "net: sfp-bus: constify link_modes to sfp_select_interface()" which is
> now in net-next, then the patches between and including:
> 
> "net: phylink: validate sfp_select_interface() returned interface" to
> "net: phylink: clean up phylink_resolve()"
> 
> That should get enough together for the PCS "neg" mode to be consistent
> with what the MAC driver sees.
> 
> The remaining bits that I still need to sort out is the contents of
> phylink_pcs_neg_mode() for the 802.3z mode with PHY, and also working
> out some way of handling the SGMII case where the PHY and PCS disagree
> (one only supporting inband the other not supporting inband.)
> 
> I'm not sure when I'll be able to get to that - things are getting
> fairly chaotic again, meaning I have again less time to spend on
> mainline... and I'd like to take some vacation time very soon (I really
> need some time off!)

No problem, I'm also quite busy at the moment and I have the workaround
to test the hardware, so it is nothing urgent for me.

> > I think I see the problem you are describing.
> > 
> > When the driver starts it will negotiate MLO_AN_PHY based on the
> > capabilities of the PHY and of the PCS. However when I switch to 1GBit/s
> > it should switch to MLO_AN_INBAND but this does not work. Here the
> > output of phylink:
> 
> I'm designing this to work the other way - inband being able to fall
> back to PHY (out of band) mode rather than PHY mode being able to fall
> forwards to inband mode.

I tested again with 89e0a87ef79db9f3ce879e9d977429ba89ca8229 and I think
in my setup the problem is that it doesn't fall back to PHY mode but
takes it as default mode. Here what happens when I have the mxl-gpy PHY
connected to a 1000 GBit/s port:
[    9.331179] mvpp2 f2000000.ethernet eth1: Using firmware node mac address 00:51:82:11:22:02
[   14.674836] mvpp2 f2000000.ethernet eth1: PHY f212a600.mdio-mii:11 doesn't supply possible interfaces
[   14.674853] mvpp2 f2000000.ethernet eth1:  interface 2 (mii) rate match none supports 0-3,6,13-14
[   14.674864] mvpp2 f2000000.ethernet eth1:  interface 4 (sgmii) rate match none supports 0-3,5-6,13-14
[   14.674871] mvpp2 f2000000.ethernet eth1:  interface 9 (rgmii) rate match none supports 0-3,5-6,13-14
[   14.674877] mvpp2 f2000000.ethernet eth1:  interface 10 (rgmii-id) rate match none supports 0-3,5-6,13-14
[   14.674883] mvpp2 f2000000.ethernet eth1:  interface 11 (rgmii-rxid) rate match none supports 0-3,5-6,13-14
[   14.674889] mvpp2 f2000000.ethernet eth1:  interface 12 (rgmii-txid) rate match none supports 0-3,5-6,13-14
[   14.674895] mvpp2 f2000000.ethernet eth1:  interface 22 (1000base-x) rate match none supports 5-6,13-14
[   14.674900] mvpp2 f2000000.ethernet eth1:  interface 23 (2500base-x) rate match none supports 6,13-14,47
[   14.674907] mvpp2 f2000000.ethernet eth1: PHY [f212a600.mdio-mii:11] driver [Maxlinear Ethernet GPY215C] (irq=POLL)
[   14.685444] mvpp2 f2000000.ethernet eth1: phy: 2500base-x setting supported 00,00000000,00008000,0000606f advertising 00,00000000,00008000,0000606f
[   14.686635] mvpp2 f2000000.ethernet eth1: configuring for phy/2500base-x link mode
[   14.694263] mvpp2 f2000000.ethernet eth1: major config, requested phy/2500base-x
[   14.700402] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/2500base-x
[   14.700408] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/2500base-x/none adv=00,00000000,00000000,00000000 pause=00
[   14.702726] mvpp2 f2000000.ethernet eth1: phy link down 2500base-x/1Gbps/Full/none/off
[   17.768349] mvpp2 f2000000.ethernet eth1: phy link up sgmii/1Gbps/Full/none/rx/tx
[   17.768370] mvpp2 f2000000.ethernet eth1: major config, requested phy/sgmii
[   17.774602] mvpp2 f2000000.ethernet eth1: firmware wants phy mode, but PHY requires inband
[   17.782976] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/sgmii
[   17.784200] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/sgmii/none adv=00,00000000,00000000,00000000 pause=03
[   17.784278] mvpp2 f2000000.ethernet eth1: pcs link up
[   17.784485] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, active
[   17.784504] mvpp2 f2000000.ethernet eth1: enabling tx_lpi, timer 250us
[   17.784543] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control rx/tx

It agrees on PHY mode when starting, which then sets MLO_AN_PHY.
However, when it sees a 1 GBit/s link it will just print the error
message "firmware wants phy mode, but PHY requires inband". After "Link
is Up" is shown the link will not work becuse of the missmatch between
the PCS and the PHY.
 
> > The problem is that the PCS continues to be in phy mode but the PHY
> > driver currently only supports LINK_INBAND_ENABLE and SGMII for 1GBit/s.
> > 
> > What I'm wondering is if it wouldn't make sense to adapt the phy driver
> > to support MLO_AN_PHY in SGMII/1000BASE-X mode.
> 
> PHYs have no idea about MLO_AN_xxx at all, neither should they. This
> is not part of phylib's API, and I don't think PHYs should ever know
> about MLO_AN_xxx stuff (which is something purely between phylink and
> the MAC driver.) The structure here is:
> 
>       MAC            PCS                   PHY
>        ^              ^                    ^ ^-----.
>        v              v                    v       |
>    MAC driver <-> PCS driver <-------> PHY driver  |
>        ^           ^                    ^          |
>        |           |                    |          |
>   MLO_AN_xxx  PHYLINK_PCS_NEG_xxx       |          |
>        `           '                    |          |
>         \         /                     v          |
>           phylink <----------------> phylib <------'
> 
> MLO_AN_xxx is far beyond the PHY, and more or less defines the overall
> "system" operating mode. PHYLINK_PCS_NEG_xxx defines the properties
> used for the PCS link to the next device towards the media. This is
> more of relevance to what the PHY should be using on its MAC-facing
> interface.

Okay this makes sense. I more thought about if the PHY e.g. would
signalise it can do inband/outband and the PCS can only do outband.
Basically exactly what you write in the last comment.

> > Currently the mxl-gpy phy driver can only support:
> > 10/100/1000 MBit/s: SGMII with MLO_AN_INBAND
> > 2500MBit/s          2500Base-X with MLO_AN_PHY
> > 
> > However, the PHY would also support the following mode:
> > 10/100/1000 MBit/s: SGMII with MLO_AN_PHY
> 
> The problem with this is some PHYs will not pass data _unless_ their
> SGMII control frame to the PCS has been acknowledged by the PCS - in
> other words, inband has to be used. However, that can be coped with,
> because such a PHY driver should be saying that it only supports
> LINK_INBAND_ENABLE in SGMII mode... and firmware must tell phylink
> that it wants to use inband mode (as that's exactly what firmware
> must do today in this situation.)
> 
> > I just don't know how the PHY driver could know about what it should
> > configure.
> 
> Currently, I haven't added an interface to cope with the case where
> a PHY says LINK_INBAND_ENABLE | LINK_INBAND_DISABLE to allow it to be
> configured in that case... that's something that will eventually be
> needed.

Thanks a lot for the work and help so far.

Regards,
Stefan

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Russell King (Oracle)]

On Fri, May 03, 2024 at 06:35:53PM +0200, Stefan Eichenberger wrote:
> On Thu, May 02, 2024 at 03:14:13PM +0100, Russell King (Oracle) wrote:
> > On Thu, May 02, 2024 at 03:44:24PM +0200, Stefan Eichenberger wrote:
> > > Hi Russell,
> > > 
> > > Sorry for the late reply but I wanted to give you some update after
> > > testing with the latest version of your patches on net-queue.
> > 
> > I've also been randomly distracted, and I've been meaning to ping you
> > to test some of the updates.
> > 
> > http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
> > 
> > The current set begins with:
> > 
> > "net: sfp-bus: constify link_modes to sfp_select_interface()" which is
> > now in net-next, then the patches between and including:
> > 
> > "net: phylink: validate sfp_select_interface() returned interface" to
> > "net: phylink: clean up phylink_resolve()"
> > 
> > That should get enough together for the PCS "neg" mode to be consistent
> > with what the MAC driver sees.
> > 
> > The remaining bits that I still need to sort out is the contents of
> > phylink_pcs_neg_mode() for the 802.3z mode with PHY, and also working
> > out some way of handling the SGMII case where the PHY and PCS disagree
> > (one only supporting inband the other not supporting inband.)
> > 
> > I'm not sure when I'll be able to get to that - things are getting
> > fairly chaotic again, meaning I have again less time to spend on
> > mainline... and I'd like to take some vacation time very soon (I really
> > need some time off!)
> 
> No problem, I'm also quite busy at the moment and I have the workaround
> to test the hardware, so it is nothing urgent for me.
> 
> > > I think I see the problem you are describing.
> > > 
> > > When the driver starts it will negotiate MLO_AN_PHY based on the
> > > capabilities of the PHY and of the PCS. However when I switch to 1GBit/s
> > > it should switch to MLO_AN_INBAND but this does not work. Here the
> > > output of phylink:
> > 
> > I'm designing this to work the other way - inband being able to fall
> > back to PHY (out of band) mode rather than PHY mode being able to fall
> > forwards to inband mode.
> 
> I tested again with 89e0a87ef79db9f3ce879e9d977429ba89ca8229 and I think
> in my setup the problem is that it doesn't fall back to PHY mode but
> takes it as default mode. Here what happens when I have the mxl-gpy PHY
> connected to a 1000 GBit/s port:
> [    9.331179] mvpp2 f2000000.ethernet eth1: Using firmware node mac address 00:51:82:11:22:02
> [   14.674836] mvpp2 f2000000.ethernet eth1: PHY f212a600.mdio-mii:11 doesn't supply possible interfaces
> [   14.674853] mvpp2 f2000000.ethernet eth1:  interface 2 (mii) rate match none supports 0-3,6,13-14
> [   14.674864] mvpp2 f2000000.ethernet eth1:  interface 4 (sgmii) rate match none supports 0-3,5-6,13-14
> [   14.674871] mvpp2 f2000000.ethernet eth1:  interface 9 (rgmii) rate match none supports 0-3,5-6,13-14
> [   14.674877] mvpp2 f2000000.ethernet eth1:  interface 10 (rgmii-id) rate match none supports 0-3,5-6,13-14
> [   14.674883] mvpp2 f2000000.ethernet eth1:  interface 11 (rgmii-rxid) rate match none supports 0-3,5-6,13-14
> [   14.674889] mvpp2 f2000000.ethernet eth1:  interface 12 (rgmii-txid) rate match none supports 0-3,5-6,13-14
> [   14.674895] mvpp2 f2000000.ethernet eth1:  interface 22 (1000base-x) rate match none supports 5-6,13-14
> [   14.674900] mvpp2 f2000000.ethernet eth1:  interface 23 (2500base-x) rate match none supports 6,13-14,47
> [   14.674907] mvpp2 f2000000.ethernet eth1: PHY [f212a600.mdio-mii:11] driver [Maxlinear Ethernet GPY215C] (irq=POLL)
> [   14.685444] mvpp2 f2000000.ethernet eth1: phy: 2500base-x setting supported 00,00000000,00008000,0000606f advertising 00,00000000,00008000,0000606f
> [   14.686635] mvpp2 f2000000.ethernet eth1: configuring for phy/2500base-x link mode
> [   14.694263] mvpp2 f2000000.ethernet eth1: major config, requested phy/2500base-x

                                                                       ^^^

You're still requesting (from firmware) for PHY mode, and phylink will
_always_ use out-of-band if firmware requests that.

> [   14.700402] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/2500base-x

So it uses PHY mode for 2500base-X, which is correct.

> [   17.768370] mvpp2 f2000000.ethernet eth1: major config, requested phy/sgmii

Still requesting PHY mode with SGMII, which historically we've always
used out-of-band mode for, so we preserve that behaviour.

> [   17.774602] mvpp2 f2000000.ethernet eth1: firmware wants phy mode, but PHY requires inband

So we complain about it with an error, because it is wrong...

> [   17.782976] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/sgmii

and we still try to use it (correctly, because that's what phylink
has always done in this case.)

As I tried to explain, there is fall-back from MLO_AN_INBAND to
MLO_AN_PHY, but there won't be fall-forward from MLO_AN_PHY to
MLO_AN_INBAND.

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

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Stefan Eichenberger]

On Fri, May 03, 2024 at 11:41:03PM +0100, Russell King (Oracle) wrote:
> On Fri, May 03, 2024 at 06:35:53PM +0200, Stefan Eichenberger wrote:
> > On Thu, May 02, 2024 at 03:14:13PM +0100, Russell King (Oracle) wrote:
> > > On Thu, May 02, 2024 at 03:44:24PM +0200, Stefan Eichenberger wrote:
> > > > Hi Russell,
> > > > 
> > > > Sorry for the late reply but I wanted to give you some update after
> > > > testing with the latest version of your patches on net-queue.
> > > 
> > > I've also been randomly distracted, and I've been meaning to ping you
> > > to test some of the updates.
> > > 
> > > http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
> > > 
> > > The current set begins with:
> > > 
> > > "net: sfp-bus: constify link_modes to sfp_select_interface()" which is
> > > now in net-next, then the patches between and including:
> > > 
> > > "net: phylink: validate sfp_select_interface() returned interface" to
> > > "net: phylink: clean up phylink_resolve()"
> > > 
> > > That should get enough together for the PCS "neg" mode to be consistent
> > > with what the MAC driver sees.
> > > 
> > > The remaining bits that I still need to sort out is the contents of
> > > phylink_pcs_neg_mode() for the 802.3z mode with PHY, and also working
> > > out some way of handling the SGMII case where the PHY and PCS disagree
> > > (one only supporting inband the other not supporting inband.)
> > > 
> > > I'm not sure when I'll be able to get to that - things are getting
> > > fairly chaotic again, meaning I have again less time to spend on
> > > mainline... and I'd like to take some vacation time very soon (I really
> > > need some time off!)
> > 
> > No problem, I'm also quite busy at the moment and I have the workaround
> > to test the hardware, so it is nothing urgent for me.
> > 
> > > > I think I see the problem you are describing.
> > > > 
> > > > When the driver starts it will negotiate MLO_AN_PHY based on the
> > > > capabilities of the PHY and of the PCS. However when I switch to 1GBit/s
> > > > it should switch to MLO_AN_INBAND but this does not work. Here the
> > > > output of phylink:
> > > 
> > > I'm designing this to work the other way - inband being able to fall
> > > back to PHY (out of band) mode rather than PHY mode being able to fall
> > > forwards to inband mode.
> > 
> > I tested again with 89e0a87ef79db9f3ce879e9d977429ba89ca8229 and I think
> > in my setup the problem is that it doesn't fall back to PHY mode but
> > takes it as default mode. Here what happens when I have the mxl-gpy PHY
> > connected to a 1000 GBit/s port:
> > [    9.331179] mvpp2 f2000000.ethernet eth1: Using firmware node mac address 00:51:82:11:22:02
> > [   14.674836] mvpp2 f2000000.ethernet eth1: PHY f212a600.mdio-mii:11 doesn't supply possible interfaces
> > [   14.674853] mvpp2 f2000000.ethernet eth1:  interface 2 (mii) rate match none supports 0-3,6,13-14
> > [   14.674864] mvpp2 f2000000.ethernet eth1:  interface 4 (sgmii) rate match none supports 0-3,5-6,13-14
> > [   14.674871] mvpp2 f2000000.ethernet eth1:  interface 9 (rgmii) rate match none supports 0-3,5-6,13-14
> > [   14.674877] mvpp2 f2000000.ethernet eth1:  interface 10 (rgmii-id) rate match none supports 0-3,5-6,13-14
> > [   14.674883] mvpp2 f2000000.ethernet eth1:  interface 11 (rgmii-rxid) rate match none supports 0-3,5-6,13-14
> > [   14.674889] mvpp2 f2000000.ethernet eth1:  interface 12 (rgmii-txid) rate match none supports 0-3,5-6,13-14
> > [   14.674895] mvpp2 f2000000.ethernet eth1:  interface 22 (1000base-x) rate match none supports 5-6,13-14
> > [   14.674900] mvpp2 f2000000.ethernet eth1:  interface 23 (2500base-x) rate match none supports 6,13-14,47
> > [   14.674907] mvpp2 f2000000.ethernet eth1: PHY [f212a600.mdio-mii:11] driver [Maxlinear Ethernet GPY215C] (irq=POLL)
> > [   14.685444] mvpp2 f2000000.ethernet eth1: phy: 2500base-x setting supported 00,00000000,00008000,0000606f advertising 00,00000000,00008000,0000606f
> > [   14.686635] mvpp2 f2000000.ethernet eth1: configuring for phy/2500base-x link mode
> > [   14.694263] mvpp2 f2000000.ethernet eth1: major config, requested phy/2500base-x
> 
>                                                                        ^^^
> 
> You're still requesting (from firmware) for PHY mode, and phylink will
> _always_ use out-of-band if firmware requests that.
> 
> > [   14.700402] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/2500base-x
> 
> So it uses PHY mode for 2500base-X, which is correct.
> 
> > [   17.768370] mvpp2 f2000000.ethernet eth1: major config, requested phy/sgmii
> 
> Still requesting PHY mode with SGMII, which historically we've always
> used out-of-band mode for, so we preserve that behaviour.
> 
> > [   17.774602] mvpp2 f2000000.ethernet eth1: firmware wants phy mode, but PHY requires inband
> 
> So we complain about it with an error, because it is wrong...
> 
> > [   17.782976] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/sgmii
> 
> and we still try to use it (correctly, because that's what phylink
> has always done in this case.)
> 
> As I tried to explain, there is fall-back from MLO_AN_INBAND to
> MLO_AN_PHY, but there won't be fall-forward from MLO_AN_PHY to
> MLO_AN_INBAND.

I still don't get it, sorry. So the mxl-gpy driver currently has two
modes:
2500 MBit/s -> PHY_INTERFACE_MODE_2500BASEX -> (0 no inband)
1000 MBit/s -> PHY_INTERFACE_MODE_SGMII -> LINK_INBAND_ENABLE
If I use this configureation it will not work because there is no
fallback from MLO_AN_PHY to MLO_AN_INBAND.

Now if I understand everyting correctly, this happens for 1000 MBit/s
and SGMII because the firmware decides to use PHY mode. I modified
the PHY driver to use 1000BASEX instead:
2500 MBit/s -> PHY_INTERFACE_MODE_2500BASEX -> (0 no inband)
1000 MBit/s -> PHY_INTERFACE_MODE_1000BASEX -> LINK_INBAND_ENABLE
However, the same thing happens:
[   14.635831] mvpp2 f2000000.ethernet eth1: PHY [f212a600.mdio-mii:11] driver [Maxlinear Ethernet GPY215C] (irq=POLL)
[   14.646742] mvpp2 f2000000.ethernet eth1: phy: 2500base-x setting supported 00,00000000,00008000,0000606f advertising 00,00000000,00008000,0000606f
[   14.647986] mvpp2 f2000000.ethernet eth1: configuring for phy/2500base-x link mode
[   14.655784] mvpp2 f2000000.ethernet eth1: major config, requested phy/2500base-x
[   14.663313] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/2500base-x
[   14.663323] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/2500base-x/none adv=00,00000000,00000000,00000000 pause=00
[   14.666098] mvpp2 f2000000.ethernet eth1: phy link down 2500base-x/2.5Gbps/Full/none/rx/tx
[   18.760959] mvpp2 f2000000.ethernet eth1: phy link up 2500base-x/2.5Gbps/Full/none/rx/tx
[   18.760977] mvpp2 f2000000.ethernet eth1: pcs link up
[   18.761211] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, inactive
[   18.761231] mvpp2 f2000000.ethernet eth1: Link is Up - 2.5Gbps/Full - flow control rx/tx
[   70.983936] mvpp2 f2000000.ethernet eth1: phy link down 2500base-x/Unknown/Full/none/rx/tx
[   70.983965] mvpp2 f2000000.ethernet eth1: deactivating EEE, was inactive
[   70.984017] mvpp2 f2000000.ethernet eth1: pcs link down
[   70.985000] mvpp2 f2000000.ethernet eth1: Link is Down
[   74.057088] mvpp2 f2000000.ethernet eth1: phy link up 1000base-x/1Gbps/Full/none/rx/tx
[   74.057109] mvpp2 f2000000.ethernet eth1: major config, requested phy/1000base-x
[   74.063342] mvpp2 f2000000.ethernet eth1: firmware wants phy mode, but PHY requires inband
[   74.071706] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/1000base-x
[   74.072902] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/1000base-x/none adv=00,00000000,00000000,00000000 pause=03
[   74.072976] mvpp2 f2000000.ethernet eth1: pcs link up
[   74.073225] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, active
[   74.073245] mvpp2 f2000000.ethernet eth1: enabling tx_lpi, timer 250us
[   74.073279] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control rx/tx

If I then request inband by setting managed = "in-band-status" in the
device tree the logic will fail because there is not phydev and
therefore the link will never be configured for phy mode (it falls back
to "Legacy, so determine inband depending on the advertising bit"):
[    9.299037] mvpp2 f2000000.ethernet eth1: Using firmware node mac address 00:51:82:11:22:02
[   14.586343] mvpp2 f2000000.ethernet eth1: configuring for inband/2500base-x link mode
[   14.595669] mvpp2 f2000000.ethernet eth1: major config, requested inband/2500base-x
[   14.631876] mvpp2 f2000000.ethernet eth1: major config, active inband/inband/an-enabled/2500base-x
[   14.631887] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=inband/2500base-x/none adv=00,00000000,00008000,0000e240 pause=04
[   14.633208] mvpp2 f2000000.ethernet eth1: pcs link up
[   14.633241] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, inactive
[   14.633262] mvpp2 f2000000.ethernet eth1: Link is Up - 2.5Gbps/Full - flow control off
[   60.713862] mvpp2 f2000000.ethernet eth1: pcs link down
[   60.713947] mvpp2 f2000000.ethernet eth1: deactivating EEE, was inactive
[   60.714978] mvpp2 f2000000.ethernet eth1: Link is Down
[   60.720200] mvpp2 f2000000.ethernet eth1: pcs link up
[   60.720586] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, inactive
[   60.720619] mvpp2 f2000000.ethernet eth1: Link is Up - 2.5Gbps/Full - flow control off
[   63.012413] mvpp2 f2000000.ethernet eth1: pcs link down
[   63.012444] mvpp2 f2000000.ethernet eth1: deactivating EEE, was inactive
[   63.013480] mvpp2 f2000000.ethernet eth1: Link is Down

Or is there a way to configure the firmware to use inband by default but
sill having a phydev device? 

For me it is not entirely clear which scenario should work for the
mxl-gpy in the end?

Scenario A:
2500 MBit/s -> PHY_INTERFACE_MODE_2500BASEX -> (0 no inband)
1000 MBit/s -> PHY_INTERFACE_MODE_SGMII -> LINK_INBAND_ENABLE
Then I would need a way to tell that the default mode is inband but that
there is still a phydev device available. Maybe this is possible and I
simply don't know how to do it?

Scenario B:
2500 MBit/s -> PHY_INTERFACE_MODE_2500BASEX -> LINK_INBAND_DISABLE
1000 MBit/s -> PHY_INTERFACE_MODE_1000BASEX -> LINK_INBAND_ENABLE
Then the phylink logic should change the firmwares mode depending on the
speed. Would this also work with 100MBit/s and 10MBit/s?

Scenario C (which I understand is not wanted):
2500 MBit/s -> PHY_INTERFACE_MODE_2500BASEX -> (0 no inband)
1000 MBit/s -> PHY_INTERFACE_MODE_SGMII -> (0 no inband)
This would already work but would require me to change the phy driver
and is not the desired behaviour anyways.

Sorry for my confusion, regards,
Stefan

Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

[Simon Horman]

On Tue, Apr 16, 2024 at 02:10:32PM +0200, Stefan Eichenberger wrote:
> Add a new device tree property to disable SGMII autonegotiation and
> instead use the option to match the SGMII speed to what was negotiated
> on the twisted pair interface (tpi). This allows us to disable SGMII
> autonegotiation on Ethernet controllers that are not compatible with
> this mode.
> 
> Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
> ---
>  drivers/net/phy/mxl-gpy.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> index b2d36a3a96f1..4147b4c29eaf 100644
> --- a/drivers/net/phy/mxl-gpy.c
> +++ b/drivers/net/phy/mxl-gpy.c
> @@ -114,6 +114,7 @@ struct gpy_priv {
>  	 * is enabled.
>  	 */
>  	u64 lb_dis_to;
> +	bool sgmii_match_tpi_speed;
>  };
>  
>  static const struct {
> @@ -262,8 +263,17 @@ static int gpy_mbox_read(struct phy_device *phydev, u32 addr)
>  
>  static int gpy_config_init(struct phy_device *phydev)
>  {
> +	struct gpy_priv *priv = phydev->priv;
>  	int ret;
>  
> +	/* Disalbe SGMII Autoneg if we want to match SGMII to TPI speed */

nit: Disable