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

[netdev-bot+sashiko@kernel.org]

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