[PATCH net-next 0/6] net: phy: dp83869: Add support for downstream SFP cages

[PATCH net-next 0/6] net: phy: dp83869: Add support for downstream SFP cages

[Romain Gantois]

Hello everyone,

This is version one of my series that adds support for downstream SFP cages
to the DP83869 PHY driver.

The DP83869HM PHY transceiver supports a variety of different conversion
modes, including RGMII to SGMII and RGMII to 1000Base-X. With these last
two modes, the DP83869 can act as a bridge between an Ethernet MAC and an SFP
cage in one of the following two configurations:

RGMII-SGMII bridge:

+-------+       +------------+        +-----------+--------------------+
|       |RGMII  |            | SGMII  |                                |
|  MAC  |<----->| DP83869HM  |<------>| SFP cage with SGMII SFP module |
|       |       |            |        |                                |
+-------+       +------------+        +-----------+--------------------+

RGMII-1000Base-X converter:

+-------+        +------------+1000Base-X+-----------+--------------------+
|       |RGMII   |            |          |                                |
|  MAC  |<------>| DP83869HM  |<-------->| SFP cage with DAC/fiber module |
|       |        |            |          |                                |
+-------+        +------------+          +-----------+--------------------+

The RGMII-SGMII and RGMII-1000Base-X are currently supported in the PHY
driver, but there are some flaws that prevent proper operation with
downstream SFP modules. Additionally, the sfp_upstream_ops callbacks which
are needed to interact with the downstream SFP modules are not yet
implemented.

This series adds full support for both SGMII and DAC SFP modules by
implementing these sfp_upstream_ops callbacks and fixing relevant issues in
the existing DP83869 operational modes.

Best Regards,

Romain

Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
Romain Gantois (6):
      net: phy: dp83869: Disable autonegotiation in RGMII/1000Base-X mode
      net: phy: dp83869: Perform software restart after configuring op mode
      net: phy: dp83869: Ensure that the FORCE_LINK_GOOD bit is cleared
      net: phy: dp83869: Support 1000Base-X and 100Base-FX SFP modules
      net: phy: dp83869: Support SGMII SFP modules
      net: phy: dp83869: Fix link up reporting in SGMII bridge mode

 drivers/net/phy/dp83869.c | 269 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 233 insertions(+), 36 deletions(-)
---
base-commit: 30972a4ea092bacb9784fe251327571be6a99f9c
change-id: 20240628-b4-dp83869-sfp-8ea96b3f438c

Best regards,
-- 
Romain Gantois <romain.gantois@bootlin.com>

[PATCH net-next 1/6] net: phy: dp83869: Disable autonegotiation in RGMII/1000Base-X mode

[Romain Gantois]

Currently, the DP83869 driver only disables autonegotiation in fiber
configurations for 100Base-FX mode. However, the DP83869 PHY does not
support autonegotiation in any of its fiber modes.

Disable autonegotiation for all fiber modes.

Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
 drivers/net/phy/dp83869.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index d7aaefb5226b6..f6b05e3a3173e 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -647,6 +647,21 @@ static int dp83869_configure_fiber(struct phy_device *phydev,
 	linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported);
 	linkmode_set_bit(ADVERTISED_FIBRE, phydev->advertising);
 
+	/* Auto neg is not supported in 100/1000base FX modes */
+	bmcr = phy_read(phydev, MII_BMCR);
+	if (bmcr < 0)
+		return bmcr;
+
+	phydev->autoneg = AUTONEG_DISABLE;
+	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
+	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->advertising);
+
+	if (bmcr & BMCR_ANENABLE) {
+		ret =  phy_modify(phydev, MII_BMCR, BMCR_ANENABLE, 0);
+		if (ret < 0)
+			return ret;
+	}
+
 	if (dp83869->mode == DP83869_RGMII_1000_BASE) {
 		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
 				 phydev->supported);
@@ -655,21 +670,6 @@ static int dp83869_configure_fiber(struct phy_device *phydev,
 				 phydev->supported);
 		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseFX_Half_BIT,
 				 phydev->supported);
-
-		/* Auto neg is not supported in 100base FX mode */
-		bmcr = phy_read(phydev, MII_BMCR);
-		if (bmcr < 0)
-			return bmcr;
-
-		phydev->autoneg = AUTONEG_DISABLE;
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->advertising);
-
-		if (bmcr & BMCR_ANENABLE) {
-			ret =  phy_modify(phydev, MII_BMCR, BMCR_ANENABLE, 0);
-			if (ret < 0)
-				return ret;
-		}
 	}
 
 	/* Update advertising from supported */

-- 
2.45.2

Re: [PATCH net-next 1/6] net: phy: dp83869: Disable autonegotiation in RGMII/1000Base-X mode

[Andrew Lunn]

On Mon, Jul 01, 2024 at 10:51:03AM +0200, Romain Gantois wrote:
> Currently, the DP83869 driver only disables autonegotiation in fiber
> configurations for 100Base-FX mode. However, the DP83869 PHY does not
> support autonegotiation in any of its fiber modes.
> 
> Disable autonegotiation for all fiber modes.

I'm assuming to does work in copper mode?

    Andrew

Re: [PATCH net-next 1/6] net: phy: dp83869: Disable autonegotiation in RGMII/1000Base-X mode

[Romain Gantois]

On lundi 1 juillet 2024 18:40:24 UTC+2 Andrew Lunn wrote:
> On Mon, Jul 01, 2024 at 10:51:03AM +0200, Romain Gantois wrote:
> > Currently, the DP83869 driver only disables autonegotiation in fiber
> > configurations for 100Base-FX mode. However, the DP83869 PHY does not
> > support autonegotiation in any of its fiber modes.
> > 
> > Disable autonegotiation for all fiber modes.
> 
> I'm assuming to does work in copper mode?

I'm unable to test any of the copper modes for the DP83869HM but
according to the datasheet, autonegotiation should work in those.

To be clear, "fiber mode" in the DP83869 linguo also includes
1000Base-X which can be used with a direct-attach copper cable. From
what I've seen, autonegotiation is not supported in this configuration.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 1/6] net: phy: dp83869: Disable autonegotiation in RGMII/1000Base-X mode

[Russell King (Oracle)]

On Tue, Jul 02, 2024 at 10:44:12AM +0200, Romain Gantois wrote:
> To be clear, "fiber mode" in the DP83869 linguo also includes
> 1000Base-X which can be used with a direct-attach copper cable. From
> what I've seen, autonegotiation is not supported in this configuration.

Why? Direct-attached cables are 1000base-CX, which is defined by 802.3.
It uses the 1000base-X PCS which is shared with 1000base-SX, 1000base-LX
etc. If one looks at 37.1.3, Relationship to architectural layering,
or 36.1.5, Inter-sublayer interfaces, one can see in that diagram that
the PCS *including* auto-negotiation is included for SX, LX *and* CX.

Moreover, 39.3 states that TP1 and TP4 will be commin in many
implementations of LX, SX and CX.

Moreover, 39.1, 1000base-CX introduction, states that it incorporates
clause 36 and clause 38. Clause 36.2.2 states that the 1000base-X PCS
incorporates clause 37 auto-negotiation.

So, I think AN is supposed to be supported on CX in the same way that
it's supported for SX and LX.

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

Re: [PATCH net-next 1/6] net: phy: dp83869: Disable autonegotiation in RGMII/1000Base-X mode

[Romain Gantois]

Hello Russell,

On mardi 2 juillet 2024 11:24:18 UTC+2 Russell King (Oracle) wrote:
> On Tue, Jul 02, 2024 at 10:44:12AM +0200, Romain Gantois wrote:
> > To be clear, "fiber mode" in the DP83869 linguo also includes
> > 1000Base-X which can be used with a direct-attach copper cable. From
> > what I've seen, autonegotiation is not supported in this configuration.
> 
> Why? Direct-attached cables are 1000base-CX, which is defined by 802.3.
> It uses the 1000base-X PCS which is shared with 1000base-SX, 1000base-LX
> etc. If one looks at 37.1.3, Relationship to architectural layering,
> or 36.1.5, Inter-sublayer interfaces, one can see in that diagram that
> the PCS *including* auto-negotiation is included for SX, LX *and* CX.
> 
> Moreover, 39.3 states that TP1 and TP4 will be commin in many
> implementations of LX, SX and CX.
> 
> Moreover, 39.1, 1000base-CX introduction, states that it incorporates
> clause 36 and clause 38. Clause 36.2.2 states that the 1000base-X PCS
> incorporates clause 37 auto-negotiation.
> 
> So, I think AN is supposed to be supported on CX in the same way that
> it's supported for SX and LX.

This seems to be a limitation of this particular PHY. From the DP83869
datasheet:

"7.4.2.1 1000BASE-X
The DP83869HM supports the 1000Base-X Fiber Ethernet protocol as
defined in IEEE 802.3 standard. In 1000M Fiber mode, the PHY will use
two differential channels for communication. In fiber mode, the speed is not
decided through auto-negotiation. Both sides of the link must be
configured to the same operating speed. The PHY can be configured to
operate in 1000BASE-X through the register settings (Section 7.4.8) or
strap settings (Section 7.5.1.2)."

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 1/6] net: phy: dp83869: Disable autonegotiation in RGMII/1000Base-X mode

[Russell King (Oracle)]

On Tue, Jul 02, 2024 at 11:42:04AM +0200, Romain Gantois wrote:
> Hello Russell,
> 
> This seems to be a limitation of this particular PHY. From the DP83869
> datasheet:
> 
> "7.4.2.1 1000BASE-X
> The DP83869HM supports the 1000Base-X Fiber Ethernet protocol as
> defined in IEEE 802.3 standard. In 1000M Fiber mode, the PHY will use
> two differential channels for communication. In fiber mode, the speed is not
> decided through auto-negotiation. Both sides of the link must be
> configured to the same operating speed. The PHY can be configured to
> operate in 1000BASE-X through the register settings (Section 7.4.8) or
> strap settings (Section 7.5.1.2)."

I think you grossly misunderstand 1000base-X there. You seem to be
equating auto-negotiation with negotiation of speed. That isn't
necessarily the case.

Clause 37 auto-negotiation doesn't negotiate speed. It negotiates
other aspects of the link. See 37.2.1:

LSB                                                                      MSB

 D0     D1    D2   D3   D4    D5   D6   D7   D8   D9 D10 D11 D12 D13 D14 D15
rsvd rsvd rsvd rsvd rsvd      FD   HD PS1 PS2 rsvd rsvd rsvd RF1 RF2 Ack NP

FD/HD - full duplex/half duplex capability
PS1/PS2 - pause capabilties
RF1/RF2 - remote fault bits
Ack - Ack bit
NP - Next Page bit

So, just because the PHY documentation states that speed is not
negotiated, that doesn't mean that negotiation is not supported.
IEEE 802.3 *requires* AN be implemented.

Moreover, the clue is in the name - 1000base-X. The 1000 part. That
means it's a protocol operating at 1G speed, just the same as 1000base-T
which only operates at 1G speed.

BTW, with twisted pair, negotiation does include speed, and the result
of that is used to select between 1000base-T for 1G speeds, 100base-Tx
for 100M, and 10base-T for 10M - these each are separate protocols.
There is no 1000base-T operating at 100M or 10M speeds - that just
doesn't exist.

Hope this clears up the issue.

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

Re: [PATCH net-next 1/6] net: phy: dp83869: Disable autonegotiation in RGMII/1000Base-X mode

[Romain Gantois]

On mardi 2 juillet 2024 12:15:24 UTC+2 Russell King (Oracle) wrote:
> On Tue, Jul 02, 2024 at 11:42:04AM +0200, Romain Gantois wrote:
...
> So, just because the PHY documentation states that speed is not
> negotiated, that doesn't mean that negotiation is not supported.
> IEEE 802.3 *requires* AN be implemented.

Got it. Thanks for clarifying, I'll remove this patch from v2 and see
if I can get this autoneg to work.

Best Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net-next 2/6] net: phy: dp83869: Perform software restart after configuring op mode

[Romain Gantois]

The DP83869 PHY requires a software restart after OP_MODE is changed in the
OP_MODE_DECODE register.

Add this restart in dp83869_configure_mode().

Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
 drivers/net/phy/dp83869.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index f6b05e3a3173e..6bb9bb1c0e962 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -786,6 +786,10 @@ static int dp83869_configure_mode(struct phy_device *phydev,
 		return -EINVAL;
 	}
 
+	ret = phy_write(phydev, DP83869_CTRL, DP83869_SW_RESTART);
+
+	usleep_range(10, 20);
+
 	return ret;
 }
 

-- 
2.45.2

Re: [PATCH net-next 2/6] net: phy: dp83869: Perform software restart after configuring op mode

[Andrew Lunn]

On Mon, Jul 01, 2024 at 10:51:04AM +0200, Romain Gantois wrote:
> The DP83869 PHY requires a software restart after OP_MODE is changed in the
> OP_MODE_DECODE register.
> 
> Add this restart in dp83869_configure_mode().
> 
> Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
> ---
>  drivers/net/phy/dp83869.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
> index f6b05e3a3173e..6bb9bb1c0e962 100644
> --- a/drivers/net/phy/dp83869.c
> +++ b/drivers/net/phy/dp83869.c
> @@ -786,6 +786,10 @@ static int dp83869_configure_mode(struct phy_device *phydev,


Not directly this patch, but dp83869_configure_mode() has:

ret = phy_write(phydev, MII_BMCR, MII_DP83869_BMCR_DEFAULT);

where #define MII_DP83869_BMCR_DEFAULT	(BMCR_ANENABLE | \
					 BMCR_FULLDPLX | \
					 BMCR_SPEED1000)

When considering the previous patch, maybe BMCR_ANENABLE should be
conditional on the mode being selected?

	Andrew

Re: [PATCH net-next 2/6] net: phy: dp83869: Perform software restart after configuring op mode

[Romain Gantois]

On lundi 1 juillet 2024 18:44:33 UTC+2 Andrew Lunn wrote:
> On Mon, Jul 01, 2024 at 10:51:04AM +0200, Romain Gantois wrote:
> > The DP83869 PHY requires a software restart after OP_MODE is changed in
> > the
> > OP_MODE_DECODE register.
> > 
> > Add this restart in dp83869_configure_mode().
> > 
> > Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
> > ---
> > 
> >  drivers/net/phy/dp83869.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
> > index f6b05e3a3173e..6bb9bb1c0e962 100644
> > --- a/drivers/net/phy/dp83869.c
> > +++ b/drivers/net/phy/dp83869.c
> > @@ -786,6 +786,10 @@ static int dp83869_configure_mode(struct phy_device
> > *phydev,
> Not directly this patch, but dp83869_configure_mode() has:
> 
> ret = phy_write(phydev, MII_BMCR, MII_DP83869_BMCR_DEFAULT);
> 
> where #define MII_DP83869_BMCR_DEFAULT	(BMCR_ANENABLE | \
> 					 BMCR_FULLDPLX | \
> 					 BMCR_SPEED1000)
> 
> When considering the previous patch, maybe BMCR_ANENABLE should be
> conditional on the mode being selected?

Indeed, this would definitely make sense.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net-next 3/6] net: phy: dp83869: Ensure that the FORCE_LINK_GOOD bit is cleared

[Romain Gantois]

This bit is located in the PHY_CONTROL register and should be cleared on
reset. However, this is not always the case, which can cause unexpected
behavior such as link up being incorrectly reported as good when the
DP83869 is brought up directly in RGMII-SGMII bridge mode.

Make sure that this bit is cleared for all operational modes.

Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
 drivers/net/phy/dp83869.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 6bb9bb1c0e962..579cf6f84e030 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -713,17 +713,20 @@ static int dp83869_configure_mode(struct phy_device *phydev,
 	if (ret)
 		return ret;
 
-	phy_ctrl_val = (dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT |
+	/* The FORCE_LINK_GOOD bit 10 in the PHYCTRL register should be
+	 * unset after a hardware reset but it is not. make sure it is
+	 * cleared so that the PHY can function properly.
+	 * Also configure TX/RX FIFO depth for modes that require it.
+	 */
+	ret = phy_write(phydev, MII_DP83869_PHYCTRL,
+			dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT |
 			dp83869->tx_fifo_depth << DP83869_TX_FIFO_SHIFT |
 			DP83869_PHY_CTRL_DEFAULT);
+	if (ret)
+		return ret;
 
 	switch (dp83869->mode) {
 	case DP83869_RGMII_COPPER_ETHERNET:
-		ret = phy_write(phydev, MII_DP83869_PHYCTRL,
-				phy_ctrl_val);
-		if (ret)
-			return ret;
-
 		ret = phy_write(phydev, MII_CTRL1000, DP83869_CFG1_DEFAULT);
 		if (ret)
 			return ret;
@@ -746,28 +749,14 @@ static int dp83869_configure_mode(struct phy_device *phydev,
 
 		break;
 	case DP83869_1000M_MEDIA_CONVERT:
-		ret = phy_write(phydev, MII_DP83869_PHYCTRL,
-				phy_ctrl_val);
-		if (ret)
-			return ret;
-
 		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
 				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
 		if (ret)
 			return ret;
 		break;
 	case DP83869_100M_MEDIA_CONVERT:
-		ret = phy_write(phydev, MII_DP83869_PHYCTRL,
-				phy_ctrl_val);
-		if (ret)
-			return ret;
 		break;
 	case DP83869_SGMII_COPPER_ETHERNET:
-		ret = phy_write(phydev, MII_DP83869_PHYCTRL,
-				phy_ctrl_val);
-		if (ret)
-			return ret;
-
 		ret = phy_write(phydev, MII_CTRL1000, DP83869_CFG1_DEFAULT);
 		if (ret)
 			return ret;

-- 
2.45.2

[PATCH net-next 4/6] net: phy: dp83869: Support 1000Base-X and 100Base-FX SFP modules

[Romain Gantois]

The DP83869HM PHY transceiver can be configured in RGMII-1000Base-X mode to
interface an RGMII MAC with a copper direct-attach (DAC) or fiber SFP
module. SFP upstream ops are needed to notify the DP83869 driver of SFP
module insertions and let it configure the PHY appropriately.

Add relevant SFP callbacks to the DP83869 driver to support 1000Base-X and
100Base-FX modules.

Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
 drivers/net/phy/dp83869.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 579cf6f84e030..a3ccaad738b28 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -10,6 +10,8 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/phy.h>
+#include <linux/phylink.h>
+#include <linux/sfp.h>
 #include <linux/delay.h>
 #include <linux/bitfield.h>
 
@@ -843,6 +845,62 @@ static int dp83869_config_init(struct phy_device *phydev)
 	return ret;
 }
 
+static int dp83869_module_insert(void *upstream, const struct sfp_eeprom_id *id)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(phy_support);
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
+	DECLARE_PHY_INTERFACE_MASK(interfaces);
+	struct phy_device *phydev = upstream;
+	struct dp83869_private *dp83869;
+	phy_interface_t interface;
+
+	linkmode_zero(phy_support);
+	phylink_set(phy_support, 1000baseX_Full);
+	phylink_set(phy_support, 100baseFX_Full);
+	phylink_set(phy_support, FIBRE);
+
+	linkmode_zero(sfp_support);
+	sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces);
+
+	linkmode_and(sfp_support, phy_support, sfp_support);
+
+	if (linkmode_empty(sfp_support)) {
+		dev_err(&phydev->mdio.dev, "incompatible SFP module inserted\n");
+		return -EINVAL;
+	}
+
+	interface = sfp_select_interface(phydev->sfp_bus, sfp_support);
+
+	dev_info(&phydev->mdio.dev, "%s SFP compatible link mode: %s\n", __func__,
+		 phy_modes(interface));
+
+	dp83869 = phydev->priv;
+
+	switch (interface) {
+	case PHY_INTERFACE_MODE_100BASEX:
+		dp83869->mode = DP83869_RGMII_100_BASE;
+		phydev->port = PORT_FIBRE;
+		break;
+	case PHY_INTERFACE_MODE_1000BASEX:
+		dp83869->mode = DP83869_RGMII_1000_BASE;
+		phydev->port = PORT_FIBRE;
+		break;
+	default:
+		dev_err(&phydev->mdio.dev,
+			"incompatible PHY-to-SFP module link mode %s!\n",
+			phy_modes(interface));
+		return -EINVAL;
+	}
+
+	return dp83869_configure_mode(phydev, dp83869);
+}
+
+static const struct sfp_upstream_ops dp83869_sfp_ops = {
+	.attach = phy_sfp_attach,
+	.detach = phy_sfp_detach,
+	.module_insert = dp83869_module_insert,
+};
+
 static int dp83869_probe(struct phy_device *phydev)
 {
 	struct dp83869_private *dp83869;
@@ -859,6 +917,10 @@ static int dp83869_probe(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	ret = phy_sfp_probe(phydev, &dp83869_sfp_ops);
+	if (ret)
+		return ret;
+
 	if (dp83869->mode == DP83869_RGMII_100_BASE ||
 	    dp83869->mode == DP83869_RGMII_1000_BASE)
 		phydev->port = PORT_FIBRE;

-- 
2.45.2

Re: [PATCH net-next 4/6] net: phy: dp83869: Support 1000Base-X and 100Base-FX SFP modules

[Andrew Lunn]

> +	dev_info(&phydev->mdio.dev, "%s SFP compatible link mode: %s\n", __func__,
> +		 phy_modes(interface));

Maybe phydev_debug().

> +
> +	dp83869 = phydev->priv;
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_100BASEX:
> +		dp83869->mode = DP83869_RGMII_100_BASE;
> +		phydev->port = PORT_FIBRE;
> +		break;
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +		dp83869->mode = DP83869_RGMII_1000_BASE;
> +		phydev->port = PORT_FIBRE;
> +		break;
> +	default:
> +		dev_err(&phydev->mdio.dev,
> +			"incompatible PHY-to-SFP module link mode %s!\n",
> +			phy_modes(interface));

phydev_err()?


    Andrew

---
pw-bot: cr

[PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules

[Romain Gantois]

The DP83869HM PHY transceiver can be configured in RGMII-SGMII bridge mode
to interface an RGMII MAC with a standard SFP module containing an
integrated PHY. Additional SFP upstream ops are needed to notify the
DP83869 driver of SFP PHY detection so that it can initialize it.

Add relevant SFP callbacks to the DP83869 driver to support SGMII
modules.

Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
 drivers/net/phy/dp83869.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index a3ccaad738b28..a07ec1be84baf 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -153,6 +153,8 @@ struct dp83869_private {
 	bool rxctrl_strap_quirk;
 	int clk_output_sel;
 	int mode;
+	/* PHY in a downstream SFP module */
+	struct phy_device *mod_phy;
 };
 
 static int dp83869_read_status(struct phy_device *phydev)
@@ -845,6 +847,93 @@ static int dp83869_config_init(struct phy_device *phydev)
 	return ret;
 }
 
+static void dp83869_sfp_phy_change(struct phy_device *phydev, bool up)
+{
+	/* phylink uses this to populate its own structs from phy_device fields
+	 * we don't actually need this callback but if we don't implement it
+	 * the phy core will crash
+	 */
+}
+
+static int dp83869_connect_phy(void *upstream, struct phy_device *phy)
+{
+	struct phy_device *phydev = upstream;
+	struct dp83869_private *dp83869;
+
+	dp83869 = phydev->priv;
+
+	if (dp83869->mode != DP83869_RGMII_SGMII_BRIDGE)
+		return 0;
+
+	if (!phy->drv) {
+		dev_warn(&phy->mdio.dev, "No driver bound to SFP module phy!\n");
+		return 0;
+	}
+
+	phy_support_asym_pause(phy);
+	linkmode_set_bit(PHY_INTERFACE_MODE_SGMII, phy->host_interfaces);
+	phy->interface = PHY_INTERFACE_MODE_SGMII;
+	phy->port = PORT_TP;
+
+	phy->speed = SPEED_UNKNOWN;
+	phy->duplex = DUPLEX_UNKNOWN;
+	phy->pause = MLO_PAUSE_NONE;
+	phy->interrupts = PHY_INTERRUPT_DISABLED;
+	phy->irq = PHY_POLL;
+	phy->phy_link_change = &dp83869_sfp_phy_change;
+	phy->state = PHY_READY;
+
+	dp83869->mod_phy = phy;
+
+	return 0;
+}
+
+static void dp83869_disconnect_phy(void *upstream)
+{
+	struct phy_device *phydev = upstream;
+	struct dp83869_private *dp83869;
+
+	dp83869 = phydev->priv;
+	dp83869->mod_phy = NULL;
+}
+
+static int dp83869_module_start(void *upstream)
+{
+	struct phy_device *phydev = upstream;
+	struct dp83869_private *dp83869;
+	struct phy_device *mod_phy;
+	int ret;
+
+	dp83869 = phydev->priv;
+	mod_phy = dp83869->mod_phy;
+	if (!mod_phy)
+		return 0;
+
+	ret = phy_init_hw(mod_phy);
+	if (ret) {
+		dev_err(&mod_phy->mdio.dev, "Failed to initialize PHY hardware: error %d", ret);
+		return ret;
+	}
+
+	phy_start(mod_phy);
+
+	return 0;
+}
+
+static void dp83869_module_stop(void *upstream)
+{
+	struct phy_device *phydev = upstream;
+	struct dp83869_private *dp83869;
+	struct phy_device *mod_phy;
+
+	dp83869 = phydev->priv;
+	mod_phy = dp83869->mod_phy;
+	if (!mod_phy)
+		return;
+
+	phy_stop(mod_phy);
+}
+
 static int dp83869_module_insert(void *upstream, const struct sfp_eeprom_id *id)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(phy_support);
@@ -858,6 +947,13 @@ static int dp83869_module_insert(void *upstream, const struct sfp_eeprom_id *id)
 	phylink_set(phy_support, 1000baseX_Full);
 	phylink_set(phy_support, 100baseFX_Full);
 	phylink_set(phy_support, FIBRE);
+	phylink_set(phy_support, 10baseT_Full);
+	phylink_set(phy_support, 100baseT_Full);
+	phylink_set(phy_support, 1000baseT_Full);
+	phylink_set(phy_support, Autoneg);
+	phylink_set(phy_support, Pause);
+	phylink_set(phy_support, Asym_Pause);
+	phylink_set(phy_support, TP);
 
 	linkmode_zero(sfp_support);
 	sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces);
@@ -877,6 +973,10 @@ static int dp83869_module_insert(void *upstream, const struct sfp_eeprom_id *id)
 	dp83869 = phydev->priv;
 
 	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+		dp83869->mode = DP83869_RGMII_SGMII_BRIDGE;
+		phydev->port = PORT_TP;
+		break;
 	case PHY_INTERFACE_MODE_100BASEX:
 		dp83869->mode = DP83869_RGMII_100_BASE;
 		phydev->port = PORT_FIBRE;
@@ -899,6 +999,10 @@ static const struct sfp_upstream_ops dp83869_sfp_ops = {
 	.attach = phy_sfp_attach,
 	.detach = phy_sfp_detach,
 	.module_insert = dp83869_module_insert,
+	.module_start = dp83869_module_start,
+	.module_stop = dp83869_module_stop,
+	.connect_phy = dp83869_connect_phy,
+	.disconnect_phy = dp83869_disconnect_phy,
 };
 
 static int dp83869_probe(struct phy_device *phydev)

-- 
2.45.2

Re: [PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules

[Andrew Lunn]

> +static int dp83869_connect_phy(void *upstream, struct phy_device *phy)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct dp83869_private *dp83869;
> +
> +	dp83869 = phydev->priv;
> +
> +	if (dp83869->mode != DP83869_RGMII_SGMII_BRIDGE)
> +		return 0;
> +
> +	if (!phy->drv) {
> +		dev_warn(&phy->mdio.dev, "No driver bound to SFP module phy!\n");

more instances which could be phydev_{err|warn|info}().

> +		return 0;
> +	}
> +
> +	phy_support_asym_pause(phy);

That is unusual. This is normally used by a MAC driver to indicate it
supports asym pause. It is the MAC which implements pause, but the PHY
negotiates it. So this tells phylib what to advertise to the link
partner. Why would a PHY do this? What if the MAC does not support
asym pause?

> +	linkmode_set_bit(PHY_INTERFACE_MODE_SGMII, phy->host_interfaces);
> +	phy->interface = PHY_INTERFACE_MODE_SGMII;
> +	phy->port = PORT_TP;
> +
> +	phy->speed = SPEED_UNKNOWN;
> +	phy->duplex = DUPLEX_UNKNOWN;
> +	phy->pause = MLO_PAUSE_NONE;
> +	phy->interrupts = PHY_INTERRUPT_DISABLED;
> +	phy->irq = PHY_POLL;
> +	phy->phy_link_change = &dp83869_sfp_phy_change;
> +	phy->state = PHY_READY;

I don't know of any other PHY which messes with the state machine like
this. This needs some explanation.

> +
> +	dp83869->mod_phy = phy;
> +
> +	return 0;
> +}
> +
> +static void dp83869_disconnect_phy(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct dp83869_private *dp83869;
> +
> +	dp83869 = phydev->priv;
> +	dp83869->mod_phy = NULL;
> +}
> +
> +static int dp83869_module_start(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct dp83869_private *dp83869;
> +	struct phy_device *mod_phy;
> +	int ret;
> +
> +	dp83869 = phydev->priv;
> +	mod_phy = dp83869->mod_phy;
> +	if (!mod_phy)
> +		return 0;
> +
> +	ret = phy_init_hw(mod_phy);
> +	if (ret) {
> +		dev_err(&mod_phy->mdio.dev, "Failed to initialize PHY hardware: error %d", ret);
> +		return ret;
> +	}
> +
> +	phy_start(mod_phy);

Something else no other PHY driver does....

	Andrew

Re: [PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules

[Romain Gantois]

Hello Andrew, thanks for the review!

I think this particular patch warrants that I explain myself a bit more.

Some SGMII SFP modules will work fine once they're inserted and the appropriate 
probe() function has been called by the SFP PHY driver. However, this is not 
necessarily the case, as some SFP PHYs require further configuration before the 
link can be brought up (e.g. calling phy_init_hw() on them which will 
eventually call things like config_init()).

This configuration usually doesn't happen before the PHY device is attached to 
a network device. In this case, the DP83869 PHY is placed between the MAC and 
the SFP PHY. Thus, the DP83869 is attached to a network device while the SFP 
PHY is not. This means that the DP83869 driver basically takes on the role of 
the MAC driver in some aspects.

In this patch, I used the connect_phy() callback as a way to get a handle to 
the downstream SFP PHY. This callback is only implemented by phylink so far.

I used the module_start() callback to initialize the SFP PHY hardware and 
start it's state machine.

On lundi 1 juillet 2024 19:00:29 UTC+2 Andrew Lunn wrote:
> > +static int dp83869_connect_phy(void *upstream, struct phy_device *phy)
> > +{
> > +	struct phy_device *phydev = upstream;
> > +	struct dp83869_private *dp83869;
> > +
> > +	dp83869 = phydev->priv;
> > +
> > +	if (dp83869->mode != DP83869_RGMII_SGMII_BRIDGE)
> > +		return 0;
> > +
> > +	if (!phy->drv) {
> > +		dev_warn(&phy->mdio.dev, "No driver bound to SFP module phy!
\n");
> 
> more instances which could be phydev_{err|warn|info}().
> 
> > +		return 0;
> > +	}
> > +
> > +	phy_support_asym_pause(phy);
> 
> That is unusual. This is normally used by a MAC driver to indicate it
> supports asym pause. It is the MAC which implements pause, but the PHY
> negotiates it. So this tells phylib what to advertise to the link
> partner. Why would a PHY do this? What if the MAC does not support
> asym pause?
> 

The idea here was that the downstream SFP PHY should advertise pause 
capabilities if the upstream MAC and intermediate DP83869 PHY supported them. 
However, the way I implemented this is indeed flawed, since the DP83869 driver 
should check that the MAC wants to advertise pause capabilities before calling 
this on the downstream PHY.

I suggest the following logic instead:

if (pause bits are set in dp83869 advertising mask)
	copy pause bits from sfp_phy->supported to sfp_phy->advertising

> > +	linkmode_set_bit(PHY_INTERFACE_MODE_SGMII, phy->host_interfaces);
> > +	phy->interface = PHY_INTERFACE_MODE_SGMII;
> > +	phy->port = PORT_TP;
> > +
> > +	phy->speed = SPEED_UNKNOWN;
> > +	phy->duplex = DUPLEX_UNKNOWN;
> > +	phy->pause = MLO_PAUSE_NONE;
> > +	phy->interrupts = PHY_INTERRUPT_DISABLED;
> > +	phy->irq = PHY_POLL;
> > +	phy->phy_link_change = &dp83869_sfp_phy_change;
> > +	phy->state = PHY_READY;
> 
> I don't know of any other PHY which messes with the state machine like
> this. This needs some explanation.

phylink_sfp_connect_phy() does something similar. The reasoning behind setting 
PHY_READY is that the later call to phy_start() will fail if the PHY isn't in 
the PHY_READY or PHY_HALTED state.

No other PHY driver does this because as of now, phylink is the only implementer 
of the connect_phy() callback. Other PHY drivers don't seem to support handling 
the initial configuration of a downstream SFP PHY.

> 
> > +
> > +	dp83869->mod_phy = phy;
> > +
> > +	return 0;
> > +}
> > +
> > +static void dp83869_disconnect_phy(void *upstream)
> > +{
> > +	struct phy_device *phydev = upstream;
> > +	struct dp83869_private *dp83869;
> > +
> > +	dp83869 = phydev->priv;
> > +	dp83869->mod_phy = NULL;
> > +}
> > +
> > +static int dp83869_module_start(void *upstream)
> > +{
> > +	struct phy_device *phydev = upstream;
> > +	struct dp83869_private *dp83869;
> > +	struct phy_device *mod_phy;
> > +	int ret;
> > +
> > +	dp83869 = phydev->priv;
> > +	mod_phy = dp83869->mod_phy;
> > +	if (!mod_phy)
> > +		return 0;
> > +
> > +	ret = phy_init_hw(mod_phy);
> > +	if (ret) {
> > +		dev_err(&mod_phy->mdio.dev, "Failed to initialize PHY hardware: 
error
> > %d", ret); +		return ret;
> > +	}
> > +
> > +	phy_start(mod_phy);
> 
> Something else no other PHY driver does....

phy_init_hw() is necessary here to ensure that the SFP PHY is configured properly before 
the link is brought up. phy_start() is used to start the phylib state machine, which is what 
would happen if the SFP PHY was directly connected to a MAC.

As with the connect_phy() case, no other PHY  driver currently implements module_start(), 
only phylink does, which is why this code might look out of place.

Of course, there could be flaws in my understanding of phylib or the SFP core, please let 
me know what you think.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules

[Andrew Lunn]

On Tue, Jul 02, 2024 at 10:11:07AM +0200, Romain Gantois wrote:
> Hello Andrew, thanks for the review!
> 
> I think this particular patch warrants that I explain myself a bit more.
> 
> Some SGMII SFP modules will work fine once they're inserted and the appropriate 
> probe() function has been called by the SFP PHY driver. However, this is not 
> necessarily the case, as some SFP PHYs require further configuration before the 
> link can be brought up (e.g. calling phy_init_hw() on them which will 
> eventually call things like config_init()).
> 
> This configuration usually doesn't happen before the PHY device is attached to 
> a network device. In this case, the DP83869 PHY is placed between the MAC and 
> the SFP PHY. Thus, the DP83869 is attached to a network device while the SFP 
> PHY is not. This means that the DP83869 driver basically takes on the role of 
> the MAC driver in some aspects.
> 
> In this patch, I used the connect_phy() callback as a way to get a handle to 
> the downstream SFP PHY. This callback is only implemented by phylink so far.
> 
> I used the module_start() callback to initialize the SFP PHY hardware and 
> start it's state machine.

The SFP PHY is however a PHY which phylib is managing. And you have
phylink on top of that, which knows about both PHYs. Architecturally,
i really think phylink should be dealing with all this
configuration.

The MAC driver has told phylink its pause capabilities.
phylink_bringup_phy() will tell phylib these capabilities by calling
phy_support_asym_pause(). Why does this not work for the SFP PHY?

phylink knows when the SFP PHY is plugged in, and knows if the link is
admin up. It should be starting the state machine, not the PHY.

Do you have access to a macchiatobin? I suggest you play with one, see
how the marvell PHY driver works when you plug in a copper SFP module.

	Andrew

Re: [PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules

[Maxime Chevallier]

Hello Andrew,

On Tue, 2 Jul 2024 15:21:09 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Tue, Jul 02, 2024 at 10:11:07AM +0200, Romain Gantois wrote:
> > Hello Andrew, thanks for the review!
> > 
> > I think this particular patch warrants that I explain myself a bit more.
> > 
> > Some SGMII SFP modules will work fine once they're inserted and the appropriate 
> > probe() function has been called by the SFP PHY driver. However, this is not 
> > necessarily the case, as some SFP PHYs require further configuration before the 
> > link can be brought up (e.g. calling phy_init_hw() on them which will 
> > eventually call things like config_init()).
> > 
> > This configuration usually doesn't happen before the PHY device is attached to 
> > a network device. In this case, the DP83869 PHY is placed between the MAC and 
> > the SFP PHY. Thus, the DP83869 is attached to a network device while the SFP 
> > PHY is not. This means that the DP83869 driver basically takes on the role of 
> > the MAC driver in some aspects.
> > 
> > In this patch, I used the connect_phy() callback as a way to get a handle to 
> > the downstream SFP PHY. This callback is only implemented by phylink so far.
> > 
> > I used the module_start() callback to initialize the SFP PHY hardware and 
> > start it's state machine.  
> 
> The SFP PHY is however a PHY which phylib is managing. And you have
> phylink on top of that, which knows about both PHYs. Architecturally,
> i really think phylink should be dealing with all this
> configuration.
> 
> The MAC driver has told phylink its pause capabilities.
> phylink_bringup_phy() will tell phylib these capabilities by calling
> phy_support_asym_pause(). Why does this not work for the SFP PHY?
> 
> phylink knows when the SFP PHY is plugged in, and knows if the link is
> admin up. It should be starting the state machine, not the PHY.

When the SFP upstream is a PHY, phylink isn't involved in managing the
SFP PHY, only the sfp-bus.c code will, using the SFP upstream ops, for
which phylink is one possible provider. When phylink is the SFP
upstream, it does all the mod_phy handling, so other upstream_ops
providers should also do the same.

But I do agree with you in that this logic should not belong to any
specific PHY driver, and be made generic. phylink is one place to
implement that indeed, but so far phylink can't manage both PHYs on the
link (if I'm not mistaken). I don't know how this all fits in phylink
itself, or if this should rather be some phylib / sfp-bus helpers and
extra plumbing to ease writing phy drivers that do SFP and want to
better manage the module PHY.

If I'm not mistaken, Romain's setup uses a MAC that doesn't even use
phylink yet (but the porting guide was updated recently fortunately ;) )

> 
> Do you have access to a macchiatobin? I suggest you play with one, see
> how the marvell PHY driver works when you plug in a copper SFP module.

On my side I have access to one, but it's hard to tell as the 3310 only
really supports 10G copper SFPs so far and I only have one. However from
testing on other boards, it looks like when a PHY is acting as
upstream, copper SFP will work when their default behaviour is to start
autoneg, link establishment and so on automatically when inserted.

The behaviour is currently not consistent between systems that have
phylink as the SFP upstream (no media converter) and systems that have
a media converter.

I think Romain has tested the platform he's using for this series with
multiple copper SFPs, some will work fine, and some just don't. Same
copper SFP work just fine when used on systems that have a straight
MAC <-> SFP link managed by phylink.

Maxime

Re: [PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules

[Russell King (Oracle)]

On Tue, Jul 02, 2024 at 04:56:28PM +0200, Maxime Chevallier wrote:
> But I do agree with you in that this logic should not belong to any
> specific PHY driver, and be made generic. phylink is one place to
> implement that indeed, but so far phylink can't manage both PHYs on the
> link (if I'm not mistaken).

This is not a phylink problem, but a phy*lib* implementation problem.
It was decided that phy*lib* would take part in layering violations
with various functions called *direct* from the core networking layer
bypassing MAC drivers entirely.

Even with phy*link* that still happens - as soon as netdev->phydev
has been set, the networking core will forward PHY related calls
to that phydev bypassing the MAC driver and phylink.

If we want to start handling multiple layers of PHYs, then we have
to get rid of this layering bypass.

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

Re: [PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules

[Andrew Lunn]

On Tue, Jul 02, 2024 at 07:13:50PM +0100, Russell King (Oracle) wrote:
> On Tue, Jul 02, 2024 at 04:56:28PM +0200, Maxime Chevallier wrote:
> > But I do agree with you in that this logic should not belong to any
> > specific PHY driver, and be made generic. phylink is one place to
> > implement that indeed, but so far phylink can't manage both PHYs on the
> > link (if I'm not mistaken).
> 
> This is not a phylink problem, but a phy*lib* implementation problem.
> It was decided that phy*lib* would take part in layering violations
> with various functions called *direct* from the core networking layer
> bypassing MAC drivers entirely.
> 
> Even with phy*link* that still happens - as soon as netdev->phydev
> has been set, the networking core will forward PHY related calls
> to that phydev bypassing the MAC driver and phylink.
> 
> If we want to start handling multiple layers of PHYs, then we have
> to get rid of this layering bypass.

Or at least minimise them.

SQI values probably don't cause an issue in this situation, that seems
to be mostly automotive which is unlikely to have multiple PHYs.

link_down_events probably should be cleaned up and set as part of
ethtool_ops->get_link_ext_stats.

All the plca is more automotive, i doubt there will be two PHYs
involved there.

PHY tunabled we need the work bootlin is doing so we can direct to
towards a specific PHY. Going through the MAC does not help us.  The
same is true for PHY statistics.

There are no PHY drivers which implement module_info, so that bypass
can be deleted.

Work is being done on tsinfo, etc.

Cable test ideally wants to be run on the outer PHY, but again, the
bootlin work should solve this.

PSE and multiple PHYs also seems unlikely.

So i don't think the problem is too big.

	Andrew

Re: [PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules

[Russell King (Oracle)]

On Tue, Jul 02, 2024 at 03:21:09PM +0200, Andrew Lunn wrote:
> The SFP PHY is however a PHY which phylib is managing. And you have
> phylink on top of that, which knows about both PHYs. Architecturally,
> i really think phylink should be dealing with all this
> configuration.
> 
> The MAC driver has told phylink its pause capabilities.
> phylink_bringup_phy() will tell phylib these capabilities by calling
> phy_support_asym_pause(). Why does this not work for the SFP PHY?
> 
> phylink knows when the SFP PHY is plugged in, and knows if the link is
> admin up. It should be starting the state machine, not the PHY.

phylink only knows about SFPs that are directly connected to the
MAC/PCS. It has no knowledge of SFPs that are behind a PHY (like
on the Macchiatobin with 88x3310 PHYs.)

Due to the structure of the networking layer, I don't see how we
could sanely make stacked PHYs work - we expect the ethtool APIs
to target the media PHY, but in the case of a platform such as
Macchiatobin, we potentially have _two_ media facing PHYs on one
network interface. There's the 88x3310 which has its own RJ45
socket, and then if one plugs in a copper SFP, you get another
media-facing PHY with its own RJ45 socket. Which PHY should
ethtool ksettings_set interact with?

The ethtool API wasn't designed for this kind of thing!

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

[PATCH net-next 6/6] net: phy: dp83869: Fix link up reporting in SGMII bridge mode

[Romain Gantois]

In RGMII-SGMII bridge mode, the DP83869HM PHY can perform an SGMII
autonegotiation with a downstream link partner. In this operational mode,
the standard link and autonegotiation status bits do not report correct
values, the extended fiber status register must be checked.

Link parameters should theoretically be obtainable from the DP83869
registers after SGMII autonegotiation has completed. However, for unknown
reasons, this is not the case, and the link partner fiber capabilities
register will incorrectly report a remote fault even if the SGMII
autonegotiation was successful.

Modify the read_status() callback of the DP83869 driver to check the fiber
status register in RGMII-SGMII bridge mode. On link up, obtain the link
parameters from the downstream SFP PHY driver if possible. If not, set
speed and duplex to "unknown".

Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
 drivers/net/phy/dp83869.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index a07ec1be84baf..843af90667d41 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -43,6 +43,7 @@
 #define DP83869_IO_MUX_CFG	0x0170
 #define DP83869_OP_MODE		0x01df
 #define DP83869_FX_CTRL		0x0c00
+#define DP83869_FX_STS		0x0c01
 
 #define DP83869_SW_RESET	BIT(15)
 #define DP83869_SW_RESTART	BIT(14)
@@ -72,6 +73,10 @@
 /* This is the same bit mask as the BMCR so re-use the BMCR default */
 #define DP83869_FX_CTRL_DEFAULT	MII_DP83869_BMCR_DEFAULT
 
+/* FX_STS bits */
+#define DP83869_FX_LSTATUS         BIT(2)
+#define DP83869_FX_ANEGCOMPLETE    BIT(5)
+
 /* CFG1 bits */
 #define DP83869_CFG1_DEFAULT	(ADVERTISE_1000HALF | \
 				 ADVERTISE_1000FULL | \
@@ -160,7 +165,8 @@ struct dp83869_private {
 static int dp83869_read_status(struct phy_device *phydev)
 {
 	struct dp83869_private *dp83869 = phydev->priv;
-	int ret;
+	int ret, old_link = phydev->link;
+	u32 status;
 
 	ret = genphy_read_status(phydev);
 	if (ret)
@@ -176,6 +182,38 @@ static int dp83869_read_status(struct phy_device *phydev)
 		}
 	}
 
+	if (dp83869->mode == DP83869_RGMII_SGMII_BRIDGE) {
+		/* check if SGMII link is up */
+		status = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_FX_STS);
+
+		phydev->link = status & DP83869_FX_LSTATUS ? 1 : 0;
+		phydev->autoneg_complete = status & DP83869_FX_ANEGCOMPLETE ? 1 : 0;
+
+		if (!phydev->autoneg_complete) {
+			phydev->link = 0;
+		} else if (phydev->link && !old_link) {
+			/* It seems like link status and duplex resolved from
+			 * SGMII autonegotiation are incorrectly reported in
+			 * the fiber link partner capabilities register and in
+			 * the PHY status register. If there is a handle to the
+			 * downstream PHY, read link parameters from it. If
+			 * not, fallback to unknown.
+			 */
+
+			if (dp83869->mod_phy) {
+				ret = phy_read_status(dp83869->mod_phy);
+				if (ret)
+					return ret;
+
+				phydev->speed = dp83869->mod_phy->speed;
+				phydev->duplex = dp83869->mod_phy->duplex;
+			} else {
+				phydev->speed = SPEED_UNKNOWN;
+				phydev->duplex = DUPLEX_UNKNOWN;
+			}
+		}
+	}
+
 	return 0;
 }
 

-- 
2.45.2

Re: [PATCH net-next 6/6] net: phy: dp83869: Fix link up reporting in SGMII bridge mode

[Andrew Lunn]

> +			if (dp83869->mod_phy) {
> +				ret = phy_read_status(dp83869->mod_phy);
> +				if (ret)
> +					return ret;

Locking? When phylib does this in phy_check_link_status(), we have:

	lockdep_assert_held(&phydev->lock);

I don't see anything which takes the downstreams PHY lock.

Is this also introducing race conditions? What happens if the link
just went down? phy_check_link_status() takes actions. Will they still
happen when phylib next talks to the downstream PHY? It is probably
safer to call phy_check_link_status() than phy_read_status().

   Andrew

Re: [PATCH net-next 6/6] net: phy: dp83869: Fix link up reporting in SGMII bridge mode

[Romain Gantois]

On lundi 1 juillet 2024 19:09:40 UTC+2 Andrew Lunn wrote:
> > +			if (dp83869->mod_phy) {
> > +				ret = phy_read_status(dp83869->mod_phy);
> > +				if (ret)
> > +					return ret;
> 
> Locking? When phylib does this in phy_check_link_status(), we have:
> 
> 	lockdep_assert_held(&phydev->lock);
> 
> I don't see anything which takes the downstreams PHY lock.
> 
> Is this also introducing race conditions? What happens if the link
> just went down? phy_check_link_status() takes actions. Will they still
> happen when phylib next talks to the downstream PHY? It is probably
> safer to call phy_check_link_status() than phy_read_status().

Given that the phylib state machine will call phy_check_link_status() itself,
I think that this call to phy_read_status() could be dropped entirely and that
dp83869_read_status() could just directly read mod_phy->{link,speed,duplex}.

This raises the question of a potential race condition when reading
mod_phy->{link, speed, duplex}. I haven't seen any kind of locking used in
other parts of the net subsystem when reading these parameters.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 6/6] net: phy: dp83869: Fix link up reporting in SGMII bridge mode

[Russell King (Oracle)]

On Tue, Jul 02, 2024 at 11:04:37AM +0200, Romain Gantois wrote:
> This raises the question of a potential race condition when reading
> mod_phy->{link, speed, duplex}. I haven't seen any kind of locking used in
> other parts of the net subsystem when reading these parameters.

Drivers access these under phydev->lock due to a callback from phylib
which will be made from the state machine which holds that lock.
This includes phylink.

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