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

[Julien Blanc <julien.blanc@sprinte.eu>]

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