net: ethernet: nb8800: Do not apply TX delay at MAC level

PATCH Superseded Sebastian Frias 4 participants 11 messages Nov 4, 2016 – Nov 14, 2016

Subsystems (3)

Networking Network drivers Ethernet PHY

lore

Share & tools

lore

Sebastian Frias

author

2016-11-04 15:02 UTC #0 lore raw

PATCH 1/2 net: ethernet: nb8800: Do not apply TX delay at MAC level

The delay can be applied at PHY or MAC level, but since PHY drivers will apply the delay at PHY level when using one of the "internal delay" declinations of RGMII mode (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again at MAC level causes issues.

Signed-off-by Sebastian Frias <[email protected]>

---
 drivers/net/ethernet/aurora/nb8800.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/ethernet/aurora/nb8800.c +1 -1

index b59aa35..d2855c9 100644
@@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
 		break;
 
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+		pad_mode = PAD_MODE_RGMII;
 		break;
 
 	default:

Signature · 1 line

1.7.11.2

Andrew Lunn

maintainer

2016-11-04 15:11 UTC #1 lore raw

Quoted 16 lines · Sebastian Frias

The delay can be applied at PHY or MAC level, but since PHY drivers will apply the delay at PHY level when using one of the "internal delay" declinations of RGMII mode (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again at MAC level causes issues.
Signed-off-by Sebastian Frias <[email protected]>
---
 drivers/net/ethernet/aurora/nb8800.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/aurora/nb8800.c +0 -0
index b59aa35..d2855c9 100644

drivers/net/ethernet/aurora/nb8800.c static int nb8800_tangox_init(struct net_device *dev) · line 1286

Quoted 6 lines

drivers/net/ethernet/aurora/nb8800.c +1 -1

@@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
 		break;
 
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+		pad_mode = PAD_MODE_RGMII;
 		break;

How many boards use this Ethernet driver? How many boards are your potentially breaking, because they need this delay?

I guess it is a small number, because doesn't it require the PHY is also broken, not adding a delay when it should?

Andrew

Måns Rullgård

2016-11-04 15:27 UTC #2 lore raw

Quoted 6 lines · Andrew Lunn

Quoted 5 lines · Sebastian Frias
The delay can be applied at PHY or MAC level, but since PHY drivers will apply the delay at PHY level when using one of the "internal delay" declinations of RGMII mode (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again at MAC level causes issues.

If this is correct, most of the PHY drivers are broken.

Quoted 20 lines

Quoted 10 lines
Signed-off-by Sebastian Frias <[email protected]>
---
 drivers/net/ethernet/aurora/nb8800.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/aurora/nb8800.c +0 -0
index b59aa35..d2855c9 100644
drivers/net/ethernet/aurora/nb8800.c static int nb8800_tangox_init(struct net_device *dev) · line 1286
Quoted 6 lines
drivers/net/ethernet/aurora/nb8800.c +1 -1
@@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
 		break;
 
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+		pad_mode = PAD_MODE_RGMII;
 		break;
How many boards use this Ethernet driver? How many boards are your potentially breaking, because they need this delay?

Quoted 2 lines

I guess it is a small number, because doesn't it require the PHY is also broken, not adding a delay when it should?

What if the PHY doesn't have that option?

Signature · 1 line

Måns Rullgård

Sebastian Frias

author

2016-11-04 15:29 UTC #3 lore raw

In reply to Andrew Lunn

Hi Andrew,

Quoted 19 lines · Andrew Lunn

Quoted 18 lines · Sebastian Frias
The delay can be applied at PHY or MAC level, but since PHY drivers will apply the delay at PHY level when using one of the "internal delay" declinations of RGMII mode (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again at MAC level causes issues.
Signed-off-by Sebastian Frias <[email protected]>
---
 drivers/net/ethernet/aurora/nb8800.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/aurora/nb8800.c +0 -0
index b59aa35..d2855c9 100644
@@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
 		break;
 

Quoted 8 lines

drivers/net/ethernet/aurora/nb8800.c static int nb8800_tangox_init(struct net_device *dev) · line 1286
Quoted 4 lines
drivers/net/ethernet/aurora/nb8800.c +1 -1
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+		pad_mode = PAD_MODE_RGMII;
 		break;
How many boards use this Ethernet driver? How many boards are your potentially breaking, because they need this delay?

This part is specific to the Tango architecture, as noted by the function name "nb8800_tangox_init".

Also the register used here is Sigma-specific (i.e.: not related to the Aurora VLSI MAC, "au-nb8800")

The thing is that without this patch if we set phy-connection-type="rgmii-txid" on the DT, then both, the PHY and the MAC, will apply the delay.

Best regards,

Sebastian

Quoted 5 lines

I guess it is a small number, because doesn't it require the PHY is also broken, not adding a delay when it should?
Andrew

Måns Rullgård

2016-11-04 15:18 UTC #4 lore raw

In reply to Sebastian Frias

Quoted 5 lines · Sebastian Frias

The delay can be applied at PHY or MAC level, but since PHY drivers will apply the delay at PHY level when using one of the "internal delay" declinations of RGMII mode (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again at MAC level causes issues.

The Broadcom GENET driver does the same thing.

Quoted 20 lines

Signed-off-by Sebastian Frias <[email protected]>

---
 drivers/net/ethernet/aurora/nb8800.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/ethernet/aurora/nb8800.c +1 -1

index b59aa35..d2855c9 100644
@@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
 		break;
 
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+		pad_mode = PAD_MODE_RGMII;
 		break;
 
 	default:

Signature · 1 line

1.7.11.2

If this change is correct (and I'm not convinced it is), that case should be merged with the one above it and PHY_INTERFACE_MODE_RGMII_RXID added as well.

Signature · 1 line

Måns Rullgård

Sebastian Frias

author

2016-11-04 15:36 UTC #5 lore raw

Hi Måns,

Quoted 10 lines · Måns Rullgård

Quoted 5 lines · Sebastian Frias
The delay can be applied at PHY or MAC level, but since PHY drivers will apply the delay at PHY level when using one of the "internal delay" declinations of RGMII mode (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again at MAC level causes issues.
The Broadcom GENET driver does the same thing.

Well, I don't know who uses that driver, or why they did it that way.

However, with the current code and DT bindings, if one requires the delay, phy-connection-type="rgmii-txid" must be set.

But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers will apply the delay.

I think a better way of dealing with this is that both, PHY and MAC drivers exchange information so that the delay is applied only once.

I can see how to do that in another patch set.

Quoted 17 lines

Signed-off-by Sebastian Frias <[email protected]>

---
 drivers/net/ethernet/aurora/nb8800.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/ethernet/aurora/nb8800.c +1 -1

index b59aa35..d2855c9 100644
@@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
 		break;
 
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+		pad_mode = PAD_MODE_RGMII;
 		break;

Quoted 8 lines

Quoted 3 lines
drivers/net/ethernet/aurora/nb8800.c +0 -0
 	default:
Signature · 1 line
1.7.11.2
If this change is correct (and I'm not convinced it is), that case should be merged with the one above it and PHY_INTERFACE_MODE_RGMII_RXID added as well.

I can do a single patch.

The reason I made two patches was that it was clear what this patch does, i.e.: do not apply the delay at MAC level, and what the subsequent patch does, i.e.: handle all RGMII declinations.

Best regards,

Sebastian

Florian Fainelli

2016-11-04 16:28 UTC #6 lore raw

Quoted 11 lines · Sebastian Frias

Hi Måns,
Quoted 8 lines · Måns Rullgård
Quoted 5 lines · Sebastian Frias
The delay can be applied at PHY or MAC level, but since PHY drivers will apply the delay at PHY level when using one of the "internal delay" declinations of RGMII mode (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again at MAC level causes issues.

Quoted 4 lines

Quoted 2 lines
The Broadcom GENET driver does the same thing.
Well, I don't know who uses that driver, or why they did it that way.

I do use this driver and it works for me (tm), although I tested mostly with Broadcom PHYs and Ethernet switches, rarely with third party PHYs, but had that too, but all of that is in tree though, drivers/net/phy/broadcom.com, drivers/net/dsa/b53/ so feel free to "audit" that part of the code too.

The configuration of the GENET port multiplexer requires us to specify how we want to align the clock and data, if we don't do that, and the PHY is also not agreeing with how its own delays should be configured, mayhem ensues, ranging from occasional transmit success, to high rates of CRC/FCS errors in best cases.

I did verify that the settings were correct using a scope FWIW.

Quoted 3 lines

However, with the current code and DT bindings, if one requires the delay, phy-connection-type="rgmii-txid" must be set.

Yes, and we would set it correctly for our Broadcom reference boards using this driver.

Quoted 6 lines

But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers will apply the delay.
I think a better way of dealing with this is that both, PHY and MAC drivers exchange information so that the delay is applied only once.

Exchange what information? The PHY device interface (phydev->interface) conveys the needed information for both entities.

Quoted 39 lines

I can see how to do that in another patch set.
Quoted 17 lines
Quoted 17 lines
Signed-off-by Sebastian Frias <[email protected]>
---
 drivers/net/ethernet/aurora/nb8800.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/aurora/nb8800.c +1 -1
index b59aa35..d2855c9 100644
@@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
 		break;
 
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+		pad_mode = PAD_MODE_RGMII;
 		break;
 
Quoted 8 lines
Quoted 3 lines
drivers/net/ethernet/aurora/nb8800.c +0 -0
 	default:
Signature · 1 line
1.7.11.2
If this change is correct (and I'm not convinced it is), that case should be merged with the one above it and PHY_INTERFACE_MODE_RGMII_RXID added as well.
I can do a single patch.
The reason I made two patches was that it was clear what this patch does, i.e.: do not apply the delay at MAC level, and what the subsequent patch does, i.e.: handle all RGMII declinations.
Best regards,
Sebastian

Signature · 1 line

Florian

Måns Rullgård

2016-11-04 16:49 UTC #7 lore raw

Quoted 39 lines · Florian Fainelli

Quoted 11 lines · Sebastian Frias
Hi Måns,
Quoted 8 lines · Måns Rullgård
Quoted 5 lines · Sebastian Frias
The delay can be applied at PHY or MAC level, but since PHY drivers will apply the delay at PHY level when using one of the "internal delay" declinations of RGMII mode (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again at MAC level causes issues.
Quoted 4 lines
Quoted 2 lines
The Broadcom GENET driver does the same thing.
Well, I don't know who uses that driver, or why they did it that way.
I do use this driver and it works for me (tm), although I tested mostly with Broadcom PHYs and Ethernet switches, rarely with third party PHYs, but had that too, but all of that is in tree though, drivers/net/phy/broadcom.com, drivers/net/dsa/b53/ so feel free to "audit" that part of the code too.
The configuration of the GENET port multiplexer requires us to specify how we want to align the clock and data, if we don't do that, and the PHY is also not agreeing with how its own delays should be configured, mayhem ensues, ranging from occasional transmit success, to high rates of CRC/FCS errors in best cases.
I did verify that the settings were correct using a scope FWIW.
Quoted 3 lines
However, with the current code and DT bindings, if one requires the delay, phy-connection-type="rgmii-txid" must be set.
Yes, and we would set it correctly for our Broadcom reference boards using this driver.
Quoted 1 line

Quoted 8 lines

Quoted 5 lines
But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers will apply the delay.
I think a better way of dealing with this is that both, PHY and MAC drivers exchange information so that the delay is applied only once.
Exchange what information? The PHY device interface (phydev->interface) conveys the needed information for both entities.

There doesn't seem to be any consensus among the drivers regarding where the delay should be applied. Since only a few drivers, MAC or PHY, act on this property, most combinations still work by chance. It is common for boards to set the delay at the PHY using external config pins so no software setup is required (although I have one Sigma based board that gets this wrong). I suspect if drivers/net/ethernet/broadcom/genet were used with one of the four PHY drivers that also set the delay based on this DT property, things would go wrong.

Signature · 1 line

Måns Rullgård

Sebastian Frias

author

2016-11-09 13:02 UTC #8 lore raw

Quoted 9 lines · Måns Rullgård

Quoted 8 lines
Quoted 5 lines
But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers will apply the delay.
I think a better way of dealing with this is that both, PHY and MAC drivers exchange information so that the delay is applied only once.
Exchange what information? The PHY device interface (phydev->interface) conveys the needed information for both entities.

Quoted 9 lines

There doesn't seem to be any consensus among the drivers regarding where the delay should be applied. Since only a few drivers, MAC or PHY, act on this property, most combinations still work by chance. It is common for boards to set the delay at the PHY using external config pins so no software setup is required (although I have one Sigma based board that gets this wrong). I suspect if drivers/net/ethernet/broadcom/genet were used with one of the four PHY drivers that also set the delay based on this DT property, things would go wrong.

Exactly, what about a patch like (I can make a formal submission, even merge it with the patch discussed in this thread, consider this a RFC):

drivers/net/ethernet/aurora/nb8800.c +4 -0

index fba2699..4217ff4 100644
@@ -1283,6 +1283,10 @@ static int nb8800_tangox_init(struct net_device *dev)
 	case PHY_INTERFACE_MODE_RGMII_RXID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
 		pad_mode = PAD_MODE_RGMII;
+
+		if ((dev->phydev->flags & PHY_SUPPORTS_TXID) == 0)
+			pad_mode |= PAD_MODE_GTX_CLK_DELAY;
+
 		break;
 
 	default:

drivers/net/phy/at803x.c +3 -1

index 2e0c759..5eddb04 100644
@@ -426,7 +426,9 @@ static int at803x_aneg_done(struct phy_device *phydev)
 	.suspend		= at803x_suspend,
 	.resume			= at803x_resume,
 	.features		= PHY_GBIT_FEATURES,
-	.flags			= PHY_HAS_INTERRUPT,
+	.flags			= PHY_HAS_INTERRUPT |
+				  PHY_SUPPORTS_RXID |
+				  PHY_SUPPORTS_TXID,
 	.config_aneg		= genphy_config_aneg,
 	.read_status		= genphy_read_status,
 	.ack_interrupt		= at803x_ack_interrupt,

include/linux/phy.h +2 -0

index e7e1fd3..0f0b17e 100644
@@ -61,6 +61,8 @@
 #define PHY_HAS_INTERRUPT	0x00000001
 #define PHY_HAS_MAGICANEG	0x00000002
 #define PHY_IS_INTERNAL		0x00000004
+#define PHY_SUPPORTS_RXID       0x00000008
+#define PHY_SUPPORTS_TXID       0x00000010
 #define MDIO_DEVICE_IS_PHY	0x80000000
 
 /* Interface Mode definitions */

Florian Fainelli

2016-11-09 17:03 UTC #9 lore raw

Quoted 20 lines · Sebastian Frias

Quoted 9 lines · Måns Rullgård
Quoted 8 lines
Quoted 5 lines
But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers will apply the delay.
I think a better way of dealing with this is that both, PHY and MAC drivers exchange information so that the delay is applied only once.
Exchange what information? The PHY device interface (phydev->interface) conveys the needed information for both entities.
Quoted 9 lines
There doesn't seem to be any consensus among the drivers regarding where the delay should be applied. Since only a few drivers, MAC or PHY, act on this property, most combinations still work by chance. It is common for boards to set the delay at the PHY using external config pins so no software setup is required (although I have one Sigma based board that gets this wrong). I suspect if drivers/net/ethernet/broadcom/genet were used with one of the four PHY drivers that also set the delay based on this DT property, things would go wrong.

Quoted 2 lines

Exactly, what about a patch like (I can make a formal submission, even merge it with the patch discussed in this thread, consider this a RFC):

I really don't see a point in doing this when we can just clarify what phydev->interface does and already have the knowledge that we need without introducing additional flags in the phy driver.

Quoted 45 lines

drivers/net/ethernet/aurora/nb8800.c +4 -0

index fba2699..4217ff4 100644
@@ -1283,6 +1283,10 @@ static int nb8800_tangox_init(struct net_device *dev)
 	case PHY_INTERFACE_MODE_RGMII_RXID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
 		pad_mode = PAD_MODE_RGMII;
+
+		if ((dev->phydev->flags & PHY_SUPPORTS_TXID) == 0)
+			pad_mode |= PAD_MODE_GTX_CLK_DELAY;
+
 		break;
 
 	default:

drivers/net/phy/at803x.c +3 -1

index 2e0c759..5eddb04 100644
@@ -426,7 +426,9 @@ static int at803x_aneg_done(struct phy_device *phydev)
 	.suspend		= at803x_suspend,
 	.resume			= at803x_resume,
 	.features		= PHY_GBIT_FEATURES,
-	.flags			= PHY_HAS_INTERRUPT,
+	.flags			= PHY_HAS_INTERRUPT |
+				  PHY_SUPPORTS_RXID |
+				  PHY_SUPPORTS_TXID,
 	.config_aneg		= genphy_config_aneg,
 	.read_status		= genphy_read_status,
 	.ack_interrupt		= at803x_ack_interrupt,

include/linux/phy.h +2 -0

index e7e1fd3..0f0b17e 100644
@@ -61,6 +61,8 @@
 #define PHY_HAS_INTERRUPT	0x00000001
 #define PHY_HAS_MAGICANEG	0x00000002
 #define PHY_IS_INTERNAL		0x00000004
+#define PHY_SUPPORTS_RXID       0x00000008
+#define PHY_SUPPORTS_TXID       0x00000010
 #define MDIO_DEVICE_IS_PHY	0x80000000
 
 /* Interface Mode definitions */

Signature · 1 line

Florian

Sebastian Frias

author

2016-11-14 13:22 UTC #10 lore raw

Quoted 21 lines · Florian Fainelli

Quoted 20 lines · Sebastian Frias
Quoted 18 lines · Måns Rullgård
Quoted 8 lines
Quoted 5 lines
But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers will apply the delay.
I think a better way of dealing with this is that both, PHY and MAC drivers exchange information so that the delay is applied only once.
Exchange what information? The PHY device interface (phydev->interface) conveys the needed information for both entities.
There doesn't seem to be any consensus among the drivers regarding where the delay should be applied. Since only a few drivers, MAC or PHY, act on this property, most combinations still work by chance. It is common for boards to set the delay at the PHY using external config pins so no software setup is required (although I have one Sigma based board that gets this wrong). I suspect if drivers/net/ethernet/broadcom/genet were used with one of the four PHY drivers that also set the delay based on this DT property, things would go wrong.

Quoted 7 lines

Quoted 2 lines
Exactly, what about a patch like (I can make a formal submission, even merge it with the patch discussed in this thread, consider this a RFC):
I really don't see a point in doing this when we can just clarify what phydev->interface does and already have the knowledge that we need without introducing additional flags in the phy driver.

Ok, so who can clarify what "phydev->interface" does, especially in the context of this discussion?

What happens when a TX delay must be applied and:
- both the PHY and the MAC support the delay
- only the PHY supports the delay
- only the MAC supports the delay

Best regards,

Sebastian