| Message ID | 684cde92-a961-6221-6d59-113cd488ea98@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2,1/2] r8169: fix RTL8111EVL EEE and green settings | expand |
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.
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
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
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
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
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. */
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(-)