* [PATCH] r8169: RxConfig hack for the 8168evl. @ 2012-06-20 22:09 Francois Romieu 2012-06-23 4:49 ` David Miller 2012-06-25 3:31 ` hayeswang 0 siblings, 2 replies; 6+ messages in thread From: Francois Romieu @ 2012-06-20 22:09 UTC (permalink / raw) To: Hayes Wang; +Cc: netdev, thomas.pi The 8168evl (RTL_GIGA_MAC_VER_34) based Gigabyte GA-990FXA motherboards are very prone to NETDEV watchdog problems without this change. See https://bugzilla.kernel.org/show_bug.cgi?id=42899 for instance. I don't know why it *works*. It's depressingly effective though. For the record: - the problem may go along IOMMU (AMD-Vi) errors but it really looks like a red herring. - the patch sets the RX_MULTI_EN bit. If the 8168c doc is any guide, the chipset now fetches several Rx descriptors at a time. - long ago the driver ignored the RX_MULTI_EN bit. e542a2269f232d61270ceddd42b73a4348dee2bb changed the RxConfig settings. Whatever the problem it's now labeled a regression. - Realtek's own driver can identify two different 8168evl devices (CFG_METHOD_16 and CFG_METHOD_17) where the r8169 driver only sees one. It sucks. Signed-off-by: Francois Romieu <romieu@fr.zoreil.com> --- Hayes, any hindsight ? drivers/net/ethernet/realtek/r8169.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index 7260aa7..d7a04e0 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -3894,6 +3894,7 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp) case RTL_GIGA_MAC_VER_22: case RTL_GIGA_MAC_VER_23: case RTL_GIGA_MAC_VER_24: + case RTL_GIGA_MAC_VER_34: RTL_W32(RxConfig, RX128_INT_EN | RX_MULTI_EN | RX_DMA_BURST); break; default: -- 1.7.10.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] r8169: RxConfig hack for the 8168evl. 2012-06-20 22:09 [PATCH] r8169: RxConfig hack for the 8168evl Francois Romieu @ 2012-06-23 4:49 ` David Miller 2012-06-25 3:31 ` hayeswang 1 sibling, 0 replies; 6+ messages in thread From: David Miller @ 2012-06-23 4:49 UTC (permalink / raw) To: romieu; +Cc: hayeswang, netdev, thomas.pi From: Francois Romieu <romieu@fr.zoreil.com> Date: Thu, 21 Jun 2012 00:09:18 +0200 > The 8168evl (RTL_GIGA_MAC_VER_34) based Gigabyte GA-990FXA motherboards > are very prone to NETDEV watchdog problems without this change. See > https://bugzilla.kernel.org/show_bug.cgi?id=42899 for instance. > > I don't know why it *works*. It's depressingly effective though. > > For the record: > - the problem may go along IOMMU (AMD-Vi) errors but it really looks > like a red herring. > - the patch sets the RX_MULTI_EN bit. If the 8168c doc is any guide, > the chipset now fetches several Rx descriptors at a time. > - long ago the driver ignored the RX_MULTI_EN bit. > e542a2269f232d61270ceddd42b73a4348dee2bb changed the RxConfig > settings. Whatever the problem it's now labeled a regression. > - Realtek's own driver can identify two different 8168evl devices > (CFG_METHOD_16 and CFG_METHOD_17) where the r8169 driver only > sees one. It sucks. > > Signed-off-by: Francois Romieu <romieu@fr.zoreil.com> Applied, thanks Francois. ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] r8169: RxConfig hack for the 8168evl. 2012-06-20 22:09 [PATCH] r8169: RxConfig hack for the 8168evl Francois Romieu 2012-06-23 4:49 ` David Miller @ 2012-06-25 3:31 ` hayeswang 2012-06-26 9:22 ` Francois Romieu 1 sibling, 1 reply; 6+ messages in thread From: hayeswang @ 2012-06-25 3:31 UTC (permalink / raw) To: 'Francois Romieu'; +Cc: netdev, thomas.pi Hi, > - the patch sets the RX_MULTI_EN bit. If the 8168c doc is any guide, > the chipset now fetches several Rx descriptors at a time. > - long ago the driver ignored the RX_MULTI_EN bit. > e542a2269f232d61270ceddd42b73a4348dee2bb changed the RxConfig > settings. Whatever the problem it's now labeled a regression. The definition of the IO 0x44 bit 14 is opposite for new chips. For 8111C, 0 means fetching one Rx descriptor, and 1 means fetching multi-descriptors. For 8111D and the later chips, 0 means fetching multi-descriptors, and 1 means fetching one Rx descriptor. However, I have no idea about why it influences the issue. > - Realtek's own driver can identify two different 8168evl devices > (CFG_METHOD_16 and CFG_METHOD_17) where the r8169 driver only > sees one. It sucks. The CFG_METHOD_16 is the internal test chip. We don't have mass production for it. Even it could be removed from driver. I don't think the kernel have to support it. Best Regards, Hayes ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] r8169: RxConfig hack for the 8168evl. 2012-06-25 3:31 ` hayeswang @ 2012-06-26 9:22 ` Francois Romieu 2012-06-26 23:41 ` David Miller 2012-06-27 2:43 ` hayeswang 0 siblings, 2 replies; 6+ messages in thread From: Francois Romieu @ 2012-06-26 9:22 UTC (permalink / raw) To: hayeswang; +Cc: netdev, thomas.pi hayeswang <hayeswang@realtek.com> : [...] > The definition of the IO 0x44 bit 14 is opposite for new chips. > For 8111C, 0 means fetching one Rx descriptor, and 1 means fetching > multi-descriptors. > For 8111D and the later chips, 0 means fetching multi-descriptors, and 1 means > fetching one Rx descriptor. Ok. Is there much point fetching one Rx descriptor versus several ? [...] > The CFG_METHOD_16 is the internal test chip. We don't have mass production for > it. Even it could be removed from driver. I don't think the kernel have to > support it. Ok. There seem to be a few differences for the CFG_METHOD_16 chipset between the kernel driver and Realtek's own. I have noticed the points below. Should some of those be included ? Thanks. Subject: [PATCH] r8169: narrow 8168evl support. Some bits taken from the comparison with Realtek's 8.031.00 8168 driver - 0x7cf00000 / 0x2c900000 is a Realtek internal, test-only chipset - rtl8168evl_reset_packet_filter is only there for documentation purpose (no change) - TDFNR ? - the Magic Packet feature should be set and read differently - r8168_pll_power_up tweak - what is the mysterious 0xf2 register for in rtl_hw_start_8168e_2 ? - hardware checksum offloading fix for small packets Signed-off-by: Francois Romieu <romieu@fr.zoreil.com> Cc: Hayes Wang <hayeswang@realtek.com> --- drivers/net/ethernet/realtek/r8169.c | 87 ++++++++++++++++++++++++---------- 1 file changed, 62 insertions(+), 25 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index 7260aa7..ad6bcf6 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -364,6 +364,8 @@ enum rtl8110_registers { }; enum rtl8168_8101_registers { + TDFNR = 0x57, /* Transmit descriptor fetch number. */ +#define TDFNR_MASK 0x3f CSIDR = 0x64, CSIAR = 0x68, #define CSIAR_FLAG 0x80000000 @@ -1265,6 +1267,12 @@ static void rtl8169_xmii_reset_enable(struct rtl8169_private *tp) rtl_writephy(tp, MII_BMCR, val & 0xffff); } +static void rtl8168evl_reset_packet_filter(void __iomem *ioaddr) +{ + rtl_w1w0_eri(ioaddr, 0xdc, ERIAR_MASK_0001, 0x00, 0x01, ERIAR_EXGMAC); + rtl_w1w0_eri(ioaddr, 0xdc, ERIAR_MASK_0001, 0x01, 0x00, ERIAR_EXGMAC); +} + static void rtl_link_chg_patch(struct rtl8169_private *tp) { void __iomem *ioaddr = tp->mmio_addr; @@ -1291,11 +1299,7 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp) rtl_eri_write(ioaddr, 0x1dc, ERIAR_MASK_1111, 0x0000003f, ERIAR_EXGMAC); } - /* Reset packet filter */ - rtl_w1w0_eri(ioaddr, 0xdc, ERIAR_MASK_0001, 0x00, 0x01, - ERIAR_EXGMAC); - rtl_w1w0_eri(ioaddr, 0xdc, ERIAR_MASK_0001, 0x01, 0x00, - ERIAR_EXGMAC); + rtl8168evl_reset_packet_filter(ioaddr); } else if (tp->mac_version == RTL_GIGA_MAC_VER_35 || tp->mac_version == RTL_GIGA_MAC_VER_36) { if (RTL_R8(PHYstatus) & _1000bpsF) { @@ -1355,7 +1359,7 @@ static u32 __rtl8169_get_wol(struct rtl8169_private *tp) { void __iomem *ioaddr = tp->mmio_addr; u8 options; - u32 wolopts = 0; + u32 wolopts = 0, csi; options = RTL_R8(Config1); if (!(options & PMEnable)) @@ -1364,8 +1368,18 @@ static u32 __rtl8169_get_wol(struct rtl8169_private *tp) options = RTL_R8(Config3); if (options & LinkUp) wolopts |= WAKE_PHY; - if (options & MagicPacket) - wolopts |= WAKE_MAGIC; + + switch (tp->mac_version) { + case RTL_GIGA_MAC_VER_34: + csi = rtl_eri_read(ioaddr, 0xde, ERIAR_EXGMAC); + if (csi & 0x01) + wolopts |= WAKE_MAGIC; + break; + default: + if (options & MagicPacket) + wolopts |= WAKE_MAGIC; + break; + } options = RTL_R8(Config5); if (options & UWF) @@ -1399,24 +1413,19 @@ static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts) u16 reg; u8 mask; } cfg[] = { - { WAKE_PHY, Config3, LinkUp }, { WAKE_MAGIC, Config3, MagicPacket }, + { WAKE_PHY, Config3, LinkUp }, { WAKE_UCAST, Config5, UWF }, { WAKE_BCAST, Config5, BWF }, { WAKE_MCAST, Config5, MWF }, { WAKE_ANY, Config5, LanWake } }; + int start = 0; u8 options; + u32 csi; RTL_W8(Cfg9346, Cfg9346_Unlock); - for (i = 0; i < ARRAY_SIZE(cfg); i++) { - options = RTL_R8(cfg[i].reg) & ~cfg[i].mask; - if (wolopts & cfg[i].opt) - options |= cfg[i].mask; - RTL_W8(cfg[i].reg, options); - } - switch (tp->mac_version) { case RTL_GIGA_MAC_VER_01 ... RTL_GIGA_MAC_VER_17: options = RTL_R8(Config1) & ~PMEnable; @@ -1424,6 +1433,13 @@ static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts) options |= PMEnable; RTL_W8(Config1, options); break; + case RTL_GIGA_MAC_VER_34: + csi = rtl_eri_read(ioaddr, 0xde, ERIAR_EXGMAC) & ~0x01; + if (wolopts & WAKE_MAGIC) + csi |= 0x01; + rtl_eri_write(ioaddr, 0xde, 4, csi, ERIAR_EXGMAC); + + start++; default: options = RTL_R8(Config2) & ~PME_SIGNAL; if (wolopts) @@ -1432,6 +1448,13 @@ static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts) break; } + for (i = start; i < ARRAY_SIZE(cfg); i++) { + options = RTL_R8(cfg[i].reg) & ~cfg[i].mask; + if (wolopts & cfg[i].opt) + options |= cfg[i].mask; + RTL_W8(cfg[i].reg, options); + } + RTL_W8(Cfg9346, Cfg9346_Lock); } @@ -1900,7 +1923,7 @@ static void rtl8169_get_mac_version(struct rtl8169_private *tp, { 0x7cf00000, 0x48000000, RTL_GIGA_MAC_VER_35 }, /* 8168E family. */ - { 0x7c800000, 0x2c800000, RTL_GIGA_MAC_VER_34 }, + { 0x7cf00000, 0x2c800000, RTL_GIGA_MAC_VER_34 }, { 0x7cf00000, 0x2c200000, RTL_GIGA_MAC_VER_33 }, { 0x7cf00000, 0x2c100000, RTL_GIGA_MAC_VER_32 }, { 0x7c800000, 0x2c000000, RTL_GIGA_MAC_VER_33 }, @@ -3778,6 +3801,9 @@ static void r8168_pll_power_down(struct rtl8169_private *tp) case RTL_GIGA_MAC_VER_33: RTL_W8(PMCH, RTL_R8(PMCH) & ~0x80); break; + case RTL_GIGA_MAC_VER_34: + RTL_W8(PMCH, RTL_R8(PMCH) & ~0xc0); + break; } } @@ -3795,6 +3821,9 @@ static void r8168_pll_power_up(struct rtl8169_private *tp) case RTL_GIGA_MAC_VER_33: RTL_W8(PMCH, RTL_R8(PMCH) | 0x80); break; + case RTL_GIGA_MAC_VER_34: + RTL_W8(PMCH, RTL_R8(PMCH) | 0xc0); + break; } r8168_phy_power_up(tp); @@ -4797,6 +4826,9 @@ static void rtl_hw_start_8168e_2(struct rtl8169_private *tp) RTL_W8(MaxTxPacketSize, EarlySize); + RTL_W8(0xf2, (RTL_R8(0xf2) & ~0x02) | 0x05); + RTL_W8(TDFNR, (RTL_R8(TDFNR) & ~TDFNR_MASK) | 0x8); + rtl_disable_clock_request(pdev); RTL_W32(TxConfig, RTL_R32(TxConfig) | TXCFG_AUTO_FIFO); @@ -5474,14 +5506,19 @@ static inline void rtl8169_tso_csum(struct rtl8169_private *tp, opts[0] |= TD_LSO; opts[offset] |= min(mss, TD_MSS_MAX) << info->mss_shift; } else if (skb->ip_summed == CHECKSUM_PARTIAL) { - const struct iphdr *ip = ip_hdr(skb); - - if (ip->protocol == IPPROTO_TCP) - opts[offset] |= info->checksum.tcp; - else if (ip->protocol == IPPROTO_UDP) - opts[offset] |= info->checksum.udp; - else - WARN_ON_ONCE(1); + if (likely(skb-> len >= 60 || + (tp->mac_version != RTL_GIGA_MAC_VER_34))) { + const struct iphdr *ip = ip_hdr(skb); + + if (ip->protocol == IPPROTO_TCP) + opts[offset] |= info->checksum.tcp; + else if (ip->protocol == IPPROTO_UDP) + opts[offset] |= info->checksum.udp; + else + WARN_ON_ONCE(1); + } else { + skb_checksum_help(skb); + } } } -- 1.7.10.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] r8169: RxConfig hack for the 8168evl. 2012-06-26 9:22 ` Francois Romieu @ 2012-06-26 23:41 ` David Miller 2012-06-27 2:43 ` hayeswang 1 sibling, 0 replies; 6+ messages in thread From: David Miller @ 2012-06-26 23:41 UTC (permalink / raw) To: romieu; +Cc: hayeswang, netdev, thomas.pi From: Francois Romieu <romieu@fr.zoreil.com> Date: Tue, 26 Jun 2012 11:22:51 +0200 > hayeswang <hayeswang@realtek.com> : > [...] >> The definition of the IO 0x44 bit 14 is opposite for new chips. >> For 8111C, 0 means fetching one Rx descriptor, and 1 means fetching >> multi-descriptors. >> For 8111D and the later chips, 0 means fetching multi-descriptors, and 1 means >> fetching one Rx descriptor. > > Ok. Is there much point fetching one Rx descriptor versus several ? It can help if the chip accesses enough at a time to fill a full cache line. In drivers I've written for chips that can do this, I've had the code only post RX descriptors in chunks rather than one at a time, to facilitate this even further. ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] r8169: RxConfig hack for the 8168evl. 2012-06-26 9:22 ` Francois Romieu 2012-06-26 23:41 ` David Miller @ 2012-06-27 2:43 ` hayeswang 1 sibling, 0 replies; 6+ messages in thread From: hayeswang @ 2012-06-27 2:43 UTC (permalink / raw) To: 'Francois Romieu'; +Cc: netdev, thomas.pi > From: Francois Romieu [mailto:romieu@fr.zoreil.com] [...] > > The definition of the IO 0x44 bit 14 is opposite for new chips. > > For 8111C, 0 means fetching one Rx descriptor, and 1 means fetching > > multi-descriptors. > > For 8111D and the later chips, 0 means fetching > multi-descriptors, and 1 means > > fetching one Rx descriptor. > > Ok. Is there much point fetching one Rx descriptor versus several ? I just know fetching several Rx descriptor has better performance if the decsriptors are enough. However, I have no idea about how the hardware implements it. > > [...] > > The CFG_METHOD_16 is the internal test chip. We don't have > mass production for > > it. Even it could be removed from driver. I don't think the > kernel have to > > support it. > > Ok. > > There seem to be a few differences for the CFG_METHOD_16 > chipset between > the kernel driver and Realtek's own. I have noticed the points below. > Should some of those be included ? I think you should reference the CFG_METHOD_17, not CFG_METHOD_16. [...] > - 0x7cf00000 / 0x2c900000 is a Realtek internal, test-only chipset Should be 0x7cf00000 / 0x2c800000 [...] > rtl8169_get_mac_version(struct rtl8169_private *tp, > { 0x7cf00000, 0x48000000, RTL_GIGA_MAC_VER_35 }, > > /* 8168E family. */ > - { 0x7c800000, 0x2c800000, RTL_GIGA_MAC_VER_34 }, > + { 0x7cf00000, 0x2c800000, RTL_GIGA_MAC_VER_34 }, Should be { 0x7cf00000, 0x2c900000, RTL_GIGA_MAC_VER_34 }, [...] > @@ -4797,6 +4826,9 @@ static void rtl_hw_start_8168e_2(struct > rtl8169_private *tp) > > RTL_W8(MaxTxPacketSize, EarlySize); > > + RTL_W8(0xf2, (RTL_R8(0xf2) & ~0x02) | 0x05); Please remove this line. We verify it would case kernel panic. [...] Best Regards, Hayes ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-06-27 2:43 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-06-20 22:09 [PATCH] r8169: RxConfig hack for the 8168evl Francois Romieu 2012-06-23 4:49 ` David Miller 2012-06-25 3:31 ` hayeswang 2012-06-26 9:22 ` Francois Romieu 2012-06-26 23:41 ` David Miller 2012-06-27 2:43 ` hayeswang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.