[RFC PATCH net v2 0/3] Fix OdroidC2 Gigabit Tx link issue

This patchset fixes an issue with the OdroidC2 board (DWMAC + RTL8211F).
Initially reported as a low Tx throughput issue at gigabit speed, the
platform enters LPI too often. This eventually break the link (both Tx
and Rx), and require to bring the interface down and up again to get the
Rx path working again.

The root cause of this issue is not fully understood yet but disabling EEE
advertisement on the PHY prevent this feature to be negotiated.
With this change, the link is stable and reliable, with the expected
throughput performance.

The patchset adds options in the generic phy driver to disable EEE
advertisement, through device tree. The way it is done is very similar
to the handling of the max-speed property.

This V2 is posted is posted as an RFC. Since it changes the generic PHY
it propably requires to be a bit more careful.
If you are not confortable taking for the coming rc, I can rebase on
net-next instead.

Chnages since V1: [1]
 - Disable the advertisement of EEE in the generic code instead of the
   realtek driver.

[1] : http://lkml.kernel.org/r/1479220154-25851-1-git-send-email-jbrunet@baylibre.com

Jerome Brunet (3):
  net: phy: add an option to disable EEE advertisement
  dt: bindings: add ethernet phy eee-disable-advert option documentation
  ARM64: dts: meson: odroidc2: disable advertisement EEE for GbE.

 Documentation/devicetree/bindings/net/phy.txt      |  5 ++
 .../arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts | 15 ++++
 drivers/net/phy/phy.c                              |  3 +
 drivers/net/phy/phy_device.c                       | 80 +++++++++++++++++++---
 include/linux/phy.h                                |  3 +
 5 files changed, 97 insertions(+), 9 deletions(-)

-- 
2.7.4

This patch adds an option to disable EEE advertisement in the generic PHY
by providing a mask of prohibited modes corresponding to the value found in
the MDIO_AN_EEE_ADV register.

On some platforms, PHY Low power idle seems to be causing issues, even
breaking the link some cases. The patch provides a convenient way for these
platforms to disable EEE advertisement and work around the issue.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/net/phy/phy.c        |  3 ++
 drivers/net/phy/phy_device.c | 80 +++++++++++++++++++++++++++++++++++++++-----
 include/linux/phy.h          |  3 ++
 3 files changed, 77 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index f424b867f73e..a44ee14bd953 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -1348,6 +1348,9 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
 {
 	int val = ethtool_adv_to_mmd_eee_adv_t(data->advertised);
 
+	/* Mask prohibited EEE modes */
+	val &= ~phydev->eee_advert_disabled;
+
 	phy_write_mmd_indirect(phydev, MDIO_AN_EEE_ADV, MDIO_MMD_AN, val);
 
 	return 0;

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1a4bf8acad78..74c628e046cb 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -1116,6 +1116,43 @@ static int genphy_config_advert(struct phy_device *phydev)
 }
 
 /**
+ * genphy_config_eee_advert - disable unwanted eee mode advertisement
+ * @phydev: target phy_device struct
+ *
+ * Description: Writes MDIO_AN_EEE_ADV after disabling unsupported energy
+ *   efficent ethernet modes. Returns 0 if the PHY's advertisement hasn't
+ *   changed, and 1 if it has changed.
+ */
+static int genphy_config_eee_advert(struct phy_device *phydev)
+{
+	u32 disabled = phydev->eee_advert_disabled;
+	u32 old_adv, adv;
+
+	/* Nothing to disable */
+	if (!disabled)
+		return 0;
+
+	/* If the following call fails, we assume that EEE is not
+	 * supported by the phy. If we read 0, EEE is not advertised
+	 * In both case, we don't need to continue
+	 */
+	adv = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV, MDIO_MMD_AN);
+	if (adv <= 0)
+		return 0;
+
+	old_adv = adv;
+	adv &= ~disabled;
+
+	/* Advertising remains unchanged with the ban */
+	if (old_adv == adv)
+		return 0;
+
+	phy_write_mmd_indirect(phydev, MDIO_AN_EEE_ADV, MDIO_MMD_AN, adv);
+
+	return 1;
+}
+
+/**
  * genphy_setup_forced - configures/forces speed/duplex from @phydev
  * @phydev: target phy_device struct
  *

@@ -1173,15 +1210,20 @@ EXPORT_SYMBOL(genphy_restart_aneg);
  */
 int genphy_config_aneg(struct phy_device *phydev)
 {
-	int result;
+	int err, changed;
+
+	changed = genphy_config_eee_advert(phydev);
 
 	if (AUTONEG_ENABLE != phydev->autoneg)
 		return genphy_setup_forced(phydev);
 
-	result = genphy_config_advert(phydev);
-	if (result < 0) /* error */
-		return result;
-	if (result == 0) {
+	err = genphy_config_advert(phydev);
+	if (err < 0) /* error */
+		return err;
+
+	changed |= err;
+
+	if (changed == 0) {
 		/* Advertisement hasn't changed, but maybe aneg was never on to
 		 * begin with?  Or maybe phy was isolated?
 		 */

@@ -1191,16 +1233,16 @@ int genphy_config_aneg(struct phy_device *phydev)
 			return ctl;
 
 		if (!(ctl & BMCR_ANENABLE) || (ctl & BMCR_ISOLATE))
-			result = 1; /* do restart aneg */
+			changed = 1; /* do restart aneg */
 	}
 
 	/* Only restart aneg if we are advertising something different
 	 * than we were before.
 	 */
-	if (result > 0)
-		result = genphy_restart_aneg(phydev);
+	if (changed > 0)
+		return genphy_restart_aneg(phydev);
 
-	return result;
+	return 0;
 }
 EXPORT_SYMBOL(genphy_config_aneg);
 

@@ -1558,6 +1600,21 @@ static void of_set_phy_supported(struct phy_device *phydev)
 		__set_phy_supported(phydev, max_speed);
 }
 
+static void of_set_phy_eee_disable(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	u32 disabled;
+
+	if (!IS_ENABLED(CONFIG_OF_MDIO))
+		return;
+
+	if (!node)
+		return;
+
+	if (!of_property_read_u32(node, "eee-advert-disable", &disabled))
+		phydev->eee_advert_disabled = disabled;
+}
+
 /**
  * phy_probe - probe and init a PHY device
  * @dev: device to probe and init

@@ -1595,6 +1652,11 @@ static int phy_probe(struct device *dev)
 	of_set_phy_supported(phydev);
 	phydev->advertising = phydev->supported;
 
+	/* Get the EEE modes we want to prohibit. We will ask
+	 * the PHY stop advertising these mode later on
+	 */
+	of_set_phy_eee_disable(phydev);
+
 	/* Set the state to READY by default */
 	phydev->state = PHY_READY;
 

diff --git a/include/linux/phy.h b/include/linux/phy.h
index e25f1830fbcf..7f2ea0af16d1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -401,6 +401,9 @@ struct phy_device {
 	u32 advertising;
 	u32 lp_advertising;
 
+	/* Energy efficient ethernet modes which should be prohibited */
+	u32 eee_advert_disabled;
+
 	int autoneg;
 
 	int link_timeout;

-- 
2.7.4

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 Documentation/devicetree/bindings/net/phy.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index bc1c3c8bf8fa..7f066b7c1e2c 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt

@@ -35,6 +35,11 @@ Optional Properties:
 - broken-turn-around: If set, indicates the PHY device does not correctly
   release the turn around line low at the end of a MDIO transaction.
 
+- eee-advert-disable: Bits to clear in the MDIO_AN_EEE_ADV register to
+  disable EEE modes. Example
+    * 0x4: disable EEE for 1000T,
+    * 0x6: disable EEE for 100TX and 1000T
+
 Example:
 
 ethernet-phy@0 {

-- 
2.7.4

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
index e6e3491d48a5..b34da077b2f8 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts

@@ -98,3 +98,18 @@
 	pinctrl-0 = <&i2c_a_pins>;
 	pinctrl-names = "default";
 };
+
+&ethmac {
+	phy-handle = <&eth_phy0>;
+
+	mdio {
+		compatible = "snps,dwmac-mdio";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		eth_phy0: ethernet-phy@0 {
+			reg = <0>;
+			eee-advert-disable = <0x4>;
+		};
+	};
+};

-- 
2.7.4

On Mon, Nov 21, 2016 at 04:35:23PM +0100, Jerome Brunet wrote:

Hi Jerome

I like the direction this patchset is taking. But hex values are
pretty unfriendly. Please add a set of boolean properties, and do the
mapping to hex in the C code.

That would also make extending this API easier. e.g. say you have a
10Gbps PHY with EEE, and you need to disable it. This hex value
quickly gets ugly, eee-advert-disable-10000 is nice and simple.

	Andrew

On Mon, 2016-11-21 at 17:01 +0100, Andrew Lunn wrote:

On Mon, Nov 21, 2016 at 04:35:23PM +0100, Jerome Brunet wrote:

quoted

Signed-off-by: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 Documentation/devicetree/bindings/net/phy.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/phy.txt

b/Documentation/devicetree/bindings/net/phy.txt
index bc1c3c8bf8fa..7f066b7c1e2c 100644

--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt

@@ -35,6 +35,11 @@ Optional Properties:

 - broken-turn-around: If set, indicates the PHY device does not
correctly
   release the turn around line low at the end of a MDIO
transaction.
 
+- eee-advert-disable: Bits to clear in the MDIO_AN_EEE_ADV
register to
+  disable EEE modes. Example
+    * 0x4: disable EEE for 1000T,
+    * 0x6: disable EEE for 100TX and 1000T
+

Hi Jerome

I like the direction this patchset is taking. But hex values are
pretty unfriendly. 

Agreed

Please add a set of boolean properties, and do the
mapping to hex in the C code.

That would also make extending this API easier. e.g. say you have a
10Gbps PHY with EEE, and you need to disable it. This hex value
quickly gets ugly, eee-advert-disable-10000 is nice and simple.

What I did not realize when doing this patch for the realtek driver is
that there is already 6 valid modes defined in the kernel

#define MDIO_EEE_100TX		MDIO_AN_EEE_ADV_100TX	/*
100TX EEE cap */
#define MDIO_EEE_1000T		MDIO_AN_EEE_ADV_1000T	/*
1000T EEE cap */
#define MDIO_EEE_10GT		0x0008	/* 10GT EEE cap */
#define MDIO_EEE_1000KX		0x0010	/* 1000KX EEE cap
*/
#define MDIO_EEE_10GKX4		0x0020	/* 10G KX4 EEE cap
*/
#define MDIO_EEE_10GKR		0x0040	/* 10G KR EEE cap
*/

I took care of only 2 in the case of realtek.c since it only support
MDIO_EEE_100TX and MDIO_EEE_1000T.

Defining a property for each is certainly doable but it does not look
very nice either. If it extends in the future, it will get even more
messier, especially if you want to disable everything.

What do you think about keeping a single mask value but use the define
above in the DT ? It would be more readable than hex and easy to
extend, don't you think ?

These defines are already part of the uapi so I guess we can use those
in the DT bindings ?

	Andrew

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

What I did not realize when doing this patch for the realtek driver is
that there is already 6 valid modes defined in the kernel

#define MDIO_EEE_100TX		MDIO_AN_EEE_ADV_100TX	/*
100TX EEE cap */
#define MDIO_EEE_1000T		MDIO_AN_EEE_ADV_1000T	/*
1000T EEE cap */
#define MDIO_EEE_10GT		0x0008	/* 10GT EEE cap */
#define MDIO_EEE_1000KX		0x0010	/* 1000KX EEE cap
*/
#define MDIO_EEE_10GKX4		0x0020	/* 10G KX4 EEE cap
*/
#define MDIO_EEE_10GKR		0x0040	/* 10G KR EEE cap
*/

I took care of only 2 in the case of realtek.c since it only support
MDIO_EEE_100TX and MDIO_EEE_1000T.

Defining a property for each is certainly doable but it does not look
very nice either. If it extends in the future, it will get even more
messier, especially if you want to disable everything.

Yes, agreed.
 

What do you think about keeping a single mask value but use the define
above in the DT ? It would be more readable than hex and easy to
extend, don't you think ?

These defines are already part of the uapi so I guess we can use those
in the DT bindings ?

I don't think they are accessible from the dtc include path. You will
need to make a copy, in include/dt-bindings/net/phy.h

But yes, using these defines is a good idea.

     Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Jerome,

On 21 November 2016 at 21:05, Jerome Brunet [off-list ref] wrote:

iperf3 tcp test summary at my end

Test Complete. Summary Results:
[ ID] Interval           Transfer     Bandwidth       Retr
[  4]   0.00-100.00 sec  10.9 GBytes   936 Mbits/sec    0             sender
[  4]   0.00-100.00 sec  10.9 GBytes   936 Mbits/sec                  receiver
CPU Utilization: local/sender 5.7% (0.2%u/5.5%s), remote/receiver
11.9% (0.9%u/11.0%s)

iperf3 udp test summary at my end.

Test Complete. Summary Results:
[ ID] Interval           Transfer     Bandwidth       Jitter
Lost/Total Datagrams
[  4]   0.00-100.00 sec  12.5 MBytes  1.05 Mbits/sec  0.025 ms  0/1599 (0%)
[  4] Sent 1599 datagrams
CPU Utilization: local/sender 0.1% (0.0%u/0.1%s), remote/receiver 0.0%
(0.0%u/0.0%s)

Best Regards
-Anand Moon

_______________________________________________
linux-amlogic mailing list
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Le 21/11/2016 à 08:47, Andrew Lunn a écrit :

quoted

What I did not realize when doing this patch for the realtek driver is
that there is already 6 valid modes defined in the kernel

#define MDIO_EEE_100TX		MDIO_AN_EEE_ADV_100TX	/*
100TX EEE cap */
#define MDIO_EEE_1000T		MDIO_AN_EEE_ADV_1000T	/*
1000T EEE cap */
#define MDIO_EEE_10GT		0x0008	/* 10GT EEE cap */
#define MDIO_EEE_1000KX		0x0010	/* 1000KX EEE cap
*/
#define MDIO_EEE_10GKX4		0x0020	/* 10G KX4 EEE cap
*/
#define MDIO_EEE_10GKR		0x0040	/* 10G KR EEE cap
*/

I took care of only 2 in the case of realtek.c since it only support
MDIO_EEE_100TX and MDIO_EEE_1000T.

Defining a property for each is certainly doable but it does not look
very nice either. If it extends in the future, it will get even more
messier, especially if you want to disable everything.

Yes, agreed.

One risk with the definition a group of advertisement capabilities
(under the form of a bitmask for instance) to enable/disable is that we
end up with Device Tree contain some kind of configuration policy as
opposed to just flagging particular hardware features as broken.

Fortunately, there does not seem to be a ton of PHYs out there which
require EEE to be disabled to function properly so having individual
properties vs. bitmasks/groups is kind of speculative here.

Another approach to solving this problem could be to register a PHY
fixup which disables EEE at the PHY level, and which is only called for
specific boards affected by this problem (of_machine_is_compatible()).
This code can leave in arch/*/* when that is possible, or it can just be
somewhere where it is relevant, e.g; in the PHY driver for instance
(similarly to how PCI fixups are done).
-- 
Florian

On Mon, 2016-11-21 at 21:35 -0800, Florian Fainelli wrote:

Le 21/11/2016 à 08:47, Andrew Lunn a écrit :

quoted

quoted

What I did not realize when doing this patch for the realtek
driver is
that there is already 6 valid modes defined in the kernel

#define MDIO_EEE_100TX		MDIO_AN_EEE_ADV_100TX	
/*
100TX EEE cap */
#define MDIO_EEE_1000T		MDIO_AN_EEE_ADV_1000T	
/*
1000T EEE cap */
#define MDIO_EEE_10GT		0x0008	/* 10GT EEE
cap */
#define MDIO_EEE_1000KX		0x0010	/* 1000KX
EEE cap
*/
#define MDIO_EEE_10GKX4		0x0020	/* 10G KX4
EEE cap
*/
#define MDIO_EEE_10GKR		0x0040	/* 10G KR EEE
cap
*/

I took care of only 2 in the case of realtek.c since it only
support
MDIO_EEE_100TX and MDIO_EEE_1000T.

Defining a property for each is certainly doable but it does not
look
very nice either. If it extends in the future, it will get even
more
messier, especially if you want to disable everything.

Yes, agreed.

One risk with the definition a group of advertisement capabilities
(under the form of a bitmask for instance) to enable/disable is that
we
end up with Device Tree contain some kind of configuration policy as
opposed to just flagging particular hardware features as broken.

The code proposed only allows to disable EEE advertisement (not
enable), so we should not see it used as a configuration policy in DT.
To make this more explicit, I could replace the property "eee-advert-
disable" by "eee-broken" ?

Fortunately, there does not seem to be a ton of PHYs out there which
require EEE

It is quite difficult to have the real picture here because some PHYs
have EEE disabled by default and you have to explicitly enable it.
I have no idea of the ratio between the 2 phy policies.

to be disabled to function properly so having individual
properties vs. bitmasks/groups is kind of speculative here.

In the particular instance of the OdroidC2, disabling EEE for GbE only
enough. However, If you have a PHY broken with, I think it is likely
that you might want to disable all (supported) EEE modes. That's reason
why I prefer bitmask. I agree both are functionally similar, this is
kind of a cosmetic debate.

Another approach to solving this problem could be to register a PHY
fixup which disables EEE at the PHY level, and which is only called
for
specific boards affected by this problem
(of_machine_is_compatible()).
This code can leave in arch/*/* when that is possible, 

That something I was looking at, but we don't have these files anymore
on ARM64 (looking at your comment, you already know this)

or it can just be
somewhere where it is relevant, e.g; in the PHY driver for instance
(similarly to how PCI fixups are done).

Do you prefer having board specific code inside generic driver than
having the setting living in DT? Peppe told me they also had a few
platform with similar issues. The point is that this could be useful to
other people, so it could spread a grow a bit.

I would prefer having this in the DT, but I can definitely do it the
PHY with of_machine_is_compatible() and register_fixup is this what you
prefer/want. 

Cheers
Jerome


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Jerome,

On Mon, Nov 21, 2016 at 4:35 PM, Jerome Brunet [off-list ref] wrote:

This patchset fixes an issue with the OdroidC2 board (DWMAC + RTL8211F).
Initially reported as a low Tx throughput issue at gigabit speed, the
platform enters LPI too often. This eventually break the link (both Tx
and Rx), and require to bring the interface down and up again to get the
Rx path working again.

The root cause of this issue is not fully understood yet but disabling EEE
advertisement on the PHY prevent this feature to be negotiated.
With this change, the link is stable and reliable, with the expected
throughput performance.

I have just sent a series which allows configuring the TX delay on the
MAC (dwmac-meson8b glue) side: [0]
Disabling the TX delay generated by the MAC fixes TX throughput for
me, even when leaving EEE enabled in the RTL8211F PHY driver!

Unfortunately the RTL8211F PHY is a black-box for the community
because there is no public datasheeet available.
*maybe* (pure speculation!) they're enabling the TX delay based on
some internal magic only when EEE is enabled.

Jerome, could you please re-test the behavior on your Odroid-C2 when
you have EEE still enabled but the TX-delay disabled?
In my case throughput is fine, and "$ ethtool -S eth0 | grep lpi" gives:
    irq_tx_path_in_lpi_mode_n: 0
    irq_tx_path_exit_lpi_mode_n: 0
    irq_rx_path_in_lpi_mode_n: 0
    irq_rx_path_exit_lpi_mode_n: 0


Regards,
Martin


[0] http://lists.infradead.org/pipermail/linux-amlogic/2016-November/001674.html

On Thu, 2016-11-24 at 15:40 +0100, Martin Blumenstingl wrote:

Hi Jerome,

On Mon, Nov 21, 2016 at 4:35 PM, Jerome Brunet [off-list ref]
wrote:

quoted

This patchset fixes an issue with the OdroidC2 board (DWMAC +
RTL8211F).
Initially reported as a low Tx throughput issue at gigabit speed,
the
platform enters LPI too often. This eventually break the link (both
Tx
and Rx), and require to bring the interface down and up again to
get the
Rx path working again.

The root cause of this issue is not fully understood yet but
disabling EEE
advertisement on the PHY prevent this feature to be negotiated.
With this change, the link is stable and reliable, with the
expected
throughput performance.

I have just sent a series which allows configuring the TX delay on
the
MAC (dwmac-meson8b glue) side: [0]
Disabling the TX delay generated by the MAC fixes TX throughput for
me, even when leaving EEE enabled in the RTL8211F PHY driver!

Unfortunately the RTL8211F PHY is a black-box for the community
because there is no public datasheeet available.
*maybe* (pure speculation!) they're enabling the TX delay based on
some internal magic only when EEE is enabled.

Hi already tried acting on the register setting the TX_delay. I also
tried on the PHY. I never been able to improve situation on the
Odroic2. Only disabling EEE improved the situation.

To make sure, i tried again with your patch but the result remains
unchanged. With Tx_delay disabled (either the mac or the phy), the
situation is even worse, it seems that nothing gets through

Jerome, could you please re-test the behavior on your Odroid-C2 when
you have EEE still enabled but the TX-delay disabled?
In my case throughput is fine, and "$ ethtool -S eth0 | grep lpi"
gives:
    irq_tx_path_in_lpi_mode_n: 0
    irq_tx_path_exit_lpi_mode_n: 0
    irq_rx_path_in_lpi_mode_n: 0
    irq_rx_path_exit_lpi_mode_n: 0

I still have lpi interrupts on my side. I don't get how a properly
configured tx_delay would disable EEE. I must be missing something
here.

Regards,
Martin


[0] http://lists.infradead.org/pipermail/linux-amlogic/2016-November/
001674.html

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Nov 24, 2016 at 5:01 PM, Jerome Brunet [off-list ref] wrote:

On Thu, 2016-11-24 at 15:40 +0100, Martin Blumenstingl wrote:

quoted

Hi Jerome,

On Mon, Nov 21, 2016 at 4:35 PM, Jerome Brunet [off-list ref]
wrote:

quoted

This patchset fixes an issue with the OdroidC2 board (DWMAC +
RTL8211F).
Initially reported as a low Tx throughput issue at gigabit speed,
the
platform enters LPI too often. This eventually break the link (both
Tx
and Rx), and require to bring the interface down and up again to
get the
Rx path working again.

The root cause of this issue is not fully understood yet but
disabling EEE
advertisement on the PHY prevent this feature to be negotiated.
With this change, the link is stable and reliable, with the
expected
throughput performance.

I have just sent a series which allows configuring the TX delay on
the
MAC (dwmac-meson8b glue) side: [0]
Disabling the TX delay generated by the MAC fixes TX throughput for
me, even when leaving EEE enabled in the RTL8211F PHY driver!

Unfortunately the RTL8211F PHY is a black-box for the community
because there is no public datasheeet available.
*maybe* (pure speculation!) they're enabling the TX delay based on
some internal magic only when EEE is enabled.

Hi already tried acting on the register setting the TX_delay. I also
tried on the PHY. I never been able to improve situation on the
Odroic2. Only disabling EEE improved the situation.

OK, thanks for clarifying this!

To make sure, i tried again with your patch but the result remains
unchanged. With Tx_delay disabled (either the mac or the phy), the
situation is even worse, it seems that nothing gets through

This is interesting, because in your case you should have a 4ns TX
delay (2ns from the MAC and presumably 2ns from the PHY).
Maybe that is also the reason why the TX delay is configurable in 2ns
steps in PRG_ETHERNET0 on Amlogic SoCs.

out of curiosity: have you tried setting a 4ns (half clock-cycle) TX
delay for the MAC and disabling it in the PHY?


Regards,
Martin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 2016-11-24 at 18:10 +0100, Martin Blumenstingl wrote:

On Thu, Nov 24, 2016 at 5:01 PM, Jerome Brunet [off-list ref]
wrote:

quoted

On Thu, 2016-11-24 at 15:40 +0100, Martin Blumenstingl wrote:

quoted

Hi Jerome,

On Mon, Nov 21, 2016 at 4:35 PM, Jerome Brunet <jbrunet@baylibre.
com>
wrote:

quoted

This patchset fixes an issue with the OdroidC2 board (DWMAC +
RTL8211F).
Initially reported as a low Tx throughput issue at gigabit
speed,
the
platform enters LPI too often. This eventually break the link
(both
Tx
and Rx), and require to bring the interface down and up again
to
get the
Rx path working again.

The root cause of this issue is not fully understood yet but
disabling EEE
advertisement on the PHY prevent this feature to be negotiated.
With this change, the link is stable and reliable, with the
expected
throughput performance.

I have just sent a series which allows configuring the TX delay
on
the
MAC (dwmac-meson8b glue) side: [0]
Disabling the TX delay generated by the MAC fixes TX throughput
for
me, even when leaving EEE enabled in the RTL8211F PHY driver!

Unfortunately the RTL8211F PHY is a black-box for the community
because there is no public datasheeet available.
*maybe* (pure speculation!) they're enabling the TX delay based
on
some internal magic only when EEE is enabled.

Hi already tried acting on the register setting the TX_delay. I
also
tried on the PHY. I never been able to improve situation on the
Odroic2. Only disabling EEE improved the situation.

OK, thanks for clarifying this!

quoted

To make sure, i tried again with your patch but the result remains
unchanged. With Tx_delay disabled (either the mac or the phy), the
situation is even worse, it seems that nothing gets through

This is interesting, because in your case you should have a 4ns TX
delay (2ns from the MAC and presumably 2ns from the PHY).
Maybe that is also the reason why the TX delay is configurable in 2ns
steps in PRG_ETHERNET0 on Amlogic SoCs.

out of curiosity: have you tried setting a 4ns (half clock-cycle) TX
delay for the MAC and disabling it in the PHY?

Just replied on the other thread. Long story short, Odroidc2 seems to
really require EEE to be switched off.

Again, thx for your help Martin

Regards,
Martin

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html