diff mbox series

[2/2] net: phy: dp83869: Fix RGMII-SGMII and 1000BASE-X

Message ID 20240514122728.1490156-2-thomas.gessler@brueckmann-gmbh.de (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [1/2] net: phy: dp83869: Add PHY ID for chip revision 3 | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 925 this patch: 925
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 936 this patch: 936
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 936 this patch: 936
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 440 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Thomas Gessler May 14, 2024, 12:27 p.m. UTC
The PHY supports multiple modes of which not all are properly
implemented by the driver. In the case of the RGMII-to-SGMII and
1000BASE-X modes, this was primarily due to the use of non-standard
registers for auto-negotiation settings and link status. This patch adds
device-specific get_features(), config_aneg(), aneg_done(), and
read_status() functions for these modes. They are based on the genphy_*
versions with the correct registers and fall back to the genphy_*
versions for other modes.

The RGMII-to-SGMII mode is special, because the chip does not really act
as a PHY in this mode but rather as a bridge. It requires a connected
SGMII PHY and gets the negotiated speed and duplex from it through SGMII
auto-negotiation. To use the DP83869 as a virtual PHY, we assume that
the connected SGMII PHY supports 10/100/1000M half/full duplex and
therefore support and always advertise those settings.

Signed-off-by: Thomas Gessler <thomas.gessler@brueckmann-gmbh.de>
---
 drivers/net/phy/dp83869.c | 391 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 387 insertions(+), 4 deletions(-)

Comments

Andrew Lunn May 14, 2024, 12:56 p.m. UTC | #1
On Tue, May 14, 2024 at 02:27:28PM +0200, Thomas Gessler wrote:
> The PHY supports multiple modes of which not all are properly
> implemented by the driver. In the case of the RGMII-to-SGMII and
> 1000BASE-X modes, this was primarily due to the use of non-standard
> registers for auto-negotiation settings and link status. This patch adds
> device-specific get_features(), config_aneg(), aneg_done(), and
> read_status() functions for these modes. They are based on the genphy_*
> versions with the correct registers and fall back to the genphy_*
> versions for other modes.
> 
> The RGMII-to-SGMII mode is special, because the chip does not really act
> as a PHY in this mode but rather as a bridge.

It is known as a media converter. You see them used between an RGMII
port and an SFP cage. Is that your use case?

It requires a connected
> SGMII PHY and gets the negotiated speed and duplex from it through SGMII
> auto-negotiation. To use the DP83869 as a virtual PHY, we assume that
> the connected SGMII PHY supports 10/100/1000M half/full duplex and
> therefore support and always advertise those settings.

Not all copper SFP modules support auto-neg. This is all really messy
because there is no standardisation. Also 1000BaseT_Half is often not
supported.

	Andrew
Russell King (Oracle) May 14, 2024, 1:05 p.m. UTC | #2
On Tue, May 14, 2024 at 02:27:28PM +0200, Thomas Gessler wrote:
> The PHY supports multiple modes of which not all are properly
> implemented by the driver. In the case of the RGMII-to-SGMII and
> 1000BASE-X modes, this was primarily due to the use of non-standard
> registers for auto-negotiation settings and link status. This patch adds
> device-specific get_features(), config_aneg(), aneg_done(), and
> read_status() functions for these modes. They are based on the genphy_*
> versions with the correct registers and fall back to the genphy_*
> versions for other modes.

I'm reading this, and wondering... do you have a use case for this,
or are you adding it because "the PHY supports this" ?

> The RGMII-to-SGMII mode is special, because the chip does not really act
> as a PHY in this mode but rather as a bridge. It requires a connected
> SGMII PHY and gets the negotiated speed and duplex from it through SGMII
> auto-negotiation. To use the DP83869 as a virtual PHY, we assume that
> the connected SGMII PHY supports 10/100/1000M half/full duplex and
> therefore support and always advertise those settings.

I call this configuration a "stacked PHY" system, and you're right that
it's a setup that we have no support for at the moment. We assume that
there is exactly one PHY in each network device.

I think we would need a lot of re-architecting of the phylib <-> netdev
linkage to allow stacked PHY systems to work sensibly.

If you don't have a use case for this, then it would be better not to
add support for it at this stage, otherwise it may restrict what we can
do in the future when coming up with a solution for stacked PHY support.
Alternatively, you may wish to discuss this topic and work on a
solution.
Andrew Lunn May 14, 2024, 1:08 p.m. UTC | #3
> +/* FX_CTRL bits */
> +#define DP83869_CTRL0_SPEED_SEL_MSB		BIT(6)
> +#define DP83869_CTRL0_DUPLEX_MODE		BIT(8)
> +#define DP83869_CTRL0_RESTART_AN		BIT(9)
> +#define DP83869_CTRL0_ISOLATE			BIT(10)
> +#define DP83869_CTRL0_PWRDN			BIT(11)
> +#define DP83869_CTRL0_ANEG_EN			BIT(12)
> +#define DP83869_CTRL0_SPEED_SEL_LSB		BIT(13)
> +#define DP83869_CTRL0_LOOPBACK			BIT(14)

This looks like a standard BMCR. Please just use defines from mii.h,
since they are well known.

> +/* FX_STS bits */
> +#define DP83869_STTS_LINK_STATUS		BIT(2)
> +#define DP83869_STTS_ANEG_COMPLETE		BIT(5)

And these are standard BMCR bits.

> +
> +/* FX_ANADV bits */
> +#define DP83869_BP_FULL_DUPLEX			BIT(5)
> +#define DP83869_BP_HALF_DUPLEX			BIT(6)
> +#define DP83869_BP_PAUSE			BIT(7)
> +#define DP83869_BP_ASYMMETRIC_PAUSE		BIT(8)

ADVERTISE_1000XPSE_ASYN, ADVERTISE_1000XPAUSE, ...

Please go through all these defines and see what match to existing
ones.

	Andrew
Thomas Geßler May 15, 2024, 7:47 a.m. UTC | #4
On Tue, 14 May 2024, Andrew Lunn wrote:
> Please go through all these defines and see what match to existing
> ones.

Will do.

I defined them to make explicit that they are the correct bits for
registers with non-standard offsets that exist in addition to the usual
BMCR, BMSR, etc. (which are used only for other PHY modes). But since the
bits are identical, I guess this rather leads to duplication.

Thomas
Thomas Geßler May 15, 2024, 8:15 a.m. UTC | #5
On Tue, 14 May 2024, Andrew Lunn wrote:
> On Tue, May 14, 2024 at 02:27:28PM +0200, Thomas Gessler wrote:
> > The RGMII-to-SGMII mode is special, because the chip does not really act
> > as a PHY in this mode but rather as a bridge.
> 
> It is known as a media converter. You see them used between an RGMII
> port and an SFP cage. Is that your use case?

Basically. I would call this an RGMII-SGMII bridge. A "media converter" I
would call a device that changes the physical medium, like 1000BASE-T
copper/RJ45 to 1000BASE-X fiber/SFP.

We have this chip on a daughter card with exposed SGMII lines that can be
plugged into customer-specific motherboards. The motherboard will have
either an SGMII-copper PHY (this can also be a DP83869) with 10/100/1000
auto-neg enabled but without MDIO exposed to the CPU on the daughter card;
or an SFP cage. The SFP module, in turn, can be for 1000BASE-X fiber,
1000BASE-X-to-1000-BASE-T copper, or SGMII copper supporting 10/100/1000
auto-neg.

So I would like to support all those configurations, which can be done
with this chip.

> > SGMII PHY and gets the negotiated speed and duplex from it through SGMII
> > auto-negotiation. To use the DP83869 as a virtual PHY, we assume that
> > the connected SGMII PHY supports 10/100/1000M half/full duplex and
> > therefore support and always advertise those settings.
> 
> Not all copper SFP modules support auto-neg. This is all really messy
> because there is no standardisation. Also 1000BaseT_Half is often not
> supported.

I agree. Is there a better way to implement this use case? The problem
remains that in this mode the chip is not really a PHY, but rather a
bridge to an external PHY. See also Russell's e-mail.

I actually started out by NOT supporting or advertising any of the
10/100/1000BASE-T speeds when in RGMII-to-SGMII mode. This also works for
the SGMII auto-negotiation, since all it does is get the negotiated
speed/duplex from the connected PHY. However, this leads to a problem when
trying to disable auto-neg and force speed with ethtool.
phy_sanitize_settings() will then limit the speed to 10M because 100M and
1000M do not match any supported speeds.

Thomas
Thomas Geßler May 15, 2024, 8:45 a.m. UTC | #6
On Tue, 14 May 2024, Russell King (Oracle) wrote:
> On Tue, May 14, 2024 at 02:27:28PM +0200, Thomas Gessler wrote:
> > This patch adds
> > device-specific get_features(), config_aneg(), aneg_done(), and
> > read_status() functions for these modes. They are based on the genphy_*
> > versions with the correct registers and fall back to the genphy_*
> > versions for other modes.
> 
> I'm reading this, and wondering... do you have a use case for this,
> or are you adding it because "the PHY supports this" ?

We use this chip in both modes. The driver did not work for the 1000BASE-X
and RGMII-to-SGMII modes out of the box, so I started a thread in the TI
forum and tried to get some info there.

https://e2e.ti.com/support/interface-group/interface/f/interface-forum/1320180/dp83869hm-link-not-working-with-rgmii---sgmii-bridge---1000base-t-using-linux/

This led to the development of this patch, which makes the modes work for
our use cases.

> If you don't have a use case for this, then it would be better not to
> add support for it at this stage, otherwise it may restrict what we can
> do in the future when coming up with a solution for stacked PHY support.

I understand, it would be fine for me to leave this unmerged for now,
especially because of the unclear RGMII-to-SGMII situation, and continue
patching this locally. The only problem I see for other users is that the
driver ostensibly supports all modes the chip supports and can enable each
of them with device-tree settings. Several of the modes, however, can
simply not work because the driver accesses the wrong registers (BMCR/BMSR
instead of the specialized FX_CTRL/FX_STS). This is the main reason why
the custom config_aneg(), read_status() etc. are necessary.

Thomas
Andrew Lunn May 15, 2024, 3:10 p.m. UTC | #7
On Wed, May 15, 2024 at 10:15:33AM +0200, Thomas Geßler wrote:
> On Tue, 14 May 2024, Andrew Lunn wrote:
> > On Tue, May 14, 2024 at 02:27:28PM +0200, Thomas Gessler wrote:
> > > The RGMII-to-SGMII mode is special, because the chip does not really act
> > > as a PHY in this mode but rather as a bridge.
> > 
> > It is known as a media converter. You see them used between an RGMII
> > port and an SFP cage. Is that your use case?
> 
> Basically. I would call this an RGMII-SGMII bridge. A "media converter" I
> would call a device that changes the physical medium, like 1000BASE-T
> copper/RJ45 to 1000BASE-X fiber/SFP.
> 
> We have this chip on a daughter card with exposed SGMII lines that can be
> plugged into customer-specific motherboards. The motherboard will have
> either an SGMII-copper PHY (this can also be a DP83869) with 10/100/1000
> auto-neg enabled but without MDIO exposed to the CPU on the daughter card;
> or an SFP cage. The SFP module, in turn, can be for 1000BASE-X fiber,
> 1000BASE-X-to-1000-BASE-T copper, or SGMII copper supporting 10/100/1000
> auto-neg.

The SFP use case is probably not too hard to support. There are PHYs
drivers today which have the needed plumbing for this. Look at the
marvell10g driver, its mv3310_sfp_ops. qcom/qca807x.c: qca807x_sfp_ops
etc.

	Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index d248a13c1749..cc7a9889829e 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -42,6 +42,9 @@ 
 #define DP83869_IO_MUX_CFG	0x0170
 #define DP83869_OP_MODE		0x01df
 #define DP83869_FX_CTRL		0x0c00
+#define DP83869_FX_STS		0x0c01
+#define DP83869_FX_ANADV	0x0c04
+#define DP83869_FX_LPABL	0x0c05
 
 #define DP83869_SW_RESET	BIT(15)
 #define DP83869_SW_RESTART	BIT(14)
@@ -116,6 +119,39 @@ 
 #define DP83869_OP_MODE_MII			BIT(5)
 #define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
 
+/* FX_CTRL bits */
+#define DP83869_CTRL0_SPEED_SEL_MSB		BIT(6)
+#define DP83869_CTRL0_DUPLEX_MODE		BIT(8)
+#define DP83869_CTRL0_RESTART_AN		BIT(9)
+#define DP83869_CTRL0_ISOLATE			BIT(10)
+#define DP83869_CTRL0_PWRDN			BIT(11)
+#define DP83869_CTRL0_ANEG_EN			BIT(12)
+#define DP83869_CTRL0_SPEED_SEL_LSB		BIT(13)
+#define DP83869_CTRL0_LOOPBACK			BIT(14)
+
+/* FX_STS bits */
+#define DP83869_STTS_LINK_STATUS		BIT(2)
+#define DP83869_STTS_ANEG_COMPLETE		BIT(5)
+
+/* FX_ANADV bits */
+#define DP83869_BP_FULL_DUPLEX			BIT(5)
+#define DP83869_BP_HALF_DUPLEX			BIT(6)
+#define DP83869_BP_PAUSE			BIT(7)
+#define DP83869_BP_ASYMMETRIC_PAUSE		BIT(8)
+
+/* FX_LPABL bits
+ * Bits 12:10 for RGMII-to-SGMII mode are undocumented and were determined
+ * through tests. It appears that, in this mode, the tx_config_Reg defined in
+ * the SGMII spec is copied to FX_LPABL after SGMII auto-negotiation.
+ */
+#define DP83869_LP_ABILITY_FULL_DUPLEX		BIT(5)
+#define DP83869_LP_ABILITY_PAUSE		BIT(7)
+#define DP83869_LP_ABILITY_ASYMMETRIC_PAUSE	BIT(8)
+#define DP83869_LP_ABILITY_SGMII_100		BIT(10)
+#define DP83869_LP_ABILITY_SGMII_1000		BIT(11)
+#define DP83869_LP_ABILITY_SGMII_DUPLEX		BIT(12)
+#define DP83869_LP_ABILITY_ACK			BIT(14)
+
 /* RXFCFG bits*/
 #define DP83869_WOL_MAGIC_EN		BIT(0)
 #define DP83869_WOL_PATTERN_EN		BIT(1)
@@ -154,19 +190,319 @@  struct dp83869_private {
 	int mode;
 };
 
+static int dp83869_fx_setup_forced(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	u16 ctl = 0;
+
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	if (dp83869->mode == DP83869_RGMII_1000_BASE)
+		ctl |= DP83869_CTRL0_SPEED_SEL_MSB;
+
+	if (dp83869->mode == DP83869_RGMII_SGMII_BRIDGE) {
+		if (phydev->speed == SPEED_1000)
+			ctl |= DP83869_CTRL0_SPEED_SEL_MSB;
+		else if (phydev->speed == SPEED_100)
+			ctl |= DP83869_CTRL0_SPEED_SEL_LSB;
+
+		/* Contrary to the data sheet, there is NO need to clear
+		 * 10M_SGMII_RATE_ADAPT_DISABLE in 10M_SGMII_CFG for 10M SGMII.
+		 */
+	}
+
+	if (phydev->duplex == DUPLEX_FULL)
+		ctl |= DP83869_CTRL0_DUPLEX_MODE;
+
+	return phy_modify_mmd(phydev, DP83869_DEVADDR, DP83869_FX_CTRL,
+			      ~(u16)(DP83869_CTRL0_LOOPBACK |
+				      DP83869_CTRL0_ISOLATE |
+				      DP83869_CTRL0_PWRDN), ctl);
+}
+
+static int dp83869_fx_config_advert(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	int err, changed = 0;
+	u32 adv = 0;
+
+	if (dp83869->mode == DP83869_RGMII_1000_BASE) {
+		linkmode_and(phydev->advertising, phydev->advertising,
+			     phydev->supported);
+
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+				      phydev->advertising))
+			adv |= DP83869_BP_FULL_DUPLEX;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				      phydev->advertising))
+			adv |= DP83869_BP_PAUSE;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+				      phydev->advertising))
+			adv |= DP83869_BP_ASYMMETRIC_PAUSE;
+	}
+
+	if (dp83869->mode == DP83869_RGMII_SGMII_BRIDGE) {
+		/* As we cannot control the connected SGMII PHY, we force
+		 * advertising all speeds.
+		 */
+		linkmode_copy(phydev->advertising, phydev->supported);
+		adv |= DP83869_BP_HALF_DUPLEX;
+		adv |= DP83869_BP_FULL_DUPLEX;
+	}
+
+	err = phy_modify_mmd_changed(phydev, DP83869_DEVADDR, DP83869_FX_ANADV,
+				     DP83869_BP_HALF_DUPLEX |
+				     DP83869_BP_FULL_DUPLEX | DP83869_BP_PAUSE |
+				     DP83869_BP_ASYMMETRIC_PAUSE, adv);
+	if (err < 0)
+		return err;
+	if (err > 0)
+		changed = 1;
+
+	return changed;
+}
+
+static int dp83869_fx_restart_aneg(struct phy_device *phydev)
+{
+	return phy_modify_mmd(phydev, DP83869_DEVADDR, DP83869_FX_CTRL,
+			      DP83869_CTRL0_ISOLATE, DP83869_CTRL0_ANEG_EN |
+			      DP83869_CTRL0_RESTART_AN);
+}
+
+static int dp83869_fx_check_and_restart_aneg(struct phy_device *phydev,
+					     bool restart)
+{
+	int ret;
+
+	if (!restart) {
+		ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_FX_CTRL);
+		if (ret < 0)
+			return ret;
+
+		if (!(ret & DP83869_CTRL0_ANEG_EN) ||
+		    (ret & DP83869_CTRL0_ISOLATE))
+			restart = true;
+	}
+
+	if (restart)
+		return dp83869_fx_restart_aneg(phydev);
+
+	return 0;
+}
+
+static int dp83869_config_aneg(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	bool changed = false;
+	int err;
+
+	if (dp83869->mode != DP83869_RGMII_1000_BASE &&
+	    dp83869->mode != DP83869_RGMII_SGMII_BRIDGE)
+		return genphy_config_aneg(phydev);
+
+	if (phydev->autoneg != AUTONEG_ENABLE)
+		return dp83869_fx_setup_forced(phydev);
+
+	err = dp83869_fx_config_advert(phydev);
+	if (err < 0)
+		return err;
+	else if (err)
+		changed = true;
+
+	return dp83869_fx_check_and_restart_aneg(phydev, changed);
+}
+
+static int dp83869_aneg_done(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+
+	if (dp83869->mode == DP83869_RGMII_1000_BASE ||
+	    dp83869->mode == DP83869_RGMII_SGMII_BRIDGE) {
+		int retval = phy_read_mmd(phydev, DP83869_DEVADDR,
+					  DP83869_FX_STS);
+
+		return (retval < 0) ? retval :
+				      (retval & DP83869_STTS_ANEG_COMPLETE);
+	} else {
+		return genphy_aneg_done(phydev);
+	}
+}
+
+static int dp83869_fx_update_link(struct phy_device *phydev)
+{
+	int status = 0, fx_ctrl;
+
+	fx_ctrl = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_FX_CTRL);
+	if (fx_ctrl < 0)
+		return fx_ctrl;
+
+	if (fx_ctrl & DP83869_CTRL0_RESTART_AN)
+		goto done;
+
+	if (!phy_polling_mode(phydev) || !phydev->link) {
+		status = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_FX_STS);
+		if (status < 0)
+			return status;
+		else if (status & DP83869_STTS_LINK_STATUS)
+			goto done;
+	}
+
+	status = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_FX_STS);
+	if (status < 0)
+		return status;
+ done:
+	phydev->link = status & DP83869_STTS_LINK_STATUS ? 1 : 0;
+	phydev->autoneg_complete = status & DP83869_STTS_ANEG_COMPLETE ? 1 : 0;
+
+	if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete)
+		phydev->link = 0;
+
+	return 0;
+}
+
+static int dp83869_1000basex_read_lpa(struct phy_device *phydev)
+{
+	int fx_lpabl;
+
+	if (phydev->autoneg == AUTONEG_ENABLE) {
+		if (!phydev->autoneg_complete) {
+			mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
+							0);
+			mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
+			linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+					 phydev->lp_advertising, 0);
+			return 0;
+		}
+
+		fx_lpabl = phy_read_mmd(phydev, DP83869_DEVADDR,
+					DP83869_FX_LPABL);
+		if (fx_lpabl < 0)
+			return fx_lpabl;
+
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+				 phydev->lp_advertising,
+				 fx_lpabl & DP83869_LP_ABILITY_FULL_DUPLEX);
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				 phydev->lp_advertising,
+				 fx_lpabl & DP83869_LP_ABILITY_PAUSE);
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+				 phydev->lp_advertising,
+				 fx_lpabl &
+				 DP83869_LP_ABILITY_ASYMMETRIC_PAUSE);
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+				 phydev->lp_advertising,
+				 fx_lpabl & DP83869_LP_ABILITY_ACK);
+
+	} else {
+		linkmode_zero(phydev->lp_advertising);
+	}
+
+	return 0;
+}
+
+static int dp83869_fx_read_status_fixed(struct phy_device *phydev)
+{
+	int fx_ctrl = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_FX_CTRL);
+
+	if (fx_ctrl < 0)
+		return fx_ctrl;
+
+	if (fx_ctrl & DP83869_CTRL0_DUPLEX_MODE)
+		phydev->duplex = DUPLEX_FULL;
+	else
+		phydev->duplex = DUPLEX_HALF;
+
+	if (fx_ctrl & DP83869_CTRL0_SPEED_SEL_MSB)
+		phydev->speed = SPEED_1000;
+	else if (fx_ctrl & DP83869_CTRL0_SPEED_SEL_LSB)
+		phydev->speed = SPEED_100;
+	else
+		phydev->speed = SPEED_10;
+
+	return 0;
+}
+
+static int dp83869_fx_read_status(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	int err, old_link = phydev->link;
+
+	err = dp83869_fx_update_link(phydev);
+	if (err)
+		return err;
+
+	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+		return 0;
+
+	phydev->master_slave_get = MASTER_SLAVE_CFG_UNSUPPORTED;
+	phydev->master_slave_state = MASTER_SLAVE_STATE_UNSUPPORTED;
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	if (dp83869->mode == DP83869_RGMII_1000_BASE) {
+		err = dp83869_1000basex_read_lpa(phydev);
+		if (err < 0)
+			return err;
+
+		if (phydev->autoneg == AUTONEG_ENABLE &&
+		    phydev->autoneg_complete) {
+			phy_resolve_aneg_linkmode(phydev);
+		} else if (phydev->autoneg == AUTONEG_DISABLE) {
+			err = dp83869_fx_read_status_fixed(phydev);
+			if (err < 0)
+				return err;
+		}
+	} else if (dp83869->mode == DP83869_RGMII_SGMII_BRIDGE) {
+		if (phydev->autoneg == AUTONEG_ENABLE &&
+		    phydev->autoneg_complete) {
+			int fx_lpabl;
+
+			fx_lpabl = phy_read_mmd(phydev, DP83869_DEVADDR,
+						DP83869_FX_LPABL);
+			if (fx_lpabl < 0)
+				return fx_lpabl;
+
+			if (fx_lpabl & DP83869_LP_ABILITY_SGMII_1000)
+				phydev->speed = SPEED_1000;
+			else if (fx_lpabl & DP83869_LP_ABILITY_SGMII_100)
+				phydev->speed = SPEED_100;
+			else
+				phydev->speed = SPEED_10;
+
+			if (fx_lpabl & DP83869_LP_ABILITY_SGMII_DUPLEX)
+				phydev->duplex = DUPLEX_FULL;
+			else
+				phydev->duplex = DUPLEX_HALF;
+		} else if (phydev->autoneg == AUTONEG_DISABLE) {
+			err = dp83869_fx_read_status_fixed(phydev);
+			if (err < 0)
+				return err;
+		}
+	}
+
+	return 0;
+}
+
 static int dp83869_read_status(struct phy_device *phydev)
 {
 	struct dp83869_private *dp83869 = phydev->priv;
 	int ret;
 
-	ret = genphy_read_status(phydev);
+	if (dp83869->mode == DP83869_RGMII_1000_BASE ||
+	    dp83869->mode == DP83869_RGMII_SGMII_BRIDGE)
+		ret = dp83869_fx_read_status(phydev);
+	else
+		ret = genphy_read_status(phydev);
+
 	if (ret)
 		return ret;
 
-	if (linkmode_test_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported)) {
+	if (dp83869->mode == DP83869_RGMII_100_BASE) {
 		if (phydev->link) {
-			if (dp83869->mode == DP83869_RGMII_100_BASE)
-				phydev->speed = SPEED_100;
+			phydev->speed = SPEED_100;
 		} else {
 			phydev->speed = SPEED_UNKNOWN;
 			phydev->duplex = DUPLEX_UNKNOWN;
@@ -780,6 +1116,7 @@  static int dp83869_configure_mode(struct phy_device *phydev,
 
 		break;
 	case DP83869_RGMII_1000_BASE:
+		break;
 	case DP83869_RGMII_100_BASE:
 		ret = dp83869_configure_fiber(phydev, dp83869);
 		break;
@@ -874,6 +1211,49 @@  static int dp83869_probe(struct phy_device *phydev)
 	return dp83869_config_init(phydev);
 }
 
+static int dp83869_get_features(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	int err;
+
+	err = genphy_read_abilities(phydev);
+	if (err)
+		return err;
+
+	/* The PHY reports all speeds (10/100/1000BASE-T full/half-duplex and
+	 * 1000BASE-X) as supported independent of the selected mode. We clear
+	 * the settings that are nonsensical for each mode.
+	 */
+
+	if (dp83869->mode == DP83869_RGMII_SGMII_BRIDGE) {
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+				   phydev->supported);
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_TP_BIT, phydev->supported);
+	}
+
+	if (dp83869->mode == DP83869_RGMII_1000_BASE) {
+		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
+				 phydev->supported);
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_TP_BIT, phydev->supported);
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_MII_BIT,
+				   phydev->supported);
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
+				   phydev->supported);
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
+				   phydev->supported);
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
+				   phydev->supported);
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+				   phydev->supported);
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
+				   phydev->supported);
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+				   phydev->supported);
+	}
+
+	return 0;
+}
+
 static int dp83869_phy_reset(struct phy_device *phydev)
 {
 	int ret;
@@ -896,10 +1276,13 @@  static int dp83869_phy_reset(struct phy_device *phydev)
 	PHY_ID_MATCH_MODEL(_id),				\
 	.name		= (_name),				\
 	.probe          = dp83869_probe,			\
+	.get_features	= dp83869_get_features,			\
 	.config_init	= dp83869_config_init,			\
 	.soft_reset	= dp83869_phy_reset,			\
 	.config_intr	= dp83869_config_intr,			\
 	.handle_interrupt = dp83869_handle_interrupt,		\
+	.config_aneg	= dp83869_config_aneg,			\
+	.aneg_done	= dp83869_aneg_done,			\
 	.read_status	= dp83869_read_status,			\
 	.get_tunable	= dp83869_get_tunable,			\
 	.set_tunable	= dp83869_set_tunable,			\