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

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

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

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

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!

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

> > > +	/* 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

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