[PATCH net-next 0/3] net: phy: dp83869: Support 1Gbps fiber SFP modules

[PATCH net-next 0/3] net: phy: dp83869: Support 1Gbps fiber SFP modules

[Romain Gantois]

Hello everyone,

This is version one of my series which adds support for downstream
1000Base-X SFP modules in the DP83869 PHY driver. It depends on the
following series from Maxime Chevallier, which introduces an ethernet port
representation and simplifies SFP support in PHY drivers:

https://lore.kernel.org/all/20250507135331.76021-1-maxime.chevallier@bootlin.com/

The DP83869 PHY supports a variety of different operational modes,
including RGMII-to-1000Base-X, which allows it to interface an
RGMII-capable MAC with a fiber SFP module.

RGMII-to-100Base-FX and RGMII-to-SGMII modes are also supported by the
DP83869, which makes it possible to support 100Mbps fiber modules and
copper modules. However, this series focuses on 1Gbps fiber modules as a
first step.

The first two patches in this series do some preliminary groundwork so that
the DP83869 can function properly in RGMII-to-1000Base-X mode. These
definitely toe the line between fixes and new features, but since the
targeted behaviors have never been implemented in the DP83869 driver, I've
decided to include them here. Please let me know if you disagree with this.

Best Regards,

Romain

Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
Romain Gantois (3):
      net: phy: dp83869: Restart PHY when configuring mode
      net: phy: dp83869: ensure FORCE_LINK_GOOD is cleared
      net: phy: dp83869: Support 1000Base-X SFP modules

 drivers/net/phy/dp83869.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)
---
base-commit: 5b6d59102462301b7ad256b1213964044d3fb50f
change-id: 20250513-dp83869-1000basex-4d4045aa416c

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

[PATCH net-next 1/3] net: phy: dp83869: Restart PHY when configuring mode

[Romain Gantois]

According to the DP83869 PHY datasheet, a software restart is required at
the end of every operational mode configuration. This resets all of the
PHY's circuits except the registers in the register file.

The DP83869 driver currently does not perform this restart operation, which
could theoretically cause issues if the PHY is in an intermediary state
when the operational mode is changed.

Add this software restart operation to 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 a62cd838a9eacc9edb0f472470a63079b6b72207..010434c94e01f44ac3c0b7e147468f4f7dca33f4 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -798,6 +798,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.49.0

[PATCH net-next 2/3] net: phy: dp83869: ensure FORCE_LINK_GOOD is cleared

[Romain Gantois]

The FORCE_LINK_GOOD bit in the PHY_CONTROL register forces the reported
link status to 1 if the selected speed is 1Gbps.

According to the DP83869 PHY datasheet, this bit should default to 0 after
a hardware reset. However, the opposite has been observed on some DP83869
components.

As a consequence, a valid link will be reported in 1000Base-X operational
modes, even if the autonegotiation process failed.

Make sure that the FORCE_LINK_GOOD bit is cleared during initial
configuration.

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

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 010434c94e01f44ac3c0b7e147468f4f7dca33f4..000660aae16ed46166774e7235cd8a6df94be047 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -89,6 +89,7 @@
 #define DP83869_STRAP_MIRROR_ENABLED           BIT(12)
 
 /* PHYCTRL bits */
+#define DP83869_FORCE_LINK_GOOD	BIT(10)
 #define DP83869_RX_FIFO_SHIFT	12
 #define DP83869_TX_FIFO_SHIFT	14
 
@@ -810,6 +811,15 @@ static int dp83869_config_init(struct phy_device *phydev)
 	struct dp83869_private *dp83869 = phydev->priv;
 	int ret, val;
 
+	/* The FORCE_LINK_GOOD bit 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.
+	 */
+	ret = phy_clear_bits(phydev, MII_DP83869_PHYCTRL,
+			     DP83869_FORCE_LINK_GOOD);
+	if (ret)
+		return ret;
+
 	/* Force speed optimization for the PHY even if it strapped */
 	ret = phy_modify(phydev, DP83869_CFG2, DP83869_DOWNSHIFT_EN,
 			 DP83869_DOWNSHIFT_EN);

-- 
2.49.0

[PATCH net-next 3/3] net: phy: dp83869: Support 1000Base-X SFP modules

[Romain Gantois]

The DP83869 PHY supports multiple operational modes, including
RGMII-to-1000Base-X, which can be used to link an RGMII-capable Ethernet
MAC to a downstream fiber SFP module.

+-------+          +---------+            +-----------------+
|       |  RGMII   |         | 1000Base-X |                 |
|  MAC  |<-------> | DP83869 |<---------->| fiber SFP module|
|       |          |         |            |                 |
+-------+          +---------+            +-----------------+

Register the attach_port() callback, to set the supported downstream
interface modes for SFP ports and provide the configure_mii() callback,
which sets the correct DP83869 operational mode when a compatible SFP
module is inserted.

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

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 000660aae16ed46166774e7235cd8a6df94be047..6d43c39ac525714a495327ec5a1a22b5e653b1cd 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/phy.h>
+#include <linux/phy_port.h>
 #include <linux/delay.h>
 #include <linux/bitfield.h>
 
@@ -876,6 +877,57 @@ static int dp83869_config_init(struct phy_device *phydev)
 	return ret;
 }
 
+static int dp83869_port_configure_serdes(struct phy_port *port, bool enable,
+					 phy_interface_t interface)
+{
+	struct phy_device *phydev = port_phydev(port);
+	struct dp83869_private *dp83869;
+	int ret;
+
+	if (!enable)
+		return 0;
+
+	dp83869 = phydev->priv;
+
+	switch (interface) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+		dp83869->mode = DP83869_RGMII_1000_BASE;
+		break;
+	default:
+		phydev_err(phydev, "Incompatible SFP module inserted\n");
+		return -EINVAL;
+	}
+
+	ret = dp83869_configure_mode(phydev, dp83869);
+	if (ret)
+		return ret;
+
+	/* Update advertisement */
+	if (mutex_trylock(&phydev->lock)) {
+		ret = dp83869_config_aneg(phydev);
+		mutex_unlock(&phydev->lock);
+	}
+
+	return ret;
+}
+
+static const struct phy_port_ops dp83869_serdes_port_ops = {
+	.configure_mii = dp83869_port_configure_serdes,
+};
+
+static int dp83869_attach_port(struct phy_device *phydev,
+			       struct phy_port *port)
+{
+	if (!port->is_serdes)
+		return 0;
+
+	port->ops = &dp83869_serdes_port_ops;
+
+	__set_bit(PHY_INTERFACE_MODE_1000BASEX, port->interfaces);
+
+	return 0;
+}
+
 static int dp83869_probe(struct phy_device *phydev)
 {
 	struct dp83869_private *dp83869;
@@ -931,6 +983,7 @@ static int dp83869_phy_reset(struct phy_device *phydev)
 	.set_tunable	= dp83869_set_tunable,			\
 	.get_wol	= dp83869_get_wol,			\
 	.set_wol	= dp83869_set_wol,			\
+	.attach_port    = dp83869_attach_port,                  \
 	.suspend	= genphy_suspend,			\
 	.resume		= genphy_resume,			\
 }

-- 
2.49.0

Re: [PATCH net-next 3/3] net: phy: dp83869: Support 1000Base-X SFP modules

[Antoine Tenart]

On Wed, May 14, 2025 at 09:49:59AM +0200, Romain Gantois wrote:
>  
> +static int dp83869_port_configure_serdes(struct phy_port *port, bool enable,
> +					 phy_interface_t interface)
> +{
> +	struct phy_device *phydev = port_phydev(port);
> +	struct dp83869_private *dp83869;
> +	int ret;
> +
> +	if (!enable)
> +		return 0;
> +
> +	dp83869 = phydev->priv;
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +		dp83869->mode = DP83869_RGMII_1000_BASE;
> +		break;
> +	default:
> +		phydev_err(phydev, "Incompatible SFP module inserted\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = dp83869_configure_mode(phydev, dp83869);
> +	if (ret)
> +		return ret;
> +
> +	/* Update advertisement */
> +	if (mutex_trylock(&phydev->lock)) {
> +		ret = dp83869_config_aneg(phydev);
> +		mutex_unlock(&phydev->lock);
> +	}

Just skimmed through this quickly and it's not clear to me why aneg is
restarted only if there was no contention on the global phydev lock;
it's not guaranteed a concurrent holder would do the same. If this is
intended, a comment would be welcomed.

Thanks!

Re: [PATCH net-next 3/3] net: phy: dp83869: Support 1000Base-X SFP modules

[Romain Gantois]

[-- Attachment #1: Type: text/plain, Size: 2219 bytes --]

On Wednesday, 14 May 2025 11:01:07 CEST Antoine Tenart wrote:
> On Wed, May 14, 2025 at 09:49:59AM +0200, Romain Gantois wrote:
> > +static int dp83869_port_configure_serdes(struct phy_port *port, bool
> > enable, +					 phy_interface_t interface)
> > +{
> > +	struct phy_device *phydev = port_phydev(port);
> > +	struct dp83869_private *dp83869;
> > +	int ret;
> > +
> > +	if (!enable)
> > +		return 0;
> > +
> > +	dp83869 = phydev->priv;
> > +
> > +	switch (interface) {
> > +	case PHY_INTERFACE_MODE_1000BASEX:
> > +		dp83869->mode = DP83869_RGMII_1000_BASE;
> > +		break;
> > +	default:
> > +		phydev_err(phydev, "Incompatible SFP module inserted\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = dp83869_configure_mode(phydev, dp83869);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Update advertisement */
> > +	if (mutex_trylock(&phydev->lock)) {
> > +		ret = dp83869_config_aneg(phydev);
> > +		mutex_unlock(&phydev->lock);
> > +	}
> 
> Just skimmed through this quickly and it's not clear to me why aneg is
> restarted only if there was no contention on the global phydev lock;
> it's not guaranteed a concurrent holder would do the same. If this is
> intended, a comment would be welcomed.

The reasoning here is that there are code paths which call 
dp83869_port_configure_serdes() with phydev->lock already held, for example:

phy_start() -> sfp_upstream_start() -> sfp_start() -> \
	sfp_sm_event() -> __sfp_sm_event() -> sfp_sm_module() -> \ 
	sfp_module_insert() -> phy_sfp_module_insert() -> \
	dp83869_port_configure_serdes()

so taking this lock could result in a deadlock.

mutex_trylock() is definitely not a perfect solution though, but I went with it
partly because the marvell-88x2222 driver already does it this way, and partly 
because if phydev->lock() is held, then there's a solid chance that the phy 
state machine is already taking care of reconfiguring the advertisement. 
However, I'll admit that this is a bit of a shaky argument.

If someone has a better solution in mind, I'll gladly hear it out, but for now 
I guess I'll just add a comment explaining why trylock() is being used.

Thanks!

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next 0/3] net: phy: dp83869: Support 1Gbps fiber SFP modules

[Maxime Chevallier]

Hi Romain,

On Wed, 14 May 2025 09:49:56 +0200
Romain Gantois <romain.gantois@bootlin.com> wrote:

> Hello everyone,
> 
> This is version one of my series which adds support for downstream
> 1000Base-X SFP modules in the DP83869 PHY driver. It depends on the
> following series from Maxime Chevallier, which introduces an ethernet port
> representation and simplifies SFP support in PHY drivers:
> 
> https://lore.kernel.org/all/20250507135331.76021-1-maxime.chevallier@bootlin.com/

Thanks a lot for giving this work a shot ! Maybe a small nit, but as
the dependency isn't merged yet, you should mark this series as RFC, as
it will fail on the autobuilders.

I'll take a deeper look into that this week.

Maxime

Re: [PATCH net-next 3/3] net: phy: dp83869: Support 1000Base-X SFP modules

[Andrew Lunn]

> +static int dp83869_port_configure_serdes(struct phy_port *port, bool enable,
> +					 phy_interface_t interface)
> +{
> +	struct phy_device *phydev = port_phydev(port);
> +	struct dp83869_private *dp83869;
> +	int ret;
> +
> +	if (!enable)
> +		return 0;
> +
> +	dp83869 = phydev->priv;
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +		dp83869->mode = DP83869_RGMII_1000_BASE;
> +		break;
> +	default:
> +		phydev_err(phydev, "Incompatible SFP module inserted\n");
> +		return -EINVAL;
> +	}

There is also DP83869_RGMII_SGMII_BRIDGE. Can this be used with the
SERDES? Copper SFPs often want SGMII.

	Andrew

Re: [PATCH net-next 3/3] net: phy: dp83869: Support 1000Base-X SFP modules

[Romain Gantois]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

On Wednesday, 14 May 2025 14:22:48 CEST Andrew Lunn wrote:
> > +static int dp83869_port_configure_serdes(struct phy_port *port, bool
> > enable, +					 phy_interface_t interface)
> > +{
> > +	struct phy_device *phydev = port_phydev(port);
> > +	struct dp83869_private *dp83869;
> > +	int ret;
> > +
> > +	if (!enable)
> > +		return 0;
> > +
> > +	dp83869 = phydev->priv;
> > +
> > +	switch (interface) {
> > +	case PHY_INTERFACE_MODE_1000BASEX:
> > +		dp83869->mode = DP83869_RGMII_1000_BASE;
> > +		break;
> > +	default:
> > +		phydev_err(phydev, "Incompatible SFP module inserted\n");
> > +		return -EINVAL;
> > +	}
> 
> There is also DP83869_RGMII_SGMII_BRIDGE. Can this be used with the
> SERDES? Copper SFPs often want SGMII.

It can definitely be used to support non-DAC copper modules. In fact, I've 
implemented support for these modules locally, but I'm planning to upstream 
this part of the SFP support later, as there is some additional trickiness to 
solve beforehand.

To quickly summarize the issue, non-DAC copper module support requires reading 
autonegotiation results from the downstream PHY and relaying them back to the 
MAC. This requires some kind of stacked PHY support in the kernel, which has 
been in discussion for a while now:

https://lore.kernel.org/all/20241119115136.74297db7@fedora.home/

So as things currently stand, SGMII SFP support in this driver is blocked 
until some kind of generic stacked PHY support is implemented in the kernel.

Thanks,

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next 3/3] net: phy: dp83869: Support 1000Base-X SFP modules

[Andrew Lunn]

> > > +	/* Update advertisement */
> > > +	if (mutex_trylock(&phydev->lock)) {
> > > +		ret = dp83869_config_aneg(phydev);
> > > +		mutex_unlock(&phydev->lock);
> > > +	}
> > 
> > Just skimmed through this quickly and it's not clear to me why aneg is
> > restarted only if there was no contention on the global phydev lock;
> > it's not guaranteed a concurrent holder would do the same. If this is
> > intended, a comment would be welcomed.
> 
> The reasoning here is that there are code paths which call 
> dp83869_port_configure_serdes() with phydev->lock already held, for example:
> 
> phy_start() -> sfp_upstream_start() -> sfp_start() -> \
> 	sfp_sm_event() -> __sfp_sm_event() -> sfp_sm_module() -> \ 
> 	sfp_module_insert() -> phy_sfp_module_insert() -> \
> 	dp83869_port_configure_serdes()
> 
> so taking this lock could result in a deadlock.
> 
> mutex_trylock() is definitely not a perfect solution though, but I went with it
> partly because the marvell-88x2222 driver already does it this way, and partly 
> because if phydev->lock() is held, then there's a solid chance that the phy 
> state machine is already taking care of reconfiguring the advertisement. 
> However, I'll admit that this is a bit of a shaky argument.
> 
> If someone has a better solution in mind, I'll gladly hear it out, but for now 
> I guess I'll just add a comment explaining why trylock() is being used.

The marvell10g driver should be the reference to look at.

As you say, phy_start() will eventually get around to calling
dp83869_config_aneg(). What is more interesting here are the paths
which lead to this function which don't result in a call to
dp83869_config_aneg(). What are those?

	Andrew

Re: [PATCH net-next 3/3] net: phy: dp83869: Support 1000Base-X SFP modules

[Andrew Lunn]

> > There is also DP83869_RGMII_SGMII_BRIDGE. Can this be used with the
> > SERDES? Copper SFPs often want SGMII.
> 
> It can definitely be used to support non-DAC copper modules. In fact, I've 
> implemented support for these modules locally, but I'm planning to upstream 
> this part of the SFP support later, as there is some additional trickiness to 
> solve beforehand.

Ah, yes. Please add a comment to the commit message.

	Andrew

Re: [PATCH net-next 3/3] net: phy: dp83869: Support 1000Base-X SFP modules

[Romain Gantois]

[-- Attachment #1: Type: text/plain, Size: 2463 bytes --]

On Wednesday, 14 May 2025 14:32:22 CEST Andrew Lunn wrote:
> > > > +	/* Update advertisement */
> > > > +	if (mutex_trylock(&phydev->lock)) {
> > > > +		ret = dp83869_config_aneg(phydev);
> > > > +		mutex_unlock(&phydev->lock);
> > > > +	}
> > > 
> > > Just skimmed through this quickly and it's not clear to me why aneg is
> > > restarted only if there was no contention on the global phydev lock;
> > > it's not guaranteed a concurrent holder would do the same. If this is
> > > intended, a comment would be welcomed.
> > 
> > The reasoning here is that there are code paths which call
> > dp83869_port_configure_serdes() with phydev->lock already held, for
> > example:
> > 
> > phy_start() -> sfp_upstream_start() -> sfp_start() -> \
> > 
> > 	sfp_sm_event() -> __sfp_sm_event() -> sfp_sm_module() -> \
> > 	sfp_module_insert() -> phy_sfp_module_insert() -> \
> > 	dp83869_port_configure_serdes()
> > 
> > so taking this lock could result in a deadlock.
> > 
> > mutex_trylock() is definitely not a perfect solution though, but I went
> > with it partly because the marvell-88x2222 driver already does it this
> > way, and partly because if phydev->lock() is held, then there's a solid
> > chance that the phy state machine is already taking care of reconfiguring
> > the advertisement. However, I'll admit that this is a bit of a shaky
> > argument.
> > 
> > If someone has a better solution in mind, I'll gladly hear it out, but for
> > now I guess I'll just add a comment explaining why trylock() is being
> > used.
> The marvell10g driver should be the reference to look at.
> 
> As you say, phy_start() will eventually get around to calling
> dp83869_config_aneg(). What is more interesting here are the paths
> which lead to this function which don't result in a call to
> dp83869_config_aneg(). What are those?

Whenever you insert an SFP module, either sfp_irq() or sfp_poll() will will 
detect it and eventually call module_insert() from the SFP state machine. As 
far as I'm aware, this doesn't imply any calls to config_aneg().

Since the DP83869 remaps fiber registers to 0 when we switch it to RGMII-
to-1000Base-X mode, the value in MII_ADVERTISE won't be correct anymore,
which is why we have to reconfigure the advertisement after a module is 
inserted. It doesn't seem like the marvell10g driver does this, and I'm not 
sure why that's the case.

Thanks,

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next 0/3] net: phy: dp83869: Support 1Gbps fiber SFP modules

[Romain Gantois]

[-- Attachment #1: Type: text/plain, Size: 905 bytes --]

Hi Maxime,

On Wednesday, 14 May 2025 14:20:19 CEST Maxime Chevallier wrote:
> Hi Romain,
> 
> On Wed, 14 May 2025 09:49:56 +0200
> 
> Romain Gantois <romain.gantois@bootlin.com> wrote:
> > Hello everyone,
> > 
> > This is version one of my series which adds support for downstream
> > 1000Base-X SFP modules in the DP83869 PHY driver. It depends on the
> > following series from Maxime Chevallier, which introduces an ethernet port
> > representation and simplifies SFP support in PHY drivers:
> > 
> > https://lore.kernel.org/all/20250507135331.76021-1-maxime.chevallier@bootl
> > in.com/
> Thanks a lot for giving this work a shot ! Maybe a small nit, but as
> the dependency isn't merged yet, you should mark this series as RFC, as
> it will fail on the autobuilders.

Ah I see, I'll fix that in v2 then.

Thanks,

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]