[PATCH v2 1/2] r8169: fix RTL8111EVL EEE and green settings

[PATCH v2 1/2] r8169: fix RTL8111EVL EEE and green settings

[Heiner Kallweit]

Name of functions rtl_w0w1_eri and rtl_w0w1_phy is somewhat misleading
regarding order of arguments. One could assume that w0w1 means
argument with bits to be reset comes before argument with bits to set.
However this is not the case.
So fix the order of arguments in several statements.

In addition fix EEE advertisement. The current code resets the bits
for 100BaseT and 1000BaseT EEE advertisement what is not what we want.

I have a little of a hard time to find a proper "Fixes" line as the
issue seems to have been there forever (at least it existed already
when the driver was moved to the current place in 2011).

The patch was tested on a Zotac Mini-PC with a RTL8111E-VL chip.
Before the patch EEE was disabled, now it's properly advertised and
works fine.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- no changes
---
 drivers/net/ethernet/realtek/r8169.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index dcb8c3938..19f3074a0 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -3789,26 +3789,26 @@ static void rtl8168e_2_hw_phy_config(struct rtl8169_private *tp)
 	rtl_writephy(tp, 0x1f, 0x0000);
 
 	/* EEE setting */
-	rtl_w0w1_eri(tp, 0x1b0, ERIAR_MASK_1111, 0x0000, 0x0003, ERIAR_EXGMAC);
+	rtl_w0w1_eri(tp, 0x1b0, ERIAR_MASK_1111, 0x0003, 0x0000, ERIAR_EXGMAC);
 	rtl_writephy(tp, 0x1f, 0x0005);
 	rtl_writephy(tp, 0x05, 0x8b85);
-	rtl_w0w1_phy(tp, 0x06, 0x0000, 0x2000);
+	rtl_w0w1_phy(tp, 0x06, 0x2000, 0x0000);
 	rtl_writephy(tp, 0x1f, 0x0004);
 	rtl_writephy(tp, 0x1f, 0x0007);
 	rtl_writephy(tp, 0x1e, 0x0020);
-	rtl_w0w1_phy(tp, 0x15, 0x0000, 0x0100);
+	rtl_w0w1_phy(tp, 0x15, 0x0100, 0x0000);
 	rtl_writephy(tp, 0x1f, 0x0002);
 	rtl_writephy(tp, 0x1f, 0x0000);
 	rtl_writephy(tp, 0x0d, 0x0007);
 	rtl_writephy(tp, 0x0e, 0x003c);
 	rtl_writephy(tp, 0x0d, 0x4007);
-	rtl_writephy(tp, 0x0e, 0x0000);
+	rtl_writephy(tp, 0x0e, 0x0006);
 	rtl_writephy(tp, 0x0d, 0x0000);
 
 	/* Green feature */
 	rtl_writephy(tp, 0x1f, 0x0003);
-	rtl_w0w1_phy(tp, 0x19, 0x0000, 0x0001);
-	rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0400);
+	rtl_w0w1_phy(tp, 0x19, 0x0001, 0x0000);
+	rtl_w0w1_phy(tp, 0x10, 0x0400, 0x0000);
 	rtl_writephy(tp, 0x1f, 0x0000);
 
 	/* Broken BIOS workaround: feed GigaMAC registers with MAC address. */
-- 
2.15.0

[PATCH v2 2/2] r8169: use same RTL8111EVL green settings as in vendor driver

[Heiner Kallweit]

Adjust the code to use the same green settings as in the latest
vendor driver.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- replace magic numbers with constants
---
 drivers/net/ethernet/realtek/r8169.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 19f3074a0..2cb3622c4 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -3810,6 +3810,11 @@ static void rtl8168e_2_hw_phy_config(struct rtl8169_private *tp)
 	rtl_w0w1_phy(tp, 0x19, 0x0001, 0x0000);
 	rtl_w0w1_phy(tp, 0x10, 0x0400, 0x0000);
 	rtl_writephy(tp, 0x1f, 0x0000);
+	rtl_writephy(tp, 0x1f, 0x0005);
+	rtl_w0w1_phy(tp, 0x01, 0x0100, 0x0000);
+	rtl_writephy(tp, 0x1f, 0x0000);
+	/* soft-reset phy */
+	rtl_writephy(tp, MII_BMCR, BMCR_RESET | BMCR_ANENABLE | BMCR_ANRESTART);
 
 	/* Broken BIOS workaround: feed GigaMAC registers with MAC address. */
 	rtl_rar_exgmac_set(tp, tp->dev->dev_addr);
-- 
2.15.0

Re: [PATCH v2 1/2] r8169: fix RTL8111EVL EEE and green settings

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sun, 19 Nov 2017 11:09:58 +0100

> Name of functions rtl_w0w1_eri and rtl_w0w1_phy is somewhat misleading
> regarding order of arguments. One could assume that w0w1 means
> argument with bits to be reset comes before argument with bits to set.
> However this is not the case.
> So fix the order of arguments in several statements.
> 
> In addition fix EEE advertisement. The current code resets the bits
> for 100BaseT and 1000BaseT EEE advertisement what is not what we want.
> 
> I have a little of a hard time to find a proper "Fixes" line as the
> issue seems to have been there forever (at least it existed already
> when the driver was moved to the current place in 2011).
> 
> The patch was tested on a Zotac Mini-PC with a RTL8111E-VL chip.
> Before the patch EEE was disabled, now it's properly advertised and
> works fine.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied.

Re: [PATCH v2 2/2] r8169: use same RTL8111EVL green settings as in vendor driver

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sun, 19 Nov 2017 11:15:46 +0100

> Adjust the code to use the same green settings as in the latest
> vendor driver.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied.

Re: [PATCH v2 1/2] r8169: fix RTL8111EVL EEE and green settings

[Andrew Lunn]

Hi Heiner

Do you have access to the data sheet?

I had a quick look through the driver. It would be nice to refactor it
to follow the usual Linux conventions:

Turn the MDIO read/write functions into an MDIO bus driver.

Move the PHY code into drivers/net/phy/realtek.c, and in the process,
replace all the magic numbers with #defines.

Do you have any interest in doing this?

   Andrew

Re: [PATCH v2 1/2] r8169: fix RTL8111EVL EEE and green settings

[Heiner Kallweit]

Am 21.11.2017 um 02:34 schrieb Andrew Lunn:
> Hi Heiner
Hi Andrew,

> 
> Do you have access to the data sheet?
> 
Not to more recent ones. I only have two older data sheets for early
members of the rtl8169 family.

> I had a quick look through the driver. It would be nice to refactor it
> to follow the usual Linux conventions:
> 
> Turn the MDIO read/write functions into an MDIO bus driver.
> 
I thought the same when looking at this driver.
It's a nightmare to maintain a driver with almost 9.000 lines of code
and numerous "switch mac_id" clauses.
Not sure whether still members of this chip family are being
developed that we may have to add in future.

My first thought was to factor out support for the original 8169 family
(mac id <= 6) into a separate driver as first step as it differs
significantly from later members of the chip family (e.g. TBI support
in addition to MII).

> Move the PHY code into drivers/net/phy/realtek.c, and in the process,
> replace all the magic numbers with #defines.
> 
Yes, this definitely would be desirable. However I found that the
available datasheets for the external PHY's usually only document
the registers on page 0. All the magic settings on other pages often
are just copied from vendor drivers.
Or are there other, more comprehensive versions of the datasheets
being available under NDA?

> Do you have any interest in doing this?
> 
Would be a nice challenge. I'm willing to look into this if I can
get hold of the official datasheets.

>    Andrew
> 
Heiner

Re: [PATCH v2 1/2] r8169: fix RTL8111EVL EEE and green settings

[Heiner Kallweit]

Am 21.11.2017 um 02:34 schrieb Andrew Lunn:
> Hi Heiner
> 
> Do you have access to the data sheet?
> 
> I had a quick look through the driver. It would be nice to refactor it
> to follow the usual Linux conventions:
> 
> Turn the MDIO read/write functions into an MDIO bus driver.
> 
> Move the PHY code into drivers/net/phy/realtek.c, and in the process,
> replace all the magic numbers with #defines.
> 
> Do you have any interest in doing this?
> 
>    Andrew
> 
Hi Andrew,

I worked a little on this topic and meanwhile have an experimental
patch set for switching the driver to phylib, incl. MDIO bus driver.
It works well on a RTL8168evl (RTL_GIGA_MAC_VER_34).
Will submit this patch set as RfC in the next days.

Still open is factoring out all phy init stuff to drivers/net/phy.
There are open issues where I would appreciate advice from the
Realtek guys.

The PHY in RTL8168evl identifies as RTL8211E. Question would be
whether such an internal PHY with this id is identical to an
external PHY with the same id.

For most NIC's the driver loads firmware. The firmware can refer
to PHY and MAC as well. In case of RTL8168evl the firmware
(rtl8168e-3.fw) seems to be for the PHY only. So next question
is whether this firmware would be applicable for any RTL8211E
external PHY too.
In this case one option would be to move the firmware handling to
drivers/firmware so that it can be used from PHY drivers and from
NIC drivers as well.

Regards, Heiner

Re: [PATCH v2 1/2] r8169: fix RTL8111EVL EEE and green settings

[Andrew Lunn]

On Sat, Dec 16, 2017 at 06:11:01PM +0100, Heiner Kallweit wrote:
> Am 21.11.2017 um 02:34 schrieb Andrew Lunn:
> > Hi Heiner
> > 
> > Do you have access to the data sheet?
> > 
> > I had a quick look through the driver. It would be nice to refactor it
> > to follow the usual Linux conventions:
> > 
> > Turn the MDIO read/write functions into an MDIO bus driver.
> > 
> > Move the PHY code into drivers/net/phy/realtek.c, and in the process,
> > replace all the magic numbers with #defines.
> > 
> > Do you have any interest in doing this?
> > 
> >    Andrew
> > 
> Hi Andrew,
> 
> I worked a little on this topic and meanwhile have an experimental
> patch set for switching the driver to phylib, incl. MDIO bus driver.
> It works well on a RTL8168evl (RTL_GIGA_MAC_VER_34).
> Will submit this patch set as RfC in the next days.
> 
> Still open is factoring out all phy init stuff to drivers/net/phy.
> There are open issues where I would appreciate advice from the
> Realtek guys.

Hi Heiner

> The PHY in RTL8168evl identifies as RTL8211E. Question would be
> whether such an internal PHY with this id is identical to an
> external PHY with the same id.

I cannot make a 100% reliable recommendation, but i would say it is
likely the internal and the external PHY are compatible. I've seen
similar situations with Marvell Ethernet switches with Internal PHYs,
which use the same ID as discreet PHYs, and the same driver has
worked.

> In this case one option would be to move the firmware handling to
> drivers/firmware so that it can be used from PHY drivers and from
> NIC drivers as well.

If the core is the same for MAC and PHY, then moving it somewhere it
can be shared would be good.

    Andrew