Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1526806 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH net v2 0/3] Fix OdroidC2 Gigabit Tx link issue Jerome Brunet <jbrunet@baylibre.com> - 2016-11-21 16:40 +0100
    [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy eee-disable-advert option documentation Jerome Brunet <jbrunet@baylibre.com> - 2016-11-21 16:40 +0100
      Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy  eee-disable-advert option documentation Andrew Lunn <andrew@lunn.ch> - 2016-11-21 17:10 +0100
        Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy  eee-disable-advert option documentation Jerome Brunet <jbrunet@baylibre.com> - 2016-11-21 17:20 +0100
          Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy  eee-disable-advert option documentation Andrew Lunn <andrew@lunn.ch> - 2016-11-21 17:50 +0100
            Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy  eee-disable-advert option documentation Florian Fainelli <f.fainelli@gmail.com> - 2016-11-22 06:40 +0100
              Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy  eee-disable-advert option documentation Jerome Brunet <jbrunet@baylibre.com> - 2016-11-22 11:20 +0100
    [RFC PATCH net v2 3/3] ARM64: dts: meson: odroidc2: disable advertisement EEE for GbE. Jerome Brunet <jbrunet@baylibre.com> - 2016-11-21 16:50 +0100
    Re: [RFC PATCH net v2 0/3] Fix OdroidC2 Gigabit Tx link issue Martin Blumenstingl <martin.blumenstingl@googlemail.com> - 2016-11-24 15:50 +0100
      Re: [RFC PATCH net v2 0/3] Fix OdroidC2 Gigabit Tx link issue Jerome Brunet <jbrunet@baylibre.com> - 2016-11-24 17:10 +0100
        Re: [RFC PATCH net v2 0/3] Fix OdroidC2 Gigabit Tx link issue Martin Blumenstingl <martin.blumenstingl@googlemail.com> - 2016-11-24 18:30 +0100

#1526806 — [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

[toc] | [next] | [standalone]

#1526808 — [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy eee-disable-advert option documentation

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

[toc] | [prev] | [next] | [standalone]

#1526854 — Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy eee-disable-advert option documentation

On Mon, Nov 21, 2016 at 04:35:23PM +0100, Jerome Brunet wrote:
> 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
> +

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

[toc] | [prev] | [next] | [standalone]

#1526863 — Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy eee-disable-advert option documentation

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:
> > 
> > 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
> > +
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1526898 — Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy eee-disable-advert option documentation

> 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

[toc] | [prev] | [next] | [standalone]

#1527238 — Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy eee-disable-advert option documentation

Le 21/11/2016 à 08:47, Andrew Lunn a écrit :
>> 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

[toc] | [prev] | [next] | [standalone]

#1527369 — Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy eee-disable-advert option documentation

On Mon, 2016-11-21 at 21:35 -0800, Florian Fainelli wrote:
> Le 21/11/2016 à 08:47, Andrew Lunn a écrit :
> > 
> > > 
> > > 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

[toc] | [prev] | [next] | [standalone]

#1526821 — [RFC PATCH net v2 3/3] ARM64: dts: meson: odroidc2: disable advertisement EEE for GbE.

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

[toc] | [prev] | [next] | [standalone]

#1529351

Hi Jerome,

On Mon, Nov 21, 2016 at 4:35 PM, Jerome Brunet <jbrunet@baylibre.com> 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

[toc] | [prev] | [next] | [standalone]

#1529522

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 <jbrunet@baylibre.com>
> 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.

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

[toc] | [prev] | [next] | [standalone]

#1529585

On Thu, Nov 24, 2016 at 5:01 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> 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 <jbrunet@baylibre.com>
>> 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.
>
> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web