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

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

[Jerome Brunet]

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

[RFC PATCH net v2 1/3] net: phy: add an option to disable EEE advertisement

[Jerome Brunet]

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

Re: [RFC PATCH net v2 1/3] net: phy: add an option to disable EEE advertisement

[Anand Moon]

Hi Jerome,

On 21 November 2016 at 21:05, Jerome Brunet <jbrunet@baylibre.com> wrote:
> 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
>

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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Jerome Brunet]

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

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

[Andrew Lunn]

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

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

[Jerome Brunet]

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

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

[Andrew Lunn]

> 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

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

[Florian Fainelli]

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

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

[Jerome Brunet]

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

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

[Jerome Brunet]

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

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

[Martin Blumenstingl]

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

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

[Jerome Brunet]

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

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

[Martin Blumenstingl]

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

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

[Jerome Brunet]

On Thu, 2016-11-24 at 18:10 +0100, Martin Blumenstingl wrote:
> 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?

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