Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver
On Tue, Sep 19, 2006 at 02:54:33PM +1000, Stephen Rothwell wrote: On powerpc and ppc, insl_ns and insl are identical as are outsl_ns and outsl, so remove the conditional use of insl_ns and outsl_ns. The rest of this patch might indeed be correct, but the above comment bothers me. The ns versions of routines are supposed to be non-byte-swapped versions of the insl/outsl routines (which would byte-swap on big-endian archs such as powerpc.) diff --git a/drivers/net/3c509.c b/drivers/net/3c509.c index cbdae54..add6381 100644 --- a/drivers/net/3c509.c +++ b/drivers/net/3c509.c @@ -879,11 +879,7 @@ #endif outw(skb-len, ioaddr + TX_FIFO); outw(0x00, ioaddr + TX_FIFO); /* ... and the packet rounded to a doubleword. */ -#ifdef __powerpc__ - outsl_ns(ioaddr + TX_FIFO, skb-data, (skb-len + 3) 2); -#else outsl(ioaddr + TX_FIFO, skb-data, (skb-len + 3) 2); -#endif Dohh, a hack like this to work around some possbile byte-swapping bug should never have been done in the first place :-( However, I presume someone added the __powerpc__ define here because they picked up a 3c509 at a garage sale, stuck it in a powerpc, found out it didn't work due to a byte-swapping bug, and then patched it as above. I'm disturbed that somehow outsl_ns() became identical to outsl() at some point, presumably breaking this patch. --linas - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver
On Tue, Sep 19, 2006 at 08:52:18PM +0200, Matt Sealey wrote: [...] Linas Vepstas wrote: On Tue, Sep 19, 2006 at 02:54:33PM +1000, Stephen Rothwell wrote: On powerpc and ppc, insl_ns and insl are identical as are outsl_ns and outsl, so remove the conditional use of insl_ns and outsl_ns. The rest of this patch might indeed be correct, but the above comment bothers me. The ns versions of routines are supposed to be non-byte-swapped versions of the insl/outsl routines (which would Never mind. Silly me don't know my history. asm-powerpc/io.h clearly states * The *_ns versions below don't do byte-swapping. * Neither do the standard versions now, these are just here * for older code. Seems that the byteswapping machanism was changed a while ago, and no longer handled in this way any more. --linas - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver
However, I presume someone added the __powerpc__ define here because they picked up a 3c509 at a garage sale, stuck it in a powerpc, found out it didn't work due to a byte-swapping bug, and then patched it as above. I'm disturbed that somehow outsl_ns() became identical to outsl() at some point, presumably breaking this patch. The problem is that somebody had the bright idea to implement ppc outsl as byteswapping a lng time ago. This was totally bogus and got fixed, but it looks like this driver holds a remain of that period. Ben. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver
On Tue, 2006-09-19 at 20:52 +0200, Matt Sealey wrote:
Some northbridges and PCI bridges have clever byteswapping in
hardware, maybe this is just an effect of that. In theory depending on
the host bridge, you should pass in big endian data and have it swap or
not swap, not pick that way in the driver, UNLESS your driver expects
bigendian data, in which case on a bigendian platform you can tell it to
write without swapping. Voila, two functions.
It's generally considered pretty bad for a northbridge to try to muck
around with byte order. There are fairly well defined rules to plug a
little endian bus (PCI, ISA, ...) on a big endian machine.
The trick that some people didn't get a while ago is that while
accessors like inw/inl shall return a byteswapped data, string
operations like in insw/insl who are copying from a fifo basically to
memory (and opposite write versions) shall -not- byteswap since the data
isn't interpreted. it's a byte stream. It doesn't have any endian
semantic associated to it until it's actually read back from memory in
which case the appropriate endian swap (if any) has to be used depending
on the endianness and size of a given field read/written.
Since some people didn't get it, in the early days, some BE
architectures like ppc had versions of insw/insl that did byteswap,
which was wrong. The bits in this driver are remains from that era.
Note that to aggravate the problem, it still happens that HW engineers
try to be smart when hooking a 16 bits or 32 bits FIFO to a BE machine
and byteswap it in hardware. This is of course totally bogus but did
happen with IDE controllers typically (I think atari or amiga has one of
these, the Tivo is like that too, and a bunch of embedded things). The
net result is that you have to pump the data fifo using a byteswapping
accessor and you cannot use DMA unless you DMA controller can re-swap
the other way around But lots of HW people still don't get it :)
However the existance of these PCI bridges these days? I haven't seen
one in years, and when I have nobody has ever enabled the magic swappy
thing as it's unreliable and can't always tell how you present the data.
One wishes that there was a ntoh and hton style macro in standard use
for PCI access.. hang on though that jsut wouldn't work would it.
Nah. We have the basic rule that readl/writel are little endian. PowerPC
additionally provides arch specific low level in_{be,le}32 type
accessors with explicit endianness. Or you can also use
cpu_to_le32/le32_to_cpu kind of macros to convert between native and
explicit endianness.
Ben.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver
Linas Vepstas writes: The rest of this patch might indeed be correct, but the above comment bothers me. The ns versions of routines are supposed to be non-byte-swapped versions of the insl/outsl routines (which would byte-swap on big-endian archs such as powerpc.) If it were true that in/outsw and in/outsl were actually used to transfer arrays of 16-bit data items or 32-bit data items to/from an I/O device, I would agree with you, but they aren't. They are universally used to transfer arrays of bytes, with the optimization of doing so 2 or 4 bytes at a time. That is why in/outsw and in/outsl don't (and shouldn't) do byte swapping. Paul. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
