[PATCH] Revert "net: phy: marvell 88e151x: Fix handling of bare RGMII interface type"
From 2f97d889cecfa2ba06652469765f31c70a68e867 Mon Sep 17 00:00:00 2001 From: Rufus Segar rufuss@carallon.com Date: Tue, 3 Dec 2024 14:35:10 +0000 Subject: [PATCH] Revert "net: phy: marvell 88e151x: Fix handling of bare RGMII interface type"
This reverts commit 431be621c6cbc72efd1d45fa36686a682cbb470a.
This change is erroneous, the RX/TX delay should only be added when in RGMII-ID mode. RGMII mode should have no RX/TX delay. --- drivers/net/phy/marvell.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index b0a0b7fcb3..08608a99b9 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -461,9 +461,7 @@ static int m88e151x_config(struct phy_device *phydev)
reg = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_88E151x_PHY_MSCR); reg &= ~MIIM_88E151x_RGMII_RXTX_DELAY; - if (phydev->interface == PHY_INTERFACE_MODE_RGMII || - phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) reg |= MIIM_88E151x_RGMII_RXTX_DELAY; else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) reg |= MIIM_88E151x_RGMII_RX_DELAY; -- 2.47.0
On Tue, Dec 03, 2024 at 02:45:36PM +0000, Rufus Segar wrote:
From 2f97d889cecfa2ba06652469765f31c70a68e867 Mon Sep 17 00:00:00 2001 From: Rufus Segar rufuss@carallon.com Date: Tue, 3 Dec 2024 14:35:10 +0000 Subject: [PATCH] Revert "net: phy: marvell 88e151x: Fix handling of bare RGMII interface type"
This reverts commit 431be621c6cbc72efd1d45fa36686a682cbb470a.
This change is erroneous, the RX/TX delay should only be added when in RGMII-ID mode. RGMII mode should have no RX/TX delay.
drivers/net/phy/marvell.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index b0a0b7fcb3..08608a99b9 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -461,9 +461,7 @@ static int m88e151x_config(struct phy_device *phydev)
reg = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_88E151x_PHY_MSCR); reg &= ~MIIM_88E151x_RGMII_RXTX_DELAY;
if (phydev->interface == PHY_INTERFACE_MODE_RGMII ||phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) reg |= MIIM_88E151x_RGMII_RXTX_DELAY;
Please provide more details and ideally links to relevant documentation as to why a 6 year old commit that itself says it's fixing a use case is wrong, thanks.
This reverts commit 431be621c6cbc72efd1d45fa36686a682cbb470a.
Section 3.3 of Reduced Gigabit Media Independent Interface (RGMII) Version 2.0 (4/1/2002) details that a PHYs using a ~2ns internal delay are referred to as RGMII-ID. This internal delay is optional.
Page 147-148 of the Marvell Doc. No. MV-S107146-U0 Rev. F details timings of the RX/TX delays. We see that with the TX/RX_CLK delay enabled, our RX/TX_CTL signal is shifted w.r.t CLK to reflect the delay added.
In 431be62 there is no timing difference between RGMII and RGMII-ID, and so programmers wanting to explicitly set their PHY to RGMII will find that delay added anyway. This could throw off timing if that internal delay is undesired.
We should be handling all 4 possible RGMII cases of PHY_INTERFACE_MODE: RGMII, RGMII_ID, RGMII_TXID, and RGMII_RXID. Reverting 431be62 implements this.
See also m88e1111_config_init_rgmii_delays in the equivalent driver in Linux (drivers/net/phy/marvell.c), which does not set these delays in RGMII mode.
68e6eca was tested out on an 88E1512 PHY in RGMII-ID mode. This reversion has been tested by myself on an 88E1518 in RGMII-ID mode. This patch affects boards using this driver in "rgmii" mode, as the internal delay will no longer be enabled. Namely kikwood-nsa310s.
Signed-off-by: Rufus Segar rhs@riseup.net --- V2: add justification for why this commit should be reverted.
Additional info: From reading the commit message of 431be62, its not clear what the problem being fixed was. My guess is their use case should have been fixed by simply changing to RGMII-ID mode in their configuration.
drivers/net/phy/marvell.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index b0a0b7fcb3..08608a99b9 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -461,8 +461,7 @@ static int m88e151x_config(struct phy_device *phydev)
reg = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_88E151x_PHY_MSCR); reg &= ~MIIM_88E151x_RGMII_RXTX_DELAY; - if (phydev->interface == PHY_INTERFACE_MODE_RGMII || - phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) reg |= MIIM_88E151x_RGMII_RXTX_DELAY; else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) reg |= MIIM_88E151x_RGMII_RX_DELAY;
On Wed, 04 Dec 2024 13:34:30 +0000, Rufus Segar wrote:
This reverts commit 431be621c6cbc72efd1d45fa36686a682cbb470a.
Section 3.3 of Reduced Gigabit Media Independent Interface (RGMII) Version 2.0 (4/1/2002) details that a PHYs using a ~2ns internal delay are referred to as RGMII-ID. This internal delay is optional.
Page 147-148 of the Marvell Doc. No. MV-S107146-U0 Rev. F details timings of the RX/TX delays. We see that with the TX/RX_CLK delay enabled, our RX/TX_CTL signal is shifted w.r.t CLK to reflect the delay added.
[...]
Applied to u-boot/next, thanks!
participants (3)
-
Rufus Segar -
Rufus Segar -
Tom Rini