[PATCH net-next] net: stmmac: dwmac-rk: only read the delays the MAC applies

[PATCH net-next] net: stmmac: dwmac-rk: only read the delays the MAC applies

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

The tx_delay and rx_delay properties are only used for the directions in
which the MAC inserts the RGMII clock delay itself: both for "rgmii",
TX for "rgmii-rxid" and RX for "rgmii-txid". For "rgmii-id" (the PHY
adds both delays, the recommended setup) and for RMII they are ignored
by rk_gmac_powerup().

The driver nevertheless parses both properties unconditionally and logs
an error, plus the default it picks, for each one that is missing. A
correct device tree using "rgmii-rxid" therefore gets

  Can not read property: rx_delay.
  set rx_delay to 0x10

and one using "rgmii-id" gets the same pair for tx_delay as well, for a
value that is never programmed.

Only parse the delay values that will actually be used, so the error is
reserved for device trees that really lack a required delay. Behaviour
for "rgmii", "rgmii-rxid" and "rgmii-txid" is unchanged.

Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
Tested on an Orange Pi 5 (RK3588S, Motorcomm YT8531C on gmac1) with its
device tree using "rgmii-id": the probe log has "init for RGMII_ID" and
no "Can not read property" lines any more, the link comes up at
1000/Full and transfers run at line rate (940 Mbit/s receive, 0 errors).
The "rgmii", "rgmii-rxid" and "rgmii-txid" paths are unchanged and were
not re-tested.

This patch was developed with Claude (Anthropic) as a coding assistant:
it wrote the code and the commit message, and I reviewed and tested it
as described above.
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 47 ++++++++++++++++----------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index c4db919ec164..f06428ce3cfc 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -1331,26 +1331,37 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
 			bsp_priv->clock_input = false;
 	}
 
-	ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
-	if (ret) {
-		bsp_priv->tx_delay = 0x30;
-		dev_err(dev, "Can not read property: tx_delay.");
-		dev_err(dev, "set tx_delay to 0x%x\n",
-			bsp_priv->tx_delay);
-	} else {
-		dev_info(dev, "TX delay(0x%x).\n", value);
-		bsp_priv->tx_delay = value;
+	/*
+	 * The MAC only inserts the delays which neither the PCB nor the PHY
+	 * provide (see rk_gmac_powerup()), so only the delay values for
+	 * those directions are needed.
+	 */
+	if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII ||
+	    bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII_RXID) {
+		ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
+		if (ret) {
+			bsp_priv->tx_delay = 0x30;
+			dev_err(dev, "Can not read property: tx_delay.");
+			dev_err(dev, "set tx_delay to 0x%x\n",
+				bsp_priv->tx_delay);
+		} else {
+			dev_info(dev, "TX delay(0x%x).\n", value);
+			bsp_priv->tx_delay = value;
+		}
 	}
 
-	ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
-	if (ret) {
-		bsp_priv->rx_delay = 0x10;
-		dev_err(dev, "Can not read property: rx_delay.");
-		dev_err(dev, "set rx_delay to 0x%x\n",
-			bsp_priv->rx_delay);
-	} else {
-		dev_info(dev, "RX delay(0x%x).\n", value);
-		bsp_priv->rx_delay = value;
+	if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII ||
+	    bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII_TXID) {
+		ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
+		if (ret) {
+			bsp_priv->rx_delay = 0x10;
+			dev_err(dev, "Can not read property: rx_delay.");
+			dev_err(dev, "set rx_delay to 0x%x\n",
+				bsp_priv->rx_delay);
+		} else {
+			dev_info(dev, "RX delay(0x%x).\n", value);
+			bsp_priv->rx_delay = value;
+		}
 	}
 
 	bsp_priv->grf = syscon_regmap_lookup_by_phandle(dev->of_node,

---
base-commit: d26955e57475434b59d43ba372b2a50d33d5e0ed
change-id: 20261110-stmmac-dwmac-rk-delays-41a0387624c9

Best regards,
--  
Arun Iyengar <aiyengar1990@gmail.com>

Re: [PATCH net-next] net: stmmac: dwmac-rk: only read the delays the MAC applies

[Maxime Chevallier]

Hi,

On 11/10/26 08:13, Arun Iyengar via B4 Relay wrote:
> From: Arun Iyengar <aiyengar1990@gmail.com>
> 
> The tx_delay and rx_delay properties are only used for the directions in
> which the MAC inserts the RGMII clock delay itself: both for "rgmii",
> TX for "rgmii-rxid" and RX for "rgmii-txid". For "rgmii-id" (the PHY
> adds both delays, the recommended setup) and for RMII they are ignored
> by rk_gmac_powerup().
> 
> The driver nevertheless parses both properties unconditionally and logs
> an error, plus the default it picks, for each one that is missing. A
> correct device tree using "rgmii-rxid" therefore gets
> 
>   Can not read property: rx_delay.
>   set rx_delay to 0x10
> 
> and one using "rgmii-id" gets the same pair for tx_delay as well, for a
> value that is never programmed.
> 
> Only parse the delay values that will actually be used, so the error is
> reserved for device trees that really lack a required delay. Behaviour
> for "rgmii", "rgmii-rxid" and "rgmii-txid" is unchanged.
> 
> Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>

LGTM, I was worried about breaking compat but the rk_gmac_powerup() only
applies the rx/tx delays based on the ID mode, so

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime