2004-01-24 01:36:27

by Grant Grundler

[permalink] [raw]
Subject: [PATCH] 2.6.1 tg3 DMA engine test failure

Jeff, Dave,
tg3 patch to resolve another problem with bcm5701 card running at 33Mhz
on an ia64 rx4640 using a to-be-released system firmware (new feature
is PCI hotplug).

The failure message was "DMA engine test failure".
The problem was the read-back (DMA Write test) failed completely.
No data was transferred back to the host.

The orginal failure was seen on RHAS 3.0 (tg3 v2.2) but the
tg3_test_dma() code is the same for 2.6.1 kernel. The appended
patch addresses three issues:

1) Don't reset subcomponents. Broadcom indicated only the GRC reset
has been tested and "unpredictable behavior" could happen
by resetting subcomponents. Removing the RDMAC/WDMAC reset
fixed the "DMA engine test failure".

2) I've "improved" the test by verifying the data on the card
is what we put into the buffer initially. I.E. the test can
now differentiate between outbound and inbound corruptions.

3) Broadcom engineer noted the meaning of DMA_RWCTRL_ASSERT_ALL_BE
has changed for bcm570[34] and also advised against setting
it on BCM570[01] chips. I'm just implementing his advice.
Comment below spells out more details.

The patch was tested on rx2600 (ia64) and a500 (parisc).

thanks,
grant

===== drivers/net/tg3.c 1.120 vs edited =====
--- 1.120/drivers/net/tg3.c Mon Dec 22 00:04:23 2003
+++ edited/drivers/net/tg3.c Thu Jan 22 20:51:15 2004
@@ -7190,26 +7190,33 @@
test_desc.addr_lo = buf_dma & 0xffffffff;
test_desc.nic_mbuf = 0x00002100;
test_desc.len = size;
+
+ /*
+ * HP ZX1 was seeing test failures for 5701 cards running at 33Mhz
+ * the *second* time the tg3 driver was getting loaded after an
+ * initial scan.
+ *
+ * Broadcom tells me:
+ * ...the DMA engine is connected to the GRC block and a DMA
+ * reset may affect the GRC block in some unpredictable way...
+ * The behavior of resets to individual blocks has not been tested.
+ *
+ * Broadcom noted the GRC reset will also reset all sub-components.
+ */
if (to_device) {
test_desc.cqid_sqid = (13 << 8) | 2;
- tw32(RDMAC_MODE, RDMAC_MODE_RESET);
- tr32(RDMAC_MODE);
- udelay(40);

tw32(RDMAC_MODE, RDMAC_MODE_ENABLE);
tr32(RDMAC_MODE);
udelay(40);
} else {
test_desc.cqid_sqid = (16 << 8) | 7;
- tw32(WDMAC_MODE, WDMAC_MODE_RESET);
- tr32(WDMAC_MODE);
- udelay(40);

tw32(WDMAC_MODE, WDMAC_MODE_ENABLE);
tr32(WDMAC_MODE);
udelay(40);
}
- test_desc.flags = 0x00000004;
+ test_desc.flags = 0x00000005;

for (i = 0; i < (sizeof(test_desc) / sizeof(u32)); i++) {
u32 val;
@@ -7368,10 +7375,27 @@
GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5701) {
/* Remove this if it causes problems for some boards. */
tp->dma_rwctrl |= DMA_RWCTRL_USE_MEM_READ_MULT;
+ } else
+ if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5703 ||
+ GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5704) {
+ /* More words of wisdom from Broadcom:
+ * By setting this bit 23 in register 0x6c, all DMA
+ * writes (to host) will have the byte enables asserted
+ * at the beginning and the end of the buffer. This means
+ * that if the skbuff starts at host address xxx2, the
+ * DMA will write 2 bytes of garbage to xxx0. The same
+ * thing will happen at the end of the buffer.
+ * While this may not cause any real problem because
+ * buffers may have extra padding, I think it is better
+ * to DMA the exact amount of data.
+ *
+ * On the 5703 and 5704, this bit has been redefined
+ * to enable a PCIX-related bug fix. Setting it to 1
+ * means the bug fix is enabled.
+ */
+ tp->dma_rwctrl |= DMA_RWCTRL_ASSERT_ALL_BE;
}

- tp->dma_rwctrl |= DMA_RWCTRL_ASSERT_ALL_BE;
-
tw32(TG3PCI_DMA_RW_CTRL, tp->dma_rwctrl);

#if 0
@@ -7385,28 +7409,38 @@
goto out;

while (1) {
- u32 *p, i;
+ u32 *p = buf, i;

- p = buf;
for (i = 0; i < TEST_BUFFER_SIZE / sizeof(u32); i++)
p[i] = i;

/* Send the buffer to the chip. */
ret = tg3_do_test_dma(tp, buf, buf_dma, TEST_BUFFER_SIZE, 1);
- if (ret)
+ if (ret) {
+ printk(KERN_ERR "tg3_test_dma() Write the buffer failed %d\n", ret);
break;
+ }

- p = buf;
- for (i = 0; i < TEST_BUFFER_SIZE / sizeof(u32); i++)
+ /* validate data reached card RAM correctly. */
+ for (i = 0; i < TEST_BUFFER_SIZE / sizeof(u32); i++) {
+ u32 val;
+ tg3_read_mem(tp, 0x2100 + (i*4), &val);
+ if (val != p[i]) {
+ printk( KERN_ERR " tg3_test_dma() Card buffer currupted on write! (%d != %d)\n", val, i);
+ /* ret = -ENODEV here? */
+ }
p[i] = 0;
+ }

/* Now read it back. */
ret = tg3_do_test_dma(tp, buf, buf_dma, TEST_BUFFER_SIZE, 0);
- if (ret)
+ if (ret) {
+ printk(KERN_ERR "tg3_test_dma() Read the buffer failed %d\n", ret);
+
break;
+ }

/* Verify it. */
- p = buf;
for (i = 0; i < TEST_BUFFER_SIZE / sizeof(u32); i++) {
if (p[i] == i)
continue;
@@ -7417,6 +7451,7 @@
tw32(TG3PCI_DMA_RW_CTRL, tp->dma_rwctrl);
break;
} else {
+ printk(KERN_ERR "tg3_test_dma() buffer corrupted on read back! (%d != %d)\n", p[i], i);
ret = -ENODEV;
goto out;
}


2004-01-24 05:09:13

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] 2.6.1 tg3 DMA engine test failure

From: Grant Grundler <[email protected]>
Date: Fri, 23 Jan 2004 18:36:14 -0700

3) Broadcom engineer noted the meaning of DMA_RWCTRL_ASSERT_ALL_BE
has changed for bcm570[34] and also advised against setting
it on BCM570[01] chips. I'm just implementing his advice.
Comment below spells out more details.

Setting this bit is absolutely required on many RISC PCI boxes, where
streaming mappings must have cacheline sized DMA transactions done
on them with all byte enables on.

In fact, since the later chips don't allow controlling this, some of
them cause streaming byte hole errors on sparc64 and other RISC
systems when they do cacheline sized DMA to streaming DMA mappings
with not all the byte enables on.

So I'm not going to add this part of your changes.

2004-01-24 07:30:40

by Grant Grundler

[permalink] [raw]
Subject: Re: [PATCH] 2.6.1 tg3 DMA engine test failure

On Fri, Jan 23, 2004 at 09:00:23PM -0800, David S. Miller wrote:
> From: Grant Grundler <[email protected]>
> Date: Fri, 23 Jan 2004 18:36:14 -0700
>
> 3) Broadcom engineer noted the meaning of DMA_RWCTRL_ASSERT_ALL_BE
> has changed for bcm570[34] and also advised against setting
> it on BCM570[01] chips. I'm just implementing his advice.
> Comment below spells out more details.
>
> Setting this bit is absolutely required on many RISC PCI boxes, where
> streaming mappings must have cacheline sized DMA transactions done
> on them with all byte enables on.

My gut feeling is if linux aligns or pads things nicely for any reason,
then the bye enables don't get used or clobber padding.

> In fact, since the later chips don't allow controlling this, some of
> them cause streaming byte hole errors on sparc64 and other RISC
> systems when they do cacheline sized DMA to streaming DMA mappings
> with not all the byte enables on.

yup.

> So I'm not going to add this part of your changes.

No problem. My take is, if it hasn't caused a problem on x86 because
things are well aligned, then no reason to change the code.
Knowing the issues about other RISC archs

Maybe keep a shorter note about the bit changed meaning in later models
just to document the issues.

thanks,
grant

2004-01-24 07:41:24

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] 2.6.1 tg3 DMA engine test failure

From: Grant Grundler <[email protected]>
Date: Sat, 24 Jan 2004 00:30:32 -0700

My gut feeling is if linux aligns or pads things nicely for any reason,
then the bye enables don't get used or clobber padding.

If the packet data length is an odd number of bytes, there is nothing
we can do about this, and the newer tigon3 chips are going to use a
cacheline burst for the end of the packet with the trailing byte
enables turned off. I've seen this myself and sparc64 PCI controllers
generate a streaming byte hole error interrupt when it occurs and I
get messages logged in dmesg :)

Maybe keep a shorter note about the bit changed meaning in later models
just to document the issues.

We can "document it" by having the setting of this bit be protected by
chip version numbers. I'd happily accept such a patch.

2004-01-24 21:15:45

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] 2.6.1 tg3 DMA engine test failure

David,

There were two separate components to Grant's patch (hint ggg... split
up your patches).

What do you think about GRC-resets-sub-components part?

That appears valid (and probably wise) to me, but correct me if I'm wrong...

Jeff




2004-01-24 21:24:21

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] 2.6.1 tg3 DMA engine test failure

From: Jeff Garzik <[email protected]>
Date: Sat, 24 Jan 2004 16:15:29 -0500

There were two separate components to Grant's patch (hint ggg... split
up your patches).

What do you think about GRC-resets-sub-components part?

That appears valid (and probably wise) to me, but correct me if I'm wrong...

I know, I tried to give the impression that I was fine with Grant's
patch besides the ALL_BE bit part.

I'll take care of further reviewing and merging this around Jeff.

2004-01-25 01:49:06

by Grant Grundler

[permalink] [raw]
Subject: Re: [PATCH] 2.6.1 tg3 DMA engine test failure

On Sat, Jan 24, 2004 at 04:15:29PM -0500, Jeff Garzik wrote:
> David,
>
> There were two separate components to Grant's patch (hint ggg... split
> up your patches).

you are right - sorry.
I'll break it down when I submit patches for RHEL3/ia64.
(Actually, I'll only submit the changes david accepted).

I had already rejected some other issues broadcom wanted me to address.


> What do you think about GRC-resets-sub-components part?
> That appears valid (and probably wise) to me, but correct me if I'm wrong...

BTW, next on the horizon is removing FTQ reset.
I'm told the FTQ reset is NOT performed by the Tru64 (Alpha/Unix) driver
and Broadcom is testing that with the next release of bcm5700 now.

thanks,
grant

2004-01-25 01:53:45

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] 2.6.1 tg3 DMA engine test failure

Grant Grundler wrote:
> On Sat, Jan 24, 2004 at 04:15:29PM -0500, Jeff Garzik wrote:
>
>>David,
>>
>>There were two separate components to Grant's patch (hint ggg... split
>>up your patches).
>
>
> you are right - sorry.
> I'll break it down when I submit patches for RHEL3/ia64.
> (Actually, I'll only submit the changes david accepted).
>
> I had already rejected some other issues broadcom wanted me to address.

Hey, feel free to address as many issues as you would like! :)


>>What do you think about GRC-resets-sub-components part?
>>That appears valid (and probably wise) to me, but correct me if I'm wrong...
>
>
> BTW, next on the horizon is removing FTQ reset.
> I'm told the FTQ reset is NOT performed by the Tru64 (Alpha/Unix) driver
> and Broadcom is testing that with the next release of bcm5700 now.

We just went through this with Broadcom, when David applied fixes
related to ASF... rather than what Broadcom is _testing_, though, I'm
more interested to know if GRC resets FTQ's according to the hardware
engineers?

Jeff



2004-01-25 02:13:14

by Grant Grundler

[permalink] [raw]
Subject: Re: [PATCH] 2.6.1 tg3 DMA engine test failure

On Fri, Jan 23, 2004 at 11:32:41PM -0800, David S. Miller wrote:
> From: Grant Grundler <[email protected]>
> Date: Sat, 24 Jan 2004 00:30:32 -0700
>
> My gut feeling is if linux aligns or pads things nicely for any reason,
> then the bye enables don't get used or clobber padding.
>
> If the packet data length is an odd number of bytes, there is nothing
> we can do about this, and the newer tigon3 chips are going to use a
> cacheline burst for the end of the packet with the trailing byte
> enables turned off. I've seen this myself and sparc64 PCI controllers
> generate a streaming byte hole error interrupt when it occurs and I
> get messages logged in dmesg :)

Ugh. Ok.

I was concerned about about the buffer not ending on a well aligned
boundary and someone elses data getting clobbered. But RX buffers are
managed by the OS and I expect them to be well aligned.

> Maybe keep a shorter note about the bit changed meaning in later models
> just to document the issues.
>
> We can "document it" by having the setting of this bit be protected by
> chip version numbers. I'd happily accept such a patch.

Well, you pointed out it needs to be set on bcm5700/01 for it's
intended purpose. And it needs to be set on 5703/04 to enable
PCI-X bug workaround. BE bit "always" needs to be set.
Did you intend to alias the BE constant to another name?
(and then use respective chip version for each constant)

BTW, I don't know what the story is with bcm5705.

thanks,
grant

2004-01-25 05:31:08

by Grant Grundler

[permalink] [raw]
Subject: Re: [PATCH] 2.6.1 tg3 DMA engine test failure

On Sat, Jan 24, 2004 at 08:53:29PM -0500, Jeff Garzik wrote:
> Hey, feel free to address as many issues as you would like! :)

:^)

I'll try to keep it at "One at a Time".
I muddy the waters enough as it is.


> >BTW, next on the horizon is removing FTQ reset.
> >I'm told the FTQ reset is NOT performed by the Tru64 (Alpha/Unix) driver
> >and Broadcom is testing that with the next release of bcm5700 now.
>
> We just went through this with Broadcom, when David applied fixes
> related to ASF... rather than what Broadcom is _testing_, though, I'm
> more interested to know if GRC resets FTQ's according to the hardware
> engineers?

email from "Wed Jan 21" says:
"FTQ stands for Flow Through Queue and they are used to connect different
state machines. It turns out that it should also be unnecessary to reset
the FTQs as they get reset during GRC reset. While the FTQ reset itself
is harmless, we recently discovered that it created a race condition
with ASF firmware...."

I don't know more than that. "should also" probably isn't as
conclusive as you would like and it's now third hand.
But you probably know who to ask at Broadcom...

hth,
grant

2004-01-25 07:20:30

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] 2.6.1 tg3 DMA engine test failure

From: Grant Grundler <[email protected]>
Date: Sat, 24 Jan 2004 22:31:01 -0700

email from "Wed Jan 21" says:
"FTQ stands for Flow Through Queue and they are used to connect different
state machines. It turns out that it should also be unnecessary to reset
the FTQs as they get reset during GRC reset. While the FTQ reset itself
is harmless, we recently discovered that it created a race condition
with ASF firmware...."

A fix for this mentioned ASF race is in the current 2.4.x
and 2.6.x tg3 drivers.

2004-01-26 08:01:56

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] 2.6.1 tg3 DMA engine test failure


Ok, here is the final patch I added to my tree.

Thanks again Grant.

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.1519 -> 1.1520
# drivers/net/tg3.c 1.120 -> 1.121
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/01/25 [email protected] 1.1520
# [TG3]: Fix DMA test failures.
#
# 1) Do not reset subcomponents. GRC reset takes care of it.
# 2) Improve test by verifying the data on the card too.
# 3) Do not set DMA_RWCTRL_ASSERT_ALL_BE on chips where the
# meaning of this bit is different.
# --------------------------------------------
#
diff -Nru a/drivers/net/tg3.c b/drivers/net/tg3.c
--- a/drivers/net/tg3.c Sun Jan 25 23:57:37 2004
+++ b/drivers/net/tg3.c Sun Jan 25 23:57:37 2004
@@ -7190,26 +7190,33 @@
test_desc.addr_lo = buf_dma & 0xffffffff;
test_desc.nic_mbuf = 0x00002100;
test_desc.len = size;
+
+ /*
+ * HP ZX1 was seeing test failures for 5701 cards running at 33Mhz
+ * the *second* time the tg3 driver was getting loaded after an
+ * initial scan.
+ *
+ * Broadcom tells me:
+ * ...the DMA engine is connected to the GRC block and a DMA
+ * reset may affect the GRC block in some unpredictable way...
+ * The behavior of resets to individual blocks has not been tested.
+ *
+ * Broadcom noted the GRC reset will also reset all sub-components.
+ */
if (to_device) {
test_desc.cqid_sqid = (13 << 8) | 2;
- tw32(RDMAC_MODE, RDMAC_MODE_RESET);
- tr32(RDMAC_MODE);
- udelay(40);

tw32(RDMAC_MODE, RDMAC_MODE_ENABLE);
tr32(RDMAC_MODE);
udelay(40);
} else {
test_desc.cqid_sqid = (16 << 8) | 7;
- tw32(WDMAC_MODE, WDMAC_MODE_RESET);
- tr32(WDMAC_MODE);
- udelay(40);

tw32(WDMAC_MODE, WDMAC_MODE_ENABLE);
tr32(WDMAC_MODE);
udelay(40);
}
- test_desc.flags = 0x00000004;
+ test_desc.flags = 0x00000005;

for (i = 0; i < (sizeof(test_desc) / sizeof(u32)); i++) {
u32 val;
@@ -7368,9 +7375,19 @@
GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5701) {
/* Remove this if it causes problems for some boards. */
tp->dma_rwctrl |= DMA_RWCTRL_USE_MEM_READ_MULT;
- }

- tp->dma_rwctrl |= DMA_RWCTRL_ASSERT_ALL_BE;
+ /* On 5700/5701 chips, we need to set this bit.
+ * Otherwise the chip will issue cacheline transactions
+ * to streamable DMA memory with not all the byte
+ * enables turned on. This is an error on several
+ * RISC PCI controllers, in particular sparc64.
+ *
+ * On 5703/5704 chips, this bit has been reassigned
+ * a different meaning. In particular, it is used
+ * on those chips to enable a PCI-X workaround.
+ */
+ tp->dma_rwctrl |= DMA_RWCTRL_ASSERT_ALL_BE;
+ }

tw32(TG3PCI_DMA_RW_CTRL, tp->dma_rwctrl);

@@ -7385,28 +7402,38 @@
goto out;

while (1) {
- u32 *p, i;
+ u32 *p = buf, i;

- p = buf;
for (i = 0; i < TEST_BUFFER_SIZE / sizeof(u32); i++)
p[i] = i;

/* Send the buffer to the chip. */
ret = tg3_do_test_dma(tp, buf, buf_dma, TEST_BUFFER_SIZE, 1);
- if (ret)
+ if (ret) {
+ printk(KERN_ERR "tg3_test_dma() Write the buffer failed %d\n", ret);
break;
+ }

- p = buf;
- for (i = 0; i < TEST_BUFFER_SIZE / sizeof(u32); i++)
+ /* validate data reached card RAM correctly. */
+ for (i = 0; i < TEST_BUFFER_SIZE / sizeof(u32); i++) {
+ u32 val;
+ tg3_read_mem(tp, 0x2100 + (i*4), &val);
+ if (val != p[i]) {
+ printk( KERN_ERR " tg3_test_dma() Card buffer currupted on write! (%d != %d)\n", val, i);
+ /* ret = -ENODEV here? */
+ }
p[i] = 0;
+ }

/* Now read it back. */
ret = tg3_do_test_dma(tp, buf, buf_dma, TEST_BUFFER_SIZE, 0);
- if (ret)
+ if (ret) {
+ printk(KERN_ERR "tg3_test_dma() Read the buffer failed %d\n", ret);
+
break;
+ }

/* Verify it. */
- p = buf;
for (i = 0; i < TEST_BUFFER_SIZE / sizeof(u32); i++) {
if (p[i] == i)
continue;
@@ -7417,6 +7444,7 @@
tw32(TG3PCI_DMA_RW_CTRL, tp->dma_rwctrl);
break;
} else {
+ printk(KERN_ERR "tg3_test_dma() buffer corrupted on read back! (%d != %d)\n", p[i], i);
ret = -ENODEV;
goto out;
}