All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.