[PATCH v3] Add config phase for dp83td510e phy

[PATCH v3] Add config phase for dp83td510e phy

[Julien Blanc]

Add a config phase for the Texas Instruments DP83TD510E ethenet PHY

The config phase currently sets the following properties from the
device tree:
  * RMII / RGMII mode (note : RMII master / slave can only be set
    by straps and cannot be changed at runtime)
  * RGMII delays. These delays can be enabled on the phy side, only
    as a boolean. Delays are enabled if phy-mode is rgmii-id, or
    rgmii-[rx|tx]id which enables only the corresponding delay.
  * In case another mode is encountered, do nothing and return
    success (keep old behavior)

Signed-off-by: Julien Blanc <julien.blanc@sprinte.eu>
---
Changes in v3:
- fix returning an uninitialized value if mode was not rgmii[-xx]
  or rmii (mii is a valid mode for this phy as well). Return
  success in that case to keep the old driver behavior.
- updated commit description accordingly

Changes in v2:
- remove the usage of phy_get_internal_delay
- use booleans to make it clear that thy phy supports only a
  fixed delay activation, no configurable delay

 drivers/net/phy/dp83td510.c | 68 +++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
index 9e9a41bf6457..f47802825cfe 100644
--- a/drivers/net/phy/dp83td510.c
+++ b/drivers/net/phy/dp83td510.c
@@ -30,6 +30,12 @@
 #define DP83TD510E_INT1_LINK			BIT(13)
 #define DP83TD510E_INT1_LINK_EN			BIT(5)
 
+#define DP83TD510E_RCSR				0x17
+#define DP83TD510E_RMII_MODE_EN			BIT(5)
+#define DP83TD510E_RGMII_MODE_EN		BIT(9)
+#define DP83TD510E_TX_CLK_SHIFT			BIT(11)
+#define DP83TD510E_RX_CLK_SHIFT			BIT(12)
+
 #define DP83TD510E_CTRL				0x1f
 #define DP83TD510E_CTRL_HW_RESET		BIT(15)
 #define DP83TD510E_CTRL_SW_RESET		BIT(14)
@@ -649,6 +655,67 @@ static int dp83td510_config_aneg(struct phy_device *phydev)
 	return genphy_c45_check_and_restart_aneg(phydev, changed);
 }
 
+static bool dp83td510_config_rgmii_rx_delay(struct phy_device *phydev)
+{
+	return phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	       phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID;
+}
+
+static bool dp83td510_config_rgmii_tx_delay(struct phy_device *phydev)
+{
+	return phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	       phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID;
+}
+
+static int dp83td510_config_init(struct phy_device *phydev)
+{
+	int rgmii_delay = 0;
+	bool rx_int_delay;
+	bool tx_int_delay;
+	int ret;
+
+	if (phy_interface_is_rgmii(phydev)) {
+		rx_int_delay = dp83td510_config_rgmii_rx_delay(phydev);
+		/* Set DP83TD510E_RX_CLK_SHIFT to enable rx clk internal delay */
+		if (rx_int_delay)
+			rgmii_delay |= DP83TD510E_RX_CLK_SHIFT;
+
+		tx_int_delay = dp83td510_config_rgmii_tx_delay(phydev);
+
+		/* Set DP83TD510E_TX_CLK_SHIFT to enable tx clk internal delay */
+		if (tx_int_delay)
+			rgmii_delay |= DP83TD510E_TX_CLK_SHIFT;
+
+		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_RCSR,
+				     DP83TD510E_RX_CLK_SHIFT | DP83TD510E_TX_CLK_SHIFT,
+				     rgmii_delay);
+		if (ret)
+			return ret;
+
+		ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
+				       DP83TD510E_RCSR, DP83TD510E_RGMII_MODE_EN);
+
+		if (ret)
+			return ret;
+
+	} else if (phydev->interface == PHY_INTERFACE_MODE_RMII) {
+		// set RMII_MODE_EN, clear RGMII_MODE_EN (exclusive)
+		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_RCSR,
+				     DP83TD510E_RMII_MODE_EN | DP83TD510E_RGMII_MODE_EN,
+				     DP83TD510E_RMII_MODE_EN);
+		if (ret)
+			return ret;
+	} else {
+		// may be RMII, which is supported, or something else. Just 
+		// return success to keep the old behavior and not break 
+		// anything. Configuration may have been done by straps so 
+		// it's better to keep as-is.
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static int dp83td510_get_sqi(struct phy_device *phydev)
 {
 	int sqi, ret;
@@ -942,6 +1009,7 @@ static struct phy_driver dp83td510_driver[] = {
 	.name		= "TI DP83TD510E",
 
 	.flags          = PHY_POLL_CABLE_TEST,
+	.config_init	= dp83td510_config_init,
 	.probe		= dp83td510_probe,
 	.config_aneg	= dp83td510_config_aneg,
 	.read_status	= dp83td510_read_status,
-- 
2.47.3

Re: [PATCH v3] Add config phase for dp83td510e phy

[Andrew Lunn]

On Tue, Sep 15, 2026 at 07:15:09AM +0000, Julien Blanc wrote:
> Add a config phase for the Texas Instruments DP83TD510E ethenet PHY
> 
> The config phase currently sets the following properties from the
> device tree:
>   * RMII / RGMII mode (note : RMII master / slave can only be set
>     by straps and cannot be changed at runtime)
>   * RGMII delays. These delays can be enabled on the phy side, only
>     as a boolean. Delays are enabled if phy-mode is rgmii-id, or
>     rgmii-[rx|tx]id which enables only the corresponding delay.
>   * In case another mode is encountered, do nothing and return
>     success (keep old behavior)
> 
> Signed-off-by: Julien Blanc <julien.blanc@sprinte.eu>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3] Add config phase for dp83td510e phy

[Jakub Kicinski]

some checkpatch nit picks to address here:

On Tue, 15 Sep 2026 07:15:09 +0000 Julien Blanc wrote:
> +	if (phy_interface_is_rgmii(phydev)) {
> +		rx_int_delay = dp83td510_config_rgmii_rx_delay(phydev);
> +		/* Set DP83TD510E_RX_CLK_SHIFT to enable rx clk internal delay */

nit: we prefer staying under 80 char line width if it doesn't make 
the code less readable. Comments should always be under 80 chars.
You can probably drop this comment completely, IDK what value it adds,
the if below is clear enough

> +		if (rx_int_delay)
> +			rgmii_delay |= DP83TD510E_RX_CLK_SHIFT;
> +
> +		tx_int_delay = dp83td510_config_rgmii_tx_delay(phydev);
> +
> +		/* Set DP83TD510E_TX_CLK_SHIFT to enable tx clk internal delay */

ditto

> +		if (tx_int_delay)
> +			rgmii_delay |= DP83TD510E_TX_CLK_SHIFT;
> +
> +		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_RCSR,
> +				     DP83TD510E_RX_CLK_SHIFT | DP83TD510E_TX_CLK_SHIFT,

this is also >80 char but more of a judgement call, up to you

> +				     rgmii_delay);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
> +				       DP83TD510E_RCSR, DP83TD510E_RGMII_MODE_EN);
> +
> +		if (ret)
> +			return ret;
> +
> +	} else if (phydev->interface == PHY_INTERFACE_MODE_RMII) {
> +		// set RMII_MODE_EN, clear RGMII_MODE_EN (exclusive)

Okay, but you're mixing comment styles, above you used /**/
(so does the rest of this file)

> +		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_RCSR,
> +				     DP83TD510E_RMII_MODE_EN | DP83TD510E_RGMII_MODE_EN,
> +				     DP83TD510E_RMII_MODE_EN);
> +		if (ret)
> +			return ret;
> +	} else {
> +		// may be RMII, which is supported, or something else. Just 
> +		// return success to keep the old behavior and not break 
> +		// anything. Configuration may have been done by straps so 
> +		// it's better to keep as-is.

This comment block has trailing space chars, please clean up

> +		ret = 0;
> +	}
> +
> +	return ret;
> +}

Please use --subject-prefix="PATCH net-next v4" when reposting,
to clearly mark the patch for net-next and add/preserve Andrew's
review tag
-- 
pw-bot: cr