This is the second part of my Xilleon port.
I am sending the full set of patches to [email protected]
which is archived at: http://www.linux-mips.org/archives/
Only the patches that touch generic parts of the kernel are coming
here.
The Xilleon (32bit MIPS SOC) has a write back buffer that seems to
operate on the pci bus and does not get flushed before a read. The
result is that a memory barrier must be done before a read intended to
flush PCI writes.
The second problem is that writes need to be flushed in e100_exec_cmd.
I am not sure exactly what was going wrong, but without this patch
packets in the send queue were not being sent until... Well I don't
know exactly when, but reception of packets and interrupts by other devices on
the PCI bus seemed to get them to be kicked out.
The result was that when pinging from an external host, the round trip
time was usually the same as the ping interval (i.e. one second).
This lead to very poor NFS performance.
With the patch applied the ethernet seems to work flawlessly
Patch against 2.6.14-rc2 from linux-mips.org
Signed-off-by: David Daney <[email protected]>
Memory barriers and write flushing added for use with xilleon port.
---
commit 8817d129d5d5fc662858925aa39ddda0cb3b73a0
tree bfc8ec97ad24b9477919f861cc29fc396258dc5f
parent 7c598df35a43f53dd6704bb2490f82fcd1e28a9a
author David Daney <[email protected]> Tue, 04 Oct 2005 13:11:51 -0700
committer David Daney <[email protected]> Tue, 04 Oct 2005 13:11:51 -0700
drivers/net/e100.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/net/e100.c b/drivers/net/e100.c
--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -584,6 +584,7 @@ static inline void e100_write_flush(stru
{
/* Flush previous PCI writes through intermediate bridges
* by doing a benign read */
+ wmb();
(void)readb(&nic->csr->scb.status);
}
@@ -807,9 +808,13 @@ static inline int e100_exec_cmd(struct n
goto err_unlock;
}
- if(unlikely(cmd != cuc_resume))
+ wmb();
+ if(unlikely(cmd != cuc_resume)) {
writel(dma_addr, &nic->csr->scb.gen_ptr);
+ e100_write_flush(nic);
+ }
writeb(cmd, &nic->csr->scb.cmd_lo);
+ e100_write_flush(nic);
err_unlock:
spin_unlock_irqrestore(&nic->cmd_lock, flags);
On Wed, 2005-10-19 at 22:54 -0700, David Daney wrote:
> This is the second part of my Xilleon port.
>
> I am sending the full set of patches to [email protected]
> which is archived at: http://www.linux-mips.org/archives/
>
> Only the patches that touch generic parts of the kernel are coming
> here.
>
> The Xilleon (32bit MIPS SOC) has a write back buffer that seems to
> operate on the pci bus and does not get flushed before a read. The
> result is that a memory barrier must be done before a read intended to
> flush PCI writes.
this is broken hardware; the real solution is to put that wmb() into the
readl() function, as opposed to patching half the kernel for this!
And the second problem seems to be an reodering, which is also not quite
allowed. That also needs fixing, probably in the writel/writeb() macros.
On 10/19/05, David Daney <[email protected]> wrote:
> @@ -584,6 +584,7 @@ static inline void e100_write_flush(stru
> {
> /* Flush previous PCI writes through intermediate bridges
> * by doing a benign read */
> + wmb();
> (void)readb(&nic->csr->scb.status);
> }
I find it odd that this is needed, the readb is meant to flush all
posted writes on the pci bus, if your bus is conforming to pci
specifications, this must succeed. wmb is for host side (processor
memory) writes to complete, and since we're usually only try to force
a writeX command to execute immediately with the readb (otherwise lazy
writes work okay) we shouldn't need a wmb *here*. not to say it might
not be missing somewhere else.
> @@ -807,9 +808,13 @@ static inline int e100_exec_cmd(struct n
> goto err_unlock;
> }
>
> - if(unlikely(cmd != cuc_resume))
> + wmb();
> + if(unlikely(cmd != cuc_resume)) {
> writel(dma_addr, &nic->csr->scb.gen_ptr);
> + e100_write_flush(nic);
> + }
> writeb(cmd, &nic->csr->scb.cmd_lo);
> + e100_write_flush(nic);
wouldn't the last e100_write_flush be all that is needed? e100 only
needs them to come in order, they don't need to be flushed one at a
time.
I can see how this change might be needed in a true write posting environment.
jesse
On Thu, Oct 20, 2005 at 12:01:01PM -0700, Jesse Brandeburg wrote:
> > @@ -584,6 +584,7 @@ static inline void e100_write_flush(stru
> > {
> > /* Flush previous PCI writes through intermediate bridges
> > * by doing a benign read */
> > + wmb();
> > (void)readb(&nic->csr->scb.status);
> > }
>
> I find it odd that this is needed, the readb is meant to flush all
> posted writes on the pci bus, if your bus is conforming to pci
> specifications, this must succeed. wmb is for host side (processor
> memory) writes to complete, and since we're usually only try to force
> a writeX command to execute immediately with the readb (otherwise lazy
> writes work okay) we shouldn't need a wmb *here*. not to say it might
> not be missing somewhere else.
wmb is defined as a sync instruction which will only complete once the
write has actually left the CPU, that is citing the spec "has become
globally visible". Uncached stores such as writeX() may be held in a
writeback buffers potencially infinitely, until this buffer is needed
by another write operation. The real surprise is to see such behaviour
in a modern piece of silicon; the only that I knew of were the R3000-class
processors and that era has ended over a decade ago, so ATI seems to have
done something funny here.
Ralf
Ralf Baechle wrote:
> On Thu, Oct 20, 2005 at 12:01:01PM -0700, Jesse Brandeburg wrote:
>
>
>>>@@ -584,6 +584,7 @@ static inline void e100_write_flush(stru
>>> {
>>> /* Flush previous PCI writes through intermediate bridges
>>> * by doing a benign read */
>>>+ wmb();
>>> (void)readb(&nic->csr->scb.status);
>>> }
>>
>>I find it odd that this is needed, the readb is meant to flush all
>>posted writes on the pci bus, if your bus is conforming to pci
>>specifications, this must succeed. wmb is for host side (processor
>>memory) writes to complete, and since we're usually only try to force
>>a writeX command to execute immediately with the readb (otherwise lazy
>>writes work okay) we shouldn't need a wmb *here*. not to say it might
>>not be missing somewhere else.
>
>
> wmb is defined as a sync instruction which will only complete once the
> write has actually left the CPU, that is citing the spec "has become
> globally visible". Uncached stores such as writeX() may be held in a
> writeback buffers potencially infinitely, until this buffer is needed
> by another write operation. The real surprise is to see such behaviour
> in a modern piece of silicon; the only that I knew of were the R3000-class
> processors and that era has ended over a decade ago, so ATI seems to have
> done something funny here.
In light of all the comments, and:
1) the fact that the drivers for the e100 in the 2.4.30 kernel
distribution work well.
2) other pci drivers work well with this port (usb/ohci, net/8139too).
3) the properties of the write back buffer are not well documented.
I am going to take a more detailed look at trying to fix this problem in
a less intrusive manner.
David Daney.