Re: [PATCH] net: phy: at803x: disable delay only for RGMII mode

[Niklas Cassel <niklas.cassel@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

On Wed, Feb 13, 2019 at 02:40:18PM +0100, Marc Gonzalez wrote:
> On 13/02/2019 14:29, Andrew Lunn wrote:
> 
> >> So we have these modes:
> >>
> >> PHY_INTERFACE_MODE_RGMII: TX and RX delays disabled
> >> PHY_INTERFACE_MODE_RGMII_ID: TX and RX delays enabled
> >> PHY_INTERFACE_MODE_RGMII_RXID: RX delay enabled, TX delay disabled
> >> PHY_INTERFACE_MODE_RGMII_TXID: TX delay enabled, RX delay disabled
> >>
> >> What I don't like with this patch, is that if we specify phy-mode
> >> PHY_INTERFACE_MODE_RGMII_TXID, this patch will enable TX delay,
> >> but RX delay will not be explicitly set.
> > 
> > That is not the behaviour we want. It is best to assume the device is
> > in a random state, and correctly enable/disable all delays as
> > requested. Only leave the hardware alone if PHY_INTERFACE_MODE_NA is
> > used.
> 
> That's what my patch did:
> https://www.spinics.net/lists/netdev/msg445053.html
> 
> But see Florian's remarks:
> https://www.spinics.net/lists/netdev/msg445133.html

Hello Marc,

I saw that comment from Florian. However that was way back in 2017.
Maybe the phy-modes were not as well defined back then?

Andrew recently suggested to fix the driver so that it conforms with the
phy-modes, and fix any SoC that specified an incorrect phy-mode in DT
and thus relied upon the broken behavior of the PHY driver:
https://www.spinics.net/lists/netdev/msg445133.html


So, I've rebased your old patch, see attachment.
I suggest that Peter test it on am335x-evm.

am335x-evm appears to rely on the current broken behavior of the PHY
driver, so we will probably need to fix the am335x-evm according to this:
https://www.spinics.net/lists/netdev/msg445117.html
and merge that as well.


Andrew, Florian, do you both agree?


Kind regards,
Niklas

[-- Attachment #2: 0001-net-phy-at803x-Fix-RGMII-RX-and-TX-clock-delays-setu.patch --]
[-- Type: text/plain, Size: 2910 bytes --]

From 7f19f32d3074c9990e46349b76ae13dc9c1133d6 Mon Sep 17 00:00:00 2001
From: Marc Gonzalez <marc.w.gonzalez@free.fr>
Date: Wed, 13 Feb 2019 18:29:02 +0100
Subject: [PATCH] net: phy: at803x: Fix RGMII RX and TX clock delays setup

The current code supports enabling RGMII RX and TX clock delays.
The unstated assumption is that these settings are disabled by
default at reset, which is not the case.

RX clock delay is enabled at reset. And TX clock delay "survives"
across SW resets. Thus, if the bootloader enables TX clock delay,
it will remain enabled at reset in Linux.

Provide disable functions to configure the RGMII clock delays
exactly as specified in the fwspec.

Fixes: cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode")
Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
---
 drivers/net/phy/at803x.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 90dc62c15fc5..700c1e4d34ad 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -103,12 +103,24 @@ static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
 	return phy_write(phydev, AT803X_DEBUG_DATA, val);
 }
 
+static inline int at803x_enable_rx_delay(struct phy_device *phydev)
+{
+	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0, 0,
+				     AT803X_DEBUG_RX_CLK_DLY_EN);
+}
+
 static inline int at803x_disable_rx_delay(struct phy_device *phydev)
 {
 	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
 				     AT803X_DEBUG_RX_CLK_DLY_EN, 0);
 }
 
+static inline int at803x_enable_tx_delay(struct phy_device *phydev)
+{
+	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5, 0,
+				     AT803X_DEBUG_TX_CLK_DLY_EN);
+}
+
 static inline int at803x_disable_tx_delay(struct phy_device *phydev)
 {
 	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
@@ -242,20 +254,22 @@ static int at803x_config_init(struct phy_device *phydev)
 		return ret;
 
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
-			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			phydev->interface == PHY_INTERFACE_MODE_RGMII) {
+	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
+		ret = at803x_enable_rx_delay(phydev);
+	else
 		ret = at803x_disable_rx_delay(phydev);
-		if (ret < 0)
-			return ret;
-	}
+
+	if (ret < 0)
+		return ret;
 
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
-			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			phydev->interface == PHY_INTERFACE_MODE_RGMII) {
+	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
+		ret = at803x_enable_tx_delay(phydev);
+	else
 		ret = at803x_disable_tx_delay(phydev);
-		if (ret < 0)
-			return ret;
-	}
+
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
-- 
2.20.1