[PATCH 1/2] net: phy: dp83869: Add PHY ID for chip revision 3

[PATCH 1/2] net: phy: dp83869: Add PHY ID for chip revision 3

[Thomas Gessler]

The recent silicon revision 3 of the DP83869 has a different PHY ID
which has to be added to the driver in order for the PHY to be detected.
There appear to be no documented differences between the revisions,
although there are some discussions in the TI forum about different
behavior for some registers.

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

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index d7aaefb5226b..d248a13c1749 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -15,7 +15,8 @@
 
 #include <dt-bindings/net/ti-dp83869.h>
 
-#define DP83869_PHY_ID		0x2000a0f1
+#define DP83869REV1_PHY_ID	0x2000a0f1
+#define DP83869REV3_PHY_ID	0x2000a0f3
 #define DP83561_PHY_ID		0x2000a1a4
 #define DP83869_DEVADDR		0x1f
 
@@ -909,14 +910,16 @@ static int dp83869_phy_reset(struct phy_device *phydev)
 }
 
 static struct phy_driver dp83869_driver[] = {
-	DP83869_PHY_DRIVER(DP83869_PHY_ID, "TI DP83869"),
+	DP83869_PHY_DRIVER(DP83869REV1_PHY_ID, "TI DP83869 Rev. 1"),
+	DP83869_PHY_DRIVER(DP83869REV3_PHY_ID, "TI DP83869 Rev. 3"),
 	DP83869_PHY_DRIVER(DP83561_PHY_ID, "TI DP83561-SP"),
 
 };
 module_phy_driver(dp83869_driver);
 
 static struct mdio_device_id __maybe_unused dp83869_tbl[] = {
-	{ PHY_ID_MATCH_MODEL(DP83869_PHY_ID) },
+	{ PHY_ID_MATCH_MODEL(DP83869REV1_PHY_ID) },
+	{ PHY_ID_MATCH_MODEL(DP83869REV3_PHY_ID) },
 	{ PHY_ID_MATCH_MODEL(DP83561_PHY_ID) },
 	{ }
 };
-- 
2.34.1

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

[Thomas Gessler]

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

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,			\
-- 
2.34.1

Re: [PATCH 1/2] net: phy: dp83869: Add PHY ID for chip revision 3

[Andrew Lunn]

On Tue, May 14, 2024 at 02:27:27PM +0200, Thomas Gessler wrote:
> The recent silicon revision 3 of the DP83869 has a different PHY ID
> which has to be added to the driver in order for the PHY to be detected.
> There appear to be no documented differences between the revisions,
> although there are some discussions in the TI forum about different
> behavior for some registers.

Do you have the datasheet? What does it say about the ID in registers
2 and 3? Often the lower nibble is the revision. Meaning there can be
16 revisions of a PHY.

>  static struct phy_driver dp83869_driver[] = {
> -	DP83869_PHY_DRIVER(DP83869_PHY_ID, "TI DP83869"),
> +	DP83869_PHY_DRIVER(DP83869REV1_PHY_ID, "TI DP83869 Rev. 1"),
> +	DP83869_PHY_DRIVER(DP83869REV3_PHY_ID, "TI DP83869 Rev. 3"),
>  	DP83869_PHY_DRIVER(DP83561_PHY_ID, "TI DP83561-SP"),

If you look at DP83869_PHY_DRIVER() it uses:

#define DP83869_PHY_DRIVER(_id, _name)                          \
{                                                               \
        PHY_ID_MATCH_MODEL(_id),                                \

As the name suggests, it matches on the model. The revision is
ignored. A mask is applied to ignore the lower nibble. So this change
looks pointless.

>  static struct mdio_device_id __maybe_unused dp83869_tbl[] = {
> -	{ PHY_ID_MATCH_MODEL(DP83869_PHY_ID) },
> +	{ PHY_ID_MATCH_MODEL(DP83869REV1_PHY_ID) },
> +	{ PHY_ID_MATCH_MODEL(DP83869REV3_PHY_ID) },
>  	{ PHY_ID_MATCH_MODEL(DP83561_PHY_ID) },

And this has the same issue. If you want the match to include the
revision, you need to use PHY_ID_MATCH_EXACT(). If the different
revisions are supposed to be compatible, a single PHY_ID_MATCH_MODEL()
is sufficient.

    Andrew

---
pw-bot: cr

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

[Andrew Lunn]

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

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

[Russell King (Oracle)]

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.

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

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

[Andrew Lunn]

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

Re: [PATCH 1/2] net: phy: dp83869: Add PHY ID for chip revision 3

[Thomas Geßler]

Hi Andrew,

On Tue, 14 May 2024, Andrew Lunn wrote:
> As the name suggests, it matches on the model. The revision is
> ignored. A mask is applied to ignore the lower nibble. So this change
> looks pointless.

Ah, I see. I did not realize that the match ignores the lower bits. I was
having trouble getting the driver to match when first experimenting with
the chip and thought this was part of the problem. As it turns out, it now
works without this patch.

Please disregard it.

Thomas

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

[Thomas Geßler]

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

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

[Thomas Geßler]

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

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

[Thomas Geßler]

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

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

[Andrew Lunn]

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