Discussion:
[PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock delay issues
Marc Gonzalez
2017-07-21 11:25:05 UTC
Permalink
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.

Signed-off-by: Marc Gonzalez <***@sigmadesigns.com>
---
drivers/net/phy/at803x.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index c1e52b9dc58d..7a0954513b91 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -283,6 +283,12 @@ static int at803x_config_init(struct phy_device *phydev)
if (ret < 0)
return ret;

+ /*
+ * NB: This code assumes that RGMII RX clock delay is disabled
+ * at reset, but actually, RX clock delay is enabled at reset.
+ * Disabling the delay if it has not been explicitly requested
+ * breaks boards that rely on the enabled-by-default behavior.
+ */
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
ret = at803x_enable_rx_delay(phydev);
@@ -290,6 +296,12 @@ static int at803x_config_init(struct phy_device *phydev)
return ret;
}

+ /*
+ * NB: This code assumes that RGMII TX clock delay is disabled
+ * at reset, but actually, TX clock delay "survives" across SW
+ * resets. If the bootloader enables TX clock delay, Linux is
+ * stuck with that setting.
+ */
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
ret = at803x_enable_tx_delay(phydev);
--
2.11.0
Marc Gonzalez
2017-07-21 11:25:52 UTC
Permalink
There are 4 RGMII phy-modes to handle.

Signed-off-by: Marc Gonzalez <***@sigmadesigns.com>
---
drivers/net/ethernet/aurora/nb8800.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index 041cfb7952f8..ded041dbafe7 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
mac_mode |= HALF_DUPLEX;

if (gigabit) {
- if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
+ if (phy_interface_is_rgmii(dev->phydev))
mac_mode |= RGMII_MODE;

mac_mode |= GMAC_MODE;
--
2.11.0
Måns Rullgård
2017-07-21 13:00:57 UTC
Permalink
Post by Marc Gonzalez
There are 4 RGMII phy-modes to handle.
---
drivers/net/ethernet/aurora/nb8800.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index 041cfb7952f8..ded041dbafe7 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
mac_mode |= HALF_DUPLEX;
if (gigabit) {
- if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
+ if (phy_interface_is_rgmii(dev->phydev))
mac_mode |= RGMII_MODE;
mac_mode |= GMAC_MODE;
--
2.11.0
--
Måns Rullgård
Florian Fainelli
2017-07-21 18:06:41 UTC
Permalink
Post by Marc Gonzalez
There are 4 RGMII phy-modes to handle.
... so make sure that the MAC is configured to set the RGMII_MODE
accordingly for all 4 RGMII mode.
Fixes: 52dfc8301248 ("net: ethernet: add driver for Aurora VLSI NB8800
Ethernet controller")
Post by Marc Gonzalez
---
drivers/net/ethernet/aurora/nb8800.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index 041cfb7952f8..ded041dbafe7 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
mac_mode |= HALF_DUPLEX;
if (gigabit) {
- if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
+ if (phy_interface_is_rgmii(dev->phydev))
mac_mode |= RGMII_MODE;
mac_mode |= GMAC_MODE;
--
Florian
Marc Gonzalez
2017-07-21 11:26:30 UTC
Permalink
According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
("Documentation: devicetree: clarify usage of the RGMII phy-modes")
there are 4 RGMII modes to handle:

"rgmii" (RX and TX delays are added by the MAC when required)
"rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
the MAC should not add the RX or TX delays in this case)
"rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
the MAC should not add an RX delay in this case)
"rgmii-txid" (RGMII with internal TX delay provided by the PHY,
the MAC should not add an TX delay in this case)

Add TX delay in the MAC only for rgmii and rgmii-rxid.

Signed-off-by: Marc Gonzalez <***@sigmadesigns.com>
---
drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ded041dbafe7..f3ed320eb4ad 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
break;

case PHY_INTERFACE_MODE_RGMII:
- pad_mode = PAD_MODE_RGMII;
+ case PHY_INTERFACE_MODE_RGMII_RXID:
+ pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
break;

+ case PHY_INTERFACE_MODE_RGMII_ID:
case PHY_INTERFACE_MODE_RGMII_TXID:
- pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+ pad_mode = PAD_MODE_RGMII;
break;

default:
--
2.11.0
Måns Rullgård
2017-07-21 13:04:07 UTC
Permalink
Post by Marc Gonzalez
According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
("Documentation: devicetree: clarify usage of the RGMII phy-modes")
"rgmii" (RX and TX delays are added by the MAC when required)
"rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
the MAC should not add the RX or TX delays in this case)
"rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
the MAC should not add an RX delay in this case)
"rgmii-txid" (RGMII with internal TX delay provided by the PHY,
the MAC should not add an TX delay in this case)
Add TX delay in the MAC only for rgmii and rgmii-rxid.
---
drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ded041dbafe7..f3ed320eb4ad 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
break;
- pad_mode = PAD_MODE_RGMII;
+ pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
break;
- pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+ pad_mode = PAD_MODE_RGMII;
break;
I still don't like this. Having both the MAC and PHY drivers react to
the phy-connection-type property is bound to cause trouble somewhere.

The only way out of the current mess is to define new properties for
both MAC and PHY that override the existing ones if present.
--
Måns Rullgård
Marc Gonzalez
2017-07-21 13:43:02 UTC
Permalink
Post by Måns Rullgård
Post by Marc Gonzalez
According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
("Documentation: devicetree: clarify usage of the RGMII phy-modes")
"rgmii" (RX and TX delays are added by the MAC when required)
"rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
the MAC should not add the RX or TX delays in this case)
"rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
the MAC should not add an RX delay in this case)
"rgmii-txid" (RGMII with internal TX delay provided by the PHY,
the MAC should not add an TX delay in this case)
Add TX delay in the MAC only for rgmii and rgmii-rxid.
---
drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ded041dbafe7..f3ed320eb4ad 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
break;
- pad_mode = PAD_MODE_RGMII;
+ pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
break;
- pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+ pad_mode = PAD_MODE_RGMII;
break;
I still don't like this. Having both the MAC and PHY drivers react to
the phy-connection-type property is bound to cause trouble somewhere.
The only way out of the current mess is to define new properties for
both MAC and PHY that override the existing ones if present.
Do you mean defining 4 new bindings and their corresponding
phy_interface_t enum values? For example:

"rgmii-v2"
"rgmii-id-v2"
"rgmii-rxid-v2"
"rgmii-txid-v2"

PHY_INTERFACE_MODE_RGMII_V2,
PHY_INTERFACE_MODE_RGMII_ID_V2,
PHY_INTERFACE_MODE_RGMII_RXID_V2,
PHY_INTERFACE_MODE_RGMII_TXID_V2,

And then handling these new enums in the at803x and nb8800 drivers?

FWIW, PAD_MODE_GTX_CLK_DELAY is broken in tango5 (doesn't add any
delay). I'm considering removing MAC-side TX delay altogether.

Regards.
Måns Rullgård
2017-07-21 13:47:04 UTC
Permalink
Post by Marc Gonzalez
Post by Måns Rullgård
Post by Marc Gonzalez
According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
("Documentation: devicetree: clarify usage of the RGMII phy-modes")
"rgmii" (RX and TX delays are added by the MAC when required)
"rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
the MAC should not add the RX or TX delays in this case)
"rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
the MAC should not add an RX delay in this case)
"rgmii-txid" (RGMII with internal TX delay provided by the PHY,
the MAC should not add an TX delay in this case)
Add TX delay in the MAC only for rgmii and rgmii-rxid.
---
drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ded041dbafe7..f3ed320eb4ad 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
break;
- pad_mode = PAD_MODE_RGMII;
+ pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
break;
- pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+ pad_mode = PAD_MODE_RGMII;
break;
I still don't like this. Having both the MAC and PHY drivers react to
the phy-connection-type property is bound to cause trouble somewhere.
The only way out of the current mess is to define new properties for
both MAC and PHY that override the existing ones if present.
Do you mean defining 4 new bindings and their corresponding
"rgmii-v2"
"rgmii-id-v2"
"rgmii-rxid-v2"
"rgmii-txid-v2"
PHY_INTERFACE_MODE_RGMII_V2,
PHY_INTERFACE_MODE_RGMII_ID_V2,
PHY_INTERFACE_MODE_RGMII_RXID_V2,
PHY_INTERFACE_MODE_RGMII_TXID_V2,
And then handling these new enums in the at803x and nb8800 drivers?
It has already been suggested to add new properties specifying desired
delays in picoseconds. If present on the MAC node, the MAC driver
should attempt to provide the delay, and if present on the PHY node, the
PHY driver is responsible.
--
Måns Rullgård
Marc Gonzalez
2017-07-21 14:18:09 UTC
Permalink
Post by Måns Rullgård
Post by Marc Gonzalez
Post by Måns Rullgård
Post by Marc Gonzalez
According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
("Documentation: devicetree: clarify usage of the RGMII phy-modes")
"rgmii" (RX and TX delays are added by the MAC when required)
"rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
the MAC should not add the RX or TX delays in this case)
"rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
the MAC should not add an RX delay in this case)
"rgmii-txid" (RGMII with internal TX delay provided by the PHY,
the MAC should not add an TX delay in this case)
Add TX delay in the MAC only for rgmii and rgmii-rxid.
---
drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ded041dbafe7..f3ed320eb4ad 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
break;
- pad_mode = PAD_MODE_RGMII;
+ pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
break;
- pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+ pad_mode = PAD_MODE_RGMII;
break;
I still don't like this. Having both the MAC and PHY drivers react to
the phy-connection-type property is bound to cause trouble somewhere.
The only way out of the current mess is to define new properties for
both MAC and PHY that override the existing ones if present.
Do you mean defining 4 new bindings and their corresponding
"rgmii-v2"
"rgmii-id-v2"
"rgmii-rxid-v2"
"rgmii-txid-v2"
PHY_INTERFACE_MODE_RGMII_V2,
PHY_INTERFACE_MODE_RGMII_ID_V2,
PHY_INTERFACE_MODE_RGMII_RXID_V2,
PHY_INTERFACE_MODE_RGMII_TXID_V2,
And then handling these new enums in the at803x and nb8800 drivers?
It has already been suggested to add new properties specifying desired
delays in picoseconds. If present on the MAC node, the MAC driver
should attempt to provide the delay, and if present on the PHY node, the
PHY driver is responsible.
If you introduced PHY and/or MAC specific properties to configure the
delays in the appropriate unit of time (say ps), you could use a
non-compliant 'phy-mode' just to satisfy the driver/PHY library and
still override the delays you need.
So we would need two properties (RX and TX).
"rgmii_rx_skew_ps" and "rgmii_tx_skew_ps"

but it's not clear to me how the MAC probe function communicates
the arguments to the phy driver. Looks like we would need to add
two fields to struct phy_device, and maybe define a new phy-mode
to instruct the PHY driver to look for the two fields.

I don't have time to work on that for now, but I do need to
fix the nb8800 driver now. Can we apply the following patch
in the interim?

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ded041dbafe7..e94159507847 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1268,11 +1268,10 @@ static int nb8800_tangox_init(struct net_device *dev)
break;

case PHY_INTERFACE_MODE_RGMII:
- pad_mode = PAD_MODE_RGMII;
- break;
-
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
- pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+ pad_mode = PAD_MODE_RGMII;
break;

default:

Regards.
Måns Rullgård
2017-07-21 14:24:28 UTC
Permalink
Post by Marc Gonzalez
Post by Måns Rullgård
Post by Marc Gonzalez
Post by Måns Rullgård
Post by Marc Gonzalez
According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
("Documentation: devicetree: clarify usage of the RGMII phy-modes")
"rgmii" (RX and TX delays are added by the MAC when required)
"rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
the MAC should not add the RX or TX delays in this case)
"rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
the MAC should not add an RX delay in this case)
"rgmii-txid" (RGMII with internal TX delay provided by the PHY,
the MAC should not add an TX delay in this case)
Add TX delay in the MAC only for rgmii and rgmii-rxid.
---
drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ded041dbafe7..f3ed320eb4ad 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
break;
- pad_mode = PAD_MODE_RGMII;
+ pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
break;
- pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+ pad_mode = PAD_MODE_RGMII;
break;
I still don't like this. Having both the MAC and PHY drivers react to
the phy-connection-type property is bound to cause trouble somewhere.
The only way out of the current mess is to define new properties for
both MAC and PHY that override the existing ones if present.
Do you mean defining 4 new bindings and their corresponding
"rgmii-v2"
"rgmii-id-v2"
"rgmii-rxid-v2"
"rgmii-txid-v2"
PHY_INTERFACE_MODE_RGMII_V2,
PHY_INTERFACE_MODE_RGMII_ID_V2,
PHY_INTERFACE_MODE_RGMII_RXID_V2,
PHY_INTERFACE_MODE_RGMII_TXID_V2,
And then handling these new enums in the at803x and nb8800 drivers?
It has already been suggested to add new properties specifying desired
delays in picoseconds. If present on the MAC node, the MAC driver
should attempt to provide the delay, and if present on the PHY node, the
PHY driver is responsible.
If you introduced PHY and/or MAC specific properties to configure the
delays in the appropriate unit of time (say ps), you could use a
non-compliant 'phy-mode' just to satisfy the driver/PHY library and
still override the delays you need.
So we would need two properties (RX and TX).
"rgmii_rx_skew_ps" and "rgmii_tx_skew_ps"
but it's not clear to me how the MAC probe function communicates
the arguments to the phy driver. Looks like we would need to add
two fields to struct phy_device, and maybe define a new phy-mode
to instruct the PHY driver to look for the two fields.
There's no need for the drivers to communicate. The location of the
properties in the device tree determines which driver should deal with
it.
Post by Marc Gonzalez
I don't have time to work on that for now, but I do need to
fix the nb8800 driver now. Can we apply the following patch
in the interim?
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ded041dbafe7..e94159507847 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1268,11 +1268,10 @@ static int nb8800_tangox_init(struct net_device *dev)
break;
- pad_mode = PAD_MODE_RGMII;
- break;
-
- pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+ pad_mode = PAD_MODE_RGMII;
break;
Simply stop reacting to the delay aspect of the phy-connection-type
property? Yes, I'm fine with that.
--
Måns Rullgård
Florian Fainelli
2017-07-21 18:15:49 UTC
Permalink
Post by Måns Rullgård
Post by Marc Gonzalez
Post by Måns Rullgård
Post by Marc Gonzalez
Post by Måns Rullgård
Post by Marc Gonzalez
According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
("Documentation: devicetree: clarify usage of the RGMII phy-modes")
"rgmii" (RX and TX delays are added by the MAC when required)
"rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
the MAC should not add the RX or TX delays in this case)
"rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
the MAC should not add an RX delay in this case)
"rgmii-txid" (RGMII with internal TX delay provided by the PHY,
the MAC should not add an TX delay in this case)
Add TX delay in the MAC only for rgmii and rgmii-rxid.
---
drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ded041dbafe7..f3ed320eb4ad 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
break;
- pad_mode = PAD_MODE_RGMII;
+ pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
break;
- pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+ pad_mode = PAD_MODE_RGMII;
break;
I still don't like this. Having both the MAC and PHY drivers react to
the phy-connection-type property is bound to cause trouble somewhere.
The only way out of the current mess is to define new properties for
both MAC and PHY that override the existing ones if present.
Do you mean defining 4 new bindings and their corresponding
"rgmii-v2"
"rgmii-id-v2"
"rgmii-rxid-v2"
"rgmii-txid-v2"
PHY_INTERFACE_MODE_RGMII_V2,
PHY_INTERFACE_MODE_RGMII_ID_V2,
PHY_INTERFACE_MODE_RGMII_RXID_V2,
PHY_INTERFACE_MODE_RGMII_TXID_V2,
And then handling these new enums in the at803x and nb8800 drivers?
It has already been suggested to add new properties specifying desired
delays in picoseconds. If present on the MAC node, the MAC driver
should attempt to provide the delay, and if present on the PHY node, the
PHY driver is responsible.
If you introduced PHY and/or MAC specific properties to configure the
delays in the appropriate unit of time (say ps), you could use a
non-compliant 'phy-mode' just to satisfy the driver/PHY library and
still override the delays you need.
So we would need two properties (RX and TX).
"rgmii_rx_skew_ps" and "rgmii_tx_skew_ps"
but it's not clear to me how the MAC probe function communicates
the arguments to the phy driver. Looks like we would need to add
two fields to struct phy_device, and maybe define a new phy-mode
to instruct the PHY driver to look for the two fields.
There's no need for the drivers to communicate. The location of the
properties in the device tree determines which driver should deal with
it.
Exactly. I really like how the PHY delay properties are defined in
Documentation/devicetree/bindings/net/micrel-ksz90x1.txt because they
are quite generic and for a MAC counterpart those defined in
Documentation/devicetree/bindings/net/dwmac-sun8i.txt and
Documentation/devicetree/bindings/net/meson-dwmac.txt are also good
examples.
Post by Måns Rullgård
Post by Marc Gonzalez
I don't have time to work on that for now, but I do need to
fix the nb8800 driver now. Can we apply the following patch
in the interim?
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ded041dbafe7..e94159507847 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1268,11 +1268,10 @@ static int nb8800_tangox_init(struct net_device *dev)
break;
- pad_mode = PAD_MODE_RGMII;
- break;
-
- pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+ pad_mode = PAD_MODE_RGMII;
break;
Simply stop reacting to the delay aspect of the phy-connection-type
property? Yes, I'm fine with that.
--
Florian
Marc Gonzalez
2017-07-21 12:47:53 UTC
Permalink
This post might be inappropriate. Click to display it.
Marc Gonzalez
2017-07-21 13:16:04 UTC
Permalink
Post by Marc Gonzalez
fping caps at 1000 packets per second, which limits
its usefulness as a benchmarking tool.
"Normal" ping is slightly faster (5000 packets per second).

time ping -f -c 60000 -s 1450 -q 172.27.64.1

--- 172.27.64.1 ping statistics ---
60000 packets transmitted, 60000 received, 0% packet loss, time 12059ms
rtt min/avg/max/mdev = 0.159/0.167/3.536/0.021 ms, ipg/ewma 0.201/0.166 ms

real 0m12.067s
user 0m0.899s
sys 0m1.886s

NIC statistics:
rx_bytes_ok: 89760375
rx_frames_ok: 60003
rx_undersize_frames: 0
rx_fragment_frames: 0
rx_64_byte_frames: 2
rx_127_byte_frames: 0
rx_255_byte_frames: 1
rx_511_byte_frames: 0
rx_1023_byte_frames: 0
rx_max_size_frames: 60000
rx_oversize_frames: 0
rx_bad_fcs_frames: 0
rx_broadcast_frames: 1
rx_multicast_frames: 0
rx_control_frames: 0
rx_pause_frames: 0
rx_unsup_control_frames: 0
rx_align_error_frames: 0
rx_overrun_frames: 0
rx_jabber_frames: 0
rx_bytes: 89760375
rx_frames: 60003
tx_bytes_ok: 89760128
tx_frames_ok: 60002
tx_64_byte_frames: 2
tx_127_byte_frames: 0
tx_255_byte_frames: 0
tx_511_byte_frames: 0
tx_1023_byte_frames: 0
tx_max_size_frames: 60000
tx_oversize_frames: 0
tx_broadcast_frames: 1
tx_multicast_frames: 0
tx_control_frames: 0
tx_pause_frames: 0
tx_underrun_frames: 0
tx_single_collision_frames: 0
tx_multi_collision_frames: 0
tx_deferred_collision_frames: 0
tx_late_collision_frames: 0
tx_excessive_collision_frames: 0
tx_bytes: 89760128
tx_frames: 60002
tx_collisions: 0
Marc Gonzalez
2017-07-21 11:29:49 UTC
Permalink
RX and TX clock delays are required.

Signed-off-by: Marc Gonzalez <***@sigmadesigns.com>
---
arch/arm/boot/dts/tango4-vantage-1172.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tango4-vantage-1172.dts b/arch/arm/boot/dts/tango4-vantage-1172.dts
index 86d8df98802f..13bcc460bcb2 100644
--- a/arch/arm/boot/dts/tango4-vantage-1172.dts
+++ b/arch/arm/boot/dts/tango4-vantage-1172.dts
@@ -22,7 +22,7 @@
};

&eth0 {
- phy-connection-type = "rgmii";
+ phy-connection-type = "rgmii-id";
phy-handle = <&eth0_phy>;
#address-cells = <1>;
#size-cells = <0>;
--
2.11.0
Timur Tabi
2017-07-21 13:20:39 UTC
Permalink
Post by Marc Gonzalez
+ * NB: This code assumes that RGMII RX clock delay is disabled
+ * at reset, but actually, RX clock delay is enabled at reset.
Could we change this to say, "RX clock delay is enabled at reset on some
systems."? Otherwise, it implies that the code is wrong 100% of the
time and should be fixed, not documented.
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.
Marc Gonzalez
2017-07-21 13:29:32 UTC
Permalink
This post might be inappropriate. Click to display it.
Timur Tabi
2017-07-21 14:06:44 UTC
Permalink
Post by Marc Gonzalez
I don't understand what you're saying.
It is a correct observation that the code enabling
RGMII RX clock delay is a NOP, since that bit will
always be set at that point.
The spec for the 8035 (I haven't checked for 8030 and 8031,
is that what you meant by "other systems"?) states that
"Control bit for rgmii interface rx clock delay"
is 1 after HW reset, 1 after SW reset.
So my statement "RX clock delay is enabled at reset"
is universally true. It's not just on some systems.
Ok, taken out of context, the comment doesn't really explain why the
code is the way it is. I'm not really happy about the word "assumes".
Maybe you should add a sentence explaining when the code is NOT a no-op.
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.
Marc Gonzalez
2017-07-21 14:36:41 UTC
Permalink
Post by Timur Tabi
Post by Marc Gonzalez
I don't understand what you're saying.
It is a correct observation that the code enabling
RGMII RX clock delay is a NOP, since that bit will
always be set at that point.
The spec for the 8035 (I haven't checked for 8030 and 8031,
is that what you meant by "other systems"?) states that
"Control bit for rgmii interface rx clock delay"
is 1 after HW reset, 1 after SW reset.
So my statement "RX clock delay is enabled at reset"
is universally true. It's not just on some systems.
Ok, taken out of context, the comment doesn't really explain why the
code is the way it is. I'm not really happy about the word "assumes".
If a HW setting defaults to 0 at reset, and some init is called
right after reset, then you know the setting's value is 0.
If you need that value to be 1, all you need is a function
setting it to 1. This is the current situation.

Commit 2e5f9f281ee8 assumes 0 at reset, and provides a function
setting the value to 1.

Reality is different. The HW setting defaults to 1 at reset.
So it turns out that the function setting the value to 1
is pointless, because the value is already 1. There is
however no way to set the value to 0.

Does that explain why I wrote "assume"?

Also the commit message:
"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."
Post by Timur Tabi
Maybe you should add a sentence explaining when the code is NOT a no-op.
The code is *NEVER* NOT a no-op.
I.e. the code enabling RX clock delay is ALWAYS a no-op.
I don't understand what you think is unclear in my comment.

Regards.
Loading...