[PATCH net-next v5] Add config phase for dp83td510e phy

[PATCH net-next v5] 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
Changes in v5:
- diff got mangled, resending

Changes in v4:
- fixes formatting and comment style issues

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 | 69 +++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
index 9e9a41bf6457..aa61220dbce3 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,68 @@ 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);
+		if (rx_int_delay)
+			rgmii_delay |= DP83TD510E_RX_CLK_SHIFT;
+
+		tx_int_delay = dp83td510_config_rgmii_tx_delay(phydev);
+
+		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 MII, 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 not touch it.
+		 */
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static int dp83td510_get_sqi(struct phy_device *phydev)
 {
 	int sqi, ret;
@@ -942,6 +1010,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 net-next v5] Add config phase for dp83td510e phy

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The TX delay may be programmed with the wrong polarity.
- [Medium] net: phy: dp83td510: fail to clear RMII mode when enabling
  RGMII
- [Low] There are three commit message problems.

--

Add config phase for dp83td510e phy

This adds a config_init callback to the TI DP83TD510E PHY driver. The
callback programs the RCSR register to choose RMII or RGMII mode from
phy-mode, and it turns the RGMII RX/TX internal clock shifts on or off to
match rgmii-id, rgmii-rxid and rgmii-txid. Any other interface mode is
left alone.

> Add config phase for dp83td510e phy

[Severity: Low]
This isn't a bug, but the subject has no subsystem prefix. netdev usually
expects something like:

  net: phy: dp83td510: add config_init to set MII mode and RGMII delays

> Add a config phase for the Texas Instruments DP83TD510E ethenet PHY

There's a typo here: "ethenet" should be "ethernet".

> Signed-off-by: Julien Blanc <julien.blanc@sprinte.eu>
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

A blank line sits between Signed-off-by and Reviewed-by, which splits the
trailer block. Because of that, git interpret-trailers and b4 won't treat
Signed-off-by as a trailer. Could the blank line be removed?

> diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
> index 9e9a41bf64574..aa61220dbce36 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)

[ ... ]

> +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);
> +		if (rx_int_delay)
> +			rgmii_delay |= DP83TD510E_RX_CLK_SHIFT;
> +
> +		tx_int_delay = dp83td510_config_rgmii_tx_delay(phydev);
> +
> +		if (tx_int_delay)
> +			rgmii_delay |= DP83TD510E_TX_CLK_SHIFT;

[Severity: Medium]
Does DP83TD510E_TX_CLK_SHIFT have the same polarity as
DP83TD510E_RX_CLK_SHIFT?

This RCSR layout is identical to the DP83822 one: register 0x17,
RMII_MODE_EN at BIT(5), RGMII_MODE_EN at BIT(9), TX_CLK_SHIFT at BIT(11)
and RX_CLK_SHIFT at BIT(12). On the DP83822, setting the TX shift bit
turns the TX internal delay off:

drivers/net/phy/dp83822.c:dp83822_config_init() {
    ...
		/* Set DP83822_TX_CLK_SHIFT to disable tx clk internal delay */
		if (tx_int_delay <= 0)
			rgmii_delay |= DP83822_TX_CLK_SHIFT;
    ...
}

The DP83822 driver used to get this backwards, and it was fixed in commit
c8a5c731fd12 ("net: phy: dp83822: Fix RGMII TX delay configuration"). That
commit cites the DP83822 datasheet and says the TX clock shift is disabled
when the bit is set.

If bit 11 of the DP83TD510E register works the same way, every RGMII mode
would get the wrong TX delay:

- Plain rgmii would get a PHY TX delay, on top of any MAC or PCB delay.
- rgmii-id and rgmii-txid would get no TX delay at all.

Does the DP83TD510E datasheet confirm that this bit is active-high for the
TX delay? It might be worth citing the datasheet description in the commit
message.

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

[Severity: Medium]
In the RGMII branch, dp83td510_config_init() uses phy_set_bits_mmd()
to set DP83TD510E_RGMII_MODE_EN. That leaves DP83TD510E_RMII_MODE_EN
as it was.

The RMII branch just below treats the two bits as mutually exclusive.
Its comment says so, and it clears RGMII_MODE_EN when it sets
RMII_MODE_EN. The RGMII branch doesn't do the reverse.

Suppose the straps or the bootloader left RMII_MODE_EN set in RCSR,
and the device tree says phy-mode = "rgmii*". Then the PHY ends up
with both mode bits set. By the patch's own reasoning that is not a
valid state, and the MAC interface may never come up.

Could this use phy_modify_mmd() instead? The mask would be
DP83TD510E_RMII_MODE_EN | DP83TD510E_RGMII_MODE_EN and the value
DP83TD510E_RGMII_MODE_EN, mirroring the RMII branch. The RX/TX clock
shift bits could be folded into the same read-modify-write, so RCSR is
only written once.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/30be27333dbf1a83efbe20e7b07acc6290805d65.camel%40sprinte.eu

Re: [PATCH net-next v5] Add config phase for dp83td510e phy

[Julien Blanc]

Le vendredi 25 septembre 2026 à 00:53 +0000, netdev-bot+sashiko@kernel.org a
écrit :
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 1
> 
> [Severity: Low]
> This isn't a bug, but the subject has no subsystem prefix. netdev usually
> expects something like:
> 
>   net: phy: dp83td510: add config_init to set MII mode and RGMII delays

Will fix, v6 coming soon.

> > Add a config phase for the Texas Instruments DP83TD510E ethenet PHY
> 
> There's a typo here: "ethenet" should be "ethernet".

Same

> 
> > Signed-off-by: Julien Blanc <julien.blanc@sprinte.eu>
> > 
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> A blank line sits between Signed-off-by and Reviewed-by, which splits the
> trailer block. Because of that, git interpret-trailers and b4 won't treat
> Signed-off-by as a trailer. Could the blank line be removed?

Noted as well for v6.
> 
> This RCSR layout is identical to the DP83822 one: register 0x17,
> RMII_MODE_EN at BIT(5), RGMII_MODE_EN at BIT(9), TX_CLK_SHIFT at BIT(11)
> and RX_CLK_SHIFT at BIT(12). On the DP83822, setting the TX shift bit
> turns the TX internal delay off:
> 
> drivers/net/phy/dp83822.c:dp83822_config_init() {
>     ...
> 		/* Set DP83822_TX_CLK_SHIFT to disable tx clk internal delay
> */
> 		if (tx_int_delay <= 0)
> 			rgmii_delay |= DP83822_TX_CLK_SHIFT;
>     ...
> }
> 
> The DP83822 driver used to get this backwards, and it was fixed in commit
> c8a5c731fd12 ("net: phy: dp83822: Fix RGMII TX delay configuration"). That
> commit cites the DP83822 datasheet and says the TX clock shift is disabled
> when the bit is set.
> 
> If bit 11 of the DP83TD510E register works the same way, every RGMII mode
> would get the wrong TX delay:

No, for DP83TD510E, the datasheet states that :

12 cfg_rgmii_rx_clk_shift_sel R/W 0x0 1b = RGMII RX clock and data are shifted
                                      0b = RGMII RX clock and data are aligned
11 cfg_rgmii_tx_clk_shift_sel R/W 0x0 1b = RGMII TX clock and data are shifted
                                      0b = RGMII TX clock and data are aligned

While the text is unclear, this actually means that writing a 1 enables the
internal delay. So what the patch does is correct as the DP83TD510e does not
work like the DP83822. I'll add a note in the commit message.

> > +
> > +		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> > DP83TD510E_RCSR,
> > +				     DP83TD510E_RX_CLK_SHIFT |
> > +				     DP83TD510E_TX_CLK_SHIFT,
> > +				     rgmii_delay);
> > +		if (ret)
> > +			return ret;
> 
> [Severity: Medium]
> In the RGMII branch, dp83td510_config_init() uses phy_set_bits_mmd()
> to set DP83TD510E_RGMII_MODE_EN. That leaves DP83TD510E_RMII_MODE_EN
> as it was.
> 
> The RMII branch just below treats the two bits as mutually exclusive.
> Its comment says so, and it clears RGMII_MODE_EN when it sets
> RMII_MODE_EN. The RGMII branch doesn't do the reverse.

Per the datasheet :

9 cfg_rgmii_en    R/W 0x0 1b = RGMII enable
                          0b = RGMII disable
5 cfg_rmii_mode   R/W 0x0 1b = RMII MAC
                          0b = MII MAC
                          (0x17[9] should be disabled)

So there's no need to clear the bit 5 when enabling RGMII, but it is needed when
enabling RMII, because otherwise RGMII mode would be selected, whatever is held
in bit 5. It's not "mutually exclusive", but rather "RGMII bit has priority".

Should i change the comment accordingly? (the comment is indeed misleading).

> 
> Suppose the straps or the bootloader left RMII_MODE_EN set in RCSR,
> and the device tree says phy-mode = "rgmii*". Then the PHY ends up
> with both mode bits set. By the patch's own reasoning that is not a
> valid state, and the MAC interface may never come up.
> 
> Could this use phy_modify_mmd() instead? The mask would be
> DP83TD510E_RMII_MODE_EN | DP83TD510E_RGMII_MODE_EN and the value
> DP83TD510E_RGMII_MODE_EN, mirroring the RMII branch. The RX/TX clock
> shift bits could be folded into the same read-modify-write, so RCSR is
> only written once.

That would work, but it is actually a bit odd to force MII mode over RMII, when
that bit is not relevant at all when RGMII is enabled. Other opinions?

Regards,

Julien