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

[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 0/3] net: phy: dp83869: Support 1000Base-X SFP

[Romain Gantois]

Hi everyone,

This series adds support for using the DP83869 PHY as a transceiver between an
RGMII upper MAC and a downstream 1000Base-X SFP module.

Patch 1 and 2 of the series are necessary to get the PHY to properly switch its
operating mode to RGMII<->1000Base-X when an SFP module is inserted.

Patch 3 adds the actual SFP support, with only 1000Base-X modules supported for
now.

Side note: A wider-scoped series adding general SFP support to this PHY was sent
some time ago, but was not pursued, mainly due to complications with SGMII
support:

https://lore.kernel.org/netdev/20250514-dp83869-1000basex-v1-0-1bdb3c9c3d63@bootlin.com/

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

 drivers/net/phy/dp83869.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)
---
base-commit: 32dbca2817e3611165636113f246bed30ef21b3d
change-id: 20251103-sfp-1000basex-a05c78484a54

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

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

[Romain Gantois]

The DP83869 PHY requires a software restart when the OP_MODE is changed.

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 1f381d7b13ff..fecacaa83b04 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -797,6 +797,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.51.2

[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 fecacaa83b04..adcd899472f2 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
 
@@ -809,6 +810,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.51.2

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

[Romain Gantois]

Associate with an SFP cage described in the device tree and provide the
module_insert() callback that will set the appropriate DP83869 operation
mode when an SFP module is inserted.

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

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index adcd899472f2..e279dfa268a4 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/sfp.h>
 #include <linux/delay.h>
 #include <linux/bitfield.h>
 
@@ -875,6 +876,77 @@ static int dp83869_config_init(struct phy_device *phydev)
 	return ret;
 }
 
+static void dp83869_module_remove(void *upstream)
+{
+	struct phy_device *phydev = upstream;
+
+	phydev_info(phydev, "SFP module removed\n");
+
+	/* Set speed and duplex to unknown to avoid downshifting warning. */
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+}
+
+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);
+	struct phy_device *phydev = upstream;
+	const struct sfp_module_caps *caps;
+	struct dp83869_private *dp83869;
+	phy_interface_t interface;
+	int ret;
+
+	linkmode_zero(phy_support);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, phy_support);
+
+	caps = sfp_get_module_caps(phydev->sfp_bus);
+
+	linkmode_and(sfp_support, phy_support, caps->link_modes);
+
+	if (linkmode_empty(sfp_support)) {
+		phydev_err(phydev, "incompatible SFP module inserted\n");
+		return -EINVAL;
+	}
+
+	interface = sfp_select_interface(phydev->sfp_bus, sfp_support);
+
+	phydev_info(phydev, "%s SFP compatible link mode: %s\n", __func__,
+		    phy_modes(interface));
+
+	dp83869 = phydev->priv;
+
+	switch (interface) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+		dp83869->mode = DP83869_RGMII_1000_BASE;
+		phydev->port = PORT_FIBRE;
+		break;
+	default:
+		phydev_err(phydev, "incompatible PHY-to-SFP module link mode %s!\n",
+			   phy_modes(interface));
+		return -EINVAL;
+	}
+
+	ret = dp83869_configure_mode(phydev, dp83869);
+	if (ret)
+		return ret;
+
+	/* Reconfigure advertisement */
+	if (mutex_trylock(&phydev->lock)) {
+		ret = dp83869_config_aneg(phydev);
+		mutex_unlock(&phydev->lock);
+	}
+
+	return ret;
+}
+
+static const struct sfp_upstream_ops dp83869_sfp_ops = {
+	.attach = phy_sfp_attach,
+	.detach = phy_sfp_detach,
+	.module_insert = dp83869_module_insert,
+	.module_remove = dp83869_module_remove,
+};
+
 static int dp83869_probe(struct phy_device *phydev)
 {
 	struct dp83869_private *dp83869;
@@ -891,6 +963,12 @@ static int dp83869_probe(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	if (of_property_read_bool(phydev->mdio.dev.of_node, "sfp")) {
+		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.51.2

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

[Russell King (Oracle)]

On Tue, Nov 04, 2025 at 09:50:34AM +0100, Romain Gantois wrote:
> The DP83869 PHY requires a software restart when the OP_MODE is changed.
> 
> 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 1f381d7b13ff..fecacaa83b04 100644
> --- a/drivers/net/phy/dp83869.c
> +++ b/drivers/net/phy/dp83869.c
> @@ -797,6 +797,10 @@ static int dp83869_configure_mode(struct phy_device *phydev,
>  		return -EINVAL;
>  	}
>  
> +	ret = phy_write(phydev, DP83869_CTRL, DP83869_SW_RESTART);

So if dp83869_configure_fiber() returns an error, that doesn't matter?
(This overwrites its error.)

-- 
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 3/3] net: phy: dp83869: Support 1000Base-X SFP

[Russell King (Oracle)]

On Tue, Nov 04, 2025 at 09:50:36AM +0100, Romain Gantois wrote:
> +static void dp83869_module_remove(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +
> +	phydev_info(phydev, "SFP module removed\n");
> +
> +	/* Set speed and duplex to unknown to avoid downshifting warning. */
> +	phydev->speed = SPEED_UNKNOWN;
> +	phydev->duplex = DUPLEX_UNKNOWN;

Should this be done by core phylib code?

> +}
> +
> +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);
> +	struct phy_device *phydev = upstream;
> +	const struct sfp_module_caps *caps;
> +	struct dp83869_private *dp83869;
> +	phy_interface_t interface;
> +	int ret;
> +
> +	linkmode_zero(phy_support);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, phy_support);
> +
> +	caps = sfp_get_module_caps(phydev->sfp_bus);
> +
> +	linkmode_and(sfp_support, phy_support, caps->link_modes);
> +
> +	if (linkmode_empty(sfp_support)) {
> +		phydev_err(phydev, "incompatible SFP module inserted\n");
> +		return -EINVAL;
> +	}
> +
> +	interface = sfp_select_interface(phydev->sfp_bus, sfp_support);
> +
> +	phydev_info(phydev, "%s SFP compatible link mode: %s\n", __func__,
> +		    phy_modes(interface));
> +
> +	dp83869 = phydev->priv;
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +		dp83869->mode = DP83869_RGMII_1000_BASE;
> +		phydev->port = PORT_FIBRE;
> +		break;
> +	default:
> +		phydev_err(phydev, "incompatible PHY-to-SFP module link mode %s!\n",
> +			   phy_modes(interface));
> +		return -EINVAL;
> +	}

If you only support 1000BASE-X, please test that in the interface mode
mask (caps->interfaces) rather than going round this long winded
method.

-- 
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/3] net: phy: dp83869: Restart PHY when configuring mode

[Romain Gantois]

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

Hi Russell,

On Tuesday, 4 November 2025 09:57:59 CET Russell King (Oracle) wrote:
> On Tue, Nov 04, 2025 at 09:50:34AM +0100, Romain Gantois wrote:
> > The DP83869 PHY requires a software restart when the OP_MODE is changed.
...
> > @@ -797,6 +797,10 @@ static int dp83869_configure_mode(struct phy_device
> > *phydev,> 
> >  		return -EINVAL;
> >  	
> >  	}
> > 
> > +	ret = phy_write(phydev, DP83869_CTRL, DP83869_SW_RESTART);
> 
> So if dp83869_configure_fiber() returns an error, that doesn't matter?
> (This overwrites its error.)

That's a blunder on my part, thanks for pointing it out.

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

[Romain Gantois]

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

On Tuesday, 4 November 2025 10:01:36 CET Russell King (Oracle) wrote:
> On Tue, Nov 04, 2025 at 09:50:36AM +0100, Romain Gantois wrote:
> > +static void dp83869_module_remove(void *upstream)
> > +{
> > +	struct phy_device *phydev = upstream;
> > +
> > +	phydev_info(phydev, "SFP module removed\n");
> > +
> > +	/* Set speed and duplex to unknown to avoid downshifting warning. */
> > +	phydev->speed = SPEED_UNKNOWN;
> > +	phydev->duplex = DUPLEX_UNKNOWN;
> 
> Should this be done by core phylib code?

I guess that enough PHY drivers do this by hand that a new phylib helper could 
be warranted. Maybe something like phy_clear_aneg_results(), which would set 
speed, duplex, pause and asym_pause to default values.

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

[Maxime Chevallier]

Hi,

On 04/11/2025 10:10, Romain Gantois wrote:
> On Tuesday, 4 November 2025 10:01:36 CET Russell King (Oracle) wrote:
>> On Tue, Nov 04, 2025 at 09:50:36AM +0100, Romain Gantois wrote:
>>> +static void dp83869_module_remove(void *upstream)
>>> +{
>>> +	struct phy_device *phydev = upstream;
>>> +
>>> +	phydev_info(phydev, "SFP module removed\n");
>>> +
>>> +	/* Set speed and duplex to unknown to avoid downshifting warning. */
>>> +	phydev->speed = SPEED_UNKNOWN;
>>> +	phydev->duplex = DUPLEX_UNKNOWN;
>>
>> Should this be done by core phylib code?
> 
> I guess that enough PHY drivers do this by hand that a new phylib helper could 
> be warranted. Maybe something like phy_clear_aneg_results(), which would set 
> speed, duplex, pause and asym_pause to default values.

Note that when phy_port eventually gets merged, we'll have a 
common .module_remove() for PHY SFP :

https://lore.kernel.org/netdev/20251013143146.364919-9-maxime.chevallier@bootlin.com/

We could definitely do that here :)

Maxime

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

[Romain Gantois]

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

On Tuesday, 4 November 2025 10:15:45 CET Maxime Chevallier wrote:
> Hi,
> 
> On 04/11/2025 10:10, Romain Gantois wrote:
> > On Tuesday, 4 November 2025 10:01:36 CET Russell King (Oracle) wrote:
> >> On Tue, Nov 04, 2025 at 09:50:36AM +0100, Romain Gantois wrote:
> >>> +static void dp83869_module_remove(void *upstream)
> >>> +{
> >>> +	struct phy_device *phydev = upstream;
> >>> +
> >>> +	phydev_info(phydev, "SFP module removed\n");
> >>> +
> >>> +	/* Set speed and duplex to unknown to avoid downshifting warning. */
> >>> +	phydev->speed = SPEED_UNKNOWN;
> >>> +	phydev->duplex = DUPLEX_UNKNOWN;
> >> 
> >> Should this be done by core phylib code?
> > 
> > I guess that enough PHY drivers do this by hand that a new phylib helper
> > could be warranted. Maybe something like phy_clear_aneg_results(), which
> > would set speed, duplex, pause and asym_pause to default values.
> 
> Note that when phy_port eventually gets merged, we'll have a
> common .module_remove() for PHY SFP :
> 
> https://lore.kernel.org/netdev/20251013143146.364919-9-maxime.chevallier@boo
> tlin.com/
> 
> We could definitely do that here :)

That would be even better!

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